wdt_start() does the "no ->start? return -ENOSYS" check, don't open-code that in wdt_expire_now().

Also, wdt_start() maintains some global (and later some per-device) state, which would get out of sync with this direct method call - not that it matters much here since the board is supposed to reset very soon.

Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Stefan Roese sr@denx.de Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- drivers/watchdog/wdt-uclass.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c index 17334dbda6..df8164da2a 100644 --- a/drivers/watchdog/wdt-uclass.c +++ b/drivers/watchdog/wdt-uclass.c @@ -120,10 +120,8 @@ int wdt_expire_now(struct udevice *dev, ulong flags) if (ops->expire_now) { return ops->expire_now(dev, flags); } else { - if (!ops->start) - return -ENOSYS; + ret = wdt_start(dev, 1, flags);

- ret = ops->start(dev, 1, flags); if (ret < 0) return ret;

The addition of .pre_probe and .per_device_auto made this look bad. Fix it.

Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Stefan Roese sr@denx.de Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- drivers/watchdog/wdt-uclass.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c index b29d214724..81287c759a 100644 --- a/drivers/watchdog/wdt-uclass.c +++ b/drivers/watchdog/wdt-uclass.c @@ -210,10 +210,10 @@ static int wdt_pre_probe(struct udevice *dev) }

UCLASS_DRIVER(wdt) = { - .id = UCLASS_WDT, - .name = "watchdog", - .flags = DM_UC_FLAG_SEQ_ALIAS, - .post_bind = wdt_post_bind, + .id = UCLASS_WDT, + .name = "watchdog", + .flags = DM_UC_FLAG_SEQ_ALIAS, + .post_bind = wdt_post_bind, .pre_probe = wdt_pre_probe, .per_device_auto = sizeof(struct wdt_priv), };

As a step towards handling all DM watchdogs in watchdog_reset(), use a per-device flag to keep track of whether the device has been started instead of a bit in gd->flags.

We will still need that bit to know whether we are past initr_watchdog() and hence have populated gd->watchdog_dev - incidentally, that is how it was used prior to commit 9c44ff1c5f3c.

Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Stefan Roese sr@denx.de Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- drivers/watchdog/wdt-uclass.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c index 0a1f43771c..358fc68e27 100644 --- a/drivers/watchdog/wdt-uclass.c +++ b/drivers/watchdog/wdt-uclass.c @@ -33,6 +33,8 @@ struct wdt_priv { * ->reset(). */ ulong next_reset; + /* Whether watchdog_start() has been called on the device. */ + bool running; };

static void init_watchdog_dev(struct udevice *dev) @@ -74,6 +76,7 @@ int initr_watchdog(void) } init_watchdog_dev(gd->watchdog_dev);

+ gd->flags |= GD_FLG_WDT_READY; return 0; }

@@ -86,8 +89,11 @@ int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags) return -ENOSYS;

ret = ops->start(dev, timeout_ms, flags); - if (ret == 0) - gd->flags |= GD_FLG_WDT_READY; + if (ret == 0) { + struct wdt_priv *priv = dev_get_uclass_priv(dev); + + priv->running = true; + }

return ret; } @@ -101,8 +107,11 @@ int wdt_stop(struct udevice *dev) return -ENOSYS;

ret = ops->stop(dev); - if (ret == 0) - gd->flags &= ~GD_FLG_WDT_READY; + if (ret == 0) { + struct wdt_priv *priv = dev_get_uclass_priv(dev); + + priv->running = false; + }

return ret; } @@ -156,6 +165,9 @@ void watchdog_reset(void)

