[U-Boot] [PATCH 1/2 v2] watchdog: Implement generic watchdog_reset() version

[U-Boot] [PATCH 1/2 v2] 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: Heiko Schocher <hs@denx.de>
Cc: Tom Rini <trini@konsulko.com>
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 [1] 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 note that I didn't remove the "timeout-sec" handling from the
driver already reading it from the DT (cdns & at91) in this patch.
The reason for this is, that in the cdns case, the removal also brings
a functional change, which I wanted to do in a separate patch. And
in the at91 case its because there are updates to this driver already 
queued in the at91 next git branch which would most likely cause merge
conflict. I'll send a cleanup patch for this driver later after these
patches have been applied.

Thanks,
Stefan

v2:
- Rename watchdog_start() to initr_watchdog() and move it to board_r.c.
  This way its only called once, after the DM subsystem has bound all
  watchdog drivers. This enables the use of the currently implemented
  logic of using the correct watchdog in U-Boot (via alias etc).

 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 ------------------
 common/board_r.c                              | 40 +++++++++++++++++++
 drivers/watchdog/wdt-uclass.c                 | 26 ++++++++++++
 include/asm-generic/global_data.h             |  4 ++
 9 files changed, 70 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/common/board_r.c b/common/board_r.c
index 472987d5d5..d80f16c3ed 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -48,6 +48,7 @@
 #include <linux/compiler.h>
 #include <linux/err.h>
 #include <efi_loader.h>
+#include <wdt.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -621,6 +622,42 @@ static int initr_bedbug(void)
 }
 #endif
 
+#if defined(CONFIG_WATCHDOG) && defined(CONFIG_WDT)
+#ifndef WDT_DEFAULT_TIMEOUT
+#define WDT_DEFAULT_TIMEOUT	60
+#endif
+
+static int initr_watchdog(void)
+{
+	u32 timeout = WDT_DEFAULT_TIMEOUT;
+
+	/*
+	 * 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;
+		}
+	}
+
+	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);
+
+	return 0;
+}
+#endif
+
 static int run_main_loop(void)
 {
 #ifdef CONFIG_SANDBOX
@@ -670,6 +707,9 @@ static init_fnc_t init_sequence_r[] = {
 #ifdef CONFIG_DM
 	initr_dm,
 #endif
+#if defined(CONFIG_WATCHDOG) && defined(CONFIG_WDT)
+	initr_watchdog,
+#endif
 #if defined(CONFIG_ARM) || defined(CONFIG_NDS32) || defined(CONFIG_RISCV) || \
 	defined(CONFIG_SANDBOX)
 	board_init,	/* Setup chipselects */
diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 23b7e3360d..bbfac4f0f9 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -10,6 +10,8 @@
 #include <dm/device-internal.h>
 #include <dm/lists.h>
 
+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 +65,30 @@ 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);
+	}
+}
+#endif
+
 static int wdt_post_bind(struct udevice *dev)
 {
 #if defined(CONFIG_NEEDS_MANUAL_RELOC)
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 2/2 v2] watchdog: cadence: Remove driver specific "timeout-sec" handling

[Stefan Roese]

Now that we have a generic DT property "timeout-sec" handling, the
driver specific implementation can be dropped.

This patch also changes the timeout restriction to the min and max
values (clipping). Before this patch, the value provided via
"timeout-sec" was used if the parameter was too high or low. Now
the driver specific min and max values are used instead.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
---
v2:
- New patch

 drivers/watchdog/cdns_wdt.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/cdns_wdt.c b/drivers/watchdog/cdns_wdt.c
index fc85fbcec2..3ba3c8501c 100644
--- a/drivers/watchdog/cdns_wdt.c
+++ b/drivers/watchdog/cdns_wdt.c
@@ -23,7 +23,6 @@ struct cdns_regs {
 
 struct cdns_wdt_priv {
 	bool rst;
-	u32 timeout;
 	struct cdns_regs *regs;
 };
 
@@ -142,10 +141,9 @@ static int cdns_wdt_start(struct udevice *dev, u64 timeout, ulong flags)
 		return -1;
 	}
 
-	if ((timeout < CDNS_WDT_MIN_TIMEOUT) ||
-	    (timeout > CDNS_WDT_MAX_TIMEOUT)) {
-		timeout = priv->timeout;
-	}
+	/* Restrict timeout to min and max value */
+	timeout = max_t(u64, timeout, CDNS_WDT_MIN_TIMEOUT);
+	timeout = min_t(u64, timeout, CDNS_WDT_MAX_TIMEOUT);
 
 	debug("%s: CLK_FREQ %ld, timeout %lld\n", __func__, clk_f, timeout);
 
@@ -235,12 +233,9 @@ static int cdns_wdt_ofdata_to_platdata(struct udevice *dev)
 	if (IS_ERR(priv->regs))
 		return PTR_ERR(priv->regs);
 
-	priv->timeout = dev_read_u32_default(dev, "timeout-sec",
-					     CDNS_WDT_DEFAULT_TIMEOUT);
-
 	priv->rst = dev_read_bool(dev, "reset-on-timeout");
 
-	debug("%s: timeout %d, reset %d\n", __func__, priv->timeout, priv->rst);
+	debug("%s: reset %d\n", __func__, priv->rst);
 
 	return 0;
 }
-- 
2.21.0

[U-Boot] [PATCH 2/2 v2] watchdog: cadence: Remove driver specific "timeout-sec" handling

[Michal Simek]

On 08. 04. 19 11:28, Stefan Roese wrote:
> Now that we have a generic DT property "timeout-sec" handling, the
> driver specific implementation can be dropped.
> 
> This patch also changes the timeout restriction to the min and max
> values (clipping). Before this patch, the value provided via
> "timeout-sec" was used if the parameter was too high or low. Now
> the driver specific min and max values are used instead.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> ---
> v2:
> - New patch
> 
>  drivers/watchdog/cdns_wdt.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/watchdog/cdns_wdt.c b/drivers/watchdog/cdns_wdt.c
> index fc85fbcec2..3ba3c8501c 100644
> --- a/drivers/watchdog/cdns_wdt.c
> +++ b/drivers/watchdog/cdns_wdt.c
> @@ -23,7 +23,6 @@ struct cdns_regs {
>  
>  struct cdns_wdt_priv {
>  	bool rst;
> -	u32 timeout;
>  	struct cdns_regs *regs;
>  };
>  
> @@ -142,10 +141,9 @@ static int cdns_wdt_start(struct udevice *dev, u64 timeout, ulong flags)
>  		return -1;
>  	}
>  
> -	if ((timeout < CDNS_WDT_MIN_TIMEOUT) ||
> -	    (timeout > CDNS_WDT_MAX_TIMEOUT)) {
> -		timeout = priv->timeout;
> -	}

