[PATCH] Add support for always enabled watchdog timers

[PATCH] Add support for always enabled watchdog timers

[Evgeny Boger]

From: Evgeny Boger <boger@contactless.ru>

Add option to use with watchdog timers which are always enabled 
in hardware, i.e. there is no way to enable/disable it via GPIO pin.
The driver will start pinging WDT immediately upon loading
and will continue to do so even after stopping the watchdog.

Signed-off-by: Evgeny Boger <boger@contactless.ru>
---
 .../devicetree/bindings/watchdog/gpio-wdt.txt      | 14 ++++++-
 drivers/watchdog/gpio_wdt.c                        | 45 +++++++++++++++++-----
 2 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
index 37afec1..1f8ca46 100644
--- a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
@@ -12,8 +12,11 @@ Required Properties:
     the opposite level disables the WDT. Active level is determined
     by the GPIO flags.
 - hw_margin_ms: Maximum time to reset watchdog circuit (milliseconds).
+- always-enabled: Use with wathdog timer which is always enabled
+  in hardware, i.e. there is no way to enable/disable it via GPIO pin.
+  Driver will start pinging WDT immediately upon loading.
 
-Example:
+Examples:
 	watchdog: watchdog {
 		/* ADM706 */
 		compatible = "linux,wdt-gpio";
@@ -21,3 +24,12 @@ Example:
 		hw_algo = "toggle";
 		hw_margin_ms = <1600>;
 	};
+
+	watchdog: watchdog {
+		/* TPS3813 */
+		compatible = "linux,wdt-gpio";
+		gpios = <&gpio3 9 GPIO_ACTIVE_LOW>;
+		hw_algo = "toggle";
+		hw_margin_ms = <10000>;
+		always-enabled;
+	};
diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
index 220a9e0..29f9bff 100644
--- a/drivers/watchdog/gpio_wdt.c
+++ b/drivers/watchdog/gpio_wdt.c
@@ -37,6 +37,8 @@ struct gpio_wdt_priv {
 	struct notifier_block	notifier;
 	struct timer_list	timer;
 	struct watchdog_device	wdd;
+	bool		always_enabled;
+	bool		started;
 };
 
 static void gpio_wdt_disable(struct gpio_wdt_priv *priv)
@@ -48,10 +50,8 @@ static void gpio_wdt_disable(struct gpio_wdt_priv *priv)
 		gpio_direction_input(priv->gpio);
 }
 
-static int gpio_wdt_start(struct watchdog_device *wdd)
+static int gpio_wdt_start_timer(struct gpio_wdt_priv *priv)
 {
-	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
-
 	priv->state = priv->active_low;
 	gpio_direction_output(priv->gpio, priv->state);
 	priv->last_jiffies = jiffies;
@@ -60,12 +60,28 @@ static int gpio_wdt_start(struct watchdog_device *wdd)
 	return 0;
 }
 
-static int gpio_wdt_stop(struct watchdog_device *wdd)
+static int gpio_wdt_start(struct watchdog_device *wdd)
 {
 	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
+	priv->started = true;
+	if (priv->always_enabled) {
+		/* harware ping timer is already enabled */
+		priv->last_jiffies = jiffies;
+	} else {
+		gpio_wdt_start_timer(priv);
+	}
 
-	mod_timer(&priv->timer, 0);
-	gpio_wdt_disable(priv);
+	return 0;
+}
+
+static int gpio_wdt_stop(struct watchdog_device *wdd)
+{
+	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
+	priv->started = false;
+	if (!priv->always_enabled) {
+		mod_timer(&priv->timer, 0);
+		gpio_wdt_disable(priv);
+	}
 
 	return 0;
 }
@@ -91,10 +107,12 @@ static void gpio_wdt_hwping(unsigned long data)
 	struct watchdog_device *wdd = (struct watchdog_device *)data;
 	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
 
