[PATCH v3 00/10] handling all DM watchdogs in watchdog_reset()

[PATCH v3 00/10] handling all DM watchdogs in watchdog_reset()

[Rasmus Villemoes]

This series is an attempt at expanding the wdt-uclass provided
watchdog_reset() to handle all DM watchdogs, not just the first
one. As a sort of nice side effect, it turns out that doing that makes
wdt-uclass fit better into the uclass model, in that we now do the
work that was previously done in the ad hoc initr_watchdog for one
device in .pre_probe and .post_probe hooks instead.

It also includes a patch adding a driver for a gpio-petted watchdog
device (and a sandbox test of that) - it is included here because that
also gives a relatively easy way to have more than one (kind of)
watchdog device in the sandbox, which is then used at the end to test
that watchdog_reset() behaves as expected.

v2 here: https://lore.kernel.org/u-boot/20210527220017.1266765-1-rasmus.villemoes@prevas.dk/

Changes in v3:

- Rebase to master (b7ad721c83)

- Add various R-b tags from Stefan and Simon

- Add comments to members of struct wdt_priv in patches 2 and 5.

- Change a newly introduced printf(), for failing uclass_get(), in
  patch 7 to log_debug (but keep existing printfs as-is, apart from
  the inclusion of device names).

- Add another log_debug in patch 7 if device probing fails.

- Add comment before uclass_foreach_dev() loop in watchdog_reset(),
  and furthermore include a device_active() check inside the loop.
  
- Drop unneeded <common.h> include in patch 8.

- Use timer_test_add_offset() instead of mdelay() in patch 10.

Rasmus Villemoes (10):
  watchdog: wdt-uclass.c: use wdt_start() in wdt_expire_now()
  watchdog: wdt-uclass.c: introduce struct wdt_priv
  watchdog: wdt-uclass.c: neaten UCLASS_DRIVER definition
  watchdog: wdt-uclass.c: refactor initr_watchdog()
  watchdog: wdt-uclass.c: keep track of each device's running state
  sandbox: disable CONFIG_WATCHDOG_AUTOSTART
  watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()
  watchdog: add gpio watchdog driver
  sandbox: add test of wdt_gpio driver
  sandbox: add test of wdt-uclass' watchdog_reset()

 arch/sandbox/dts/test.dts                     |   8 +
 configs/sandbox64_defconfig                   |   2 +
 configs/sandbox_defconfig                     |   2 +
 .../watchdog/gpio-wdt.txt                     |  19 ++
 drivers/watchdog/Kconfig                      |   9 +
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/gpio_wdt.c                   |  68 +++++++
 drivers/watchdog/wdt-uclass.c                 | 172 ++++++++++++------
 include/asm-generic/global_data.h             |   6 -
 test/dm/wdt.c                                 |  90 ++++++++-
 10 files changed, 316 insertions(+), 61 deletions(-)
 create mode 100644 doc/device-tree-bindings/watchdog/gpio-wdt.txt
 create mode 100644 drivers/watchdog/gpio_wdt.c

-- 
2.31.1

[PATCH v3 01/10] watchdog: wdt-uclass.c: use wdt_start() in wdt_expire_now()

[Rasmus Villemoes]

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 2687135296..634428fa24 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -118,10 +118,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;
 
-- 
2.31.1

[PATCH v3 02/10] watchdog: wdt-uclass.c: introduce struct wdt_priv

[Rasmus Villemoes]

As preparation for having the wdt-uclass provided watchdog_reset()
function handle all DM watchdog devices, and not just the first such,
introduce a uclass-owned struct to hold the reset_period and
next_reset, so these become per-device instead of being static
variables.

No functional change intended.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/wdt-uclass.c | 74 +++++++++++++++++++++++++----------
 1 file changed, 54 insertions(+), 20 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 634428fa24..d0c1630cd4 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -18,15 +18,24 @@ DECLARE_GLOBAL_DATA_PTR;
 
 #define WATCHDOG_TIMEOUT_SECS	(CONFIG_WATCHDOG_TIMEOUT_MSECS / 1000)
 
