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

[PATCH v4 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. Some of the ad hoc work done for the first DM watchdog in
initr_watchdog() is now moved to a .pre_probe hook so it is
automatically done for all devices.

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

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

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

Changes in v4:

- Do not make init_watchdog_dev() into a .post_probe hook, but call it
  from within the probing loop in initr_watchdog().

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

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

-- 
2.31.1

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

[Rasmus Villemoes]

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

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

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

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

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

[Rasmus Villemoes]

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

No functional change intended.

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

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index df8164da2a..b29d214724 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -20,15 +20,24 @@ DECLARE_GLOBAL_DATA_PTR;
 
 #define WATCHDOG_TIMEOUT_SECS	(CONFIG_WATCHDOG_TIMEOUT_MSECS / 1000)
 
-/*
- * Reset every 1000ms, or however often is required as indicated by a
- * hw_margin_ms property.
- */
-static ulong reset_period = 1000;
+struct wdt_priv {
+	/* Timeout, in seconds, to configure this device to. */
+	u32 timeout;
+	/*
+	 * Time, in milliseconds, between calling the device's ->reset()
+	 * method from watchdog_reset().
+	 */
+	ulong reset_period;
+	/*
+	 * Next time (as returned by get_timer(0)) to call
+	 * ->reset().
+	 */
+	ulong next_reset;
+};
 
 int initr_watchdog(void)
 {
-	u32 timeout = WATCHDOG_TIMEOUT_SECS;
+	struct wdt_priv *priv;
 	int ret;
 
 	/*
@@ -44,28 +53,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 (!IS_ENABLED(CONFIG_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;
 }
@@ -139,18 +141,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
@@ -177,9 +182,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 v4 02/10] watchdog: wdt-uclass.c: introduce struct wdt_priv

[Stefan Roese]

On 02.08.21 17: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.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

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

Thanks,
Stefan

> ---
>   drivers/watchdog/wdt-uclass.c | 74 +++++++++++++++++++++++++----------
>   1 file changed, 54 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index df8164da2a..b29d214724 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -20,15 +20,24 @@ DECLARE_GLOBAL_DATA_PTR;
>   
>   #define WATCHDOG_TIMEOUT_SECS	(CONFIG_WATCHDOG_TIMEOUT_MSECS / 1000)
>   
> -/*
> - * Reset every 1000ms, or however often is required as indicated by a
> - * hw_margin_ms property.
> - */
> -static ulong reset_period = 1000;
> +struct wdt_priv {
> +	/* Timeout, in seconds, to configure this device to. */
> +	u32 timeout;
> +	/*
> +	 * Time, in milliseconds, between calling the device's ->reset()
> +	 * method from watchdog_reset().
> +	 */
> +	ulong reset_period;
> +	/*
> +	 * Next time (as returned by get_timer(0)) to call
> +	 * ->reset().
> +	 */
> +	ulong next_reset;
> +};
>   
>   int initr_watchdog(void)
>   {
> -	u32 timeout = WATCHDOG_TIMEOUT_SECS;
> +	struct wdt_priv *priv;
>   	int ret;
>   
>   	/*
> @@ -44,28 +53,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 (!IS_ENABLED(CONFIG_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;
>   }
> @@ -139,18 +141,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
> @@ -177,9 +182,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

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

[Rasmus Villemoes]

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

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

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index b29d214724..81287c759a 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -210,10 +210,10 @@ static int wdt_pre_probe(struct udevice *dev)
 }
 
 UCLASS_DRIVER(wdt) = {
-	.id		= UCLASS_WDT,
-	.name		= "watchdog",
-	.flags		= DM_UC_FLAG_SEQ_ALIAS,
-	.post_bind	= wdt_post_bind,
+	.id			= UCLASS_WDT,
+	.name			= "watchdog",
+	.flags			= DM_UC_FLAG_SEQ_ALIAS,
+	.post_bind		= wdt_post_bind,
 	.pre_probe		= wdt_pre_probe,
 	.per_device_auto	= sizeof(struct wdt_priv),
 };
-- 
2.31.1

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

[Rasmus Villemoes]

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

Include the device name in various printfs.

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

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 81287c759a..0a1f43771c 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -35,11 +35,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 (!IS_ENABLED(CONFIG_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
@@ -53,21 +72,7 @@ int initr_watchdog(void)
 			return 0;
 		}
 	}
-	priv = dev_get_uclass_priv(gd->watchdog_dev);
-
-	if (!IS_ENABLED(CONFIG_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 v4 05/10] watchdog: wdt-uclass.c: keep track of each device's running state

[Rasmus Villemoes]

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

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

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

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 0a1f43771c..358fc68e27 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -33,6 +33,8 @@ struct wdt_priv {
 	 * ->reset().
 	 */
 	ulong next_reset;
+	/* Whether watchdog_start() has been called on the device. */
+	bool running;
 };
 
 static void init_watchdog_dev(struct udevice *dev)
@@ -74,6 +76,7 @@ int initr_watchdog(void)
 	}
 	init_watchdog_dev(gd->watchdog_dev);
 
+	gd->flags |= GD_FLG_WDT_READY;
 	return 0;
 }
 
@@ -86,8 +89,11 @@ int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
 		return -ENOSYS;
 
 	ret = ops->start(dev, timeout_ms, flags);
-	if (ret == 0)
-		gd->flags |= GD_FLG_WDT_READY;
+	if (ret == 0) {
+		struct wdt_priv *priv = dev_get_uclass_priv(dev);
+
+		priv->running = true;
+	}
 
 	return ret;
 }
@@ -101,8 +107,11 @@ int wdt_stop(struct udevice *dev)
 		return -ENOSYS;
 
 	ret = ops->stop(dev);
-	if (ret == 0)
-		gd->flags &= ~GD_FLG_WDT_READY;
+	if (ret == 0) {
+		struct wdt_priv *priv = dev_get_uclass_priv(dev);
+
+		priv->running = false;
+	}
 
 	return ret;
 }
@@ -156,6 +165,9 @@ void watchdog_reset(void)
 
 	dev = gd->watchdog_dev;
 	priv = dev_get_uclass_priv(dev);
