[PATCH v3 0/2] Add reaction control in rti

[PATCH v3 0/2] Add reaction control in rti

[Judith Mendez]

This allows for reaction control in rti driver. Since AM62L SoC [0]
does not have WWD reset output routed to a ESM module like all other
K3 SoC's and has a reset signal routed to the reset HW block, add a new
compatible for AM62L and configure reset reaction for AM62L SoC instead
of NMI.

This patch has been tested on AM62L EVM [1].

Changes since v2:
- Pick Krzysztof's tag
- Take Andrew's sugestions from v2 for patch 2/2
 -> switch to device_get_match_data
 -> fix comment and assignment of reaction variable logic in rti_wdt_start

v2: https://lore.kernel.org/linux-devicetree/20250625143338.2381726-1-jm@ti.com/
v1-resend: https://lore.kernel.org/linux-devicetree/20250624202605.1333645-1-jm@ti.com/
v1: https://lore.kernel.org/linux-devicetree/20250624194509.1314095-1-jm@ti.com/

[0] https://www.ti.com/product/AM62L
[1] https://www.ti.com/tool/TMDS62LEVM

Judith Mendez (2):
  dt-bindings: watchdog: ti,rti-wdt: Add ti,am62l-rti-wdt compatible
  watchdog: rti_wdt: Add reaction control

 .../bindings/watchdog/ti,rti-wdt.yaml         |  1 +
 drivers/watchdog/rti_wdt.c                    | 32 ++++++++++++++++---
 2 files changed, 29 insertions(+), 4 deletions(-)

-- 
2.49.0

[PATCH v3 1/2] dt-bindings: watchdog: ti,rti-wdt: Add ti,am62l-rti-wdt compatible

[Judith Mendez]

Add a new compatible ti,am62l-rti-wdt for am62l SoC [0].

[0] https://www.ti.com/product/AM62L
Signed-off-by: Judith Mendez <jm@ti.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
index 62ddc284a524..2966e5bfb6c0 100644
--- a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
@@ -23,6 +23,7 @@ allOf:
 properties:
   compatible:
     enum:
+      - ti,am62l-rti-wdt
       - ti,j7-rti-wdt
 
   reg:
-- 
2.49.0

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

[Judith Mendez]

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 */
+	if (wdt->data->reset)
+		reaction = RTIWWDRXN_RST;
+	else
+		reaction = RTIWWDRXN_NMI;
+
+	writel_relaxed(reaction, wdt->base + RTIWWDRXCTRL);
 
 	/* Open window size 50%; this is the largest window size available */
 	writel_relaxed(RTIWWDSIZE_50P, wdt->base + RTIWWDSIZECTRL);
@@ -255,6 +268,8 @@ static int rti_wdt_probe(struct platform_device *pdev)
 	wdd->timeout = DEFAULT_HEARTBEAT;
 	wdd->parent = dev;
 
+	wdt->data = device_get_match_data(dev);
+
 	watchdog_set_drvdata(wdd, wdt);
 	watchdog_set_nowayout(wdd, 1);
 	watchdog_set_restart_priority(wdd, 128);
@@ -369,8 +384,17 @@ static void rti_wdt_remove(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 }
 
+static struct rti_wdt_data j7_wdt = {
+	.reset = false,
+};
+
+static struct rti_wdt_data am62l_wdt = {
+	.reset = true,
+};
+
 static const struct of_device_id rti_wdt_of_match[] = {
-	{ .compatible = "ti,j7-rti-wdt", },
+	{ .compatible = "ti,j7-rti-wdt", .data = &j7_wdt },
+	{ .compatible = "ti,am62l-rti-wdt", .data = &am62l_wdt },
 	{},
 };
 MODULE_DEVICE_TABLE(of, rti_wdt_of_match);
-- 
2.49.0

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

[Guenter Roeck]

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" ?

Guenter

> +	if (wdt->data->reset)
> +		reaction = RTIWWDRXN_RST;
> +	else
> +		reaction = RTIWWDRXN_NMI;
> +
> +	writel_relaxed(reaction, wdt->base + RTIWWDRXCTRL);
>  
>  	/* Open window size 50%; this is the largest window size available */
>  	writel_relaxed(RTIWWDSIZE_50P, wdt->base + RTIWWDSIZECTRL);
> @@ -255,6 +268,8 @@ static int rti_wdt_probe(struct platform_device *pdev)
>  	wdd->timeout = DEFAULT_HEARTBEAT;
>  	wdd->parent = dev;
>  
> +	wdt->data = device_get_match_data(dev);
> +
>  	watchdog_set_drvdata(wdd, wdt);
>  	watchdog_set_nowayout(wdd, 1);
>  	watchdog_set_restart_priority(wdd, 128);
> @@ -369,8 +384,17 @@ static void rti_wdt_remove(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  }
>  
> +static struct rti_wdt_data j7_wdt = {
> +	.reset = false,
> +};
> +
> +static struct rti_wdt_data am62l_wdt = {
> +	.reset = true,
> +};
> +
>  static const struct of_device_id rti_wdt_of_match[] = {
> -	{ .compatible = "ti,j7-rti-wdt", },
> +	{ .compatible = "ti,j7-rti-wdt", .data = &j7_wdt },
> +	{ .compatible = "ti,am62l-rti-wdt", .data = &am62l_wdt },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, rti_wdt_of_match);
> -- 
> 2.49.0
> 

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