-/*
- * Reset every 1000ms, or however often is required as indicated by a
- * hw_margin_ms property.
- */
-static ulong reset_period = 1000;
+struct wdt_priv {
+	/* Timeout, in seconds, to configure this device to. */
+	u32 timeout;
+	/*
+	 * Time, in milliseconds, between calling the device's ->reset()
+	 * method from watchdog_reset().
+	 */
+	ulong reset_period;
+	/*
+	 * Next time (as returned by get_timer(0)) to call
+	 * ->reset().
+	 */
+	ulong next_reset;
+};
 
 int initr_watchdog(void)
 {
-	u32 timeout = WATCHDOG_TIMEOUT_SECS;
+	struct wdt_priv *priv;
 	int ret;
 
 	/*
@@ -42,28 +51,21 @@ int initr_watchdog(void)
 			return 0;
 		}
 	}
-
-	if (CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)) {
-		timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec",
-					       WATCHDOG_TIMEOUT_SECS);
-		reset_period = dev_read_u32_default(gd->watchdog_dev,
-						    "hw_margin_ms",
-						    4 * reset_period) / 4;
-	}
+	priv = dev_get_uclass_priv(gd->watchdog_dev);
 
 	if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) {
 		printf("WDT:   Not starting\n");
 		return 0;
 	}
 
-	ret = wdt_start(gd->watchdog_dev, timeout * 1000, 0);
+	ret = wdt_start(gd->watchdog_dev, priv->timeout * 1000, 0);
 	if (ret != 0) {
 		printf("WDT:   Failed to start\n");
 		return 0;
 	}
 
 	printf("WDT:   Started with%s servicing (%ds timeout)\n",
-	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", timeout);
+	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout);
 
 	return 0;
 }
@@ -137,18 +139,21 @@ int wdt_expire_now(struct udevice *dev, ulong flags)
  */
 void watchdog_reset(void)
 {
-	static ulong next_reset;
+	struct wdt_priv *priv;
+	struct udevice *dev;
 	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);
 	/* Do not reset the watchdog too often */
 	now = get_timer(0);
-	if (time_after_eq(now, next_reset)) {
-		next_reset = now + reset_period;
-		wdt_reset(gd->watchdog_dev);
+	if (time_after_eq(now, priv->next_reset)) {
+		priv->next_reset = now + priv->reset_period;
+		wdt_reset(dev);
 	}
 }
 #endif
@@ -175,9 +180,38 @@ static int wdt_post_bind(struct udevice *dev)
 	return 0;
 }
 
+static int wdt_pre_probe(struct udevice *dev)
+{
+	u32 timeout = WATCHDOG_TIMEOUT_SECS;
+	/*
+	 * Reset every 1000ms, or however often is required as
+	 * indicated by a hw_margin_ms property.
+	 */
+	ulong reset_period = 1000;
+	struct wdt_priv *priv;
+
+	if (CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)) {
+		timeout = dev_read_u32_default(dev, "timeout-sec", timeout);
+		reset_period = dev_read_u32_default(dev, "hw_margin_ms",
+						    4 * reset_period) / 4;
+	}
+	priv = dev_get_uclass_priv(dev);
+	priv->timeout = timeout;
+	priv->reset_period = reset_period;
+	/*
+	 * Pretend this device was last reset "long" ago so the first
+	 * watchdog_reset will actually call its ->reset method.
+	 */
+	priv->next_reset = get_timer(0);
+
+	return 0;
+}
+
 UCLASS_DRIVER(wdt) = {
 	.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),
 };
-- 
2.31.1

[PATCH v3 03/10] watchdog: wdt-uclass.c: neaten UCLASS_DRIVER definition

[Rasmus Villemoes]

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 d0c1630cd4..8ddba9b046 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -208,10 +208,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),
 };
-- 
2.31.1

[PATCH v3 04/10] watchdog: wdt-uclass.c: refactor initr_watchdog()

[Rasmus Villemoes]

In preparation for handling all DM watchdogs in watchdog_reset(), pull
out the code which handles starting (or not) the gd->watchdog_dev
device.

Include the device name in various printfs.

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 | 37 ++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 8ddba9b046..32f5b1d0f2 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -33,11 +33,30 @@ struct wdt_priv {
 	ulong next_reset;
 };
 
