[PATCH] at91sam9_wdt: Allow watchdog to reset device at early boot

[Timo Kokkonen]

On 05.12.2014 21:02, Guenter Roeck wrote:
> On Fri, Dec 05, 2014 at 08:42:07PM +0200, Timo Kokkonen wrote:
>> On 05.12.2014 16:12, Boris Brezillon wrote:
>>> Hi Timo,
>>>
>>> On Fri, 05 Dec 2014 14:57:05 +0200
>>> Timo Kokkonen <timo.kokkonen at offcode.fi> wrote:
>>>
>>>> On 27.11.2014 21:00, Boris Brezillon wrote:
>>>>> Hi Timo,
>>>>>
>>>>> Sorry for the late reply.
>>>>>
>>>>> On Fri, 21 Nov 2014 14:23:30 +0200
>>>>> Timo Kokkonen <timo.kokkonen at offcode.fi> wrote:
>>>>>
>>>>>> By default the driver will start a kernel timer which keeps on kicking
>>>>>> the watchdog HW until user space has opened the watchdog
>>>>>> device. Usually this is desirable as the watchdog HW is running by
>>>>>> default and the user space may not have any watchdog daemon running at
>>>>>> all.
>>>>>>
>>>>>> However, on production systems it may be mandatory that also early
>>>>>> crashes and lockups will lead to a watchdog reset, even if they happen
>>>>>> before the user space has opened the watchdog device.
>>>>>>
>>>>>> To resolve the issue, add a new device tree property
>>>>>> "enable-early-reset" which will prevent the kernel timer from pinging
>>>>>> the watchdog HW on behalf of user space. The default is still to use
>>>>>> kernel timer, but more strict behavior can be enabled via the device
>>>>>> tree property.
>>>>>>
>>>>>> Signed-off-by: Timo Kokkonen <timo.kokkonen at offcode.fi>
>>>>>> ---
>>>>>>    Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++
>>>>>>    drivers/watchdog/at91sam9_wdt.c                          | 6 +++++-
>>>>>>    2 files changed, 9 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
>>>>>> index f90e294..a0b7b75 100644
>>>>>> --- a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
>>>>>> +++ b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
>>>>>> @@ -28,6 +28,10 @@ Optional properties:
>>>>>>    	entering idle state.
>>>>>>    - atmel,dbg-halt : Should be present if you want to stop the watchdog when
>>>>>>    	entering debug state.
>>>>>> +- enable-early-reset : Should be present if you want to let the
>>>>>> +	watchdog timer to expire even before user space has opened the
>>>>>> +	device. If not set, a kernel timer will keep on pinging the
>>>>>> +	watchdog until it is opened.
>>>>>
>>>>> If you want to make this property generic, maybe you should document it
>>>>> in a generic binding doc
>>>>> (Documentation/devicetree/bindings/watchdog/watchdog.txt ?).
>>>>> Once you're at it, maybe you could document the generic timeout-sec
>>>>> property in this file.
>>>>> Moreover, you might want to parse this property in watchdog_core.c and
>>>>> store the information in the watchdog_device struct.
>>>>
>>>> I gave a little thought about this today and we could maybe have it like
>>>> watchdog_init_timeout() is today. But I can't really think of any
>>>> generic handling for this property, anything else except storing the
>>>> parsed value to a variable in watchdog_device struct. Everything else is
>>>> HW specific, except how we read the variable.. If we had some logic or
>>>> checking for this variable (other than to check it is not negative)
>>>> maybe then it would make sense.
>>>
>>> Okay, I'm fine with keeping this DT property parsing out of the core.
>>
>> Ok, good.
>>
>>>>
>>>> So I could write a patch to document generic watchdog properties in
>>>> Documentation/devicetree/bindings/watchdog/watchdog.txt (anything else
>>>> generic that would go there except timeout-sec and enable-early-reset?)
>>>
>>> I'm not sure you and Guenter agreed on the 'early-keepalive-sec'
>>> property (and this one won't be used in atmel driver since you can't
>>> change the timeout once set), so I'd say the timeout-sec and
>>> enable-early-reset are the only common properties for now.
>>
>> Actually it can be done in Atmel driver as well. It already has a
>> timer that keeps on pinging the watchdog until user space opens it.
>> We just modify the code so that it keeps on pinging the watchdog
>> until early-keepalive-sec has expired. No need to change the HW
>> timeouts at all. And I rather have it implemented that way anyway,
>> the 16 second timeout is a little tight if there happens to be a lot
>> of stuff already in the early user space.
>>
>> Yeah, I didn't get anything back from Guenter about my
>> early-keepalive-sec property proposal. Frankly, I already forgot
>> about it my self already. I guess I'll get back to you with v3 set
>> once I get time for it. Probably easiest to continue from there.
>>
> Sorry for that, folks. I have been a bit overwhelmed in the last cycle.
>
> early-timeout-sec seems to be the best property name to me right now.
> It is aligned with the existing timeout-sec. early-keepalive-sec seems
> kind of different just for the purpose of being different.

early-timeout-sec sounds good to me. I'll go with that.

> Not sure about how to name enable-early-reset. I'd prefer to have something
> generic, even if only implemented in a single driver for now, but I don't
> really know right now what that might/should look like. Maybe just
> "enable-early" to indicate that the watchdog should be enabled during init ?

Do we need the enable-early or such property at all? Just leave 
early-timeout-sec to zero and then let it behave just like enable-early 
would do?

-Timo