[Andrew Davis]

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 */

Andrew

> Guenter
> 
>> +	if (wdt->data->reset)
>> +		reaction = RTIWWDRXN_RST;
>> +	else
>> +		reaction = RTIWWDRXN_NMI;
>> +
>> +	writel_relaxed(reaction, wdt->base + RTIWWDRXCTRL);
>>   
>>   	/* Open window size 50%; this is the largest window size available */
>>   	writel_relaxed(RTIWWDSIZE_50P, wdt->base + RTIWWDSIZECTRL);
>> @@ -255,6 +268,8 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>   	wdd->timeout = DEFAULT_HEARTBEAT;
>>   	wdd->parent = dev;
>>   
>> +	wdt->data = device_get_match_data(dev);
>> +
>>   	watchdog_set_drvdata(wdd, wdt);
>>   	watchdog_set_nowayout(wdd, 1);
>>   	watchdog_set_restart_priority(wdd, 128);
>> @@ -369,8 +384,17 @@ static void rti_wdt_remove(struct platform_device *pdev)
>>   	pm_runtime_disable(&pdev->dev);
>>   }
>>   
>> +static struct rti_wdt_data j7_wdt = {
>> +	.reset = false,
>> +};
>> +
>> +static struct rti_wdt_data am62l_wdt = {
>> +	.reset = true,
>> +};
>> +
>>   static const struct of_device_id rti_wdt_of_match[] = {
>> -	{ .compatible = "ti,j7-rti-wdt", },
>> +	{ .compatible = "ti,j7-rti-wdt", .data = &j7_wdt },
>> +	{ .compatible = "ti,am62l-rti-wdt", .data = &am62l_wdt },
>>   	{},
>>   };
>>   MODULE_DEVICE_TABLE(of, rti_wdt_of_match);
>> -- 
>> 2.49.0
>>

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

[Guenter Roeck]

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.

Guenter

> Andrew
> 
> > Guenter
> > 
> > > +	if (wdt->data->reset)
> > > +		reaction = RTIWWDRXN_RST;
> > > +	else
> > > +		reaction = RTIWWDRXN_NMI;
> > > +
> > > +	writel_relaxed(reaction, wdt->base + RTIWWDRXCTRL);
> > >   	/* Open window size 50%; this is the largest window size available */
> > >   	writel_relaxed(RTIWWDSIZE_50P, wdt->base + RTIWWDSIZECTRL);
> > > @@ -255,6 +268,8 @@ static int rti_wdt_probe(struct platform_device *pdev)
> > >   	wdd->timeout = DEFAULT_HEARTBEAT;
> > >   	wdd->parent = dev;
> > > +	wdt->data = device_get_match_data(dev);
> > > +
> > >   	watchdog_set_drvdata(wdd, wdt);
> > >   	watchdog_set_nowayout(wdd, 1);
> > >   	watchdog_set_restart_priority(wdd, 128);
> > > @@ -369,8 +384,17 @@ static void rti_wdt_remove(struct platform_device *pdev)
> > >   	pm_runtime_disable(&pdev->dev);
> > >   }
> > > +static struct rti_wdt_data j7_wdt = {
> > > +	.reset = false,
> > > +};
> > > +
> > > +static struct rti_wdt_data am62l_wdt = {
> > > +	.reset = true,
> > > +};
> > > +
> > >   static const struct of_device_id rti_wdt_of_match[] = {
> > > -	{ .compatible = "ti,j7-rti-wdt", },
> > > +	{ .compatible = "ti,j7-rti-wdt", .data = &j7_wdt },
> > > +	{ .compatible = "ti,am62l-rti-wdt", .data = &am62l_wdt },
> > >   	{},
> > >   };
> > >   MODULE_DEVICE_TABLE(of, rti_wdt_of_match);
> > > -- 
> > > 2.49.0
> > > 

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

[Judith Mendez]

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;

~ Judith

...

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

[Judith Mendez]

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

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

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

[Guenter Roeck]

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 ?

Guenter

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

[Judith Mendez]

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.

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

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

[Andrew Davis]

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.

Andrew

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

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

[Judith Mendez]

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?

  ~ Judith

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

[Andrew Davis]

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.

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.

Andrew

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

[Judith Mendez]

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