-int initr_watchdog(void)
+static void init_watchdog_dev(struct udevice *dev)
 {
 	struct wdt_priv *priv;
 	int ret;
 
+	priv = dev_get_uclass_priv(dev);
+
+	if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) {
+		printf("WDT:   Not starting %s\n", dev->name);
+		return;
+	}
+
+	ret = wdt_start(dev, priv->timeout * 1000, 0);
+	if (ret != 0) {
+		printf("WDT:   Failed to start %s\n", dev->name);
+		return;
+	}
+
+	printf("WDT:   Started %s with%s servicing (%ds timeout)\n", dev->name,
+	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout);
+}
+
+int initr_watchdog(void)
+{
 	/*
 	 * Init watchdog: This will call the probe function of the
 	 * watchdog driver, enabling the use of the device
@@ -51,21 +70,7 @@ int initr_watchdog(void)
 			return 0;
 		}
 	}
-	priv = dev_get_uclass_priv(gd->watchdog_dev);
-
-	if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) {
-		printf("WDT:   Not starting\n");
-		return 0;
-	}
-
-	ret = wdt_start(gd->watchdog_dev, priv->timeout * 1000, 0);
-	if (ret != 0) {
-		printf("WDT:   Failed to start\n");
-		return 0;
-	}
-
-	printf("WDT:   Started with%s servicing (%ds timeout)\n",
-	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout);
+	init_watchdog_dev(gd->watchdog_dev);
 
 	return 0;
 }
-- 
2.31.1

[PATCH v3 05/10] watchdog: wdt-uclass.c: keep track of each device's running state

[Rasmus Villemoes]

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 32f5b1d0f2..ca20c124d4 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -31,6 +31,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)
@@ -72,6 +74,7 @@ int initr_watchdog(void)
 	}
 	init_watchdog_dev(gd->watchdog_dev);
 
+	gd->flags |= GD_FLG_WDT_READY;
 	return 0;
 }
 
@@ -84,8 +87,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;
 }
@@ -99,8 +105,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;
 }
@@ -154,6 +163,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)) {
-- 
2.31.1

[PATCH v3 06/10] sandbox: disable CONFIG_WATCHDOG_AUTOSTART

[Rasmus Villemoes]

A later refactoring will put most of initr_watchdog() into a
.post_probe function, which means that all watchdog devices end up
being subject to the auto-start functionality. For the unit tests, it
is more convenient if the tests are in charge of when the watchdog
devices are started and stopped.

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 fafa85a5c0..bf436963ec 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 f279bd1a0f..56254e1ea2 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -270,6 +270,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
-- 
2.31.1

[PATCH v3 07/10] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

[Rasmus Villemoes]

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. This essentially boils down to making the init_watchdog_dev
function into a .post_probe method.

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: Stefan Roese <sr@denx.de>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/wdt-uclass.c     | 96 ++++++++++++++++++-------------
 include/asm-generic/global_data.h |  6 --
 2 files changed, 56 insertions(+), 46 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index ca20c124d4..0972541148 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -35,44 +35,23 @@ struct wdt_priv {
 	bool running;
 };
 
-static void init_watchdog_dev(struct udevice *dev)
+int initr_watchdog(void)
 {
-	struct wdt_priv *priv;
+	struct udevice *dev;
+	struct uclass *uc;
 	int ret;
 
-	priv = dev_get_uclass_priv(dev);
-
-	if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) {
-		printf("WDT:   Not starting %s\n", dev->name);
-		return;
-	}
-
-	ret = wdt_start(dev, priv->timeout * 1000, 0);
-	if (ret != 0) {
-		printf("WDT:   Failed to start %s\n", dev->name);
-		return;
+	ret = uclass_get(UCLASS_WDT, &uc);
+	if (ret) {
+		log_debug("Error getting UCLASS_WDT: %d\n", ret);
+		return 0;
 	}
 
-	printf("WDT:   Started %s with%s servicing (%ds timeout)\n", dev->name,
-	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout);
-}
-
-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;
-		}
+	uclass_foreach_dev(dev, uc) {
+		ret = device_probe(dev);
+		if (ret)
+			log_debug("Error probing %s: %d\n", dev->name, ret);
 	}
-	init_watchdog_dev(gd->watchdog_dev);
 
 	gd->flags |= GD_FLG_WDT_READY;
 	return 0;
@@ -155,22 +134,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
@@ -224,11 +215,36 @@ static int wdt_pre_probe(struct udevice *dev)
 	return 0;
 }
 
+static int wdt_post_probe(struct udevice *dev)
+{
+	struct wdt_priv *priv;
+	int ret;
+
+	priv = dev_get_uclass_priv(dev);
+
+	if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) {
+		printf("WDT:   Not starting %s\n", dev->name);
+		return 0;
+	}
+
+	ret = wdt_start(dev, priv->timeout * 1000, 0);
+	if (ret != 0) {
+		printf("WDT:   Failed to start %s\n", dev->name);
+		return 0;
+	}
+
+	printf("WDT:   Started %s with%s servicing (%ds timeout)\n", dev->name,
+	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout);
+
+	return 0;
+}
+
 UCLASS_DRIVER(wdt) = {
 	.id			= UCLASS_WDT,
 	.name			= "watchdog",
 	.flags			= DM_UC_FLAG_SEQ_ALIAS,
 	.post_bind		= wdt_post_bind,
 	.pre_probe		= wdt_pre_probe,
+	.post_probe		= wdt_post_probe,
 	.per_device_auto	= sizeof(struct wdt_priv),
 };
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index 47921d27b1..b554016628 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -445,12 +445,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
-- 
2.31.1

[PATCH v3 08/10] watchdog: add gpio watchdog driver

[Rasmus Villemoes]

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_giA@mail.gmail.com/

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),
+};
-- 
2.31.1

[PATCH v3 09/10] sandbox: add test of wdt_gpio driver

[Rasmus Villemoes]

It seems that no other test has claimed gpio_a:7 yet, so use that.

The only small wrinkle is modifying the existing wdt test to use
uclass_get_device_by_driver() since we now have two UCLASS_WDT
instances in play, so it's a little more robust to fetch the device by
driver and not merely uclass+index.

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   |  6 ++++++
 configs/sandbox64_defconfig |  1 +
 configs/sandbox_defconfig   |  1 +
 test/dm/wdt.c               | 36 +++++++++++++++++++++++++++++++++++-
 4 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 8e7eaf2d15..f0be050759 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -755,6 +755,12 @@
 		};
 	};
 
+	gpio-wdt {
+		gpios = <&gpio_a 7 0>;
+		compatible = "linux,wdt-gpio";
+		always-running;
+	};
+
 	mbox: mbox {
 		compatible = "sandbox,mbox";
 		#mbox-cells = <1>;
diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index bf436963ec..b941149dfb 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -225,6 +225,7 @@ CONFIG_SPLASH_SCREEN_ALIGN=y
 CONFIG_VIDEO_BMP_RLE8=y
 # CONFIG_WATCHDOG_AUTOSTART is not set
 CONFIG_WDT=y
+CONFIG_WDT_GPIO=y
 CONFIG_WDT_SANDBOX=y
 CONFIG_FS_CBFS=y
 CONFIG_FS_CRAMFS=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 56254e1ea2..84145b8383 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -272,6 +272,7 @@ CONFIG_W1_EEPROM=y
 CONFIG_W1_EEPROM_SANDBOX=y
 # CONFIG_WATCHDOG_AUTOSTART is not set
 CONFIG_WDT=y
+CONFIG_WDT_GPIO=y
 CONFIG_WDT_SANDBOX=y
 CONFIG_FS_CBFS=y
 CONFIG_FS_CRAMFS=y
diff --git a/test/dm/wdt.c b/test/dm/wdt.c
index 24b991dff6..abff853a02 100644
--- a/test/dm/wdt.c
+++ b/test/dm/wdt.c
@@ -6,6 +6,7 @@
 #include <common.h>
 #include <dm.h>
 #include <wdt.h>
+#include <asm/gpio.h>
 #include <asm/state.h>
 #include <asm/test.h>
 #include <dm/test.h>
@@ -19,7 +20,8 @@ static int dm_test_wdt_base(struct unit_test_state *uts)
 	struct udevice *dev;
 	const u64 timeout = 42;
 
-	ut_assertok(uclass_get_device(UCLASS_WDT, 0, &dev));
+	ut_assertok(uclass_get_device_by_driver(UCLASS_WDT,
+						DM_DRIVER_GET(wdt_sandbox), &dev));
 	ut_assertnonnull(dev);
 	ut_asserteq(0, state->wdt.counter);
 	ut_asserteq(false, state->wdt.running);
@@ -39,3 +41,35 @@ static int dm_test_wdt_base(struct unit_test_state *uts)
 	return 0;
 }
 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)
+{
+	/*
+	 * 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.
+	 */
+	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_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, UT_TESTF_SCAN_FDT);
-- 
2.31.1

