[U-Boot] [PATCH] watchdog: Implement generic watchdog_reset() version

[U-Boot] [PATCH] watchdog: Implement generic watchdog_reset() version

[Stefan Roese]

This patch tries to implement a generic watchdog_reset() function that
can be used by all boards that want to service the watchdog device in
U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG.

Without this approach, new boards or platforms needed to implement a
board specific version of this functionality, mostly copy'ing the same
code over and over again into their board or platforms code base.

With this new generic function, the scattered other functions are now
removed to be replaced by the generic one. The new version also enables
the configuration of the watchdog timeout via the DT "timeout-sec"
property (if enabled via CONFIG_OF_CONTROL).

This patch also adds a new flag to the GD flags, to flag that the
watchdog is ready to use and adds the pointer to the watchdog device
to the GD. This enables us to remove the global "watchdog_dev"
variable, which was prone to cause problems because of its potentially
very early use in watchdog_reset(), even before the BSS is cleared.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Tom Rini <trini@konsulko.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Heiko Schocher <hs@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: "Marek Behún" <marek.behun@nic.cz>
Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
---
This patch depends on the following patches:

[1] watchdog: Move watchdog_dev to data section (BSS may not be cleared)
https://patchwork.ozlabs.org/patch/1075500/

[2] watchdog: Handle SPL build with watchdog disabled
https://patchwork.ozlabs.org/patch/1074098/

I would like to see [2] applied in this release, since its a real fix on
some of the platforms with minimal chances of breakage.

This patch now is a bigger rework and is intended for the next merge
window. Please review and test if possible. Comments welcome.

Thanks,
Stefan

 arch/mips/mach-mt7620/cpu.c                   | 36 ---------
 board/CZ.NIC/turris_mox/turris_mox.c          | 30 --------
 board/CZ.NIC/turris_omnia/turris_omnia.c      | 35 ---------
 .../microblaze-generic/microblaze-generic.c   | 40 ----------
 board/xilinx/zynq/board.c                     | 39 ----------
 board/xilinx/zynqmp/zynqmp.c                  | 39 ----------
 drivers/watchdog/wdt-uclass.c                 | 74 +++++++++++++++++++
 include/asm-generic/global_data.h             |  4 +
 8 files changed, 78 insertions(+), 219 deletions(-)

diff --git a/arch/mips/mach-mt7620/cpu.c b/arch/mips/mach-mt7620/cpu.c
index fe74f26a54..fcd0484a6d 100644
--- a/arch/mips/mach-mt7620/cpu.c
+++ b/arch/mips/mach-mt7620/cpu.c
@@ -69,28 +69,6 @@ int print_cpuinfo(void)
 	return 0;
 }
 
-#ifdef CONFIG_WATCHDOG
-static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
-
-/* Called by macro WATCHDOG_RESET */
-void watchdog_reset(void)
-{
-	static ulong next_reset;
-	ulong now;
-
-	if (!watchdog_dev)
-		return;
-
-	now = get_timer(0);
-
-	/* Do not reset the watchdog too often */
-	if (now > next_reset) {
-		next_reset = now + 1000;	/* reset every 1000ms */
-		wdt_reset(watchdog_dev);
-	}
-}
-#endif
-
 int arch_misc_init(void)
 {
 	/*
@@ -103,19 +81,5 @@ int arch_misc_init(void)
 	flush_dcache_range(gd->bd->bi_memstart,
 			   gd->bd->bi_memstart + gd->ram_size - 1);
 
-#ifdef CONFIG_WATCHDOG
-	/* Init watchdog */
-	if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
-		debug("Watchdog: Not found by seq!\n");
-		if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
-			puts("Watchdog: Not found!\n");
-			return 0;
-		}
-	}
-
-	wdt_start(watchdog_dev, 60000, 0);	/* 60 seconds */
-	printf("Watchdog: Started\n");
-#endif
-
 	return 0;
 }
diff --git a/board/CZ.NIC/turris_mox/turris_mox.c b/board/CZ.NIC/turris_mox/turris_mox.c
index 96cb9c7e5c..8a4872343b 100644
--- a/board/CZ.NIC/turris_mox/turris_mox.c
+++ b/board/CZ.NIC/turris_mox/turris_mox.c
@@ -119,41 +119,11 @@ int board_fix_fdt(void *blob)
 }
 #endif
 
-#ifdef CONFIG_WDT_ARMADA_37XX
-static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
-
-void watchdog_reset(void)
-{
-	static ulong next_reset;
-	ulong now;
-
-	if (!watchdog_dev)
-		return;
-
-	now = timer_get_us();
-
-	/* Do not reset the watchdog too often */
-	if (now > next_reset) {
-		wdt_reset(watchdog_dev);
-		next_reset = now + 100000;
-	}
-}
-#endif
-
 int board_init(void)
 {
 	/* address of boot parameters */
 	gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;
 
-#ifdef CONFIG_WDT_ARMADA_37XX
-	if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
-		printf("Cannot find Armada 3720 watchdog!\n");
-	} else {
-		printf("Enabling Armada 3720 watchdog (3 minutes timeout).\n");
-		wdt_start(watchdog_dev, 180000, 0);
-	}
-#endif
-
 	return 0;
 }
 
diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c
index c7f6479a0c..8101cfd88a 100644
--- a/board/CZ.NIC/turris_omnia/turris_omnia.c
+++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
@@ -364,25 +364,12 @@ static bool disable_mcu_watchdog(void)
 }
 #endif
 
-#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT_ORION)
-static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
-#endif
-
 int board_init(void)
 {
 	/* adress of boot parameters */
 	gd->bd->bi_boot_params = mvebu_sdram_bar(0) + 0x100;
 
 #ifndef CONFIG_SPL_BUILD
-# ifdef CONFIG_WDT_ORION
-	if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
-		puts("Cannot find Armada 385 watchdog!\n");
-	} else {
-		puts("Enabling Armada 385 watchdog.\n");
-		wdt_start(watchdog_dev, (u32) 25000000 * 120, 0);
-	}
-# endif
-
 	if (disable_mcu_watchdog())
 		puts("Disabled MCU startup watchdog.\n");
 
@@ -392,28 +379,6 @@ int board_init(void)
 	return 0;
 }
 
-#ifdef CONFIG_WATCHDOG
-/* Called by macro WATCHDOG_RESET */
-void watchdog_reset(void)
-{
-# if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT_ORION)
-	static ulong next_reset = 0;
-	ulong now;
-
-	if (!watchdog_dev)
-		return;
-
-	now = timer_get_us();
-
-	/* Do not reset the watchdog too often */
-	if (now > next_reset) {
-		wdt_reset(watchdog_dev);
-		next_reset = now + 1000;
-	}
-# endif
-}
-#endif
-
 int board_late_init(void)
 {
 #ifndef CONFIG_SPL_BUILD
diff --git a/board/xilinx/microblaze-generic/microblaze-generic.c b/board/xilinx/microblaze-generic/microblaze-generic.c
index 28c9efa3a2..ba82292e35 100644
--- a/board/xilinx/microblaze-generic/microblaze-generic.c
+++ b/board/xilinx/microblaze-generic/microblaze-generic.c
@@ -24,10 +24,6 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
-static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
-#endif /* !CONFIG_SPL_BUILD && CONFIG_WDT */
-
 ulong ram_base;
 
 int dram_init_banksize(void)
@@ -43,44 +39,8 @@ int dram_init(void)
 	return 0;
 };
 
-#ifdef CONFIG_WDT
-/* Called by macro WATCHDOG_RESET */
-void watchdog_reset(void)
-{
-#if !defined(CONFIG_SPL_BUILD)
-	ulong now;
-	static ulong next_reset;
-
-	if (!watchdog_dev)
-		return;
-
-	now = timer_get_us();
-
-	/* Do not reset the watchdog too often */
-	if (now > next_reset) {
-		wdt_reset(watchdog_dev);
-		next_reset = now + 1000;
-	}
-#endif /* !CONFIG_SPL_BUILD */
-}
-#endif /* CONFIG_WDT */
-
 int board_late_init(void)
 {
-#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
-	watchdog_dev = NULL;
-
-	if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
-		debug("Watchdog: Not found by seq!\n");
-		if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
-			puts("Watchdog: Not found!\n");
-			return 0;
-		}
-	}
-
-	wdt_start(watchdog_dev, 0, 0);
-	puts("Watchdog: Started\n");
-#endif /* !CONFIG_SPL_BUILD && CONFIG_WDT */
 #if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SYSRESET_MICROBLAZE)
 	int ret;
 
