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

[Andrej Picej <andrej.picej@norik.com>]

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