-	if (time_after(jiffies, priv->last_jiffies +
-		       msecs_to_jiffies(wdd->timeout * 1000))) {
-		dev_crit(wdd->dev, "Timer expired. System will reboot soon!\n");
-		return;
+	if (priv->started) {
+		if (time_after(jiffies, priv->last_jiffies +
+			       msecs_to_jiffies(wdd->timeout * 1000))) {
+			dev_crit(wdd->dev, "Timer expired. System will reboot soon!\n");
+			return;
+		}
 	}
 
 	/* Restart timer */
@@ -197,6 +215,9 @@ static int gpio_wdt_probe(struct platform_device *pdev)
 	/* Use safe value (1/2 of real timeout) */
 	priv->hw_margin = msecs_to_jiffies(hw_margin / 2);
 
+	priv->always_enabled = of_property_read_bool(pdev->dev.of_node,
+						"always-enabled");
+
 	watchdog_set_drvdata(&priv->wdd, priv);
 
 	priv->wdd.info		= &gpio_wdt_ident;
@@ -207,6 +228,7 @@ static int gpio_wdt_probe(struct platform_device *pdev)
 	if (watchdog_init_timeout(&priv->wdd, 0, &pdev->dev) < 0)
 		priv->wdd.timeout = SOFT_TIMEOUT_DEF;
 
+	priv->started = false;
 	setup_timer(&priv->timer, gpio_wdt_hwping, (unsigned long)&priv->wdd);
 
 	ret = watchdog_register_device(&priv->wdd);
@@ -218,6 +240,9 @@ static int gpio_wdt_probe(struct platform_device *pdev)
 	if (ret)
 		watchdog_unregister_device(&priv->wdd);
 
+	if (priv->always_enabled)
+		gpio_wdt_start_timer(priv);
+
 	return ret;
 }
 
-- 
1.9.1

Re: [PATCH] Add support for always enabled watchdog timers

[Guenter Roeck]

On 08/16/2014 05:45 PM, Evgeny Boger wrote:
> From: Evgeny Boger <boger@contactless.ru>
>
> Add option to use with watchdog timers which are always enabled
> in hardware, i.e. there is no way to enable/disable it via GPIO pin.
> The driver will start pinging WDT immediately upon loading
> and will continue to do so even after stopping the watchdog.
>
The headline needs a reference to the affected driver.

Also, please copy the dt mailing list and maintainers.

> Signed-off-by: Evgeny Boger <boger@contactless.ru>
> ---
>   .../devicetree/bindings/watchdog/gpio-wdt.txt      | 14 ++++++-
>   drivers/watchdog/gpio_wdt.c                        | 45 +++++++++++++++++-----
>   2 files changed, 48 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> index 37afec1..1f8ca46 100644
> --- a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> @@ -12,8 +12,11 @@ Required Properties:
>       the opposite level disables the WDT. Active level is determined
>       by the GPIO flags.
>   - hw_margin_ms: Maximum time to reset watchdog circuit (milliseconds).
> +- always-enabled: Use with wathdog timer which is always enabled

s/wathdog/watchdog/

> +  in hardware, i.e. there is no way to enable/disable it via GPIO pin.
> +  Driver will start pinging WDT immediately upon loading.
>
> -Example:
> +Examples:
>   	watchdog: watchdog {
>   		/* ADM706 */
>   		compatible = "linux,wdt-gpio";
> @@ -21,3 +24,12 @@ Example:
>   		hw_algo = "toggle";
>   		hw_margin_ms = <1600>;
>   	};
> +
> +	watchdog: watchdog {
> +		/* TPS3813 */
> +		compatible = "linux,wdt-gpio";
> +		gpios = <&gpio3 9 GPIO_ACTIVE_LOW>;
> +		hw_algo = "toggle";
> +		hw_margin_ms = <10000>;
> +		always-enabled;
> +	};
> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
> index 220a9e0..29f9bff 100644
> --- a/drivers/watchdog/gpio_wdt.c
> +++ b/drivers/watchdog/gpio_wdt.c
> @@ -37,6 +37,8 @@ struct gpio_wdt_priv {
>   	struct notifier_block	notifier;
>   	struct timer_list	timer;
>   	struct watchdog_device	wdd;
> +	bool		always_enabled;
> +	bool		started;

Please keep indentation aligned with existing code.

>   };
>
>   static void gpio_wdt_disable(struct gpio_wdt_priv *priv)
> @@ -48,10 +50,8 @@ static void gpio_wdt_disable(struct gpio_wdt_priv *priv)
>   		gpio_direction_input(priv->gpio);
>   }
>
> -static int gpio_wdt_start(struct watchdog_device *wdd)
> +static int gpio_wdt_start_timer(struct gpio_wdt_priv *priv)

Unfortunate function name. gpio_wdt_enable would be more appropriate,
or maybe __gpio_wdt_start.

