watchdog: add pulse support to gpio watchdog driver

A common external watchdog circuit is kept alive by triggering a short
pulse on the reset pin. This patch adds support for this use case, while
making the algorithm configurable in the devicetree.

The "linux,wdt-gpio" driver being modified is based off the equivalent
driver in the Linux kernel, which provides support for this algorithm.
This patch brings parity to this driver, and is kept aligned with
the functionality and devicetree configuration in the kernel.

It should be noted that this adds a required property named 'hw_algo'
to the devicetree binding, following suit with the kernel. I'm happy to
make this backward-compatible if preferred.

Signed-off-by: Paul Doelle <paaull.git@gmail.com>
---
 arch/sandbox/dts/test.dts                     | 11 ++++-
 .../watchdog/gpio-wdt.txt                     |  8 +++-
 drivers/watchdog/gpio_wdt.c                   | 40 +++++++++++++---
 test/dm/wdt.c                                 | 46 ++++++++++++++++---
 4 files changed, 90 insertions(+), 15 deletions(-)

Hi Paul,

On 04.07.22 11:00, Paul Doelle wrote:
> A common external watchdog circuit is kept alive by triggering a short
> pulse on the reset pin. This patch adds support for this use case, while
> making the algorithm configurable in the devicetree.
> 
> The "linux,wdt-gpio" driver being modified is based off the equivalent
> driver in the Linux kernel, which provides support for this algorithm.
> This patch brings parity to this driver, and is kept aligned with
> the functionality and devicetree configuration in the kernel.
> 
> It should be noted that this adds a required property named 'hw_algo'
> to the devicetree binding, following suit with the kernel. I'm happy to
> make this backward-compatible if preferred.

We should make sure not to break any existing boards using this driver.
A quick check shows:

$ git grep "linux,wdt-gpio"
arch/arm/dts/am335x-rut.dts:            compatible = "linux,wdt-gpio";
arch/arm/dts/imx8mm-data-modul-edm-sbc.dts:             compatible = 
"linux,wdt-gpio";

Both boards already implement the "hw_algo" property though. So I see
no problem applying your patch as-is. We might break some out-of-tree
boards but frankly I don't care that much about those.

