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

[U-Boot] [PATCH 1/4 v5] 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>
Cc: Maxim Sloyko <maxims@google.com>
Cc: Erik van Luijk <evanluijk@interact.nl>
Cc: Ryder Lee <ryder.lee@mediatek.com>
Cc: Weijie Gao <weijie.gao@mediatek.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: "Álvaro Fernández Rojas" <noltari@gmail.com>
Cc: Philippe Reynes <philippe.reynes@softathome.com>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Reviewed-by: Michal Simek <michal.simek@xilinx.com>
Tested-by: Michal Simek <michal.simek@xilinx.com> (on zcu100)
---
We have a few boards that have CONFIG_WDT (DM watchdog driver) enabled
and CONFIG_WATCHDOG (U-Boot watchdog servicing) disabled. This might
lead to a watchdog reboot, if the board does not boot fast enough into
some OS (like Linux) which services the watchdog. With this patch
now, the watchdog is started on all those boards (if found via the DT)
and also serviced as this patch also "implies" CONFIG_WATCHDOG. If
this is not desired, then please send a patch to disable
CONFIG_WATCHDOG for this board. Here the list of those 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>

Thanks,
Stefan

v5:
- Remove board specific x530 watchdog support and implement this in
  this generic apprioach as well. For this, we need to support the
  watchdog in SPL now. Move the watchdog init function initr_watchdog()
  into wdt.h to not duplicate this code.

v4:
- No change

v3:
- Use already available CONFIG_WATCHDOG_TIMEOUT_MSECS option vs the
  newly introduced WDT_DEFAULT_TIMEOUT default timeout value (if not
  provided via DT property).
- Guard initr_watchdog by CONFIG_WDT instead of CONFIG_WATCHDOG &&
  CONFIG_WDT as suggested by Michal. This makes it possible to enable
  and start the U-Boot watchdog without enabling the U-Boot watchdog
  servicing.
- Add imply CONFIG_WATCHDOG to CONFIG_WDT, as this restores the
  current behaviour to use the U-Boot watchdog servicing feature, if
  the DM watchdog is enabled (CONFIG_WDT). If this is not desired,
  CONFIG_WATCHDOG can be disabled in the board defconfig.
- Remove CONFIG_WATCHDOG from include/configs/turris_omnia.h

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 ----------------
 board/alliedtelesis/x530/x530.c               | 36 +---------------
 .../microblaze-generic/microblaze-generic.c   | 40 ------------------
 board/xilinx/zynq/board.c                     | 39 ------------------
 board/xilinx/zynqmp/zynqmp.c                  | 39 ------------------
 common/board_r.c                              |  4 ++
 common/spl/spl.c                              |  5 +++
 drivers/watchdog/Kconfig                      |  1 +
 drivers/watchdog/wdt-uclass.c                 | 26 ++++++++++++
 include/asm-generic/global_data.h             |  4 ++
 include/configs/turris_omnia.h                |  5 ---
 include/wdt.h                                 | 41 +++++++++++++++++++
 14 files changed, 82 insertions(+), 259 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 0287f23283..4c08f810a2 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, 120000, 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/alliedtelesis/x530/x530.c b/board/alliedtelesis/x530/x530.c
index 6934fd8017..97dbed79dd 100644
--- a/board/alliedtelesis/x530/x530.c
+++ b/board/alliedtelesis/x530/x530.c
@@ -25,10 +25,6 @@ DECLARE_GLOBAL_DATA_PTR;
 #define CONFIG_NVS_LOCATION		0xf4800000
 #define CONFIG_NVS_SIZE			(512 << 10)
 
