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

[Andrej Picej <andrej.picej@norik.com>]

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