diff --git a/board/xilinx/zynq/board.c b/board/xilinx/zynq/board.c
index ea26aad16f..6857f2c0b8 100644
--- a/board/xilinx/zynq/board.c
+++ b/board/xilinx/zynq/board.c
@@ -18,10 +18,6 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
-static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
-#endif
-
 #if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_BOARD_EARLY_INIT_F)
 int board_early_init_f(void)
 {
@@ -31,19 +27,6 @@ int board_early_init_f(void)
 
 int board_init(void)
 {
-#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
-	if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
-		debug("Watchdog: Not found by seq!\n");
-		if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
-			puts("Watchdog: Not found!\n");
-			return 0;
-		}
-	}
-
-	wdt_start(watchdog_dev, 0, 0);
-	puts("Watchdog: Started\n");
-# endif
-
 	return 0;
 }
 
@@ -127,25 +110,3 @@ int dram_init(void)
 	return 0;
 }
 #endif
-
-#if defined(CONFIG_WATCHDOG)
-/* Called by macro WATCHDOG_RESET */
-void watchdog_reset(void)
-{
-# if !defined(CONFIG_SPL_BUILD)
-	static ulong next_reset;
-	ulong now;
-
-	if (!watchdog_dev)
-		return;
-
-	now = timer_get_us();
-
-	/* Do not reset the watchdog too often */
-	if (now > next_reset) {
-		wdt_reset(watchdog_dev);
-		next_reset = now + 1000;
-	}
-# endif
-}
-#endif
diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
index db27247850..1c12891b82 100644
--- a/board/xilinx/zynqmp/zynqmp.c
+++ b/board/xilinx/zynqmp/zynqmp.c
@@ -24,10 +24,6 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
-static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
-#endif
-
 #if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_ZYNQMPPL) && \
     !defined(CONFIG_SPL_BUILD)
 static xilinx_desc zynqmppl = XILINX_ZYNQMP_DESC;
@@ -340,44 +336,9 @@ int board_init(void)
 	}
 #endif
 
-#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
-	if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
-		debug("Watchdog: Not found by seq!\n");
-		if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
-			puts("Watchdog: Not found!\n");
-			return 0;
-		}
-	}
-
-	wdt_start(watchdog_dev, 0, 0);
-	puts("Watchdog: Started\n");
-#endif
-
 	return 0;
 }
 
-#ifdef CONFIG_WATCHDOG
-/* Called by macro WATCHDOG_RESET */
-void watchdog_reset(void)
-{
-# if !defined(CONFIG_SPL_BUILD)
-	static ulong next_reset;
-	ulong now;
-
-	if (!watchdog_dev)
-		return;
-
-	now = timer_get_us();
-
-	/* Do not reset the watchdog too often */
-	if (now > next_reset) {
-		wdt_reset(watchdog_dev);
-		next_reset = now + 1000;
-	}
-# endif
-}
-#endif
-
 int board_early_init_r(void)
 {
 	u32 val;
diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 23b7e3360d..a9330cba0a 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -10,6 +10,12 @@
 #include <dm/device-internal.h>
 #include <dm/lists.h>
 
+#ifndef WDT_DEFAULT_TIMEOUT
+#define WDT_DEFAULT_TIMEOUT	60
+#endif
+
+DECLARE_GLOBAL_DATA_PTR;
+
 int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
 {
 	const struct wdt_ops *ops = device_get_ops(dev);
@@ -63,6 +69,67 @@ int wdt_expire_now(struct udevice *dev, ulong flags)
 	return ret;
 }
 
+#if defined(CONFIG_WATCHDOG)
+/*
+ * Called by macro WATCHDOG_RESET. This function be called *very* early,
+ * so we need to make sure, that the watchdog driver is ready before using
+ * it in this function.
+ */
+void watchdog_reset(void)
+{
+	static ulong next_reset;
+	ulong now;
+
+	/* Exit if GD is not ready or watchdog is not initialized yet */
+	if (!gd || !(gd->flags & GD_FLG_WDT_READY))
+		return;
+
+	/* Do not reset the watchdog too often */
+	now = get_timer(0);
+	if (now > next_reset) {
+		next_reset = now + 1000;	/* reset every 1000ms */
+		wdt_reset(gd->watchdog_dev);
+	}
+}
+
+static void watchdog_start(void)
+{
+	u32 timeout = WDT_DEFAULT_TIMEOUT;
+
+	/*
+	 * Use only the first watchdog device in U-Boot to trigger the
+	 * watchdog reset
+	 */
+	if (gd->watchdog_dev) {
+		debug("Only one watchdog device used!\n");
+		return;
+	}
+
+	/*
+	 * Init watchdog: This will call the probe function of the
+	 * watchdog driver, enabling the use of the device
+	 */
+	if (uclass_get_device(UCLASS_WDT, 0,
+			      (struct udevice **)&gd->watchdog_dev)) {
+		debug("Watchdog: Not found!\n");
+		return;
+	}
+
+	if (CONFIG_IS_ENABLED(OF_CONTROL)) {
+		timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec",
+					       WDT_DEFAULT_TIMEOUT);
+	}
+
+	wdt_start(gd->watchdog_dev, timeout * 1000, 0);
+	gd->flags |= GD_FLG_WDT_READY;
+	printf("WDT:   Started (%ds timeout)\n", timeout);
+}
+#else
+static void watchdog_start(void)
+{
+}
+#endif
+
 static int wdt_post_bind(struct udevice *dev)
 {
 #if defined(CONFIG_NEEDS_MANUAL_RELOC)
@@ -82,6 +149,13 @@ static int wdt_post_bind(struct udevice *dev)
 		reloc_done++;
 	}
 #endif
+
+	/*
+	 * Start the watchdog service in U-Boot if selected via
+	 * CONFIG_WATCHDOG
+	 */
+	watchdog_start();
+
 	return 0;
 }
 
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index 78dcf40bff..3cb583cbb1 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -133,6 +133,9 @@ typedef struct global_data {
 	struct spl_handoff *spl_handoff;
 # endif
 #endif
+#ifdef CONFIG_WATCHDOG
+	struct udevice *watchdog_dev;
+#endif
 } gd_t;
 #endif
 