>   {
> -	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
> -
>   	priv->state = priv->active_low;
>   	gpio_direction_output(priv->gpio, priv->state);
>   	priv->last_jiffies = jiffies;
> @@ -60,12 +60,28 @@ static int gpio_wdt_start(struct watchdog_device *wdd)
>   	return 0;
>   }
>
> -static int gpio_wdt_stop(struct watchdog_device *wdd)
> +static int gpio_wdt_start(struct watchdog_device *wdd)
>   {
>   	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
> +	priv->started = true;
> +	if (priv->always_enabled) {
> +		/* harware ping timer is already enabled */

s/harware/hardware/

> +		priv->last_jiffies = jiffies;
> +	} else {
> +		gpio_wdt_start_timer(priv);
> +	}
>
> -	mod_timer(&priv->timer, 0);
> -	gpio_wdt_disable(priv);
> +	return 0;
> +}
> +
> +static int gpio_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
> +	priv->started = false;
> +	if (!priv->always_enabled) {
> +		mod_timer(&priv->timer, 0);
> +		gpio_wdt_disable(priv);
> +	}
>
>   	return 0;
>   }
> @@ -91,10 +107,12 @@ static void gpio_wdt_hwping(unsigned long data)
>   	struct watchdog_device *wdd = (struct watchdog_device *)data;
>   	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
>
> -	if (time_after(jiffies, priv->last_jiffies +
> -		       msecs_to_jiffies(wdd->timeout * 1000))) {
> -		dev_crit(wdd->dev, "Timer expired. System will reboot soon!\n");
> -		return;
> +	if (priv->started) {
> +		if (time_after(jiffies, priv->last_jiffies +
> +			       msecs_to_jiffies(wdd->timeout * 1000))) {
> +			dev_crit(wdd->dev, "Timer expired. System will reboot soon!\n");
> +			return;
> +		}

No need to add another if and indentation level. Just make it

	if (priv->started &&
	    time_after(jiffies, priv->last_jiffies +
		...

	
>   	}
>
>   	/* Restart timer */
> @@ -197,6 +215,9 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>   	/* Use safe value (1/2 of real timeout) */
>   	priv->hw_margin = msecs_to_jiffies(hw_margin / 2);
>
> +	priv->always_enabled = of_property_read_bool(pdev->dev.of_node,
> +						"always-enabled");
> +
>   	watchdog_set_drvdata(&priv->wdd, priv);
>
>   	priv->wdd.info		= &gpio_wdt_ident;
> @@ -207,6 +228,7 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>   	if (watchdog_init_timeout(&priv->wdd, 0, &pdev->dev) < 0)
>   		priv->wdd.timeout = SOFT_TIMEOUT_DEF;
>
> +	priv->started = false;
>   	setup_timer(&priv->timer, gpio_wdt_hwping, (unsigned long)&priv->wdd);
>
>   	ret = watchdog_register_device(&priv->wdd);
> @@ -218,6 +240,9 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>   	if (ret)
>   		watchdog_unregister_device(&priv->wdd);
>
> +	if (priv->always_enabled)
> +		gpio_wdt_start_timer(priv);
> +

This will cause a problem if register_reboot_notifier failed. You'll have
to implement proper error handling for this case, something like the following.

	...
	priv->notifier.notifier_call = gpio_wdt_notify_sys;
	ret = register_reboot_notifier(&priv->notifier);
	if (ret)
		goto unregister;

	if (priv->always_enabled)
		gpio_wdt_enable(priv);

	return 0;

unregister:
	watchdog_unregister_device(&priv->wdd);
	return ret;
}

Guenter

Re: [PATCH] Add support for always enabled watchdog timers

[Alexander Shiyan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 1621 bytes --]

Sat, 23 Aug 2014 10:16:08 -0700 от Guenter Roeck <linux@roeck-us.net>:
> On 08/16/2014 05:45 PM, Evgeny Boger wrote:
> > From: Evgeny Boger <boger@contactless.ru>
> >
> > Add option to use with watchdog timers which are always enabled
> > in hardware, i.e. there is no way to enable/disable it via GPIO pin.
> > The driver will start pinging WDT immediately upon loading
> > and will continue to do so even after stopping the watchdog.
> >
> The headline needs a reference to the affected driver.
> 
> Also, please copy the dt mailing list and maintainers.
> 
> > Signed-off-by: Evgeny Boger <boger@contactless.ru>
> > ---
> >   .../devicetree/bindings/watchdog/gpio-wdt.txt      | 14 ++++++-
> >   drivers/watchdog/gpio_wdt.c                        | 45 +++++++++++++++++-----
> >   2 files changed, 48 insertions(+), 11 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> > index 37afec1..1f8ca46 100644
> > --- a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> > +++ b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> > @@ -12,8 +12,11 @@ Required Properties:
> >       the opposite level disables the WDT. Active level is determined
> >       by the GPIO flags.
> >   - hw_margin_ms: Maximum time to reset watchdog circuit (milliseconds).
> > +- always-enabled: Use with wathdog timer which is always enabled
 
Similar to NOWAYOUT?

---

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] Add support for always enabled watchdog timers

[Guenter Roeck]

On 08/23/2014 10:25 AM, Alexander Shiyan wrote:
> Sat, 23 Aug 2014 10:16:08 -0700 от Guenter Roeck <linux@roeck-us.net>:
>> On 08/16/2014 05:45 PM, Evgeny Boger wrote:
>>> From: Evgeny Boger <boger@contactless.ru>
>>>
>>> Add option to use with watchdog timers which are always enabled
>>> in hardware, i.e. there is no way to enable/disable it via GPIO pin.
>>> The driver will start pinging WDT immediately upon loading
>>> and will continue to do so even after stopping the watchdog.
>>>
>> The headline needs a reference to the affected driver.
>>
>> Also, please copy the dt mailing list and maintainers.
>>
>>> Signed-off-by: Evgeny Boger <boger@contactless.ru>
>>> ---
>>>    .../devicetree/bindings/watchdog/gpio-wdt.txt      | 14 ++++++-
>>>    drivers/watchdog/gpio_wdt.c                        | 45 +++++++++++++++++-----
>>>    2 files changed, 48 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
>>> index 37afec1..1f8ca46 100644
>>> --- a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
>>> +++ b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
>>> @@ -12,8 +12,11 @@ Required Properties:
>>>        the opposite level disables the WDT. Active level is determined
>>>        by the GPIO flags.
>>>    - hw_margin_ms: Maximum time to reset watchdog circuit (milliseconds).
>>> +- always-enabled: Use with wathdog timer which is always enabled
>
> Similar to NOWAYOUT?
>
I think this one is different. NOWAYOUT is a software flag, while
the flag here is supposed to mean that the hardware watchdog
is always enabled and running, and can not be disabled. Or at least
this is my understanding.

Guenter

[PATCH v2] Add support for always enabled watchdog timers to gpio_wdt driver

[Evgeny Boger]

From: Evgeny Boger <boger@contactless.ru>

Add option to use with watchdog timers which are always enabled
in hardware, i.e. there is no way to enable/disable it via GPIO pin.
The driver will start pinging WDT immediately upon loading
and will continue to do so even after stopping the watchdog.

Signed-off-by: Evgeny Boger <boger@contactless.ru>
---

Changes since v1:
* fixed typos and indentation
* gpio_wdt_start_timer renamed to gpio_wdt_start_hwping. 
  This function actually starts the timer to ping hardware.
  I personally find both proposed gpio_wdt_enable and __gpio_wdt_start 
  names discouraging in this context.
* fixed broken error handling for register_reboot_notifier

 .../devicetree/bindings/watchdog/gpio-wdt.txt      | 14 ++++++-
 drivers/watchdog/gpio_wdt.c                        | 46 +++++++++++++++++-----
 2 files changed, 50 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
index 37afec1..047fe3f 100644
--- a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
@@ -12,8 +12,11 @@ Required Properties:
     the opposite level disables the WDT. Active level is determined
     by the GPIO flags.
 - hw_margin_ms: Maximum time to reset watchdog circuit (milliseconds).
+- always-enabled: Use with watchdog timer which is always enabled
+  in hardware, i.e. there is no way to enable/disable it via GPIO pin.
+  Driver will start pinging WDT immediately upon loading.
 
-Example:
+Examples:
 	watchdog: watchdog {
 		/* ADM706 */
 		compatible = "linux,wdt-gpio";
@@ -21,3 +24,12 @@ Example:
 		hw_algo = "toggle";
 		hw_margin_ms = <1600>;
 	};
+
+	watchdog: watchdog {
+		/* TPS3813 */
+		compatible = "linux,wdt-gpio";
+		gpios = <&gpio3 9 GPIO_ACTIVE_LOW>;
+		hw_algo = "toggle";
+		hw_margin_ms = <10000>;
+		always-enabled;
+	};
diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
index 220a9e0..b6ab6e7 100644
--- a/drivers/watchdog/gpio_wdt.c
+++ b/drivers/watchdog/gpio_wdt.c
@@ -37,6 +37,8 @@ struct gpio_wdt_priv {
 	struct notifier_block	notifier;
 	struct timer_list	timer;
 	struct watchdog_device	wdd;
+	bool		always_enabled;
+	bool		started;
 };
 
 static void gpio_wdt_disable(struct gpio_wdt_priv *priv)
@@ -48,10 +50,8 @@ static void gpio_wdt_disable(struct gpio_wdt_priv *priv)
 		gpio_direction_input(priv->gpio);
 }
 
