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

[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

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