[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fb2cc029-796e-4bc7-a1fa-b43256a86c39@ti.com>
Date: Wed, 16 Jul 2025 13:47:33 -0500
From: Judith Mendez <jm@...com>
To: Guenter Roeck <linux@...ck-us.net>, Andrew Davis <afd@...com>
CC: Wim Van Sebroeck <wim@...ux-watchdog.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
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 v3 2/2] watchdog: rti_wdt: Add reaction control
Hi all,
On 7/10/25 9:08 AM, 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@...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;
Since there is no response, I assume no one has an issue with the above
comment, so will respin the series with that change.
~ Judith
Powered by blists - more mailing lists