-#ifdef CONFIG_WATCHDOG
-static struct udevice *watchdog_dev;
-#endif
-
 static struct serdes_map board_serdes_map[] = {
 	{PEX0, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
 	{DEFAULT_SERDES, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
@@ -80,10 +76,6 @@ struct mv_ddr_topology_map *mv_ddr_topology_map_get(void)
 
 int board_early_init_f(void)
 {
-#ifdef CONFIG_WATCHDOG
-	watchdog_dev = NULL;
-#endif
-
 	/* Configure MPP */
 	writel(0x00001111, MVEBU_MPP_BASE + 0x00);
 	writel(0x00000000, MVEBU_MPP_BASE + 0x04);
@@ -99,13 +91,6 @@ int board_early_init_f(void)
 
 void spl_board_init(void)
 {
-#ifdef CONFIG_WATCHDOG
-	int ret;
-
-	ret = uclass_get_device(UCLASS_WDT, 0, &watchdog_dev);
-	if (!ret)
-		wdt_start(watchdog_dev, 120000, 0);
-#endif
 }
 
 int board_init(void)
@@ -128,29 +113,10 @@ int board_init(void)
 void arch_preboot_os(void)
 {
 #ifdef CONFIG_WATCHDOG
-	wdt_stop(watchdog_dev);
+	wdt_stop(gd->watchdog_dev);
 #endif
 }
 
-#ifdef CONFIG_WATCHDOG
-void watchdog_reset(void)
-{
-	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
-
 static int led_7seg_init(unsigned int segments)
 {
 	int node;
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 5189925beb..c840e92d9c 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;
@@ -344,44 +340,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 1ad44bbe3f..150e8cd424 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;
 
@@ -677,6 +678,9 @@ static init_fnc_t init_sequence_r[] = {
 #ifdef CONFIG_DM
 	initr_dm,
 #endif
+#if 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/common/spl/spl.c b/common/spl/spl.c
index 88d4b8a9bf..0a6a47c202 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -22,6 +22,7 @@
 #include <linux/compiler.h>
 #include <fdt_support.h>
 #include <bootcount.h>
+#include <wdt.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -600,6 +601,10 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
 	spl_board_init();
 #endif
 
+#if defined(CONFIG_SPL_WATCHDOG_SUPPORT) && defined(CONFIG_WDT)
+	initr_watchdog();
+#endif
+
 	if (IS_ENABLED(CONFIG_SPL_OS_BOOT) || CONFIG_IS_ENABLED(HANDOFF))
 		dram_init_banksize();
 
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9d7f503b69..aa8e725573 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:
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..d16f50f677 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
+#if defined(CONFIG_WDT)
+	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 */
diff --git a/include/configs/turris_omnia.h b/include/configs/turris_omnia.h
index c7805cf36b..0e65a12345 100644
--- a/include/configs/turris_omnia.h
+++ b/include/configs/turris_omnia.h
@@ -29,11 +29,6 @@
 #define CONFIG_SPL_I2C_MUX
 #define CONFIG_SYS_I2C_MVTWSI
 
-/* Watchdog support */
-#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT_ORION)
-# define CONFIG_WATCHDOG
-#endif
-
 /*
  * SDIO/MMC Card Configuration
  */
diff --git a/include/wdt.h b/include/wdt.h
index e9a7c5355a..aa77d3e9b4 100644
--- a/include/wdt.h
+++ b/include/wdt.h
@@ -6,6 +6,9 @@
 #ifndef _WDT_H_
 #define _WDT_H_
 
+#include <dm.h>
+#include <dm/read.h>
+
 /*
  * Implement a simple watchdog uclass. Watchdog is basically a timer that
  * is used to detect or recover from malfunction. During normal operation
@@ -103,4 +106,42 @@ struct wdt_ops {
 	int (*expire_now)(struct udevice *dev, ulong flags);
 };
 
+#if defined(CONFIG_WDT)
+#ifndef CONFIG_WATCHDOG_TIMEOUT_MSECS
+#define CONFIG_WATCHDOG_TIMEOUT_MSECS	(60 * 1000)
+#endif
+#define WATCHDOG_TIMEOUT_SECS	(CONFIG_WATCHDOG_TIMEOUT_MSECS / 1000)
+
+static inline int initr_watchdog(void)
+{
+	u32 timeout = WATCHDOG_TIMEOUT_SECS;
+
+	/*
+	 * 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",
+					       WATCHDOG_TIMEOUT_SECS);
+	}
+
+	wdt_start(gd->watchdog_dev, timeout * 1000, 0);
+	gd->flags |= GD_FLG_WDT_READY;
+	printf("WDT:   Started with%s servicing (%ds timeout)\n",
+	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", timeout);
+
+	return 0;
+}
+#endif
+
 #endif  /* _WDT_H_ */
-- 
2.21.0

[U-Boot] [PATCH 2/4 v5] 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>
Reviewed-by: Michal Simek <michal.simek@xilinx.com>
Tested-by: Michal Simek <michal.simek@xilinx.com> (on zcu100)
---
v5:
- No Change

v4:
- No Change

v3:
- Divide timeout in _start() by 1000 to get value in seconds

v2:
- New patch

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

diff --git a/drivers/watchdog/cdns_wdt.c b/drivers/watchdog/cdns_wdt.c
index fc85fbcec2..6a608b6371 100644
--- a/drivers/watchdog/cdns_wdt.c
+++ b/drivers/watchdog/cdns_wdt.c
@@ -10,6 +10,7 @@
 #include <dm.h>
 #include <wdt.h>
 #include <clk.h>
+#include <div64.h>
 #include <linux/io.h>
 
 DECLARE_GLOBAL_DATA_PTR;
@@ -23,7 +24,6 @@ struct cdns_regs {
 
 struct cdns_wdt_priv {
 	bool rst;
-	u32 timeout;
 	struct cdns_regs *regs;
 };
 
@@ -142,10 +142,10 @@ 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;
-	}
+	/* Calculate timeout in seconds and restrict to min and max value */
+	do_div(timeout, 1000);
+	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 +235,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 3/4 v5] watchdog: mpc8xx_wdt: Watchdog driver and macros cleanup

[Stefan Roese]

With the generic watchdog driver now implemented, this patch removes
some legacy stuff from the MPC8xx watchdog driver and its Kconfig
integration. CONFIG_MPC8xx_WATCHDOG is completely removed and
hw_watchdog_reset() is made static, as the watchdog will now get
serviced via the DM infrastructure if enabled via CONFIG_WATCHDOG.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
---
v5:
- No change

v4:
- New patch

arch/powerpc/Kconfig            | 2 +-
 arch/powerpc/cpu/mpc8xx/Kconfig | 6 +++---
 drivers/watchdog/Kconfig        | 1 -
 drivers/watchdog/Makefile       | 2 +-
 drivers/watchdog/mpc8xx_wdt.c   | 4 +---
 5 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index c727d9162c..0b1629ba62 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -35,7 +35,7 @@ config MPC8xx
 	bool "MPC8xx"
 	select BOARD_EARLY_INIT_F
 	imply CMD_REGINFO
-	imply MPC8xx_WATCHDOG
+	imply WDT_MPC8xx
 
 endchoice
 
diff --git a/arch/powerpc/cpu/mpc8xx/Kconfig b/arch/powerpc/cpu/mpc8xx/Kconfig
index b0e90a0f20..3e8ea38529 100644
--- a/arch/powerpc/cpu/mpc8xx/Kconfig
+++ b/arch/powerpc/cpu/mpc8xx/Kconfig
@@ -25,9 +25,9 @@ config MPC885
 
 endchoice
 
-config MPC8xx_WATCHDOG
-	bool "Watchdog"
-	select HW_WATCHDOG
+#config MPC8xx_WATCHDOG
+#	bool "Watchdog"
+#	select HW_WATCHDOG
 
 config 8xx_GCLK_FREQ
 	int "CPU GCLK Frequency"
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index aa8e725573..3bce0aa0b8 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -151,7 +151,6 @@ config WDT_MT7621
 config WDT_MPC8xx
 	bool "MPC8xx watchdog timer support"
 	depends on WDT && MPC8xx
-	select CONFIG_MPC8xx_WATCHDOG
 	help
 	   Select this to enable mpc8xx watchdog timer
 
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index d901240ad1..40b2f4bc66 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -24,6 +24,6 @@ obj-$(CONFIG_WDT_BCM6345) += bcm6345_wdt.o
 obj-$(CONFIG_BCM2835_WDT)       += bcm2835_wdt.o
 obj-$(CONFIG_WDT_ORION) += orion_wdt.o
 obj-$(CONFIG_WDT_CDNS) += cdns_wdt.o
-obj-$(CONFIG_MPC8xx_WATCHDOG) += mpc8xx_wdt.o
+obj-$(CONFIG_WDT_MPC8xx) += mpc8xx_wdt.o
 obj-$(CONFIG_WDT_MT7621) += mt7621_wdt.o
 obj-$(CONFIG_WDT_MTK) += mtk_wdt.o
diff --git a/drivers/watchdog/mpc8xx_wdt.c b/drivers/watchdog/mpc8xx_wdt.c
index c24c2a9da6..675b62d8b3 100644
--- a/drivers/watchdog/mpc8xx_wdt.c
+++ b/drivers/watchdog/mpc8xx_wdt.c
@@ -10,7 +10,7 @@
 #include <asm/cpm_8xx.h>
 #include <asm/io.h>
 
-void hw_watchdog_reset(void)
+static void hw_watchdog_reset(void)
 {
 	immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR;
 
@@ -18,7 +18,6 @@ void hw_watchdog_reset(void)
 	out_be16(&immap->im_siu_conf.sc_swsr, 0xaa39);	/* write magic2 */
 }
 
-#ifdef CONFIG_WDT_MPC8xx
 static int mpc8xx_wdt_start(struct udevice *dev, u64 timeout, ulong flags)
 {
 	immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR;
@@ -66,4 +65,3 @@ U_BOOT_DRIVER(wdt_mpc8xx) = {
 	.of_match = mpc8xx_wdt_ids,
 	.ops = &mpc8xx_wdt_ops,
 };
-#endif /* CONFIG_WDT_MPC8xx */
-- 
2.21.0

[U-Boot] [PATCH 4/4 v5] watchdog: at91sam9_wdt: Remove now superfluous wdt start and reset

[Stefan Roese]

With the 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).

The watchdog servicing is enabled via CONFIG_WATCHDOG.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Heiko Schocher <hs@denx.de>
Cc: Eugen Hristev <eugen.hristev@microchip.com>
---
v5:
- No change

v4:
- New patch (as this at91 watchdog changes just hit mainline)

 arch/arm/mach-at91/clock.c                    | 45 -------------------
 arch/arm/mach-at91/include/mach/at91_wdt.h    |  2 -
 .../gardena-smart-gateway-at91sam_defconfig   |  2 -
 drivers/watchdog/at91sam9_wdt.c               |  8 ----
 4 files changed, 57 deletions(-)

diff --git a/arch/arm/mach-at91/clock.c b/arch/arm/mach-at91/clock.c
index 1d3df2c09d..8344daeb39 100644
--- a/arch/arm/mach-at91/clock.c
+++ b/arch/arm/mach-at91/clock.c
@@ -14,8 +14,6 @@
 
 #define EN_UPLL_TIMEOUT		500
 
-static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
-
 void at91_periph_clk_enable(int id)
 {
 	struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
@@ -123,46 +121,3 @@ void at91_pllicpr_init(u32 icpr)
 
 	writel(icpr, &pmc->pllicpr);
 }
-
-/* 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);
-	}
-}
-
-int arch_early_init_r(void)
-{
-	struct at91_wdt_priv *priv;
-
-	/* 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;
-		}
-	}
-
-	priv = dev_get_priv(watchdog_dev);
-	if (!priv) {
-		printf("Watchdog: priv not available!\n");
-		return 0;
-	}
-
-	wdt_start(watchdog_dev, priv->timeout * 1000, 0);
-	printf("Watchdog: Started\n");
-
-	return 0;
-}
diff --git a/arch/arm/mach-at91/include/mach/at91_wdt.h b/arch/arm/mach-at91/include/mach/at91_wdt.h
index a8fc73b3d1..8ef8e007d7 100644
--- a/arch/arm/mach-at91/include/mach/at91_wdt.h
+++ b/arch/arm/mach-at91/include/mach/at91_wdt.h
@@ -28,7 +28,6 @@ typedef struct at91_wdt {
 struct at91_wdt_priv {
 	void __iomem *regs;
 	u32 regval;
-	u32 timeout;
 };
 
 #endif
@@ -51,6 +50,5 @@ struct at91_wdt_priv {
 
 /* Hardware timeout in seconds */
 #define WDT_MAX_TIMEOUT		16
-#define WDT_DEFAULT_TIMEOUT	2
 
 #endif
diff --git a/configs/gardena-smart-gateway-at91sam_defconfig b/configs/gardena-smart-gateway-at91sam_defconfig
index b395a5a175..956897dc7f 100644
--- a/configs/gardena-smart-gateway-at91sam_defconfig
+++ b/configs/gardena-smart-gateway-at91sam_defconfig
@@ -23,7 +23,6 @@ CONFIG_USE_BOOTARGS=y
 CONFIG_BOOTARGS="console=ttyS0,115200 earlyprintk mtdparts=atmel_nand:256k(bootstrap)ro,768k(uboot)ro,256k(env_redundant),256k(env),512k(dtb),6M(kernel)ro,-(rootfs) rootfstype=ubifs ubi.mtd=6 root=ubi0:rootfs rw"
 CONFIG_SYS_CONSOLE_IS_IN_ENV=y
 CONFIG_SYS_CONSOLE_INFO_QUIET=y
-CONFIG_ARCH_EARLY_INIT_R=y
 CONFIG_SPL_SYS_MALLOC_SIMPLE=y
 CONFIG_SPL_SEPARATE_BSS=y
 # CONFIG_TPL_BANNER_PRINT is not set
@@ -76,7 +75,6 @@ CONFIG_TIMER=y
 CONFIG_SPL_TIMER=y
 CONFIG_ATMEL_PIT_TIMER=y
 # CONFIG_SYS_WHITE_ON_BLACK is not set
-CONFIG_WATCHDOG=y
 CONFIG_WDT=y
 CONFIG_WDT_AT91=y
 # CONFIG_UBIFS_SILENCE_MSG is not set
diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index 000769d46d..48433cc158 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -107,14 +107,6 @@ static int at91_wdt_probe(struct udevice *dev)
 	if (!priv->regs)
 		return -EINVAL;
 
-#if CONFIG_IS_ENABLED(OF_CONTROL)
-	priv->timeout = dev_read_u32_default(dev, "timeout-sec",
-					     WDT_DEFAULT_TIMEOUT);
-	debug("%s: timeout %d", __func__, priv->timeout);
-#else
-	priv->timeout = WDT_DEFAULT_TIMEOUT;
-#endif
-
 	debug("%s: Probing wdt%u\n", __func__, dev->seq);
 
 	return 0;
-- 
2.21.0

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

[Stefan Roese]

On 25.04.19 09:17, 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>
> Cc: Maxim Sloyko <maxims@google.com>
> Cc: Erik van Luijk <evanluijk@interact.nl>
> Cc: Ryder Lee <ryder.lee@mediatek.com>
> Cc: Weijie Gao <weijie.gao@mediatek.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: "Álvaro Fernández Rojas" <noltari@gmail.com>
> Cc: Philippe Reynes <philippe.reynes@softathome.com>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Reviewed-by: Michal Simek <michal.simek@xilinx.com>
> Tested-by: Michal Simek <michal.simek@xilinx.com> (on zcu100)

Applied to u-boot-marvell/master.

Thanks,
Stefan

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

[Stefan Roese]

On 25.04.19 09:17, 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>
> Reviewed-by: Michal Simek <michal.simek@xilinx.com>
> Tested-by: Michal Simek <michal.simek@xilinx.com> (on zcu100)

Applied to u-boot-marvell/master.

Thanks,
Stefan

[U-Boot] [PATCH 3/4 v5] watchdog: mpc8xx_wdt: Watchdog driver and macros cleanup

[Stefan Roese]

On 25.04.19 09:17, Stefan Roese wrote:
> With the generic watchdog driver now implemented, this patch removes
> some legacy stuff from the MPC8xx watchdog driver and its Kconfig
> integration. CONFIG_MPC8xx_WATCHDOG is completely removed and
> hw_watchdog_reset() is made static, as the watchdog will now get
> serviced via the DM infrastructure if enabled via CONFIG_WATCHDOG.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>

Applied to u-boot-marvell/master.

Thanks,
Stefan

[U-Boot] [PATCH 4/4 v5] watchdog: at91sam9_wdt: Remove now superfluous wdt start and reset

[Stefan Roese]

On 25.04.19 09:17, Stefan Roese wrote:
> With the 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).
> 
> The watchdog servicing is enabled via CONFIG_WATCHDOG.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Heiko Schocher <hs@denx.de>
> Cc: Eugen Hristev <eugen.hristev@microchip.com>

Applied to u-boot-marvell/master.

Thanks,
Stefan

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

[Simon Goldschmidt]

Hi Stefan,

Am 25.04.2019 um 09:17 schrieb 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>

<snip>

> --- 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
> +#if defined(CONFIG_WDT)
> +	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	   */

Sorry to warm up a thread that is more than 4 months old, but I just 
stumbled accross this line when searching for space in 'gd':

The comment some lines above in global_data.h clearly states that the 
top 16 bits of flags are reserved for arch-specific flags, and your 
patch here uses the lowest of these 16 arch-specific flags for generic code.

Is this a problem? Does any arch code use the upper 16 bits? I would 
have thought you'd at least need to adjust the comment to reflect your 
new usage...

The reason I ask is that I'd need a place to put some (~5?) 
'is_initialized' bits for some code running in SPL in the 'board_init_f' 
code where BSS shouldn't be used. gd->flags would be ideal for that, but 
I'm hesistant to dive in further into the 'arch-specific' upper 16 bits...

Regards,
Simon

>   
>   #endif /* __ASM_GENERIC_GBL_DATA_H */
> diff --git a/include/configs/turris_omnia.h b/include/configs/turris_omnia.h
> index c7805cf36b..0e65a12345 100644
> --- a/include/configs/turris_omnia.h
> +++ b/include/configs/turris_omnia.h
> @@ -29,11 +29,6 @@
>   #define CONFIG_SPL_I2C_MUX
>   #define CONFIG_SYS_I2C_MVTWSI
>   
> -/* Watchdog support */
> -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT_ORION)
> -# define CONFIG_WATCHDOG
> -#endif
> -
>   /*
>    * SDIO/MMC Card Configuration
>    */
> diff --git a/include/wdt.h b/include/wdt.h
> index e9a7c5355a..aa77d3e9b4 100644
> --- a/include/wdt.h
> +++ b/include/wdt.h
> @@ -6,6 +6,9 @@
>   #ifndef _WDT_H_
>   #define _WDT_H_
>   
> +#include <dm.h>
> +#include <dm/read.h>
> +
>   /*
>    * Implement a simple watchdog uclass. Watchdog is basically a timer that
>    * is used to detect or recover from malfunction. During normal operation
> @@ -103,4 +106,42 @@ struct wdt_ops {
>   	int (*expire_now)(struct udevice *dev, ulong flags);
>   };
>   
> +#if defined(CONFIG_WDT)
> +#ifndef CONFIG_WATCHDOG_TIMEOUT_MSECS
> +#define CONFIG_WATCHDOG_TIMEOUT_MSECS	(60 * 1000)
> +#endif
> +#define WATCHDOG_TIMEOUT_SECS	(CONFIG_WATCHDOG_TIMEOUT_MSECS / 1000)
> +
> +static inline int initr_watchdog(void)
> +{
> +	u32 timeout = WATCHDOG_TIMEOUT_SECS;
> +
> +	/*
> +	 * 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",
> +					       WATCHDOG_TIMEOUT_SECS);
> +	}
> +
> +	wdt_start(gd->watchdog_dev, timeout * 1000, 0);
> +	gd->flags |= GD_FLG_WDT_READY;
> +	printf("WDT:   Started with%s servicing (%ds timeout)\n",
> +	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", timeout);
> +
> +	return 0;
> +}
> +#endif
> +
>   #endif  /* _WDT_H_ */
> 

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

[Stefan Roese]

Hi Simon,

(added Simon Glass and Bin to Cc)

On 13.08.19 22:16, Simon Goldschmidt wrote:
> Am 25.04.2019 um 09:17 schrieb 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>
> 
> <snip>
> 
>> --- 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
>> +#if defined(CONFIG_WDT)
>> +	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	   */
> 
> Sorry to warm up a thread that is more than 4 months old, but I just
> stumbled accross this line when searching for space in 'gd':
> 
> The comment some lines above in global_data.h clearly states that the
> top 16 bits of flags are reserved for arch-specific flags, and your
> patch here uses the lowest of these 16 arch-specific flags for generic code.

I totally missed this comment.
  
> Is this a problem? Does any arch code use the upper 16 bits? I would
> have thought you'd at least need to adjust the comment to reflect your
> new usage...

As stated above, I did not check about any other (arch-specific)
GD_FLG_ definitions outside of this file.
  
> The reason I ask is that I'd need a place to put some (~5?)
> 'is_initialized' bits for some code running in SPL in the 'board_init_f'
> code where BSS shouldn't be used. gd->flags would be ideal for that, but
> I'm hesistant to dive in further into the 'arch-specific' upper 16 bits...

And you should be. A quick grep shows that we already have a problem with
my patch touching the upper bits:

$ git grep "define GD_FLG_"
arch/x86/include/asm/global_data.h:#define GD_FLG_COLD_BOOT     0x10000 /* Cold Boot */
arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT     0x20000 /* Warm Boot */

This should definitely be fixed. I see 3 options right now:

a) Reserve only the upper 8 bits for arch-specific stuff
b) Use a new variable (gd->flags_arch ?) for this arch
c) Remove the arch-specific GD_FLG's completely

I can't tell if c) is doable - Bin and / or Simon Glass might know,
if the x86 GD_FLG_foo_BOOT are really needed in gd->flags. I see that
both are assigned in the .S files, but only GD_FLG_COLD_BOOT is
referenced later on:

arch/x86/cpu/coreboot/coreboot.c:       if (gd->flags & GD_FLG_COLD_BOOT)

If c) is not an option, then I would prefer to implement b). Here
we could also only add this new "flags_arch" variable for arch's
that implement such flags (e.g. x86 right now). I could work on such
a patch, if we agree on this solution.