@@ -161,5 +164,6 @@ typedef struct global_data {
 #define GD_FLG_ENV_DEFAULT	0x02000 /* Default variable flag	   */
 #define GD_FLG_SPL_EARLY_INIT	0x04000 /* Early SPL init is done	   */
 #define GD_FLG_LOG_READY	0x08000 /* Log system is ready for use	   */
+#define GD_FLG_WDT_READY	0x10000 /* Watchdog is ready for use	   */
 
 #endif /* __ASM_GENERIC_GBL_DATA_H */
-- 
2.21.0

[U-Boot] [PATCH] watchdog: Implement generic watchdog_reset() version

[Stefan Roese]

On 04.04.19 13:23, Stefan Roese wrote:
> This patch tries to implement a generic watchdog_reset() function that
> can be used by all boards that want to service the watchdog device in
> U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG.
> 
> Without this approach, new boards or platforms needed to implement a
> board specific version of this functionality, mostly copy'ing the same
> code over and over again into their board or platforms code base.
> 
> With this new generic function, the scattered other functions are now
> removed to be replaced by the generic one. The new version also enables
> the configuration of the watchdog timeout via the DT "timeout-sec"
> property (if enabled via CONFIG_OF_CONTROL).
> 
> This patch also adds a new flag to the GD flags, to flag that the
> watchdog is ready to use and adds the pointer to the watchdog device
> to the GD. This enables us to remove the global "watchdog_dev"
> variable, which was prone to cause problems because of its potentially
> very early use in watchdog_reset(), even before the BSS is cleared.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Heiko Schocher <hs@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: "Marek Behún" <marek.behun@nic.cz>
> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
> ---
> This patch depends on the following patches:
> 
> [1] watchdog: Move watchdog_dev to data section (BSS may not be cleared)
> https://patchwork.ozlabs.org/patch/1075500/
> 
> [2] watchdog: Handle SPL build with watchdog disabled
> https://patchwork.ozlabs.org/patch/1074098/
> 
> I would like to see [2] applied in this release, since its a real fix on
> some of the platforms with minimal chances of breakage.

Sorry, this should be: "I would like to see [1] applied" (instead
of [2]).

Thanks,
Stefan

[U-Boot] [PATCH] watchdog: Implement generic watchdog_reset() version

[Michal Simek]

On 04. 04. 19 13:23, Stefan Roese wrote:
> This patch tries to implement a generic watchdog_reset() function that
> can be used by all boards that want to service the watchdog device in
> U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG.
> 
> Without this approach, new boards or platforms needed to implement a
> board specific version of this functionality, mostly copy'ing the same
> code over and over again into their board or platforms code base.
> 
> With this new generic function, the scattered other functions are now
> removed to be replaced by the generic one. The new version also enables
> the configuration of the watchdog timeout via the DT "timeout-sec"
> property (if enabled via CONFIG_OF_CONTROL).
> 
> This patch also adds a new flag to the GD flags, to flag that the
> watchdog is ready to use and adds the pointer to the watchdog device
> to the GD. This enables us to remove the global "watchdog_dev"
> variable, which was prone to cause problems because of its potentially
> very early use in watchdog_reset(), even before the BSS is cleared.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Heiko Schocher <hs@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: "Marek Behún" <marek.behun@nic.cz>
> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
> ---
> This patch depends on the following patches:
> 
> [1] watchdog: Move watchdog_dev to data section (BSS may not be cleared)
> https://patchwork.ozlabs.org/patch/1075500/
> 
> [2] watchdog: Handle SPL build with watchdog disabled
> https://patchwork.ozlabs.org/patch/1074098/
> 
> I would like to see [2] applied in this release, since its a real fix on
> some of the platforms with minimal chances of breakage.
> 
> This patch now is a bigger rework and is intended for the next merge
> window. Please review and test if possible. Comments welcome.
> 
> Thanks,
> Stefan
> 
>  arch/mips/mach-mt7620/cpu.c                   | 36 ---------
>  board/CZ.NIC/turris_mox/turris_mox.c          | 30 --------
>  board/CZ.NIC/turris_omnia/turris_omnia.c      | 35 ---------
>  .../microblaze-generic/microblaze-generic.c   | 40 ----------
>  board/xilinx/zynq/board.c                     | 39 ----------
>  board/xilinx/zynqmp/zynqmp.c                  | 39 ----------
>  drivers/watchdog/wdt-uclass.c                 | 74 +++++++++++++++++++
>  include/asm-generic/global_data.h             |  4 +
>  8 files changed, 78 insertions(+), 219 deletions(-)
> 
> diff --git a/arch/mips/mach-mt7620/cpu.c b/arch/mips/mach-mt7620/cpu.c
> index fe74f26a54..fcd0484a6d 100644
> --- a/arch/mips/mach-mt7620/cpu.c
> +++ b/arch/mips/mach-mt7620/cpu.c
> @@ -69,28 +69,6 @@ int print_cpuinfo(void)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_WATCHDOG
> -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> -
> -/* Called by macro WATCHDOG_RESET */
> -void watchdog_reset(void)
> -{
> -	static ulong next_reset;
> -	ulong now;
> -
> -	if (!watchdog_dev)
> -		return;
> -
> -	now = get_timer(0);
> -
> -	/* Do not reset the watchdog too often */
> -	if (now > next_reset) {
> -		next_reset = now + 1000;	/* reset every 1000ms */
> -		wdt_reset(watchdog_dev);
> -	}
> -}
> -#endif
> -
>  int arch_misc_init(void)
>  {
>  	/*
> @@ -103,19 +81,5 @@ int arch_misc_init(void)
>  	flush_dcache_range(gd->bd->bi_memstart,
>  			   gd->bd->bi_memstart + gd->ram_size - 1);
>  
> -#ifdef CONFIG_WATCHDOG
> -	/* Init watchdog */
> -	if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
> -		debug("Watchdog: Not found by seq!\n");
> -		if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
> -			puts("Watchdog: Not found!\n");
> -			return 0;
> -		}
> -	}
> -
> -	wdt_start(watchdog_dev, 60000, 0);	/* 60 seconds */
> -	printf("Watchdog: Started\n");
> -#endif
> -
>  	return 0;
>  }
> diff --git a/board/CZ.NIC/turris_mox/turris_mox.c b/board/CZ.NIC/turris_mox/turris_mox.c
> index 96cb9c7e5c..8a4872343b 100644
> --- a/board/CZ.NIC/turris_mox/turris_mox.c
> +++ b/board/CZ.NIC/turris_mox/turris_mox.c
> @@ -119,41 +119,11 @@ int board_fix_fdt(void *blob)
>  }
>  #endif
>  
> -#ifdef CONFIG_WDT_ARMADA_37XX
> -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> -
> -void watchdog_reset(void)
> -{
> -	static ulong next_reset;
> -	ulong now;
> -
> -	if (!watchdog_dev)
> -		return;
> -
> -	now = timer_get_us();
> -
> -	/* Do not reset the watchdog too often */
> -	if (now > next_reset) {
> -		wdt_reset(watchdog_dev);
> -		next_reset = now + 100000;
> -	}
> -}
> -#endif
> -
>  int board_init(void)
>  {
>  	/* address of boot parameters */
>  	gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;
>  
> -#ifdef CONFIG_WDT_ARMADA_37XX
> -	if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
> -		printf("Cannot find Armada 3720 watchdog!\n");
> -	} else {
> -		printf("Enabling Armada 3720 watchdog (3 minutes timeout).\n");
> -		wdt_start(watchdog_dev, 180000, 0);
> -	}
> -#endif
> -
>  	return 0;
>  }
>  
> diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c
> index c7f6479a0c..8101cfd88a 100644
> --- a/board/CZ.NIC/turris_omnia/turris_omnia.c
> +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
> @@ -364,25 +364,12 @@ static bool disable_mcu_watchdog(void)
>  }
>  #endif
>  
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT_ORION)
> -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> -#endif
> -
>  int board_init(void)
>  {
>  	/* adress of boot parameters */
>  	gd->bd->bi_boot_params = mvebu_sdram_bar(0) + 0x100;
>  
>  #ifndef CONFIG_SPL_BUILD
> -# ifdef CONFIG_WDT_ORION
> -	if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
> -		puts("Cannot find Armada 385 watchdog!\n");
> -	} else {
> -		puts("Enabling Armada 385 watchdog.\n");
> -		wdt_start(watchdog_dev, (u32) 25000000 * 120, 0);
> -	}
> -# endif
> -
>  	if (disable_mcu_watchdog())
>  		puts("Disabled MCU startup watchdog.\n");
>  
> @@ -392,28 +379,6 @@ int board_init(void)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_WATCHDOG
> -/* Called by macro WATCHDOG_RESET */
> -void watchdog_reset(void)
> -{
> -# if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT_ORION)
> -	static ulong next_reset = 0;
> -	ulong now;
> -
> -	if (!watchdog_dev)
> -		return;
> -
> -	now = timer_get_us();
> -
> -	/* Do not reset the watchdog too often */
> -	if (now > next_reset) {
> -		wdt_reset(watchdog_dev);
> -		next_reset = now + 1000;
> -	}
> -# endif
> -}
> -#endif
> -
>  int board_late_init(void)
>  {
>  #ifndef CONFIG_SPL_BUILD
> diff --git a/board/xilinx/microblaze-generic/microblaze-generic.c b/board/xilinx/microblaze-generic/microblaze-generic.c
> index 28c9efa3a2..ba82292e35 100644
> --- a/board/xilinx/microblaze-generic/microblaze-generic.c
> +++ b/board/xilinx/microblaze-generic/microblaze-generic.c
> @@ -24,10 +24,6 @@
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
> -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> -#endif /* !CONFIG_SPL_BUILD && CONFIG_WDT */
> -
>  ulong ram_base;
>  
>  int dram_init_banksize(void)
> @@ -43,44 +39,8 @@ int dram_init(void)
>  	return 0;
>  };
>  
> -#ifdef CONFIG_WDT
> -/* Called by macro WATCHDOG_RESET */
> -void watchdog_reset(void)
> -{
> -#if !defined(CONFIG_SPL_BUILD)
> -	ulong now;
> -	static ulong next_reset;
> -
> -	if (!watchdog_dev)
> -		return;
> -
> -	now = timer_get_us();
> -
> -	/* Do not reset the watchdog too often */
> -	if (now > next_reset) {
> -		wdt_reset(watchdog_dev);
> -		next_reset = now + 1000;
> -	}
> -#endif /* !CONFIG_SPL_BUILD */
> -}
> -#endif /* CONFIG_WDT */
> -
>  int board_late_init(void)
>  {
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
> -	watchdog_dev = NULL;
> -
> -	if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
> -		debug("Watchdog: Not found by seq!\n");
> -		if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
> -			puts("Watchdog: Not found!\n");
> -			return 0;
> -		}
> -	}
> -
> -	wdt_start(watchdog_dev, 0, 0);
> -	puts("Watchdog: Started\n");
> -#endif /* !CONFIG_SPL_BUILD && CONFIG_WDT */
>  #if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SYSRESET_MICROBLAZE)
>  	int ret;
>  
> diff --git a/board/xilinx/zynq/board.c b/board/xilinx/zynq/board.c
> index ea26aad16f..6857f2c0b8 100644
> --- a/board/xilinx/zynq/board.c
> +++ b/board/xilinx/zynq/board.c
> @@ -18,10 +18,6 @@
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
> -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> -#endif
> -
>  #if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_BOARD_EARLY_INIT_F)
>  int board_early_init_f(void)
>  {
> @@ -31,19 +27,6 @@ int board_early_init_f(void)
>  
>  int board_init(void)
>  {
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
> -	if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
> -		debug("Watchdog: Not found by seq!\n");
> -		if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
> -			puts("Watchdog: Not found!\n");
> -			return 0;
> -		}
> -	}
> -
> -	wdt_start(watchdog_dev, 0, 0);
> -	puts("Watchdog: Started\n");
> -# endif
> -
>  	return 0;
>  }
>  
> @@ -127,25 +110,3 @@ int dram_init(void)
>  	return 0;
>  }
>  #endif
> -
> -#if defined(CONFIG_WATCHDOG)
> -/* Called by macro WATCHDOG_RESET */
> -void watchdog_reset(void)
> -{
> -# if !defined(CONFIG_SPL_BUILD)
> -	static ulong next_reset;
> -	ulong now;
> -
> -	if (!watchdog_dev)
> -		return;
> -
> -	now = timer_get_us();
> -
> -	/* Do not reset the watchdog too often */
> -	if (now > next_reset) {
> -		wdt_reset(watchdog_dev);
> -		next_reset = now + 1000;
> -	}
> -# endif
> -}
> -#endif
> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
> index db27247850..1c12891b82 100644
> --- a/board/xilinx/zynqmp/zynqmp.c
> +++ b/board/xilinx/zynqmp/zynqmp.c
> @@ -24,10 +24,6 @@
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
> -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> -#endif
> -
>  #if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_ZYNQMPPL) && \
>      !defined(CONFIG_SPL_BUILD)
>  static xilinx_desc zynqmppl = XILINX_ZYNQMP_DESC;
> @@ -340,44 +336,9 @@ int board_init(void)
>  	}
>  #endif
>  
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
> -	if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
> -		debug("Watchdog: Not found by seq!\n");
> -		if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
> -			puts("Watchdog: Not found!\n");
> -			return 0;
> -		}
> -	}
> -
> -	wdt_start(watchdog_dev, 0, 0);
> -	puts("Watchdog: Started\n");
> -#endif
> -
>  	return 0;
>  }
>  
> -#ifdef CONFIG_WATCHDOG
> -/* Called by macro WATCHDOG_RESET */
> -void watchdog_reset(void)
> -{
> -# if !defined(CONFIG_SPL_BUILD)
> -	static ulong next_reset;
> -	ulong now;
> -
> -	if (!watchdog_dev)
> -		return;
> -
> -	now = timer_get_us();
> -
> -	/* Do not reset the watchdog too often */
> -	if (now > next_reset) {
> -		wdt_reset(watchdog_dev);
> -		next_reset = now + 1000;
> -	}
> -# endif
> -}
> -#endif
> -
>  int board_early_init_r(void)
>  {
>  	u32 val;
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 23b7e3360d..a9330cba0a 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -10,6 +10,12 @@
>  #include <dm/device-internal.h>
>  #include <dm/lists.h>
>  
> +#ifndef WDT_DEFAULT_TIMEOUT
> +#define WDT_DEFAULT_TIMEOUT	60
> +#endif
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
>  int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
>  {
>  	const struct wdt_ops *ops = device_get_ops(dev);
> @@ -63,6 +69,67 @@ int wdt_expire_now(struct udevice *dev, ulong flags)
>  	return ret;
>  }
>  
> +#if defined(CONFIG_WATCHDOG)

This is not enough because you can enable it just for full u-boot but
not for SPL. It means you should also handle CONFIG_SPL_WATCHDOG_SUPPORT
macros.

Just try to enable CADENCE_WDT, WDT and Watchdog for zcu100 and build


> +/*
> + * Called by macro WATCHDOG_RESET. This function be called *very* early,
> + * so we need to make sure, that the watchdog driver is ready before using
> + * it in this function.
> + */
> +void watchdog_reset(void)
> +{
> +	static ulong next_reset;
> +	ulong now;
> +
> +	/* Exit if GD is not ready or watchdog is not initialized yet */
> +	if (!gd || !(gd->flags & GD_FLG_WDT_READY))
> +		return;
> +
> +	/* Do not reset the watchdog too often */
> +	now = get_timer(0);
> +	if (now > next_reset) {
> +		next_reset = now + 1000;	/* reset every 1000ms */
> +		wdt_reset(gd->watchdog_dev);
> +	}
> +}
> +
> +static void watchdog_start(void)
> +{
> +	u32 timeout = WDT_DEFAULT_TIMEOUT;
> +
> +	/*
> +	 * Use only the first watchdog device in U-Boot to trigger the
> +	 * watchdog reset
> +	 */
> +	if (gd->watchdog_dev) {

I hope that gd structure is cleared somewhere.

> +		debug("Only one watchdog device used!\n");
> +		return;
> +	}
> +
> +	/*
> +	 * Init watchdog: This will call the probe function of the
> +	 * watchdog driver, enabling the use of the device
> +	 */
> +	if (uclass_get_device(UCLASS_WDT, 0,
> +			      (struct udevice **)&gd->watchdog_dev)) {
> +		debug("Watchdog: Not found!\n");
> +		return;
> +	}

I think you should c&p what I have done in zynq/zynqmp. It means check
watchdog0 alias first because this is supposed to be the first primary
watchdog.

zynq/zynqmp/mb code
       if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
               debug("Watchdog: Not found by seq!\n");
               if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
                       puts("Watchdog: Not found!\n");
                       return 0;
               }
       }



