[PATCH v2 0/3] Suspending i.MX watchdog in WAIT mode

[PATCH v2 0/3] Suspending i.MX watchdog in WAIT mode

[Andrej Picej]

The i.MX6 watchdog can't be stopped once started. Additionally, watchdog
hardware configuration needs to be able to handle low-power modes of the
SoC. For low-power modes, there are two configuration bits in the TRM:
- WDZST bit disables the watchdog timer in "deeper" low power modes and
- WDW bit disables the watchdog timer in "WAIT" mode

WDZST bit support is already in place since 1a9c5efa576e ("watchdog: imx2_wdt: disable watchdog timer during low power mode").
On the other hand, handling of WDZST bit was omitted so far but now
these patch series bring support for it.

SoC's "WAIT" low-power mode corresponds to Linux's freeze or
Suspend-to-Idle (S0) mode which can be activated with:

$ echo freeze > /sys/power/state

Without these patches, board would be reset by the watchdog after
timeout of 128 seconds since watchdog would not be stopped when SoC
entered Suspend-to-Idle mode. With patches in place, boards using
imx2-wdt are able to stay in Suspend-to-Idle mode indefinitely.

Last but not least, WDW bit is not found on all imx2-wdt supported i.MX
devices, therefore a new device-tree property "fsl,suspend-in-wait" has
been introduced for this.

Here is a v1: https://lore.kernel.org/lkml/20221019111714.1953262-1-andrej.picej@norik.com/

Change log for v2 in the corresponding patches.

Andrej Picej (3):
  watchdog: imx2_wdg: suspend watchdog in WAIT mode
  dt-bindings: watchdog: fsl-imx: document suspend in wait mode
  ARM: dts: imx6ul/ull: suspend i.MX6UL watchdog in wait mode

 .../bindings/watchdog/fsl-imx-wdt.yaml        | 22 +++++++++++
 .../boot/dts/imx6ul-phytec-phycore-som.dtsi   |  4 ++
 drivers/watchdog/imx2_wdt.c                   | 37 +++++++++++++++++++
 3 files changed, 63 insertions(+)

-- 
2.25.1

[PATCH v2 1/3] watchdog: imx2_wdg: suspend watchdog in WAIT mode

[Andrej Picej]

Putting device into the "Suspend-To-Idle" mode causes watchdog to
trigger and reset the board after set watchdog timeout period elapses.

Introduce new device-tree property "fsl,suspend-in-wait" which suspends
watchdog in WAIT mode. This is done by setting WDW bit in WCR
(Watchdog Control Register) Watchdog operation is restored after exiting
WAIT mode as expected. WAIT mode coresponds with Linux's
"Suspend-To-Idle".

Signed-off-by: Andrej Picej <andrej.picej@norik.com>
Reviewed-by: Fabio Estevam <festevam@gmail.com>
---
Changes in v2:
 - validate the property with compatible string, as this functionality
   is not supported by all devices.
---
 drivers/watchdog/imx2_wdt.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index d0c5d47ddede..dd9866c6f1e5 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -35,6 +35,7 @@
 
 #define IMX2_WDT_WCR		0x00		/* Control Register */
 #define IMX2_WDT_WCR_WT		(0xFF << 8)	/* -> Watchdog Timeout Field */
+#define IMX2_WDT_WCR_WDW	BIT(7)		/* -> Watchdog disable for WAIT */
 #define IMX2_WDT_WCR_WDA	BIT(5)		/* -> External Reset WDOG_B */
 #define IMX2_WDT_WCR_SRS	BIT(4)		/* -> Software Reset Signal */
 #define IMX2_WDT_WCR_WRE	BIT(3)		/* -> WDOG Reset Enable */
@@ -67,6 +68,27 @@ struct imx2_wdt_device {
 	bool ext_reset;
 	bool clk_is_on;
 	bool no_ping;
+	bool sleep_wait;
+};
+
+static const char * const wdw_boards[] __initconst = {
+	"fsl,imx25-wdt",
+	"fsl,imx35-wdt",
+	"fsl,imx50-wdt",
+	"fsl,imx51-wdt",
+	"fsl,imx53-wdt",
+	"fsl,imx6q-wdt",
+	"fsl,imx6sl-wdt",
+	"fsl,imx6sll-wdt",
+	"fsl,imx6sx-wdt",
+	"fsl,imx6ul-wdt",
+	"fsl,imx7d-wdt",
+	"fsl,imx8mm-wdt",
+	"fsl,imx8mn-wdt",
+	"fsl,imx8mp-wdt",
+	"fsl,imx8mq-wdt",
+	"fsl,vf610-wdt",
+	NULL
 };
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
@@ -129,6 +151,9 @@ static inline void imx2_wdt_setup(struct watchdog_device *wdog)
 
 	/* Suspend timer in low power mode, write once-only */
 	val |= IMX2_WDT_WCR_WDZST;
+	/* Suspend timer in low power WAIT mode, write once-only */
+	if (wdev->sleep_wait)
+		val |= IMX2_WDT_WCR_WDW;
 	/* Strip the old watchdog Time-Out value */
 	val &= ~IMX2_WDT_WCR_WT;
 	/* Generate internal chip-level reset if WDOG times out */
@@ -313,6 +338,18 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
 
 	wdev->ext_reset = of_property_read_bool(dev->of_node,
 						"fsl,ext-reset-output");
