[RFC] improve binding for linux,wdt-gpio

[RFC] improve binding for linux,wdt-gpio

[Uwe Kleine-König]

This is just a suggestion up to now, I don't have any code yet to share.

Apart from minor rewording to make the document easier to understand and
less ambiguous the relevant changes are:

 - add an "enable-gpio" property.
   I admit the device I'm currently working with doesn't have this.
   Still I imagine this to be a common hardware property. I added it
   mainly to demonstrate the shortcomings of the existing binding.
 - rename "gpios" to "trigger-gpio"
   This is more descriptive. And given there is "enable-gpio" now, too,
   using plain "gpios" seems wrong.
 - deprecate always-running
   Apart from the description describing the driver behaviour and not
   the device property, always-running only seems to make sense in
   combination with hw_algo = "level" and in reality should only
   invalidate the sentence: "The watchdog timer is disabled when GPIO is
   left floating or connected to a three-state buffer."
   With this semantic using a property "disable-on-tri-state" sounds
   more sensible. And note that even the following would make sense
   hardware-wise, while the device tree looks stupid:

	watchdog {
		compatible = "linux,wdt-gpio";
		trigger-gpio = ...;
		hw_algo = "toggle";
		always-running;
		enable-gpio = ...;
	};

   (i.e. always-running, but disable possible by a dedicated gpio.)

I'm aware that using ...-gpios is more common than ...-gpio. I don't
feel strong here, but as only a single gpio makes sense here, having
-gpios seems wrong.

Also I considered renaming hw_margin_ms and hw_algo to use - instead of
_. Doing this might even ease to implement the changes above in a
compatible way. I.e. assume the watchdog can be disabled by tristating
the gpio if the description uses underscores instead of hyphen.

Feedback very welcome!

Best regards
Uwe

---
 .../devicetree/bindings/watchdog/gpio-wdt.txt      | 37 ++++++++++++++--------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
index 198794963786..ceb1a5f95f44 100644
--- a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
@@ -2,27 +2,36 @@
 
 Required Properties:
 - compatible: Should contain "linux,wdt-gpio".
-- gpios: From common gpio binding; gpio connection to WDT reset pin.
-- hw_algo: The algorithm used by the driver. Should be one of the
+- trigger-gpio: reference to the gpio connected to watchdog's input pin
+  (typically called WDI).
+- hw_algo: The algorithm used by the device. Should be one of the
   following values:
-  - toggle: Either a high-to-low or a low-to-high transition clears
-    the WDT counter. The watchdog timer is disabled when GPIO is
-    left floating or connected to a three-state buffer.
-  - level: Low or high level starts counting WDT timeout,
-    the opposite level disables the WDT. Active level is determined
-    by the GPIO flags.
-- hw_margin_ms: Maximum time to reset watchdog circuit (milliseconds).
+  - toggle: Both a high-to-low and a low-to-high transition clear
+    the watchdog counter.
+  - level: Low or high level starts counting watchdog timeout,
+    the opposite level disables the watchdog. Active level is determined
+    by the GPIO flags of the trigger-gpio (with active meaning the watchdog is
+    enabled).
+- hw_margin_ms: Maximum time to reset watchdog circuit in milliseconds.
 
 Optional Properties:
-- always-running: If the watchdog timer cannot be disabled, add this flag to
-  have the driver keep toggling the signal without a client. It will only cease
-  to toggle the signal when the device is open and the timeout elapsed.
+- enable-gpio: Reference to a gpio that when inactive stops the watchdog.
+- disable-on-tri-state: property that signals that the watchdog can be stopped
+  by setting the trigger-gpio to tri-state.
+
+Deprecated Properties:
+- always-running: This property signals the watchdog timer cannot be disabled.
+  Without this property the watchdog is expected to turn off for hw_algo=toggle
+  watchdogs when the gpio is set to tri-state.
+  Don't use it for new device descriptions as it is misleading in the presence
+  of an enable-gpio.
+- gpios: deprecated alias of trigger-gpio
 
 Example:
 	watchdog: watchdog {
 		/* ADM706 */
-		compatible = "linux,wdt-gpio";
-		gpios = <&gpio3 9 GPIO_ACTIVE_LOW>;
+		compatible = "adi,adm706", "linux,wdt-gpio";
+		trigger-gpio = <&gpio3 9 GPIO_ACTIVE_LOW>;
 		hw_algo = "toggle";
 		hw_margin_ms = <1600>;
 	};
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] improve binding for linux,wdt-gpio

