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.