Any comments / suggestions?

Thanks,
Stefan

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

[Simon Glass]

Hi,

On Wed, 14 Aug 2019 at 00:22, Stefan Roese <sr@denx.de> wrote:
>
> Hi Simon,
>
> (added Simon Glass and Bin to Cc)
>
> On 13.08.19 22:16, Simon Goldschmidt wrote:
> > Am 25.04.2019 um 09:17 schrieb 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>
> >
> > <snip>
> >
> >> --- 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
> >> +#if defined(CONFIG_WDT)
> >> +    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       */
> >
> > Sorry to warm up a thread that is more than 4 months old, but I just
> > stumbled accross this line when searching for space in 'gd':
> >
> > The comment some lines above in global_data.h clearly states that the
> > top 16 bits of flags are reserved for arch-specific flags, and your
> > patch here uses the lowest of these 16 arch-specific flags for generic code.
>
> I totally missed this comment.
>
> > Is this a problem? Does any arch code use the upper 16 bits? I would
> > have thought you'd at least need to adjust the comment to reflect your
> > new usage...
>
> As stated above, I did not check about any other (arch-specific)
> GD_FLG_ definitions outside of this file.
>
> > The reason I ask is that I'd need a place to put some (~5?)
> > 'is_initialized' bits for some code running in SPL in the 'board_init_f'
> > code where BSS shouldn't be used. gd->flags would be ideal for that, but
> > I'm hesistant to dive in further into the 'arch-specific' upper 16 bits...
>
> And you should be. A quick grep shows that we already have a problem with
> my patch touching the upper bits:
>
> $ git grep "define GD_FLG_"
> arch/x86/include/asm/global_data.h:#define GD_FLG_COLD_BOOT     0x10000 /* Cold Boot */
> arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT     0x20000 /* Warm Boot */
>
> This should definitely be fixed. I see 3 options right now:
>
> a) Reserve only the upper 8 bits for arch-specific stuff
> b) Use a new variable (gd->flags_arch ?) for this arch
> c) Remove the arch-specific GD_FLG's completely
>
> I can't tell if c) is doable - Bin and / or Simon Glass might know,
> if the x86 GD_FLG_foo_BOOT are really needed in gd->flags. I see that
> both are assigned in the .S files, but only GD_FLG_COLD_BOOT is
> referenced later on:

Probably we can drop warm boot.

>
> arch/x86/cpu/coreboot/coreboot.c:       if (gd->flags & GD_FLG_COLD_BOOT)
>
> If c) is not an option, then I would prefer to implement b). Here
> we could also only add this new "flags_arch" variable for arch's
> that implement such flags (e.g. x86 right now). I could work on such
> a patch, if we agree on this solution.
>
> Any comments / suggestions?

I'm not keen on arch-specific flags here. They should be in
gd->arch... if needed. global_data is current used by generic code.

So maybe move the x86 flag to the generic header?

Regards,
Simon

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

[Stefan Roese]

Hi Simon,

On 14.08.19 21:35, Simon Glass wrote:
> Hi,
> 
> On Wed, 14 Aug 2019 at 00:22, Stefan Roese <sr@denx.de> wrote:
>>
>> Hi Simon,
>>
>> (added Simon Glass and Bin to Cc)
>>
>> On 13.08.19 22:16, Simon Goldschmidt wrote:
>>> Am 25.04.2019 um 09:17 schrieb 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>
>>>
>>> <snip>
>>>
>>>> --- 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
>>>> +#if defined(CONFIG_WDT)
>>>> +    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       */
>>>
>>> Sorry to warm up a thread that is more than 4 months old, but I just
>>> stumbled accross this line when searching for space in 'gd':
>>>
>>> The comment some lines above in global_data.h clearly states that the
>>> top 16 bits of flags are reserved for arch-specific flags, and your
>>> patch here uses the lowest of these 16 arch-specific flags for generic code.
>>
>> I totally missed this comment.
>>
>>> Is this a problem? Does any arch code use the upper 16 bits? I would
>>> have thought you'd at least need to adjust the comment to reflect your
>>> new usage...
>>
>> As stated above, I did not check about any other (arch-specific)
>> GD_FLG_ definitions outside of this file.
>>
>>> The reason I ask is that I'd need a place to put some (~5?)
>>> 'is_initialized' bits for some code running in SPL in the 'board_init_f'
>>> code where BSS shouldn't be used. gd->flags would be ideal for that, but
>>> I'm hesistant to dive in further into the 'arch-specific' upper 16 bits...
>>
>> And you should be. A quick grep shows that we already have a problem with
>> my patch touching the upper bits:
>>
>> $ git grep "define GD_FLG_"
>> arch/x86/include/asm/global_data.h:#define GD_FLG_COLD_BOOT     0x10000 /* Cold Boot */
>> arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT     0x20000 /* Warm Boot */
>>
>> This should definitely be fixed. I see 3 options right now:
>>
>> a) Reserve only the upper 8 bits for arch-specific stuff
>> b) Use a new variable (gd->flags_arch ?) for this arch
>> c) Remove the arch-specific GD_FLG's completely
>>
>> I can't tell if c) is doable - Bin and / or Simon Glass might know,
>> if the x86 GD_FLG_foo_BOOT are really needed in gd->flags. I see that
>> both are assigned in the .S files, but only GD_FLG_COLD_BOOT is
>> referenced later on:
> 
> Probably we can drop warm boot.

Bin, do you think so as well?

>>
>> arch/x86/cpu/coreboot/coreboot.c:       if (gd->flags & GD_FLG_COLD_BOOT)
>>
>> If c) is not an option, then I would prefer to implement b). Here
>> we could also only add this new "flags_arch" variable for arch's
>> that implement such flags (e.g. x86 right now). I could work on such
>> a patch, if we agree on this solution.
>>
>> Any comments / suggestions?
> 
> I'm not keen on arch-specific flags here. They should be in
> gd->arch... if needed. global_data is current used by generic code.
> 
> So maybe move the x86 flag to the generic header?

Yes, I also think that those flags should not be sprinkled in different
headers but collected in the generic header. I'll prepare a patch to
move the x86 flags and remove the comment about the upper 16 bits
usage. We can remove the x86 warm boot flag later, if this isn't
used at all.

Thanks,
Stefan

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

[Bin Meng]

Hi Stefan,

On Thu, Aug 15, 2019 at 2:07 PM Stefan Roese <sr@denx.de> wrote:
>
> Hi Simon,
>
> On 14.08.19 21:35, Simon Glass wrote:
> > Hi,
> >
> > On Wed, 14 Aug 2019 at 00:22, Stefan Roese <sr@denx.de> wrote:
> >>
> >> Hi Simon,
> >>
> >> (added Simon Glass and Bin to Cc)
> >>
> >> On 13.08.19 22:16, Simon Goldschmidt wrote:
> >>> Am 25.04.2019 um 09:17 schrieb 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>
> >>>
> >>> <snip>
> >>>
> >>>> --- 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
> >>>> +#if defined(CONFIG_WDT)
> >>>> +    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       */
> >>>
> >>> Sorry to warm up a thread that is more than 4 months old, but I just
> >>> stumbled accross this line when searching for space in 'gd':
> >>>
> >>> The comment some lines above in global_data.h clearly states that the
> >>> top 16 bits of flags are reserved for arch-specific flags, and your
> >>> patch here uses the lowest of these 16 arch-specific flags for generic code.
> >>
> >> I totally missed this comment.
> >>
> >>> Is this a problem? Does any arch code use the upper 16 bits? I would
> >>> have thought you'd at least need to adjust the comment to reflect your
> >>> new usage...
> >>
> >> As stated above, I did not check about any other (arch-specific)
> >> GD_FLG_ definitions outside of this file.
> >>
> >>> The reason I ask is that I'd need a place to put some (~5?)
> >>> 'is_initialized' bits for some code running in SPL in the 'board_init_f'
> >>> code where BSS shouldn't be used. gd->flags would be ideal for that, but
> >>> I'm hesistant to dive in further into the 'arch-specific' upper 16 bits...
> >>
> >> And you should be. A quick grep shows that we already have a problem with
> >> my patch touching the upper bits:
> >>
> >> $ git grep "define GD_FLG_"
> >> arch/x86/include/asm/global_data.h:#define GD_FLG_COLD_BOOT     0x10000 /* Cold Boot */
> >> arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT     0x20000 /* Warm Boot */
> >>
> >> This should definitely be fixed. I see 3 options right now:
> >>
> >> a) Reserve only the upper 8 bits for arch-specific stuff
> >> b) Use a new variable (gd->flags_arch ?) for this arch
> >> c) Remove the arch-specific GD_FLG's completely
> >>
> >> I can't tell if c) is doable - Bin and / or Simon Glass might know,
> >> if the x86 GD_FLG_foo_BOOT are really needed in gd->flags. I see that
> >> both are assigned in the .S files, but only GD_FLG_COLD_BOOT is
> >> referenced later on:
> >
> > Probably we can drop warm boot.
>
> Bin, do you think so as well?
>

I believe we can drop these 2 flags completely. Currently usage of
warm/cold boot flags is only limited to coreboot codes.

arch/x86/cpu/coreboot/coreboot.c::last_stage_init()

        if (gd->flags & GD_FLG_COLD_BOOT)
                timestamp_add_to_bootstage();

timestamp_add_to_bootstage() will never be called for coreboot.

> >>
> >> arch/x86/cpu/coreboot/coreboot.c:       if (gd->flags & GD_FLG_COLD_BOOT)
> >>
> >> If c) is not an option, then I would prefer to implement b). Here
> >> we could also only add this new "flags_arch" variable for arch's
> >> that implement such flags (e.g. x86 right now). I could work on such
> >> a patch, if we agree on this solution.
> >>
> >> Any comments / suggestions?
> >
> > I'm not keen on arch-specific flags here. They should be in
> > gd->arch... if needed. global_data is current used by generic code.
> >
> > So maybe move the x86 flag to the generic header?
>
> Yes, I also think that those flags should not be sprinkled in different
> headers but collected in the generic header. I'll prepare a patch to
> move the x86 flags and remove the comment about the upper 16 bits
> usage. We can remove the x86 warm boot flag later, if this isn't
> used at all.
>

Regards,
Bin

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

[Stefan Roese]

Hi Bin,

On 15.08.19 16:19, Bin Meng wrote:
> Hi Stefan,
> 
> On Thu, Aug 15, 2019 at 2:07 PM Stefan Roese <sr@denx.de> wrote:
>>
>> Hi Simon,
>>
>> On 14.08.19 21:35, Simon Glass wrote:
>>> Hi,
>>>
>>> On Wed, 14 Aug 2019 at 00:22, Stefan Roese <sr@denx.de> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> (added Simon Glass and Bin to Cc)
>>>>
>>>> On 13.08.19 22:16, Simon Goldschmidt wrote:
>>>>> Am 25.04.2019 um 09:17 schrieb 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>
>>>>>
>>>>> <snip>
>>>>>
>>>>>> --- 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
>>>>>> +#if defined(CONFIG_WDT)
>>>>>> +    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       */
>>>>>
>>>>> Sorry to warm up a thread that is more than 4 months old, but I just
>>>>> stumbled accross this line when searching for space in 'gd':
>>>>>
>>>>> The comment some lines above in global_data.h clearly states that the
>>>>> top 16 bits of flags are reserved for arch-specific flags, and your
>>>>> patch here uses the lowest of these 16 arch-specific flags for generic code.
>>>>
>>>> I totally missed this comment.
>>>>
>>>>> Is this a problem? Does any arch code use the upper 16 bits? I would
>>>>> have thought you'd at least need to adjust the comment to reflect your
>>>>> new usage...
>>>>
>>>> As stated above, I did not check about any other (arch-specific)
>>>> GD_FLG_ definitions outside of this file.
>>>>
>>>>> The reason I ask is that I'd need a place to put some (~5?)
>>>>> 'is_initialized' bits for some code running in SPL in the 'board_init_f'
>>>>> code where BSS shouldn't be used. gd->flags would be ideal for that, but
>>>>> I'm hesistant to dive in further into the 'arch-specific' upper 16 bits...
>>>>
>>>> And you should be. A quick grep shows that we already have a problem with
>>>> my patch touching the upper bits:
>>>>
>>>> $ git grep "define GD_FLG_"
>>>> arch/x86/include/asm/global_data.h:#define GD_FLG_COLD_BOOT     0x10000 /* Cold Boot */
>>>> arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT     0x20000 /* Warm Boot */
>>>>
>>>> This should definitely be fixed. I see 3 options right now:
>>>>
>>>> a) Reserve only the upper 8 bits for arch-specific stuff
>>>> b) Use a new variable (gd->flags_arch ?) for this arch
>>>> c) Remove the arch-specific GD_FLG's completely
>>>>
>>>> I can't tell if c) is doable - Bin and / or Simon Glass might know,
>>>> if the x86 GD_FLG_foo_BOOT are really needed in gd->flags. I see that
>>>> both are assigned in the .S files, but only GD_FLG_COLD_BOOT is
>>>> referenced later on:
>>>
>>> Probably we can drop warm boot.
>>
>> Bin, do you think so as well?
>>
> 
> I believe we can drop these 2 flags completely. Currently usage of
> warm/cold boot flags is only limited to coreboot codes.
> 
> arch/x86/cpu/coreboot/coreboot.c::last_stage_init()
> 
>          if (gd->flags & GD_FLG_COLD_BOOT)
>                  timestamp_add_to_bootstage();
> 
> timestamp_add_to_bootstage() will never be called for coreboot.