[PATCH v3 10/10] sandbox: add test of wdt-uclass' watchdog_reset()

[Rasmus Villemoes]

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 f0be050759..78efd3bc10 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -758,6 +758,7 @@
 	gpio-wdt {
 		gpios = <&gpio_a 7 0>;
 		compatible = "linux,wdt-gpio";
+		hw_margin_ms = <100>;
 		always-running;
 	};
 
@@ -1239,6 +1240,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);
-- 
2.31.1

Re: [PATCH v3 02/10] watchdog: wdt-uclass.c: introduce struct wdt_priv

[Stefan Roese]

On 02.07.21 14:45, Rasmus Villemoes wrote:
> As preparation for having the wdt-uclass provided watchdog_reset()
> function handle all DM watchdog devices, and not just the first such,
> introduce a uclass-owned struct to hold the reset_period and
> next_reset, so these become per-device instead of being static
> variables.
> 
> No functional change intended.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Again:

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

Thanks,
Stefan

> ---
>   drivers/watchdog/wdt-uclass.c | 74 +++++++++++++++++++++++++----------
>   1 file changed, 54 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 634428fa24..d0c1630cd4 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -18,15 +18,24 @@ DECLARE_GLOBAL_DATA_PTR;
>   
>   #define WATCHDOG_TIMEOUT_SECS	(CONFIG_WATCHDOG_TIMEOUT_MSECS / 1000)
>   
> -/*
> - * Reset every 1000ms, or however often is required as indicated by a
> - * hw_margin_ms property.
> - */
> -static ulong reset_period = 1000;
> +struct wdt_priv {
> +	/* Timeout, in seconds, to configure this device to. */
> +	u32 timeout;
> +	/*
> +	 * Time, in milliseconds, between calling the device's ->reset()
> +	 * method from watchdog_reset().
> +	 */
> +	ulong reset_period;
> +	/*
> +	 * Next time (as returned by get_timer(0)) to call
> +	 * ->reset().
> +	 */
> +	ulong next_reset;
> +};
>   
>   int initr_watchdog(void)
>   {
> -	u32 timeout = WATCHDOG_TIMEOUT_SECS;
> +	struct wdt_priv *priv;
>   	int ret;
>   
>   	/*
> @@ -42,28 +51,21 @@ int initr_watchdog(void)
>   			return 0;
>   		}
>   	}
> -
> -	if (CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)) {
> -		timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec",
> -					       WATCHDOG_TIMEOUT_SECS);
> -		reset_period = dev_read_u32_default(gd->watchdog_dev,
> -						    "hw_margin_ms",
> -						    4 * reset_period) / 4;
> -	}
> +	priv = dev_get_uclass_priv(gd->watchdog_dev);
>   
>   	if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) {
>   		printf("WDT:   Not starting\n");
>   		return 0;
>   	}
>   
> -	ret = wdt_start(gd->watchdog_dev, timeout * 1000, 0);
> +	ret = wdt_start(gd->watchdog_dev, priv->timeout * 1000, 0);
>   	if (ret != 0) {
>   		printf("WDT:   Failed to start\n");
>   		return 0;
>   	}
>   
>   	printf("WDT:   Started with%s servicing (%ds timeout)\n",
> -	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", timeout);
> +	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout);
>   
>   	return 0;
>   }
> @@ -137,18 +139,21 @@ int wdt_expire_now(struct udevice *dev, ulong flags)
>    */
>   void watchdog_reset(void)
>   {
> -	static ulong next_reset;
> +	struct wdt_priv *priv;
> +	struct udevice *dev;
>   	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);
>   	/* Do not reset the watchdog too often */
>   	now = get_timer(0);
> -	if (time_after_eq(now, next_reset)) {
> -		next_reset = now + reset_period;
> -		wdt_reset(gd->watchdog_dev);
> +	if (time_after_eq(now, priv->next_reset)) {
> +		priv->next_reset = now + priv->reset_period;
> +		wdt_reset(dev);
>   	}
>   }
>   #endif
> @@ -175,9 +180,38 @@ static int wdt_post_bind(struct udevice *dev)
>   	return 0;
>   }
>   
> +static int wdt_pre_probe(struct udevice *dev)
> +{
> +	u32 timeout = WATCHDOG_TIMEOUT_SECS;
> +	/*
> +	 * Reset every 1000ms, or however often is required as
> +	 * indicated by a hw_margin_ms property.
> +	 */
> +	ulong reset_period = 1000;
> +	struct wdt_priv *priv;
> +
> +	if (CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)) {
> +		timeout = dev_read_u32_default(dev, "timeout-sec", timeout);
> +		reset_period = dev_read_u32_default(dev, "hw_margin_ms",
> +						    4 * reset_period) / 4;
> +	}
> +	priv = dev_get_uclass_priv(dev);
> +	priv->timeout = timeout;
> +	priv->reset_period = reset_period;
> +	/*
> +	 * Pretend this device was last reset "long" ago so the first
> +	 * watchdog_reset will actually call its ->reset method.
> +	 */
> +	priv->next_reset = get_timer(0);
> +
> +	return 0;
> +}
> +
>   UCLASS_DRIVER(wdt) = {
>   	.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),
>   };
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH v3 07/10] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

