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@xxxxxxxxx>
> >> Reviewed-by: Fabio Estevam <festevam@xxxxxxxxx>
> >> ---
> >> 
> >> 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.