[Guenter Roeck]

Hi Uwe,

On Tue, Jul 28, 2015 at 10:33:48PM +0200, Uwe Kleine-König wrote:
> This is just a suggestion up to now, I don't have any code yet to share.
> 
> Apart from minor rewording to make the document easier to understand and
> less ambiguous the relevant changes are:
> 
>  - add an "enable-gpio" property.
>    I admit the device I'm currently working with doesn't have this.
>    Still I imagine this to be a common hardware property. I added it
>    mainly to demonstrate the shortcomings of the existing binding.
>  - rename "gpios" to "trigger-gpio"
>    This is more descriptive. And given there is "enable-gpio" now, too,
>    using plain "gpios" seems wrong.
>  - deprecate always-running
>    Apart from the description describing the driver behaviour and not
>    the device property, always-running only seems to make sense in
>    combination with hw_algo = "level" and in reality should only
>    invalidate the sentence: "The watchdog timer is disabled when GPIO is
>    left floating or connected to a three-state buffer."

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.

Of course, 'always-running' _may_ describe driver behavior, but that doesn't
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.

That gets us into the rat-hole of arguing if property X describes driver
behavior or the hardware, and of rejecting properties because they may
be driver descriptions in some use cases (because 'always-running' can
be set even if the hardware doesn't mandate it, making it driver behavior).
I'd rather not go there.

>    With this semantic using a property "disable-on-tri-state" sounds
>    more sensible. And note that even the following would make sense
>    hardware-wise, while the device tree looks stupid:
> 
> 	watchdog {
> 		compatible = "linux,wdt-gpio";
> 		trigger-gpio = ...;
> 		hw_algo = "toggle";
> 		always-running;
> 		enable-gpio = ...;
> 	};
> 
>    (i.e. always-running, but disable possible by a dedicated gpio.)
> 
It might also mean that _enable_ is possible with a dedicated gpio.
That doesn't mean it can be stopped once started. Again, there are lots
of watchdogs out there which can be enabled/started but not stopped.

> I'm aware that using ...-gpios is more common than ...-gpio. I don't
> feel strong here, but as only a single gpio makes sense here, having
> -gpios seems wrong.
> 
Documentation/devicetree/bindings/gpio/gpio.txt states that gpio pin
references should be named <name>-gpios. It even lists examples such as

	enable-gpios = <&gpio2 2>;

I thought this was a hard rule, and I seem to recall requests to change
something-gpio to something-spios, but I may be wrong.

Thanks,
Guenter

> Also I considered renaming hw_margin_ms and hw_algo to use - instead of
> _. Doing this might even ease to implement the changes above in a
> compatible way. I.e. assume the watchdog can be disabled by tristating
> the gpio if the description uses underscores instead of hyphen.
> 

> Feedback very welcome!
> 
> Best regards
> Uwe
> 
> ---
>  .../devicetree/bindings/watchdog/gpio-wdt.txt      | 37 ++++++++++++++--------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> index 198794963786..ceb1a5f95f44 100644
> --- a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> @@ -2,27 +2,36 @@
>  
>  Required Properties:
>  - compatible: Should contain "linux,wdt-gpio".
> -- gpios: From common gpio binding; gpio connection to WDT reset pin.
> -- hw_algo: The algorithm used by the driver. Should be one of the
> +- trigger-gpio: reference to the gpio connected to watchdog's input pin
> +  (typically called WDI).
> +- hw_algo: The algorithm used by the device. Should be one of the
>    following values:
> -  - toggle: Either a high-to-low or a low-to-high transition clears
> -    the WDT counter. The watchdog timer is disabled when GPIO is
> -    left floating or connected to a three-state buffer.
> -  - level: Low or high level starts counting WDT timeout,
> -    the opposite level disables the WDT. Active level is determined
> -    by the GPIO flags.
> -- hw_margin_ms: Maximum time to reset watchdog circuit (milliseconds).
> +  - toggle: Both a high-to-low and a low-to-high transition clear
> +    the watchdog counter.
> +  - level: Low or high level starts counting watchdog timeout,
> +    the opposite level disables the watchdog. Active level is determined
> +    by the GPIO flags of the trigger-gpio (with active meaning the watchdog is
> +    enabled).
> +- hw_margin_ms: Maximum time to reset watchdog circuit in milliseconds.
>  
>  Optional Properties:
> -- always-running: If the watchdog timer cannot be disabled, add this flag to
> -  have the driver keep toggling the signal without a client. It will only cease
> -  to toggle the signal when the device is open and the timeout elapsed.
> +- enable-gpio: Reference to a gpio that when inactive stops the watchdog.
> +- disable-on-tri-state: property that signals that the watchdog can be stopped
> +  by setting the trigger-gpio to tri-state.
> +
> +Deprecated Properties:
> +- always-running: This property signals the watchdog timer cannot be disabled.
> +  Without this property the watchdog is expected to turn off for hw_algo=toggle
> +  watchdogs when the gpio is set to tri-state.
> +  Don't use it for new device descriptions as it is misleading in the presence
> +  of an enable-gpio.
> +- gpios: deprecated alias of trigger-gpio
>  
>  Example:
>  	watchdog: watchdog {
>  		/* ADM706 */
> -		compatible = "linux,wdt-gpio";
> -		gpios = <&gpio3 9 GPIO_ACTIVE_LOW>;
> +		compatible = "adi,adm706", "linux,wdt-gpio";
> +		trigger-gpio = <&gpio3 9 GPIO_ACTIVE_LOW>;
>  		hw_algo = "toggle";
>  		hw_margin_ms = <1600>;
>  	};
> -- 
> 2.1.4
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] improve binding for linux,wdt-gpio