[Simon Glass]

Hi Rasmus,

On Fri, 2 Jul 2021 at 06:45, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> 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. This essentially boils down to making the init_watchdog_dev
> function into a .post_probe method.
>
> 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: Stefan Roese <sr@denx.de>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/watchdog/wdt-uclass.c     | 96 ++++++++++++++++++-------------
>  include/asm-generic/global_data.h |  6 --
>  2 files changed, 56 insertions(+), 46 deletions(-)
>

Please see my belated reply on the previous version of this patch.

Regards,
Simon

Re: [PATCH v3 07/10] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

[Stefan Roese]

Hi Rasmus,

On 05.07.21 17:30, Simon Glass wrote:
> Hi Rasmus,
> 
> On Fri, 2 Jul 2021 at 06:45, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> 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. This essentially boils down to making the init_watchdog_dev
>> function into a .post_probe method.
>>
>> 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: Stefan Roese <sr@denx.de>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>   drivers/watchdog/wdt-uclass.c     | 96 ++++++++++++++++++-------------
>>   include/asm-generic/global_data.h |  6 --
>>   2 files changed, 56 insertions(+), 46 deletions(-)
>>
> 
> Please see my belated reply on the previous version of this patch.

Rasmus, do you plan to send an updated patchset version, addressing
Simons's comments?

