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

[boris.brezillon@free-electrons.com (Boris Brezillon)]

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.

> 
> 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.
 
> and change this patch so that it doesn't document anything in 
> atmel-wdt.txt as we already have it documented in generic watchdog.txt.
> 
> Sounds good?

Yes, sounds good to me.

Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com