linux-kernel - Re: [PATCH v2 2/2] watchdog: rti

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <76622b57-1f37-417c-9d70-7064cc21d2cb@ti.com>
Date: Tue, 1 Jul 2025 17:34:24 -0500
From: Judith Mendez <jm@...com>
To: Andrew Davis <afd@...com>, Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Guenter Roeck <linux@...ck-us.net>, Rob Herring <robh@...nel.org>,
        Krzysztof
 Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>
CC: Vignesh Raghavendra <vigneshr@...com>, Tero Kristo <kristo@...nel.org>,
        <linux-watchdog@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] watchdog: rti_wdt: Add reaction control

On 6/27/25 1:31 PM, Andrew Davis wrote:
> On 6/25/25 9:33 AM, Judith Mendez wrote:
>> This allows to configure reaction between NMI and reset for WWD.
>>
>> On K3 SoC's other than AM62L SoC [0], watchdog reset output is routed
>> to the ESM module which can subsequently route the signal to safety
>> master or SoC reset. On AM62L, the watchdog reset output is routed
>> to the SoC HW reset block. So, add a new compatible for AM62l to add
>> SoC data and configure reaction to reset instead of NMI.
> 
> Should this be something we configure, not selected based on device,
> do we know if all user of AM62L want the device reset on WDT?

I will assume that end user will want watchdog to actually reset
the system if watchdog is enabled.

If they don't use watchdog, they wont care. I believe it should be per
device/SoC.

> 
>>
>> [0] https://www.ti.com/product/AM62L
>> Signed-off-by: Judith Mendez <jm@...com>
>> ---
>> Changes since v1-resend:
>> - no change
>> ---
>>   drivers/watchdog/rti_wdt.c | 31 +++++++++++++++++++++++++++----
>>   1 file changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
>> index d1f9ce4100a8..d419884c86c4 100644
>> --- a/drivers/watchdog/rti_wdt.c
>> +++ b/drivers/watchdog/rti_wdt.c
>> @@ -35,7 +35,8 @@
>>   #define RTIWWDRXCTRL    0xa4
>>   #define RTIWWDSIZECTRL    0xa8
>> -#define RTIWWDRX_NMI    0xa
>> +#define RTIWWDRXN_RST    0x5
>> +#define RTIWWDRXN_NMI    0xa
>>   #define RTIWWDSIZE_50P        0x50
>>   #define RTIWWDSIZE_25P        0x500
>> @@ -63,22 +64,29 @@
>>   static int heartbeat;
>> +struct rti_wdt_data {
>> +    bool reset;
>> +};
>> +
>>   /*
>>    * struct to hold data for each WDT device
>>    * @base - base io address of WD device
>>    * @freq - source clock frequency of WDT
>>    * @wdd  - hold watchdog device as is in WDT core
>> + * @data - hold configuration data
>>    */
>>   struct rti_wdt_device {
>>       void __iomem        *base;
>>       unsigned long        freq;
>>       struct watchdog_device    wdd;
>> +    const struct rti_wdt_data *data;
>>   };
>>   static int rti_wdt_start(struct watchdog_device *wdd)
>>   {
>>       u32 timer_margin;
>>       struct rti_wdt_device *wdt = watchdog_get_drvdata(wdd);
>> +    u8 reaction;
>>       int ret;
>>       ret = pm_runtime_resume_and_get(wdd->parent);
>> @@ -101,8 +109,12 @@ static int rti_wdt_start(struct watchdog_device 
>> *wdd)
>>        */
>>       wdd->min_hw_heartbeat_ms = 520 * wdd->timeout + MAX_HW_ERROR;
>> -    /* Generate NMI when wdt expires */
>> -    writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL);
>> +    /* Generate reset or NMI when timer expires/serviced outside of 
>> window */
>> +    reaction = RTIWWDRXN_NMI;
>> +    if (wdt->data->reset)
>> +        reaction = RTIWWDRXN_RST;
>> +
> 
> Suggest:
> 
> /* Reset device if wdt serviced outside of window or generate NMI if 
> available */
> if (wdt->data->reset)
>      reaction = RTIWWDRXN_RST;
> else
>      reaction = RTIWWDRXN_NMI;

Sure, will fix for v3

> 
>> +    writel_relaxed(reaction, wdt->base + RTIWWDRXCTRL);
>>       /* Open window size 50%; this is the largest window size 
>> available */
>>       writel_relaxed(RTIWWDSIZE_50P, wdt->base + RTIWWDSIZECTRL);
>> @@ -255,6 +267,8 @@ static int rti_wdt_probe(struct platform_device 
>> *pdev)
>>       wdd->timeout = DEFAULT_HEARTBEAT;
>>       wdd->parent = dev;
>> +    wdt->data = of_device_get_match_data(dev);
> 
> You can use device_get_match_data() here.
> 
Sure, will fix for v3

> Andrew
> 
>> +
>>       watchdog_set_drvdata(wdd, wdt);
>>       watchdog_set_nowayout(wdd, 1);
>>       watchdog_set_restart_priority(wdd, 128);
>> @@ -369,8 +383,17 @@ static void rti_wdt_remove(struct platform_device 
>> *pdev)
>>       pm_runtime_disable(&pdev->dev);
>>   }
>> +static struct rti_wdt_data j7_wdt = {
>> +    .reset = false,
>> +};
>> +
>> +static struct rti_wdt_data am62l_wdt = {
>> +    .reset = true,
>> +};
>> +
>>   static const struct of_device_id rti_wdt_of_match[] = {
>> -    { .compatible = "ti,j7-rti-wdt", },
>> +    { .compatible = "ti,j7-rti-wdt", .data = &j7_wdt },
>> +    { .compatible = "ti,am62l-rti-wdt", .data = &am62l_wdt },
>>       {},
>>   };
>>   MODULE_DEVICE_TABLE(of, rti_wdt_of_match);