here should be timeout /= 1000;
because the whole timeout is handled in seconds not miliseconds.

> +	/* Restrict timeout to min and max value */
> +	timeout = max_t(u64, timeout, CDNS_WDT_MIN_TIMEOUT);
> +	timeout = min_t(u64, timeout, CDNS_WDT_MAX_TIMEOUT);
>  
>  	debug("%s: CLK_FREQ %ld, timeout %lld\n", __func__, clk_f, timeout);
>  
> @@ -235,12 +233,9 @@ static int cdns_wdt_ofdata_to_platdata(struct udevice *dev)
>  	if (IS_ERR(priv->regs))
>  		return PTR_ERR(priv->regs);
>  
> -	priv->timeout = dev_read_u32_default(dev, "timeout-sec",
> -					     CDNS_WDT_DEFAULT_TIMEOUT);
> -
>  	priv->rst = dev_read_bool(dev, "reset-on-timeout");
>  
> -	debug("%s: timeout %d, reset %d\n", __func__, priv->timeout, priv->rst);
> +	debug("%s: reset %d\n", __func__, priv->rst);
>  
>  	return 0;
>  }
> 

M

[U-Boot] [PATCH 1/2 v2] watchdog: Implement generic watchdog_reset() version

[Michal Simek]

On 08. 04. 19 11:28, 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: Heiko Schocher <hs@denx.de>
> Cc: Tom Rini <trini@konsulko.com>
> 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 [1] 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 note that I didn't remove the "timeout-sec" handling from the
> driver already reading it from the DT (cdns & at91) in this patch.
> The reason for this is, that in the cdns case, the removal also brings
> a functional change, which I wanted to do in a separate patch. And
> in the at91 case its because there are updates to this driver already 
> queued in the at91 next git branch which would most likely cause merge
> conflict. I'll send a cleanup patch for this driver later after these
> patches have been applied.
> 
> Thanks,
> Stefan
> 
> v2:
> - Rename watchdog_start() to initr_watchdog() and move it to board_r.c.
>   This way its only called once, after the DM subsystem has bound all
>   watchdog drivers. This enables the use of the currently implemented
>   logic of using the correct watchdog in U-Boot (via alias etc).
> 
>  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 ------------------
>  common/board_r.c                              | 40 +++++++++++++++++++
>  drivers/watchdog/wdt-uclass.c                 | 26 ++++++++++++
>  include/asm-generic/global_data.h             |  4 ++
>  9 files changed, 70 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/common/board_r.c b/common/board_r.c
> index 472987d5d5..d80f16c3ed 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -48,6 +48,7 @@
>  #include <linux/compiler.h>
>  #include <linux/err.h>
>  #include <efi_loader.h>
> +#include <wdt.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -621,6 +622,42 @@ static int initr_bedbug(void)
>  }
>  #endif
>  
> +#if defined(CONFIG_WATCHDOG) && defined(CONFIG_WDT)

This is not correct.
Here should be just CONFIG_WDT.

Because there are cases where you want just start watchdog in u-boot but
never service it by u-boot.


