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

[Judith Mendez <jm@ti.com>]

Hi all,

On 7/17/25 3:10 PM, Andrew Davis wrote:
> On 7/17/25 12:51 PM, Judith Mendez wrote:
>> Hi Andrew,
>>
>> On 7/17/25 11:44 AM, Andrew Davis wrote:
>>> On 7/17/25 10:24 AM, Judith Mendez wrote:
>>>> Hi Guenter,
>>>>
>>>> On 7/16/25 1:50 PM, Guenter Roeck wrote:
>>>>> On 7/10/25 07:08, Judith Mendez wrote:
>>>>>> Hi Guenter, Andrew,
>>>>>>
>>>>>> On 7/7/25 5:55 PM, Guenter Roeck wrote:
>>>>>>> On Mon, Jul 07, 2025 at 04:49:31PM -0500, Andrew Davis wrote:
>>>>>>>> On 7/7/25 3:58 PM, Guenter Roeck wrote:
>>>>>>>>> On Mon, Jul 07, 2025 at 01:00:02PM -0500, 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 d1f9ce4100a8..c9ee443c70af 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,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 */
>>>>>>>>>
>>>>>>>>> Shouldn't that be "or generate NMI if _not_ available" ?
>>>>>>>>>
>>>>>>>>
>>>>>>>> For almost all the K3 devices, the WDT has two selectable 
>>>>>>>> outputs, one resets
>>>>>>>> the device directly, the other is this "NMI" which is wired to 
>>>>>>>> an ESM module
>>>>>>>> which can take other actions (but usually it just also resets 
>>>>>>>> the device).
>>>>>>>> For AM62L that second NMI output is not wired (no ESM module), 
>>>>>>>> so our only
>>>>>>>> choice is to set the WDT to direct reset mode.
>>>>>>>>
>>>>>>>> The wording is a little strange, but the "or generate NMI if 
>>>>>>>> available" meaning
>>>>>>>> if NMI is available, then do that. Reset being the fallback when 
>>>>>>>> _not_ available.
>>>>>>>>
>>>>>>>> Maybe this would work better:
>>>>>>>>
>>>>>>>> /* If WDT is serviced outside of window, generate NMI if 
>>>>>>>> available, or reset device */
>>>>>>>>
>>>>>>>
>>>>>>> The problem is that the code doesn't match the comment. The code 
>>>>>>> checks the
>>>>>>> "reset" flag and requests a reset if available. If doesn't check 
>>>>>>> an "nmi"
>>>>>>> flag.
>>>>>>>
>>>>>>> If the preference is NMI, as your comment suggests, the flag 
>>>>>>> should be named
>>>>>>> "nmi" and be set if NMI is available. That would align the code 
>>>>>>> and the
>>>>>>> comment. Right now both code and comment are misleading, since 
>>>>>>> the presence
>>>>>>> of a reset flag (and setting it to false) suggests that a direct 
>>>>>>> reset is
>>>>>>> not available, and that reset is preferred if available. A reset 
>>>>>>> is the
>>>>>>> normally expected behavior for a watchdog, so the fact that this 
>>>>>>> is _not_
>>>>>>> the case for this watchdog should be made more visible.
>>>>>>
>>>>>>
>>>>>> How about:
>>>>>>
>>>>>>
>>>>>> /* If WWDT serviced outside of window, generate NMI or reset the 
>>>>>> device
>>>>>> if NMI not available */
>>>>>>
>>>>>> if (wdt->data->reset)
>>>>>>      reaction = RTIWWDRXN_RST;
>>>>>> else
>>>>>>      reaction = RTIWWDRXN_NMI;
>>>>>>
>>>>>
>>>>> As I have said before, the problem is the "reset" flag. Its name 
>>>>> suggests that
>>>>> it means "reset is available". That is not what it actually means. 
>>>>> It means
>>>>> "NMI is not available". So I suggested to rename it to "nmi" or 
>>>>> maybe "no_nmi".
>>>>> Please educate me - why is that such a problem to name the flag to 
>>>>> match its
>>>>> meaning ?
>>>>
>>>> wdt->data->reset makes more sense because it shows there is a
>>>> physical line routed to the MAIN RESET HW LOGIC:
>>>>
>>>>  >> if (wdt->data->reset)
>>>>  >>      reaction = RTIWWDRXN_RST;
>>>>  >> else
>>>>  >>      reaction = RTIWWDRXN_NMI;
>>>>
>>>> If there is a direct reset line to MAIN RESET HW logic, then the
>>>> reaction should be reset, if there is no reset line, then generate
>>>> and NMI to ESM.
>>>>
>>>
>>> There is a reset line on all K3 devices, if you did it this way then
>>> all devices would have wdt->data->reset set to true and you wouldn't
>>> need this logic at all. The thing that changes is if NMI/ESM is
>>> available or not, so as Guenter suggests the flag should be called
>>> "nmi" or similar and you switch on that.
>>
>> Looking at the integration spec, I do not see a direct reset line for
>> any device besides am62l, could you confirm that what I am reading
>> is correct please?
>>
> 
> I'm not even finding the direct reset line for AM62L, some of these
> datasheets are lacking the reset routing.

You can only find in integration spec, not any other spec or datasheet.

> 
> Anyway, one thing I did notice in these datasheets is that the
> default value for the RTIWWDRXCTRL register is 0x5 (send reset).
> So even if these other devices do not wire the reset we are still
> changing the default by setting the register to 0xa (NMI), so the
> point would still stand. Setting the value to NMI is a change from
> the default and so should be codded that way: have a NMI flag, set
> to true for all devices that have it, leave false for AM62L.
Fine, will fix with v4.

Thanks for reviewing (:

~ Judith