> Signed-off-by: Paul Doelle <paaull.git@gmail.com>
> ---
>   arch/sandbox/dts/test.dts                     | 11 ++++-
>   .../watchdog/gpio-wdt.txt                     |  8 +++-
>   drivers/watchdog/gpio_wdt.c                   | 40 +++++++++++++---
>   test/dm/wdt.c                                 | 46 ++++++++++++++++---
>   4 files changed, 90 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> index 8f93775ff4..4dc591d995 100644
> --- a/arch/sandbox/dts/test.dts
> +++ b/arch/sandbox/dts/test.dts
> @@ -830,10 +830,19 @@
>   		};
>   	};
>   
> -	gpio-wdt {
> +	wdt-gpio-toggle {
>   		gpios = <&gpio_a 7 0>;
>   		compatible = "linux,wdt-gpio";
>   		hw_margin_ms = <100>;
> +		hw_algo = "toggle";
> +		always-running;
> +	};
> +
> +	wdt-gpio-level {
> +		gpios = <&gpio_a 7 0>;
> +		compatible = "linux,wdt-gpio";
> +		hw_margin_ms = <100>;
> +		hw_algo = "level";
>   		always-running;
>   	};
>   
> diff --git a/doc/device-tree-bindings/watchdog/gpio-wdt.txt b/doc/device-tree-bindings/watchdog/gpio-wdt.txt
> index c9a8559a3e..746c2c081e 100644
> --- a/doc/device-tree-bindings/watchdog/gpio-wdt.txt
> +++ b/doc/device-tree-bindings/watchdog/gpio-wdt.txt
> @@ -5,7 +5,12 @@ Describes a simple watchdog timer which is reset by toggling a gpio.
>   Required properties:
>   
>   - compatible: Must be "linux,wdt-gpio".
> -- gpios: gpio to toggle when wdt driver reset method is called.
> +- gpios: From common gpio binding; gpio connection to WDT reset pin.
> +- hw_algo: The algorithm used by the driver. Should be one of the
> +  following values:
> +  - toggle: Toggle from high-to-low or low-to-high when resetting the watchdog.
> +  - level: Maintain a constant high/low level, and trigger a short pulse when
> +    resetting the watchdog. Active level is determined by the GPIO flags.
>   - always-running: Boolean property indicating that the watchdog cannot
>     be disabled. At present, U-Boot only supports this kind of GPIO
>     watchdog.
> @@ -15,5 +20,6 @@ Example:
>   	gpio-wdt {
>   		gpios = <&gpio0 1 0>;
>   		compatible = "linux,wdt-gpio";
> +		hw_algo = "toggle";
>   		always-running;
>   	};
> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
> index 982a66b3f9..fe06ec8cc9 100644
> --- a/drivers/watchdog/gpio_wdt.c
> +++ b/drivers/watchdog/gpio_wdt.c
> @@ -4,20 +4,38 @@
>   #include <dm/device_compat.h>
>   #include <wdt.h>
>   #include <asm/gpio.h>
> +#include <linux/delay.h>
> +
> +enum {
> +	HW_ALGO_TOGGLE,
> +	HW_ALGO_LEVEL,
> +};
>   
>   struct gpio_wdt_priv {
> -	struct gpio_desc gpio;
> -	bool always_running;
> -	int state;
> +	struct		gpio_desc gpio;
> +	unsigned int	hw_algo;
> +	bool		always_running;
> +	int		state;
>   };
>   
>   static int gpio_wdt_reset(struct udevice *dev)
>   {
>   	struct gpio_wdt_priv *priv = dev_get_priv(dev);
>   
> -	priv->state = !priv->state;
> -
> -	return dm_gpio_set_value(&priv->gpio, priv->state);
> +	switch (priv->hw_algo) {
> +	case HW_ALGO_TOGGLE:
> +		/* Toggle output pin */
> +		priv->state = !priv->state;
> +		dm_gpio_set_value(&priv->gpio, priv->state);
> +		break;
> +	case HW_ALGO_LEVEL:
> +		/* Pulse */
> +		dm_gpio_set_value(&priv->gpio, 1);
> +		udelay(1);
> +		dm_gpio_set_value(&priv->gpio, 0);
> +		break;
> +	}
> +	return 0;
>   }
>   
>   static int gpio_wdt_start(struct udevice *dev, u64 timeout, ulong flags)
> @@ -34,6 +52,16 @@ static int dm_probe(struct udevice *dev)
>   {
>   	struct gpio_wdt_priv *priv = dev_get_priv(dev);
>   	int ret;
> +	const char *algo = dev_read_string(dev, "hw_algo");
> +
> +	if (!algo)
> +		return -EINVAL;
> +	if (!strcmp(algo, "toggle"))
> +		priv->hw_algo = HW_ALGO_TOGGLE;
> +	else if (!strcmp(algo, "level"))
> +		priv->hw_algo = HW_ALGO_LEVEL;
> +	else
> +		return -EINVAL;
>   
>   	priv->always_running = dev_read_bool(dev, "always-running");
>   	ret = gpio_request_by_name(dev, "gpios", 0, &priv->gpio, GPIOD_IS_OUT);
> diff --git a/test/dm/wdt.c b/test/dm/wdt.c
> index ee615f0e14..535f00a874 100644
> --- a/test/dm/wdt.c
> +++ b/test/dm/wdt.c
> @@ -44,20 +44,20 @@ static int dm_test_wdt_base(struct unit_test_state *uts)
>   }
>   DM_TEST(dm_test_wdt_base, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
>   
> -static int dm_test_wdt_gpio(struct unit_test_state *uts)
> +static int dm_test_wdt_gpio_toggle(struct unit_test_state *uts)
>   {
>   	/*
>   	 * The sandbox wdt gpio is "connected" to gpio bank a, offset
>   	 * 7. Use the sandbox back door to verify that the gpio-wdt
> -	 * driver behaves as expected.
> +	 * driver behaves as expected when using the 'toggle' algorithm.
>   	 */
>   	struct udevice *wdt, *gpio;
>   	const u64 timeout = 42;
>   	const int offset = 7;
>   	int val;
>   
> -	ut_assertok(uclass_get_device_by_driver(UCLASS_WDT,
> -						DM_DRIVER_GET(wdt_gpio), &wdt));
> +	ut_assertok(uclass_get_device_by_name(UCLASS_WDT,
> +					      "wdt-gpio-toggle", &wdt));
>   	ut_assertnonnull(wdt);
>   
>   	ut_assertok(uclass_get_device_by_name(UCLASS_GPIO, "base-gpios", &gpio));
> @@ -74,7 +74,39 @@ static int dm_test_wdt_gpio(struct unit_test_state *uts)
>   
>   	return 0;
>   }
> -DM_TEST(dm_test_wdt_gpio, UT_TESTF_SCAN_FDT);
> +DM_TEST(dm_test_wdt_gpio_toggle, UT_TESTF_SCAN_FDT);
> +
> +static int dm_test_wdt_gpio_level(struct unit_test_state *uts)
> +{
> +	/*
> +	 * The sandbox wdt gpio is "connected" to gpio bank a, offset
> +	 * 7. Use the sandbox back door to verify that the gpio-wdt
> +	 * driver behaves as expected when using the 'level' algorithm.
> +	 */
> +	struct udevice *wdt, *gpio;
> +	const u64 timeout = 42;
> +	const int offset = 7;
> +	int val;
> +
> +	ut_assertok(uclass_get_device_by_name(UCLASS_WDT,
> +					      "wdt-gpio-level", &wdt));
> +	ut_assertnonnull(wdt);
> +
> +	ut_assertok(uclass_get_device_by_name(UCLASS_GPIO, "base-gpios", &gpio));
> +	ut_assertnonnull(gpio);
> +	ut_assertok(wdt_start(wdt, timeout, 0));
> +
> +	val = sandbox_gpio_get_value(gpio, offset);
> +	ut_assertok(wdt_reset(wdt));
> +	ut_asserteq(val, sandbox_gpio_get_value(gpio, offset));
> +	ut_assertok(wdt_reset(wdt));
> +	ut_asserteq(val, sandbox_gpio_get_value(gpio, offset));
> +
> +	ut_asserteq(-ENOSYS, wdt_stop(wdt));
> +
> +	return 0;
> +}
> +DM_TEST(dm_test_wdt_gpio_level, UT_TESTF_SCAN_FDT);
>   
>   static int dm_test_wdt_watchdog_reset(struct unit_test_state *uts)
>   {
> @@ -86,8 +118,8 @@ static int dm_test_wdt_watchdog_reset(struct unit_test_state *uts)
>   	uint reset_count;
>   	int val;
>   
> -	ut_assertok(uclass_get_device_by_driver(UCLASS_WDT,
> -						DM_DRIVER_GET(wdt_gpio), &gpio_wdt));
> +	ut_assertok(uclass_get_device_by_name(UCLASS_WDT,
> +					      "wdt-gpio-toggle", &gpio_wdt));
>   	ut_assertnonnull(gpio_wdt);
>   	ut_assertok(uclass_get_device_by_driver(UCLASS_WDT,
>   						DM_DRIVER_GET(wdt_sandbox), &sandbox_wdt));

Looks good, so:

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan