Messages in this thread | | | Date | Wed, 26 Oct 2022 09:03:30 +0200 | Subject | Re: [PATCH v2 1/3] watchdog: imx2_wdg: suspend watchdog in WAIT mode | From | Andrej Picej <> |
| |
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@norik.com> >>>> Reviewed-by: Fabio Estevam <festevam@gmail.com> >>>> --- >>>> >>>> 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
| |