Why is this the case? Will GD_FLG_COLD_BOOT never be set for the
coreboot target?

Thanks,
Stefan

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

[Bin Meng]

Hi Stefan,

On Fri, Aug 16, 2019 at 1:11 PM Stefan Roese <sr@denx.de> wrote:
>
> Hi Bin,
>
> On 15.08.19 16:19, Bin Meng wrote:
> > Hi Stefan,
> >
> > On Thu, Aug 15, 2019 at 2:07 PM Stefan Roese <sr@denx.de> wrote:
> >>
> >> Hi Simon,
> >>
> >> On 14.08.19 21:35, Simon Glass wrote:
> >>> Hi,
> >>>
> >>> On Wed, 14 Aug 2019 at 00:22, Stefan Roese <sr@denx.de> wrote:
> >>>>
> >>>> Hi Simon,
> >>>>
> >>>> (added Simon Glass and Bin to Cc)
> >>>>
> >>>> On 13.08.19 22:16, Simon Goldschmidt wrote:
> >>>>> Am 25.04.2019 um 09:17 schrieb 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>
> >>>>>
> >>>>> <snip>
> >>>>>
> >>>>>> --- 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
> >>>>>> +#if defined(CONFIG_WDT)
> >>>>>> +    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       */
> >>>>>
> >>>>> Sorry to warm up a thread that is more than 4 months old, but I just
> >>>>> stumbled accross this line when searching for space in 'gd':
> >>>>>
> >>>>> The comment some lines above in global_data.h clearly states that the
> >>>>> top 16 bits of flags are reserved for arch-specific flags, and your
> >>>>> patch here uses the lowest of these 16 arch-specific flags for generic code.
> >>>>
> >>>> I totally missed this comment.
> >>>>
> >>>>> Is this a problem? Does any arch code use the upper 16 bits? I would
> >>>>> have thought you'd at least need to adjust the comment to reflect your
> >>>>> new usage...
> >>>>
> >>>> As stated above, I did not check about any other (arch-specific)
> >>>> GD_FLG_ definitions outside of this file.
> >>>>
> >>>>> The reason I ask is that I'd need a place to put some (~5?)
> >>>>> 'is_initialized' bits for some code running in SPL in the 'board_init_f'
> >>>>> code where BSS shouldn't be used. gd->flags would be ideal for that, but
> >>>>> I'm hesistant to dive in further into the 'arch-specific' upper 16 bits...
> >>>>
> >>>> And you should be. A quick grep shows that we already have a problem with
> >>>> my patch touching the upper bits:
> >>>>
> >>>> $ git grep "define GD_FLG_"
> >>>> arch/x86/include/asm/global_data.h:#define GD_FLG_COLD_BOOT     0x10000 /* Cold Boot */
> >>>> arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT     0x20000 /* Warm Boot */
> >>>>
> >>>> This should definitely be fixed. I see 3 options right now:
> >>>>
> >>>> a) Reserve only the upper 8 bits for arch-specific stuff
> >>>> b) Use a new variable (gd->flags_arch ?) for this arch
> >>>> c) Remove the arch-specific GD_FLG's completely
> >>>>
> >>>> I can't tell if c) is doable - Bin and / or Simon Glass might know,
> >>>> if the x86 GD_FLG_foo_BOOT are really needed in gd->flags. I see that
> >>>> both are assigned in the .S files, but only GD_FLG_COLD_BOOT is
> >>>> referenced later on:
> >>>
> >>> Probably we can drop warm boot.
> >>
> >> Bin, do you think so as well?
> >>
> >
> > I believe we can drop these 2 flags completely. Currently usage of
> > warm/cold boot flags is only limited to coreboot codes.
> >
> > arch/x86/cpu/coreboot/coreboot.c::last_stage_init()
> >
> >          if (gd->flags & GD_FLG_COLD_BOOT)
> >                  timestamp_add_to_bootstage();
> >
> > timestamp_add_to_bootstage() will never be called for coreboot.
>
> Why is this the case? Will GD_FLG_COLD_BOOT never be set for the
> coreboot target?

GD_FLG_COLD_BOOT is only set in the 16-bit start code while on
coreboot it boots directly from the 32-bit start code.

Regards,
Bin

[PATCH 3/4 v5] watchdog: mpc8xx_wdt: Watchdog driver and macros cleanup

[Christophe Leroy]

Hi Stefan,

On 04/25/2019 07:17 AM, Stefan Roese wrote:
> With the generic watchdog driver now implemented, this patch removes
> some legacy stuff from the MPC8xx watchdog driver and its Kconfig
> integration. CONFIG_MPC8xx_WATCHDOG is completely removed and
> hw_watchdog_reset() is made static, as the watchdog will now get
> serviced via the DM infrastructure if enabled via CONFIG_WATCHDOG.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> v5:
> - No change
> 
> v4:
> - New patch
> 
> arch/powerpc/Kconfig            | 2 +-
>   arch/powerpc/cpu/mpc8xx/Kconfig | 6 +++---
>   drivers/watchdog/Kconfig        | 1 -
>   drivers/watchdog/Makefile       | 2 +-
>   drivers/watchdog/mpc8xx_wdt.c   | 4 +---
>   5 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index c727d9162c..0b1629ba62 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -35,7 +35,7 @@ config MPC8xx
>   	bool "MPC8xx"
>   	select BOARD_EARLY_INIT_F
>   	imply CMD_REGINFO
> -	imply MPC8xx_WATCHDOG
> +	imply WDT_MPC8xx
>   
>   endchoice
>   
> diff --git a/arch/powerpc/cpu/mpc8xx/Kconfig b/arch/powerpc/cpu/mpc8xx/Kconfig
> index b0e90a0f20..3e8ea38529 100644
> --- a/arch/powerpc/cpu/mpc8xx/Kconfig
> +++ b/arch/powerpc/cpu/mpc8xx/Kconfig
> @@ -25,9 +25,9 @@ config MPC885
>   
>   endchoice
>   
> -config MPC8xx_WATCHDOG
> -	bool "Watchdog"
> -	select HW_WATCHDOG
> +#config MPC8xx_WATCHDOG
> +#	bool "Watchdog"
> +#	select HW_WATCHDOG

If HW_WATCHDOG is not selected, we have a problem in cpu_init_f() 
(arch/powerpc/cpu/mpc8xx/cpu_init.c) with the following line, and the 
board with restart every second.

#ifndef CONFIG_HW_WATCHDOG
	/* deactivate watchdog if not enabled in config */
	out_be32(&immr->im_siu_conf.sc_sypcr, CONFIG_SYS_SYPCR & ~SYPCR_SWE);
#endif

Should this be changed to use CONFIG_WATCHDOG instead ?


>   
>   config 8xx_GCLK_FREQ
>   	int "CPU GCLK Frequency"
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index aa8e725573..3bce0aa0b8 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -151,7 +151,6 @@ config WDT_MT7621
>   config WDT_MPC8xx
>   	bool "MPC8xx watchdog timer support"
>   	depends on WDT && MPC8xx
> -	select CONFIG_MPC8xx_WATCHDOG

This is brought back by a later patch from Patrice. Unwanted I guess ?

Christophe

[PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version

[Christophe Leroy]

Le 25/04/2019 ? 09:17, Stefan Roese a ?crit?:
> 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>
> Cc: Maxim Sloyko <maxims@google.com>
> Cc: Erik van Luijk <evanluijk@interact.nl>
> Cc: Ryder Lee <ryder.lee@mediatek.com>
> Cc: Weijie Gao <weijie.gao@mediatek.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: "?lvaro Fern?ndez Rojas" <noltari@gmail.com>
> Cc: Philippe Reynes <philippe.reynes@softathome.com>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Reviewed-by: Michal Simek <michal.simek@xilinx.com>
> Tested-by: Michal Simek <michal.simek@xilinx.com> (on zcu100)
> ---

[...]

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

This is a problem for the MPC8xx.

When running with a MPC8xx at 132MHz clock, the watchdog will fire about 
1s after the last refresh. So the above makes the board unusable.

Is that really necessary to restrict the refresh like this in the core 
part of Watchdog driver ?

> +}
> +#endif
> +
>   static int wdt_post_bind(struct udevice *dev)
>   {
>   #if defined(CONFIG_NEEDS_MANUAL_RELOC)

Thanks,
Christophe

[PATCH 3/4 v5] watchdog: mpc8xx_wdt: Watchdog driver and macros cleanup

[Stefan Roese]

Hi Christophe,

On 19.02.20 20:15, Christophe Leroy wrote:
> Hi Stefan,
> 
> On 04/25/2019 07:17 AM, Stefan Roese wrote:
>> With the generic watchdog driver now implemented, this patch removes
>> some legacy stuff from the MPC8xx watchdog driver and its Kconfig
>> integration. CONFIG_MPC8xx_WATCHDOG is completely removed and
>> hw_watchdog_reset() is made static, as the watchdog will now get
>> serviced via the DM infrastructure if enabled via CONFIG_WATCHDOG.
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> v5:
>> - No change
>>
>> v4:
>> - New patch
>>
>> arch/powerpc/Kconfig??????????? | 2 +-
>> ? arch/powerpc/cpu/mpc8xx/Kconfig | 6 +++---
>> ? drivers/watchdog/Kconfig??????? | 1 -
>> ? drivers/watchdog/Makefile?????? | 2 +-
>> ? drivers/watchdog/mpc8xx_wdt.c?? | 4 +---
>> ? 5 files changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index c727d9162c..0b1629ba62 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -35,7 +35,7 @@ config MPC8xx
>> ????? bool "MPC8xx"
>> ????? select BOARD_EARLY_INIT_F
>> ????? imply CMD_REGINFO
>> -??? imply MPC8xx_WATCHDOG
>> +??? imply WDT_MPC8xx
>> ? endchoice
>> diff --git a/arch/powerpc/cpu/mpc8xx/Kconfig 
>> b/arch/powerpc/cpu/mpc8xx/Kconfig
>> index b0e90a0f20..3e8ea38529 100644
>> --- a/arch/powerpc/cpu/mpc8xx/Kconfig
>> +++ b/arch/powerpc/cpu/mpc8xx/Kconfig
>> @@ -25,9 +25,9 @@ config MPC885
>> ? endchoice
>> -config MPC8xx_WATCHDOG
>> -??? bool "Watchdog"
>> -??? select HW_WATCHDOG
>> +#config MPC8xx_WATCHDOG
>> +#??? bool "Watchdog"
>> +#??? select HW_WATCHDOG
> 
> If HW_WATCHDOG is not selected, we have a problem in cpu_init_f() 
> (arch/powerpc/cpu/mpc8xx/cpu_init.c) with the following line, and the 
> board with restart every second.
> 
> #ifndef CONFIG_HW_WATCHDOG
>  ????/* deactivate watchdog if not enabled in config */
>  ????out_be32(&immr->im_siu_conf.sc_sypcr, CONFIG_SYS_SYPCR & ~SYPCR_SWE);
> #endif
> 
> Should this be changed to use CONFIG_WATCHDOG instead ?

Yes, that sound reasonable.

Thanks,
Stefan

[PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version

[Stefan Roese]

Hi Christophe

On 19.02.20 20:21, Christophe Leroy wrote:
> 
> 
> Le 25/04/2019 ? 09:17, Stefan Roese a ?crit?:
>> 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>
>> Cc: Maxim Sloyko <maxims@google.com>
>> Cc: Erik van Luijk <evanluijk@interact.nl>
>> Cc: Ryder Lee <ryder.lee@mediatek.com>
>> Cc: Weijie Gao <weijie.gao@mediatek.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: "?lvaro Fern?ndez Rojas" <noltari@gmail.com>
>> Cc: Philippe Reynes <philippe.reynes@softathome.com>
>> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
>> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> Reviewed-by: Michal Simek <michal.simek@xilinx.com>
>> Tested-by: Michal Simek <michal.simek@xilinx.com> (on zcu100)
>> ---
> 
> [...]
> 
>> 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);
>> +??? }
> 
> This is a problem for the MPC8xx.
> 
> When running with a MPC8xx at 132MHz clock, the watchdog will fire about 
> 1s after the last refresh. So the above makes the board unusable.

So you need a shorted delay between the wdt_reset() calls? Is this
correct? We could introduce a new Kconfig option which defaults to
1000 (ms) and you can "select" a shorter value for MPC8xx.

Would this work for you?

> Is that really necessary to restrict the refresh like this in the core 
> part of Watchdog driver ?

It seems to work for most cases (all until I hear of this MPC8xx issue).

Thanks,
Stefan

[PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version

[Christophe Leroy]

On 02/20/2020 05:43 AM, Stefan Roese wrote:
> Hi Christophe
> 
> On 19.02.20 20:21, Christophe Leroy wrote:
>>
>>
>> Le 25/04/2019 ? 09:17, Stefan Roese a ?crit?:
>>> 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>
>>> Cc: Maxim Sloyko <maxims@google.com>
>>> Cc: Erik van Luijk <evanluijk@interact.nl>
>>> Cc: Ryder Lee <ryder.lee@mediatek.com>
>>> Cc: Weijie Gao <weijie.gao@mediatek.com>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Cc: "?lvaro Fern?ndez Rojas" <noltari@gmail.com>
>>> Cc: Philippe Reynes <philippe.reynes@softathome.com>
>>> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
>>> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>> Reviewed-by: Michal Simek <michal.simek@xilinx.com>
>>> Tested-by: Michal Simek <michal.simek@xilinx.com> (on zcu100)
>>> ---
>>
>> [...]
>>
>>> 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);
>>> +??? }
>>
>> This is a problem for the MPC8xx.
>>
>> When running with a MPC8xx at 132MHz clock, the watchdog will fire 
>> about 1s after the last refresh. So the above makes the board unusable.
> 
> So you need a shorted delay between the wdt_reset() calls? Is this
> correct? We could introduce a new Kconfig option which defaults to
> 1000 (ms) and you can "select" a shorter value for MPC8xx.

Exactly. However, why is this limitation needed at all ? Why is it a 
problem to refresh more often ?


Christophe

[PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version

[Stefan Roese]

On 20.02.20 07:38, Christophe Leroy wrote:

<snip>

>>>> +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);
>>>> +??? }
>>>
>>> This is a problem for the MPC8xx.
>>>
>>> When running with a MPC8xx at 132MHz clock, the watchdog will fire 
>>> about 1s after the last refresh. So the above makes the board unusable.
>>
>> So you need a shorted delay between the wdt_reset() calls? Is this
>> correct? We could introduce a new Kconfig option which defaults to
>> 1000 (ms) and you can "select" a shorter value for MPC8xx.
> 
> Exactly. However, why is this limitation needed at all ? Why is it a 
> problem to refresh more often ?

Very likely its not. What is a reasonable value for your platform? 100
or 500ms? I think we could change it to default to a shorter value, but
such a change should go in early in the merge window, so that other
platforms have a bit of time to test it.

Please feel free to send a patch for this and please add a comment to
explain, why the delay is this "short".

Thanks,
Stefan

[PATCH 3/4 v5] watchdog: mpc8xx_wdt: Watchdog driver and macros cleanup

[Christophe Leroy]