+	if (!priv->running)
+		return;
+
 	/* Do not reset the watchdog too often */
 	now = get_timer(0);
 	if (time_after_eq(now, priv->next_reset)) {
-- 
2.31.1

[PATCH v4 06/10] sandbox: disable CONFIG_WATCHDOG_AUTOSTART

[Rasmus Villemoes]

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

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

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

[PATCH v4 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, and call the init_watchdog_dev helper for each.

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

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

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

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

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

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 358fc68e27..0ce8b3a425 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -61,20 +61,24 @@ static void init_watchdog_dev(struct udevice *dev)
 
 int initr_watchdog(void)
 {
-	/*
-	 * Init watchdog: This will call the probe function of the
-	 * watchdog driver, enabling the use of the device
-	 */
-	if (uclass_get_device_by_seq(UCLASS_WDT, 0,
-				     (struct udevice **)&gd->watchdog_dev)) {
-		debug("WDT:   Not found by seq!\n");
-		if (uclass_get_device(UCLASS_WDT, 0,
-				      (struct udevice **)&gd->watchdog_dev)) {
-			printf("WDT:   Not found!\n");
-			return 0;
+	struct udevice *dev;
+	struct uclass *uc;
+	int ret;
+
+	ret = uclass_get(UCLASS_WDT, &uc);
+	if (ret) {
+		log_debug("Error getting UCLASS_WDT: %d\n", ret);
+		return 0;
+	}
+
+	uclass_foreach_dev(dev, uc) {
+		ret = device_probe(dev);
+		if (ret) {
+			log_debug("Error probing %s: %d\n", dev->name, ret);
+			continue;
 		}
+		init_watchdog_dev(dev);
 	}
-	init_watchdog_dev(gd->watchdog_dev);
 
 	gd->flags |= GD_FLG_WDT_READY;
 	return 0;
@@ -157,22 +161,34 @@ void watchdog_reset(void)
 {
 	struct wdt_priv *priv;
 	struct udevice *dev;
+	struct uclass *uc;
 	ulong now;
 
 	/* Exit if GD is not ready or watchdog is not initialized yet */
 	if (!gd || !(gd->flags & GD_FLG_WDT_READY))
 		return;
 
-	dev = gd->watchdog_dev;
-	priv = dev_get_uclass_priv(dev);
-	if (!priv->running)
+	if (uclass_get(UCLASS_WDT, &uc))
 		return;
 
-	/* Do not reset the watchdog too often */
-	now = get_timer(0);
-	if (time_after_eq(now, priv->next_reset)) {
-		priv->next_reset = now + priv->reset_period;
-		wdt_reset(dev);
+	/*
+	 * All devices bound to the wdt uclass should have been probed
+	 * in initr_watchdog(). But just in case something went wrong,
+	 * check device_active() before accessing the uclass private
+	 * data.
+	 */
+	uclass_foreach_dev(dev, uc) {
+		if (!device_active(dev))
+			continue;
+		priv = dev_get_uclass_priv(dev);
+		if (!priv->running)
+			continue;
+		/* Do not reset the watchdog too often */
+		now = get_timer(0);
+		if (time_after_eq(now, priv->next_reset)) {
+			priv->next_reset = now + priv->reset_period;
+			wdt_reset(dev);
+		}
 	}
 }
 #endif
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index e55070303f..28d749538c 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -447,12 +447,6 @@ struct global_data {
 	 */
 	fdt_addr_t translation_offset;
 #endif
-#if CONFIG_IS_ENABLED(WDT)
-	/**
-	 * @watchdog_dev: watchdog device
-	 */
-	struct udevice *watchdog_dev;
-#endif
 #ifdef CONFIG_GENERATE_ACPI_TABLE
 	/**
 	 * @acpi_ctx: ACPI context pointer
-- 
2.31.1

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

[Simon Glass]

Hi Rasmus,

On Mon, 2 Aug 2021 at 09: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, and call the init_watchdog_dev helper for each.
>
> Similarly let watchdog_reset() loop over the whole uclass - each
> having their own ratelimiting metadata, and a separate "is this device
> running" flag.
>
> This gets rid of the watchdog_dev member of struct global_data.  We
> do, however, still need the GD_FLG_WDT_READY set in
> initr_watchdog(). This is because watchdog_reset() can get called
> before DM is ready, and I don't think we can call uclass_get() that
> early.
>
> The current code just returns 0 if "getting" the first device fails -
> that can of course happen because there are no devices, but it could
> also happen if its ->probe call failed. In keeping with that, continue
> with the handling of the remaining devices even if one fails to
> probe. This is also why we cannot use uclass_probe_all().
>
> If desired, it's possible to later add a per-device "u-boot,autostart"
> boolean property, so that one can do CONFIG_WATCHDOG_AUTOSTART
> per-device.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/watchdog/wdt-uclass.c     | 56 ++++++++++++++++++++-----------
>  include/asm-generic/global_data.h |  6 ----
>  2 files changed, 36 insertions(+), 26 deletions(-)

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

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

[Stefan Roese]

On 02.08.21 17:00, Rasmus Villemoes 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, and call the init_watchdog_dev helper for each.
> 
> Similarly let watchdog_reset() loop over the whole uclass - each
> having their own ratelimiting metadata, and a separate "is this device
> running" flag.
> 
> This gets rid of the watchdog_dev member of struct global_data.  We
> do, however, still need the GD_FLG_WDT_READY set in
> initr_watchdog(). This is because watchdog_reset() can get called
> before DM is ready, and I don't think we can call uclass_get() that
> early.
> 
> The current code just returns 0 if "getting" the first device fails -
> that can of course happen because there are no devices, but it could
> also happen if its ->probe call failed. In keeping with that, continue
> with the handling of the remaining devices even if one fails to
> probe. This is also why we cannot use uclass_probe_all().
> 
> If desired, it's possible to later add a per-device "u-boot,autostart"
> boolean property, so that one can do CONFIG_WATCHDOG_AUTOSTART
> per-device.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

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

Thanks,
Stefan

> ---
>   drivers/watchdog/wdt-uclass.c     | 56 ++++++++++++++++++++-----------
>   include/asm-generic/global_data.h |  6 ----
>   2 files changed, 36 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 358fc68e27..0ce8b3a425 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -61,20 +61,24 @@ static void init_watchdog_dev(struct udevice *dev)
>   
>   int initr_watchdog(void)
>   {
> -	/*
> -	 * Init watchdog: This will call the probe function of the
> -	 * watchdog driver, enabling the use of the device
> -	 */
> -	if (uclass_get_device_by_seq(UCLASS_WDT, 0,
> -				     (struct udevice **)&gd->watchdog_dev)) {
> -		debug("WDT:   Not found by seq!\n");
> -		if (uclass_get_device(UCLASS_WDT, 0,
> -				      (struct udevice **)&gd->watchdog_dev)) {
> -			printf("WDT:   Not found!\n");
> -			return 0;
> +	struct udevice *dev;
> +	struct uclass *uc;
> +	int ret;
> +
> +	ret = uclass_get(UCLASS_WDT, &uc);
> +	if (ret) {
> +		log_debug("Error getting UCLASS_WDT: %d\n", ret);
> +		return 0;
> +	}
> +
> +	uclass_foreach_dev(dev, uc) {
> +		ret = device_probe(dev);
> +		if (ret) {
> +			log_debug("Error probing %s: %d\n", dev->name, ret);
> +			continue;
>   		}
> +		init_watchdog_dev(dev);
>   	}
> -	init_watchdog_dev(gd->watchdog_dev);
>   
>   	gd->flags |= GD_FLG_WDT_READY;
>   	return 0;
> @@ -157,22 +161,34 @@ void watchdog_reset(void)
>   {
>   	struct wdt_priv *priv;
>   	struct udevice *dev;
> +	struct uclass *uc;
>   	ulong now;
>   
>   	/* Exit if GD is not ready or watchdog is not initialized yet */
>   	if (!gd || !(gd->flags & GD_FLG_WDT_READY))
>   		return;
>   
> -	dev = gd->watchdog_dev;
> -	priv = dev_get_uclass_priv(dev);
> -	if (!priv->running)
> +	if (uclass_get(UCLASS_WDT, &uc))
>   		return;
>   
> -	/* Do not reset the watchdog too often */
> -	now = get_timer(0);
> -	if (time_after_eq(now, priv->next_reset)) {
> -		priv->next_reset = now + priv->reset_period;
> -		wdt_reset(dev);
> +	/*
> +	 * All devices bound to the wdt uclass should have been probed
> +	 * in initr_watchdog(). But just in case something went wrong,
> +	 * check device_active() before accessing the uclass private
> +	 * data.
> +	 */
> +	uclass_foreach_dev(dev, uc) {
> +		if (!device_active(dev))
> +			continue;
> +		priv = dev_get_uclass_priv(dev);
> +		if (!priv->running)
> +			continue;
> +		/* Do not reset the watchdog too often */
> +		now = get_timer(0);
> +		if (time_after_eq(now, priv->next_reset)) {
> +			priv->next_reset = now + priv->reset_period;
> +			wdt_reset(dev);
> +		}
>   	}
>   }
>   #endif
> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> index e55070303f..28d749538c 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -447,12 +447,6 @@ struct global_data {
>   	 */
>   	fdt_addr_t translation_offset;
>   #endif
> -#if CONFIG_IS_ENABLED(WDT)
> -	/**
> -	 * @watchdog_dev: watchdog device
> -	 */
> -	struct udevice *watchdog_dev;
> -#endif
>   #ifdef CONFIG_GENERATE_ACPI_TABLE
>   	/**
>   	 * @acpi_ctx: ACPI context pointer
> 


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

[Stefan Roese]

Hi Rasmus,

On 02.08.21 17:00, Rasmus Villemoes 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, and call the init_watchdog_dev helper for each.
> 
> Similarly let watchdog_reset() loop over the whole uclass - each
> having their own ratelimiting metadata, and a separate "is this device
> running" flag.
> 
> This gets rid of the watchdog_dev member of struct global_data.  We
> do, however, still need the GD_FLG_WDT_READY set in
> initr_watchdog(). This is because watchdog_reset() can get called
> before DM is ready, and I don't think we can call uclass_get() that
> early.
> 
> The current code just returns 0 if "getting" the first device fails -
> that can of course happen because there are no devices, but it could
> also happen if its ->probe call failed. In keeping with that, continue
> with the handling of the remaining devices even if one fails to
> probe. This is also why we cannot use uclass_probe_all().
> 
> If desired, it's possible to later add a per-device "u-boot,autostart"
> boolean property, so that one can do CONFIG_WATCHDOG_AUTOSTART
> per-device.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   drivers/watchdog/wdt-uclass.c     | 56 ++++++++++++++++++++-----------
>   include/asm-generic/global_data.h |  6 ----
>   2 files changed, 36 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 358fc68e27..0ce8b3a425 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -61,20 +61,24 @@ static void init_watchdog_dev(struct udevice *dev)
>   
>   int initr_watchdog(void)
>   {
> -	/*
> -	 * Init watchdog: This will call the probe function of the
> -	 * watchdog driver, enabling the use of the device
> -	 */
> -	if (uclass_get_device_by_seq(UCLASS_WDT, 0,
> -				     (struct udevice **)&gd->watchdog_dev)) {
> -		debug("WDT:   Not found by seq!\n");
> -		if (uclass_get_device(UCLASS_WDT, 0,
> -				      (struct udevice **)&gd->watchdog_dev)) {
> -			printf("WDT:   Not found!\n");
> -			return 0;
> +	struct udevice *dev;
> +	struct uclass *uc;
> +	int ret;
> +
> +	ret = uclass_get(UCLASS_WDT, &uc);
> +	if (ret) {
> +		log_debug("Error getting UCLASS_WDT: %d\n", ret);
> +		return 0;
> +	}
> +
> +	uclass_foreach_dev(dev, uc) {
> +		ret = device_probe(dev);
> +		if (ret) {
> +			log_debug("Error probing %s: %d\n", dev->name, ret);
> +			continue;
>   		}
> +		init_watchdog_dev(dev);
>   	}
> -	init_watchdog_dev(gd->watchdog_dev);
>   
>   	gd->flags |= GD_FLG_WDT_READY;
>   	return 0;
> @@ -157,22 +161,34 @@ void watchdog_reset(void)
>   {
>   	struct wdt_priv *priv;
>   	struct udevice *dev;
> +	struct uclass *uc;
>   	ulong now;
>   
>   	/* Exit if GD is not ready or watchdog is not initialized yet */
>   	if (!gd || !(gd->flags & GD_FLG_WDT_READY))
>   		return;
>   
> -	dev = gd->watchdog_dev;
> -	priv = dev_get_uclass_priv(dev);
> -	if (!priv->running)
> +	if (uclass_get(UCLASS_WDT, &uc))
>   		return;
>   
> -	/* Do not reset the watchdog too often */
> -	now = get_timer(0);
> -	if (time_after_eq(now, priv->next_reset)) {
> -		priv->next_reset = now + priv->reset_period;
> -		wdt_reset(dev);
> +	/*
> +	 * All devices bound to the wdt uclass should have been probed
> +	 * in initr_watchdog(). But just in case something went wrong,
> +	 * check device_active() before accessing the uclass private
> +	 * data.
> +	 */
> +	uclass_foreach_dev(dev, uc) {
> +		if (!device_active(dev))
> +			continue;
> +		priv = dev_get_uclass_priv(dev);
> +		if (!priv->running)
> +			continue;
> +		/* Do not reset the watchdog too often */
> +		now = get_timer(0);
> +		if (time_after_eq(now, priv->next_reset)) {
> +			priv->next_reset = now + priv->reset_period;
> +			wdt_reset(dev);
> +		}
>   	}
>   }
>   #endif
> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> index e55070303f..28d749538c 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -447,12 +447,6 @@ struct global_data {
>   	 */
>   	fdt_addr_t translation_offset;
>   #endif
> -#if CONFIG_IS_ENABLED(WDT)
> -	/**
> -	 * @watchdog_dev: watchdog device
> -	 */
> -	struct udevice *watchdog_dev;
> -#endif

After applying the patchset v4 to current master, I see this error when
building for "x530":

board/alliedtelesis/x530/x530.c: In function 'arch_preboot_os':
board/alliedtelesis/x530/x530.c:125:13: error: 'gd_t' {aka 'volatile 
struct global_data'} has no member named 'watchdog_dev'
   125 |  wdt_stop(gd->watchdog_dev);
       |             ^~
make[1]: *** [scripts/Makefile.build:254: 
board/alliedtelesis/x530/x530.o] Error 1

Perhaps we need a common function now to stop all watchdogs, which can
be called from such places?

Thanks,
Stefan

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

[Rasmus Villemoes]

On 03/08/2021 10.28, Stefan Roese wrote:
> Hi Rasmus,
> 
>>   #endif
>> diff --git a/include/asm-generic/global_data.h
>> b/include/asm-generic/global_data.h
>> index e55070303f..28d749538c 100644
>> --- a/include/asm-generic/global_data.h
>> +++ b/include/asm-generic/global_data.h
>> @@ -447,12 +447,6 @@ struct global_data {
>>        */
>>       fdt_addr_t translation_offset;
>>   #endif
>> -#if CONFIG_IS_ENABLED(WDT)
>> -    /**
>> -     * @watchdog_dev: watchdog device
>> -     */
>> -    struct udevice *watchdog_dev;
>> -#endif
> 
> After applying the patchset v4 to current master, I see this error when
> building for "x530":

I'm not sure how I missed this, I was convinced I had done a grep and
seen that all references to ->watchdog_dev were gone.

> board/alliedtelesis/x530/x530.c: In function 'arch_preboot_os':
> board/alliedtelesis/x530/x530.c:125:13: error: 'gd_t' {aka 'volatile
> struct global_data'} has no member named 'watchdog_dev'
>   125 |  wdt_stop(gd->watchdog_dev);
>       |             ^~
> make[1]: *** [scripts/Makefile.build:254:
> board/alliedtelesis/x530/x530.o] Error 1
> 
> Perhaps we need a common function now to stop all watchdogs, which can
> be called from such places?

Yes, I think that's the right thing, even if there's just one single
caller. Dead code elimination should remove that global function for all
other boards. Here is what I have right now:

    watchdog: wdt-uclass: add wdt_stop_all() helper

    Since the watchdog_dev member of struct global_data is going away in
    favor of the wdt-uclass handling all watchdog devices, prepare for
    that by adding a helper to call wdt_stop() on all known devices.

    Initially, this will only be used in one single
    place (board/alliedtelesis/x530/x530.c), and that user currently
    ignores the return value of wdt_stop(). It's not clear what one should
    do if we encounter an error (remember the error but still stop the
    remaining ones? return immediately? try to unwind and
    restart the devices already stopped?). Moreover, some watchdogs are by
    design always-running (and don't have a ->stop method at all), so at
    the very least some exception for -ENOSYS would be in order.

    So for now, and until a real use case appears from which we can decide
    the desired semantics, have the function return void and just emit a
    log_debug() if an error is encountered.

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 358fc68e27..75ff4c2a6c 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -116,6 +116,31 @@ int wdt_stop(struct udevice *dev)
        return ret;
 }

+void wdt_stop_all(void)
+{
+       struct wdt_priv *priv;
+       struct udevice *dev;
+       struct uclass *uc;
+       int ret;
+
+       ret = uclass_get(UCLASS_WDT, &uc);
+       if (ret) {
+               log_debug("Error getting UCLASS_WDT: %d\n", ret);
+               return;
+       }
+
+       uclass_foreach_dev(dev, uc) {
+               if (!device_active(dev))
+                       continue;
+               priv = dev_get_uclass_priv(dev);
+               if (!priv->running)
+                       continue;
+               ret = wdt_stop(dev);
+               if (ret)
+                       log_debug("wdt_stop(%s) failed: %d\n",
dev->name, ret);
+       }
+}
+
 int wdt_reset(struct udevice *dev)
 {
        const struct wdt_ops *ops = device_get_ops(dev);

(along with a declaration in wdt.h, and another patch for x530 to make
use of it). Something like that?

Rasmus

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

[Stefan Roese]

Hi Rasmus,

On 11.08.21 13:32, Rasmus Villemoes wrote:
> On 03/08/2021 10.28, Stefan Roese wrote:
>> Hi Rasmus,
>>
>>>    #endif
>>> diff --git a/include/asm-generic/global_data.h
>>> b/include/asm-generic/global_data.h
>>> index e55070303f..28d749538c 100644
>>> --- a/include/asm-generic/global_data.h
>>> +++ b/include/asm-generic/global_data.h
>>> @@ -447,12 +447,6 @@ struct global_data {
>>>         */
>>>        fdt_addr_t translation_offset;
>>>    #endif
>>> -#if CONFIG_IS_ENABLED(WDT)
>>> -    /**
>>> -     * @watchdog_dev: watchdog device
>>> -     */
>>> -    struct udevice *watchdog_dev;
>>> -#endif
>>
>> After applying the patchset v4 to current master, I see this error when
>> building for "x530":
> 
> I'm not sure how I missed this, I was convinced I had done a grep and
> seen that all references to ->watchdog_dev were gone.
> 
>> board/alliedtelesis/x530/x530.c: In function 'arch_preboot_os':
>> board/alliedtelesis/x530/x530.c:125:13: error: 'gd_t' {aka 'volatile
>> struct global_data'} has no member named 'watchdog_dev'
>>    125 |  wdt_stop(gd->watchdog_dev);
>>        |             ^~
>> make[1]: *** [scripts/Makefile.build:254:
>> board/alliedtelesis/x530/x530.o] Error 1
>>
>> Perhaps we need a common function now to stop all watchdogs, which can
>> be called from such places?
> 
> Yes, I think that's the right thing, even if there's just one single
> caller. Dead code elimination should remove that global function for all
> other boards. Here is what I have right now:
> 
>      watchdog: wdt-uclass: add wdt_stop_all() helper
> 
>      Since the watchdog_dev member of struct global_data is going away in
>      favor of the wdt-uclass handling all watchdog devices, prepare for
>      that by adding a helper to call wdt_stop() on all known devices.
> 
>      Initially, this will only be used in one single
>      place (board/alliedtelesis/x530/x530.c), and that user currently
>      ignores the return value of wdt_stop(). It's not clear what one should
>      do if we encounter an error (remember the error but still stop the
>      remaining ones? return immediately? try to unwind and
>      restart the devices already stopped?). Moreover, some watchdogs are by
>      design always-running (and don't have a ->stop method at all), so at
>      the very least some exception for -ENOSYS would be in order.
> 
>      So for now, and until a real use case appears from which we can decide
>      the desired semantics, have the function return void and just emit a
>      log_debug() if an error is encountered.
> 
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 358fc68e27..75ff4c2a6c 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -116,6 +116,31 @@ int wdt_stop(struct udevice *dev)
>          return ret;
>   }
> 
> +void wdt_stop_all(void)
> +{
> +       struct wdt_priv *priv;
> +       struct udevice *dev;
> +       struct uclass *uc;
> +       int ret;
> +
> +       ret = uclass_get(UCLASS_WDT, &uc);
> +       if (ret) {
> +               log_debug("Error getting UCLASS_WDT: %d\n", ret);

Perhaps log_err()?

> +               return;
> +       }
> +
> +       uclass_foreach_dev(dev, uc) {
> +               if (!device_active(dev))
> +                       continue;
> +               priv = dev_get_uclass_priv(dev);
> +               if (!priv->running)
> +                       continue;
> +               ret = wdt_stop(dev);
> +               if (ret)
> +                       log_debug("wdt_stop(%s) failed: %d\n",
> dev->name, ret);

Again, log_err() might be better?

> +       }
> +}
> +
>   int wdt_reset(struct udevice *dev)
>   {
>          const struct wdt_ops *ops = device_get_ops(dev);
> 
> (along with a declaration in wdt.h, and another patch for x530 to make
> use of it). Something like that?

Looks good, thanks for quickly working on this. Not sure, if this new
function should be "void" or better "int" so that the error can be
returned.

A follow-up patch on-top your patchset v4 will cause git bisect
problems. So you need to integrate this into your patches and send a
v5 I'm afraid.

Thanks,
Stefan

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

[Rasmus Villemoes]

On 11/08/2021 13.49, Stefan Roese wrote:
> Hi Rasmus,
> 
> On 11.08.21 13:32, Rasmus Villemoes wrote:
>> On 03/08/2021 10.28, Stefan Roese wrote:
>>> Hi Rasmus,
>>>
>>>>    #endif
>>>> diff --git a/include/asm-generic/global_data.h
>>>> b/include/asm-generic/global_data.h
>>>> index e55070303f..28d749538c 100644
>>>> --- a/include/asm-generic/global_data.h
>>>> +++ b/include/asm-generic/global_data.h
>>>> @@ -447,12 +447,6 @@ struct global_data {
>>>>         */
>>>>        fdt_addr_t translation_offset;
>>>>    #endif
>>>> -#if CONFIG_IS_ENABLED(WDT)
>>>> -    /**
>>>> -     * @watchdog_dev: watchdog device
>>>> -     */
>>>> -    struct udevice *watchdog_dev;
>>>> -#endif
>>>
>>> After applying the patchset v4 to current master, I see this error when
>>> building for "x530":
>>
>> I'm not sure how I missed this, I was convinced I had done a grep and
>> seen that all references to ->watchdog_dev were gone.
>>
>>> board/alliedtelesis/x530/x530.c: In function 'arch_preboot_os':
>>> board/alliedtelesis/x530/x530.c:125:13: error: 'gd_t' {aka 'volatile
>>> struct global_data'} has no member named 'watchdog_dev'
>>>    125 |  wdt_stop(gd->watchdog_dev);
>>>        |             ^~
>>> make[1]: *** [scripts/Makefile.build:254:
>>> board/alliedtelesis/x530/x530.o] Error 1
>>>
>>> Perhaps we need a common function now to stop all watchdogs, which can
>>> be called from such places?
>>
>> Yes, I think that's the right thing, even if there's just one single
>> caller. Dead code elimination should remove that global function for all
>> other boards. Here is what I have right now:
>>
>>      watchdog: wdt-uclass: add wdt_stop_all() helper
>>
>>      Since the watchdog_dev member of struct global_data is going away in
>>      favor of the wdt-uclass handling all watchdog devices, prepare for
>>      that by adding a helper to call wdt_stop() on all known devices.
>>
>>      Initially, this will only be used in one single
>>      place (board/alliedtelesis/x530/x530.c), and that user currently
>>      ignores the return value of wdt_stop(). It's not clear what one
>> should
>>      do if we encounter an error (remember the error but still stop the
>>      remaining ones? return immediately? try to unwind and
>>      restart the devices already stopped?). Moreover, some watchdogs
>> are by
>>      design always-running (and don't have a ->stop method at all), so at
>>      the very least some exception for -ENOSYS would be in order.
>>
>>      So for now, and until a real use case appears from which we can
>> decide
>>      the desired semantics, have the function return void and just emit a
>>      log_debug() if an error is encountered.
>>
>> diff --git a/drivers/watchdog/wdt-uclass.c
>> b/drivers/watchdog/wdt-uclass.c
>> index 358fc68e27..75ff4c2a6c 100644
>> --- a/drivers/watchdog/wdt-uclass.c
>> +++ b/drivers/watchdog/wdt-uclass.c
>> @@ -116,6 +116,31 @@ int wdt_stop(struct udevice *dev)
>>          return ret;
>>   }
>>
>> +void wdt_stop_all(void)
>> +{
>> +       struct wdt_priv *priv;
>> +       struct udevice *dev;
>> +       struct uclass *uc;
>> +       int ret;
>> +
>> +       ret = uclass_get(UCLASS_WDT, &uc);
>> +       if (ret) {
>> +               log_debug("Error getting UCLASS_WDT: %d\n", ret);
> 
> Perhaps log_err()?

No, we've already been over this in earlier discussions (it's the exact
same pattern and reasoning as initr_watchdog). If I made it log_err(),
it would cost .text for something that never-ever happens in practice,
while log_debug() is usually a no-op, but can be compiled in if
something truly fishy seems to be going on.

>>   int wdt_reset(struct udevice *dev)
>>   {
>>          const struct wdt_ops *ops = device_get_ops(dev);
>>
>> (along with a declaration in wdt.h, and another patch for x530 to make
>> use of it). Something like that?
> 
> Looks good, thanks for quickly working on this. Not sure, if this new
> function should be "void" or better "int" so that the error can be
> returned.

That's why I included my tentative commit log, so you could see my
explanation for why I made it void. Until some user shows up that
_wants_ a return value, there's no point making it return int. When that
user shows up, we can discuss which int (return early on failure?
remember that an error was seen but still call wdt_stop on remaining
devices? etc. etc.).

> A follow-up patch on-top your patchset v4 will cause git bisect
> problems. So you need to integrate this into your patches and send a
> v5 I'm afraid.

Definitely, it's already reworked in between 6 and 7 in my local branch,
I just didn't want to send a 12-patch series without some "yeah,
something like that would work".

Rasmus

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

[Stefan Roese]

On 11.08.21 14:13, Rasmus Villemoes wrote:
> On 11/08/2021 13.49, Stefan Roese wrote:
>> Hi Rasmus,
>>
>> On 11.08.21 13:32, Rasmus Villemoes wrote:
>>> On 03/08/2021 10.28, Stefan Roese wrote:
>>>> Hi Rasmus,
>>>>
>>>>>     #endif
>>>>> diff --git a/include/asm-generic/global_data.h
>>>>> b/include/asm-generic/global_data.h
>>>>> index e55070303f..28d749538c 100644
>>>>> --- a/include/asm-generic/global_data.h
>>>>> +++ b/include/asm-generic/global_data.h
>>>>> @@ -447,12 +447,6 @@ struct global_data {
>>>>>          */
>>>>>         fdt_addr_t translation_offset;
>>>>>     #endif
>>>>> -#if CONFIG_IS_ENABLED(WDT)
>>>>> -    /**
>>>>> -     * @watchdog_dev: watchdog device
>>>>> -     */
>>>>> -    struct udevice *watchdog_dev;
>>>>> -#endif
>>>>
>>>> After applying the patchset v4 to current master, I see this error when
>>>> building for "x530":
>>>
>>> I'm not sure how I missed this, I was convinced I had done a grep and
>>> seen that all references to ->watchdog_dev were gone.
>>>
>>>> board/alliedtelesis/x530/x530.c: In function 'arch_preboot_os':
>>>> board/alliedtelesis/x530/x530.c:125:13: error: 'gd_t' {aka 'volatile
>>>> struct global_data'} has no member named 'watchdog_dev'
>>>>     125 |  wdt_stop(gd->watchdog_dev);
>>>>         |             ^~
>>>> make[1]: *** [scripts/Makefile.build:254:
>>>> board/alliedtelesis/x530/x530.o] Error 1
>>>>
>>>> Perhaps we need a common function now to stop all watchdogs, which can
>>>> be called from such places?
>>>
>>> Yes, I think that's the right thing, even if there's just one single
>>> caller. Dead code elimination should remove that global function for all
>>> other boards. Here is what I have right now:
>>>
>>>       watchdog: wdt-uclass: add wdt_stop_all() helper
>>>
>>>       Since the watchdog_dev member of struct global_data is going away in
>>>       favor of the wdt-uclass handling all watchdog devices, prepare for
>>>       that by adding a helper to call wdt_stop() on all known devices.
>>>
>>>       Initially, this will only be used in one single
>>>       place (board/alliedtelesis/x530/x530.c), and that user currently
>>>       ignores the return value of wdt_stop(). It's not clear what one
>>> should
>>>       do if we encounter an error (remember the error but still stop the
>>>       remaining ones? return immediately? try to unwind and
>>>       restart the devices already stopped?). Moreover, some watchdogs
>>> are by
>>>       design always-running (and don't have a ->stop method at all), so at
>>>       the very least some exception for -ENOSYS would be in order.
>>>
>>>       So for now, and until a real use case appears from which we can
>>> decide
>>>       the desired semantics, have the function return void and just emit a
>>>       log_debug() if an error is encountered.
>>>
>>> diff --git a/drivers/watchdog/wdt-uclass.c
>>> b/drivers/watchdog/wdt-uclass.c
>>> index 358fc68e27..75ff4c2a6c 100644
>>> --- a/drivers/watchdog/wdt-uclass.c
>>> +++ b/drivers/watchdog/wdt-uclass.c
>>> @@ -116,6 +116,31 @@ int wdt_stop(struct udevice *dev)
>>>           return ret;
>>>    }
>>>
>>> +void wdt_stop_all(void)
>>> +{
>>> +       struct wdt_priv *priv;
>>> +       struct udevice *dev;
>>> +       struct uclass *uc;
>>> +       int ret;
>>> +
>>> +       ret = uclass_get(UCLASS_WDT, &uc);
>>> +       if (ret) {
>>> +               log_debug("Error getting UCLASS_WDT: %d\n", ret);
>>
>> Perhaps log_err()?
> 
> No, we've already been over this in earlier discussions (it's the exact
> same pattern and reasoning as initr_watchdog). If I made it log_err(),
> it would cost .text for something that never-ever happens in practice,
> while log_debug() is usually a no-op, but can be compiled in if
> something truly fishy seems to be going on.

Okay, then please stick with log_debug().

>>>    int wdt_reset(struct udevice *dev)
>>>    {
>>>           const struct wdt_ops *ops = device_get_ops(dev);
>>>
>>> (along with a declaration in wdt.h, and another patch for x530 to make
>>> use of it). Something like that?
>>
>> Looks good, thanks for quickly working on this. Not sure, if this new
>> function should be "void" or better "int" so that the error can be
>> returned.
> 
> That's why I included my tentative commit log, so you could see my
> explanation for why I made it void.

Ah, sorry for not reading it thoroughly. My bad.

> Until some user shows up that
> _wants_ a return value, there's no point making it return int. When that
> user shows up, we can discuss which int (return early on failure?
> remember that an error was seen but still call wdt_stop on remaining
> devices? etc. etc.).

I'm fine with it.

>> A follow-up patch on-top your patchset v4 will cause git bisect
>> problems. So you need to integrate this into your patches and send a
>> v5 I'm afraid.
> 
> Definitely, it's already reworked in between 6 and 7 in my local branch,
> I just didn't want to send a 12-patch series without some "yeah,
> something like that would work".

Perfect. Please go ahead then.

Thanks,
Stefan

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

[Wolfgang Denk]

Dear Rasmus,

In message <3d48015a-07d3-e296-b9ba-a1edd455ce9e@prevas.dk> you wrote:
>
> >> +       if (ret) {
> >> +               log_debug("Error getting UCLASS_WDT: %d\n", ret);
> > 
> > Perhaps log_err()?
>
> No, we've already been over this in earlier discussions (it's the exact
> same pattern and reasoning as initr_watchdog). If I made it log_err(),
> it would cost .text for something that never-ever happens in practice,
> while log_debug() is usually a no-op, but can be compiled in if
> something truly fishy seems to be going on.

This argument fits on all types or effors: they are supposed to
never ever happen - at least in theory; in reality they do, and more
often than we like.

And a proper error message is mandatory for correct error handling.

> > Looks good, thanks for quickly working on this. Not sure, if this new
> > function should be "void" or better "int" so that the error can be
> > returned.
>
> That's why I included my tentative commit log, so you could see my
> explanation for why I made it void. Until some user shows up that
> _wants_ a return value, there's no point making it return int. When that
> user shows up, we can discuss which int (return early on failure?
> remember that an error was seen but still call wdt_stop on remaining
> devices? etc. etc.).

Returning an error code is always a good ide, no matter if
current users check it or not.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
To the systems programmer,  users  and  applications  serve  only  to
provide a test load.

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2043 bytes --]

On Wed, Aug 11, 2021 at 02:29:12PM +0200, Wolfgang Denk wrote:
> Dear Rasmus,
> 
> In message <3d48015a-07d3-e296-b9ba-a1edd455ce9e@prevas.dk> you wrote:
> >
> > >> +       if (ret) {
> > >> +               log_debug("Error getting UCLASS_WDT: %d\n", ret);
> > > 
> > > Perhaps log_err()?
> >
> > No, we've already been over this in earlier discussions (it's the exact
> > same pattern and reasoning as initr_watchdog). If I made it log_err(),
> > it would cost .text for something that never-ever happens in practice,
> > while log_debug() is usually a no-op, but can be compiled in if
> > something truly fishy seems to be going on.
> 
> This argument fits on all types or effors: they are supposed to
> never ever happen - at least in theory; in reality they do, and more
> often than we like.
> 
> And a proper error message is mandatory for correct error handling.

Error messages are a fine line to walk.  We've got to have been very
badly corrupted to go down this error path.  There's going to be lots of
other error messages popping out.  Saving a bit of .text is good.  It
makes it easier to justify spending a little .text later.

> > > Looks good, thanks for quickly working on this. Not sure, if this new
> > > function should be "void" or better "int" so that the error can be
> > > returned.
> >
> > That's why I included my tentative commit log, so you could see my
> > explanation for why I made it void. Until some user shows up that
> > _wants_ a return value, there's no point making it return int. When that
> > user shows up, we can discuss which int (return early on failure?
> > remember that an error was seen but still call wdt_stop on remaining
> > devices? etc. etc.).
> 
> Returning an error code is always a good ide, no matter if
> current users check it or not.

And here I agree, catch an error code, pass the error code back to the
caller.  That's far more important than making sure that every error
code we catch logs a message by default every time.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Wolfgang Denk]

Dear Tom,

In message <20210811124318.GT858@bill-the-cat> you wrote:
> 
> > This argument fits on all types or effors: they are supposed to
> > never ever happen - at least in theory; in reality they do, and more
> > often than we like.
> > 
> > And a proper error message is mandatory for correct error handling.
> 
> Error messages are a fine line to walk.  We've got to have been very
> badly corrupted to go down this error path.  There's going to be lots of
> other error messages popping out.  Saving a bit of .text is good.  It
> makes it easier to justify spending a little .text later.

Letting errors slip through unnoticed when there is a trival way to
at least inform the user of the problem is simply unacceptable.

Please do not let U-Boot degrade into such a crappy piece of code.

There are tons of other places where we don't even mention code
size, so if you want to save on that, there are many bette4r places
to save than error handling.

> And here I agree, catch an error code, pass the error code back to the
> caller.  That's far more important than making sure that every error
> code we catch logs a message by default every time.

It does not matter where the error is reported - in the called
function, or in some caller firther up the call tree.  But it _must_
be reportet at least once.

So if we don't issue an error message here, we need to check and fix
the callers, too.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
If programming was easy, they wouldn't need something as  complicated
as a human being to do it, now would they?
                       - L. Wall & R. L. Schwartz, _Programming Perl_

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3271 bytes --]

On Thu, Aug 12, 2021 at 08:40:21AM +0200, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20210811124318.GT858@bill-the-cat> you wrote:
> > 
> > > This argument fits on all types or effors: they are supposed to
> > > never ever happen - at least in theory; in reality they do, and more
> > > often than we like.
> > > 
> > > And a proper error message is mandatory for correct error handling.
> > 
> > Error messages are a fine line to walk.  We've got to have been very
> > badly corrupted to go down this error path.  There's going to be lots of
> > other error messages popping out.  Saving a bit of .text is good.  It
> > makes it easier to justify spending a little .text later.
> 
> Letting errors slip through unnoticed when there is a trival way to
> at least inform the user of the problem is simply unacceptable.
> 
> Please do not let U-Boot degrade into such a crappy piece of code.
> 
> There are tons of other places where we don't even mention code
> size, so if you want to save on that, there are many bette4r places
> to save than error handling.

Alright, lets take a look at what kind of area of the code we're talking
about.  uclass_get is a pretty fundamental thing.  If that fails, your
system is on fire.  Things are massively corrupt.  Lets look at other
existing callers to see what happens.  Most callers check the return
code, like you need to, and pass it up the chain to deal with.  We have
a few board specific ones such as
board/Marvell/octeontx2/board.c::board_quiesce_devices() that is also
conceptually like the x530 case in the next part of the series.  That
does print on failure.  The rest of the ones that print an error message
are about commands and it's somewhat helpful there.

So yes, return codes need to be checked and passed.  But no, not every
single error path needs to print to the user along every part of an
error path either.

> > And here I agree, catch an error code, pass the error code back to the
> > caller.  That's far more important than making sure that every error
> > code we catch logs a message by default every time.
> 
> It does not matter where the error is reported - in the called
> function, or in some caller firther up the call tree.  But it _must_
> be reportet at least once.
> 
> So if we don't issue an error message here, we need to check and fix
> the callers, too.

That would be the next patch in the series where the BSP author isn't
currently checking the return value, and this series doesn't change
that.  Perhaps it should, and CC the maintainer.  But I think has been
said a few times over the course of this series, what exactly is one
going to do about the failure?  Getting specific for a moment, if you're
in the case of "shutdown the watchdog" and the watchdog doesn't shutdown
like you want it to, do you hang and hope the watchdog is alive to kick
things still?  hang and lock the system?  Figure the system is on fire
anyhow but add another message to the failure spew?

Again, I think the change that's needed to this patch is to make it
return the error code to the caller.  Let the caller decide.  And make
sure to CC the board maintainer on the next go-round so they can chime
in about that.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Simon Glass]

Hi Tom,

On Thu, 12 Aug 2021 at 07:48, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Aug 12, 2021 at 08:40:21AM +0200, Wolfgang Denk wrote:
> > Dear Tom,
> >
> > In message <20210811124318.GT858@bill-the-cat> you wrote:
> > >
> > > > This argument fits on all types or effors: they are supposed to
> > > > never ever happen - at least in theory; in reality they do, and more
> > > > often than we like.
> > > >
> > > > And a proper error message is mandatory for correct error handling.
> > >
> > > Error messages are a fine line to walk.  We've got to have been very
> > > badly corrupted to go down this error path.  There's going to be lots of
> > > other error messages popping out.  Saving a bit of .text is good.  It
> > > makes it easier to justify spending a little .text later.
> >
> > Letting errors slip through unnoticed when there is a trival way to
> > at least inform the user of the problem is simply unacceptable.
> >
> > Please do not let U-Boot degrade into such a crappy piece of code.
> >
> > There are tons of other places where we don't even mention code
> > size, so if you want to save on that, there are many bette4r places
> > to save than error handling.
>
> Alright, lets take a look at what kind of area of the code we're talking
> about.  uclass_get is a pretty fundamental thing.  If that fails, your
> system is on fire.  Things are massively corrupt.  Lets look at other
> existing callers to see what happens.  Most callers check the return
> code, like you need to, and pass it up the chain to deal with.  We have
> a few board specific ones such as
> board/Marvell/octeontx2/board.c::board_quiesce_devices() that is also
> conceptually like the x530 case in the next part of the series.  That
> does print on failure.  The rest of the ones that print an error message
> are about commands and it's somewhat helpful there.
>
> So yes, return codes need to be checked and passed.  But no, not every
> single error path needs to print to the user along every part of an
> error path either.
>
> > > And here I agree, catch an error code, pass the error code back to the
> > > caller.  That's far more important than making sure that every error
> > > code we catch logs a message by default every time.
> >
> > It does not matter where the error is reported - in the called
> > function, or in some caller firther up the call tree.  But it _must_
> > be reportet at least once.
> >
> > So if we don't issue an error message here, we need to check and fix
> > the callers, too.
>
> That would be the next patch in the series where the BSP author isn't
> currently checking the return value, and this series doesn't change
> that.  Perhaps it should, and CC the maintainer.  But I think has been
> said a few times over the course of this series, what exactly is one
> going to do about the failure?  Getting specific for a moment, if you're
> in the case of "shutdown the watchdog" and the watchdog doesn't shutdown
> like you want it to, do you hang and hope the watchdog is alive to kick
> things still?  hang and lock the system?  Figure the system is on fire
> anyhow but add another message to the failure spew?
>
> Again, I think the change that's needed to this patch is to make it
> return the error code to the caller.  Let the caller decide.  And make
> sure to CC the board maintainer on the next go-round so they can chime
> in about that.

I strongly agree with this.I try to encourage people not to report
errors inside drivers, since the caller should be able to deal with
it, particularly if the error number provides useful information. It
bloats the code.

But we do have these strange cases where there is no obvious thing to
do. Where we are probing several devices to fire them up and one
fails, people worry that this means that some of them won't get
probed. In that case I tend to continue on and probe them all, but
then return error if any of them failed. At least the caller knows.

Also I like the new log_ret() and log_msg_ret() functions, which can
log an error as it goes up the stack if LOG_ERROR_RETURN is enabled.
It is nice to be able to see where an error came from. We could
perhaps improve this by logging the error when it is created (the
first time some calls 'return -Exxx').

I'm not a fan of board code which calls a function and doesn't check
the error. The board may not operate correctly, or it may limp along,
but the board author should be able to get all the bugs of it at the
start so that we are not requested invalid clocks, etc.

Regards,
Simon

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

[Wolfgang Denk]

Dear Tom,

In message <20210812134833.GU858@bill-the-cat> you wrote:
> 
> Alright, lets take a look at what kind of area of the code we're talking
> about.  uclass_get is a pretty fundamental thing.  If that fails, your
> system is on fire.  Things are massively corrupt.

Full agreement here.

> So yes, return codes need to be checked and passed.  But no, not every
> single error path needs to print to the user along every part of an
> error path either.

So if "the system is on fire" is one of the cases where an error
message should be omitted to save maybe 50 or 100 bytes of image
size?  This sounds wrong to me.

When a system crashes or hangs, it is extremely helpful to gen an
indication of what happened.

Printing messages only with debug enabled is pretty worthless, as in
the real world the failures will happen in the field when running
the production image with debug not enabled.

And as soon as you do enable debug, you will have a different image,
which may or may not show the problem.  We have all been there
before.

> That would be the next patch in the series where the BSP author isn't
> currently checking the return value, and this series doesn't change
> that.  Perhaps it should, and CC the maintainer.

Indeed.

> But I think has been
> said a few times over the course of this series, what exactly is one
> going to do about the failure?  Getting specific for a moment, if you're
> in the case of "shutdown the watchdog" and the watchdog doesn't shutdown
> like you want it to, do you hang and hope the watchdog is alive to kick
> things still?  hang and lock the system?  Figure the system is on fire
> anyhow but add another message to the failure spew?

Adding a message about the _cause_ of the failure, i. e. at least
information about the place where it was first detected, is what
will be helpful to the engineer who is supposed to analyze the
problem.

And yes, if such a fundamental operation fails, it is wise to abort
the operation of this device - either by hard resetting it or by
hanging it, depending of what the chances are that a reboot might
fix the problem.

IMO it is better to fail a broken device in a reliable mode instead
of letting it run and having more spectacular crashes (likely with
more serious consequences) happen later.

> Again, I think the change that's needed to this patch is to make it
> return the error code to the caller.  Let the caller decide.  And make
> sure to CC the board maintainer on the next go-round so they can chime
> in about that.

If we agree that in the disputed case "the system is on fire", then
there is actually very little to decide.  There should be only one
possible action: stop here, before more damage happens.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
... not meant for the weak-at-heart: /^(?=.*one)(?=.*two)/
If you can follow that, you can use it.
          - Randal L. Schwartz in <ukpw67rhl3.fsf@julie.teleport.com>

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1608 bytes --]

On Thu, Aug 12, 2021 at 04:21:29PM +0200, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20210812134833.GU858@bill-the-cat> you wrote:
> > 
> > Alright, lets take a look at what kind of area of the code we're talking
> > about.  uclass_get is a pretty fundamental thing.  If that fails, your
> > system is on fire.  Things are massively corrupt.
> 
> Full agreement here.
> 
> > So yes, return codes need to be checked and passed.  But no, not every
> > single error path needs to print to the user along every part of an
> > error path either.
> 
> So if "the system is on fire" is one of the cases where an error
> message should be omitted to save maybe 50 or 100 bytes of image
> size?  This sounds wrong to me.

It sounds right to me because it's unlikely everything caught fire
because of this call right here and likely it's because of one of the
messages much further up on the console log.  Hopefully we haven't
caused that message to be unavailable now due to unhelpful failure
messages.

A log message needs to have value to it above and beyond boiling down to
"%s: %d", __func__, __LINE__ having been reached.  This, right here, is
not a log message that matters.  With DM we've made a great deal of
progress in being able to populate meaningful errors back up to our
callers rather than -1 for everything.  So yes, in sum, these functions
need to return a value.  The BSP ought to care (in the next patch), even
if it doesn't today when it could.  But that's on the BSP author as they
know better than you or I what that system is being used for.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Wolfgang Denk]

Dear Tom,

In message <20210812162034.GY858@bill-the-cat> you wrote:
> 
> > So if "the system is on fire" is one of the cases where an error
> > message should be omitted to save maybe 50 or 100 bytes of image
> > size?  This sounds wrong to me.
>
> It sounds right to me because it's unlikely everything caught fire
> because of this call right here and likely it's because of one of the
> messages much further up on the console log.  Hopefully we haven't
> caused that message to be unavailable now due to unhelpful failure
> messages.

Omitting code to handle situations that are unlikely to happen is
definitely not what I consider robust programming, and nothing which
I would let pass a code review.

But if you insist, there is no more to do for me here that to note
that fact that we disagree.


Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
People are very flexible and learn to adjust to strange  surroundings
--  they can become accustomed to read Lisp and Fortran programs, for
example.   - Leon Sterling and Ehud Shapiro, Art of Prolog, MIT Press

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

[Stefan Roese]

On 12.08.21 15:48, Tom Rini wrote:
> On Thu, Aug 12, 2021 at 08:40:21AM +0200, Wolfgang Denk wrote:
>> Dear Tom,
>>
>> In message <20210811124318.GT858@bill-the-cat> you wrote:
>>>
>>>> This argument fits on all types or effors: they are supposed to
>>>> never ever happen - at least in theory; in reality they do, and more
>>>> often than we like.
>>>>
>>>> And a proper error message is mandatory for correct error handling.
>>>
>>> Error messages are a fine line to walk.  We've got to have been very
>>> badly corrupted to go down this error path.  There's going to be lots of
>>> other error messages popping out.  Saving a bit of .text is good.  It
>>> makes it easier to justify spending a little .text later.
>>
>> Letting errors slip through unnoticed when there is a trival way to
>> at least inform the user of the problem is simply unacceptable.
>>
>> Please do not let U-Boot degrade into such a crappy piece of code.
>>
>> There are tons of other places where we don't even mention code
>> size, so if you want to save on that, there are many bette4r places
>> to save than error handling.
> 
> Alright, lets take a look at what kind of area of the code we're talking
> about.  uclass_get is a pretty fundamental thing.  If that fails, your
> system is on fire.  Things are massively corrupt.  Lets look at other
> existing callers to see what happens.  Most callers check the return
> code, like you need to, and pass it up the chain to deal with.  We have
> a few board specific ones such as
> board/Marvell/octeontx2/board.c::board_quiesce_devices() that is also
> conceptually like the x530 case in the next part of the series.  That
> does print on failure.  The rest of the ones that print an error message
> are about commands and it's somewhat helpful there.
> 
> So yes, return codes need to be checked and passed.  But no, not every
> single error path needs to print to the user along every part of an
> error path either.
> 
>>> And here I agree, catch an error code, pass the error code back to the
>>> caller.  That's far more important than making sure that every error
>>> code we catch logs a message by default every time.
>>
>> It does not matter where the error is reported - in the called
>> function, or in some caller firther up the call tree.  But it _must_
>> be reportet at least once.
>>
>> So if we don't issue an error message here, we need to check and fix
>> the callers, too.
> 
> That would be the next patch in the series where the BSP author isn't
> currently checking the return value, and this series doesn't change
> that.  Perhaps it should, and CC the maintainer.  But I think has been
> said a few times over the course of this series, what exactly is one
> going to do about the failure?  Getting specific for a moment, if you're
> in the case of "shutdown the watchdog" and the watchdog doesn't shutdown
> like you want it to, do you hang and hope the watchdog is alive to kick
> things still?  hang and lock the system?  Figure the system is on fire
> anyhow but add another message to the failure spew?
> 
> Again, I think the change that's needed to this patch is to make it
> return the error code to the caller.  Let the caller decide.  And make
> sure to CC the board maintainer on the next go-round so they can chime
> in about that.

Getting back to this to hopefully get this decided:

It seems that we (most of us?) agree on this change, that wdt_stop_all()
shall be changed to return an error code and the caller can decide what
to do with it?

If yes, then Rasmus, could you please re-spin this patchset accordingly
and send v6?

Thanks,
Stefan

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3986 bytes --]

On Tue, Aug 17, 2021 at 11:28:39AM +0200, Stefan Roese wrote:
> On 12.08.21 15:48, Tom Rini wrote:
> > On Thu, Aug 12, 2021 at 08:40:21AM +0200, Wolfgang Denk wrote:
> > > Dear Tom,
> > > 
> > > In message <20210811124318.GT858@bill-the-cat> you wrote:
> > > > 
> > > > > This argument fits on all types or effors: they are supposed to
> > > > > never ever happen - at least in theory; in reality they do, and more
> > > > > often than we like.
> > > > > 
> > > > > And a proper error message is mandatory for correct error handling.
> > > > 
> > > > Error messages are a fine line to walk.  We've got to have been very
> > > > badly corrupted to go down this error path.  There's going to be lots of
> > > > other error messages popping out.  Saving a bit of .text is good.  It
> > > > makes it easier to justify spending a little .text later.
> > > 
> > > Letting errors slip through unnoticed when there is a trival way to
> > > at least inform the user of the problem is simply unacceptable.
> > > 
> > > Please do not let U-Boot degrade into such a crappy piece of code.
> > > 
> > > There are tons of other places where we don't even mention code
> > > size, so if you want to save on that, there are many bette4r places
> > > to save than error handling.
> > 
> > Alright, lets take a look at what kind of area of the code we're talking
> > about.  uclass_get is a pretty fundamental thing.  If that fails, your
> > system is on fire.  Things are massively corrupt.  Lets look at other
> > existing callers to see what happens.  Most callers check the return
> > code, like you need to, and pass it up the chain to deal with.  We have
> > a few board specific ones such as
> > board/Marvell/octeontx2/board.c::board_quiesce_devices() that is also
> > conceptually like the x530 case in the next part of the series.  That
> > does print on failure.  The rest of the ones that print an error message
> > are about commands and it's somewhat helpful there.
> > 
> > So yes, return codes need to be checked and passed.  But no, not every
> > single error path needs to print to the user along every part of an
> > error path either.
> > 
> > > > And here I agree, catch an error code, pass the error code back to the
> > > > caller.  That's far more important than making sure that every error
> > > > code we catch logs a message by default every time.
> > > 
> > > It does not matter where the error is reported - in the called
> > > function, or in some caller firther up the call tree.  But it _must_
> > > be reportet at least once.
> > > 
> > > So if we don't issue an error message here, we need to check and fix
> > > the callers, too.
> > 
> > That would be the next patch in the series where the BSP author isn't
> > currently checking the return value, and this series doesn't change
> > that.  Perhaps it should, and CC the maintainer.  But I think has been
> > said a few times over the course of this series, what exactly is one
> > going to do about the failure?  Getting specific for a moment, if you're
> > in the case of "shutdown the watchdog" and the watchdog doesn't shutdown
> > like you want it to, do you hang and hope the watchdog is alive to kick
> > things still?  hang and lock the system?  Figure the system is on fire
> > anyhow but add another message to the failure spew?
> > 
> > Again, I think the change that's needed to this patch is to make it
> > return the error code to the caller.  Let the caller decide.  And make
> > sure to CC the board maintainer on the next go-round so they can chime
> > in about that.
> 
> Getting back to this to hopefully get this decided:
> 
> It seems that we (most of us?) agree on this change, that wdt_stop_all()
> shall be changed to return an error code and the caller can decide what
> to do with it?
> 
> If yes, then Rasmus, could you please re-spin this patchset accordingly
> and send v6?

Yes, please and thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Stefan Roese]

Hi Tom,

On 17.08.21 14:35, Tom Rini wrote:

<snip>

>> Getting back to this to hopefully get this decided:
>>
>> It seems that we (most of us?) agree on this change, that wdt_stop_all()
>> shall be changed to return an error code and the caller can decide what
>> to do with it?
>>
>> If yes, then Rasmus, could you please re-spin this patchset accordingly
>> and send v6?
> 
> Yes, please and thanks.

Tom, would you like me to push this patchset in at this stage (rc2), or
better defer to the next merge window?

Thanks,
Stefan

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 726 bytes --]

On Fri, Aug 27, 2021 at 08:30:48AM +0200, Stefan Roese wrote:
> Hi Tom,
> 
> On 17.08.21 14:35, Tom Rini wrote:
> 
> <snip>
> 
> > > Getting back to this to hopefully get this decided:
> > > 
> > > It seems that we (most of us?) agree on this change, that wdt_stop_all()
> > > shall be changed to return an error code and the caller can decide what
> > > to do with it?
> > > 
> > > If yes, then Rasmus, could you please re-spin this patchset accordingly
> > > and send v6?
> > 
> > Yes, please and thanks.
> 
> Tom, would you like me to push this patchset in at this stage (rc2), or
> better defer to the next merge window?

I'm going to open -next on Monday, so lets put it there.  Thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Rasmus Villemoes]

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

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

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

  https://lore.kernel.org/lkml/CAL_JsqKEGaFpiFV_oAtE+S_bnHkg4qry+bhx2EDs=NSbVf_giA@mail.gmail.com/

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

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

diff --git a/doc/device-tree-bindings/watchdog/gpio-wdt.txt b/doc/device-tree-bindings/watchdog/gpio-wdt.txt
new file mode 100644
index 0000000000..c9a8559a3e
--- /dev/null
+++ b/doc/device-tree-bindings/watchdog/gpio-wdt.txt
@@ -0,0 +1,19 @@
+GPIO watchdog timer
+
+Describes a simple watchdog timer which is reset by toggling a gpio.
+
+Required properties:
+
+- compatible: Must be "linux,wdt-gpio".
+- gpios: gpio to toggle when wdt driver reset method is called.
+- always-running: Boolean property indicating that the watchdog cannot
+  be disabled. At present, U-Boot only supports this kind of GPIO
+  watchdog.
+
+Example:
+
+	gpio-wdt {
+		gpios = <&gpio0 1 0>;
+		compatible = "linux,wdt-gpio";
+		always-running;
+	};
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index f0ff2612a6..6fbb5c1b6d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -147,6 +147,15 @@ config WDT_CORTINA
 	  This driver support all CPU ISAs supported by Cortina
 	  Access CAxxxx SoCs.
 
+config WDT_GPIO
+	bool "External gpio watchdog support"
+	depends on WDT
+	depends on DM_GPIO
+	help
+	  Support for external watchdog fed by toggling a gpio. See
+	  doc/device-tree-bindings/watchdog/gpio-wdt.txt for
+	  information on how to describe the watchdog in device tree.
+
 config WDT_MPC8xx
 	bool "MPC8xx watchdog timer support"
 	depends on WDT && MPC8xx
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 5c7ef593fe..f14415bb8e 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_WDT_BOOKE) += booke_wdt.o
 obj-$(CONFIG_WDT_CORTINA) += cortina_wdt.o
 obj-$(CONFIG_WDT_ORION) += orion_wdt.o
 obj-$(CONFIG_WDT_CDNS) += cdns_wdt.o
+obj-$(CONFIG_WDT_GPIO) += gpio_wdt.o
 obj-$(CONFIG_WDT_MPC8xx) += mpc8xx_wdt.o
 obj-$(CONFIG_WDT_MT7620) += mt7620_wdt.o
 obj-$(CONFIG_WDT_MT7621) += mt7621_wdt.o
diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
new file mode 100644
index 0000000000..982a66b3f9
--- /dev/null
+++ b/drivers/watchdog/gpio_wdt.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <dm.h>
+#include <dm/device_compat.h>
+#include <wdt.h>
+#include <asm/gpio.h>
+
+struct gpio_wdt_priv {
+	struct gpio_desc gpio;
+	bool always_running;
+	int state;
+};
+
+static int gpio_wdt_reset(struct udevice *dev)
+{
+	struct gpio_wdt_priv *priv = dev_get_priv(dev);
+
+	priv->state = !priv->state;
+
+	return dm_gpio_set_value(&priv->gpio, priv->state);
+}
+
+static int gpio_wdt_start(struct udevice *dev, u64 timeout, ulong flags)
+{
+	struct gpio_wdt_priv *priv = dev_get_priv(dev);
+
+	if (priv->always_running)
+		return 0;
+
+	return -ENOSYS;
+}
+
+static int dm_probe(struct udevice *dev)
+{
+	struct gpio_wdt_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	priv->always_running = dev_read_bool(dev, "always-running");
+	ret = gpio_request_by_name(dev, "gpios", 0, &priv->gpio, GPIOD_IS_OUT);
+	if (ret < 0) {
+		dev_err(dev, "Request for wdt gpio failed: %d\n", ret);
+		return ret;
+	}
+
+	if (priv->always_running)
+		ret = gpio_wdt_reset(dev);
+
+	return ret;
+}
+
+static const struct wdt_ops gpio_wdt_ops = {
+	.start = gpio_wdt_start,
+	.reset = gpio_wdt_reset,
+};
+
+static const struct udevice_id gpio_wdt_ids[] = {
+	{ .compatible = "linux,wdt-gpio" },
+	{}
+};
+
+U_BOOT_DRIVER(wdt_gpio) = {
+	.name = "wdt_gpio",
+	.id = UCLASS_WDT,
+	.of_match = gpio_wdt_ids,
+	.ops = &gpio_wdt_ops,
+	.probe	= dm_probe,
+	.priv_auto = sizeof(struct gpio_wdt_priv),
+};
-- 
2.31.1

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

[Rasmus Villemoes]

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

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

Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Stefan Roese <sr@denx.de>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/sandbox/dts/test.dts   |  6 ++++++
 configs/sandbox64_defconfig |  1 +
 configs/sandbox_defconfig   |  1 +
 test/dm/wdt.c               | 36 +++++++++++++++++++++++++++++++++++-
 4 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index d5976318d1..fe5ac6ecd9 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -779,6 +779,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 3627a066b4..6cad90b03e 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 34b749b47b..4a8b4f220d 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -284,6 +284,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 v4 10/10] sandbox: add test of wdt-uclass' watchdog_reset()

[Rasmus Villemoes]

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

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

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

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index fe5ac6ecd9..bafc2e9494 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -782,6 +782,7 @@
 	gpio-wdt {
 		gpios = <&gpio_a 7 0>;
 		compatible = "linux,wdt-gpio";
+		hw_margin_ms = <100>;
 		always-running;
 	};
 
@@ -1264,6 +1265,7 @@
 
 	wdt0: wdt@0 {
 		compatible = "sandbox,wdt";
+		hw_margin_ms = <200>;
 	};
 
 	axi: axi@0 {
diff --git a/test/dm/wdt.c b/test/dm/wdt.c
index abff853a02..ee615f0e14 100644
--- a/test/dm/wdt.c
+++ b/test/dm/wdt.c
@@ -12,6 +12,8 @@
 #include <dm/test.h>
 #include <test/test.h>
 #include <test/ut.h>
+#include <linux/delay.h>
+#include <watchdog.h>
 
 /* Test that watchdog driver functions are called */
 static int dm_test_wdt_base(struct unit_test_state *uts)
@@ -73,3 +75,55 @@ static int dm_test_wdt_gpio(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_wdt_gpio, UT_TESTF_SCAN_FDT);
+
+static int dm_test_wdt_watchdog_reset(struct unit_test_state *uts)
+{
+	struct sandbox_state *state = state_get_current();
+	struct udevice *gpio_wdt, *sandbox_wdt;
+	struct udevice *gpio;
+	const u64 timeout = 42;
+	const int offset = 7;
+	uint reset_count;
+	int val;
+
+	ut_assertok(uclass_get_device_by_driver(UCLASS_WDT,
+						DM_DRIVER_GET(wdt_gpio), &gpio_wdt));
+	ut_assertnonnull(gpio_wdt);
+	ut_assertok(uclass_get_device_by_driver(UCLASS_WDT,
+						DM_DRIVER_GET(wdt_sandbox), &sandbox_wdt));
+	ut_assertnonnull(sandbox_wdt);
+	ut_assertok(uclass_get_device_by_name(UCLASS_GPIO, "base-gpios", &gpio));
+	ut_assertnonnull(gpio);
+
+	/* Neither device should be "started", so watchdog_reset() should be a no-op. */
+	reset_count = state->wdt.reset_count;
+	val = sandbox_gpio_get_value(gpio, offset);
+	watchdog_reset();
+	ut_asserteq(reset_count, state->wdt.reset_count);
+	ut_asserteq(val, sandbox_gpio_get_value(gpio, offset));
+
+	/* Start both devices. */
+	ut_assertok(wdt_start(gpio_wdt, timeout, 0));
+	ut_assertok(wdt_start(sandbox_wdt, timeout, 0));
+
+	/* Make sure both devices have just been pinged. */
+	timer_test_add_offset(100);
+	watchdog_reset();
+	reset_count = state->wdt.reset_count;
+	val = sandbox_gpio_get_value(gpio, offset);
+
+	/* The gpio watchdog should be pinged, the sandbox one not. */
+	timer_test_add_offset(30);
+	watchdog_reset();
+	ut_asserteq(reset_count, state->wdt.reset_count);
+	ut_asserteq(!val, sandbox_gpio_get_value(gpio, offset));
+
+	/* After another ~30ms, both devices should get pinged. */
+	timer_test_add_offset(30);
+	watchdog_reset();
+	ut_asserteq(reset_count + 1, state->wdt.reset_count);
+	ut_asserteq(val, sandbox_gpio_get_value(gpio, offset));
+
+	return 0;
+}
+DM_TEST(dm_test_wdt_watchdog_reset, UT_TESTF_SCAN_FDT);
-- 
2.31.1

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

[Rasmus Villemoes]

Ping. Stefan, any chance you could pick up this series? Simon has nodded
to the v4 version of patch 7, so now they all have at least one R-b tag.

Rasmus

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

[Stefan Roese]

Hi Rasmus,

On 11.08.21 08:05, Rasmus Villemoes wrote:
> Ping. Stefan, any chance you could pick up this series? Simon has nodded
> to the v4 version of patch 7, so now they all have at least one R-b tag.

Actually I'm waiting on a reply from you here. As I already tried to
integrate all patches in v4 and found some errors while running the
CI tests (my original reply here [1]). Here my message again:

"
After applying the patchset v4 to current master, I see this error when
building for "x530":

board/alliedtelesis/x530/x530.c: In function 'arch_preboot_os':
board/alliedtelesis/x530/x530.c:125:13: error: 'gd_t' {aka 'volatile
struct global_data'} has no member named 'watchdog_dev'
    125 |  wdt_stop(gd->watchdog_dev);
        |             ^~
make[1]: *** [scripts/Makefile.build:254:
board/alliedtelesis/x530/x530.o] Error 1

Perhaps we need a common function now to stop all watchdogs, which can
be called from such places?
"

Thanks,
Stefan

[1] https://yhbt.net/lore/all/e2e70a72-7aa0-79d4-7a4c-d542ed290f1e@denx.de/

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

[Rasmus Villemoes]

On 11/08/2021 08.10, Stefan Roese wrote:
> Hi Rasmus,
> 
> On 11.08.21 08:05, Rasmus Villemoes wrote:
>> Ping. Stefan, any chance you could pick up this series? Simon has nodded
>> to the v4 version of patch 7, so now they all have at least one R-b tag.
> 
> Actually I'm waiting on a reply from you here.

Sorry about that! It turns out quite a few mails from you were hiding in
my junk mail folder (but nothing else sent to the U-Boot mailing list).
Most seem to have just been "R-b", but I'll have to go over them to see
what other important things I've missed.

Rasmus