Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <56af1cc3-c10e-5694-d25f-252304732568@norik.com>
Date:   Tue, 25 Oct 2022 13:21:18 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.2
Subject: Re: [PATCH v2 1/3] watchdog: imx2_wdg: suspend watchdog in WAIT mode
Content-Language: en-GB
To:     Alexander Stein <alexander.stein@ew.tq-group.com>
Cc:     linux-watchdog@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, wim@linux-watchdog.org,
        linux@roeck-us.net, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, shawnguo@kernel.org,
        s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com,
        linux-imx@nxp.com, Anson.Huang@nxp.com, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20221025072533.2980154-1-andrej.picej@norik.com>
 <20221025072533.2980154-2-andrej.picej@norik.com>
 <13126397.uLZWGnKmhe@steina-w>
From:   Andrej Picej <andrej.picej@norik.com>
In-Reply-To: <13126397.uLZWGnKmhe@steina-w>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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