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

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

[Andrej Picej]

The i.MX6 watchdog can't be stopped once started. This means that
special hardware suspend needs to be configured when the device enters
low-power modes.
Usually i.MX devices have two bits which deal with this:
- WDZST bit disables the timer in "deeper" low power modes and
- WDW bit disables the timer in "WAIT" mode which corresponds with
Linux's "freeze" low-power mode.

WDZST bit support is already in place since 1a9c5efa576e ("watchdog: imx2_wdt: disable watchdog timer during low power mode").
WDW bit is not common for all imx2-wdt supported devices, therefore use
a new device-tree property "fsl,suspend-in-wait" which suspends the
watchdog in "WAIT" mode.

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

 .../devicetree/bindings/watchdog/fsl-imx-wdt.yaml          | 5 +++++
 arch/arm/boot/dts/imx6ul-phytec-phycore-som.dtsi           | 4 ++++
 drivers/watchdog/imx2_wdt.c                                | 7 +++++++
 3 files changed, 16 insertions(+)

-- 
2.25.1

[PATCH 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>
---
 drivers/watchdog/imx2_wdt.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index d0c5d47ddede..150ba83ce176 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,7 @@ struct imx2_wdt_device {
 	bool ext_reset;
 	bool clk_is_on;
 	bool no_ping;
+	bool sleep_wait;
 };
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
@@ -129,6 +131,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 +318,8 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
 
 	wdev->ext_reset = of_property_read_bool(dev->of_node,
 						"fsl,ext-reset-output");
+	wdev->sleep_wait = of_property_read_bool(dev->of_node,
+						"fsl,suspend-in-wait");
 	/*
 	 * The i.MX7D doesn't support low power mode, so we need to ping the watchdog
 	 * during suspend.
-- 
2.25.1

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

[Andrej Picej]

Signed-off-by: Andrej Picej <andrej.picej@norik.com>
---
 Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
index fb7695515be1..01b3e04e7e65 100644
--- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
@@ -55,6 +55,11 @@ 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.
+
 required:
   - compatible
   - interrupts
-- 
2.25.1

[PATCH 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>
---
 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 0/3] Suspending i.MX watchdog in WAIT mode

[Fabio Estevam]

Hi Andrej,

On Wed, Oct 19, 2022 at 8:17 AM Andrej Picej <andrej.picej@norik.com> wrote:
>
> The i.MX6 watchdog can't be stopped once started. This means that
> special hardware suspend needs to be configured when the device enters
> low-power modes.
> Usually i.MX devices have two bits which deal with this:
> - WDZST bit disables the timer in "deeper" low power modes and
> - WDW bit disables the timer in "WAIT" mode which corresponds with
> Linux's "freeze" low-power mode.
>
> WDZST bit support is already in place since 1a9c5efa576e ("watchdog: imx2_wdt: disable watchdog timer during low power mode").
> WDW bit is not common for all imx2-wdt supported devices, therefore use
> a new device-tree property "fsl,suspend-in-wait" which suspends the
> watchdog in "WAIT" mode.
>
> 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

For the series:

Reviewed-by: Fabio Estevam <festevam@gmail.com>

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

[Alexander Stein]

Hello Andrej,

Am Mittwoch, 19. Oktober 2022, 13:17:13 CEST schrieb Andrej Picej:
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> ---
>  Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml index
> fb7695515be1..01b3e04e7e65 100644
> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
> @@ -55,6 +55,11 @@ 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.
> +
>  required:
>    - compatible
>    - interrupts

What is the condition the watchdog is suspended in WAIT mode? Is this specific 
to SoC or platform or something else?

Best regards,
Alexander

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

[Guenter Roeck]

On Wed, Oct 19, 2022 at 01:17:11PM +0200, Andrej Picej wrote:
> The i.MX6 watchdog can't be stopped once started. This means that
> special hardware suspend needs to be configured when the device enters
> low-power modes.
> Usually i.MX devices have two bits which deal with this:
> - WDZST bit disables the timer in "deeper" low power modes and
> - WDW bit disables the timer in "WAIT" mode which corresponds with
> Linux's "freeze" low-power mode.
> 
> WDZST bit support is already in place since 1a9c5efa576e ("watchdog: imx2_wdt: disable watchdog timer during low power mode").
> WDW bit is not common for all imx2-wdt supported devices, therefore use
> a new device-tree property "fsl,suspend-in-wait" which suspends the
> watchdog in "WAIT" mode.

I think that needs to be validated using the "compatible" property;
it should not be possible to set/accept the new flag for devices
which don't support it.

Thanks,
Guenter

> 
> 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
> 
>  .../devicetree/bindings/watchdog/fsl-imx-wdt.yaml          | 5 +++++
>  arch/arm/boot/dts/imx6ul-phytec-phycore-som.dtsi           | 4 ++++
>  drivers/watchdog/imx2_wdt.c                                | 7 +++++++
>  3 files changed, 16 insertions(+)
> 
> -- 
> 2.25.1
> 

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

[Guenter Roeck]

On Wed, Oct 19, 2022 at 01:17:12PM +0200, 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".
> 

Does that have any impact on suspend/resume handling in the driver,
specifically with the "no_ping" variable used for fsl,imx7d-wdt ?

Thanks,
Guenter

> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> ---
>  drivers/watchdog/imx2_wdt.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index d0c5d47ddede..150ba83ce176 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,7 @@ struct imx2_wdt_device {
>  	bool ext_reset;
>  	bool clk_is_on;
>  	bool no_ping;
> +	bool sleep_wait;
>  };
>  
>  static bool nowayout = WATCHDOG_NOWAYOUT;
> @@ -129,6 +131,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 +318,8 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
>  
>  	wdev->ext_reset = of_property_read_bool(dev->of_node,
>  						"fsl,ext-reset-output");
> +	wdev->sleep_wait = of_property_read_bool(dev->of_node,
> +						"fsl,suspend-in-wait");
>  	/*
>  	 * The i.MX7D doesn't support low power mode, so we need to ping the watchdog
>  	 * during suspend.
> -- 
> 2.25.1
> 

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

[Krzysztof Kozlowski]

On 19/10/2022 07:17, Andrej Picej wrote:
> The i.MX6 watchdog can't be stopped once started. This means that
> special hardware suspend needs to be configured when the device enters
> low-power modes.
> Usually i.MX devices have two bits which deal with this:
> - WDZST bit disables the timer in "deeper" low power modes and
> - WDW bit disables the timer in "WAIT" mode which corresponds with
> Linux's "freeze" low-power mode.
> 
> WDZST bit support is already in place since 1a9c5efa576e ("watchdog: imx2_wdt: disable watchdog timer during low power mode").
> WDW bit is not common for all imx2-wdt supported devices, therefore use
> a new device-tree property "fsl,suspend-in-wait" which suspends the
> watchdog in "WAIT" mode.
> 
> 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
> 
>  .../devicetree/bindings/watchdog/fsl-imx-wdt.yaml          | 5 +++++

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 19/10/2022 09:00, Alexander Stein wrote:
> Hello Andrej,
> 
> Am Mittwoch, 19. Oktober 2022, 13:17:13 CEST schrieb Andrej Picej:

Missing commit msg.

>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>> ---
>>  Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml index
>> fb7695515be1..01b3e04e7e65 100644
>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>> @@ -55,6 +55,11 @@ 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.
>> +
>>  required:
>>    - compatible
>>    - interrupts
> 
> What is the condition the watchdog is suspended in WAIT mode? Is this specific 
> to SoC or platform or something else?
>

And what happens else? When it is not suspended in WAIT mode?

Best regards,
Krzysztof

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

[Andrej Picej]

Hi Guenter,

On 19. 10. 22 17:30, Guenter Roeck wrote:
> On Wed, Oct 19, 2022 at 01:17:11PM +0200, Andrej Picej wrote:
>> The i.MX6 watchdog can't be stopped once started. This means that
>> special hardware suspend needs to be configured when the device enters
>> low-power modes.
>> Usually i.MX devices have two bits which deal with this:
>> - WDZST bit disables the timer in "deeper" low power modes and
>> - WDW bit disables the timer in "WAIT" mode which corresponds with
>> Linux's "freeze" low-power mode.
>>
>> WDZST bit support is already in place since 1a9c5efa576e ("watchdog: imx2_wdt: disable watchdog timer during low power mode").
>> WDW bit is not common for all imx2-wdt supported devices, therefore use
>> a new device-tree property "fsl,suspend-in-wait" which suspends the
>> watchdog in "WAIT" mode.
> 
> I think that needs to be validated using the "compatible" property;
> it should not be possible to set/accept the new flag for devices
> which don't support it.

Ok, I can add that to a v2.

Thanks,
Andrej

> 
> Thanks,
> Guenter
> 

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

[Andrej Picej]

Hi Krzysztof,

On 19. 10. 22 17:46, Krzysztof Kozlowski wrote:
> On 19/10/2022 07:17, Andrej Picej wrote:
>> The i.MX6 watchdog can't be stopped once started. This means that
>> special hardware suspend needs to be configured when the device enters
>> low-power modes.
>> Usually i.MX devices have two bits which deal with this:
>> - WDZST bit disables the timer in "deeper" low power modes and
>> - WDW bit disables the timer in "WAIT" mode which corresponds with
>> Linux's "freeze" low-power mode.
>>
>> WDZST bit support is already in place since 1a9c5efa576e ("watchdog: imx2_wdt: disable watchdog timer during low power mode").
>> WDW bit is not common for all imx2-wdt supported devices, therefore use
>> a new device-tree property "fsl,suspend-in-wait" which suspends the
>> watchdog in "WAIT" mode.
>>
>> 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
>>
>>   .../devicetree/bindings/watchdog/fsl-imx-wdt.yaml          | 5 +++++
> 
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC.  It might happen, that command when run on an older
> kernel, gives you outdated entries.  Therefore please be sure you base
> your patches on recent Linux kernel.

I thought I did. I run that script on linux-watchdog.git, master branch.
I thought I should base my patches meant for watchdog subsystem there?

Best regards,
Andrej

> 
> Best regards,
> Krzysztof
> 

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

[Andrej Picej]

On 19. 10. 22 17:33, Guenter Roeck wrote:
> On Wed, Oct 19, 2022 at 01:17:12PM +0200, 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".
>>
> 
> Does that have any impact on suspend/resume handling in the driver,
> specifically with the "no_ping" variable used for fsl,imx7d-wdt ?

I assumed that it has the same impact as WDZST bit, which is also set 
for imx7d. So probably no impact. But I'm not really sure (unfortunately 
I don't have access to imx7d to test). If I understand correctly the 
"no-ping" variable is used because there is no support for low-power 
modes for imx7d. So imx7d devices never enter any of the low-power modes 
so WDZST and WDW bit shouldn't have effect on that.

What I wanted to do with this "fsl,suspend-in-wait" is that if people 
run into the problems where device resets undesirably during WAIT mode, 
they can set this property and the watchdog will be suspended in WAIT 
mode. Default behaviour of the driver stays the same if the flag is not set.

This was tested with imx6ul devices.

Best regards,
Andrej

> 
> Thanks,
> Guenter
> 
>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>> ---
>>   drivers/watchdog/imx2_wdt.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
>> index d0c5d47ddede..150ba83ce176 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,7 @@ struct imx2_wdt_device {
>>   	bool ext_reset;
>>   	bool clk_is_on;
>>   	bool no_ping;
>> +	bool sleep_wait;
>>   };
>>   
>>   static bool nowayout = WATCHDOG_NOWAYOUT;
>> @@ -129,6 +131,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 +318,8 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
>>   
>>   	wdev->ext_reset = of_property_read_bool(dev->of_node,
>>   						"fsl,ext-reset-output");
>> +	wdev->sleep_wait = of_property_read_bool(dev->of_node,
>> +						"fsl,suspend-in-wait");
>>   	/*
>>   	 * The i.MX7D doesn't support low power mode, so we need to ping the watchdog
>>   	 * during suspend.
>> -- 
>> 2.25.1
>>

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

[Andrej Picej]

Hi Alexander and Krzysztof,

hope I can reply to both questions here.

On 19. 10. 22 17:51, Krzysztof Kozlowski wrote:
> On 19/10/2022 09:00, Alexander Stein wrote:
>> Hello Andrej,
>>
>> Am Mittwoch, 19. Oktober 2022, 13:17:13 CEST schrieb Andrej Picej:
> 
> Missing commit msg.
> 
>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>> ---
>>>   Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml index
>>> fb7695515be1..01b3e04e7e65 100644
>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>> @@ -55,6 +55,11 @@ 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.
>>> +
>>>   required:
>>>     - compatible
>>>     - interrupts
>>
>> What is the condition the watchdog is suspended in WAIT mode? Is this specific
>> to SoC or platform or something else?
>>
> 

Sorry, what exactly do you mean by condition? When the property 
"fsl,suspend-in-wait" is set the watchdog is suspended in WAIT mode, so 
this is defined by the user. Didn't want to apply it for all the 
supported machines since there could be devices which depend on watchdog 
triggering in WAIT mode. We stumbled on this problem on imx6 devices, 
but the same bit (with the same description) is found on imx25, imx35, 
imx50/51/53, imx7 and imx8.

> And what happens else? When it is not suspended in WAIT mode?
> 

When you put the device in "freeze"/"Suspend-To-Idle" low-power mode the 
watchdog keeps running and triggers a reset after 128 seconds. So the 
maximum length the device can stay in this mode is limited to 128 seconds.

Hope this answers your questions.

Best regards,
Andrej

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

[Krzysztof Kozlowski]

On 20/10/2022 01:49, Andrej Picej wrote:
> Hi Krzysztof,
> 
> On 19. 10. 22 17:46, Krzysztof Kozlowski wrote:
>> On 19/10/2022 07:17, Andrej Picej wrote:
>>> The i.MX6 watchdog can't be stopped once started. This means that
>>> special hardware suspend needs to be configured when the device enters
>>> low-power modes.
>>> Usually i.MX devices have two bits which deal with this:
>>> - WDZST bit disables the timer in "deeper" low power modes and
>>> - WDW bit disables the timer in "WAIT" mode which corresponds with
>>> Linux's "freeze" low-power mode.
>>>
>>> WDZST bit support is already in place since 1a9c5efa576e ("watchdog: imx2_wdt: disable watchdog timer during low power mode").
>>> WDW bit is not common for all imx2-wdt supported devices, therefore use
>>> a new device-tree property "fsl,suspend-in-wait" which suspends the
>>> watchdog in "WAIT" mode.
>>>
>>> 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
>>>
>>>   .../devicetree/bindings/watchdog/fsl-imx-wdt.yaml          | 5 +++++
>>
>> Please use scripts/get_maintainers.pl to get a list of necessary people
>> and lists to CC.  It might happen, that command when run on an older
>> kernel, gives you outdated entries.  Therefore please be sure you base
>> your patches on recent Linux kernel.
> 
> I thought I did. I run that script on linux-watchdog.git, master branch.
> I thought I should base my patches meant for watchdog subsystem there?

Maintainer's tree should be fine, but then the issue is somewhere else,
because your CC list was not complete.


Best regards,
Krzysztof

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

[Andrej Picej]

On 20. 10. 22 14:04, Krzysztof Kozlowski wrote:
> On 20/10/2022 01:49, Andrej Picej wrote:
>> Hi Krzysztof,
>>
>> On 19. 10. 22 17:46, Krzysztof Kozlowski wrote:
>>> On 19/10/2022 07:17, Andrej Picej wrote:
>>>> The i.MX6 watchdog can't be stopped once started. This means that
>>>> special hardware suspend needs to be configured when the device enters
>>>> low-power modes.
>>>> Usually i.MX devices have two bits which deal with this:
>>>> - WDZST bit disables the timer in "deeper" low power modes and
>>>> - WDW bit disables the timer in "WAIT" mode which corresponds with
>>>> Linux's "freeze" low-power mode.
>>>>
>>>> WDZST bit support is already in place since 1a9c5efa576e ("watchdog: imx2_wdt: disable watchdog timer during low power mode").
>>>> WDW bit is not common for all imx2-wdt supported devices, therefore use
>>>> a new device-tree property "fsl,suspend-in-wait" which suspends the
>>>> watchdog in "WAIT" mode.
>>>>
>>>> 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
>>>>
>>>>    .../devicetree/bindings/watchdog/fsl-imx-wdt.yaml          | 5 +++++
>>>
>>> Please use scripts/get_maintainers.pl to get a list of necessary people
>>> and lists to CC.  It might happen, that command when run on an older
>>> kernel, gives you outdated entries.  Therefore please be sure you base
>>> your patches on recent Linux kernel.
>>
>> I thought I did. I run that script on linux-watchdog.git, master branch.
>> I thought I should base my patches meant for watchdog subsystem there?
> 
> Maintainer's tree should be fine, but then the issue is somewhere else,
> because your CC list was not complete.
> 

Ok I see that two email addresses were not added. Sorry for that. I will 
make sure that the CC list is complete next time.

Thanks,
Andrej

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

[Krzysztof Kozlowski]

On 20/10/2022 02:23, Andrej Picej wrote:
> Hi Alexander and Krzysztof,
> 
> hope I can reply to both questions here.
> 
> On 19. 10. 22 17:51, Krzysztof Kozlowski wrote:
>> On 19/10/2022 09:00, Alexander Stein wrote:
>>> Hello Andrej,
>>>
>>> Am Mittwoch, 19. Oktober 2022, 13:17:13 CEST schrieb Andrej Picej:
>>
>> Missing commit msg.
>>
>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>>> ---
>>>>   Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml | 5 +++++
>>>>   1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml index
>>>> fb7695515be1..01b3e04e7e65 100644
>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>> @@ -55,6 +55,11 @@ 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.
>>>> +
>>>>   required:
>>>>     - compatible
>>>>     - interrupts
>>>
>>> What is the condition the watchdog is suspended in WAIT mode? Is this specific
>>> to SoC or platform or something else?
>>>
>>
> 
> Sorry, what exactly do you mean by condition?

Ugh, I also cannot parse it now...

> When the property 
> "fsl,suspend-in-wait" is set the watchdog is suspended in WAIT mode, so 
> this is defined by the user. Didn't want to apply it for all the 
> supported machines since there could be devices which depend on watchdog 
> triggering in WAIT mode. We stumbled on this problem on imx6 devices, 
> but the same bit (with the same description) is found on imx25, imx35, 
> imx50/51/53, imx7 and imx8.

I meant, what is expected to happen if you do not enable this bit and
watchdog triggers in WAIT mode? IOW, why someone might want to enable or
disable this property?

> 
>> And what happens else? When it is not suspended in WAIT mode?
>>
> 
> When you put the device in "freeze"/"Suspend-To-Idle" low-power mode the 
> watchdog keeps running and triggers a reset after 128 seconds. So the 
> maximum length the device can stay in this mode is limited to 128 seconds.

And who wakes up the system before 128 seconds? IOW is there a use case
of not enabling this property?

Best regards,
Krzysztof

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

[Andrej Picej]

On 20. 10. 22 14:18, Krzysztof Kozlowski wrote:
> On 20/10/2022 02:23, Andrej Picej wrote:
>> Hi Alexander and Krzysztof,
>>
>> hope I can reply to both questions here.
>>
>> On 19. 10. 22 17:51, Krzysztof Kozlowski wrote:
>>> On 19/10/2022 09:00, Alexander Stein wrote:
>>>> Hello Andrej,
>>>>
>>>> Am Mittwoch, 19. Oktober 2022, 13:17:13 CEST schrieb Andrej Picej:
>>>
>>> Missing commit msg.
>>>
>>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>>>> ---
>>>>>    Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml | 5 +++++
>>>>>    1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml index
>>>>> fb7695515be1..01b3e04e7e65 100644
>>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>>> @@ -55,6 +55,11 @@ 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.
>>>>> +
>>>>>    required:
>>>>>      - compatible
>>>>>      - interrupts
>>>>
>>>> What is the condition the watchdog is suspended in WAIT mode? Is this specific
>>>> to SoC or platform or something else?
>>>>
>>>
>>
>> Sorry, what exactly do you mean by condition?
> 
> Ugh, I also cannot parse it now...
> 
>> When the property
>> "fsl,suspend-in-wait" is set the watchdog is suspended in WAIT mode, so
>> this is defined by the user. Didn't want to apply it for all the
>> supported machines since there could be devices which depend on watchdog
>> triggering in WAIT mode. We stumbled on this problem on imx6 devices,
>> but the same bit (with the same description) is found on imx25, imx35,
>> imx50/51/53, imx7 and imx8.
> 
> I meant, what is expected to happen if you do not enable this bit and
> watchdog triggers in WAIT mode? IOW, why someone might want to enable or
> disable this property?
If this is not enabled and you put the device into the Suspend-to-idle 
mode the device resets after 128 seconds. If not, the device can be left 
in that state for infinite time. I'm guessing you want me to better 
explain the property in device tree docs right?
I can do that in v2.
> 
>>
>>> And what happens else? When it is not suspended in WAIT mode?
>>>
>>
>> When you put the device in "freeze"/"Suspend-To-Idle" low-power mode the
>> watchdog keeps running and triggers a reset after 128 seconds. So the
>> maximum length the device can stay in this mode is limited to 128 seconds.
> 
> And who wakes up the system before 128 seconds? IOW is there a use case
> of not enabling this property?
> 
Well I can think of one, system can be woken up by some other interrupt. 
Like RTC which triggers interrupt (for example every 10s). So if this 
property is left disabled the watchdog can handle errors where other 
wakeup sources don't trigger interrupt or if the system is unable to 
wake from low-power state. In that case the watchdog will do a hard 
reset of the device.

But I'm not really sure if anybody uses this, just wanted to make sure 
that we keep the default behaviour as it is, since this driver is used 
by many devices and for quite some time.

Best regards,
Andrej

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

[Alexander Stein]

Am Donnerstag, 20. Oktober 2022, 14:36:10 CEST schrieb Andrej Picej:
> On 20. 10. 22 14:18, Krzysztof Kozlowski wrote:
> > On 20/10/2022 02:23, Andrej Picej wrote:
> >> Hi Alexander and Krzysztof,
> >> 
> >> hope I can reply to both questions here.
> >> 
> >> On 19. 10. 22 17:51, Krzysztof Kozlowski wrote:
> >>> On 19/10/2022 09:00, Alexander Stein wrote:
> >>>> Hello Andrej,
> >>> 
> >>>> Am Mittwoch, 19. Oktober 2022, 13:17:13 CEST schrieb Andrej Picej:
> >>> Missing commit msg.
> >>> 
> >>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> >>>>> ---
> >>>>> 
> >>>>>    Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml | 5
> >>>>>    +++++
> >>>>>    1 file changed, 5 insertions(+)
> >>>>> 
> >>>>> diff --git
> >>>>> a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
> >>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml index
> >>>>> fb7695515be1..01b3e04e7e65 100644
> >>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
> >>>>> 
> >>>>> @@ -55,6 +55,11 @@ 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.
> >>>>> +
> >>>>> 
> >>>>>    required:
> >>>>>      - compatible
> >>>>>      - interrupts
> >>>> 
> >>>> What is the condition the watchdog is suspended in WAIT mode? Is this
> >>>> specific to SoC or platform or something else?
> >> 
> >> Sorry, what exactly do you mean by condition?
> > 
> > Ugh, I also cannot parse it now...

Sorry, Krzysztof already asked the right question: When does one want to 
enable/disable this feature?

> >> When the property
> >> "fsl,suspend-in-wait" is set the watchdog is suspended in WAIT mode, so
> >> this is defined by the user. Didn't want to apply it for all the
> >> supported machines since there could be devices which depend on watchdog
> >> triggering in WAIT mode. We stumbled on this problem on imx6 devices,
> >> but the same bit (with the same description) is found on imx25, imx35,
> >> imx50/51/53, imx7 and imx8.
> > 
> > I meant, what is expected to happen if you do not enable this bit and
> > watchdog triggers in WAIT mode? IOW, why someone might want to enable or
> > disable this property?
> 
> If this is not enabled and you put the device into the Suspend-to-idle
> mode the device resets after 128 seconds. If not, the device can be left
> in that state for infinite time. I'm guessing you want me to better
> explain the property in device tree docs right?
> I can do that in v2.
> 
> >>> And what happens else? When it is not suspended in WAIT mode?
> >> 
> >> When you put the device in "freeze"/"Suspend-To-Idle" low-power mode the
> >> watchdog keeps running and triggers a reset after 128 seconds. So the
> >> maximum length the device can stay in this mode is limited to 128
> >> seconds.
> > 
> > And who wakes up the system before 128 seconds? IOW is there a use case
> > of not enabling this property?
> 
> Well I can think of one, system can be woken up by some other interrupt.
> Like RTC which triggers interrupt (for example every 10s). So if this
> property is left disabled the watchdog can handle errors where other
> wakeup sources don't trigger interrupt or if the system is unable to
> wake from low-power state. In that case the watchdog will do a hard
> reset of the device.
> 
> But I'm not really sure if anybody uses this, just wanted to make sure
> that we keep the default behaviour as it is, since this driver is used
> by many devices and for quite some time.

This sounds more like (application) configuration. If so this should not be 
configured in device tree, IMHO.

Best regards,
Alexander

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

[Andrej Picej]

On 20. 10. 22 14:41, Alexander Stein wrote:
> Am Donnerstag, 20. Oktober 2022, 14:36:10 CEST schrieb Andrej Picej:
>> On 20. 10. 22 14:18, Krzysztof Kozlowski wrote:
>>> On 20/10/2022 02:23, Andrej Picej wrote:
>>>> Hi Alexander and Krzysztof,
>>>>
>>>> hope I can reply to both questions here.
>>>>
>>>> On 19. 10. 22 17:51, Krzysztof Kozlowski wrote:
>>>>> On 19/10/2022 09:00, Alexander Stein wrote:
>>>>>> Hello Andrej,
>>>>>
>>>>>> Am Mittwoch, 19. Oktober 2022, 13:17:13 CEST schrieb Andrej Picej:
>>>>> Missing commit msg.
>>>>>
>>>>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>>>>>> ---
>>>>>>>
>>>>>>>     Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml | 5
>>>>>>>     +++++
>>>>>>>     1 file changed, 5 insertions(+)
>>>>>>>
>>>>>>> diff --git
>>>>>>> a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml index
>>>>>>> fb7695515be1..01b3e04e7e65 100644
>>>>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>>>>>
>>>>>>> @@ -55,6 +55,11 @@ 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.
>>>>>>> +
>>>>>>>
>>>>>>>     required:
>>>>>>>       - compatible
>>>>>>>       - interrupts
>>>>>>
>>>>>> What is the condition the watchdog is suspended in WAIT mode? Is this
>>>>>> specific to SoC or platform or something else?
>>>>
>>>> Sorry, what exactly do you mean by condition?
>>>
>>> Ugh, I also cannot parse it now...
> 
> Sorry, Krzysztof already asked the right question: When does one want to
> enable/disable this feature?
> 
>>>> When the property
>>>> "fsl,suspend-in-wait" is set the watchdog is suspended in WAIT mode, so
>>>> this is defined by the user. Didn't want to apply it for all the
>>>> supported machines since there could be devices which depend on watchdog
>>>> triggering in WAIT mode. We stumbled on this problem on imx6 devices,
>>>> but the same bit (with the same description) is found on imx25, imx35,
>>>> imx50/51/53, imx7 and imx8.
>>>
>>> I meant, what is expected to happen if you do not enable this bit and
>>> watchdog triggers in WAIT mode? IOW, why someone might want to enable or
>>> disable this property?
>>
>> If this is not enabled and you put the device into the Suspend-to-idle
>> mode the device resets after 128 seconds. If not, the device can be left
>> in that state for infinite time. I'm guessing you want me to better
>> explain the property in device tree docs right?
>> I can do that in v2.
>>
>>>>> And what happens else? When it is not suspended in WAIT mode?
>>>>
>>>> When you put the device in "freeze"/"Suspend-To-Idle" low-power mode the
>>>> watchdog keeps running and triggers a reset after 128 seconds. So the
>>>> maximum length the device can stay in this mode is limited to 128
>>>> seconds.
>>>
>>> And who wakes up the system before 128 seconds? IOW is there a use case
>>> of not enabling this property?
>>
>> Well I can think of one, system can be woken up by some other interrupt.
>> Like RTC which triggers interrupt (for example every 10s). So if this
>> property is left disabled the watchdog can handle errors where other
>> wakeup sources don't trigger interrupt or if the system is unable to
>> wake from low-power state. In that case the watchdog will do a hard
>> reset of the device.
>>
>> But I'm not really sure if anybody uses this, just wanted to make sure
>> that we keep the default behaviour as it is, since this driver is used
>> by many devices and for quite some time.
> 
> This sounds more like (application) configuration. If so this should not be
> configured in device tree, IMHO.
> 

Do you have an idea where should it be configured? Just keep in mind 
that this can not be configured at runtime, since this is write-once bit 
so any configuration changes regarding this functionality can not be done.

Basically if I can sum up the problem:

Without this property enabled, the WDW bit is left unset:
$ echo freeze > /sys/power/state
#device enters Suspend-to-idle, watchdog is left running and the device 
resets after 128 seconds in this state

With this property set, the WDW bit is set at watchdog initialization:
$ echo freeze > /sys/power/state
#device enters Suspend-to-idle, watchdog is suspended and the device can 
be left in this state until some other wakeup source triggers interrupt.

Thanks,
Andrej

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

[Krzysztof Kozlowski]

On 20/10/2022 09:05, Andrej Picej wrote:
> 
> 
> On 20. 10. 22 14:41, Alexander Stein wrote:
>> Am Donnerstag, 20. Oktober 2022, 14:36:10 CEST schrieb Andrej Picej:
>>> On 20. 10. 22 14:18, Krzysztof Kozlowski wrote:
>>>> On 20/10/2022 02:23, Andrej Picej wrote:
>>>>> Hi Alexander and Krzysztof,
>>>>>
>>>>> hope I can reply to both questions here.
>>>>>
>>>>> On 19. 10. 22 17:51, Krzysztof Kozlowski wrote:
>>>>>> On 19/10/2022 09:00, Alexander Stein wrote:
>>>>>>> Hello Andrej,
>>>>>>
>>>>>>> Am Mittwoch, 19. Oktober 2022, 13:17:13 CEST schrieb Andrej Picej:
>>>>>> Missing commit msg.
>>>>>>
>>>>>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>     Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml | 5
>>>>>>>>     +++++
>>>>>>>>     1 file changed, 5 insertions(+)
>>>>>>>>
>>>>>>>> diff --git
>>>>>>>> a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>>>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml index
>>>>>>>> fb7695515be1..01b3e04e7e65 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>>>>>>
>>>>>>>> @@ -55,6 +55,11 @@ 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.
>>>>>>>> +
>>>>>>>>
>>>>>>>>     required:
>>>>>>>>       - compatible
>>>>>>>>       - interrupts
>>>>>>>
>>>>>>> What is the condition the watchdog is suspended in WAIT mode? Is this
>>>>>>> specific to SoC or platform or something else?
>>>>>
>>>>> Sorry, what exactly do you mean by condition?
>>>>
>>>> Ugh, I also cannot parse it now...
>>
>> Sorry, Krzysztof already asked the right question: When does one want to
>> enable/disable this feature?
>>
>>>>> When the property
>>>>> "fsl,suspend-in-wait" is set the watchdog is suspended in WAIT mode, so
>>>>> this is defined by the user. Didn't want to apply it for all the
>>>>> supported machines since there could be devices which depend on watchdog
>>>>> triggering in WAIT mode. We stumbled on this problem on imx6 devices,
>>>>> but the same bit (with the same description) is found on imx25, imx35,
>>>>> imx50/51/53, imx7 and imx8.
>>>>
>>>> I meant, what is expected to happen if you do not enable this bit and
>>>> watchdog triggers in WAIT mode? IOW, why someone might want to enable or
>>>> disable this property?
>>>
>>> If this is not enabled and you put the device into the Suspend-to-idle
>>> mode the device resets after 128 seconds. If not, the device can be left
>>> in that state for infinite time. I'm guessing you want me to better
>>> explain the property in device tree docs right?
>>> I can do that in v2.
>>>
>>>>>> And what happens else? When it is not suspended in WAIT mode?
>>>>>
>>>>> When you put the device in "freeze"/"Suspend-To-Idle" low-power mode the
>>>>> watchdog keeps running and triggers a reset after 128 seconds. So the
>>>>> maximum length the device can stay in this mode is limited to 128
>>>>> seconds.
>>>>
>>>> And who wakes up the system before 128 seconds? IOW is there a use case
>>>> of not enabling this property?
>>>
>>> Well I can think of one, system can be woken up by some other interrupt.
>>> Like RTC which triggers interrupt (for example every 10s). So if this
>>> property is left disabled the watchdog can handle errors where other
>>> wakeup sources don't trigger interrupt or if the system is unable to
>>> wake from low-power state. In that case the watchdog will do a hard
>>> reset of the device.
>>>
>>> But I'm not really sure if anybody uses this, just wanted to make sure
>>> that we keep the default behaviour as it is, since this driver is used
>>> by many devices and for quite some time.
>>
>> This sounds more like (application) configuration. If so this should not be
>> configured in device tree, IMHO.
>>
> 
> Do you have an idea where should it be configured? Just keep in mind 
> that this can not be configured at runtime, since this is write-once bit 
> so any configuration changes regarding this functionality can not be done.
> 
> Basically if I can sum up the problem:
> 
> Without this property enabled, the WDW bit is left unset:
> $ echo freeze > /sys/power/state
> #device enters Suspend-to-idle, watchdog is left running and the device 
> resets after 128 seconds in this state

I still wonder (and still did not receive) about such use case. When
would you like to have such behavior?

> 
> With this property set, the WDW bit is set at watchdog initialization:
> $ echo freeze > /sys/power/state
> #device enters Suspend-to-idle, watchdog is suspended and the device can 
> be left in this state until some other wakeup source triggers interrupt.

Assuming there is such use case, for keeping watchdog running even
though system sleeps (and cannot poke watchdog), it's fine.

Best regards,
Krzysztof

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

[Andrej Picej]

On 20. 10. 22 20:23, Krzysztof Kozlowski wrote:
> On 20/10/2022 09:05, Andrej Picej wrote:
>>
>>
>> On 20. 10. 22 14:41, Alexander Stein wrote:
>>> Am Donnerstag, 20. Oktober 2022, 14:36:10 CEST schrieb Andrej Picej:
>>>> On 20. 10. 22 14:18, Krzysztof Kozlowski wrote:
>>>>> On 20/10/2022 02:23, Andrej Picej wrote:
>>>>>> Hi Alexander and Krzysztof,
>>>>>>
>>>>>> hope I can reply to both questions here.
>>>>>>
>>>>>> On 19. 10. 22 17:51, Krzysztof Kozlowski wrote:
>>>>>>> On 19/10/2022 09:00, Alexander Stein wrote:
>>>>>>>> Hello Andrej,
>>>>>>>
>>>>>>>> Am Mittwoch, 19. Oktober 2022, 13:17:13 CEST schrieb Andrej Picej:
>>>>>>> Missing commit msg.
>>>>>>>
>>>>>>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>      Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml | 5
>>>>>>>>>      +++++
>>>>>>>>>      1 file changed, 5 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git
>>>>>>>>> a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>>>>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml index
>>>>>>>>> fb7695515be1..01b3e04e7e65 100644
>>>>>>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>>>>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>>>>>>>
>>>>>>>>> @@ -55,6 +55,11 @@ 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.
>>>>>>>>> +
>>>>>>>>>
>>>>>>>>>      required:
>>>>>>>>>        - compatible
>>>>>>>>>        - interrupts
>>>>>>>>
>>>>>>>> What is the condition the watchdog is suspended in WAIT mode? Is this
>>>>>>>> specific to SoC or platform or something else?
>>>>>>
>>>>>> Sorry, what exactly do you mean by condition?
>>>>>
>>>>> Ugh, I also cannot parse it now...
>>>
>>> Sorry, Krzysztof already asked the right question: When does one want to
>>> enable/disable this feature?
>>>
>>>>>> When the property
>>>>>> "fsl,suspend-in-wait" is set the watchdog is suspended in WAIT mode, so
>>>>>> this is defined by the user. Didn't want to apply it for all the
>>>>>> supported machines since there could be devices which depend on watchdog
>>>>>> triggering in WAIT mode. We stumbled on this problem on imx6 devices,
>>>>>> but the same bit (with the same description) is found on imx25, imx35,
>>>>>> imx50/51/53, imx7 and imx8.
>>>>>
>>>>> I meant, what is expected to happen if you do not enable this bit and
>>>>> watchdog triggers in WAIT mode? IOW, why someone might want to enable or
>>>>> disable this property?
>>>>
>>>> If this is not enabled and you put the device into the Suspend-to-idle
>>>> mode the device resets after 128 seconds. If not, the device can be left
>>>> in that state for infinite time. I'm guessing you want me to better
>>>> explain the property in device tree docs right?
>>>> I can do that in v2.
>>>>
>>>>>>> And what happens else? When it is not suspended in WAIT mode?
>>>>>>
>>>>>> When you put the device in "freeze"/"Suspend-To-Idle" low-power mode the
>>>>>> watchdog keeps running and triggers a reset after 128 seconds. So the
>>>>>> maximum length the device can stay in this mode is limited to 128
>>>>>> seconds.
>>>>>
>>>>> And who wakes up the system before 128 seconds? IOW is there a use case
>>>>> of not enabling this property?
>>>>
>>>> Well I can think of one, system can be woken up by some other interrupt.
>>>> Like RTC which triggers interrupt (for example every 10s). So if this
>>>> property is left disabled the watchdog can handle errors where other
>>>> wakeup sources don't trigger interrupt or if the system is unable to
>>>> wake from low-power state. In that case the watchdog will do a hard
>>>> reset of the device.
>>>>
>>>> But I'm not really sure if anybody uses this, just wanted to make sure
>>>> that we keep the default behaviour as it is, since this driver is used
>>>> by many devices and for quite some time.
>>>
>>> This sounds more like (application) configuration. If so this should not be
>>> configured in device tree, IMHO.
>>>
>>
>> Do you have an idea where should it be configured? Just keep in mind
>> that this can not be configured at runtime, since this is write-once bit
>> so any configuration changes regarding this functionality can not be done.
>>
>> Basically if I can sum up the problem:
>>
>> Without this property enabled, the WDW bit is left unset:
>> $ echo freeze > /sys/power/state
>> #device enters Suspend-to-idle, watchdog is left running and the device
>> resets after 128 seconds in this state
> 
> I still wonder (and still did not receive) about such use case. When
> would you like to have such behavior?
> 

Is this not a valid one?:
>>>>> Well I can think of one, system can be woken up by some other interrupt.
>>>>> Like RTC which triggers interrupt (for example every 10s). So if this
>>>>> property is left disabled the watchdog can handle errors where other
>>>>> wakeup sources don't trigger interrupt or if the system is unable to
>>>>> wake from low-power state. In that case the watchdog will do a hard
>>>>> reset of the device.
>>>>>
>>>>> But I'm not really sure if anybody uses this, just wanted to make sure
>>>>> that we keep the default behaviour as it is, since this driver is used
>>>>> by many devices and for quite some time.

Basically watchdog acting as a supervisor for suspend states.

>>
>> With this property set, the WDW bit is set at watchdog initialization:
>> $ echo freeze > /sys/power/state
>> #device enters Suspend-to-idle, watchdog is suspended and the device can
>> be left in this state until some other wakeup source triggers interrupt.
> 
> Assuming there is such use case, for keeping watchdog running even
> though system sleeps (and cannot poke watchdog), it's fine.
> 

Best regards,
Andrej