> +
> +	if (CONFIG_IS_ENABLED(OF_CONTROL)) {
> +		timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec",
> +					       WDT_DEFAULT_TIMEOUT);
> +	}

You should remove them from drivers.

drivers/watchdog/at91sam9_wdt.c:119:    priv->timeout =
dev_read_u32_default(dev, "timeout-sec",
drivers/watchdog/cdns_wdt.c:238:        priv->timeout =
dev_read_u32_default(dev, "timeout-sec",


> +
> +	wdt_start(gd->watchdog_dev, timeout * 1000, 0);
> +	gd->flags |= GD_FLG_WDT_READY;
> +	printf("WDT:   Started (%ds timeout)\n", timeout);
> +}
> +#else
> +static void watchdog_start(void)
> +{
> +}
> +#endif
> +
>  static int wdt_post_bind(struct udevice *dev)
>  {
>  #if defined(CONFIG_NEEDS_MANUAL_RELOC)
> @@ -82,6 +149,13 @@ static int wdt_post_bind(struct udevice *dev)
>  		reloc_done++;
>  	}
>  #endif
> +
> +	/*
> +	 * Start the watchdog service in U-Boot if selected via
> +	 * CONFIG_WATCHDOG
> +	 */
> +	watchdog_start();
> +
>  	return 0;
>  }
>  
> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> index 78dcf40bff..3cb583cbb1 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -133,6 +133,9 @@ typedef struct global_data {
>  	struct spl_handoff *spl_handoff;
>  # endif
>  #endif
> +#ifdef CONFIG_WATCHDOG
> +	struct udevice *watchdog_dev;
> +#endif
>  } gd_t;
>  #endif
>  
> @@ -161,5 +164,6 @@ typedef struct global_data {
>  #define GD_FLG_ENV_DEFAULT	0x02000 /* Default variable flag	   */
>  #define GD_FLG_SPL_EARLY_INIT	0x04000 /* Early SPL init is done	   */
>  #define GD_FLG_LOG_READY	0x08000 /* Log system is ready for use	   */
> +#define GD_FLG_WDT_READY	0x10000 /* Watchdog is ready for use	   */
>  
>  #endif /* __ASM_GENERIC_GBL_DATA_H */
> 

Thanks,
Michal

[U-Boot] [PATCH] watchdog: Implement generic watchdog_reset() version

[Stefan Roese]

Hi Michal,

On 05.04.19 14:07, Michal Simek wrote:

<snip>

>> +++ b/drivers/watchdog/wdt-uclass.c
>> @@ -10,6 +10,12 @@
>>   #include <dm/device-internal.h>
>>   #include <dm/lists.h>
>>   
>> +#ifndef WDT_DEFAULT_TIMEOUT
>> +#define WDT_DEFAULT_TIMEOUT	60
>> +#endif
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>>   int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
>>   {
>>   	const struct wdt_ops *ops = device_get_ops(dev);
>> @@ -63,6 +69,67 @@ int wdt_expire_now(struct udevice *dev, ulong flags)
>>   	return ret;
>>   }
>>   
>> +#if defined(CONFIG_WATCHDOG)
> 
> This is not enough because you can enable it just for full u-boot but
> not for SPL.

Why not? CONFIG_WATCHDOG can be enabled with or without
CONFIG_SPL_WATCHDOG_SUPPORT being enabled.

> It means you should also handle CONFIG_SPL_WATCHDOG_SUPPORT
> macros.
> 
> Just try to enable CADENCE_WDT, WDT and Watchdog for zcu100 and build

I just tried this for xilinx_zynqmp_zcu100_revC_defconfig. Per default
it has enabled:

$ grep 'WATCHDOG\|WDT' .config
# CONFIG_SPL_WATCHDOG_SUPPORT is not set
# CONFIG_SYSRESET_WATCHDOG is not set
CONFIG_WATCHDOG=y
# CONFIG_WATCHDOG_RESET_DISABLE is not set
# CONFIG_BCM2835_WDT is not set
# CONFIG_ULP_WATCHDOG is not set
CONFIG_WDT=y
# CONFIG_WDT_ASPEED is not set
# CONFIG_WDT_ORION is not set
CONFIG_WDT_CDNS=y
# CONFIG_XILINX_TB_WATCHDOG is not set
# CONFIG_IMX_WATCHDOG is not set
# CONFIG_WDT_AT91 is not set

I can also enable SPL_WATCHDOG_SUPPORT and it compiles without issues:

$ grep 'WATCHDOG\|WDT' .config
CONFIG_SPL_WATCHDOG_SUPPORT=y
# CONFIG_SYSRESET_WATCHDOG is not set
CONFIG_WATCHDOG=y
# CONFIG_WATCHDOG_RESET_DISABLE is not set
# CONFIG_BCM2835_WDT is not set
# CONFIG_ULP_WATCHDOG is not set
CONFIG_WDT=y
# CONFIG_WDT_ASPEED is not set
# CONFIG_WDT_ORION is not set
CONFIG_WDT_CDNS=y
# CONFIG_XILINX_TB_WATCHDOG is not set
# CONFIG_IMX_WATCHDOG is not set
# CONFIG_WDT_AT91 is not set

What am I missing?

>> +/*
>> + * Called by macro WATCHDOG_RESET. This function be called *very* early,
>> + * so we need to make sure, that the watchdog driver is ready before using
>> + * it in this function.
>> + */
>> +void watchdog_reset(void)
>> +{
>> +	static ulong next_reset;
>> +	ulong now;
>> +
>> +	/* Exit if GD is not ready or watchdog is not initialized yet */
>> +	if (!gd || !(gd->flags & GD_FLG_WDT_READY))
>> +		return;
>> +
>> +	/* Do not reset the watchdog too often */
>> +	now = get_timer(0);
>> +	if (now > next_reset) {
>> +		next_reset = now + 1000;	/* reset every 1000ms */
>> +		wdt_reset(gd->watchdog_dev);
>> +	}
>> +}
>> +
>> +static void watchdog_start(void)
>> +{
>> +	u32 timeout = WDT_DEFAULT_TIMEOUT;
>> +
>> +	/*
>> +	 * Use only the first watchdog device in U-Boot to trigger the
>> +	 * watchdog reset
>> +	 */
>> +	if (gd->watchdog_dev) {
> 
> I hope that gd structure is cleared somewhere.

I had this also in mind with respect to BSS not being cleared very
early. Its cleared in board_init_f_init_reserve(), so quite early
before board_init_f() is called.
  
>> +		debug("Only one watchdog device used!\n");
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * Init watchdog: This will call the probe function of the
>> +	 * watchdog driver, enabling the use of the device
>> +	 */
>> +	if (uclass_get_device(UCLASS_WDT, 0,
>> +			      (struct udevice **)&gd->watchdog_dev)) {
>> +		debug("Watchdog: Not found!\n");
>> +		return;
>> +	}
> 
> I think you should c&p what I have done in zynq/zynqmp. It means check
> watchdog0 alias first because this is supposed to be the first primary
> watchdog.
> 
> zynq/zynqmp/mb code
>         if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
>                 debug("Watchdog: Not found by seq!\n");
>                 if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
>                         puts("Watchdog: Not found!\n");
>                         return 0;
>                 }
>         }

In my (internal) first version, I had exactly this code included. I
removed the first part, as it makes no real sense to call it at this
stage (wdt_post_bind() for the first WDT found in the system). My
understanding is, that wdt_post_bind() will get called for all WDT
found in the system. My current logic here is, to only register the
first WDT found to the global watchdog_dev in GD. To prevent that
multiple WDT are used to service the U-Boot watchdog via WATCHDOG_RESET.

Or does the DM subsystem automatically bind the watchdog0 alias first
in its binding sequence?

I do not have a system with multiple watchdogs here, so its not that
easy to test this. I could simulate this though somehow...
  
> 
> 
>> +
>> +	if (CONFIG_IS_ENABLED(OF_CONTROL)) {
>> +		timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec",
>> +					       WDT_DEFAULT_TIMEOUT);
>> +	}
> 
> You should remove them from drivers.
> 
> drivers/watchdog/at91sam9_wdt.c:119:    priv->timeout =
> dev_read_u32_default(dev, "timeout-sec",
> drivers/watchdog/cdns_wdt.c:238:        priv->timeout =
> dev_read_u32_default(dev, "timeout-sec",

Yes, its on my plan. I've also some additions for the AT91 driver on
the list since a few days, which will most likely get applied before
this patch. I'll send a cleanup patch to remove those now superfluous
DT handling later.

Thanks,
Stefan

[U-Boot] [PATCH] watchdog: Implement generic watchdog_reset() version

[Stefan Roese]

On 05.04.19 15:00, Stefan Roese wrote:

<snip>

>>> +		debug("Only one watchdog device used!\n");
>>> +		return;
>>> +	}
>>> +
>>> +	/*
>>> +	 * Init watchdog: This will call the probe function of the
>>> +	 * watchdog driver, enabling the use of the device
>>> +	 */
>>> +	if (uclass_get_device(UCLASS_WDT, 0,
>>> +			      (struct udevice **)&gd->watchdog_dev)) {
>>> +		debug("Watchdog: Not found!\n");
>>> +		return;
>>> +	}
>>
>> I think you should c&p what I have done in zynq/zynqmp. It means check
>> watchdog0 alias first because this is supposed to be the first primary
>> watchdog.
>>
>> zynq/zynqmp/mb code
>>          if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
>>                  debug("Watchdog: Not found by seq!\n");
>>                  if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
>>                          puts("Watchdog: Not found!\n");
>>                          return 0;
>>                  }
>>          }
> 
> In my (internal) first version, I had exactly this code included. I
> removed the first part, as it makes no real sense to call it at this
> stage (wdt_post_bind() for the first WDT found in the system). My
> understanding is, that wdt_post_bind() will get called for all WDT
> found in the system. My current logic here is, to only register the
> first WDT found to the global watchdog_dev in GD. To prevent that
> multiple WDT are used to service the U-Boot watchdog via WATCHDOG_RESET.
> 
> Or does the DM subsystem automatically bind the watchdog0 alias first
> in its binding sequence?
> 
> I do not have a system with multiple watchdogs here, so its not that
> easy to test this. I could simulate this though somehow...

