Re: [PATCH v2] gpio_wdt: Add "always_running" feature to GPIO watchdog
From: Mike Looijmans
Date: Fri Nov 21 2014 - 02:51:47 EST
ïOn 11/21/2014 08:28 AM, Guenter Roeck wrote:
On 11/20/2014 10:53 PM, Mike Looijmans wrote:
On some chips, like the TPS386000, the trigger cannot be disabled
and the CPU must keep toggling the line at all times. Add a switch
"always_running" to keep toggling the GPIO line regardless of the
state of the soft part of the watchdog. The "armed" member keeps
track of whether a timeout must also cause a reset.
Signed-off-by: Mike Looijmans <mike.looijmans@xxxxxxxx>
---
v2: Rename property "always_running" to "always-running" and add devicetree
documentation
.../devicetree/bindings/watchdog/gpio-wdt.txt | 5 +++++
drivers/watchdog/gpio_wdt.c | 20
+++++++++++++++++---
2 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
index 37afec1..1987949 100644
--- a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
@@ -13,6 +13,11 @@ Required Properties:
by the GPIO flags.
- hw_margin_ms: Maximum time to reset watchdog circuit (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.
+
Example:
watchdog: watchdog {
/* ADM706 */
diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
index 220a9e0..3266215 100644
--- a/drivers/watchdog/gpio_wdt.c
+++ b/drivers/watchdog/gpio_wdt.c
@@ -31,6 +31,8 @@ struct gpio_wdt_priv {
int gpio;
bool active_low;
bool state;
+ bool always_running;
+ bool armed;
Please align columns with the other variables.
unsigned int hw_algo;
unsigned int hw_margin;
unsigned long last_jiffies;
@@ -56,6 +58,7 @@ static int gpio_wdt_start(struct watchdog_device *wdd)
gpio_direction_output(priv->gpio, priv->state);
priv->last_jiffies = jiffies;
mod_timer(&priv->timer, priv->last_jiffies + priv->hw_margin);
+ priv->armed = true;
return 0;
}
@@ -64,8 +67,11 @@ 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->armed = false;
+ if (!priv->always_running) {
+ mod_timer(&priv->timer, 0);
+ gpio_wdt_disable(priv);
+ }
return 0;
}
@@ -91,7 +97,7 @@ 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 +
+ if (priv->armed && time_after(jiffies, priv->last_jiffies +
msecs_to_jiffies(wdd->timeout * 1000))) {
Please align continuation lines.
I'm unsure as to how you want them aligned. I thought I adhered to the kernel
coding guidelines, but please correct me if I did that wrong.
dev_crit(wdd->dev, "Timer expired. System will reboot soon!\n");
return;
@@ -197,6 +203,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_running = of_property_read_bool(pdev->dev.of_node,
+ "always-running");
+
Please align continuation line with '('.
watchdog_set_drvdata(&priv->wdd, priv);
priv->wdd.info = &gpio_wdt_ident;
@@ -218,6 +227,11 @@ static int gpio_wdt_probe(struct platform_device *pdev)
if (ret)
watchdog_unregister_device(&priv->wdd);
+ if (priv->always_running) {
+ gpio_wdt_start(&priv->wdd);
+ priv->armed = false;
+ }
+
This won't work if there is an error. The driver won't load but the timer
is started, which will likely cause a crash when the timer fires. You'll
have to fix the error handling.
If you have an "always-running" gpio watchdog and this fails, the error
handling is going to be the least of your problems (the board I use this patch
on will power down if you don't toggle the pin every 400ms after POR).
But you're absolutely right, I missed the error handling path here.
An alternative approach would be to make failure to register the notifier a
warning only. It seems harsh to me to let the system commit suicide just
because the shutdown notification didn't register. On the other hand, the
notification registration should never fail anyway (out-of-memory will result
in system failure soon enough).
I also dislike that you first set priv->armed in the start function and
then reset it, but I don't have a better idea right now.
I just relied on the compiler's optimizer to remove the first assignment.
Alternative would be to split the start function into two parts, where the
"armed" assignment is omitted from the upper part and call the upper part from
probe only.
Guenter
return ret;
}
Met vriendelijke groet / kind regards,
Mike Looijmans
System Expert
TOPIC Embedded Systems
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@xxxxxxxx
Website: www.topic.nl
Please consider the environment before printing this e-mail
Visit us at Bits & Chips Smart Systems 20th November, 1931 Congrescentum Brabanthallen 's-Hertogenbosch, stand number 34
http://bc-smartsystems.nl
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/