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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56af1cc3-c10e-5694-d25f-252304732568@norik.com>
Date:   Tue, 25 Oct 2022 13:21:18 +0200
From:   Andrej Picej <andrej.picej@...ik.com>
To:     Alexander Stein <alexander.stein@...tq-group.com>
Cc:     linux-watchdog@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, wim@...ux-watchdog.org,
        linux@...ck-us.net, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, shawnguo@...nel.org,
        s.hauer@...gutronix.de, kernel@...gutronix.de, festevam@...il.com,
        linux-imx@....com, Anson.Huang@....com, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] watchdog: imx2_wdg: suspend watchdog in WAIT mode

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@...ik.com>
>> Reviewed-by: Fabio Estevam <festevam@...il.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?
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.

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.
> 
> 
> 
>