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

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

On 20. 10. 22 20:23, Krzysztof Kozlowski wrote:
> 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@norik.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>      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?
> 

Is this not a valid one?:
>>>>> 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.

Basically watchdog acting as a supervisor for suspend states.

>>
>> 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,
Andrej