> +#ifndef WDT_DEFAULT_TIMEOUT
> +#define WDT_DEFAULT_TIMEOUT	60
> +#endif
> +
> +static int initr_watchdog(void)
> +{
> +	u32 timeout = WDT_DEFAULT_TIMEOUT;
> +
> +	/*
> +	 * 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;
> +		}
> +	}
> +
> +	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);
> +
> +	return 0;
> +}
> +#endif
> +
>  static int run_main_loop(void)
>  {
>  #ifdef CONFIG_SANDBOX
> @@ -670,6 +707,9 @@ static init_fnc_t init_sequence_r[] = {
>  #ifdef CONFIG_DM
>  	initr_dm,
>  #endif
> +#if defined(CONFIG_WATCHDOG) && defined(CONFIG_WDT)

ditto.

> +	initr_watchdog,
> +#endif
>  #if defined(CONFIG_ARM) || defined(CONFIG_NDS32) || defined(CONFIG_RISCV) || \
>  	defined(CONFIG_SANDBOX)
>  	board_init,	/* Setup chipselects */
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 23b7e3360d..bbfac4f0f9 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -10,6 +10,8 @@
>  #include <dm/device-internal.h>
>  #include <dm/lists.h>
>  
> +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 +65,30 @@ 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);
> +	}
> +}
> +#endif
> +
>  static int wdt_post_bind(struct udevice *dev)
>  {
>  #if defined(CONFIG_NEEDS_MANUAL_RELOC)
> 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

CONFIG_WDT here.

nit: don't know if you should use #if defined() or if CONFIG_IS_ENABLED
here.

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

M

[U-Boot] [PATCH 2/2 v2] watchdog: cadence: Remove driver specific "timeout-sec" handling

[Stefan Roese]

On 09.04.19 12:44, Michal Simek wrote:
> On 08. 04. 19 11:28, Stefan Roese wrote:
>> Now that we have a generic DT property "timeout-sec" handling, the
>> driver specific implementation can be dropped.
>>
>> This patch also changes the timeout restriction to the min and max
>> values (clipping). Before this patch, the value provided via
>> "timeout-sec" was used if the parameter was too high or low. Now
>> the driver specific min and max values are used instead.
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Michal Simek <michal.simek@xilinx.com>
>> ---
>> v2:
>> - New patch
>>
>>   drivers/watchdog/cdns_wdt.c | 13 ++++---------
>>   1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/watchdog/cdns_wdt.c b/drivers/watchdog/cdns_wdt.c
>> index fc85fbcec2..3ba3c8501c 100644
>> --- a/drivers/watchdog/cdns_wdt.c
>> +++ b/drivers/watchdog/cdns_wdt.c
>> @@ -23,7 +23,6 @@ struct cdns_regs {
>>   
>>   struct cdns_wdt_priv {
>>   	bool rst;
>> -	u32 timeout;
>>   	struct cdns_regs *regs;
>>   };
>>   
>> @@ -142,10 +141,9 @@ static int cdns_wdt_start(struct udevice *dev, u64 timeout, ulong flags)
>>   		return -1;
>>   	}
>>   
>> -	if ((timeout < CDNS_WDT_MIN_TIMEOUT) ||
>> -	    (timeout > CDNS_WDT_MAX_TIMEOUT)) {
>> -		timeout = priv->timeout;
>> -	}
> 
> here should be timeout /= 1000;
> because the whole timeout is handled in seconds not miliseconds.

Yes, thanks. Will update in the next version.

Thanks,
Stefan

[U-Boot] [PATCH 1/2 v2] watchdog: Implement generic watchdog_reset() version

[Stefan Roese]

On 09.04.19 12:45, Michal Simek wrote:
> On 08. 04. 19 11:28, 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: Heiko Schocher <hs@denx.de>
>> Cc: Tom Rini <trini@konsulko.com>
>> 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 [1] 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 note that I didn't remove the "timeout-sec" handling from the
>> driver already reading it from the DT (cdns & at91) in this patch.
>> The reason for this is, that in the cdns case, the removal also brings
>> a functional change, which I wanted to do in a separate patch. And
>> in the at91 case its because there are updates to this driver already
>> queued in the at91 next git branch which would most likely cause merge
>> conflict. I'll send a cleanup patch for this driver later after these
>> patches have been applied.
>>
>> Thanks,
>> Stefan
>>
>> v2:
>> - Rename watchdog_start() to initr_watchdog() and move it to board_r.c.
>>    This way its only called once, after the DM subsystem has bound all
>>    watchdog drivers. This enables the use of the currently implemented
>>    logic of using the correct watchdog in U-Boot (via alias etc).
>>
>>   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 ------------------
>>   common/board_r.c                              | 40 +++++++++++++++++++
>>   drivers/watchdog/wdt-uclass.c                 | 26 ++++++++++++
>>   include/asm-generic/global_data.h             |  4 ++
>>   9 files changed, 70 insertions(+), 219 deletions(-)

<snip>

>>   int board_early_init_r(void)
>>   {
>>   	u32 val;
>> diff --git a/common/board_r.c b/common/board_r.c
>> index 472987d5d5..d80f16c3ed 100644
>> --- a/common/board_r.c
>> +++ b/common/board_r.c
>> @@ -48,6 +48,7 @@
>>   #include <linux/compiler.h>
>>   #include <linux/err.h>
>>   #include <efi_loader.h>
>> +#include <wdt.h>
>>   
>>   DECLARE_GLOBAL_DATA_PTR;
>>   
>> @@ -621,6 +622,42 @@ static int initr_bedbug(void)
>>   }
>>   #endif
>>   
>> +#if defined(CONFIG_WATCHDOG) && defined(CONFIG_WDT)
> 
> This is not correct.
> Here should be just CONFIG_WDT.
> 
> Because there are cases where you want just start watchdog in u-boot but
> never service it by u-boot.

AFAIK, this would change the current behavior. Currently, only when
CONFIG_WATCHDOG is enabled the watchdog driver is started automatically
in U-Boot (and serviced). If I make the change you suggest above, all
boards defining CONFIG_WDT (DM watchdog support) will automatically
enable the watchdog. This might make sense, but AFAICT this changes
the current behavior.

Thanks,
Stefan
  
> 
>> +#ifndef WDT_DEFAULT_TIMEOUT
>> +#define WDT_DEFAULT_TIMEOUT	60
>> +#endif
>> +
>> +static int initr_watchdog(void)
>> +{
>> +	u32 timeout = WDT_DEFAULT_TIMEOUT;
>> +
>> +	/*
>> +	 * 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;
>> +		}
>> +	}
>> +
>> +	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);
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>>   static int run_main_loop(void)
>>   {
>>   #ifdef CONFIG_SANDBOX
>> @@ -670,6 +707,9 @@ static init_fnc_t init_sequence_r[] = {
>>   #ifdef CONFIG_DM
>>   	initr_dm,
>>   #endif
>> +#if defined(CONFIG_WATCHDOG) && defined(CONFIG_WDT)
> 
> ditto.
> 
>> +	initr_watchdog,
>> +#endif
>>   #if defined(CONFIG_ARM) || defined(CONFIG_NDS32) || defined(CONFIG_RISCV) || \
>>   	defined(CONFIG_SANDBOX)
>>   	board_init,	/* Setup chipselects */
>> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
>> index 23b7e3360d..bbfac4f0f9 100644
>> --- a/drivers/watchdog/wdt-uclass.c
>> +++ b/drivers/watchdog/wdt-uclass.c
>> @@ -10,6 +10,8 @@
>>   #include <dm/device-internal.h>
>>   #include <dm/lists.h>
>>   
>> +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 +65,30 @@ 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);
>> +	}
>> +}
>> +#endif
>> +
>>   static int wdt_post_bind(struct udevice *dev)
>>   {
>>   #if defined(CONFIG_NEEDS_MANUAL_RELOC)
>> 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
> 
> CONFIG_WDT here.
> 
> nit: don't know if you should use #if defined() or if CONFIG_IS_ENABLED
> here.
> 
>> +	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 */
>>
> 
> M
> 

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 at denx.de

[U-Boot] [PATCH 1/2 v2] watchdog: Implement generic watchdog_reset() version

[Michal Simek]

On 09. 04. 19 16:22, Stefan Roese wrote:
> On 09.04.19 12:45, Michal Simek wrote:
>> On 08. 04. 19 11:28, 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: Heiko Schocher <hs@denx.de>
>>> Cc: Tom Rini <trini@konsulko.com>
>>> 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 [1] 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 note that I didn't remove the "timeout-sec" handling from the
>>> driver already reading it from the DT (cdns & at91) in this patch.
>>> The reason for this is, that in the cdns case, the removal also brings
>>> a functional change, which I wanted to do in a separate patch. And
>>> in the at91 case its because there are updates to this driver already
>>> queued in the at91 next git branch which would most likely cause merge
>>> conflict. I'll send a cleanup patch for this driver later after these
>>> patches have been applied.
>>>
>>> Thanks,
>>> Stefan
>>>
>>> v2:
>>> - Rename watchdog_start() to initr_watchdog() and move it to board_r.c.
>>>    This way its only called once, after the DM subsystem has bound all
>>>    watchdog drivers. This enables the use of the currently implemented
>>>    logic of using the correct watchdog in U-Boot (via alias etc).
>>>
>>>   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 ------------------
>>>   common/board_r.c                              | 40 +++++++++++++++++++
>>>   drivers/watchdog/wdt-uclass.c                 | 26 ++++++++++++
>>>   include/asm-generic/global_data.h             |  4 ++
>>>   9 files changed, 70 insertions(+), 219 deletions(-)
> 
> <snip>
> 
>>>   int board_early_init_r(void)
>>>   {
>>>       u32 val;
>>> diff --git a/common/board_r.c b/common/board_r.c
>>> index 472987d5d5..d80f16c3ed 100644
>>> --- a/common/board_r.c
>>> +++ b/common/board_r.c
>>> @@ -48,6 +48,7 @@
>>>   #include <linux/compiler.h>
>>>   #include <linux/err.h>
>>>   #include <efi_loader.h>
>>> +#include <wdt.h>
>>>     DECLARE_GLOBAL_DATA_PTR;
>>>   @@ -621,6 +622,42 @@ static int initr_bedbug(void)
>>>   }
>>>   #endif
>>>   +#if defined(CONFIG_WATCHDOG) && defined(CONFIG_WDT)
>>
>> This is not correct.
>> Here should be just CONFIG_WDT.
>>
>> Because there are cases where you want just start watchdog in u-boot but
>> never service it by u-boot.
> 
> AFAIK, this would change the current behavior. Currently, only when
> CONFIG_WATCHDOG is enabled the watchdog driver is started automatically
> in U-Boot (and serviced). If I make the change you suggest above, all
> boards defining CONFIG_WDT (DM watchdog support) will automatically
> enable the watchdog. This might make sense, but AFAICT this changes
> the current behavior.

The question is what default is. Because when I was adding support for
Zynq/ZynqMP/Microblaze this logic is used there.
I think that WDT should be there and if you think there are boards which
want to have both we can cover that by Kconfig.

Thanks,
Michal

[U-Boot] [PATCH 1/2 v2] watchdog: Implement generic watchdog_reset() version

[Stefan Roese]

On 09.04.19 16:27, Michal Simek wrote:
> On 09. 04. 19 16:22, Stefan Roese wrote:
>> On 09.04.19 12:45, Michal Simek wrote:
>>> On 08. 04. 19 11:28, 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: Heiko Schocher <hs@denx.de>
>>>> Cc: Tom Rini <trini@konsulko.com>
>>>> 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 [1] 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 note that I didn't remove the "timeout-sec" handling from the
>>>> driver already reading it from the DT (cdns & at91) in this patch.
>>>> The reason for this is, that in the cdns case, the removal also brings
>>>> a functional change, which I wanted to do in a separate patch. And
>>>> in the at91 case its because there are updates to this driver already
>>>> queued in the at91 next git branch which would most likely cause merge
>>>> conflict. I'll send a cleanup patch for this driver later after these
>>>> patches have been applied.
>>>>
>>>> Thanks,
>>>> Stefan
>>>>
>>>> v2:
>>>> - Rename watchdog_start() to initr_watchdog() and move it to board_r.c.
>>>>     This way its only called once, after the DM subsystem has bound all
>>>>     watchdog drivers. This enables the use of the currently implemented
>>>>     logic of using the correct watchdog in U-Boot (via alias etc).
>>>>
>>>>    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 ------------------
>>>>    common/board_r.c                              | 40 +++++++++++++++++++
>>>>    drivers/watchdog/wdt-uclass.c                 | 26 ++++++++++++
>>>>    include/asm-generic/global_data.h             |  4 ++
>>>>    9 files changed, 70 insertions(+), 219 deletions(-)
>>
>> <snip>
>>
>>>>    int board_early_init_r(void)
>>>>    {
>>>>        u32 val;
>>>> diff --git a/common/board_r.c b/common/board_r.c
>>>> index 472987d5d5..d80f16c3ed 100644
>>>> --- a/common/board_r.c
>>>> +++ b/common/board_r.c
>>>> @@ -48,6 +48,7 @@
>>>>    #include <linux/compiler.h>
>>>>    #include <linux/err.h>
>>>>    #include <efi_loader.h>
>>>> +#include <wdt.h>
>>>>      DECLARE_GLOBAL_DATA_PTR;
>>>>    @@ -621,6 +622,42 @@ static int initr_bedbug(void)
>>>>    }
>>>>    #endif
>>>>    +#if defined(CONFIG_WATCHDOG) && defined(CONFIG_WDT)
>>>
>>> This is not correct.
>>> Here should be just CONFIG_WDT.
>>>
>>> Because there are cases where you want just start watchdog in u-boot but
>>> never service it by u-boot.
>>
>> AFAIK, this would change the current behavior. Currently, only when
>> CONFIG_WATCHDOG is enabled the watchdog driver is started automatically
>> in U-Boot (and serviced). If I make the change you suggest above, all
>> boards defining CONFIG_WDT (DM watchdog support) will automatically
>> enable the watchdog. This might make sense, but AFAICT this changes
>> the current behavior.
> 
> The question is what default is. Because when I was adding support for
> Zynq/ZynqMP/Microblaze this logic is used there.
> I think that WDT should be there and if you think there are boards which
> want to have both we can cover that by Kconfig.

As mentioned above, I agree that it makes sense to start the watchdog
automatically, if its enabled / selected via CONFIG_WDT. I suggest
that I move forward with your suggested change, but it would be fair
to Cc the maintainers of boards that have CONFIG_WDT set but
CONFIG_WATCHDOG unset. Do you (or anyone else) know of a tool / script
to detect such board configurations in U-Boot (one option set and
another unset)?

Thanks,
Stefan

[U-Boot] [PATCH 1/2 v2] watchdog: Implement generic watchdog_reset() version

[Michal Simek]

On 09. 04. 19 16:34, Stefan Roese wrote:
> On 09.04.19 16:27, Michal Simek wrote:
>> On 09. 04. 19 16:22, Stefan Roese wrote:
>>> On 09.04.19 12:45, Michal Simek wrote:
>>>> On 08. 04. 19 11:28, 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: Heiko Schocher <hs@denx.de>
>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>> 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 [1] 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 note that I didn't remove the "timeout-sec" handling from the
>>>>> driver already reading it from the DT (cdns & at91) in this patch.
>>>>> The reason for this is, that in the cdns case, the removal also brings
>>>>> a functional change, which I wanted to do in a separate patch. And
>>>>> in the at91 case its because there are updates to this driver already
>>>>> queued in the at91 next git branch which would most likely cause merge
>>>>> conflict. I'll send a cleanup patch for this driver later after these
>>>>> patches have been applied.
>>>>>
>>>>> Thanks,
>>>>> Stefan
>>>>>
>>>>> v2:
>>>>> - Rename watchdog_start() to initr_watchdog() and move it to
>>>>> board_r.c.
>>>>>     This way its only called once, after the DM subsystem has bound
>>>>> all
>>>>>     watchdog drivers. This enables the use of the currently
>>>>> implemented
>>>>>     logic of using the correct watchdog in U-Boot (via alias etc).
>>>>>
>>>>>    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
>>>>> ------------------
>>>>>    common/board_r.c                              | 40
>>>>> +++++++++++++++++++
>>>>>    drivers/watchdog/wdt-uclass.c                 | 26 ++++++++++++
>>>>>    include/asm-generic/global_data.h             |  4 ++
>>>>>    9 files changed, 70 insertions(+), 219 deletions(-)
>>>
>>> <snip>
>>>
>>>>>    int board_early_init_r(void)
>>>>>    {
>>>>>        u32 val;
>>>>> diff --git a/common/board_r.c b/common/board_r.c
>>>>> index 472987d5d5..d80f16c3ed 100644
>>>>> --- a/common/board_r.c
>>>>> +++ b/common/board_r.c
>>>>> @@ -48,6 +48,7 @@
>>>>>    #include <linux/compiler.h>
>>>>>    #include <linux/err.h>
>>>>>    #include <efi_loader.h>
>>>>> +#include <wdt.h>
>>>>>      DECLARE_GLOBAL_DATA_PTR;
>>>>>    @@ -621,6 +622,42 @@ static int initr_bedbug(void)
>>>>>    }
>>>>>    #endif
>>>>>    +#if defined(CONFIG_WATCHDOG) && defined(CONFIG_WDT)
>>>>
>>>> This is not correct.
>>>> Here should be just CONFIG_WDT.
>>>>
>>>> Because there are cases where you want just start watchdog in u-boot
>>>> but
>>>> never service it by u-boot.
>>>
>>> AFAIK, this would change the current behavior. Currently, only when
>>> CONFIG_WATCHDOG is enabled the watchdog driver is started automatically
>>> in U-Boot (and serviced). If I make the change you suggest above, all
>>> boards defining CONFIG_WDT (DM watchdog support) will automatically
>>> enable the watchdog. This might make sense, but AFAICT this changes
>>> the current behavior.
>>
>> The question is what default is. Because when I was adding support for
>> Zynq/ZynqMP/Microblaze this logic is used there.
>> I think that WDT should be there and if you think there are boards which
>> want to have both we can cover that by Kconfig.
> 
> As mentioned above, I agree that it makes sense to start the watchdog
> automatically, if its enabled / selected via CONFIG_WDT. I suggest
> that I move forward with your suggested change, but it would be fair
> to Cc the maintainers of boards that have CONFIG_WDT set but
> CONFIG_WATCHDOG unset. Do you (or anyone else) know of a tool / script
> to detect such board configurations in U-Boot (one option set and
> another unset)?

good.
Tom?

This should be also fine.

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 10fd3039aa20..3c0f90559b86 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -51,6 +51,7 @@ config ULP_WATCHDOG
 config WDT
        bool "Enable driver model for watchdog timer drivers"
        depends on DM
+       imply WATCHDOG
        help
          Enable driver model for watchdog timer. At the moment the API
          is very simple and only supports four operations:



M

[U-Boot] [PATCH 1/2 v2] watchdog: Implement generic watchdog_reset() version

[Stefan Roese]

On 10.04.19 09:44, Michal Simek wrote:
> On 09. 04. 19 16:34, Stefan Roese wrote:
>> On 09.04.19 16:27, Michal Simek wrote:
>>> On 09. 04. 19 16:22, Stefan Roese wrote:
>>>> On 09.04.19 12:45, Michal Simek wrote:
>>>>> On 08. 04. 19 11:28, 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: Heiko Schocher <hs@denx.de>
>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>> 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 [1] 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 note that I didn't remove the "timeout-sec" handling from the
>>>>>> driver already reading it from the DT (cdns & at91) in this patch.
>>>>>> The reason for this is, that in the cdns case, the removal also brings
>>>>>> a functional change, which I wanted to do in a separate patch. And
>>>>>> in the at91 case its because there are updates to this driver already
>>>>>> queued in the at91 next git branch which would most likely cause merge
>>>>>> conflict. I'll send a cleanup patch for this driver later after these
>>>>>> patches have been applied.
>>>>>>
>>>>>> Thanks,
>>>>>> Stefan
>>>>>>
>>>>>> v2:
>>>>>> - Rename watchdog_start() to initr_watchdog() and move it to
>>>>>> board_r.c.
>>>>>>      This way its only called once, after the DM subsystem has bound
>>>>>> all
>>>>>>      watchdog drivers. This enables the use of the currently
>>>>>> implemented
>>>>>>      logic of using the correct watchdog in U-Boot (via alias etc).
>>>>>>
>>>>>>     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
>>>>>> ------------------
>>>>>>     common/board_r.c                              | 40
>>>>>> +++++++++++++++++++
>>>>>>     drivers/watchdog/wdt-uclass.c                 | 26 ++++++++++++
>>>>>>     include/asm-generic/global_data.h             |  4 ++
>>>>>>     9 files changed, 70 insertions(+), 219 deletions(-)
>>>>
>>>> <snip>
>>>>
>>>>>>     int board_early_init_r(void)
>>>>>>     {
>>>>>>         u32 val;
>>>>>> diff --git a/common/board_r.c b/common/board_r.c
>>>>>> index 472987d5d5..d80f16c3ed 100644
>>>>>> --- a/common/board_r.c
>>>>>> +++ b/common/board_r.c
>>>>>> @@ -48,6 +48,7 @@
>>>>>>     #include <linux/compiler.h>
>>>>>>     #include <linux/err.h>
>>>>>>     #include <efi_loader.h>
>>>>>> +#include <wdt.h>
>>>>>>       DECLARE_GLOBAL_DATA_PTR;
>>>>>>     @@ -621,6 +622,42 @@ static int initr_bedbug(void)
>>>>>>     }
>>>>>>     #endif
>>>>>>     +#if defined(CONFIG_WATCHDOG) && defined(CONFIG_WDT)
>>>>>
>>>>> This is not correct.
>>>>> Here should be just CONFIG_WDT.
>>>>>
>>>>> Because there are cases where you want just start watchdog in u-boot
>>>>> but
>>>>> never service it by u-boot.
>>>>
>>>> AFAIK, this would change the current behavior. Currently, only when
>>>> CONFIG_WATCHDOG is enabled the watchdog driver is started automatically
>>>> in U-Boot (and serviced). If I make the change you suggest above, all
>>>> boards defining CONFIG_WDT (DM watchdog support) will automatically
>>>> enable the watchdog. This might make sense, but AFAICT this changes
>>>> the current behavior.
>>>
>>> The question is what default is. Because when I was adding support for
>>> Zynq/ZynqMP/Microblaze this logic is used there.
>>> I think that WDT should be there and if you think there are boards which
>>> want to have both we can cover that by Kconfig.
>>
>> As mentioned above, I agree that it makes sense to start the watchdog
>> automatically, if its enabled / selected via CONFIG_WDT. I suggest
>> that I move forward with your suggested change, but it would be fair
>> to Cc the maintainers of boards that have CONFIG_WDT set but
>> CONFIG_WATCHDOG unset. Do you (or anyone else) know of a tool / script
>> to detect such board configurations in U-Boot (one option set and
>> another unset)?
> 
> good.
> Tom?

I've run a Travis job on all boards with an error added for this
configuration (CONFIG_WDT set but CONFIG_WATCHDOG unset). Here the
list of these boards with their maintainers:

taurus: Heiko Schocher <hs@denx.de>
smartweb: Heiko Schocher <hs@denx.de>
ast2500: Maxim Sloyko <maxims@google.com>
picosam9g45: Erik van Luijk <evanluijk@interact.nl>
mt7623n_bpir2: Ryder Lee <ryder.lee@mediatek.com> Weijie Gao <weijie.gao@mediatek.com>
mt7629_rfb: Ryder Lee <ryder.lee@mediatek.com> Weijie Gao <weijie.gao@mediatek.com>
bitmain_antminer_s9: Michal Simek <monstr@monstr.eu>
sandbox64: Simon Glass <sjg@chromium.org>
sandbox: Simon Glass <sjg@chromium.org>
comtrend_ct5361_ram: "Álvaro Fernández Rojas" <noltari@gmail.com>
netgear_cg3100d_ram: "Álvaro Fernández Rojas" <noltari@gmail.com>
bcm968380gerg_ram: Philippe Reynes <philippe.reynes@softathome.com>
bcm963158_ram: Philippe Reynes <philippe.reynes@softathome.com>
bcm968580xref_ram: Philippe Reynes <philippe.reynes@softathome.com>
MCR3000: Christophe Leroy <christophe.leroy@c-s.fr>

> This should be also fine.
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 10fd3039aa20..3c0f90559b86 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -51,6 +51,7 @@ config ULP_WATCHDOG
>   config WDT
>          bool "Enable driver model for watchdog timer drivers"
>          depends on DM
> +       imply WATCHDOG
>          help
>            Enable driver model for watchdog timer. At the moment the API
>            is very simple and only supports four operations:

I like this idea to keep the current configuration but enabling
board maintainers to not use the U-Boot watchdog servicing feature
by disabling CONFIG_WATCHDOG if they want to.

I'll send v3 of this patch with the "imply" added and will add the
board maintainer to Cc, so that they can decide themselves.

Thanks,
Stefan

[U-Boot] [PATCH 1/2 v2] watchdog: Implement generic watchdog_reset() version

[Michal Simek]

On 10. 04. 19 10:40, Stefan Roese wrote:
> On 10.04.19 09:44, Michal Simek wrote:
>> On 09. 04. 19 16:34, Stefan Roese wrote:
>>> On 09.04.19 16:27, Michal Simek wrote:
>>>> On 09. 04. 19 16:22, Stefan Roese wrote:
>>>>> On 09.04.19 12:45, Michal Simek wrote:
>>>>>> On 08. 04. 19 11:28, 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: Heiko Schocher <hs@denx.de>
>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>> 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 [1] 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 note that I didn't remove the "timeout-sec" handling from the
>>>>>>> driver already reading it from the DT (cdns & at91) in this patch.
>>>>>>> The reason for this is, that in the cdns case, the removal also
>>>>>>> brings
>>>>>>> a functional change, which I wanted to do in a separate patch. And
>>>>>>> in the at91 case its because there are updates to this driver
>>>>>>> already
>>>>>>> queued in the at91 next git branch which would most likely cause
>>>>>>> merge
>>>>>>> conflict. I'll send a cleanup patch for this driver later after
>>>>>>> these
>>>>>>> patches have been applied.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Stefan
>>>>>>>
>>>>>>> v2:
>>>>>>> - Rename watchdog_start() to initr_watchdog() and move it to
>>>>>>> board_r.c.
>>>>>>>      This way its only called once, after the DM subsystem has bound
>>>>>>> all
>>>>>>>      watchdog drivers. This enables the use of the currently
>>>>>>> implemented
>>>>>>>      logic of using the correct watchdog in U-Boot (via alias etc).
>>>>>>>
>>>>>>>     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
>>>>>>> ------------------
>>>>>>>     common/board_r.c                              | 40
>>>>>>> +++++++++++++++++++
>>>>>>>     drivers/watchdog/wdt-uclass.c                 | 26 ++++++++++++
>>>>>>>     include/asm-generic/global_data.h             |  4 ++
>>>>>>>     9 files changed, 70 insertions(+), 219 deletions(-)
>>>>>
>>>>> <snip>
>>>>>
>>>>>>>     int board_early_init_r(void)
>>>>>>>     {
>>>>>>>         u32 val;
>>>>>>> diff --git a/common/board_r.c b/common/board_r.c
>>>>>>> index 472987d5d5..d80f16c3ed 100644
>>>>>>> --- a/common/board_r.c
>>>>>>> +++ b/common/board_r.c
>>>>>>> @@ -48,6 +48,7 @@
>>>>>>>     #include <linux/compiler.h>
>>>>>>>     #include <linux/err.h>
>>>>>>>     #include <efi_loader.h>
>>>>>>> +#include <wdt.h>
>>>>>>>       DECLARE_GLOBAL_DATA_PTR;
>>>>>>>     @@ -621,6 +622,42 @@ static int initr_bedbug(void)
>>>>>>>     }
>>>>>>>     #endif
>>>>>>>     +#if defined(CONFIG_WATCHDOG) && defined(CONFIG_WDT)
>>>>>>
>>>>>> This is not correct.
>>>>>> Here should be just CONFIG_WDT.
>>>>>>
>>>>>> Because there are cases where you want just start watchdog in u-boot
>>>>>> but
>>>>>> never service it by u-boot.
>>>>>
>>>>> AFAIK, this would change the current behavior. Currently, only when
>>>>> CONFIG_WATCHDOG is enabled the watchdog driver is started
>>>>> automatically
>>>>> in U-Boot (and serviced). If I make the change you suggest above, all
>>>>> boards defining CONFIG_WDT (DM watchdog support) will automatically
>>>>> enable the watchdog. This might make sense, but AFAICT this changes
>>>>> the current behavior.
>>>>
>>>> The question is what default is. Because when I was adding support for
>>>> Zynq/ZynqMP/Microblaze this logic is used there.
>>>> I think that WDT should be there and if you think there are boards
>>>> which
>>>> want to have both we can cover that by Kconfig.
>>>
>>> As mentioned above, I agree that it makes sense to start the watchdog
>>> automatically, if its enabled / selected via CONFIG_WDT. I suggest
>>> that I move forward with your suggested change, but it would be fair
>>> to Cc the maintainers of boards that have CONFIG_WDT set but
>>> CONFIG_WATCHDOG unset. Do you (or anyone else) know of a tool / script
>>> to detect such board configurations in U-Boot (one option set and
>>> another unset)?
>>
>> good.
>> Tom?
> 
> I've run a Travis job on all boards with an error added for this
> configuration (CONFIG_WDT set but CONFIG_WATCHDOG unset). Here the
> list of these boards with their maintainers:
> 
> taurus: Heiko Schocher <hs@denx.de>
> smartweb: Heiko Schocher <hs@denx.de>
> ast2500: Maxim Sloyko <maxims@google.com>
> picosam9g45: Erik van Luijk <evanluijk@interact.nl>
> mt7623n_bpir2: Ryder Lee <ryder.lee@mediatek.com> Weijie Gao
> <weijie.gao@mediatek.com>
> mt7629_rfb: Ryder Lee <ryder.lee@mediatek.com> Weijie Gao
> <weijie.gao@mediatek.com>
> bitmain_antminer_s9: Michal Simek <monstr@monstr.eu>

On this board disabling watchdog servicing was done by purpose.

Thanks,
Michal

[U-Boot] [PATCH 1/2 v2] watchdog: Implement generic watchdog_reset() version

[Stefan Roese]

On 10.04.19 10:46, Michal Simek wrote:
> On 10. 04. 19 10:40, Stefan Roese wrote:
>> On 10.04.19 09:44, Michal Simek wrote:
>>> On 09. 04. 19 16:34, Stefan Roese wrote:
>>>> On 09.04.19 16:27, Michal Simek wrote:
>>>>> On 09. 04. 19 16:22, Stefan Roese wrote:
>>>>>> On 09.04.19 12:45, Michal Simek wrote:
>>>>>>> On 08. 04. 19 11:28, 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: Heiko Schocher <hs@denx.de>
>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>> 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 [1] 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 note that I didn't remove the "timeout-sec" handling from the
>>>>>>>> driver already reading it from the DT (cdns & at91) in this patch.
>>>>>>>> The reason for this is, that in the cdns case, the removal also
>>>>>>>> brings
>>>>>>>> a functional change, which I wanted to do in a separate patch. And
>>>>>>>> in the at91 case its because there are updates to this driver
>>>>>>>> already
>>>>>>>> queued in the at91 next git branch which would most likely cause
>>>>>>>> merge
>>>>>>>> conflict. I'll send a cleanup patch for this driver later after
>>>>>>>> these
>>>>>>>> patches have been applied.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Stefan
>>>>>>>>
>>>>>>>> v2:
>>>>>>>> - Rename watchdog_start() to initr_watchdog() and move it to
>>>>>>>> board_r.c.
>>>>>>>>       This way its only called once, after the DM subsystem has bound
>>>>>>>> all
>>>>>>>>       watchdog drivers. This enables the use of the currently
>>>>>>>> implemented
>>>>>>>>       logic of using the correct watchdog in U-Boot (via alias etc).
>>>>>>>>
>>>>>>>>      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
>>>>>>>> ------------------
>>>>>>>>      common/board_r.c                              | 40
>>>>>>>> +++++++++++++++++++
>>>>>>>>      drivers/watchdog/wdt-uclass.c                 | 26 ++++++++++++
>>>>>>>>      include/asm-generic/global_data.h             |  4 ++
>>>>>>>>      9 files changed, 70 insertions(+), 219 deletions(-)
>>>>>>
>>>>>> <snip>
>>>>>>
>>>>>>>>      int board_early_init_r(void)
>>>>>>>>      {
>>>>>>>>          u32 val;
>>>>>>>> diff --git a/common/board_r.c b/common/board_r.c
>>>>>>>> index 472987d5d5..d80f16c3ed 100644
>>>>>>>> --- a/common/board_r.c
>>>>>>>> +++ b/common/board_r.c
>>>>>>>> @@ -48,6 +48,7 @@
>>>>>>>>      #include <linux/compiler.h>
>>>>>>>>      #include <linux/err.h>
>>>>>>>>      #include <efi_loader.h>
>>>>>>>> +#include <wdt.h>
>>>>>>>>        DECLARE_GLOBAL_DATA_PTR;
>>>>>>>>      @@ -621,6 +622,42 @@ static int initr_bedbug(void)
>>>>>>>>      }
>>>>>>>>      #endif
>>>>>>>>      +#if defined(CONFIG_WATCHDOG) && defined(CONFIG_WDT)
>>>>>>>
>>>>>>> This is not correct.
>>>>>>> Here should be just CONFIG_WDT.
>>>>>>>
>>>>>>> Because there are cases where you want just start watchdog in u-boot
>>>>>>> but
>>>>>>> never service it by u-boot.
>>>>>>
>>>>>> AFAIK, this would change the current behavior. Currently, only when
>>>>>> CONFIG_WATCHDOG is enabled the watchdog driver is started
>>>>>> automatically
>>>>>> in U-Boot (and serviced). If I make the change you suggest above, all
>>>>>> boards defining CONFIG_WDT (DM watchdog support) will automatically
>>>>>> enable the watchdog. This might make sense, but AFAICT this changes
>>>>>> the current behavior.
>>>>>
>>>>> The question is what default is. Because when I was adding support for
>>>>> Zynq/ZynqMP/Microblaze this logic is used there.
>>>>> I think that WDT should be there and if you think there are boards
>>>>> which
>>>>> want to have both we can cover that by Kconfig.
>>>>
>>>> As mentioned above, I agree that it makes sense to start the watchdog
>>>> automatically, if its enabled / selected via CONFIG_WDT. I suggest
>>>> that I move forward with your suggested change, but it would be fair
>>>> to Cc the maintainers of boards that have CONFIG_WDT set but
>>>> CONFIG_WATCHDOG unset. Do you (or anyone else) know of a tool / script
>>>> to detect such board configurations in U-Boot (one option set and
>>>> another unset)?
>>>
>>> good.
>>> Tom?
>>
>> I've run a Travis job on all boards with an error added for this
>> configuration (CONFIG_WDT set but CONFIG_WATCHDOG unset). Here the
>> list of these boards with their maintainers:
>>
>> taurus: Heiko Schocher <hs@denx.de>
>> smartweb: Heiko Schocher <hs@denx.de>
>> ast2500: Maxim Sloyko <maxims@google.com>
>> picosam9g45: Erik van Luijk <evanluijk@interact.nl>
>> mt7623n_bpir2: Ryder Lee <ryder.lee@mediatek.com> Weijie Gao
>> <weijie.gao@mediatek.com>
>> mt7629_rfb: Ryder Lee <ryder.lee@mediatek.com> Weijie Gao
>> <weijie.gao@mediatek.com>
>> bitmain_antminer_s9: Michal Simek <monstr@monstr.eu>
> 
> On this board disabling watchdog servicing was done by purpose.

And it will stay disabled even with the imply, as the defconfig
currently disables CONFIG_WATCHDOG. So no change here.

BTW: bitmain_antminer_s9_defconfig seems to be the only board that
disables CONFIG_WATCHDOG.

Thanks,
Stefan

[U-Boot] [PATCH 1/2 v2] watchdog: Implement generic watchdog_reset() version

[Michal Simek]

On 10. 04. 19 10:52, Stefan Roese wrote:
> On 10.04.19 10:46, Michal Simek wrote:
>> On 10. 04. 19 10:40, Stefan Roese wrote:
>>> On 10.04.19 09:44, Michal Simek wrote:
>>>> On 09. 04. 19 16:34, Stefan Roese wrote:
>>>>> On 09.04.19 16:27, Michal Simek wrote:
>>>>>> On 09. 04. 19 16:22, Stefan Roese wrote:
>>>>>>> On 09.04.19 12:45, Michal Simek wrote:
>>>>>>>> On 08. 04. 19 11:28, 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: Heiko Schocher <hs@denx.de>
>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>>> 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 [1] 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 note that I didn't remove the "timeout-sec" handling
>>>>>>>>> from the
>>>>>>>>> driver already reading it from the DT (cdns & at91) in this patch.
>>>>>>>>> The reason for this is, that in the cdns case, the removal also
>>>>>>>>> brings
>>>>>>>>> a functional change, which I wanted to do in a separate patch. And
>>>>>>>>> in the at91 case its because there are updates to this driver
>>>>>>>>> already
>>>>>>>>> queued in the at91 next git branch which would most likely cause
>>>>>>>>> merge
>>>>>>>>> conflict. I'll send a cleanup patch for this driver later after
>>>>>>>>> these
>>>>>>>>> patches have been applied.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Stefan
>>>>>>>>>
>>>>>>>>> v2:
>>>>>>>>> - Rename watchdog_start() to initr_watchdog() and move it to
>>>>>>>>> board_r.c.
>>>>>>>>>       This way its only called once, after the DM subsystem has
>>>>>>>>> bound
>>>>>>>>> all
>>>>>>>>>       watchdog drivers. This enables the use of the currently
>>>>>>>>> implemented
>>>>>>>>>       logic of using the correct watchdog in U-Boot (via alias
>>>>>>>>> etc).
>>>>>>>>>
>>>>>>>>>      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
>>>>>>>>> ------------------
>>>>>>>>>      common/board_r.c                              | 40
>>>>>>>>> +++++++++++++++++++
>>>>>>>>>      drivers/watchdog/wdt-uclass.c                 | 26
>>>>>>>>> ++++++++++++
>>>>>>>>>      include/asm-generic/global_data.h             |  4 ++
>>>>>>>>>      9 files changed, 70 insertions(+), 219 deletions(-)
>>>>>>>
>>>>>>> <snip>
>>>>>>>
>>>>>>>>>      int board_early_init_r(void)
>>>>>>>>>      {
>>>>>>>>>          u32 val;
>>>>>>>>> diff --git a/common/board_r.c b/common/board_r.c
>>>>>>>>> index 472987d5d5..d80f16c3ed 100644
>>>>>>>>> --- a/common/board_r.c
>>>>>>>>> +++ b/common/board_r.c
>>>>>>>>> @@ -48,6 +48,7 @@
>>>>>>>>>      #include <linux/compiler.h>
>>>>>>>>>      #include <linux/err.h>
>>>>>>>>>      #include <efi_loader.h>
>>>>>>>>> +#include <wdt.h>
>>>>>>>>>        DECLARE_GLOBAL_DATA_PTR;
>>>>>>>>>      @@ -621,6 +622,42 @@ static int initr_bedbug(void)
>>>>>>>>>      }
>>>>>>>>>      #endif
>>>>>>>>>      +#if defined(CONFIG_WATCHDOG) && defined(CONFIG_WDT)
>>>>>>>>
>>>>>>>> This is not correct.
>>>>>>>> Here should be just CONFIG_WDT.
>>>>>>>>
>>>>>>>> Because there are cases where you want just start watchdog in
>>>>>>>> u-boot
>>>>>>>> but
>>>>>>>> never service it by u-boot.
>>>>>>>
>>>>>>> AFAIK, this would change the current behavior. Currently, only when
>>>>>>> CONFIG_WATCHDOG is enabled the watchdog driver is started
>>>>>>> automatically
>>>>>>> in U-Boot (and serviced). If I make the change you suggest above,
>>>>>>> all
>>>>>>> boards defining CONFIG_WDT (DM watchdog support) will automatically
>>>>>>> enable the watchdog. This might make sense, but AFAICT this changes
>>>>>>> the current behavior.
>>>>>>
>>>>>> The question is what default is. Because when I was adding support
>>>>>> for
>>>>>> Zynq/ZynqMP/Microblaze this logic is used there.
>>>>>> I think that WDT should be there and if you think there are boards
>>>>>> which
>>>>>> want to have both we can cover that by Kconfig.
>>>>>
>>>>> As mentioned above, I agree that it makes sense to start the watchdog
>>>>> automatically, if its enabled / selected via CONFIG_WDT. I suggest
>>>>> that I move forward with your suggested change, but it would be fair
>>>>> to Cc the maintainers of boards that have CONFIG_WDT set but
>>>>> CONFIG_WATCHDOG unset. Do you (or anyone else) know of a tool / script
>>>>> to detect such board configurations in U-Boot (one option set and
>>>>> another unset)?
>>>>
>>>> good.
>>>> Tom?
>>>
>>> I've run a Travis job on all boards with an error added for this
>>> configuration (CONFIG_WDT set but CONFIG_WATCHDOG unset). Here the
>>> list of these boards with their maintainers:
>>>
>>> taurus: Heiko Schocher <hs@denx.de>
>>> smartweb: Heiko Schocher <hs@denx.de>
>>> ast2500: Maxim Sloyko <maxims@google.com>
>>> picosam9g45: Erik van Luijk <evanluijk@interact.nl>
>>> mt7623n_bpir2: Ryder Lee <ryder.lee@mediatek.com> Weijie Gao
>>> <weijie.gao@mediatek.com>
>>> mt7629_rfb: Ryder Lee <ryder.lee@mediatek.com> Weijie Gao
>>> <weijie.gao@mediatek.com>
>>> bitmain_antminer_s9: Michal Simek <monstr@monstr.eu>
>>
>> On this board disabling watchdog servicing was done by purpose.
> 
> And it will stay disabled even with the imply, as the defconfig
> currently disables CONFIG_WATCHDOG. So no change here.

yup

> 
> BTW: bitmain_antminer_s9_defconfig seems to be the only board that
> disables CONFIG_WATCHDOG.

Possible and that's the board I used for tuning this feature.

M