On 02/20/2020 05:37 AM, Stefan Roese wrote:
> Hi Christophe,
> 
> On 19.02.20 20:15, Christophe Leroy wrote:
>> Hi Stefan,
>>
>> On 04/25/2019 07:17 AM, Stefan Roese wrote:
>>> With the generic watchdog driver now implemented, this patch removes
>>> some legacy stuff from the MPC8xx watchdog driver and its Kconfig
>>> integration. CONFIG_MPC8xx_WATCHDOG is completely removed and
>>> hw_watchdog_reset() is made static, as the watchdog will now get
>>> serviced via the DM infrastructure if enabled via CONFIG_WATCHDOG.
>>>
>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
>>> ---
>>> v5:
>>> - No change
>>>
>>> v4:
>>> - New patch
>>>
>>> arch/powerpc/Kconfig??????????? | 2 +-
>>> ? arch/powerpc/cpu/mpc8xx/Kconfig | 6 +++---
>>> ? drivers/watchdog/Kconfig??????? | 1 -
>>> ? drivers/watchdog/Makefile?????? | 2 +-
>>> ? drivers/watchdog/mpc8xx_wdt.c?? | 4 +---
>>> ? 5 files changed, 6 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>> index c727d9162c..0b1629ba62 100644
>>> --- a/arch/powerpc/Kconfig
>>> +++ b/arch/powerpc/Kconfig
>>> @@ -35,7 +35,7 @@ config MPC8xx
>>> ????? bool "MPC8xx"
>>> ????? select BOARD_EARLY_INIT_F
>>> ????? imply CMD_REGINFO
>>> -??? imply MPC8xx_WATCHDOG
>>> +??? imply WDT_MPC8xx
>>> ? endchoice
>>> diff --git a/arch/powerpc/cpu/mpc8xx/Kconfig 
>>> b/arch/powerpc/cpu/mpc8xx/Kconfig
>>> index b0e90a0f20..3e8ea38529 100644
>>> --- a/arch/powerpc/cpu/mpc8xx/Kconfig
>>> +++ b/arch/powerpc/cpu/mpc8xx/Kconfig
>>> @@ -25,9 +25,9 @@ config MPC885
>>> ? endchoice
>>> -config MPC8xx_WATCHDOG
>>> -??? bool "Watchdog"
>>> -??? select HW_WATCHDOG
>>> +#config MPC8xx_WATCHDOG
>>> +#??? bool "Watchdog"
>>> +#??? select HW_WATCHDOG
>>
>> If HW_WATCHDOG is not selected, we have a problem in cpu_init_f() 
>> (arch/powerpc/cpu/mpc8xx/cpu_init.c) with the following line, and the 
>> board with restart every second.
>>
>> #ifndef CONFIG_HW_WATCHDOG
>> ?????/* deactivate watchdog if not enabled in config */
>> ?????out_be32(&immr->im_siu_conf.sc_sypcr, CONFIG_SYS_SYPCR & 
>> ~SYPCR_SWE);
>> #endif
>>
>> Should this be changed to use CONFIG_WATCHDOG instead ?
> 
> Yes, that sound reasonable.
> 

Looking at it more closely, I think there is something going wrong.

watchdog_reset() in wdt-uclass.c bails out without pinging the watchdog 
until GD_FLG_WDT_READY is set. But this is set in initr_watchdog().
It means that all WATCHDOG_RESET() until then are now ignored. This 
means all the phase during which Uboot execute from firmware until 
relocation do not service the watchdog anymore. The watchdog is ON since 
the CPU reset and must be serviced until someone decides to disable it, 
in which case it can't be re-enabled later. This means we can't disable 
it early to re-enable it once DM is ready.

And if I understand correctly doc/README.watchdog, that's exactly what 
CONFIG_HW_WATCHDOG is made for.

So I think that we just can't switch mpc8xx to CONFIG_WATCHDOG 
(include/wdt.c clears makes the difference between Hardware watchdogs 
and software watchdogs), it must remain with CONFIG_HW_WATCHDOG

Christophe

[PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version

[Rasmus Villemoes]

On 20/02/2020 07.43, Stefan Roese wrote:
> On 20.02.20 07:38, Christophe Leroy wrote:
> 
> <snip>
> 
>>>>> +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);
>>>>> +??? }
>>>>
>>>> This is a problem for the MPC8xx.
>>>>
>>>> When running with a MPC8xx at 132MHz clock, the watchdog will fire
>>>> about 1s after the last refresh. So the above makes the board unusable.
>>>
>>> So you need a shorted delay between the wdt_reset() calls? Is this
>>> correct? We could introduce a new Kconfig option which defaults to
>>> 1000 (ms) and you can "select" a shorter value for MPC8xx.
>>
>> Exactly. However, why is this limitation needed at all ? Why is it a
>> problem to refresh more often ?
> 
> Very likely its not. What is a reasonable value for your platform? 100
> or 500ms? I think we could change it to default to a shorter value, but
> such a change should go in early in the merge window, so that other
> platforms have a bit of time to test it.
> 
> Please feel free to send a patch for this and please add a comment to
> explain, why the delay is this "short".

IMO, this should come from DT. For a gpio watchdog (which for some
reason U-Boot doesn't have a generic driver for) the linux kernel uses a
hw_margin_ms property that tells the core how often the watchdog must be
pinged - that could be generalized to apply for all, with 1000ms as a
default if not set. And I've seen boards with a gpio watchdog with a
timeout of 200 ms.

Also, I'm wondering why that generic _reset only handles one watchdog
device? I can easily imagine needing to reset both, say, an external
gpio-triggered one and also the SOC's/CPU's built-in one. Why not loop
over all DM watchdogs, and have the next_reset/hw_margin etc. metadata
live with the watchdog device instead of in static variable/build-time
constants?

Rasmus

[PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version

[Stefan Roese]

On 20.02.20 08:43, Rasmus Villemoes wrote:
> On 20/02/2020 07.43, Stefan Roese wrote:
>> On 20.02.20 07:38, Christophe Leroy wrote:
>>
>> <snip>
>>
>>>>>> +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);
>>>>>> +??? }
>>>>>
>>>>> This is a problem for the MPC8xx.
>>>>>
>>>>> When running with a MPC8xx at 132MHz clock, the watchdog will fire
>>>>> about 1s after the last refresh. So the above makes the board unusable.
>>>>
>>>> So you need a shorted delay between the wdt_reset() calls? Is this
>>>> correct? We could introduce a new Kconfig option which defaults to
>>>> 1000 (ms) and you can "select" a shorter value for MPC8xx.
>>>
>>> Exactly. However, why is this limitation needed at all ? Why is it a
>>> problem to refresh more often ?
>>
>> Very likely its not. What is a reasonable value for your platform? 100
>> or 500ms? I think we could change it to default to a shorter value, but
>> such a change should go in early in the merge window, so that other
>> platforms have a bit of time to test it.
>>
>> Please feel free to send a patch for this and please add a comment to
>> explain, why the delay is this "short".
> 
> IMO, this should come from DT.

Yes, I agree. I just wanted to keep the change "simple" here with this
approach.

> For a gpio watchdog (which for some
> reason U-Boot doesn't have a generic driver for) the linux kernel uses a
> hw_margin_ms property that tells the core how often the watchdog must be
> pinged - that could be generalized to apply for all, with 1000ms as a
> default if not set. And I've seen boards with a gpio watchdog with a
> timeout of 200 ms.

Ah, okay. Its good to know, that Linux already has such a DT property.

> Also, I'm wondering why that generic _reset only handles one watchdog
> device? I can easily imagine needing to reset both, say, an external
> gpio-triggered one and also the SOC's/CPU's built-in one. Why not loop
> over all DM watchdogs, and have the next_reset/hw_margin etc. metadata
> live with the watchdog device instead of in static variable/build-time
> constants?

I agree, that the current WDT handling can be extended to support
multiple (different) WDT instances. Currently there don't seem to be
any (I might be incorrect here) board that need such a feature though.
I would suggest to implement such a feature, once its really needed.

Thanks,
Stefan