* [PATCH] gpio_wdt: Add "always_running" feature to GPIO watchdog @ 2014-11-20 13:16 Mike Looijmans 2014-11-20 17:23 ` Guenter Roeck 0 siblings, 1 reply; 12+ messages in thread From: Mike Looijmans @ 2014-11-20 13:16 UTC (permalink / raw) To: wim; +Cc: linux-watchdog, linux-kernel, Mike Looijmans 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@topic.nl> --- drivers/watchdog/gpio_wdt.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c index 220a9e0..921ee67 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; 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))) { 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"); + 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; + } + return ret; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] gpio_wdt: Add "always_running" feature to GPIO watchdog 2014-11-20 13:16 [PATCH] gpio_wdt: Add "always_running" feature to GPIO watchdog Mike Looijmans @ 2014-11-20 17:23 ` Guenter Roeck 2014-11-21 6:53 ` [PATCH v2] " Mike Looijmans 0 siblings, 1 reply; 12+ messages in thread From: Guenter Roeck @ 2014-11-20 17:23 UTC (permalink / raw) To: Mike Looijmans; +Cc: wim, linux-watchdog, linux-kernel On Thu, Nov 20, 2014 at 02:16:17PM +0100, 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@topic.nl> > --- > drivers/watchdog/gpio_wdt.c | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c > index 220a9e0..921ee67 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; > 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))) { > 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"); That will need to be documented in a devicetree bindings document, and be reviewed by the fdt maintainers. I may be wrong, but I think devicetree properties commonly use '-', not '_'. Guenter > + > 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; > + } > + > return ret; > } > > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] gpio_wdt: Add "always_running" feature to GPIO watchdog 2014-11-20 17:23 ` Guenter Roeck @ 2014-11-21 6:53 ` Mike Looijmans 2014-11-21 7:28 ` Guenter Roeck 0 siblings, 1 reply; 12+ messages in thread From: Mike Looijmans @ 2014-11-21 6:53 UTC (permalink / raw) To: wim; +Cc: linux-watchdog, linux-kernel, Mike Looijmans 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@topic.nl> --- 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; 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))) { 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"); + 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; + } + return ret; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] gpio_wdt: Add "always_running" feature to GPIO watchdog 2014-11-21 6:53 ` [PATCH v2] " Mike Looijmans @ 2014-11-21 7:28 ` Guenter Roeck 2014-11-21 7:51 ` Mike Looijmans 2014-11-21 9:40 ` [PATCH v3] " Mike Looijmans 0 siblings, 2 replies; 12+ messages in thread From: Guenter Roeck @ 2014-11-21 7:28 UTC (permalink / raw) To: Mike Looijmans, wim; +Cc: linux-watchdog, linux-kernel 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@topic.nl> > --- > > 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. > 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. 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. Guenter > return ret; > } > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] gpio_wdt: Add "always_running" feature to GPIO watchdog 2014-11-21 7:28 ` Guenter Roeck @ 2014-11-21 7:51 ` Mike Looijmans 2014-11-21 9:40 ` [PATCH v3] " Mike Looijmans 1 sibling, 0 replies; 12+ messages in thread From: Mike Looijmans @ 2014-11-21 7:51 UTC (permalink / raw) To: Guenter Roeck, wim; +Cc: linux-watchdog, linux-kernel 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@topic.nl> >> --- >> >> 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@topic.nl 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3] gpio_wdt: Add "always_running" feature to GPIO watchdog 2014-11-21 7:28 ` Guenter Roeck 2014-11-21 7:51 ` Mike Looijmans @ 2014-11-21 9:40 ` Mike Looijmans 2014-12-11 7:04 ` Mike Looijmans ` (2 more replies) 1 sibling, 3 replies; 12+ messages in thread From: Mike Looijmans @ 2014-11-21 9:40 UTC (permalink / raw) To: wim; +Cc: linux-watchdog, linux-kernel, Mike Looijmans 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@topic.nl> --- v3: Indentation adjusted to match Fix error path in probe when notification registration fails Prevent double assignment of "armed" variable .../devicetree/bindings/watchdog/gpio-wdt.txt | 5 +++ drivers/watchdog/gpio_wdt.c | 39 ++++++++++++++++---- 2 files changed, 36 insertions(+), 8 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..9bfbd73 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; unsigned int hw_algo; unsigned int hw_margin; unsigned long last_jiffies; @@ -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 void gpio_wdt_start_impl(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,25 @@ 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); + + gpio_wdt_start_impl(priv); + priv->armed = true; + + 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->armed = false; + if (!priv->always_running) { + mod_timer(&priv->timer, 0); + gpio_wdt_disable(priv); + } return 0; } @@ -91,8 +104,8 @@ 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->armed && 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 +210,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"); + watchdog_set_drvdata(&priv->wdd, priv); priv->wdd.info = &gpio_wdt_ident; @@ -216,8 +232,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) - watchdog_unregister_device(&priv->wdd); + goto error_unregister; + if (priv->always_running) + gpio_wdt_start_impl(priv); + + return 0; + +error_unregister: + watchdog_unregister_device(&priv->wdd); return ret; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3] gpio_wdt: Add "always_running" feature to GPIO watchdog 2014-11-21 9:40 ` [PATCH v3] " Mike Looijmans @ 2014-12-11 7:04 ` Mike Looijmans 2015-01-13 21:06 ` [v3] " Guenter Roeck 2015-01-13 21:52 ` Guenter Roeck 2 siblings, 0 replies; 12+ messages in thread From: Mike Looijmans @ 2014-12-11 7:04 UTC (permalink / raw) To: Guenter Roeck; +Cc: wim, linux-watchdog, linux-kernel Is this v3 patch okay or are you waiting for additional changes? Kind regards, Mike. On 11/21/2014 10:40 AM, 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@topic.nl> > --- > v3: Indentation adjusted to match > Fix error path in probe when notification registration fails > Prevent double assignment of "armed" variable > > .../devicetree/bindings/watchdog/gpio-wdt.txt | 5 +++ > drivers/watchdog/gpio_wdt.c | 39 ++++++++++++++++---- > 2 files changed, 36 insertions(+), 8 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..9bfbd73 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; > unsigned int hw_algo; > unsigned int hw_margin; > unsigned long last_jiffies; > @@ -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 void gpio_wdt_start_impl(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,25 @@ 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); > + > + gpio_wdt_start_impl(priv); > + priv->armed = true; > + > + 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->armed = false; > + if (!priv->always_running) { > + mod_timer(&priv->timer, 0); > + gpio_wdt_disable(priv); > + } > > return 0; > } > @@ -91,8 +104,8 @@ 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->armed && 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 +210,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"); > + > watchdog_set_drvdata(&priv->wdd, priv); > > priv->wdd.info = &gpio_wdt_ident; > @@ -216,8 +232,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) > - watchdog_unregister_device(&priv->wdd); > + goto error_unregister; > > + if (priv->always_running) > + gpio_wdt_start_impl(priv); > + > + return 0; > + > +error_unregister: > + watchdog_unregister_device(&priv->wdd); > 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@topic.nl Website: www.topic.nl Please consider the environment before printing this e-mail Topic zoekt gedreven (embedded) software specialisten! http://topic.nl/vacatures/topic-zoekt-software-engineers/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [v3] gpio_wdt: Add "always_running" feature to GPIO watchdog 2014-11-21 9:40 ` [PATCH v3] " Mike Looijmans 2014-12-11 7:04 ` Mike Looijmans @ 2015-01-13 21:06 ` Guenter Roeck 2015-01-13 21:52 ` Guenter Roeck 2 siblings, 0 replies; 12+ messages in thread From: Guenter Roeck @ 2015-01-13 21:06 UTC (permalink / raw) To: Mike Looijmans; +Cc: wim, linux-watchdog, linux-kernel On Fri, Nov 21, 2014 at 10:40:28AM +0100, 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@topic.nl> Reviewed-by: Guenter Roeck <linux@roeck-us.net> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [v3] gpio_wdt: Add "always_running" feature to GPIO watchdog 2014-11-21 9:40 ` [PATCH v3] " Mike Looijmans 2014-12-11 7:04 ` Mike Looijmans 2015-01-13 21:06 ` [v3] " Guenter Roeck @ 2015-01-13 21:52 ` Guenter Roeck 2015-01-14 6:28 ` [PATCH v4] " Mike Looijmans 2 siblings, 1 reply; 12+ messages in thread From: Guenter Roeck @ 2015-01-13 21:52 UTC (permalink / raw) To: Mike Looijmans; +Cc: wim, linux-watchdog, linux-kernel On Fri, Nov 21, 2014 at 10:40:28AM +0100, 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@topic.nl> > --- Actially, my reviewed-by: was to early. See below. > v3: Indentation adjusted to match > Fix error path in probe when notification registration fails > Prevent double assignment of "armed" variable > > .../devicetree/bindings/watchdog/gpio-wdt.txt | 5 +++ > drivers/watchdog/gpio_wdt.c | 39 ++++++++++++++++---- > 2 files changed, 36 insertions(+), 8 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..9bfbd73 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; > unsigned int hw_algo; > unsigned int hw_margin; > unsigned long last_jiffies; > @@ -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 void gpio_wdt_start_impl(struct gpio_wdt_priv *priv) This function is now void but still has a return value. Guenter ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4] gpio_wdt: Add "always_running" feature to GPIO watchdog 2015-01-13 21:52 ` Guenter Roeck @ 2015-01-14 6:28 ` Mike Looijmans 2015-01-16 5:01 ` Guenter Roeck 2015-02-03 12:54 ` Wim Van Sebroeck 0 siblings, 2 replies; 12+ messages in thread From: Mike Looijmans @ 2015-01-14 6:28 UTC (permalink / raw) To: linux; +Cc: wim, linux-watchdog, linux-kernel, Mike Looijmans 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@topic.nl> --- v4: Fix 'return' with a value, in function returning void .../devicetree/bindings/watchdog/gpio-wdt.txt | 5 +++ drivers/watchdog/gpio_wdt.c | 37 +++++++++++++++----- 2 files changed, 34 insertions(+), 8 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..51acda6 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; unsigned int hw_algo; unsigned int hw_margin; unsigned long last_jiffies; @@ -48,14 +50,20 @@ 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 void gpio_wdt_start_impl(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; mod_timer(&priv->timer, priv->last_jiffies + priv->hw_margin); +} + +static int gpio_wdt_start(struct watchdog_device *wdd) +{ + struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd); + + gpio_wdt_start_impl(priv); + priv->armed = true; return 0; } @@ -64,8 +72,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,8 +102,8 @@ 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->armed && 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 +208,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"); + watchdog_set_drvdata(&priv->wdd, priv); priv->wdd.info = &gpio_wdt_ident; @@ -216,8 +230,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) - watchdog_unregister_device(&priv->wdd); + goto error_unregister; + if (priv->always_running) + gpio_wdt_start_impl(priv); + + return 0; + +error_unregister: + watchdog_unregister_device(&priv->wdd); return ret; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4] gpio_wdt: Add "always_running" feature to GPIO watchdog 2015-01-14 6:28 ` [PATCH v4] " Mike Looijmans @ 2015-01-16 5:01 ` Guenter Roeck 2015-02-03 12:54 ` Wim Van Sebroeck 1 sibling, 0 replies; 12+ messages in thread From: Guenter Roeck @ 2015-01-16 5:01 UTC (permalink / raw) To: Mike Looijmans; +Cc: wim, linux-watchdog, linux-kernel On 01/13/2015 10:28 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@topic.nl> Reviewed-by: Guenter Roeck <linux@roeck-us.net> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] gpio_wdt: Add "always_running" feature to GPIO watchdog 2015-01-14 6:28 ` [PATCH v4] " Mike Looijmans 2015-01-16 5:01 ` Guenter Roeck @ 2015-02-03 12:54 ` Wim Van Sebroeck 1 sibling, 0 replies; 12+ messages in thread From: Wim Van Sebroeck @ 2015-02-03 12:54 UTC (permalink / raw) To: Mike Looijmans; +Cc: linux, linux-watchdog, linux-kernel Hi Mike, > 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@topic.nl> Patch has been added to linux-watchdog-next. Kind regards, Wim. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-02-03 12:54 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-20 13:16 [PATCH] gpio_wdt: Add "always_running" feature to GPIO watchdog Mike Looijmans 2014-11-20 17:23 ` Guenter Roeck 2014-11-21 6:53 ` [PATCH v2] " Mike Looijmans 2014-11-21 7:28 ` Guenter Roeck 2014-11-21 7:51 ` Mike Looijmans 2014-11-21 9:40 ` [PATCH v3] " Mike Looijmans 2014-12-11 7:04 ` Mike Looijmans 2015-01-13 21:06 ` [v3] " Guenter Roeck 2015-01-13 21:52 ` Guenter Roeck 2015-01-14 6:28 ` [PATCH v4] " Mike Looijmans 2015-01-16 5:01 ` Guenter Roeck 2015-02-03 12:54 ` Wim Van Sebroeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox