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

[Judith Mendez]

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@xxxxxx>
> > ---
> >   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