[1/2] watchdog: aspeed: Add pre-timeout interrupt support

Enable the pre-timeout interrupt if requested by device property.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/watchdog/aspeed_wdt.c | 53 +++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

On 10/21/22 23:00, Guenter Roeck wrote:
> On 10/21/22 12:39, Eddie James wrote:
>>
>> On 10/21/22 11:56, Guenter Roeck wrote:
>>> On Fri, Oct 21, 2022 at 10:15:58AM -0500, Eddie James wrote:
>>>> Enable the pre-timeout interrupt if requested by device property.
>>>>
>>> I am not inclined to accept this patch without detailed explanation.
>>> Why would it make sense and/or be desirable to completely bypass the
>>> watchdog core with this pretimeout support ?
>>
>>
>> Sorry, I should add more detail.
>>
>> It doesn't necessarily bypass the watchdog core. It can, if you 
>> specify reset-type="none". But if not, the watchdog will still fire 
>> at the appropriate time.
>>
>> The purpose is to get a stack dump from a kernel panic rather than a 
>> hard reset from the watchdog. The interrupt will fire a certain 
>> number of microseconds (configurable by dts property) before the 
>> watchdog does. The interrupt handler then panics, and all the CPU 
>> stacks are dumped, so hopefully you can catch where another processor 
>> was stuck.
>>
>>
>> I can submit v2 with this information in the commit message and/or 
>> comments.
>>
>
> You did not answer the question why you do not use the pretimeout 
> functionality
> supported by the watchdog core.


I misunderstood your question and I wasn't actually aware of the 
pretimeout support in the core. I have sent v2 using the core pretimeout.


Thanks,

Eddie


