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

[PATCH v2 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.

It's called v2 merely because the gpio driver and associated test has
been sent before; all but those two patches (8 and 9) are new. As for
the changes in those compared to the first submission, they are mostly
cosmetic - functionally, the only change is that it now requires the
always-running property in DT.

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                   |  69 ++++++++
 drivers/watchdog/wdt-uclass.c                 | 154 +++++++++++-------
 include/asm-generic/global_data.h             |   6 -
 test/dm/wdt.c                                 |  90 +++++++++-
 10 files changed, 298 insertions(+), 62 deletions(-)
 create mode 100644 doc/device-tree-bindings/watchdog/gpio-wdt.txt
 create mode 100644 drivers/watchdog/gpio_wdt.c

-- 
2.31.1

[PATCH v2 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.

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 v2 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.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/wdt-uclass.c | 65 ++++++++++++++++++++++++-----------
 1 file changed, 45 insertions(+), 20 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 634428fa24..3b6aa3d586 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -18,15 +18,15 @@ 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 {
+	u32 timeout;
+	ulong reset_period;
+	ulong next_reset;
+};
 
 int initr_watchdog(void)
 {
-	u32 timeout = WATCHDOG_TIMEOUT_SECS;
+	struct wdt_priv *priv;
 	int ret;
 
 	/*
@@ -42,28 +42,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 +130,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 +171,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 v2 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.

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 3b6aa3d586..360f052227 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -199,10 +199,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 v2 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.

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 360f052227..5d5dbcd0f7 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -24,11 +24,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
@@ -42,21 +61,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 v2 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.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/wdt-uclass.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 5d5dbcd0f7..026b6d1eb4 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -22,6 +22,7 @@ struct wdt_priv {
 	u32 timeout;
 	ulong reset_period;
 	ulong next_reset;
+	bool running;
 };
 
 static void init_watchdog_dev(struct udevice *dev)
@@ -63,6 +64,7 @@ int initr_watchdog(void)
 	}
 	init_watchdog_dev(gd->watchdog_dev);
 
+	gd->flags |= GD_FLG_WDT_READY;
 	return 0;
 }
 
@@ -75,8 +77,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;
 }
@@ -90,8 +95,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;
 }
@@ -145,6 +153,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 v2 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.

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 9a373bab6f..8b934698b5 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 bdbf714e2b..8911463d58 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 v2 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.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/wdt-uclass.c     | 87 ++++++++++++++++---------------
 include/asm-generic/global_data.h |  6 ---
 2 files changed, 46 insertions(+), 47 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 026b6d1eb4..e062a75574 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -25,44 +25,20 @@ 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) {
+		printf("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;
-		}
-	}
-	init_watchdog_dev(gd->watchdog_dev);
+	uclass_foreach_dev(dev, uc)
+		device_probe(dev);
 
 	gd->flags |= GD_FLG_WDT_READY;
 	return 0;
@@ -145,22 +121,26 @@ 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);
+	uclass_foreach_dev(dev, uc) {
+		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
@@ -214,11 +194,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 v2 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.

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                   | 69 +++++++++++++++++++
 4 files changed, 98 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..091af1d36e
--- /dev/null
+++ b/drivers/watchdog/gpio_wdt.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <common.h>
+#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 v2 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.

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 5ca3bc502a..cee5b14ecb 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 8b934698b5..3e4ee6e8e1 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 8911463d58..faea4d763c 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 v2 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

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 cee5b14ecb..1853b5f29a 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..8997aaebc8 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. */
+	mdelay(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. */
+	mdelay(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. */
+	mdelay(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 v2 00/10] handling all DM watchdogs in watchdog_reset()

[Rasmus Villemoes]

On 28/05/2021 00.00, Rasmus Villemoes wrote:
> 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.
> 
> It's called v2 merely because the gpio driver and associated test has
> been sent before; all but those two patches (8 and 9) are new. As for
> the changes in those compared to the first submission, they are mostly
> cosmetic - functionally, the only change is that it now requires the
> always-running property in DT.

Ping.

Re: [PATCH v2 00/10] handling all DM watchdogs in watchdog_reset()

[Stefan Roese]

Hi Rasmus,

On 09.06.21 00:41, Rasmus Villemoes wrote:
> On 28/05/2021 00.00, Rasmus Villemoes wrote:
>> 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.
>>
>> It's called v2 merely because the gpio driver and associated test has
>> been sent before; all but those two patches (8 and 9) are new. As for
>> the changes in those compared to the first submission, they are mostly
>> cosmetic - functionally, the only change is that it now requires the
>> always-running property in DT.
> 
> Ping.

It's on my list to review. I've postponed this a bit, since these
changes will definitely not make it into the upcoming v2021.07 release.

Thanks,
Stefan

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

[Simon Glass]

On Thu, 27 May 2021 at 16:00, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> 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.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/watchdog/wdt-uclass.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Simon Glass]

On Thu, 27 May 2021 at 16:00, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> 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.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/watchdog/wdt-uclass.c | 65 ++++++++++++++++++++++++-----------
>  1 file changed, 45 insertions(+), 20 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

nit below


> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 634428fa24..3b6aa3d586 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -18,15 +18,15 @@ 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 {
> +       u32 timeout;
> +       ulong reset_period;
> +       ulong next_reset;

can you comment these?


> +};
>
>  int initr_watchdog(void)
>  {
> -       u32 timeout = WATCHDOG_TIMEOUT_SECS;
> +       struct wdt_priv *priv;
>         int ret;
>
>         /*
> @@ -42,28 +42,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 +130,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 +171,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
>

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

[Simon Glass]

On Thu, 27 May 2021 at 16:00, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> The addition of .pre_probe and .per_device_auto made this look
> bad. Fix it.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/watchdog/wdt-uclass.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Simon Glass]

On Thu, 27 May 2021 at 16:00, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> 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.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/watchdog/wdt-uclass.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Simon Glass]

On Thu, 27 May 2021 at 16:00, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> 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.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/watchdog/wdt-uclass.c | 37 ++++++++++++++++++++---------------
>  1 file changed, 21 insertions(+), 16 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 06/10] sandbox: disable CONFIG_WATCHDOG_AUTOSTART

[Simon Glass]

On Thu, 27 May 2021 at 16:00, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> 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.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  configs/sandbox64_defconfig | 1 +
>  configs/sandbox_defconfig   | 1 +
>  2 files changed, 2 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Simon Glass]

Hi Rasmus,

On Thu, 27 May 2021 at 16:00, 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.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/watchdog/wdt-uclass.c     | 87 ++++++++++++++++---------------
>  include/asm-generic/global_data.h |  6 ---
>  2 files changed, 46 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 026b6d1eb4..e062a75574 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -25,44 +25,20 @@ 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) {
> +               printf("Error getting UCLASS_WDT: %d\n", ret);

log_debug()as this should not happen

> +               return 0;

Should return error here

>         }
>
> -       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;
> -               }
> -       }
> -       init_watchdog_dev(gd->watchdog_dev);
> +       uclass_foreach_dev(dev, uc)
> +               device_probe(dev);

If  watchdog fails, should we not stop? Even if we don't, I think some
sort of message should be shown so people know to fix it.

>
>         gd->flags |= GD_FLG_WDT_READY;
>         return 0;
> @@ -145,22 +121,26 @@ 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);
> +       uclass_foreach_dev(dev, uc) {

Don't you need to use foreach_dev_probe() ?

> +               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
> @@ -214,11 +194,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);

log_debug()

> +               return 0;
> +       }
> +
> +       ret = wdt_start(dev, priv->timeout * 1000, 0);
> +       if (ret != 0) {
> +               printf("WDT:   Failed to start %s\n", dev->name);

log_debug()

> +               return 0;
> +       }

I don't think it is good to start it in the post-probe. Can you do it
separately, afterwards? Probing is supposed to just probe it and it
should be safe to do that without side effects.

> +
> +       printf("WDT:   Started %s with%s servicing (%ds timeout)\n", dev->name,
> +              IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout);

log_debug() I think

> +
> +       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
>

Regards,
Simon

Re: [PATCH v2 08/10] watchdog: add gpio watchdog driver

[Simon Glass]

On Thu, 27 May 2021 at 16:00, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> 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.
>
> 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                   | 69 +++++++++++++++++++
>  4 files changed, 98 insertions(+)
>  create mode 100644 doc/device-tree-bindings/watchdog/gpio-wdt.txt
>  create mode 100644 drivers/watchdog/gpio_wdt.c
>

Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Simon Glass]

On Thu, 27 May 2021 at 16:00, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> 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.
>
> 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(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

(you could get the device by alias also, I suppose)

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

[Simon Glass]

On Thu, 27 May 2021 at 16:00, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> 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
>
> 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(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

I'd prefer to advance time though, rather than using mdelay().

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

[Rasmus Villemoes]

Hi Simon,

Thanks for review.

>> -       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) {
>> +               printf("Error getting UCLASS_WDT: %d\n", ret);
> 
> log_debug()as this should not happen

OK. [I assume the rationale is: don't add .text which will most likely
never be used, but allow the debug statements to be easily turned on
per-TU if one should ever need it.]

>> +               return 0;
>
> Should return error here

And have the initr sequence stop completely instead of trying to limp
along? Why? Note that I touched upon this in the commit message: The
existing code doesn't pass on an error from uclass_get_device*() [which
would most likely come from an underlying probe call], and returns 0
regardless from initr_watchdog(). I see no point changing that.

>> -       }
>> -       init_watchdog_dev(gd->watchdog_dev);
>> +       uclass_foreach_dev(dev, uc)
>> +               device_probe(dev);
> 
> If  watchdog fails, should we not stop? Even if we don't, I think some
> sort of message should be shown so people know to fix it.

I'd assume device_probe() would print an error message. If not, sure, I
can add some [log_debug?] message.

>>
>>         gd->flags |= GD_FLG_WDT_READY;
>>         return 0;
>> @@ -145,22 +121,26 @@ 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);
>> +       uclass_foreach_dev(dev, uc) {
> 
> Don't you need to use foreach_dev_probe() ?

Why? They've all been probed in initr_watchdog(). And the guts of
WATHCDOG_RESET(), which can be and is called from everywhere, is
certainly not the place to do anything other than actually pinging the
watchdog devices.

>> +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);
> 
> log_debug()

Perhaps, but this is just existing code I've moved around.

>> +               return 0;
>> +       }
>> +
>> +       ret = wdt_start(dev, priv->timeout * 1000, 0);
>> +       if (ret != 0) {
>> +               printf("WDT:   Failed to start %s\n", dev->name);
> 
> log_debug()

Ditto.

>> +               return 0;
>> +       }
> 
> I don't think it is good to start it in the post-probe. Can you do it
> separately, afterwards? 

Eh, yes, of course I could do this in the loop in initr_watchdog() where
I probe all watchdog devices, but the end result is exactly the same,
and it seemed that this was a perfect fit for DM since it provided this
post_probe hook. It's a no-op if CONFIG_WATCHDOG_AUTOSTART isn't set,
and if it is, that precisely means the developer wanted to start
handling the device(s) ASAP.

> Probing is supposed to just probe it and it
> should be safe to do that without side effects.

I agree in general, but watchdog devices are a bit special compared to
some random USB controller or LCD display or spi master - those devices
generally don't do anything unless asked to by the CPU, while a watchdog
device is the other way around, it does its thing _unless_ the CPU asks
it not to (often enough). Which in turn, I suppose, is the whole reason
wdt-uclass has its own hook into the initr sequence - one needs to probe
and possibly start handling the watchdog(s) ASAP.

BTW, next on my agenda is hooking in even earlier so the few boards that
use INIT_FUNC_WATCHDOG_INIT/INIT_FUNC_WATCHDOG_RESET can just use DM and
be done with it - having those INIT_FUNC_WATCHDOG_RESET in completely
random places in init_sequence_f[] is a bit silly.

>> +
>> +       printf("WDT:   Started %s with%s servicing (%ds timeout)\n", dev->name,
>> +              IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout);
> 
> log_debug() I think

Ditto, existing code moved around.

Thanks,
Rasmus

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

[Rasmus Villemoes]

On 22/06/2021 15.31, Simon Glass wrote:
> On Thu, 27 May 2021 at 16:00, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> 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
>>
>> 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(+)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> I'd prefer to advance time though, rather than using mdelay().
> 

Cool, I didn't know we could do that in sandbox. Have a pointer?

Thanks,
Rasmus

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

[Stefan Roese]

On 22.06.21 22:28, Rasmus Villemoes wrote:
> Hi Simon,
> 
> Thanks for review.
> 
>>> -       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) {
>>> +               printf("Error getting UCLASS_WDT: %d\n", ret);
>>
>> log_debug()as this should not happen
> 
> OK. [I assume the rationale is: don't add .text which will most likely
> never be used, but allow the debug statements to be easily turned on
> per-TU if one should ever need it.]
> 
>>> +               return 0;
>>
>> Should return error here
> 
> And have the initr sequence stop completely instead of trying to limp
> along? Why? Note that I touched upon this in the commit message: The
> existing code doesn't pass on an error from uclass_get_device*() [which
> would most likely come from an underlying probe call], and returns 0
> regardless from initr_watchdog(). I see no point changing that.
> 
>>> -       }
>>> -       init_watchdog_dev(gd->watchdog_dev);
>>> +       uclass_foreach_dev(dev, uc)
>>> +               device_probe(dev);
>>
>> If  watchdog fails, should we not stop? Even if we don't, I think some
>> sort of message should be shown so people know to fix it.
> 
> I'd assume device_probe() would print an error message. If not, sure, I
> can add some [log_debug?] message.
> 
>>>
>>>          gd->flags |= GD_FLG_WDT_READY;
>>>          return 0;
>>> @@ -145,22 +121,26 @@ 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);
>>> +       uclass_foreach_dev(dev, uc) {
>>
>> Don't you need to use foreach_dev_probe() ?
> 
> Why? They've all been probed in initr_watchdog(). And the guts of
> WATHCDOG_RESET(), which can be and is called from everywhere, is
> certainly not the place to do anything other than actually pinging the
> watchdog devices.
> 
>>> +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);
>>
>> log_debug()
> 
> Perhaps, but this is just existing code I've moved around.

I would prefer to not change (remove) the currently existing output of
the status of the watchdog driver. If necessary, it could be changed to
log_info().

>>> +               return 0;
>>> +       }
>>> +
>>> +       ret = wdt_start(dev, priv->timeout * 1000, 0);
>>> +       if (ret != 0) {
>>> +               printf("WDT:   Failed to start %s\n", dev->name);
>>
>> log_debug()
> 
> Ditto.
> 
>>> +               return 0;
>>> +       }
>>
>> I don't think it is good to start it in the post-probe. Can you do it
>> separately, afterwards?
> 
> Eh, yes, of course I could do this in the loop in initr_watchdog() where
> I probe all watchdog devices, but the end result is exactly the same,
> and it seemed that this was a perfect fit for DM since it provided this
> post_probe hook. It's a no-op if CONFIG_WATCHDOG_AUTOSTART isn't set,
> and if it is, that precisely means the developer wanted to start
> handling the device(s) ASAP.
> 
>> Probing is supposed to just probe it and it
>> should be safe to do that without side effects.
> 
> I agree in general, but watchdog devices are a bit special compared to
> some random USB controller or LCD display or spi master - those devices
> generally don't do anything unless asked to by the CPU, while a watchdog
> device is the other way around, it does its thing _unless_ the CPU asks
> it not to (often enough). Which in turn, I suppose, is the whole reason
> wdt-uclass has its own hook into the initr sequence - one needs to probe
> and possibly start handling the watchdog(s) ASAP.
> 
> BTW, next on my agenda is hooking in even earlier so the few boards that
> use INIT_FUNC_WATCHDOG_INIT/INIT_FUNC_WATCHDOG_RESET can just use DM and
> be done with it - having those INIT_FUNC_WATCHDOG_RESET in completely
> random places in init_sequence_f[] is a bit silly.

Nice.

>>> +
>>> +       printf("WDT:   Started %s with%s servicing (%ds timeout)\n", dev->name,
>>> +              IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout);
>>
>> log_debug() I think
> 
> Ditto, existing code moved around.

Again, please don't change this to debug level.

Thanks,
Stefan

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

[Simon Glass]

Hi Rasmus,

On Tue, 22 Jun 2021 at 14:28, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Hi Simon,
>
> Thanks for review.
>
> >> -       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) {
> >> +               printf("Error getting UCLASS_WDT: %d\n", ret);
> >
> > log_debug()as this should not happen
>
> OK. [I assume the rationale is: don't add .text which will most likely
> never be used, but allow the debug statements to be easily turned on
> per-TU if one should ever need it.]

Yes, also the error return value should give you a clue.

Please, use log_msg_ret() on your return values.

>
> >> +               return 0;
> >
> > Should return error here
>
> And have the initr sequence stop completely instead of trying to limp
> along? Why? Note that I touched upon this in the commit message: The
> existing code doesn't pass on an error from uclass_get_device*() [which
> would most likely come from an underlying probe call], and returns 0
> regardless from initr_watchdog(). I see no point changing that.

OK, I'll leave it to you. My feeling is that failure is a good way to
get a bug fixed.

>
> >> -       }
> >> -       init_watchdog_dev(gd->watchdog_dev);
> >> +       uclass_foreach_dev(dev, uc)
> >> +               device_probe(dev);
> >
> > If  watchdog fails, should we not stop? Even if we don't, I think some
> > sort of message should be shown so people know to fix it.
>
> I'd assume device_probe() would print an error message. If not, sure, I
> can add some [log_debug?] message.

No driver model never prints messages. Yes you can use log_debuig().

>
> >>
> >>         gd->flags |= GD_FLG_WDT_READY;
> >>         return 0;
> >> @@ -145,22 +121,26 @@ 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);
> >> +       uclass_foreach_dev(dev, uc) {
> >
> > Don't you need to use foreach_dev_probe() ?
>
> Why? They've all been probed in initr_watchdog(). And the guts of
> WATHCDOG_RESET(), which can be and is called from everywhere, is
> certainly not the place to do anything other than actually pinging the
> watchdog devices.

Then you should add a comment. You are using a low-level iterator,
then assuming the devices are probed.

>
> >> +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);
> >
> > log_debug()
>
> Perhaps, but this is just existing code I've moved around.

OK, well then you can leave it alone.

>
> >> +               return 0;
> >> +       }
> >> +
> >> +       ret = wdt_start(dev, priv->timeout * 1000, 0);
> >> +       if (ret != 0) {
> >> +               printf("WDT:   Failed to start %s\n", dev->name);
> >
> > log_debug()
>
> Ditto.
>
> >> +               return 0;
> >> +       }
> >
> > I don't think it is good to start it in the post-probe. Can you do it
> > separately, afterwards?
>
> Eh, yes, of course I could do this in the loop in initr_watchdog() where
> I probe all watchdog devices, but the end result is exactly the same,
> and it seemed that this was a perfect fit for DM since it provided this
> post_probe hook. It's a no-op if CONFIG_WATCHDOG_AUTOSTART isn't set,
> and if it is, that precisely means the developer wanted to start
> handling the device(s) ASAP.
>
> > Probing is supposed to just probe it and it
> > should be safe to do that without side effects.
>
> I agree in general, but watchdog devices are a bit special compared to
> some random USB controller or LCD display or spi master - those devices
> generally don't do anything unless asked to by the CPU, while a watchdog
> device is the other way around, it does its thing _unless_ the CPU asks
> it not to (often enough). Which in turn, I suppose, is the whole reason
> wdt-uclass has its own hook into the initr sequence - one needs to probe
> and possibly start handling the watchdog(s) ASAP.

It still needs a 'start' method to make it start. Having it start on
probe means the board will reset at the command line if the device is
probed. Yuck.

>
> BTW, next on my agenda is hooking in even earlier so the few boards that
> use INIT_FUNC_WATCHDOG_INIT/INIT_FUNC_WATCHDOG_RESET can just use DM and
> be done with it - having those INIT_FUNC_WATCHDOG_RESET in completely
> random places in init_sequence_f[] is a bit silly.

OK.

>
> >> +
> >> +       printf("WDT:   Started %s with%s servicing (%ds timeout)\n", dev->name,
> >> +              IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout);
> >
> > log_debug() I think
>
> Ditto, existing code moved around.

OK.

Regards,
Simon

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

[Simon Glass]

Hi Rasmus,

On Tue, 22 Jun 2021 at 14:29, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 22/06/2021 15.31, Simon Glass wrote:
> > On Thu, 27 May 2021 at 16:00, Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> >>
> >> 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
> >>
> >> 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(+)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > I'd prefer to advance time though, rather than using mdelay().
> >
>
> Cool, I didn't know we could do that in sandbox. Have a pointer?
>

You can do anything in sandbox. It is just software :-)

See timer_test_add_offset()

Regards,
Simon

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

[Rasmus Villemoes]

On 26/06/2021 20.32, Simon Glass wrote:
> Hi Rasmus,
> 
> On Tue, 22 Jun 2021 at 14:28, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>

>>> I don't think it is good to start it in the post-probe. Can you do it
>>> separately, afterwards?
>>
>> Eh, yes, of course I could do this in the loop in initr_watchdog() where
>> I probe all watchdog devices, but the end result is exactly the same,
>> and it seemed that this was a perfect fit for DM since it provided this
>> post_probe hook. It's a no-op if CONFIG_WATCHDOG_AUTOSTART isn't set,
>> and if it is, that precisely means the developer wanted to start
>> handling the device(s) ASAP.
>>
>>> Probing is supposed to just probe it and it
>>> should be safe to do that without side effects.
>>
>> I agree in general, but watchdog devices are a bit special compared to
>> some random USB controller or LCD display or spi master - those devices
>> generally don't do anything unless asked to by the CPU, while a watchdog
>> device is the other way around, it does its thing _unless_ the CPU asks
>> it not to (often enough). Which in turn, I suppose, is the whole reason
>> wdt-uclass has its own hook into the initr sequence - one needs to probe
>> and possibly start handling the watchdog(s) ASAP.
> 
> It still needs a 'start' method to make it start. Having it start on
> probe means the board will reset at the command line if the device is
> probed. Yuck.

No, because while sitting in the command line waiting for user input,
WATCHDOG_RESET() is called something like a million times per second (or
at least extremely often). For the most common case of there only being
one (or zero) DM watchdogs, I'm not changing anything at all about how
things behave. I'm just expanding the handling done in the wdt-uclass
provided functions initr_watchdog() and watchdog_reset() to all DM
watchdogs, making things more consistent. And there's
CONFIG_WATCHDOG_AUTOSTART=n which as before would make the post_probe
function into a no-op.

As I said, yes, I can move the call of the post_probe function into the
loop in initr_watchdog (and then it wouldn't be called post_probe, but
probably named something including auto_start). In practice, that won't
change anything.

Stefan, what do you think? I think this is the only contentious point at
this time, so I'll do whatever you think is right, then resend the
patches with Simon's other feedback incorporated.

Rasmus

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

[Stefan Roese]

On 28.05.21 00:00, Rasmus Villemoes wrote:
> 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.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

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

Thanks,
Stefan

> ---
>   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;
>   
> 


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 v2 02/10] watchdog: wdt-uclass.c: introduce struct wdt_priv

[Stefan Roese]

On 28.05.21 00:00, 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.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   drivers/watchdog/wdt-uclass.c | 65 ++++++++++++++++++++++++-----------
>   1 file changed, 45 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 634428fa24..3b6aa3d586 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -18,15 +18,15 @@ 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 {
> +	u32 timeout;
> +	ulong reset_period;
> +	ulong next_reset;
> +};

It's nice to see this static variable finally go away.

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

Thanks,
Stefan


>   
>   int initr_watchdog(void)
>   {
> -	u32 timeout = WATCHDOG_TIMEOUT_SECS;
> +	struct wdt_priv *priv;
>   	int ret;
>   
>   	/*
> @@ -42,28 +42,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 +130,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 +171,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 v2 03/10] watchdog: wdt-uclass.c: neaten UCLASS_DRIVER definition

[Stefan Roese]

On 28.05.21 00:00, Rasmus Villemoes wrote:
> The addition of .pre_probe and .per_device_auto made this look
> bad. Fix it.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

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

Thanks,
Stefan

> ---
>   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 3b6aa3d586..360f052227 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -199,10 +199,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),
>   };
> 


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 v2 04/10] watchdog: wdt-uclass.c: refactor initr_watchdog()

[Stefan Roese]

On 28.05.21 00:00, Rasmus Villemoes wrote:
> 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.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

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

Thanks,
Stefan

> ---
>   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 360f052227..5d5dbcd0f7 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -24,11 +24,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
> @@ -42,21 +61,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;
>   }
> 


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 v2 05/10] watchdog: wdt-uclass.c: keep track of each device's running state

[Stefan Roese]

On 28.05.21 00:00, Rasmus Villemoes wrote:
> 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.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

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

Thanks,
Stefan

> ---
>   drivers/watchdog/wdt-uclass.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 5d5dbcd0f7..026b6d1eb4 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -22,6 +22,7 @@ struct wdt_priv {
>   	u32 timeout;
>   	ulong reset_period;
>   	ulong next_reset;
> +	bool running;
>   };
>   
>   static void init_watchdog_dev(struct udevice *dev)
> @@ -63,6 +64,7 @@ int initr_watchdog(void)
>   	}
>   	init_watchdog_dev(gd->watchdog_dev);
>   
> +	gd->flags |= GD_FLG_WDT_READY;
>   	return 0;
>   }
>   
> @@ -75,8 +77,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;
>   }
> @@ -90,8 +95,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;
>   }
> @@ -145,6 +153,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)) {
> 


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 v2 06/10] sandbox: disable CONFIG_WATCHDOG_AUTOSTART

[Stefan Roese]

On 28.05.21 00:00, Rasmus Villemoes wrote:
> 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.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

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

Thanks,
Stefan

> ---
>   configs/sandbox64_defconfig | 1 +
>   configs/sandbox_defconfig   | 1 +
>   2 files changed, 2 insertions(+)
> 
> diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
> index 9a373bab6f..8b934698b5 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 bdbf714e2b..8911463d58 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
> 


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 v2 08/10] watchdog: add gpio watchdog driver

[Stefan Roese]

On 28.05.21 00:00, Rasmus Villemoes wrote:
> 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.
> 
> 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                   | 69 +++++++++++++++++++
>   4 files changed, 98 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..091af1d36e
> --- /dev/null
> +++ b/drivers/watchdog/gpio_wdt.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <common.h>

AFAIK, including "common.h" is deprecated. Other than this:

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

Thanks,
Stefan

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


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 v2 09/10] sandbox: add test of wdt_gpio driver

[Stefan Roese]

On 28.05.21 00:00, Rasmus Villemoes wrote:
> 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.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

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

Thanks,
Stefan

> ---
>   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 5ca3bc502a..cee5b14ecb 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 8b934698b5..3e4ee6e8e1 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 8911463d58..faea4d763c 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);
> 


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 v2 10/10] sandbox: add test of wdt-uclass' watchdog_reset()

[Stefan Roese]

On 28.05.21 00:00, Rasmus Villemoes wrote:
> 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
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

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

Thanks,
Stefan

> ---
>   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 cee5b14ecb..1853b5f29a 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..8997aaebc8 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. */
> +	mdelay(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. */
> +	mdelay(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. */
> +	mdelay(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);
> 


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 v2 07/10] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

[Stefan Roese]

On 28.06.21 09:44, Rasmus Villemoes wrote:
> On 26/06/2021 20.32, Simon Glass wrote:
>> Hi Rasmus,
>>
>> On Tue, 22 Jun 2021 at 14:28, Rasmus Villemoes
>> <rasmus.villemoes@prevas.dk> wrote:
>>>
> 
>>>> I don't think it is good to start it in the post-probe. Can you do it
>>>> separately, afterwards?
>>>
>>> Eh, yes, of course I could do this in the loop in initr_watchdog() where
>>> I probe all watchdog devices, but the end result is exactly the same,
>>> and it seemed that this was a perfect fit for DM since it provided this
>>> post_probe hook. It's a no-op if CONFIG_WATCHDOG_AUTOSTART isn't set,
>>> and if it is, that precisely means the developer wanted to start
>>> handling the device(s) ASAP.
>>>
>>>> Probing is supposed to just probe it and it
>>>> should be safe to do that without side effects.
>>>
>>> I agree in general, but watchdog devices are a bit special compared to
>>> some random USB controller or LCD display or spi master - those devices
>>> generally don't do anything unless asked to by the CPU, while a watchdog
>>> device is the other way around, it does its thing _unless_ the CPU asks
>>> it not to (often enough). Which in turn, I suppose, is the whole reason
>>> wdt-uclass has its own hook into the initr sequence - one needs to probe
>>> and possibly start handling the watchdog(s) ASAP.
>>
>> It still needs a 'start' method to make it start. Having it start on
>> probe means the board will reset at the command line if the device is
>> probed. Yuck.
> 
> No, because while sitting in the command line waiting for user input,
> WATCHDOG_RESET() is called something like a million times per second (or
> at least extremely often).

We have some rate-limiting in place since a few years (reset_period).

> For the most common case of there only being
> one (or zero) DM watchdogs, I'm not changing anything at all about how
> things behave. I'm just expanding the handling done in the wdt-uclass
> provided functions initr_watchdog() and watchdog_reset() to all DM
> watchdogs, making things more consistent. And there's
> CONFIG_WATCHDOG_AUTOSTART=n which as before would make the post_probe
> function into a no-op.
> 
> As I said, yes, I can move the call of the post_probe function into the
> loop in initr_watchdog (and then it wouldn't be called post_probe, but
> probably named something including auto_start). In practice, that won't
> change anything.
> 
> Stefan, what do you think? I think this is the only contentious point at
> this time, so I'll do whatever you think is right, then resend the
> patches with Simon's other feedback incorporated.

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.

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

Thanks,
Stefan

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

[Simon Glass]

Hi Rasmus,

On Mon, 28 Jun 2021 at 01:44, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 26/06/2021 20.32, Simon Glass wrote:
> > Hi Rasmus,
> >
> > On Tue, 22 Jun 2021 at 14:28, Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> >>
>
> >>> I don't think it is good to start it in the post-probe. Can you do it
> >>> separately, afterwards?
> >>
> >> Eh, yes, of course I could do this in the loop in initr_watchdog() where
> >> I probe all watchdog devices, but the end result is exactly the same,
> >> and it seemed that this was a perfect fit for DM since it provided this
> >> post_probe hook. It's a no-op if CONFIG_WATCHDOG_AUTOSTART isn't set,
> >> and if it is, that precisely means the developer wanted to start
> >> handling the device(s) ASAP.
> >>
> >>> Probing is supposed to just probe it and it
> >>> should be safe to do that without side effects.
> >>
> >> I agree in general, but watchdog devices are a bit special compared to
> >> some random USB controller or LCD display or spi master - those devices
> >> generally don't do anything unless asked to by the CPU, while a watchdog
> >> device is the other way around, it does its thing _unless_ the CPU asks
> >> it not to (often enough). Which in turn, I suppose, is the whole reason
> >> wdt-uclass has its own hook into the initr sequence - one needs to probe
> >> and possibly start handling the watchdog(s) ASAP.
> >
> > It still needs a 'start' method to make it start. Having it start on
> > probe means the board will reset at the command line if the device is
> > probed. Yuck.
>
> No, because while sitting in the command line waiting for user input,
> WATCHDOG_RESET() is called something like a million times per second (or
> at least extremely often). For the most common case of there only being
> one (or zero) DM watchdogs, I'm not changing anything at all about how
> things behave. I'm just expanding the handling done in the wdt-uclass
> provided functions initr_watchdog() and watchdog_reset() to all DM
> watchdogs, making things more consistent. And there's
> CONFIG_WATCHDOG_AUTOSTART=n which as before would make the post_probe
> function into a no-op.
>
> As I said, yes, I can move the call of the post_probe function into the
> loop in initr_watchdog (and then it wouldn't be called post_probe, but
> probably named something including auto_start). In practice, that won't
> change anything.
>
> Stefan, what do you think? I think this is the only contentious point at
> this time, so I'll do whatever you think is right, then resend the
> patches with Simon's other feedback incorporated.

From a driver model perspective, it is good practice to have a 'start'
method separate from probing. I understand that you are saying that
the watchdog should always be enabled, but we can handle that with a
separate method to start it. I don't think it needs to be started in
the post-probe method. We can just start it later. 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.

I also understand this is not quite how things work at present and I'm
fine with copying the existing behaviour. It's just that you are
introducing a driver model interface and I have pretty strong views
about the separation between probe at start, with that.

Regards,
Simon