-static int gpio_wdt_start(struct watchdog_device *wdd)
+static int gpio_wdt_start_hwping(struct gpio_wdt_priv *priv)
 {
-	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
-
 	priv->state = priv->active_low;
 	gpio_direction_output(priv->gpio, priv->state);
 	priv->last_jiffies = jiffies;
@@ -60,12 +60,30 @@ static int gpio_wdt_start(struct watchdog_device *wdd)
 	return 0;
 }
 
+static int gpio_wdt_start(struct watchdog_device *wdd)
+{
+	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
+
+	priv->started = true;
+	if (priv->always_enabled) {
+		/* hardware ping timer is already enabled */
+		priv->last_jiffies = jiffies;
+	} else {
+		gpio_wdt_start_hwping(priv);
+	}
+
+	return 0;
+}
+
 static int gpio_wdt_stop(struct watchdog_device *wdd)
 {
 	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
 
-	mod_timer(&priv->timer, 0);
-	gpio_wdt_disable(priv);
+	priv->started = false;
+	if (!priv->always_enabled) {
+		mod_timer(&priv->timer, 0);
+		gpio_wdt_disable(priv);
+	}
 
 	return 0;
 }
@@ -91,8 +109,9 @@ static void gpio_wdt_hwping(unsigned long data)
 	struct watchdog_device *wdd = (struct watchdog_device *)data;
 	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
 