+
+	if (of_property_read_bool(dev->of_node, "fsl,suspend-in-wait"))
+		if (of_device_compatible_match(dev->of_node, wdw_boards))
+			wdev->sleep_wait = 1;
+		else {
+			dev_warn(dev, "Warning: Suspending watchdog during " \
+				"WAIT mode is not supported for this device.\n");
+			wdev->sleep_wait = 0;
+		}
+	else
+		wdev->sleep_wait = 0;
+
 	/*
 	 * The i.MX7D doesn't support low power mode, so we need to ping the watchdog
 	 * during suspend.
-- 
2.25.1

[PATCH v2 2/3] dt-bindings: watchdog: fsl-imx: document suspend in wait mode

[Andrej Picej]

Property "fsl,suspend-in-wait" suspends watchdog in "WAIT" mode which
corresponds to Linux's Suspend-to-Idle S0 mode. If this property is not
set and the device is put into Suspend-to-Idle mode, the watchdog
triggers a reset after 128 seconds.

Signed-off-by: Andrej Picej <andrej.picej@norik.com>
Reviewed-by: Fabio Estevam <festevam@gmail.com>
---
Changes in v2:
 - add a commit message,
 - add a list of devices which support this functionality
---
 .../bindings/watchdog/fsl-imx-wdt.yaml        | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
index fb7695515be1..9289de97859b 100644
--- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
@@ -55,6 +55,28 @@ properties:
       If present, the watchdog device is configured to assert its
       external reset (WDOG_B) instead of issuing a software reset.
 
+  fsl,suspend-in-wait:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: |
+      If present, the watchdog device is suspended in WAIT mode
+      (Suspend-to-Idle). Only supported on following devices:
+        - "fsl,imx25-wdt",
+        - "fsl,imx35-wdt",
+        - "fsl,imx50-wdt",
+        - "fsl,imx51-wdt",
+        - "fsl,imx53-wdt",
+        - "fsl,imx6q-wdt",
+        - "fsl,imx6sl-wdt",
+        - "fsl,imx6sll-wdt",
+        - "fsl,imx6sx-wdt",
+        - "fsl,imx6ul-wdt",
+        - "fsl,imx7d-wdt",
+        - "fsl,imx8mm-wdt",
+        - "fsl,imx8mn-wdt",
+        - "fsl,imx8mp-wdt",
+        - "fsl,imx8mq-wdt",
+        - "fsl,vf610-wdt".
+
 required:
   - compatible
   - interrupts
-- 
2.25.1

[PATCH v2 3/3] ARM: dts: imx6ul/ull: suspend i.MX6UL watchdog in wait mode

[Andrej Picej]

It was discovered that the watchdog triggers when the device is put into
"Suspend-To-Idle"/"freeze" low-power mode. Setting WDW bit disables
watchdog when the device is put into WAIT mode.

Signed-off-by: Andrej Picej <andrej.picej@norik.com>
Reviewed-by: Fabio Estevam <festevam@gmail.com>
---
Changes in v2:
 - no changes
---
 arch/arm/boot/dts/imx6ul-phytec-phycore-som.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/imx6ul-phytec-phycore-som.dtsi b/arch/arm/boot/dts/imx6ul-phytec-phycore-som.dtsi
index 3cddc68917a0..5168ed0ffec3 100644
--- a/arch/arm/boot/dts/imx6ul-phytec-phycore-som.dtsi
+++ b/arch/arm/boot/dts/imx6ul-phytec-phycore-som.dtsi
@@ -102,6 +102,10 @@ &usdhc2 {
 	status = "disabled";
 };
 
+&wdog1 {
+	fsl,suspend-in-wait;
+};
+
 &iomuxc {
 	pinctrl_enet1: enet1grp {
 		fsl,pins = <
-- 
2.25.1

Re: [PATCH v2 1/3] watchdog: imx2_wdg: suspend watchdog in WAIT mode

[Alexander Stein]

Am Dienstag, 25. Oktober 2022, 09:25:31 CEST schrieb Andrej Picej:
> Putting device into the "Suspend-To-Idle" mode causes watchdog to
> trigger and reset the board after set watchdog timeout period elapses.
> 
> Introduce new device-tree property "fsl,suspend-in-wait" which suspends
> watchdog in WAIT mode. This is done by setting WDW bit in WCR
> (Watchdog Control Register) Watchdog operation is restored after exiting
> WAIT mode as expected. WAIT mode coresponds with Linux's
> "Suspend-To-Idle".
> 
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> Reviewed-by: Fabio Estevam <festevam@gmail.com>
> ---
> Changes in v2:
>  - validate the property with compatible string, as this functionality
>    is not supported by all devices.
> ---
>  drivers/watchdog/imx2_wdt.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index d0c5d47ddede..dd9866c6f1e5 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -35,6 +35,7 @@
> 
>  #define IMX2_WDT_WCR		0x00		/* Control 
Register */
>  #define IMX2_WDT_WCR_WT		(0xFF << 8)	/* -> 
Watchdog Timeout Field */
> +#define IMX2_WDT_WCR_WDW	BIT(7)		/* -> Watchdog disable 
for WAIT */
>  #define IMX2_WDT_WCR_WDA	BIT(5)		/* -> External Reset 
WDOG_B */
>  #define IMX2_WDT_WCR_SRS	BIT(4)		/* -> Software Reset 
Signal */
>  #define IMX2_WDT_WCR_WRE	BIT(3)		/* -> WDOG Reset Enable 
*/
> @@ -67,6 +68,27 @@ struct imx2_wdt_device {
>  	bool ext_reset;
>  	bool clk_is_on;
>  	bool no_ping;
> +	bool sleep_wait;
> +};
> +
> +static const char * const wdw_boards[] __initconst = {
> +	"fsl,imx25-wdt",
> +	"fsl,imx35-wdt",
> +	"fsl,imx50-wdt",
> +	"fsl,imx51-wdt",
> +	"fsl,imx53-wdt",
> +	"fsl,imx6q-wdt",
> +	"fsl,imx6sl-wdt",
> +	"fsl,imx6sll-wdt",
> +	"fsl,imx6sx-wdt",
> +	"fsl,imx6ul-wdt",
> +	"fsl,imx7d-wdt",
> +	"fsl,imx8mm-wdt",
> +	"fsl,imx8mn-wdt",
> +	"fsl,imx8mp-wdt",
> +	"fsl,imx8mq-wdt",
> +	"fsl,vf610-wdt",
> +	NULL
>  };

So the models listed in Documentation/devicetree/bindings/watchdog/fsl-imx-
wdt.yaml not supporting this feature are
* fsl,imx21-wdt
* fsl,imx27-wdt
* fsl,imx31-wdt
* fsl,ls1012a-wdt
* fsl,ls1043a-wdt
?

But all models are listed as compatible to fsl,imx21-wdt. So there is 
something wrong here. IMHO this sounds like the compatible list has to be 
split and updated. Depending on that this feature can be detected. Maintaining 
another list seems error prone to me.

Best regards,
Alexander

