Re: [PATCH v2 1/3] watchdog: imx2_wdg: suspend watchdog in WAIT mode
From: Andrej Picej
Date: Wed Oct 26 2022 - 03:03:43 EST
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