-	if (time_after(jiffies, priv->last_jiffies +
-		       msecs_to_jiffies(wdd->timeout * 1000))) {
+	if (priv->started &&
+		time_after(jiffies, priv->last_jiffies +
+			       msecs_to_jiffies(wdd->timeout * 1000))) {
 		dev_crit(wdd->dev, "Timer expired. System will reboot soon!\n");
 		return;
 	}
@@ -197,6 +216,9 @@ static int gpio_wdt_probe(struct platform_device *pdev)
 	/* Use safe value (1/2 of real timeout) */
 	priv->hw_margin = msecs_to_jiffies(hw_margin / 2);
 
+	priv->always_enabled = of_property_read_bool(pdev->dev.of_node,
+						"always-enabled");
+
 	watchdog_set_drvdata(&priv->wdd, priv);
 
 	priv->wdd.info		= &gpio_wdt_ident;
@@ -207,6 +229,7 @@ static int gpio_wdt_probe(struct platform_device *pdev)
 	if (watchdog_init_timeout(&priv->wdd, 0, &pdev->dev) < 0)
 		priv->wdd.timeout = SOFT_TIMEOUT_DEF;
 
+	priv->started = false;
 	setup_timer(&priv->timer, gpio_wdt_hwping, (unsigned long)&priv->wdd);
 
 	ret = watchdog_register_device(&priv->wdd);
@@ -215,10 +238,15 @@ static int gpio_wdt_probe(struct platform_device *pdev)
 
 	priv->notifier.notifier_call = gpio_wdt_notify_sys;
 	ret = register_reboot_notifier(&priv->notifier);
-	if (ret)
+	if (ret) {
 		watchdog_unregister_device(&priv->wdd);
+		return ret;
+	}
 
-	return ret;
+	if (priv->always_enabled)
+		gpio_wdt_start_hwping(priv);
+
+	return 0;
 }
 
 static int gpio_wdt_remove(struct platform_device *pdev)
-- 
1.9.1

Re: [PATCH] Add support for always enabled watchdog timers

[Evgeny Boger]