I just did some testing on this. One solution might be, to only use
uclass_get_device_by_seq() (instead of uclass_get_device) and return
if the alias is not found. This way, all watchdog devices will get
probed (bound) and only the one referenced by the alias will get used.

This means on the other hand, that an watchdog0 alias is needed in the
DT to enable the WATCHDOG_RESET U-Boot servicing, which might be not
backward compatible.

Do you have any other thought on this?

Thanks,
Stefan

[U-Boot] [PATCH] watchdog: Implement generic watchdog_reset() version

[Michal Simek]

On 05. 04. 19 15:00, Stefan Roese wrote:
> Hi Michal,
> 
> On 05.04.19 14:07, Michal Simek wrote:
> 
> <snip>
> 
>>> +++ b/drivers/watchdog/wdt-uclass.c
>>> @@ -10,6 +10,12 @@
>>>   #include <dm/device-internal.h>
>>>   #include <dm/lists.h>
>>>   +#ifndef WDT_DEFAULT_TIMEOUT
>>> +#define WDT_DEFAULT_TIMEOUT    60
>>> +#endif
>>> +
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>>   int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
>>>   {
>>>       const struct wdt_ops *ops = device_get_ops(dev);
>>> @@ -63,6 +69,67 @@ int wdt_expire_now(struct udevice *dev, ulong flags)
>>>       return ret;
>>>   }
>>>   +#if defined(CONFIG_WATCHDOG)
>>
>> This is not enough because you can enable it just for full u-boot but
>> not for SPL.
> 
> Why not? CONFIG_WATCHDOG can be enabled with or without
> CONFIG_SPL_WATCHDOG_SUPPORT being enabled.
> 
>> It means you should also handle CONFIG_SPL_WATCHDOG_SUPPORT
>> macros.
>>
>> Just try to enable CADENCE_WDT, WDT and Watchdog for zcu100 and build
> 
> I just tried this for xilinx_zynqmp_zcu100_revC_defconfig. Per default
> it has enabled:

ah right. I have patch in my queue to disable it by default.

Anyway this is what I am getting.

[u-boot](mainline)$ git log -n 3 --oneline
672eafbf08a0 watchdog: Implement generic watchdog_reset() version
7b5b40e4858b watchdog: Move watchdog_dev to data section (BSS may not be
cleared)
0e708abcbba1 Merge branch 'master' of git://git.denx.de/u-boot-usb
[u-boot](mainline)$ make xilinx_zynqmp_zcu100_revC_defconfig >/dev/null
&& make -j >/dev/null
lib/built-in.o: In function `inflateReset':
/mnt/disk/u-boot/lib/zlib/inflate.c:28: undefined reference to
`watchdog_reset'
/mnt/disk/u-boot/lib/zlib/inflate.c:28:(.text.inflateReset+0x58):
relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
`watchdog_reset'
lib/built-in.o: In function `inflateEnd':
/mnt/disk/u-boot/lib/zlib/inflate.c:936: undefined reference to
`watchdog_reset'
/mnt/disk/u-boot/lib/zlib/inflate.c:936:(.text.inflateEnd+0x2c):
relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
`watchdog_reset'
lib/built-in.o: In function `inflate':
/mnt/disk/u-boot/lib/zlib/inflate.c:546: undefined reference to
`watchdog_reset'
/mnt/disk/u-boot/lib/zlib/inflate.c:546:(.text.inflate+0x7b4):
relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
`watchdog_reset'
/mnt/disk/u-boot/lib/zlib/inflate.c:724: undefined reference to
`watchdog_reset'
/mnt/disk/u-boot/lib/zlib/inflate.c:724:(.text.inflate+0xcc4):
relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
`watchdog_reset'
lib/built-in.o: In function `udelay':
/mnt/disk/u-boot/lib/time.c:167: undefined reference to `watchdog_reset'
/mnt/disk/u-boot/lib/time.c:167:(.text.udelay+0x1c): relocation
truncated to fit: R_AARCH64_CALL26 against undefined symbol `watchdog_reset'
drivers/built-in.o:/mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:
more undefined references to `watchdog_reset' follow
drivers/built-in.o: In function `_debug_uart_putc':
/mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:(.text.printch+0x4c):
relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
`watchdog_reset'
/mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:(.text.printch+0x54):
relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
`watchdog_reset'
/mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:(.text.printascii+0x58):
relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
`watchdog_reset'
/mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:(.text.printascii+0x60):
relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
`watchdog_reset'
make[1]: *** [spl/u-boot-spl] Error 1
make: *** [spl/u-boot-spl] Error 2


snip

>>> +static void watchdog_start(void)
>>> +{
>>> +    u32 timeout = WDT_DEFAULT_TIMEOUT;
>>> +
>>> +    /*
>>> +     * Use only the first watchdog device in U-Boot to trigger the
>>> +     * watchdog reset
>>> +     */
>>> +    if (gd->watchdog_dev) {
>>
>> I hope that gd structure is cleared somewhere.
> 
> I had this also in mind with respect to BSS not being cleared very
> early. Its cleared in board_init_f_init_reserve(), so quite early
> before board_init_f() is called.


good.

>  
>>> +        debug("Only one watchdog device used!\n");
>>> +        return;
>>> +    }
>>> +
>>> +    /*
>>> +     * Init watchdog: This will call the probe function of the
>>> +     * watchdog driver, enabling the use of the device
>>> +     */
>>> +    if (uclass_get_device(UCLASS_WDT, 0,
>>> +                  (struct udevice **)&gd->watchdog_dev)) {
>>> +        debug("Watchdog: Not found!\n");
>>> +        return;
>>> +    }
>>
>> I think you should c&p what I have done in zynq/zynqmp. It means check
>> watchdog0 alias first because this is supposed to be the first primary
>> watchdog.
>>
>> zynq/zynqmp/mb code
>>         if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
>>                 debug("Watchdog: Not found by seq!\n");
>>                 if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
>>                         puts("Watchdog: Not found!\n");
>>                         return 0;
>>                 }
>>         }
> 
> In my (internal) first version, I had exactly this code included. I
> removed the first part, as it makes no real sense to call it at this
> stage (wdt_post_bind() for the first WDT found in the system). My
> understanding is, that wdt_post_bind() will get called for all WDT
> found in the system. My current logic here is, to only register the
> first WDT found to the global watchdog_dev in GD. To prevent that
> multiple WDT are used to service the U-Boot watchdog via WATCHDOG_RESET.
> 
> Or does the DM subsystem automatically bind the watchdog0 alias first
> in its binding sequence?
> 
> I do not have a system with multiple watchdogs here, so its not that
> easy to test this. I could simulate this though somehow...

IIRC current code will gives you the first found watchdog which doesn't
need to be that one you want to use here. On DT based platform it is the
first watchdog found from top and there is no way around.

