Hello Guenter,
On Wed, Jul 29, 2015 at 08:49:23AM -0700, Guenter Roeck wrote:
> On 07/29/2015 12:35 AM, Uwe Kleine-König wrote:
> >>always-running is meant to indicate that the watchdog can not be stopped
> >>(meaning a timer has to be used to send keepalives while the watchdog
> >>device is closed). The documentation specifically states that.
> >>
> >> "If the watchdog timer cannot be disabled ..."
> >>
> >>How would you express that condition without always-running or a similar
> >>attribute ? I am also not sure how that relates to hw_algo; I thought
> >>those properties are orthogonal.
> >For hw_algo = "level" the inactive level of the gpio disables the
> >watchdog (and resets the counter). So always-running doesn't make sense
> >for this type.
> >
> That is not what is currently implemented. "level" just means that
I'm not talking (yet) about the implementation.
> the watchdog is pinged using a -high-low-high- or -low-high-low-
> sequence, while toggle means that the level is changed with each
> ping (-low-(wait)-high-(wait)-low-(wait)-high-...).
Currently the document tells us:
hw_algo: [...]
- level: Low or high level starts counting WDT timeout, the
opposite level disables the WDT.
So similar to the "toggle" description there is some behaviour about
being disabled in certain states implied.
Keeping compatibility code to handle the current state left aside the
needed properties are more intuitive (maybe subjective?) and also more
orthogonal as follows:
input-gpio = reference to gpio connected to WDI
reset-counter = "raising-edge" | "falling-edge" | "edge"
(maybe the first two can be combined with taking the active flag
of the gpio into account ("edge" | "any-edge"))
optional: enable-gpio = reference to gpio connected to an enable pin
that can be used to enable and disable the watchdog
optional: startup-gpio = reference to gpio connected to an enable pin
that can be used to enable but not disable the watchdog. (I
still doubt that this exists in the wild but still.)
optional: disable-on-tri-state = property that signals that the
watchdog can be stopped by setting the input-gpio to tri-state.
This way the three optional properties conflict each other (pairwise)
and the meaning of reset-counter and the three optional properties is
not modified by the presence of another property.
> >>Of course, 'always-running' _may_ describe driver behavior, but that doesn't
> >Well in the current state of the binding document we have:
> >
> > add this flag to have the driver keep toggling the signal
> > without a client.
> >
> >Sure it is meant to describe a hardware specific property, but it talks
> >about the driver.
> >
> Then the fix is to update the binding document.
>
> >I'd go for these properties then:
> >
> > toggle-gpio: the gpio used to stroke the watchdog
>
> 'toggle' means something different in the current implementation.
>
> > enable-gpio: optional, the gpio to enable and disable the watchdog
> > disable-on-tri-state: optional, signals that the watchdog can
> > be stopped by setting the trigger-gpio to tri-state.
> > compatible, hw_algo and hw_margin_ms: as before.
> >
>
> I would agree that a separate 'enable' property would make sense (if you
> have a watchdog needing it). Similar to disable-on-tri-state, if there
> is some hardware which is implemented that way (mixing up hw_algo==toggle
> with that state doesn't seem correct). Deprecating hw_algo and replacing it
> with something more sensible might make sense as well ('algorithm' ?).
algorithm is IMHO better than "hw_algo". What do you think about
reset-counter as suggested above?
> We have to be careful not to mix up hw_algo and enable, though.
fine, so we agree here.
> >I think there is no need for a property that signals that the watchdog
> >is unstoppable. For level-gpio-watchdogs you can do it by setting the
> >trigger gpio to inactive, and you cannot stop level-gpio-watchdogs
> >unless enable-gpio or disable-on-tri-state is specified.
> >If we ever feel the need to describe a gpio watchdog with a input that
> >starts the device but cannot stop it, I'd suggest to use "start-gpio"
> >for that one.
> >
> >>have to be the case. There are lots of watchdogs out there which can not be
> >>stopped. Some of them run all the time, others can not be stopped once
> >>started.
> >Yeah, I'm aware of that. For this driver however I wouldn't expect that
> >you have a dedicated enable-gpio if you cannot disable the device with it.
>
> Why ? There are lots of chips which implement exactly that. There is an
> enable bit in some register which can be used to enable the watchdog,
> but once enabled it can not be stopped. I don't see why a gpio driven
> watchdog would have to be any different.
I doubt it, because you need an extra line then for a task that is only
used once per boot. And it's straight forward to do the startup on the
first ping.
> >and for hw_algo = "toggle" any sane hardware engineer would simply
> >enable the watchdog once the first toggle is detected on WDI. (OK,
> >assuming hardware engineers being sane turned out to be a weak argument
> >often in the past.)
> >
> I still don't see the relationship between enable and the toggle/level
> algorithm. Really, those two properties are orthogonal.
They should. But they are not as currently documented (and implemented).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