On 08/23/2014 09:33 PM, Guenter Roeck wrote:
> On 08/23/2014 10:25 AM, Alexander Shiyan wrote:
>> Sat, 23 Aug 2014 10:16:08 -0700 от Guenter Roeck <linux@roeck-us.net>:
>>> On 08/16/2014 05:45 PM, Evgeny Boger wrote:
>>>> From: Evgeny Boger <boger@contactless.ru>
>>>>
>>>> Add option to use with watchdog timers which are always enabled
>>>> in hardware, i.e. there is no way to enable/disable it via GPIO pin.
>>>> The driver will start pinging WDT immediately upon loading
>>>> and will continue to do so even after stopping the watchdog.
>>>>
>>> The headline needs a reference to the affected driver.
>>>
>>> Also, please copy the dt mailing list and maintainers.
>>>
>>>> Signed-off-by: Evgeny Boger <boger@contactless.ru>
>>>> ---
>>>>    .../devicetree/bindings/watchdog/gpio-wdt.txt      | 14 ++++++-
>>>>    drivers/watchdog/gpio_wdt.c                        | 45 
>>>> +++++++++++++++++-----
>>>>    2 files changed, 48 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git 
>>>> a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt 
>>>> b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
>>>> index 37afec1..1f8ca46 100644
>>>> --- a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
>>>> +++ b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
>>>> @@ -12,8 +12,11 @@ Required Properties:
>>>>        the opposite level disables the WDT. Active level is determined
>>>>        by the GPIO flags.
>>>>    - hw_margin_ms: Maximum time to reset watchdog circuit 
>>>> (milliseconds).
>>>> +- always-enabled: Use with wathdog timer which is always enabled
>>
>> Similar to NOWAYOUT?
>>
> I think this one is different. NOWAYOUT is a software flag, while
> the flag here is supposed to mean that the hardware watchdog
> is always enabled and running, and can not be disabled. Or at least
> this is my understanding.

exactly

>
> Guenter
>


-- 
С уважением,
     Евгений Богер
     ООО Бесконтактные устройства
     http://contactless.ru
     +7 (919) 965 88 36

Re: [PATCH v2] Add support for always enabled watchdog timers to gpio_wdt driver

[Guenter Roeck]

On 09/06/2014 09:47 AM, Evgeny Boger wrote:
> From: Evgeny Boger <boger@contactless.ru>
>
> Add option to use with watchdog timers which are always enabled
> in hardware, i.e. there is no way to enable/disable it via GPIO pin.
> The driver will start pinging WDT immediately upon loading
> and will continue to do so even after stopping the watchdog.
>
> Signed-off-by: Evgeny Boger <boger@contactless.ru>
> ---
>

Overall looks good to me, though we really need feedback from the DT folks
on the binding. There is a parallel effort to add this functionality into
the watchdog core, meaning the proposed binding "'always-enabled' will
very likely find its way into other drivers. We'll also need a separate
binding to express "watchdog can not be stopped after started once".
That is a separate issue, but important to keep in mind when discussing
the bindings.

Thanks,
Guenter