dev = gd->watchdog_dev; priv = dev_get_uclass_priv(dev); + if (!priv->running) + return; + /* Do not reset the watchdog too often */ now = get_timer(0); if (time_after_eq(now, priv->next_reset)) {

For the unit tests, it is more convenient if the tests are in charge of when the watchdog devices are started and stopped, so prevent wdt-uclass from doing it automatically.

Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Stefan Roese sr@denx.de Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- configs/sandbox64_defconfig | 1 + configs/sandbox_defconfig | 1 + 2 files changed, 2 insertions(+)

diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig index f7098b4969..3627a066b4 100644 --- a/configs/sandbox64_defconfig +++ b/configs/sandbox64_defconfig @@ -223,6 +223,7 @@ CONFIG_OSD=y CONFIG_SANDBOX_OSD=y CONFIG_SPLASH_SCREEN_ALIGN=y CONFIG_VIDEO_BMP_RLE8=y +# CONFIG_WATCHDOG_AUTOSTART is not set CONFIG_WDT=y CONFIG_WDT_SANDBOX=y CONFIG_FS_CBFS=y diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index bcd82f76ff..34b749b47b 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -282,6 +282,7 @@ CONFIG_W1=y CONFIG_W1_GPIO=y CONFIG_W1_EEPROM=y CONFIG_W1_EEPROM_SANDBOX=y +# CONFIG_WATCHDOG_AUTOSTART is not set CONFIG_WDT=y CONFIG_WDT_SANDBOX=y CONFIG_FS_CBFS=y

A board can have and make use of more than one watchdog device, say one built into the SOC and an external gpio-petted one. Having wdt-uclass only handle the first is both a little arbitrary and unexpected.

So change initr_watchdog() so we visit (probe) all DM watchdog devices, and call the init_watchdog_dev helper for each.

Similarly let watchdog_reset() loop over the whole uclass - each having their own ratelimiting metadata, and a separate "is this device running" flag.

This gets rid of the watchdog_dev member of struct global_data. We do, however, still need the GD_FLG_WDT_READY set in initr_watchdog(). This is because watchdog_reset() can get called before DM is ready, and I don't think we can call uclass_get() that early.

The current code just returns 0 if "getting" the first device fails - that can of course happen because there are no devices, but it could also happen if its ->probe call failed. In keeping with that, continue with the handling of the remaining devices even if one fails to probe. This is also why we cannot use uclass_probe_all().

If desired, it's possible to later add a per-device "u-boot,autostart" boolean property, so that one can do CONFIG_WATCHDOG_AUTOSTART per-device.

Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Stefan Roese sr@denx.de Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- drivers/watchdog/wdt-uclass.c | 56 ++++++++++++++++++++----------- include/asm-generic/global_data.h | 6 ---- 2 files changed, 36 insertions(+), 26 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c index 5b1c0df5d6..7570710c4d 100644 --- a/drivers/watchdog/wdt-uclass.c +++ b/drivers/watchdog/wdt-uclass.c @@ -61,20 +61,24 @@ static void init_watchdog_dev(struct udevice *dev)

int initr_watchdog(void) { - /* - * Init watchdog: This will call the probe function of the - * watchdog driver, enabling the use of the device - */ - if (uclass_get_device_by_seq(UCLASS_WDT, 0, - (struct udevice **)&gd->watchdog_dev)) { - debug("WDT: Not found by seq!\n"); - if (uclass_get_device(UCLASS_WDT, 0, - (struct udevice **)&gd->watchdog_dev)) { - printf("WDT: Not found!\n"); - return 0; + struct udevice *dev; + struct uclass *uc; + int ret; + + ret = uclass_get(UCLASS_WDT, &uc); + if (ret) { + log_debug("Error getting UCLASS_WDT: %d\n", ret); + return 0; + } + + uclass_foreach_dev(dev, uc) { + ret = device_probe(dev); + if (ret) { + log_debug("Error probing %s: %d\n", dev->name, ret); + continue; } + init_watchdog_dev(dev); } - init_watchdog_dev(gd->watchdog_dev);

gd->flags |= GD_FLG_WDT_READY; return 0; @@ -182,22 +186,34 @@ void watchdog_reset(void) { struct wdt_priv *priv; struct udevice *dev; + struct uclass *uc; ulong now;

/* Exit if GD is not ready or watchdog is not initialized yet */ if (!gd || !(gd->flags & GD_FLG_WDT_READY)) return;

- dev = gd->watchdog_dev; - priv = dev_get_uclass_priv(dev); - if (!priv->running) + if (uclass_get(UCLASS_WDT, &uc)) return;

- /* Do not reset the watchdog too often */ - now = get_timer(0); - if (time_after_eq(now, priv->next_reset)) { - priv->next_reset = now + priv->reset_period; - wdt_reset(dev); + /* + * All devices bound to the wdt uclass should have been probed + * in initr_watchdog(). But just in case something went wrong, + * check device_active() before accessing the uclass private + * data. + */ + uclass_foreach_dev(dev, uc) { + if (!device_active(dev)) + continue; + priv = dev_get_uclass_priv(dev); + if (!priv->running) + continue; + /* Do not reset the watchdog too often */ + now = get_timer(0); + if (time_after_eq(now, priv->next_reset)) { + priv->next_reset = now + priv->reset_period; + wdt_reset(dev); + } } } #endif diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index e55070303f..28d749538c 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -447,12 +447,6 @@ struct global_data { */ fdt_addr_t translation_offset; #endif -#if CONFIG_IS_ENABLED(WDT) - /** - * @watchdog_dev: watchdog device - */ - struct udevice *watchdog_dev; -#endif #ifdef CONFIG_GENERATE_ACPI_TABLE /** * @acpi_ctx: ACPI context pointer

A rather common kind of external watchdog circuit is one that is kept alive by toggling a gpio. Add a driver for handling such a watchdog.

The corresponding linux driver apparently has support for some watchdog circuits which can be disabled by tri-stating the gpio, but I have never actually encountered such a chip in the wild; the whole point of adding an external watchdog is usually that it is not in any way under software control. For forward-compatibility, and to make DT describe the hardware, the current driver only supports devices that have the always-running property. I went a little back and forth on whether I should fail ->probe or only ->start, and ended up deciding ->start was the right place.

The compatible string is probably a little odd as it has nothing to do with linux per se - however, I chose that to make .dts snippets reusable between device trees used with U-Boot and linux, and this is the (only) compatible string that linux' corresponding driver and DT binding accepts. I have asked whether one should/could add "wdt-gpio" to that binding, but the answer was no:

https://lore.kernel.org/lkml/CAL_JsqKEGaFpiFV_oAtE+S_bnHkg4qry+bhx2EDs=NSbVf...

If someone feels strongly about this, I can certainly remove the "linux," part from the string - it probably wouldn't the only place where one can't reuse a DT snippet as-is between linux and U-Boot.

Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Stefan Roese sr@denx.de Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- .../watchdog/gpio-wdt.txt | 19 ++++++ drivers/watchdog/Kconfig | 9 +++ drivers/watchdog/Makefile | 1 + drivers/watchdog/gpio_wdt.c | 68 +++++++++++++++++++ 4 files changed, 97 insertions(+) create mode 100644 doc/device-tree-bindings/watchdog/gpio-wdt.txt create mode 100644 drivers/watchdog/gpio_wdt.c

diff --git a/doc/device-tree-bindings/watchdog/gpio-wdt.txt b/doc/device-tree-bindings/watchdog/gpio-wdt.txt new file mode 100644 index 0000000000..c9a8559a3e --- /dev/null +++ b/doc/device-tree-bindings/watchdog/gpio-wdt.txt @@ -0,0 +1,19 @@ +GPIO watchdog timer + +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. +- always-running: Boolean property indicating that the watchdog cannot + be disabled. At present, U-Boot only supports this kind of GPIO + watchdog. + +Example: + + gpio-wdt { + gpios = <&gpio0 1 0>; + compatible = "linux,wdt-gpio"; + always-running; + }; diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index f0ff2612a6..6fbb5c1b6d 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -147,6 +147,15 @@ config WDT_CORTINA This driver support all CPU ISAs supported by Cortina Access CAxxxx SoCs.

+config WDT_GPIO + bool "External gpio watchdog support" + depends on WDT + depends on DM_GPIO + help + Support for external watchdog fed by toggling a gpio. See + doc/device-tree-bindings/watchdog/gpio-wdt.txt for + information on how to describe the watchdog in device tree. + config WDT_MPC8xx bool "MPC8xx watchdog timer support" depends on WDT && MPC8xx diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 5c7ef593fe..f14415bb8e 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -25,6 +25,7 @@ obj-$(CONFIG_WDT_BOOKE) += booke_wdt.o obj-$(CONFIG_WDT_CORTINA) += cortina_wdt.o obj-$(CONFIG_WDT_ORION) += orion_wdt.o obj-$(CONFIG_WDT_CDNS) += cdns_wdt.o +obj-$(CONFIG_WDT_GPIO) += gpio_wdt.o obj-$(CONFIG_WDT_MPC8xx) += mpc8xx_wdt.o obj-$(CONFIG_WDT_MT7620) += mt7620_wdt.o obj-$(CONFIG_WDT_MT7621) += mt7621_wdt.o diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c new file mode 100644 index 0000000000..982a66b3f9 --- /dev/null +++ b/drivers/watchdog/gpio_wdt.c @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: GPL-2.0+ + +#include <dm.h> +#include <dm/device_compat.h> +#include <wdt.h> +#include <asm/gpio.h> + +struct gpio_wdt_priv { + struct gpio_desc gpio; + 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); +} + +static int gpio_wdt_start(struct udevice *dev, u64 timeout, ulong flags) +{ + struct gpio_wdt_priv *priv = dev_get_priv(dev); + + if (priv->always_running) + return 0; + + return -ENOSYS; +} + +static int dm_probe(struct udevice *dev) +{ + struct gpio_wdt_priv *priv = dev_get_priv(dev); + int ret; + + priv->always_running = dev_read_bool(dev, "always-running"); + ret = gpio_request_by_name(dev, "gpios", 0, &priv->gpio, GPIOD_IS_OUT); + if (ret < 0) { + dev_err(dev, "Request for wdt gpio failed: %d\n", ret); + return ret; + } + + if (priv->always_running) + ret = gpio_wdt_reset(dev); + + return ret; +} + +static const struct wdt_ops gpio_wdt_ops = { + .start = gpio_wdt_start, + .reset = gpio_wdt_reset, +}; + +static const struct udevice_id gpio_wdt_ids[] = { + { .compatible = "linux,wdt-gpio" }, + {} +}; + +U_BOOT_DRIVER(wdt_gpio) = { + .name = "wdt_gpio", + .id = UCLASS_WDT, + .of_match = gpio_wdt_ids, + .ops = &gpio_wdt_ops, + .probe = dm_probe, + .priv_auto = sizeof(struct gpio_wdt_priv), +};

Check that the watchdog_reset() implementation in wdt-uclass behaves as expected:

- resets all activated watchdog devices - leaves unactivated/stopped devices alone - that the rate-limiting works, with a per-device threshold

Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Stefan Roese sr@denx.de Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- arch/sandbox/dts/test.dts | 2 ++ test/dm/wdt.c | 54 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index fe5ac6ecd9..bafc2e9494 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -782,6 +782,7 @@ gpio-wdt { gpios = <&gpio_a 7 0>; compatible = "linux,wdt-gpio"; + hw_margin_ms = <100>; always-running; };

@@ -1264,6 +1265,7 @@

wdt0: wdt@0 { compatible = "sandbox,wdt"; + hw_margin_ms = <200>; };

axi: axi@0 { diff --git a/test/dm/wdt.c b/test/dm/wdt.c index abff853a02..ee615f0e14 100644 --- a/test/dm/wdt.c +++ b/test/dm/wdt.c @@ -12,6 +12,8 @@ #include <dm/test.h> #include <test/test.h> #include <test/ut.h> +#include <linux/delay.h> +#include <watchdog.h>

/* Test that watchdog driver functions are called */ static int dm_test_wdt_base(struct unit_test_state *uts) @@ -73,3 +75,55 @@ static int dm_test_wdt_gpio(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_wdt_gpio, UT_TESTF_SCAN_FDT); + +static int dm_test_wdt_watchdog_reset(struct unit_test_state *uts) +{ + struct sandbox_state *state = state_get_current(); + struct udevice *gpio_wdt, *sandbox_wdt; + struct udevice *gpio; + const u64 timeout = 42; + const int offset = 7; + uint reset_count; + int val; + + ut_assertok(uclass_get_device_by_driver(UCLASS_WDT, + DM_DRIVER_GET(wdt_gpio), &gpio_wdt)); + ut_assertnonnull(gpio_wdt); + ut_assertok(uclass_get_device_by_driver(UCLASS_WDT, + DM_DRIVER_GET(wdt_sandbox), &sandbox_wdt)); + ut_assertnonnull(sandbox_wdt); + ut_assertok(uclass_get_device_by_name(UCLASS_GPIO, "base-gpios", &gpio)); + ut_assertnonnull(gpio); + + /* Neither device should be "started", so watchdog_reset() should be a no-op. */ + reset_count = state->wdt.reset_count; + val = sandbox_gpio_get_value(gpio, offset); + watchdog_reset(); + ut_asserteq(reset_count, state->wdt.reset_count); + ut_asserteq(val, sandbox_gpio_get_value(gpio, offset)); + + /* Start both devices. */ + ut_assertok(wdt_start(gpio_wdt, timeout, 0)); + ut_assertok(wdt_start(sandbox_wdt, timeout, 0)); + + /* Make sure both devices have just been pinged. */ + timer_test_add_offset(100); + watchdog_reset(); + reset_count = state->wdt.reset_count; + val = sandbox_gpio_get_value(gpio, offset); + + /* The gpio watchdog should be pinged, the sandbox one not. */ + timer_test_add_offset(30); + watchdog_reset(); + ut_asserteq(reset_count, state->wdt.reset_count); + ut_asserteq(!val, sandbox_gpio_get_value(gpio, offset)); + + /* After another ~30ms, both devices should get pinged. */ + timer_test_add_offset(30); + watchdog_reset(); + ut_asserteq(reset_count + 1, state->wdt.reset_count); + ut_asserteq(val, sandbox_gpio_get_value(gpio, offset)); + + return 0; +} +DM_TEST(dm_test_wdt_watchdog_reset, UT_TESTF_SCAN_FDT);