[PATCH] watchdog: add pulse support to gpio watchdog driver
Stefan Roese
sr at denx.de
Tue Jul 5 08:28:44 CEST 2022
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 at 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 at denx.de>
Thanks,
Stefan
More information about the U-Boot
mailing list