[Uwe Kleine-König]

Hello Guenter,

On Tue, Jul 28, 2015 at 02:21:55PM -0700, Guenter Roeck wrote:
> On Tue, Jul 28, 2015 at 10:33:48PM +0200, Uwe Kleine-König wrote:
> > This is just a suggestion up to now, I don't have any code yet to share.
> > 
> > Apart from minor rewording to make the document easier to understand and
> > less ambiguous the relevant changes are:
> > 
> >  - add an "enable-gpio" property.
> >    I admit the device I'm currently working with doesn't have this.
> >    Still I imagine this to be a common hardware property. I added it
> >    mainly to demonstrate the shortcomings of the existing binding.
> >  - rename "gpios" to "trigger-gpio"
> >    This is more descriptive. And given there is "enable-gpio" now, too,
> >    using plain "gpios" seems wrong.
> >  - deprecate always-running
> >    Apart from the description describing the driver behaviour and not
> >    the device property, always-running only seems to make sense in
> >    combination with hw_algo = "level" and in reality should only
> >    invalidate the sentence: "The watchdog timer is disabled when GPIO is
> >    left floating or connected to a three-state buffer."
> 
> 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.

> 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.

I'd go for these properties then:

	toggle-gpio: the gpio used to stroke the watchdog
	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 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.
For hw_algo = "level" there is probably no device with an enable input
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.)

> That gets us into the rat-hole of arguing if property X describes driver
> behavior or the hardware, and of rejecting properties because they may
> be driver descriptions in some use cases (because 'always-running' can
> be set even if the hardware doesn't mandate it, making it driver behavior).
> I'd rather not go there.
I think we agree here, that "always-running" is a hardware property.

> >    With this semantic using a property "disable-on-tri-state" sounds
> >    more sensible. And note that even the following would make sense
> >    hardware-wise, while the device tree looks stupid:
> > 
> > 	watchdog {
> > 		compatible = "linux,wdt-gpio";
> > 		trigger-gpio = ...;
> > 		hw_algo = "toggle";
> > 		always-running;
> > 		enable-gpio = ...;
> > 	};
> > 
> >    (i.e. always-running, but disable possible by a dedicated gpio.)
> > 
> It might also mean that _enable_ is possible with a dedicated gpio.
> That doesn't mean it can be stopped once started. Again, there are lots
> of watchdogs out there which can be enabled/started but not stopped.
> 
> > I'm aware that using ...-gpios is more common than ...-gpio. I don't
> > feel strong here, but as only a single gpio makes sense here, having
> > -gpios seems wrong.
> > 
> Documentation/devicetree/bindings/gpio/gpio.txt states that gpio pin
> references should be named <name>-gpios. It even lists examples such as
> 
> 	enable-gpios = <&gpio2 2>;
> 
> I thought this was a hard rule, and I seem to recall requests to change
> something-gpio to something-spios, but I may be wrong.
Yeah, I'm aware of that. I talked about that in #armlinux yesterday, and
Mark Brown (added to Cc:) said:

	Well, I'd prefer to change the standard TBH and allow singular.
	This keeps coming up and causing confusion for no good reason.

Sounds sensible in my ears.

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] improve binding for linux,wdt-gpio

[Guenter Roeck]

Hi Uwe,

On 07/29/2015 12:35 AM, Uwe Kleine-König wrote:
> Hello Guenter,
>
[ ... ]
>>
>> 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
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-...).

>> 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' ?).

We have to be careful not to mix up hw_algo and enable, though.

> 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.

> For hw_algo = "level" there is probably no device with an enable input

Why should that be the case ?

> 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.

>>> I'm aware that using ...-gpios is more common than ...-gpio. I don't
>>> feel strong here, but as only a single gpio makes sense here, having
>>> -gpios seems wrong.
>>>
>> Documentation/devicetree/bindings/gpio/gpio.txt states that gpio pin
>> references should be named <name>-gpios. It even lists examples such as
>>
>> 	enable-gpios = <&gpio2 2>;
>>
>> I thought this was a hard rule, and I seem to recall requests to change
>> something-gpio to something-spios, but I may be wrong.
> Yeah, I'm aware of that. I talked about that in #armlinux yesterday, and
> Mark Brown (added to Cc:) said:
>
> 	Well, I'd prefer to change the standard TBH and allow singular.
> 	This keeps coming up and causing confusion for no good reason.
>
> Sounds sensible in my ears.
>

Makes sense to me, but I'd like to see that done first.

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] improve binding for linux,wdt-gpio

[Uwe Kleine-König]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] improve binding for linux,wdt-gpio

[Guenter Roeck]

Hi Uwe,

On 07/29/2015 11:59 PM, Uwe Kleine-König wrote:
> 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.
>

It might be that you put too much emphasis on the documentation and
little or none on the code. We already established that the documentation
is less than perfect. It might make sense to clean up the documentation
first to ensure that it matches what it is actually meant to document.

Sure, I understand, the devicetree bindings are supposed to be
implementation independent. We all know that this is an ideal view
which often does not match reality, as the code tends to be written
first and the devicetree bindings tend to be an afterthought.

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] improve binding for linux,wdt-gpio

[Uwe Kleine-König]

Hello Guenter,

On Thu, Jul 30, 2015 at 12:25:07AM -0700, Guenter Roeck wrote:
> On 07/29/2015 11:59 PM, Uwe Kleine-König wrote:
> >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.
> >
> 
> It might be that you put too much emphasis on the documentation and
> little or none on the code. We already established that the documentation
> is less than perfect. It might make sense to clean up the documentation
> first to ensure that it matches what it is actually meant to document.
I'm not sure you wrote here what you intended to write?!
 
> Sure, I understand, the devicetree bindings are supposed to be
> implementation independent. We all know that this is an ideal view
> which often does not match reality, as the code tends to be written
> first and the devicetree bindings tend to be an afterthought.
I think for drivers that work with a single type of device it works well
the other way round. But for something as generic as a gpio watchdog
that is supposed to drive several different types of devices I think
agreeing on which way to go is more time saving than to come up with an
implementation first that then needs change on feedback.

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] improve binding for linux,wdt-gpio

[Mike Looijmans]

On 29-07-15 09:35, Uwe Kleine-König wrote:
> Hello Guenter,
>
> On Tue, Jul 28, 2015 at 02:21:55PM -0700, Guenter Roeck wrote:
>> On Tue, Jul 28, 2015 at 10:33:48PM +0200, Uwe Kleine-König wrote:
>>> This is just a suggestion up to now, I don't have any code yet to share.
>>>
>>> Apart from minor rewording to make the document easier to understand and
>>> less ambiguous the relevant changes are:
>>>
>>>   - add an "enable-gpio" property.
>>>     I admit the device I'm currently working with doesn't have this.
>>>     Still I imagine this to be a common hardware property. I added it
>>>     mainly to demonstrate the shortcomings of the existing binding.
>>>   - rename "gpios" to "trigger-gpio"
>>>     This is more descriptive. And given there is "enable-gpio" now, too,
>>>     using plain "gpios" seems wrong.
>>>   - deprecate always-running
>>>     Apart from the description describing the driver behaviour and not
>>>     the device property, always-running only seems to make sense in
>>>     combination with hw_algo = "level" and in reality should only
>>>     invalidate the sentence: "The watchdog timer is disabled when GPIO is
>>>     left floating or connected to a three-state buffer."
>>
>> 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.
>
>> 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.
>
> I'd go for these properties then:
>
> 	toggle-gpio: the gpio used to stroke the watchdog
> 	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 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.

