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

[linux@roeck-us.net (Guenter Roeck)]

On Fri, Dec 05, 2014 at 10:32:48PM +0200, Timo Kokkonen wrote:
> 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@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@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@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?
> 

Problem is that the possible conditions are all over the place
for "early" watchdog handling.

- Disable watchdog
- Enable watchdog (or keep it enabled), and keep it alive
  until user space kicks in (ie possibly forever)
- Enable watchdog or keep it enabled, and keep it alive
  for a specified period of time.
- Keep watchdog enabled if it is already enabled, otherwise
  keep it disabled.

There are probably more conditions which I don't recall right now.
Which of those conditions would you address with "early-timeout = <0>;" ?
"enable watchdog early and keep it alive until user space kicks in",
or "keep watchdog enabled if already running, and set specified early
timeout" ? One could argue either way which one of the two meanings
it should be.

Thanks,
Guenter