Re: [PATCH RESEND v4 2/2] watchdog: rti_wdt: Add reaction control

[Judith Mendez <jm@ti.com>]

Hi Andrew,

On 2/3/26 1:13 PM, Andrew Davis wrote:
> On 2/3/26 12:19 PM, 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.
>>
>> [0] https://www.ti.com/product/AM62L
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>>   drivers/watchdog/rti_wdt.c | 32 ++++++++++++++++++++++++++++----
>>   1 file changed, 28 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
>> index be7d7db475911..483e22efedf9b 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 nmi;
>> +};
>> +
>>   /*
>>    * 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;
> 
> Don't store the whole device match data struct, just store the
> information you need, so
> 
> * @nmi - Set to true if this WDT instance supports generating NMI
> bool nmi;
> 
>>   };
>>   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,13 @@ 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);
>> +    /* Reset device if wdt serviced outside of window or generate NMI 
>> if available */
> 
> I think where we had issues before was this comment, it should match the 
> code
> and it can do so while being very similar to the comment as it was before:
> 
> /* When WDT expires, generate an NMI or reset the device if NMI is not 
> supported */

oops, sorry, will fix... again

> 
>> +    if (wdt->data->nmi)
>> +        reaction = RTIWWDRXN_NMI;
>> +    else
>> +        reaction = RTIWWDRXN_RST;
>> +
>> +    writel_relaxed(reaction, wdt->base + RTIWWDRXCTRL);
>>       /* Open window size 50%; this is the largest window size 
>> available */
>>       writel_relaxed(RTIWWDSIZE_50P, wdt->base + RTIWWDSIZECTRL);
>> @@ -254,6 +267,8 @@ static int rti_wdt_probe(struct platform_device 
>> *pdev)
>>       wdd->timeout = DEFAULT_HEARTBEAT;
>>       wdd->parent = dev;
>> +    wdt->data = device_get_match_data(dev);
> 
> Should be rare, but this can fail to find match data so you should
> check for NULL here. Then as above, just extract out the data you
> need (nmi).
> 

Thanks for reviewing!
I will fix accordingly and respin the series.

~ Judith