>
> Guenter
>
>> Thanks,
>>
>> Eddie
>>
>>
>>>
>>> Thanks,
>>> Guenter
>>>
>>>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>>>> ---
>>>>   drivers/watchdog/aspeed_wdt.c | 53 
>>>> +++++++++++++++++++++++++++++++++--
>>>>   1 file changed, 51 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/aspeed_wdt.c 
>>>> b/drivers/watchdog/aspeed_wdt.c
>>>> index 0cff2adfbfc9..8e12181a827e 100644
>>>> --- a/drivers/watchdog/aspeed_wdt.c
>>>> +++ b/drivers/watchdog/aspeed_wdt.c
>>>> @@ -5,11 +5,14 @@
>>>>    * Joel Stanley <joel@jms.id.au>
>>>>    */
>>>> +#include <linux/bits.h>
>>>>   #include <linux/delay.h>
>>>> +#include <linux/interrupt.h>
>>>>   #include <linux/io.h>
>>>>   #include <linux/kernel.h>
>>>>   #include <linux/module.h>
>>>>   #include <linux/of.h>
>>>> +#include <linux/of_irq.h>
>>>>   #include <linux/platform_device.h>
>>>>   #include <linux/watchdog.h>
>>>> @@ -26,20 +29,32 @@ struct aspeed_wdt {
>>>>   struct aspeed_wdt_config {
>>>>       u32 ext_pulse_width_mask;
>>>> +    u32 irq_shift;
>>>> +    u32 irq_mask;
>>>>   };
>>>>   static const struct aspeed_wdt_config ast2400_config = {
>>>>       .ext_pulse_width_mask = 0xff,
>>>> +    .irq_shift = 0,
>>>> +    .irq_mask = 0,
>>>>   };
>>>>   static const struct aspeed_wdt_config ast2500_config = {
>>>>       .ext_pulse_width_mask = 0xfffff,
>>>> +    .irq_shift = 12,
>>>> +    .irq_mask = GENMASK(31, 12),
>>>> +};
>>>> +
>>>> +static const struct aspeed_wdt_config ast2600_config = {
>>>> +    .ext_pulse_width_mask = 0xfffff,
>>>> +    .irq_shift = 0,
>>>> +    .irq_mask = GENMASK(31, 10),
>>>>   };
>>>>   static const struct of_device_id aspeed_wdt_of_table[] = {
>>>>       { .compatible = "aspeed,ast2400-wdt", .data = &ast2400_config },
>>>>       { .compatible = "aspeed,ast2500-wdt", .data = &ast2500_config },
>>>> -    { .compatible = "aspeed,ast2600-wdt", .data = &ast2500_config },
>>>> +    { .compatible = "aspeed,ast2600-wdt", .data = &ast2600_config },
>>>>       { },
>>>>   };
>>>>   MODULE_DEVICE_TABLE(of, aspeed_wdt_of_table);
>>>> @@ -58,6 +73,7 @@ MODULE_DEVICE_TABLE(of, aspeed_wdt_of_table);
>>>>   #define   WDT_CTRL_RESET_SYSTEM        BIT(1)
>>>>   #define   WDT_CTRL_ENABLE        BIT(0)
>>>>   #define WDT_TIMEOUT_STATUS    0x10
>>>> +#define   WDT_TIMEOUT_STATUS_IRQ        BIT(2)
>>>>   #define   WDT_TIMEOUT_STATUS_BOOT_SECONDARY    BIT(1)
>>>>   #define WDT_CLEAR_TIMEOUT_STATUS    0x14
>>>>   #define   WDT_CLEAR_TIMEOUT_AND_BOOT_CODE_SELECTION BIT(0)
>>>> @@ -243,6 +259,17 @@ static const struct watchdog_info 
>>>> aspeed_wdt_info = {
>>>>       .identity    = KBUILD_MODNAME,
>>>>   };
>>>> +static irqreturn_t aspeed_wdt_irq(int irq, void *arg)
>>>> +{
>>>> +    struct aspeed_wdt *wdt = arg;
>>>> +    u32 status = readl(wdt->base + WDT_TIMEOUT_STATUS);
>>>> +
>>>> +    if (status & WDT_TIMEOUT_STATUS_IRQ)
>>>> +        panic("Watchdog pre-timeout IRQ");
>>>> +
>>>> +    return IRQ_NONE;
>>>> +}
>>>> +
>>>>   static int aspeed_wdt_probe(struct platform_device *pdev)
>>>>   {
>>>>       struct device *dev = &pdev->dev;
>>>> @@ -253,6 +280,7 @@ static int aspeed_wdt_probe(struct 
>>>> platform_device *pdev)
>>>>       const char *reset_type;
>>>>       u32 duration;
>>>>       u32 status;
>>>> +    u32 timeout = 0;
>>>>       int ret;
>>>>       wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>>>> @@ -291,6 +319,27 @@ static int aspeed_wdt_probe(struct 
>>>> platform_device *pdev)
>>>>       if (of_device_is_compatible(np, "aspeed,ast2400-wdt"))
>>>>           wdt->ctrl = WDT_CTRL_1MHZ_CLK;
>>>> +    if (config->irq_mask) {
>>>> +        if (!of_property_read_u32(np, "aspeed,pre-timeout-irq-us", 
>>>> &timeout) && timeout) {
>>>> +            int irq =  platform_get_irq(pdev, 0);
>>>> +
>>>> +            if (irq < 0) {
>>>> +                dev_warn(dev, "Couldn't find IRQ: %d\n", irq);
>>>> +                timeout = 0;
>>>> +            } else {
>>>> +                ret = devm_request_irq(dev, irq, aspeed_wdt_irq, 
>>>> IRQF_SHARED,
>>>> +                               dev_name(dev), wdt);
>>>> +                if (ret) {
>>>> +                    dev_warn(dev, "Couldn't request IRQ:%d\n", ret);
>>>> +                    timeout = 0;
>>>> +                } else {
>>>> +                    wdt->ctrl |= ((timeout << config->irq_shift) &
>>>> +                              config->irq_mask) | WDT_CTRL_WDT_INTR;
>>>> +                }
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +
>>>>       /*
>>>>        * Control reset on a per-device basis to ensure the
>>>>        * host is not affected by a BMC reboot
>>>> @@ -308,7 +357,7 @@ static int aspeed_wdt_probe(struct 
>>>> platform_device *pdev)
>>>>           else if (!strcmp(reset_type, "system"))
>>>>               wdt->ctrl |= WDT_CTRL_RESET_MODE_FULL_CHIP |
>>>>                        WDT_CTRL_RESET_SYSTEM;
>>>> -        else if (strcmp(reset_type, "none"))
>>>> +        else if (strcmp(reset_type, "none") && !timeout)
>>>>               return -EINVAL;
>>>>       }
>>>>       if (of_property_read_bool(np, "aspeed,external-signal"))
>>>> -- 
>>>> 2.31.1
>>>>
>