I don't see any change in the number of properties required to describe 
things. So the driver just gets more complicated, especially if you want it to 
be somewhat backward compatible.

The way you describe how one could get the "always-running" effect doesn't 
really sound intuitive. It's much easier to just have a property that plainly 
explains that the watchdog is unstoppable, than that that is the result of 
combining a bunch of seemingly unrelated properties together to get the driver 
to do what needs to be done.

>
>> 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.
> For hw_algo = "level" there is probably no device with an enable input
> 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.)
>
>> That gets us into the rat-hole of arguing if property X describes driver
>> behavior or the hardware, and of rejecting properties because they may
>> be driver descriptions in some use cases (because 'always-running' can
>> be set even if the hardware doesn't mandate it, making it driver behavior).
>> I'd rather not go there.
> I think we agree here, that "always-running" is a hardware property.

I'd that "always-running" describes both. The driver must be always stroking 
(what a nice word) the watchdog because the watchdog is always watching.

>>> ...
>



Kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
Telefax: +31 (0) 499 33 69 70
E-mail: mike.looijmans-yhtFebqMsb9it5bFGTN0CAC/G2K4zDHf@public.gmane.org
Website: www.topicproducts.com

Please consider the environment before printing this e-mail





--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] improve binding for linux,wdt-gpio

[Uwe Kleine-König]

Hello Mike,

On Thu, Jul 30, 2015 at 08:15:30AM +0200, Mike Looijmans wrote:
> >I'd go for these properties then:
> >
> >	toggle-gpio: the gpio used to stroke the watchdog
> >	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 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.
> 
> I don't see any change in the number of properties required to
> describe things. So the driver just gets more complicated,
> especially if you want it to be somewhat backward compatible.
I think the backward compatibility stuff isn't that hard to implement,
but I'd like to first agree on a sane description.

> The way you describe how one could get the "always-running" effect
> doesn't really sound intuitive. It's much easier to just have a
> property that plainly explains that the watchdog is unstoppable,
> than that that is the result of combining a bunch of seemingly
> unrelated properties together to get the driver to do what needs to
> be done.
It's not more complicated. In the state as currently documented you
have:

	def disable():
		if (has_property(always-running)):
			it's unstoppable
		elif hw_algo == "toggle":
			set to tri-state
		elif hw_algo == "level":
			set to inactive

With my suggested binding it's:

	def disable():
		if (has_property(disable-on-tri-state)):
			set to tri-state
		elif (has_property(enable-gpio)):
			set enable-gpio to inactive
		else:
			it's unstoppable

This is quite similar, but it handles more hardware (i.e. the chips with
an enable gpio). The only added difficulty I see is that you might have
to handle toggle-gpio (or input-gpio in the latest suggestion) ==
enable-gpio to model chips that are disabled by a certain level of the
input gpio (as currently modelled by hw_algo = level without
always-running).

(So yes, it's easier with the current model to find out if the watchdog
is unstoppable but that alone doesn't help you.)

> >I think we agree here, that "always-running" is a hardware property.
> 
> I'd that "always-running" describes both. The driver must be always
> stroking (what a nice word) the watchdog because the watchdog is
> always watching.
The driver-part is about policy. Maybe you'd prefer the driver to not
autostroke because it should reset the machine instead.

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] improve binding for linux,wdt-gpio

[Uwe Kleine-König]

Hello,

while hacking on the gpio driver I found yet another ambiguity:

On Tue, Jul 28, 2015 at 10:33:48PM +0200, Uwe Kleine-König wrote:
> -  - level: Low or high level starts counting WDT timeout,
> -    the opposite level disables the WDT. Active level is determined
> -    by the GPIO flags.
The driver as is does the following when

	gpio = <&gpiocontroler 5 GPIO_ACTIVE_LOW>

is specified in the device tree:

	priv->active_low = flags & OF_GPIO_ACTIVE_LOW;

So active_low is 1 for us with the above device tree snippet.

Then in the disable callback the driver does:

	gpio_set_value_cansleep(priv->gpio, !priv->active_low);

so the gpio is set to 0 to disable the chip. I would have expected it
the other way round! (Is this only me?) So the documentation should
point that out that the active level of the GPIO is supposed to disable the
chip.

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html