[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fc095373-1171-4718-b492-8a74d03f99ba@ti.com>
Date: Thu, 17 Jul 2025 10:24:07 -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 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@...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.
then the comment could be simplified to:
/* If WWDT serviced outside of window, generate reset or NMI to ESM */
to match the code better if you like.
~ Judith
Powered by blists - more mailing lists