>  static bool nowayout = WATCHDOG_NOWAYOUT;
> @@ -129,6 +151,9 @@ static inline void imx2_wdt_setup(struct watchdog_device
> *wdog)
> 
>  	/* Suspend timer in low power mode, write once-only */
>  	val |= IMX2_WDT_WCR_WDZST;
> +	/* Suspend timer in low power WAIT mode, write once-only */
> +	if (wdev->sleep_wait)
> +		val |= IMX2_WDT_WCR_WDW;
>  	/* Strip the old watchdog Time-Out value */
>  	val &= ~IMX2_WDT_WCR_WT;
>  	/* Generate internal chip-level reset if WDOG times out */
> @@ -313,6 +338,18 @@ static int __init imx2_wdt_probe(struct platform_device
> *pdev)
> 
>  	wdev->ext_reset = of_property_read_bool(dev->of_node,
>  						"fsl,ext-
reset-output");
> +
> +	if (of_property_read_bool(dev->of_node, "fsl,suspend-in-wait"))
> +		if (of_device_compatible_match(dev->of_node, 
wdw_boards))
> +			wdev->sleep_wait = 1;
> +		else {
> +			dev_warn(dev, "Warning: Suspending watchdog 
during " \
> +				"WAIT mode is not supported for 
this device.\n");
> +			wdev->sleep_wait = 0;
> +		}
> +	else
> +		wdev->sleep_wait = 0;
> +
>  	/*
>  	 * The i.MX7D doesn't support low power mode, so we need to ping 
the
> watchdog * during suspend.

Re: [PATCH v2 1/3] watchdog: imx2_wdg: suspend watchdog in WAIT mode

[Andrej Picej]

Hi Alexander,

On 25. 10. 22 11:38, Alexander Stein wrote:
> Am Dienstag, 25. Oktober 2022, 09:25:31 CEST schrieb Andrej Picej:
>> Putting device into the "Suspend-To-Idle" mode causes watchdog to
>> trigger and reset the board after set watchdog timeout period elapses.
>>
>> Introduce new device-tree property "fsl,suspend-in-wait" which suspends
>> watchdog in WAIT mode. This is done by setting WDW bit in WCR
>> (Watchdog Control Register) Watchdog operation is restored after exiting
>> WAIT mode as expected. WAIT mode coresponds with Linux's
>> "Suspend-To-Idle".
>>
>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>> Reviewed-by: Fabio Estevam <festevam@gmail.com>
>> ---
>> Changes in v2:
>>   - validate the property with compatible string, as this functionality
>>     is not supported by all devices.
>> ---
>>   drivers/watchdog/imx2_wdt.c | 37 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
>> index d0c5d47ddede..dd9866c6f1e5 100644
>> --- a/drivers/watchdog/imx2_wdt.c
>> +++ b/drivers/watchdog/imx2_wdt.c
>> @@ -35,6 +35,7 @@
>>
>>   #define IMX2_WDT_WCR		0x00		/* Control
> Register */
>>   #define IMX2_WDT_WCR_WT		(0xFF << 8)	/* ->
> Watchdog Timeout Field */
>> +#define IMX2_WDT_WCR_WDW	BIT(7)		/* -> Watchdog disable
> for WAIT */
>>   #define IMX2_WDT_WCR_WDA	BIT(5)		/* -> External Reset
> WDOG_B */
>>   #define IMX2_WDT_WCR_SRS	BIT(4)		/* -> Software Reset
> Signal */
>>   #define IMX2_WDT_WCR_WRE	BIT(3)		/* -> WDOG Reset Enable
> */
>> @@ -67,6 +68,27 @@ struct imx2_wdt_device {
>>   	bool ext_reset;
>>   	bool clk_is_on;
>>   	bool no_ping;
>> +	bool sleep_wait;
>> +};
>> +
>> +static const char * const wdw_boards[] __initconst = {
>> +	"fsl,imx25-wdt",
>> +	"fsl,imx35-wdt",
>> +	"fsl,imx50-wdt",
>> +	"fsl,imx51-wdt",
>> +	"fsl,imx53-wdt",
>> +	"fsl,imx6q-wdt",
>> +	"fsl,imx6sl-wdt",
>> +	"fsl,imx6sll-wdt",
>> +	"fsl,imx6sx-wdt",
>> +	"fsl,imx6ul-wdt",
>> +	"fsl,imx7d-wdt",
>> +	"fsl,imx8mm-wdt",
>> +	"fsl,imx8mn-wdt",
>> +	"fsl,imx8mp-wdt",
>> +	"fsl,imx8mq-wdt",
>> +	"fsl,vf610-wdt",
>> +	NULL
>>   };
> 
> So the models listed in Documentation/devicetree/bindings/watchdog/fsl-imx-
> wdt.yaml not supporting this feature are
> * fsl,imx21-wdt
> * fsl,imx27-wdt
> * fsl,imx31-wdt
> * fsl,ls1012a-wdt
> * fsl,ls1043a-wdt
> ?
yes, you are correct.

> 
> But all models are listed as compatible to fsl,imx21-wdt. So there is
> something wrong here. IMHO this sounds like the compatible list has to be
> split and updated. Depending on that this feature can be detected. Maintaining
> another list seems error prone to me.
> 

So basically the compatible lists would be split into two groups, one 
for the devices which support this WDW bit and the rest which don't 
support this?
You got a point here, but...this means that every processors 
device-tree, which has two compatible strings (with "fsl,imx21-wdt") 
should be updated, right? That sounds like quite a lot of changes, which 
I'd like to avoid if possible.

Best regards,
Andrej

> Best regards,
> Alexander
> 
>>   static bool nowayout = WATCHDOG_NOWAYOUT;
>> @@ -129,6 +151,9 @@ static inline void imx2_wdt_setup(struct watchdog_device
>> *wdog)
>>
>>   	/* Suspend timer in low power mode, write once-only */
>>   	val |= IMX2_WDT_WCR_WDZST;
>> +	/* Suspend timer in low power WAIT mode, write once-only */
>> +	if (wdev->sleep_wait)
>> +		val |= IMX2_WDT_WCR_WDW;
>>   	/* Strip the old watchdog Time-Out value */
>>   	val &= ~IMX2_WDT_WCR_WT;
>>   	/* Generate internal chip-level reset if WDOG times out */
>> @@ -313,6 +338,18 @@ static int __init imx2_wdt_probe(struct platform_device
>> *pdev)
>>
>>   	wdev->ext_reset = of_property_read_bool(dev->of_node,
>>   						"fsl,ext-
> reset-output");
>> +
>> +	if (of_property_read_bool(dev->of_node, "fsl,suspend-in-wait"))
>> +		if (of_device_compatible_match(dev->of_node,
> wdw_boards))
>> +			wdev->sleep_wait = 1;
>> +		else {
>> +			dev_warn(dev, "Warning: Suspending watchdog
> during " \
>> +				"WAIT mode is not supported for
> this device.\n");
>> +			wdev->sleep_wait = 0;
>> +		}
>> +	else
>> +		wdev->sleep_wait = 0;
>> +
>>   	/*
>>   	 * The i.MX7D doesn't support low power mode, so we need to ping
> the
>> watchdog * during suspend.
> 
> 
> 
> 

Re: [PATCH v2 1/3] watchdog: imx2_wdg: suspend watchdog in WAIT mode

[Marco Felsch]

On 22-10-25, Andrej Picej wrote:
> Putting device into the "Suspend-To-Idle" mode causes watchdog to
> trigger and reset the board after set watchdog timeout period elapses.
> 
> Introduce new device-tree property "fsl,suspend-in-wait" which suspends
> watchdog in WAIT mode. This is done by setting WDW bit in WCR
> (Watchdog Control Register) Watchdog operation is restored after exiting
> WAIT mode as expected. WAIT mode coresponds with Linux's
> "Suspend-To-Idle".
> 
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> Reviewed-by: Fabio Estevam <festevam@gmail.com>
> ---
> Changes in v2:
>  - validate the property with compatible string, as this functionality
>    is not supported by all devices.
> ---
>  drivers/watchdog/imx2_wdt.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index d0c5d47ddede..dd9866c6f1e5 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -35,6 +35,7 @@
>  
>  #define IMX2_WDT_WCR		0x00		/* Control Register */
>  #define IMX2_WDT_WCR_WT		(0xFF << 8)	/* -> Watchdog Timeout Field */
> +#define IMX2_WDT_WCR_WDW	BIT(7)		/* -> Watchdog disable for WAIT */
>  #define IMX2_WDT_WCR_WDA	BIT(5)		/* -> External Reset WDOG_B */
>  #define IMX2_WDT_WCR_SRS	BIT(4)		/* -> Software Reset Signal */
>  #define IMX2_WDT_WCR_WRE	BIT(3)		/* -> WDOG Reset Enable */
> @@ -67,6 +68,27 @@ struct imx2_wdt_device {
>  	bool ext_reset;
>  	bool clk_is_on;
>  	bool no_ping;
> +	bool sleep_wait;
> +};
> +
> +static const char * const wdw_boards[] __initconst = {
> +	"fsl,imx25-wdt",
> +	"fsl,imx35-wdt",
> +	"fsl,imx50-wdt",
> +	"fsl,imx51-wdt",
> +	"fsl,imx53-wdt",
> +	"fsl,imx6q-wdt",
> +	"fsl,imx6sl-wdt",
> +	"fsl,imx6sll-wdt",
> +	"fsl,imx6sx-wdt",
> +	"fsl,imx6ul-wdt",
> +	"fsl,imx7d-wdt",
> +	"fsl,imx8mm-wdt",
> +	"fsl,imx8mn-wdt",
> +	"fsl,imx8mp-wdt",
> +	"fsl,imx8mq-wdt",
> +	"fsl,vf610-wdt",
> +	NULL
>  };

For such things we have the data pointer within the struct of_device_id.

>  
>  static bool nowayout = WATCHDOG_NOWAYOUT;
> @@ -129,6 +151,9 @@ static inline void imx2_wdt_setup(struct watchdog_device *wdog)
>  
>  	/* Suspend timer in low power mode, write once-only */
>  	val |= IMX2_WDT_WCR_WDZST;
> +	/* Suspend timer in low power WAIT mode, write once-only */
> +	if (wdev->sleep_wait)
> +		val |= IMX2_WDT_WCR_WDW;
>  	/* Strip the old watchdog Time-Out value */
>  	val &= ~IMX2_WDT_WCR_WT;
>  	/* Generate internal chip-level reset if WDOG times out */
> @@ -313,6 +338,18 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
>  
>  	wdev->ext_reset = of_property_read_bool(dev->of_node,
>  						"fsl,ext-reset-output");
> +
> +	if (of_property_read_bool(dev->of_node, "fsl,suspend-in-wait"))

Why do we need this special property? If the device has problems when
"freeze" is used as suspend mode and this is fixed by this special bit
then we should enable it if the device supports it.

Regards,
  Marco


> +		if (of_device_compatible_match(dev->of_node, wdw_boards))
> +			wdev->sleep_wait = 1;
> +		else {
> +			dev_warn(dev, "Warning: Suspending watchdog during " \
> +				"WAIT mode is not supported for this device.\n");
> +			wdev->sleep_wait = 0;
> +		}
> +	else
> +		wdev->sleep_wait = 0;
> +
>  	/*
>  	 * The i.MX7D doesn't support low power mode, so we need to ping the watchdog
>  	 * during suspend.
> -- 
> 2.25.1
> 
> 
> 

Re: [PATCH v2 1/3] watchdog: imx2_wdg: suspend watchdog in WAIT mode

[Andrej Picej]

Hi Marco,

On 25. 10. 22 14:27, Marco Felsch wrote:
> On 22-10-25, Andrej Picej wrote:
>> Putting device into the "Suspend-To-Idle" mode causes watchdog to
>> trigger and reset the board after set watchdog timeout period elapses.
>>
>> Introduce new device-tree property "fsl,suspend-in-wait" which suspends
>> watchdog in WAIT mode. This is done by setting WDW bit in WCR
>> (Watchdog Control Register) Watchdog operation is restored after exiting
>> WAIT mode as expected. WAIT mode coresponds with Linux's
>> "Suspend-To-Idle".
>>
>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>> Reviewed-by: Fabio Estevam <festevam@gmail.com>
>> ---
>> Changes in v2:
>>   - validate the property with compatible string, as this functionality
>>     is not supported by all devices.
>> ---
>>   drivers/watchdog/imx2_wdt.c | 37 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
>> index d0c5d47ddede..dd9866c6f1e5 100644
>> --- a/drivers/watchdog/imx2_wdt.c
>> +++ b/drivers/watchdog/imx2_wdt.c
>> @@ -35,6 +35,7 @@
>>   
>>   #define IMX2_WDT_WCR		0x00		/* Control Register */
>>   #define IMX2_WDT_WCR_WT		(0xFF << 8)	/* -> Watchdog Timeout Field */
>> +#define IMX2_WDT_WCR_WDW	BIT(7)		/* -> Watchdog disable for WAIT */
>>   #define IMX2_WDT_WCR_WDA	BIT(5)		/* -> External Reset WDOG_B */
>>   #define IMX2_WDT_WCR_SRS	BIT(4)		/* -> Software Reset Signal */
>>   #define IMX2_WDT_WCR_WRE	BIT(3)		/* -> WDOG Reset Enable */
>> @@ -67,6 +68,27 @@ struct imx2_wdt_device {
>>   	bool ext_reset;
>>   	bool clk_is_on;
>>   	bool no_ping;
>> +	bool sleep_wait;
>> +};
>> +
>> +static const char * const wdw_boards[] __initconst = {
>> +	"fsl,imx25-wdt",
>> +	"fsl,imx35-wdt",
>> +	"fsl,imx50-wdt",
>> +	"fsl,imx51-wdt",
>> +	"fsl,imx53-wdt",
>> +	"fsl,imx6q-wdt",
>> +	"fsl,imx6sl-wdt",
>> +	"fsl,imx6sll-wdt",
>> +	"fsl,imx6sx-wdt",
>> +	"fsl,imx6ul-wdt",
>> +	"fsl,imx7d-wdt",
>> +	"fsl,imx8mm-wdt",
>> +	"fsl,imx8mn-wdt",
>> +	"fsl,imx8mp-wdt",
>> +	"fsl,imx8mq-wdt",
>> +	"fsl,vf610-wdt",
>> +	NULL
>>   };
> 
> For such things we have the data pointer within the struct of_device_id.
Ok, that might clear it up a bit. Thanks.

> 
>>   
>>   static bool nowayout = WATCHDOG_NOWAYOUT;
>> @@ -129,6 +151,9 @@ static inline void imx2_wdt_setup(struct watchdog_device *wdog)
>>   
>>   	/* Suspend timer in low power mode, write once-only */
>>   	val |= IMX2_WDT_WCR_WDZST;
>> +	/* Suspend timer in low power WAIT mode, write once-only */
>> +	if (wdev->sleep_wait)
>> +		val |= IMX2_WDT_WCR_WDW;
>>   	/* Strip the old watchdog Time-Out value */
>>   	val &= ~IMX2_WDT_WCR_WT;
>>   	/* Generate internal chip-level reset if WDOG times out */
>> @@ -313,6 +338,18 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
>>   
>>   	wdev->ext_reset = of_property_read_bool(dev->of_node,
>>   						"fsl,ext-reset-output");
>> +
>> +	if (of_property_read_bool(dev->of_node, "fsl,suspend-in-wait"))
> 
> Why do we need this special property? If the device has problems when
> "freeze" is used as suspend mode and this is fixed by this special bit
> then we should enable it if the device supports it.

That was our initial plan and it would be the easiest to do. But since 
it looks like nobody experienced this problem so far, we are somehow 
reluctant to set it by default. What if someone is relying on this 
feature to reset the device if the device is not waken up from "freeze" 
by some other interrupt source?

Best regards,
Andrej
> 
> Regards,
>    Marco
> 
> 
>> +		if (of_device_compatible_match(dev->of_node, wdw_boards))
>> +			wdev->sleep_wait = 1;
>> +		else {
>> +			dev_warn(dev, "Warning: Suspending watchdog during " \
>> +				"WAIT mode is not supported for this device.\n");
>> +			wdev->sleep_wait = 0;
>> +		}
>> +	else
>> +		wdev->sleep_wait = 0;
>> +
>>   	/*
>>   	 * The i.MX7D doesn't support low power mode, so we need to ping the watchdog
>>   	 * during suspend.
>> -- 
>> 2.25.1
>>
>>
>>

Re: [PATCH v2 2/3] dt-bindings: watchdog: fsl-imx: document suspend in wait mode

[Krzysztof Kozlowski]

On 25/10/2022 03:25, Andrej Picej wrote:
> Property "fsl,suspend-in-wait" suspends watchdog in "WAIT" mode which
> corresponds to Linux's Suspend-to-Idle S0 mode. If this property is not
> set and the device is put into Suspend-to-Idle mode, the watchdog
> triggers a reset after 128 seconds.
> 
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> Reviewed-by: Fabio Estevam <festevam@gmail.com>
> ---
> Changes in v2:
>  - add a commit message,
>  - add a list of devices which support this functionality
> ---
>  .../bindings/watchdog/fsl-imx-wdt.yaml        | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
> index fb7695515be1..9289de97859b 100644
> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
> @@ -55,6 +55,28 @@ properties:
>        If present, the watchdog device is configured to assert its
>        external reset (WDOG_B) instead of issuing a software reset.
>  
> +  fsl,suspend-in-wait:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: |
> +      If present, the watchdog device is suspended in WAIT mode
> +      (Suspend-to-Idle). Only supported on following devices:
> +        - "fsl,imx25-wdt",

You need to define such allow/disallow in allOf:if:then, instead. Like
example-schema is doing for foo-supply, just disallow it for some types
or use "if: not: ..."

Best regards,
Krzysztof

Re: [PATCH v2 1/3] watchdog: imx2_wdg: suspend watchdog in WAIT mode

[Guenter Roeck]

On 10/25/22 00:25, Andrej Picej wrote:
> Putting device into the "Suspend-To-Idle" mode causes watchdog to
> trigger and reset the board after set watchdog timeout period elapses.
> 

s/reset/resets/

> Introduce new device-tree property "fsl,suspend-in-wait" which suspends
> watchdog in WAIT mode. This is done by setting WDW bit in WCR
> (Watchdog Control Register) Watchdog operation is restored after exiting

'.' after ')' missing ?

> WAIT mode as expected. WAIT mode coresponds with Linux's

s/coresponds/corresponds/

> "Suspend-To-Idle".
> 
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> Reviewed-by: Fabio Estevam <festevam@gmail.com>
> ---
> Changes in v2:
>   - validate the property with compatible string, as this functionality
>     is not supported by all devices.
> ---
>   drivers/watchdog/imx2_wdt.c | 37 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index d0c5d47ddede..dd9866c6f1e5 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -35,6 +35,7 @@
>   
>   #define IMX2_WDT_WCR		0x00		/* Control Register */
>   #define IMX2_WDT_WCR_WT		(0xFF << 8)	/* -> Watchdog Timeout Field */
> +#define IMX2_WDT_WCR_WDW	BIT(7)		/* -> Watchdog disable for WAIT */
>   #define IMX2_WDT_WCR_WDA	BIT(5)		/* -> External Reset WDOG_B */
>   #define IMX2_WDT_WCR_SRS	BIT(4)		/* -> Software Reset Signal */
>   #define IMX2_WDT_WCR_WRE	BIT(3)		/* -> WDOG Reset Enable */
> @@ -67,6 +68,27 @@ struct imx2_wdt_device {
>   	bool ext_reset;
>   	bool clk_is_on;
>   	bool no_ping;
> +	bool sleep_wait;
> +};
> +
> +static const char * const wdw_boards[] __initconst = {
> +	"fsl,imx25-wdt",
> +	"fsl,imx35-wdt",
> +	"fsl,imx50-wdt",
> +	"fsl,imx51-wdt",
> +	"fsl,imx53-wdt",
> +	"fsl,imx6q-wdt",
> +	"fsl,imx6sl-wdt",
> +	"fsl,imx6sll-wdt",
> +	"fsl,imx6sx-wdt",
> +	"fsl,imx6ul-wdt",
> +	"fsl,imx7d-wdt",
> +	"fsl,imx8mm-wdt",
> +	"fsl,imx8mn-wdt",
> +	"fsl,imx8mp-wdt",
> +	"fsl,imx8mq-wdt",
> +	"fsl,vf610-wdt",
> +	NULL
>   };
>   
>   static bool nowayout = WATCHDOG_NOWAYOUT;
> @@ -129,6 +151,9 @@ static inline void imx2_wdt_setup(struct watchdog_device *wdog)
>   
>   	/* Suspend timer in low power mode, write once-only */
>   	val |= IMX2_WDT_WCR_WDZST;
> +	/* Suspend timer in low power WAIT mode, write once-only */
> +	if (wdev->sleep_wait)
> +		val |= IMX2_WDT_WCR_WDW;
>   	/* Strip the old watchdog Time-Out value */
>   	val &= ~IMX2_WDT_WCR_WT;
>   	/* Generate internal chip-level reset if WDOG times out */
> @@ -313,6 +338,18 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
>   
>   	wdev->ext_reset = of_property_read_bool(dev->of_node,
>   						"fsl,ext-reset-output");
> +
> +	if (of_property_read_bool(dev->of_node, "fsl,suspend-in-wait"))
> +		if (of_device_compatible_match(dev->of_node, wdw_boards))
> +			wdev->sleep_wait = 1;

Since sleep_wait is bool:
			wdev->sleep_wait = true;

> +		else {
> +			dev_warn(dev, "Warning: Suspending watchdog during " \
> +				"WAIT mode is not supported for this device.\n");

Do not split strings. "Warning:" is redundant. Please handle the error first.

> +			wdev->sleep_wait = 0;

Unnecessary; false by default. Also, this should fail and return -EINVAL.
Devicetree files should be correct, and warning messages tend to be ignored.

> +		}

All branches of if/else need to wither use {} or no {}.

> +	else
> +		wdev->sleep_wait = 0;
> +
Unnecessary.

I would suggest to replace the above code with something like

	if (of_property_read_bool(dev->of_node, "fsl,suspend-in-wait")) {
		if (!of_device_compatible_match(dev->of_node, wdw_boards)) {
			dev_err(dev, "Suspending watchdog in WAIT mode is not supported for this device\n");
			return -EINVAL;
		}
		wdev->sleep_wait = true;
	}

>   	/*
>   	 * The i.MX7D doesn't support low power mode, so we need to ping the watchdog
>   	 * during suspend.

I still wonder how that interacts with fsl,suspend-in-wait, but since we have a
property for that we can leave that for someone else to find out. Maybe add a
comment explaining that interaction with "fsl,suspend-in-wait" is unknown.

Thanks,
Guenter

Re: [PATCH v2 1/3] watchdog: imx2_wdg: suspend watchdog in WAIT mode

[Alexander Stein]

Hello Andrej,

Am Dienstag, 25. Oktober 2022, 13:21:18 CEST schrieb Andrej Picej:
> Hi Alexander,
> 
> On 25. 10. 22 11:38, Alexander Stein wrote:
> > Am Dienstag, 25. Oktober 2022, 09:25:31 CEST schrieb Andrej Picej:
> >> Putting device into the "Suspend-To-Idle" mode causes watchdog to
> >> trigger and reset the board after set watchdog timeout period elapses.
> >> 
> >> Introduce new device-tree property "fsl,suspend-in-wait" which suspends
> >> watchdog in WAIT mode. This is done by setting WDW bit in WCR
> >> (Watchdog Control Register) Watchdog operation is restored after exiting
> >> WAIT mode as expected. WAIT mode coresponds with Linux's
> >> "Suspend-To-Idle".
> >> 
> >> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> >> Reviewed-by: Fabio Estevam <festevam@gmail.com>
> >> ---
> >> 
> >> Changes in v2:
> >>   - validate the property with compatible string, as this functionality
> >>   
> >>     is not supported by all devices.
> >> 
> >> ---
> >> 
> >>   drivers/watchdog/imx2_wdt.c | 37 +++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 37 insertions(+)
> >> 
> >> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> >> index d0c5d47ddede..dd9866c6f1e5 100644
> >> --- a/drivers/watchdog/imx2_wdt.c
> >> +++ b/drivers/watchdog/imx2_wdt.c
> >> @@ -35,6 +35,7 @@
> >> 
> >>   #define IMX2_WDT_WCR		0x00		/* Control
> > 
> > Register */
> > 
> >>   #define IMX2_WDT_WCR_WT		(0xFF << 8)	/* ->
> > 
> > Watchdog Timeout Field */
> > 
> >> +#define IMX2_WDT_WCR_WDW	BIT(7)		/* -> Watchdog disable
> > 
> > for WAIT */
> > 
> >>   #define IMX2_WDT_WCR_WDA	BIT(5)		/* -> External Reset
> > 
> > WDOG_B */
> > 
> >>   #define IMX2_WDT_WCR_SRS	BIT(4)		/* -> Software Reset
> > 
> > Signal */
> > 
> >>   #define IMX2_WDT_WCR_WRE	BIT(3)		/* -> WDOG Reset Enable
> > 
> > */
> > 
> >> @@ -67,6 +68,27 @@ struct imx2_wdt_device {
> >> 
> >>   	bool ext_reset;
> >>   	bool clk_is_on;
> >>   	bool no_ping;
> >> 
> >> +	bool sleep_wait;
> >> +};
> >> +
> >> +static const char * const wdw_boards[] __initconst = {
> >> +	"fsl,imx25-wdt",
> >> +	"fsl,imx35-wdt",
> >> +	"fsl,imx50-wdt",
> >> +	"fsl,imx51-wdt",
> >> +	"fsl,imx53-wdt",
> >> +	"fsl,imx6q-wdt",
> >> +	"fsl,imx6sl-wdt",
> >> +	"fsl,imx6sll-wdt",
> >> +	"fsl,imx6sx-wdt",
> >> +	"fsl,imx6ul-wdt",
> >> +	"fsl,imx7d-wdt",
> >> +	"fsl,imx8mm-wdt",
> >> +	"fsl,imx8mn-wdt",
> >> +	"fsl,imx8mp-wdt",
> >> +	"fsl,imx8mq-wdt",
> >> +	"fsl,vf610-wdt",
> >> +	NULL
> >> 
> >>   };
> > 
> > So the models listed in
> > Documentation/devicetree/bindings/watchdog/fsl-imx-
> > wdt.yaml not supporting this feature are
> > * fsl,imx21-wdt
> > * fsl,imx27-wdt
> > * fsl,imx31-wdt
> > * fsl,ls1012a-wdt
> > * fsl,ls1043a-wdt
> > ?
> 
> yes, you are correct.
> 
> > But all models are listed as compatible to fsl,imx21-wdt. So there is
> > something wrong here. IMHO this sounds like the compatible list has to be
> > split and updated. Depending on that this feature can be detected.
> > Maintaining another list seems error prone to me.
> 
> So basically the compatible lists would be split into two groups, one
> for the devices which support this WDW bit and the rest which don't
> support this?

This was my idea, so only one set has to be maintained.

> You got a point here, but...this means that every processors
> device-tree, which has two compatible strings (with "fsl,imx21-wdt")
> should be updated, right? That sounds like quite a lot of changes, which
> I'd like to avoid if possible.

Well, the compatible list right now doesn't reflect the hardware features/
compatibility correctly, so IMHO it should be fixed.
But apparently Krzysztof is okay having the special property only applicable 
for a specific set of devices. But in this case you will have to maintain two 
sets of device models (bindings + driver) to which WDW applies/does not apply 
to.

Best regards,
Alexander

> Best regards,
> Andrej
> 
> > Best regards,
> > Alexander
> > 
> >>   static bool nowayout = WATCHDOG_NOWAYOUT;
> >> 
> >> @@ -129,6 +151,9 @@ static inline void imx2_wdt_setup(struct
> >> watchdog_device *wdog)
> >> 
> >>   	/* Suspend timer in low power mode, write once-only */
> >>   	val |= IMX2_WDT_WCR_WDZST;
> >> 
> >> +	/* Suspend timer in low power WAIT mode, write once-only */
> >> +	if (wdev->sleep_wait)
> >> +		val |= IMX2_WDT_WCR_WDW;
> >> 
> >>   	/* Strip the old watchdog Time-Out value */
> >>   	val &= ~IMX2_WDT_WCR_WT;
> >>   	/* Generate internal chip-level reset if WDOG times out */
> >> 
> >> @@ -313,6 +338,18 @@ static int __init imx2_wdt_probe(struct
> >> platform_device *pdev)
> >> 
> >>   	wdev->ext_reset = of_property_read_bool(dev->of_node,
> >>   	
> >>   						"fsl,ext-
> > 
> > reset-output");
> > 
> >> +
> >> +	if (of_property_read_bool(dev->of_node, "fsl,suspend-in-wait"))
> >> +		if (of_device_compatible_match(dev->of_node,
> > 
> > wdw_boards))
> > 
> >> +			wdev->sleep_wait = 1;
> >> +		else {
> >> +			dev_warn(dev, "Warning: Suspending watchdog
> > 
> > during " \
> > 
> >> +				"WAIT mode is not supported for
> > 
> > this device.\n");
> > 
> >> +			wdev->sleep_wait = 0;
> >> +		}
> >> +	else
> >> +		wdev->sleep_wait = 0;
> >> +
> >> 
> >>   	/*
> >>   	
> >>   	 * The i.MX7D doesn't support low power mode, so we need to ping
> > 
> > the
> > 
> >> watchdog * during suspend.

Re: [PATCH v2 2/3] dt-bindings: watchdog: fsl-imx: document suspend in wait mode

[Andrej Picej]

On 25. 10. 22 15:48, Krzysztof Kozlowski wrote:
> On 25/10/2022 03:25, Andrej Picej wrote:
>> Property "fsl,suspend-in-wait" suspends watchdog in "WAIT" mode which
>> corresponds to Linux's Suspend-to-Idle S0 mode. If this property is not
>> set and the device is put into Suspend-to-Idle mode, the watchdog
>> triggers a reset after 128 seconds.
>>
>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>> Reviewed-by: Fabio Estevam <festevam@gmail.com>
>> ---
>> Changes in v2:
>>   - add a commit message,
>>   - add a list of devices which support this functionality
>> ---
>>   .../bindings/watchdog/fsl-imx-wdt.yaml        | 22 +++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>> index fb7695515be1..9289de97859b 100644
>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>> @@ -55,6 +55,28 @@ properties:
>>         If present, the watchdog device is configured to assert its
>>         external reset (WDOG_B) instead of issuing a software reset.
>>   
>> +  fsl,suspend-in-wait:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description: |
>> +      If present, the watchdog device is suspended in WAIT mode
>> +      (Suspend-to-Idle). Only supported on following devices:
>> +        - "fsl,imx25-wdt",
> 
> You need to define such allow/disallow in allOf:if:then, instead. Like
> example-schema is doing for foo-supply, just disallow it for some types
> or use "if: not: ..."

Sorry missed that. So something like that should be added?:

> allOf:
>   - if:
>       not:
>         properties:
>           compatible:
>             contains:
>               enum:
>                 - fsl,imx25-wdt
>                 - fsl,imx35-wdt
>                 - fsl,imx50-wdt
>                 - fsl,imx51-wdt
>                 - fsl,imx53-wdt
>                 - fsl,imx6q-wdt
>                 - fsl,imx6sl-wdt
>                 - fsl,imx6sll-wdt
>                 - fsl,imx6sx-wdt
>                 - fsl,imx6ul-wdt
>                 - fsl,imx7d-wdt
>                 - fsl,imx8mm-wdt
>                 - fsl,imx8mn-wdt
>                 - fsl,imx8mp-wdt
>                 - fsl,imx8mq-wdt
>                 - fsl,vf610-wdt
>     then:
>       properties:
>         fsl,suspend-in-wait: false

And I'm assuming I can then remove the supported devices list from 
property description.

Are you fine with this, so we don't have to split the compatible list 
like Alexander suggested? Basically we have the same list of WDW 
supported devices in the driver.

Thank you for your review,
Andrej

Re: [PATCH v2 1/3] watchdog: imx2_wdg: suspend watchdog in WAIT mode

[Andrej Picej]

On 25. 10. 22 16:21, Guenter Roeck wrote:
> On 10/25/22 00:25, Andrej Picej wrote:
>> Putting device into the "Suspend-To-Idle" mode causes watchdog to
>> trigger and reset the board after set watchdog timeout period elapses.
>>
> 
> s/reset/resets/
> 
>> Introduce new device-tree property "fsl,suspend-in-wait" which suspends
>> watchdog in WAIT mode. This is done by setting WDW bit in WCR
>> (Watchdog Control Register) Watchdog operation is restored after exiting
> 
> '.' after ')' missing ?
> 
>> WAIT mode as expected. WAIT mode coresponds with Linux's
> 
> s/coresponds/corresponds/
> 
Will fix this in v3, thank you.

>> "Suspend-To-Idle".
>>
>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>> Reviewed-by: Fabio Estevam <festevam@gmail.com>
>> ---
>> Changes in v2:
>>   - validate the property with compatible string, as this functionality
>>     is not supported by all devices.
>> ---
>>   drivers/watchdog/imx2_wdt.c | 37 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
>> index d0c5d47ddede..dd9866c6f1e5 100644
>> --- a/drivers/watchdog/imx2_wdt.c
>> +++ b/drivers/watchdog/imx2_wdt.c
>> @@ -35,6 +35,7 @@
>>   #define IMX2_WDT_WCR        0x00        /* Control Register */
>>   #define IMX2_WDT_WCR_WT        (0xFF << 8)    /* -> Watchdog Timeout 
>> Field */
>> +#define IMX2_WDT_WCR_WDW    BIT(7)        /* -> Watchdog disable for 
>> WAIT */
>>   #define IMX2_WDT_WCR_WDA    BIT(5)        /* -> External Reset 
>> WDOG_B */
>>   #define IMX2_WDT_WCR_SRS    BIT(4)        /* -> Software Reset 
>> Signal */
>>   #define IMX2_WDT_WCR_WRE    BIT(3)        /* -> WDOG Reset Enable */
>> @@ -67,6 +68,27 @@ struct imx2_wdt_device {
>>       bool ext_reset;
>>       bool clk_is_on;
>>       bool no_ping;
>> +    bool sleep_wait;
>> +};
>> +
>> +static const char * const wdw_boards[] __initconst = {
>> +    "fsl,imx25-wdt",
>> +    "fsl,imx35-wdt",
>> +    "fsl,imx50-wdt",
>> +    "fsl,imx51-wdt",
>> +    "fsl,imx53-wdt",
>> +    "fsl,imx6q-wdt",
>> +    "fsl,imx6sl-wdt",
>> +    "fsl,imx6sll-wdt",
>> +    "fsl,imx6sx-wdt",
>> +    "fsl,imx6ul-wdt",
>> +    "fsl,imx7d-wdt",
>> +    "fsl,imx8mm-wdt",
>> +    "fsl,imx8mn-wdt",
>> +    "fsl,imx8mp-wdt",
>> +    "fsl,imx8mq-wdt",
>> +    "fsl,vf610-wdt",
>> +    NULL
>>   };
>>   static bool nowayout = WATCHDOG_NOWAYOUT;
>> @@ -129,6 +151,9 @@ static inline void imx2_wdt_setup(struct 
>> watchdog_device *wdog)
>>       /* Suspend timer in low power mode, write once-only */
>>       val |= IMX2_WDT_WCR_WDZST;
>> +    /* Suspend timer in low power WAIT mode, write once-only */
>> +    if (wdev->sleep_wait)
>> +        val |= IMX2_WDT_WCR_WDW;
>>       /* Strip the old watchdog Time-Out value */
>>       val &= ~IMX2_WDT_WCR_WT;
>>       /* Generate internal chip-level reset if WDOG times out */
>> @@ -313,6 +338,18 @@ static int __init imx2_wdt_probe(struct 
>> platform_device *pdev)
>>       wdev->ext_reset = of_property_read_bool(dev->of_node,
>>                           "fsl,ext-reset-output");
>> +
>> +    if (of_property_read_bool(dev->of_node, "fsl,suspend-in-wait"))
>> +        if (of_device_compatible_match(dev->of_node, wdw_boards))
>> +            wdev->sleep_wait = 1;
> 
> Since sleep_wait is bool:
>              wdev->sleep_wait = true;
> 
>> +        else {
>> +            dev_warn(dev, "Warning: Suspending watchdog during " \
>> +                "WAIT mode is not supported for this device.\n");
> 
> Do not split strings. "Warning:" is redundant. Please handle the error 
> first.
> 
>> +            wdev->sleep_wait = 0;
> 
> Unnecessary; false by default. Also, this should fail and return -EINVAL.
> Devicetree files should be correct, and warning messages tend to be 
> ignored.
> 
>> +        }
> 
> All branches of if/else need to wither use {} or no {}.
> 
>> +    else
>> +        wdev->sleep_wait = 0;
>> +
> Unnecessary.
> 
> I would suggest to replace the above code with something like
> 
>      if (of_property_read_bool(dev->of_node, "fsl,suspend-in-wait")) {
>          if (!of_device_compatible_match(dev->of_node, wdw_boards)) {
>              dev_err(dev, "Suspending watchdog in WAIT mode is not 
> supported for this device\n");
>              return -EINVAL;
>          }
>          wdev->sleep_wait = true;
>      }

OK, this look cleaner, will use this, thanks.

> 
>>       /*
>>        * The i.MX7D doesn't support low power mode, so we need to ping 
>> the watchdog
>>        * during suspend.
> 
> I still wonder how that interacts with fsl,suspend-in-wait, but since we 
> have a
> property for that we can leave that for someone else to find out. Maybe 
> add a
> comment explaining that interaction with "fsl,suspend-in-wait" is unknown.
I'm assuming that i.MX7D doesn't enter any of low-power modes including 
WAIT mode. If this is the case the watchdog wouldn't get disabled.

Anyway, I will add a short comment regarding the unknown behaviour of 
this property with i.MX7D device.

Best regards,
Andrej

Re: [PATCH v2 1/3] watchdog: imx2_wdg: suspend watchdog in WAIT mode

[Andrej Picej]

On 26. 10. 22 08:01, Alexander Stein wrote:
> Hello Andrej,
> 
> Am Dienstag, 25. Oktober 2022, 13:21:18 CEST schrieb Andrej Picej:
>> Hi Alexander,
>>
>> On 25. 10. 22 11:38, Alexander Stein wrote:
>>> Am Dienstag, 25. Oktober 2022, 09:25:31 CEST schrieb Andrej Picej:
>>>> Putting device into the "Suspend-To-Idle" mode causes watchdog to
>>>> trigger and reset the board after set watchdog timeout period elapses.
>>>>
>>>> Introduce new device-tree property "fsl,suspend-in-wait" which suspends
>>>> watchdog in WAIT mode. This is done by setting WDW bit in WCR
>>>> (Watchdog Control Register) Watchdog operation is restored after exiting
>>>> WAIT mode as expected. WAIT mode coresponds with Linux's
>>>> "Suspend-To-Idle".
>>>>
>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>>> Reviewed-by: Fabio Estevam <festevam@gmail.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>>    - validate the property with compatible string, as this functionality
>>>>    
>>>>      is not supported by all devices.
>>>>
>>>> ---
>>>>
>>>>    drivers/watchdog/imx2_wdt.c | 37 +++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 37 insertions(+)
>>>>
>>>> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
>>>> index d0c5d47ddede..dd9866c6f1e5 100644
>>>> --- a/drivers/watchdog/imx2_wdt.c
>>>> +++ b/drivers/watchdog/imx2_wdt.c
>>>> @@ -35,6 +35,7 @@
>>>>
>>>>    #define IMX2_WDT_WCR		0x00		/* Control
>>>
>>> Register */
>>>
>>>>    #define IMX2_WDT_WCR_WT		(0xFF << 8)	/* ->
>>>
>>> Watchdog Timeout Field */
>>>
>>>> +#define IMX2_WDT_WCR_WDW	BIT(7)		/* -> Watchdog disable
>>>
>>> for WAIT */
>>>
>>>>    #define IMX2_WDT_WCR_WDA	BIT(5)		/* -> External Reset
>>>
>>> WDOG_B */
>>>
>>>>    #define IMX2_WDT_WCR_SRS	BIT(4)		/* -> Software Reset
>>>
>>> Signal */
>>>
>>>>    #define IMX2_WDT_WCR_WRE	BIT(3)		/* -> WDOG Reset Enable
>>>
>>> */
>>>
>>>> @@ -67,6 +68,27 @@ struct imx2_wdt_device {
>>>>
>>>>    	bool ext_reset;
>>>>    	bool clk_is_on;
>>>>    	bool no_ping;
>>>>
>>>> +	bool sleep_wait;
>>>> +};
>>>> +
>>>> +static const char * const wdw_boards[] __initconst = {
>>>> +	"fsl,imx25-wdt",
>>>> +	"fsl,imx35-wdt",
>>>> +	"fsl,imx50-wdt",
>>>> +	"fsl,imx51-wdt",
>>>> +	"fsl,imx53-wdt",
>>>> +	"fsl,imx6q-wdt",
>>>> +	"fsl,imx6sl-wdt",
>>>> +	"fsl,imx6sll-wdt",
>>>> +	"fsl,imx6sx-wdt",
>>>> +	"fsl,imx6ul-wdt",
>>>> +	"fsl,imx7d-wdt",
>>>> +	"fsl,imx8mm-wdt",
>>>> +	"fsl,imx8mn-wdt",
>>>> +	"fsl,imx8mp-wdt",
>>>> +	"fsl,imx8mq-wdt",
>>>> +	"fsl,vf610-wdt",
>>>> +	NULL
>>>>
>>>>    };
>>>
>>> So the models listed in
>>> Documentation/devicetree/bindings/watchdog/fsl-imx-
>>> wdt.yaml not supporting this feature are
>>> * fsl,imx21-wdt
>>> * fsl,imx27-wdt
>>> * fsl,imx31-wdt
>>> * fsl,ls1012a-wdt
>>> * fsl,ls1043a-wdt
>>> ?
>>
>> yes, you are correct.
>>
>>> But all models are listed as compatible to fsl,imx21-wdt. So there is
>>> something wrong here. IMHO this sounds like the compatible list has to be
>>> split and updated. Depending on that this feature can be detected.
>>> Maintaining another list seems error prone to me.
>>
>> So basically the compatible lists would be split into two groups, one
>> for the devices which support this WDW bit and the rest which don't
>> support this?
> 
> This was my idea, so only one set has to be maintained.
> 
>> You got a point here, but...this means that every processors
>> device-tree, which has two compatible strings (with "fsl,imx21-wdt")
>> should be updated, right? That sounds like quite a lot of changes, which
>> I'd like to avoid if possible.
> 
> Well, the compatible list right now doesn't reflect the hardware features/
> compatibility correctly, so IMHO it should be fixed.
> But apparently Krzysztof is okay having the special property only applicable
> for a specific set of devices. But in this case you will have to maintain two
> sets of device models (bindings + driver) to which WDW applies/does not apply
> to.
> 
Ok, lets see what @Krzysztof has to say about this.

Best regards,
Andrej

Re: [PATCH v2 2/3] dt-bindings: watchdog: fsl-imx: document suspend in wait mode

[Krzysztof Kozlowski]

On 26/10/2022 02:38, Andrej Picej wrote:
> On 25. 10. 22 15:48, Krzysztof Kozlowski wrote:
>> On 25/10/2022 03:25, Andrej Picej wrote:
>>> Property "fsl,suspend-in-wait" suspends watchdog in "WAIT" mode which
>>> corresponds to Linux's Suspend-to-Idle S0 mode. If this property is not
>>> set and the device is put into Suspend-to-Idle mode, the watchdog
>>> triggers a reset after 128 seconds.
>>>
>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>> Reviewed-by: Fabio Estevam <festevam@gmail.com>
>>> ---
>>> Changes in v2:
>>>   - add a commit message,
>>>   - add a list of devices which support this functionality
>>> ---
>>>   .../bindings/watchdog/fsl-imx-wdt.yaml        | 22 +++++++++++++++++++
>>>   1 file changed, 22 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>> index fb7695515be1..9289de97859b 100644
>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>> @@ -55,6 +55,28 @@ properties:
>>>         If present, the watchdog device is configured to assert its
>>>         external reset (WDOG_B) instead of issuing a software reset.
>>>   
>>> +  fsl,suspend-in-wait:
>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>> +    description: |
>>> +      If present, the watchdog device is suspended in WAIT mode
>>> +      (Suspend-to-Idle). Only supported on following devices:
>>> +        - "fsl,imx25-wdt",
>>
>> You need to define such allow/disallow in allOf:if:then, instead. Like
>> example-schema is doing for foo-supply, just disallow it for some types
>> or use "if: not: ..."
> 
> Sorry missed that. So something like that should be added?:
> 
>> allOf:
>>   - if:
>>       not:
>>         properties:
>>           compatible:
>>             contains:
>>               enum:
>>                 - fsl,imx25-wdt
>>                 - fsl,imx35-wdt
>>                 - fsl,imx50-wdt
>>                 - fsl,imx51-wdt
>>                 - fsl,imx53-wdt
>>                 - fsl,imx6q-wdt
>>                 - fsl,imx6sl-wdt
>>                 - fsl,imx6sll-wdt
>>                 - fsl,imx6sx-wdt
>>                 - fsl,imx6ul-wdt
>>                 - fsl,imx7d-wdt
>>                 - fsl,imx8mm-wdt
>>                 - fsl,imx8mn-wdt
>>                 - fsl,imx8mp-wdt
>>                 - fsl,imx8mq-wdt
>>                 - fsl,vf610-wdt

Yes.

>>     then:
>>       properties:
>>         fsl,suspend-in-wait: false
> 
> And I'm assuming I can then remove the supported devices list from 
> property description.

Yes.

> 
> Are you fine with this, so we don't have to split the compatible list 
> like Alexander suggested? Basically we have the same list of WDW 
> supported devices in the driver.

I don't know to what you refer.

Best regards,
Krzysztof

Re: [PATCH v2 2/3] dt-bindings: watchdog: fsl-imx: document suspend in wait mode

[Andrej Picej]

On 26. 10. 22 16:12, Krzysztof Kozlowski wrote:
> On 26/10/2022 02:38, Andrej Picej wrote:
>> On 25. 10. 22 15:48, Krzysztof Kozlowski wrote:
>>> On 25/10/2022 03:25, Andrej Picej wrote:
>>>> Property "fsl,suspend-in-wait" suspends watchdog in "WAIT" mode which
>>>> corresponds to Linux's Suspend-to-Idle S0 mode. If this property is not
>>>> set and the device is put into Suspend-to-Idle mode, the watchdog
>>>> triggers a reset after 128 seconds.
>>>>
>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>>> Reviewed-by: Fabio Estevam <festevam@gmail.com>
>>>> ---
>>>> Changes in v2:
>>>>    - add a commit message,
>>>>    - add a list of devices which support this functionality
>>>> ---
>>>>    .../bindings/watchdog/fsl-imx-wdt.yaml        | 22 +++++++++++++++++++
>>>>    1 file changed, 22 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>> index fb7695515be1..9289de97859b 100644
>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>> @@ -55,6 +55,28 @@ properties:
>>>>          If present, the watchdog device is configured to assert its
>>>>          external reset (WDOG_B) instead of issuing a software reset.
>>>>    
>>>> +  fsl,suspend-in-wait:
>>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>>> +    description: |
>>>> +      If present, the watchdog device is suspended in WAIT mode
>>>> +      (Suspend-to-Idle). Only supported on following devices:
>>>> +        - "fsl,imx25-wdt",
>>>
>>> You need to define such allow/disallow in allOf:if:then, instead. Like
>>> example-schema is doing for foo-supply, just disallow it for some types
>>> or use "if: not: ..."
>>
>> Sorry missed that. So something like that should be added?:
>>
>>> allOf:
>>>    - if:
>>>        not:
>>>          properties:
>>>            compatible:
>>>              contains:
>>>                enum:
>>>                  - fsl,imx25-wdt
>>>                  - fsl,imx35-wdt
>>>                  - fsl,imx50-wdt
>>>                  - fsl,imx51-wdt
>>>                  - fsl,imx53-wdt
>>>                  - fsl,imx6q-wdt
>>>                  - fsl,imx6sl-wdt
>>>                  - fsl,imx6sll-wdt
>>>                  - fsl,imx6sx-wdt
>>>                  - fsl,imx6ul-wdt
>>>                  - fsl,imx7d-wdt
>>>                  - fsl,imx8mm-wdt
>>>                  - fsl,imx8mn-wdt
>>>                  - fsl,imx8mp-wdt
>>>                  - fsl,imx8mq-wdt
>>>                  - fsl,vf610-wdt
> 
> Yes.
> 
>>>      then:
>>>        properties:
>>>          fsl,suspend-in-wait: false
>>
>> And I'm assuming I can then remove the supported devices list from
>> property description.
> 
> Yes.
> 
>>
>> Are you fine with this, so we don't have to split the compatible list
>> like Alexander suggested? Basically we have the same list of WDW
>> supported devices in the driver.
> 
> I don't know to what you refer.
> 
I'm referring to this comment by Alexander Stein: 
(https://lore.kernel.org/all/13126397.uLZWGnKmhe@steina-w/)

> So the models listed in Documentation/devicetree/bindings/watchdog/fsl-imx-
> wdt.yaml not supporting this feature are
> * fsl,imx21-wdt
> * fsl,imx27-wdt
> * fsl,imx31-wdt
> * fsl,ls1012a-wdt
> * fsl,ls1043a-wdt
> ?
> 
> But all models are listed as compatible to fsl,imx21-wdt. So there is 
> something wrong here. IMHO this sounds like the compatible list has to be 
> split and updated. Depending on that this feature can be detected. Maintaining 
> another list seems error prone to me.

Best regards,
Andrej.