Thanks,
Stefan

Re: [PATCH v3 07/10] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

[Stefan Roese]

Hi Rasmus,

On 15.07.21 10:15, Stefan Roese wrote:
> Hi Rasmus,
> 
> On 05.07.21 17:30, Simon Glass wrote:
>> Hi Rasmus,
>>
>> On Fri, 2 Jul 2021 at 06:45, Rasmus Villemoes
>> <rasmus.villemoes@prevas.dk> wrote:
>>>
>>> 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. This essentially boils down to making the init_watchdog_dev
>>> function into a .post_probe method.
>>>
>>> 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: Stefan Roese <sr@denx.de>
>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>> ---
>>>   drivers/watchdog/wdt-uclass.c     | 96 ++++++++++++++++++-------------
>>>   include/asm-generic/global_data.h |  6 --
>>>   2 files changed, 56 insertions(+), 46 deletions(-)
>>>
>>
>> Please see my belated reply on the previous version of this patch.
> 
> Rasmus, do you plan to send an updated patchset version, addressing
> Simons's comments?

Any updates on this?

Thanks,
Stefan

Re: [PATCH v3 07/10] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

[Rasmus Villemoes]

On 31/07/2021 12.06, Stefan Roese wrote:
> Hi Rasmus,
> 
> On 15.07.21 10:15, Stefan Roese wrote:
>> Hi Rasmus,
>>
>> On 05.07.21 17:30, Simon Glass wrote:
>>> Hi Rasmus,
>>>
>>> On Fri, 2 Jul 2021 at 06:45, Rasmus Villemoes
>>> <rasmus.villemoes@prevas.dk> wrote:
>>>>
>>>> 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. This essentially boils down to making the init_watchdog_dev
>>>> function into a .post_probe method.
>>>>
>>>> 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: Stefan Roese <sr@denx.de>
>>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>>> ---
>>>>   drivers/watchdog/wdt-uclass.c     | 96
>>>> ++++++++++++++++++-------------
>>>>   include/asm-generic/global_data.h |  6 --
>>>>   2 files changed, 56 insertions(+), 46 deletions(-)
>>>>
>>>
>>> Please see my belated reply on the previous version of this patch.
>>
>> Rasmus, do you plan to send an updated patchset version, addressing
>> Simons's comments?
> 
> Any updates on this?

Just got back from vacation and trying to catch up.

I'm a little confused. On June 29, you said

  I'm fine with this post_probe() implementation but have no strong
  feelings about this. So if Simon (or someone else) does not object,
  then please continue this way.

Then Simon does reply on July 5

  Imagine you probe
  the device but then go into SDRAM init that hangs the CPU for a few
  seconds. We need a way to signal that we want the watchdog to start,
  so the board owner has a choice as to when this happens.

