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

[Andrej Picej <andrej.picej@xxxxxxxxx>]

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@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.
Ok, lets see what @Krzysztof has to say about this.

Best regards,
Andrej