> Changes since v1:
> * fixed typos and indentation
> * gpio_wdt_start_timer renamed to gpio_wdt_start_hwping.
>    This function actually starts the timer to ping hardware.
>    I personally find both proposed gpio_wdt_enable and __gpio_wdt_start
>    names discouraging in this context.
> * fixed broken error handling for register_reboot_notifier
>
>   .../devicetree/bindings/watchdog/gpio-wdt.txt      | 14 ++++++-
>   drivers/watchdog/gpio_wdt.c                        | 46 +++++++++++++++++-----
>   2 files changed, 50 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> index 37afec1..047fe3f 100644
> --- a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> @@ -12,8 +12,11 @@ Required Properties:
>       the opposite level disables the WDT. Active level is determined
>       by the GPIO flags.
>   - hw_margin_ms: Maximum time to reset watchdog circuit (milliseconds).
> +- always-enabled: Use with watchdog timer which is always enabled
> +  in hardware, i.e. there is no way to enable/disable it via GPIO pin.
> +  Driver will start pinging WDT immediately upon loading.
>
> -Example:
> +Examples:
>   	watchdog: watchdog {
>   		/* ADM706 */
>   		compatible = "linux,wdt-gpio";
> @@ -21,3 +24,12 @@ Example:
>   		hw_algo = "toggle";
>   		hw_margin_ms = <1600>;
>   	};
> +
> +	watchdog: watchdog {
> +		/* TPS3813 */
> +		compatible = "linux,wdt-gpio";
> +		gpios = <&gpio3 9 GPIO_ACTIVE_LOW>;
> +		hw_algo = "toggle";
> +		hw_margin_ms = <10000>;
> +		always-enabled;
> +	};
> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
> index 220a9e0..b6ab6e7 100644
> --- a/drivers/watchdog/gpio_wdt.c
> +++ b/drivers/watchdog/gpio_wdt.c
> @@ -37,6 +37,8 @@ struct gpio_wdt_priv {
>   	struct notifier_block	notifier;
>   	struct timer_list	timer;
>   	struct watchdog_device	wdd;
> +	bool		always_enabled;
> +	bool		started;
>   };
>
>   static void gpio_wdt_disable(struct gpio_wdt_priv *priv)
> @@ -48,10 +50,8 @@ static void gpio_wdt_disable(struct gpio_wdt_priv *priv)
>   		gpio_direction_input(priv->gpio);
>   }
>
> -static int gpio_wdt_start(struct watchdog_device *wdd)
> +static int gpio_wdt_start_hwping(struct gpio_wdt_priv *priv)
>   {
> -	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
> -
>   	priv->state = priv->active_low;
>   	gpio_direction_output(priv->gpio, priv->state);
>   	priv->last_jiffies = jiffies;
> @@ -60,12 +60,30 @@ static int gpio_wdt_start(struct watchdog_device *wdd)
>   	return 0;
>   }
>
> +static int gpio_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
> +
> +	priv->started = true;
> +	if (priv->always_enabled) {
> +		/* hardware ping timer is already enabled */
> +		priv->last_jiffies = jiffies;
> +	} else {
> +		gpio_wdt_start_hwping(priv);
> +	}
> +
> +	return 0;
> +}
> +
>   static int gpio_wdt_stop(struct watchdog_device *wdd)
>   {
>   	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
>
> -	mod_timer(&priv->timer, 0);
> -	gpio_wdt_disable(priv);
> +	priv->started = false;
> +	if (!priv->always_enabled) {
> +		mod_timer(&priv->timer, 0);
> +		gpio_wdt_disable(priv);
> +	}
>
>   	return 0;
>   }
> @@ -91,8 +109,9 @@ static void gpio_wdt_hwping(unsigned long data)
>   	struct watchdog_device *wdd = (struct watchdog_device *)data;
>   	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
>
> -	if (time_after(jiffies, priv->last_jiffies +
> -		       msecs_to_jiffies(wdd->timeout * 1000))) {
> +	if (priv->started &&
> +		time_after(jiffies, priv->last_jiffies +
> +			       msecs_to_jiffies(wdd->timeout * 1000))) {
>   		dev_crit(wdd->dev, "Timer expired. System will reboot soon!\n");
>   		return;
>   	}
> @@ -197,6 +216,9 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>   	/* Use safe value (1/2 of real timeout) */
>   	priv->hw_margin = msecs_to_jiffies(hw_margin / 2);
>
> +	priv->always_enabled = of_property_read_bool(pdev->dev.of_node,
> +						"always-enabled");
> +
>   	watchdog_set_drvdata(&priv->wdd, priv);
>
>   	priv->wdd.info		= &gpio_wdt_ident;
> @@ -207,6 +229,7 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>   	if (watchdog_init_timeout(&priv->wdd, 0, &pdev->dev) < 0)
>   		priv->wdd.timeout = SOFT_TIMEOUT_DEF;
>
> +	priv->started = false;
>   	setup_timer(&priv->timer, gpio_wdt_hwping, (unsigned long)&priv->wdd);
>
>   	ret = watchdog_register_device(&priv->wdd);
> @@ -215,10 +238,15 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>
>   	priv->notifier.notifier_call = gpio_wdt_notify_sys;
>   	ret = register_reboot_notifier(&priv->notifier);
> -	if (ret)
> +	if (ret) {
>   		watchdog_unregister_device(&priv->wdd);
> +		return ret;
> +	}
>
> -	return ret;
> +	if (priv->always_enabled)
> +		gpio_wdt_start_hwping(priv);
> +
> +	return 0;
>   }
>
>   static int gpio_wdt_remove(struct platform_device *pdev)
>