[The board owner does have a choice, it's called CONFIG_WATCHDOG_AUTOSTART.]

  I also understand this is not quite how things work at present and I'm
  fine with copying the existing behaviour.

which I didn't read as an objection, as I am precisely not changing any
existing behaviour. As I've said previously, keeping what is done in my
current post_probe function in a separate helper, called from inside the
loop in initr_watchdog which probes all watchdog devices, won't change
anything at all.

But precisely because it won't change anything, in the interest of
getting the gpio driver in, I'll send a v4 with that change alone, then
I'll let Stefan pick whichever version he and Simon can agree to.

But let me one last time repeat why I think the post_probe approach is
the cleanest and a natural fit for DM: post_probe is (AIUI) a place
where a uclass can do some action it wants done for every device
belonging to that uclass. When CONFIG_WATCHDOG_AUTOSTART is set,
wdt_uclass does have such an action. When it's not set, the post_probe
method is a no-op.

Rasmus

Re: [PATCH v3 07/10] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

[Stefan Roese]

On 02.08.21 11:18, Rasmus Villemoes wrote:
> On 31/07/2021 12.06, Stefan Roese wrote:
>> Hi Rasmus,
>>
>> On 15.07.21 10:15, Stefan Roese wrote:
>>> Hi Rasmus,
>>>
>>> On 05.07.21 17:30, Simon Glass wrote:
>>>> Hi Rasmus,
>>>>
>>>> On Fri, 2 Jul 2021 at 06:45, Rasmus Villemoes
>>>> <rasmus.villemoes@prevas.dk> wrote:
>>>>>
>>>>> 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. This essentially boils down to making the init_watchdog_dev
>>>>> function into a .post_probe method.
>>>>>
>>>>> 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: Stefan Roese <sr@denx.de>
>>>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>>>> ---
>>>>>    drivers/watchdog/wdt-uclass.c     | 96
>>>>> ++++++++++++++++++-------------
>>>>>    include/asm-generic/global_data.h |  6 --
>>>>>    2 files changed, 56 insertions(+), 46 deletions(-)
>>>>>
>>>>
>>>> Please see my belated reply on the previous version of this patch.
>>>
>>> Rasmus, do you plan to send an updated patchset version, addressing
>>> Simons's comments?
>>
>> Any updates on this?
> 
> Just got back from vacation and trying to catch up.
> 
> I'm a little confused. On June 29, you said
> 
>    I'm fine with this post_probe() implementation but have no strong
>    feelings about this. So if Simon (or someone else) does not object,
>    then please continue this way.
> 
> Then Simon does reply on July 5
> 
>    Imagine you probe
>    the device but then go into SDRAM init that hangs the CPU for a few
>    seconds. We need a way to signal that we want the watchdog to start,
>    so the board owner has a choice as to when this happens.
> 
> [The board owner does have a choice, it's called CONFIG_WATCHDOG_AUTOSTART.]
> 
>    I also understand this is not quite how things work at present and I'm
>    fine with copying the existing behaviour.
> 
> which I didn't read as an objection, as I am precisely not changing any
> existing behaviour. As I've said previously, keeping what is done in my
> current post_probe function in a separate helper, called from inside the
> loop in initr_watchdog which probes all watchdog devices, won't change
> anything at all.
> 
> But precisely because it won't change anything, in the interest of
> getting the gpio driver in, I'll send a v4 with that change alone, then
> I'll let Stefan pick whichever version he and Simon can agree to.
> 
> But let me one last time repeat why I think the post_probe approach is
> the cleanest and a natural fit for DM: post_probe is (AIUI) a place
> where a uclass can do some action it wants done for every device
> belonging to that uclass. When CONFIG_WATCHDOG_AUTOSTART is set,
> wdt_uclass does have such an action. When it's not set, the post_probe
> method is a no-op.

I might have misunderstood Simon. So if Simon is "okay" with v3, then
I can push this version soon - no need to send a v4 then.

Simon?

Thanks,
Stefan

Re: [PATCH v3 07/10] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

[Simon Glass]

Hi Rasmus,

On Mon, 2 Aug 2021 at 03:18, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 31/07/2021 12.06, Stefan Roese wrote:
> > Hi Rasmus,
> >
> > On 15.07.21 10:15, Stefan Roese wrote:
> >> Hi Rasmus,
> >>
> >> On 05.07.21 17:30, Simon Glass wrote:
> >>> Hi Rasmus,
> >>>
> >>> On Fri, 2 Jul 2021 at 06:45, Rasmus Villemoes
> >>> <rasmus.villemoes@prevas.dk> wrote:
> >>>>
> >>>> 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. This essentially boils down to making the init_watchdog_dev
> >>>> function into a .post_probe method.
> >>>>
> >>>> 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: Stefan Roese <sr@denx.de>
> >>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> >>>> ---
> >>>>   drivers/watchdog/wdt-uclass.c     | 96
> >>>> ++++++++++++++++++-------------
> >>>>   include/asm-generic/global_data.h |  6 --
> >>>>   2 files changed, 56 insertions(+), 46 deletions(-)
> >>>>
> >>>
> >>> Please see my belated reply on the previous version of this patch.
> >>
> >> Rasmus, do you plan to send an updated patchset version, addressing
> >> Simons's comments?
> >
> > Any updates on this?
>
> Just got back from vacation and trying to catch up.
>
> I'm a little confused. On June 29, you said
>
>   I'm fine with this post_probe() implementation but have no strong
>   feelings about this. So if Simon (or someone else) does not object,
>   then please continue this way.
>
> Then Simon does reply on July 5
>
>   Imagine you probe
>   the device but then go into SDRAM init that hangs the CPU for a few
>   seconds. We need a way to signal that we want the watchdog to start,
>   so the board owner has a choice as to when this happens.
>
> [The board owner does have a choice, it's called CONFIG_WATCHDOG_AUTOSTART.]
>
>   I also understand this is not quite how things work at present and I'm
>   fine with copying the existing behaviour.
>
> which I didn't read as an objection, as I am precisely not changing any
> existing behaviour. As I've said previously, keeping what is done in my
> current post_probe function in a separate helper, called from inside the
> loop in initr_watchdog which probes all watchdog devices, won't change
> anything at all.
>
> But precisely because it won't change anything, in the interest of
> getting the gpio driver in, I'll send a v4 with that change alone, then
> I'll let Stefan pick whichever version he and Simon can agree to.
>
> But let me one last time repeat why I think the post_probe approach is
> the cleanest and a natural fit for DM: post_probe is (AIUI) a place
> where a uclass can do some action it wants done for every device
> belonging to that uclass. When CONFIG_WATCHDOG_AUTOSTART is set,
> wdt_uclass does have such an action. When it's not set, the post_probe
> method is a no-op.

I can certainly see you point. It definitely looks very natural for DM.

But I still think the problem of a device possibly resetting the board
when probed (despite the various watchdog-reset things sprinkled
throughout the code) many means it is worth having an explicit 'start'
operation in this case. In fact this fits with DM even better, since
probe() is supposed to just fire up the hardware, not start any
operations.

Regards,
Simon

Re: [PATCH v3 07/10] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

[Rasmus Villemoes]

On 02/08/2021 21.20, Simon Glass wrote:
> Hi Rasmus,
> 
>> But let me one last time repeat why I think the post_probe approach is
>> the cleanest and a natural fit for DM: post_probe is (AIUI) a place
>> where a uclass can do some action it wants done for every device
>> belonging to that uclass. When CONFIG_WATCHDOG_AUTOSTART is set,
>> wdt_uclass does have such an action. When it's not set, the post_probe
>> method is a no-op.
> 
> I can certainly see you point. It definitely looks very natural for DM.
> 
> But I still think the problem of a device possibly resetting the board
> when probed (despite the various watchdog-reset things sprinkled
> throughout the code)

But that is the _very point_ of having the developer be able to opt in
[1] to starting the watchdog device ASAP (i.e. as soon as U-Boot is
capable of handling it). If some part of the boot process hangs or
U-Boot goes into an inf-loop without hitting a WATCHDOG_RESET in there,
one wants (or may want) the board to reset and hopefully follow some
other path the next time around. If the developer/project doesn't want
or need that, WATCHDOG_AUTOSTART can be disabled. And if the developer
wants to make use of this, it makes sense to have the watchdog device
monitor as much of the boot process as possible.

[1] ok, for historical reasons it's default-y, so actually an opt-out
knob, but the developer does have a choice regardless.

Rasmus

Re: [PATCH v3 07/10] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

[Simon Glass]

Hi Rasmus,

On Tue, 3 Aug 2021 at 00:48, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 02/08/2021 21.20, Simon Glass wrote:
> > Hi Rasmus,
> >
> >> But let me one last time repeat why I think the post_probe approach is
> >> the cleanest and a natural fit for DM: post_probe is (AIUI) a place
> >> where a uclass can do some action it wants done for every device
> >> belonging to that uclass. When CONFIG_WATCHDOG_AUTOSTART is set,
> >> wdt_uclass does have such an action. When it's not set, the post_probe
> >> method is a no-op.
> >
> > I can certainly see you point. It definitely looks very natural for DM.
> >
> > But I still think the problem of a device possibly resetting the board
> > when probed (despite the various watchdog-reset things sprinkled
> > throughout the code)
>
> But that is the _very point_ of having the developer be able to opt in
> [1] to starting the watchdog device ASAP (i.e. as soon as U-Boot is
> capable of handling it). If some part of the boot process hangs or
> U-Boot goes into an inf-loop without hitting a WATCHDOG_RESET in there,
> one wants (or may want) the board to reset and hopefully follow some
> other path the next time around. If the developer/project doesn't want
> or need that, WATCHDOG_AUTOSTART can be disabled. And if the developer

That requires a rebuild though.

This is more of a driver-model thing than anything else and I know it
seems like a minor point. But the way it is designed at present,
probing a device should get it ready for use and not actually start it
doing something. That is the way all(?) devices work and I think it
makes sense to keep watchdogs consistent with that.

> wants to make use of this, it makes sense to have the watchdog device
> monitor as much of the boot process as possible.

This is not an issue, since you enable all watchdogs immediately at present.

>
> [1] ok, for historical reasons it's default-y, so actually an opt-out
> knob, but the developer does have a choice regardless.
>

Regards,
Simon