On Zynq,ZynqMP and Microblaze you have have multiple watchdogs, PS or PL
based and you need to have functionality to select which was should be
used.

  
>>
>>
>>> +
>>> +    if (CONFIG_IS_ENABLED(OF_CONTROL)) {
>>> +        timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec",
>>> +                           WDT_DEFAULT_TIMEOUT);
>>> +    }
>>
>> You should remove them from drivers.
>>
>> drivers/watchdog/at91sam9_wdt.c:119:    priv->timeout =
>> dev_read_u32_default(dev, "timeout-sec",
>> drivers/watchdog/cdns_wdt.c:238:        priv->timeout =
>> dev_read_u32_default(dev, "timeout-sec",
> 
> Yes, its on my plan. I've also some additions for the AT91 driver on
> the list since a few days, which will most likely get applied before
> this patch. I'll send a cleanup patch to remove those now superfluous
> DT handling later.

I think it will be good to add timeout-sec to core and removing it from
drivers as one patch separated from this change.

Thanks,
Michal

[U-Boot] [PATCH] watchdog: Implement generic watchdog_reset() version

[Stefan Roese]

On 05.04.19 17:18, Michal Simek wrote:
> On 05. 04. 19 15:00, Stefan Roese wrote:
>> Hi Michal,
>>
>> On 05.04.19 14:07, Michal Simek wrote:
>>
>> <snip>
>>
>>>> +++ b/drivers/watchdog/wdt-uclass.c
>>>> @@ -10,6 +10,12 @@
>>>>    #include <dm/device-internal.h>
>>>>    #include <dm/lists.h>
>>>>    +#ifndef WDT_DEFAULT_TIMEOUT
>>>> +#define WDT_DEFAULT_TIMEOUT    60
>>>> +#endif
>>>> +
>>>> +DECLARE_GLOBAL_DATA_PTR;
>>>> +
>>>>    int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
>>>>    {
>>>>        const struct wdt_ops *ops = device_get_ops(dev);
>>>> @@ -63,6 +69,67 @@ int wdt_expire_now(struct udevice *dev, ulong flags)
>>>>        return ret;
>>>>    }
>>>>    +#if defined(CONFIG_WATCHDOG)
>>>
>>> This is not enough because you can enable it just for full u-boot but
>>> not for SPL.
>>
>> Why not? CONFIG_WATCHDOG can be enabled with or without
>> CONFIG_SPL_WATCHDOG_SUPPORT being enabled.
>>
>>> It means you should also handle CONFIG_SPL_WATCHDOG_SUPPORT
>>> macros.
>>>
>>> Just try to enable CADENCE_WDT, WDT and Watchdog for zcu100 and build
>>
>> I just tried this for xilinx_zynqmp_zcu100_revC_defconfig. Per default
>> it has enabled:
> 
> ah right. I have patch in my queue to disable it by default.
> 
> Anyway this is what I am getting.
> 
> [u-boot](mainline)$ git log -n 3 --oneline
> 672eafbf08a0 watchdog: Implement generic watchdog_reset() version
> 7b5b40e4858b watchdog: Move watchdog_dev to data section (BSS may not be
> cleared)
> 0e708abcbba1 Merge branch 'master' of git://git.denx.de/u-boot-usb
> [u-boot](mainline)$ make xilinx_zynqmp_zcu100_revC_defconfig >/dev/null
> && make -j >/dev/null
> lib/built-in.o: In function `inflateReset':
> /mnt/disk/u-boot/lib/zlib/inflate.c:28: undefined reference to
> `watchdog_reset'
> /mnt/disk/u-boot/lib/zlib/inflate.c:28:(.text.inflateReset+0x58):
> relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
> `watchdog_reset'
> lib/built-in.o: In function `inflateEnd':
> /mnt/disk/u-boot/lib/zlib/inflate.c:936: undefined reference to
> `watchdog_reset'
> /mnt/disk/u-boot/lib/zlib/inflate.c:936:(.text.inflateEnd+0x2c):
> relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
> `watchdog_reset'
> lib/built-in.o: In function `inflate':
> /mnt/disk/u-boot/lib/zlib/inflate.c:546: undefined reference to
> `watchdog_reset'
> /mnt/disk/u-boot/lib/zlib/inflate.c:546:(.text.inflate+0x7b4):
> relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
> `watchdog_reset'
> /mnt/disk/u-boot/lib/zlib/inflate.c:724: undefined reference to
> `watchdog_reset'
> /mnt/disk/u-boot/lib/zlib/inflate.c:724:(.text.inflate+0xcc4):
> relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
> `watchdog_reset'
> lib/built-in.o: In function `udelay':
> /mnt/disk/u-boot/lib/time.c:167: undefined reference to `watchdog_reset'
> /mnt/disk/u-boot/lib/time.c:167:(.text.udelay+0x1c): relocation
> truncated to fit: R_AARCH64_CALL26 against undefined symbol `watchdog_reset'
> drivers/built-in.o:/mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:
> more undefined references to `watchdog_reset' follow
> drivers/built-in.o: In function `_debug_uart_putc':
> /mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:(.text.printch+0x4c):
> relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
> `watchdog_reset'
> /mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:(.text.printch+0x54):
> relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
> `watchdog_reset'
> /mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:(.text.printascii+0x58):
> relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
> `watchdog_reset'
> /mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:(.text.printascii+0x60):
> relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
> `watchdog_reset'
> make[1]: *** [spl/u-boot-spl] Error 1
> make: *** [spl/u-boot-spl] Error 2

Yes. You're probably missing this patch here, which I listed as
dependencies in the commit text:

[2] watchdog: Handle SPL build with watchdog disabled
https://patchwork.ozlabs.org/patch/1074098/

Please test again with this patch applied.
  
> 
> snip
> 
>>>> +static void watchdog_start(void)
>>>> +{
>>>> +    u32 timeout = WDT_DEFAULT_TIMEOUT;
>>>> +
>>>> +    /*
>>>> +     * Use only the first watchdog device in U-Boot to trigger the
>>>> +     * watchdog reset
>>>> +     */
>>>> +    if (gd->watchdog_dev) {
>>>
>>> I hope that gd structure is cleared somewhere.
>>
>> I had this also in mind with respect to BSS not being cleared very
>> early. Its cleared in board_init_f_init_reserve(), so quite early
>> before board_init_f() is called.
> 
> 
> good.
> 
>>   
>>>> +        debug("Only one watchdog device used!\n");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * Init watchdog: This will call the probe function of the
>>>> +     * watchdog driver, enabling the use of the device
>>>> +     */
>>>> +    if (uclass_get_device(UCLASS_WDT, 0,
>>>> +                  (struct udevice **)&gd->watchdog_dev)) {
>>>> +        debug("Watchdog: Not found!\n");
>>>> +        return;
>>>> +    }
>>>
>>> I think you should c&p what I have done in zynq/zynqmp. It means check
>>> watchdog0 alias first because this is supposed to be the first primary
>>> watchdog.
>>>
>>> zynq/zynqmp/mb code
>>>          if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
>>>                  debug("Watchdog: Not found by seq!\n");
>>>                  if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
>>>                          puts("Watchdog: Not found!\n");
>>>                          return 0;
>>>                  }
>>>          }
>>
>> In my (internal) first version, I had exactly this code included. I
>> removed the first part, as it makes no real sense to call it at this
>> stage (wdt_post_bind() for the first WDT found in the system). My
>> understanding is, that wdt_post_bind() will get called for all WDT
>> found in the system. My current logic here is, to only register the
>> first WDT found to the global watchdog_dev in GD. To prevent that
>> multiple WDT are used to service the U-Boot watchdog via WATCHDOG_RESET.
>>
>> Or does the DM subsystem automatically bind the watchdog0 alias first
>> in its binding sequence?
>>
>> I do not have a system with multiple watchdogs here, so its not that
>> easy to test this. I could simulate this though somehow...
> 
> IIRC current code will gives you the first found watchdog which doesn't
> need to be that one you want to use here. On DT based platform it is the
> first watchdog found from top and there is no way around.
> 
> On Zynq,ZynqMP and Microblaze you have have multiple watchdogs, PS or PL
> based and you need to have functionality to select which was should be
> used.

I see. I still need to find a good solution for this. Best would be a
call into this uclass driver once all driver binding calls are finished.
Not sure if this is easy to accomplish though (I need to look deeper
still).

> 
>    
>>>
>>>
>>>> +
>>>> +    if (CONFIG_IS_ENABLED(OF_CONTROL)) {
>>>> +        timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec",
>>>> +                           WDT_DEFAULT_TIMEOUT);
>>>> +    }
>>>
>>> You should remove them from drivers.
>>>
>>> drivers/watchdog/at91sam9_wdt.c:119:    priv->timeout =
>>> dev_read_u32_default(dev, "timeout-sec",
>>> drivers/watchdog/cdns_wdt.c:238:        priv->timeout =
>>> dev_read_u32_default(dev, "timeout-sec",
>>
>> Yes, its on my plan. I've also some additions for the AT91 driver on
>> the list since a few days, which will most likely get applied before
>> this patch. I'll send a cleanup patch to remove those now superfluous
>> DT handling later.
> 
> I think it will be good to add timeout-sec to core and removing it from
> drivers as one patch separated from this change.

I can remove these driver specific implementations in v2 if you
prefer this. No problem.

Thanks,
Stefan

[U-Boot] [PATCH] watchdog: Implement generic watchdog_reset() version

[Michal Simek]

On 05. 04. 19 17:23, Stefan Roese wrote:
> On 05.04.19 17:18, Michal Simek wrote:
>> On 05. 04. 19 15:00, Stefan Roese wrote:
>>> Hi Michal,
>>>
>>> On 05.04.19 14:07, Michal Simek wrote:
>>>
>>> <snip>
>>>
>>>>> +++ b/drivers/watchdog/wdt-uclass.c
>>>>> @@ -10,6 +10,12 @@
>>>>>    #include <dm/device-internal.h>
>>>>>    #include <dm/lists.h>
>>>>>    +#ifndef WDT_DEFAULT_TIMEOUT
>>>>> +#define WDT_DEFAULT_TIMEOUT    60
>>>>> +#endif
>>>>> +
>>>>> +DECLARE_GLOBAL_DATA_PTR;
>>>>> +
>>>>>    int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
>>>>>    {
>>>>>        const struct wdt_ops *ops = device_get_ops(dev);
>>>>> @@ -63,6 +69,67 @@ int wdt_expire_now(struct udevice *dev, ulong
>>>>> flags)
>>>>>        return ret;
>>>>>    }
>>>>>    +#if defined(CONFIG_WATCHDOG)
>>>>
>>>> This is not enough because you can enable it just for full u-boot but
>>>> not for SPL.
>>>
>>> Why not? CONFIG_WATCHDOG can be enabled with or without
>>> CONFIG_SPL_WATCHDOG_SUPPORT being enabled.
>>>
>>>> It means you should also handle CONFIG_SPL_WATCHDOG_SUPPORT
>>>> macros.
>>>>
>>>> Just try to enable CADENCE_WDT, WDT and Watchdog for zcu100 and build
>>>
>>> I just tried this for xilinx_zynqmp_zcu100_revC_defconfig. Per default
>>> it has enabled:
>>
>> ah right. I have patch in my queue to disable it by default.
>>
>> Anyway this is what I am getting.
>>
>> [u-boot](mainline)$ git log -n 3 --oneline
>> 672eafbf08a0 watchdog: Implement generic watchdog_reset() version
>> 7b5b40e4858b watchdog: Move watchdog_dev to data section (BSS may not be
>> cleared)
>> 0e708abcbba1 Merge branch 'master' of git://git.denx.de/u-boot-usb
>> [u-boot](mainline)$ make xilinx_zynqmp_zcu100_revC_defconfig >/dev/null
>> && make -j >/dev/null
>> lib/built-in.o: In function `inflateReset':
>> /mnt/disk/u-boot/lib/zlib/inflate.c:28: undefined reference to
>> `watchdog_reset'
>> /mnt/disk/u-boot/lib/zlib/inflate.c:28:(.text.inflateReset+0x58):
>> relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
>> `watchdog_reset'
>> lib/built-in.o: In function `inflateEnd':
>> /mnt/disk/u-boot/lib/zlib/inflate.c:936: undefined reference to
>> `watchdog_reset'
>> /mnt/disk/u-boot/lib/zlib/inflate.c:936:(.text.inflateEnd+0x2c):
>> relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
>> `watchdog_reset'
>> lib/built-in.o: In function `inflate':
>> /mnt/disk/u-boot/lib/zlib/inflate.c:546: undefined reference to
>> `watchdog_reset'
>> /mnt/disk/u-boot/lib/zlib/inflate.c:546:(.text.inflate+0x7b4):
>> relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
>> `watchdog_reset'
>> /mnt/disk/u-boot/lib/zlib/inflate.c:724: undefined reference to
>> `watchdog_reset'
>> /mnt/disk/u-boot/lib/zlib/inflate.c:724:(.text.inflate+0xcc4):
>> relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
>> `watchdog_reset'
>> lib/built-in.o: In function `udelay':
>> /mnt/disk/u-boot/lib/time.c:167: undefined reference to `watchdog_reset'
>> /mnt/disk/u-boot/lib/time.c:167:(.text.udelay+0x1c): relocation
>> truncated to fit: R_AARCH64_CALL26 against undefined symbol
>> `watchdog_reset'
>> drivers/built-in.o:/mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:
>> more undefined references to `watchdog_reset' follow
>> drivers/built-in.o: In function `_debug_uart_putc':
>> /mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:(.text.printch+0x4c):
>> relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
>> `watchdog_reset'
>> /mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:(.text.printch+0x54):
>> relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
>> `watchdog_reset'
>> /mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:(.text.printascii+0x58):
>>
>> relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
>> `watchdog_reset'
>> /mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:(.text.printascii+0x60):
>>
>> relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
>> `watchdog_reset'
>> make[1]: *** [spl/u-boot-spl] Error 1
>> make: *** [spl/u-boot-spl] Error 2
> 
> Yes. You're probably missing this patch here, which I listed as
> dependencies in the commit text:
> 
> [2] watchdog: Handle SPL build with watchdog disabled
> https://patchwork.ozlabs.org/patch/1074098/
> 
> Please test again with this patch applied.

Good catch. I didn't read it fully. Thought that only that first one you
sent recently is that dependency.

M

[U-Boot] [PATCH] watchdog: Implement generic watchdog_reset() version

[Michal Simek]

On 05. 04. 19 15:24, Stefan Roese wrote:
> On 05.04.19 15:00, Stefan Roese wrote:
> 
> <snip>
> 
>>>> +        debug("Only one watchdog device used!\n");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * Init watchdog: This will call the probe function of the
>>>> +     * watchdog driver, enabling the use of the device
>>>> +     */
>>>> +    if (uclass_get_device(UCLASS_WDT, 0,
>>>> +                  (struct udevice **)&gd->watchdog_dev)) {
>>>> +        debug("Watchdog: Not found!\n");
>>>> +        return;
>>>> +    }
>>>
>>> I think you should c&p what I have done in zynq/zynqmp. It means check
>>> watchdog0 alias first because this is supposed to be the first primary
>>> watchdog.
>>>
>>> zynq/zynqmp/mb code
>>>          if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
>>>                  debug("Watchdog: Not found by seq!\n");
>>>                  if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
>>>                          puts("Watchdog: Not found!\n");
>>>                          return 0;
>>>                  }
>>>          }
>>
>> In my (internal) first version, I had exactly this code included. I
>> removed the first part, as it makes no real sense to call it at this
>> stage (wdt_post_bind() for the first WDT found in the system). My
>> understanding is, that wdt_post_bind() will get called for all WDT
>> found in the system. My current logic here is, to only register the
>> first WDT found to the global watchdog_dev in GD. To prevent that
>> multiple WDT are used to service the U-Boot watchdog via WATCHDOG_RESET.
>>
>> Or does the DM subsystem automatically bind the watchdog0 alias first
>> in its binding sequence?
>>
>> I do not have a system with multiple watchdogs here, so its not that
>> easy to test this. I could simulate this though somehow...
> 
> I just did some testing on this. One solution might be, to only use
> uclass_get_device_by_seq() (instead of uclass_get_device) and return
> if the alias is not found. This way, all watchdog devices will get
> probed (bound) and only the one referenced by the alias will get used.
> 
> This means on the other hand, that an watchdog0 alias is needed in the
> DT to enable the WATCHDOG_RESET U-Boot servicing, which might be not
> backward compatible.
> 
> Do you have any other thought on this?

As you see above this is what I used and strictly speaking using
different logic will break compatibility for xilinx platforms.

I forget details about logic seq/req_seq but maybe watchdog core should
adopt dev_read_alias_highest_id() which I added for i2c some time ago.

For example if you have two watchdogs and only one alias.
aliases {
	watchdog1 = &wdt1;
}

wdt2: {};

wdt1: {};

Which one should be used by u-boot?
wdt1 because it has alias?
or wdt2 because it is listed first?
or none because there is no watchdog0 alias?

Maybe even solution with taking first alias is wrong and xlnx,watchdog
property in chosen node is a reasonable solution.

Thanks,
Michal