Re: [PATCH 2/3] dt-bindings: watchdog: fsl-imx: document suspend in wait mode

[Krzysztof Kozlowski]

On 20/10/2022 09:05, Andrej Picej wrote:
> 
> 
> On 20. 10. 22 14:41, Alexander Stein wrote:
>> Am Donnerstag, 20. Oktober 2022, 14:36:10 CEST schrieb Andrej Picej:
>>> On 20. 10. 22 14:18, Krzysztof Kozlowski wrote:
>>>> On 20/10/2022 02:23, Andrej Picej wrote:
>>>>> Hi Alexander and Krzysztof,
>>>>>
>>>>> hope I can reply to both questions here.
>>>>>
>>>>> On 19. 10. 22 17:51, Krzysztof Kozlowski wrote:
>>>>>> On 19/10/2022 09:00, Alexander Stein wrote:
>>>>>>> Hello Andrej,
>>>>>>
>>>>>>> Am Mittwoch, 19. Oktober 2022, 13:17:13 CEST schrieb Andrej Picej:
>>>>>> Missing commit msg.
>>>>>>
>>>>>>>> Signed-off-by: Andrej Picej <andrej.picej@xxxxxxxxx>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>     Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml | 5
>>>>>>>>     +++++
>>>>>>>>     1 file changed, 5 insertions(+)
>>>>>>>>
>>>>>>>> diff --git
>>>>>>>> a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>>>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml index
>>>>>>>> fb7695515be1..01b3e04e7e65 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.yaml
>>>>>>>>
>>>>>>>> @@ -55,6 +55,11 @@ properties:
>>>>>>>>           If present, the watchdog device is configured to assert its
>>>>>>>>           external reset (WDOG_B) instead of issuing a software reset.
>>>>>>>>
>>>>>>>> +  fsl,suspend-in-wait:
>>>>>>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>>>>>>> +    description: |
>>>>>>>> +      If present, the watchdog device is suspended in WAIT mode.
>>>>>>>> +
>>>>>>>>
>>>>>>>>     required:
>>>>>>>>       - compatible
>>>>>>>>       - interrupts
>>>>>>>
>>>>>>> What is the condition the watchdog is suspended in WAIT mode? Is this
>>>>>>> specific to SoC or platform or something else?
>>>>>
>>>>> Sorry, what exactly do you mean by condition?
>>>>
>>>> Ugh, I also cannot parse it now...
>>
>> Sorry, Krzysztof already asked the right question: When does one want to
>> enable/disable this feature?
>>
>>>>> When the property
>>>>> "fsl,suspend-in-wait" is set the watchdog is suspended in WAIT mode, so
>>>>> this is defined by the user. Didn't want to apply it for all the
>>>>> supported machines since there could be devices which depend on watchdog
>>>>> triggering in WAIT mode. We stumbled on this problem on imx6 devices,
>>>>> but the same bit (with the same description) is found on imx25, imx35,
>>>>> imx50/51/53, imx7 and imx8.
>>>>
>>>> I meant, what is expected to happen if you do not enable this bit and
>>>> watchdog triggers in WAIT mode? IOW, why someone might want to enable or
>>>> disable this property?
>>>
>>> If this is not enabled and you put the device into the Suspend-to-idle
>>> mode the device resets after 128 seconds. If not, the device can be left
>>> in that state for infinite time. I'm guessing you want me to better
>>> explain the property in device tree docs right?
>>> I can do that in v2.
>>>
>>>>>> And what happens else? When it is not suspended in WAIT mode?
>>>>>
>>>>> When you put the device in "freeze"/"Suspend-To-Idle" low-power mode the
>>>>> watchdog keeps running and triggers a reset after 128 seconds. So the
>>>>> maximum length the device can stay in this mode is limited to 128
>>>>> seconds.
>>>>
>>>> And who wakes up the system before 128 seconds? IOW is there a use case
>>>> of not enabling this property?
>>>
>>> Well I can think of one, system can be woken up by some other interrupt.
>>> Like RTC which triggers interrupt (for example every 10s). So if this
>>> property is left disabled the watchdog can handle errors where other
>>> wakeup sources don't trigger interrupt or if the system is unable to
>>> wake from low-power state. In that case the watchdog will do a hard
>>> reset of the device.
>>>
>>> But I'm not really sure if anybody uses this, just wanted to make sure
>>> that we keep the default behaviour as it is, since this driver is used
>>> by many devices and for quite some time.
>>
>> This sounds more like (application) configuration. If so this should not be
>> configured in device tree, IMHO.
>>
> 
> Do you have an idea where should it be configured? Just keep in mind 
> that this can not be configured at runtime, since this is write-once bit 
> so any configuration changes regarding this functionality can not be done.
> 
> Basically if I can sum up the problem:
> 
> Without this property enabled, the WDW bit is left unset:
> $ echo freeze > /sys/power/state
> #device enters Suspend-to-idle, watchdog is left running and the device 
> resets after 128 seconds in this state

I still wonder (and still did not receive) about such use case. When
would you like to have such behavior?

> 
> With this property set, the WDW bit is set at watchdog initialization:
> $ echo freeze > /sys/power/state
> #device enters Suspend-to-idle, watchdog is suspended and the device can 
> be left in this state until some other wakeup source triggers interrupt.

Assuming there is such use case, for keeping watchdog running even
though system sleeps (and cannot poke watchdog), it's fine.

Best regards,
Krzysztof