
On 09.04.19 12:45, Michal Simek wrote:
On 08. 04. 19 11:28, Stefan Roese wrote:
This patch tries to implement a generic watchdog_reset() function that can be used by all boards that want to service the watchdog device in U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG.
Without this approach, new boards or platforms needed to implement a board specific version of this functionality, mostly copy'ing the same code over and over again into their board or platforms code base.
With this new generic function, the scattered other functions are now removed to be replaced by the generic one. The new version also enables the configuration of the watchdog timeout via the DT "timeout-sec" property (if enabled via CONFIG_OF_CONTROL).
This patch also adds a new flag to the GD flags, to flag that the watchdog is ready to use and adds the pointer to the watchdog device to the GD. This enables us to remove the global "watchdog_dev" variable, which was prone to cause problems because of its potentially very early use in watchdog_reset(), even before the BSS is cleared.
Signed-off-by: Stefan Roese sr@denx.de Cc: Heiko Schocher hs@denx.de Cc: Tom Rini trini@konsulko.com Cc: Michal Simek michal.simek@xilinx.com Cc: "Marek Behún" marek.behun@nic.cz Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com
This patch depends on the following patches:
[1] watchdog: Move watchdog_dev to data section (BSS may not be cleared) https://patchwork.ozlabs.org/patch/1075500/
[2] watchdog: Handle SPL build with watchdog disabled https://patchwork.ozlabs.org/patch/1074098/
I would like to see [1] applied in this release, since its a real fix on some of the platforms with minimal chances of breakage.
This patch now is a bigger rework and is intended for the next merge window.
Please note that I didn't remove the "timeout-sec" handling from the driver already reading it from the DT (cdns & at91) in this patch. The reason for this is, that in the cdns case, the removal also brings a functional change, which I wanted to do in a separate patch. And in the at91 case its because there are updates to this driver already queued in the at91 next git branch which would most likely cause merge conflict. I'll send a cleanup patch for this driver later after these patches have been applied.
Thanks, Stefan
v2:
Rename watchdog_start() to initr_watchdog() and move it to board_r.c. This way its only called once, after the DM subsystem has bound all watchdog drivers. This enables the use of the currently implemented logic of using the correct watchdog in U-Boot (via alias etc).
arch/mips/mach-mt7620/cpu.c | 36 ----------------- board/CZ.NIC/turris_mox/turris_mox.c | 30 -------------- board/CZ.NIC/turris_omnia/turris_omnia.c | 35 ---------------- .../microblaze-generic/microblaze-generic.c | 40 ------------------- board/xilinx/zynq/board.c | 39 ------------------ board/xilinx/zynqmp/zynqmp.c | 39 ------------------ common/board_r.c | 40 +++++++++++++++++++ drivers/watchdog/wdt-uclass.c | 26 ++++++++++++ include/asm-generic/global_data.h | 4 ++ 9 files changed, 70 insertions(+), 219 deletions(-)
<snip>
int board_early_init_r(void) { u32 val; diff --git a/common/board_r.c b/common/board_r.c index 472987d5d5..d80f16c3ed 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -48,6 +48,7 @@ #include <linux/compiler.h> #include <linux/err.h> #include <efi_loader.h> +#include <wdt.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -621,6 +622,42 @@ static int initr_bedbug(void) } #endif
+#if defined(CONFIG_WATCHDOG) && defined(CONFIG_WDT)
This is not correct. Here should be just CONFIG_WDT.
Because there are cases where you want just start watchdog in u-boot but never service it by u-boot.
AFAIK, this would change the current behavior. Currently, only when CONFIG_WATCHDOG is enabled the watchdog driver is started automatically in U-Boot (and serviced). If I make the change you suggest above, all boards defining CONFIG_WDT (DM watchdog support) will automatically enable the watchdog. This might make sense, but AFAICT this changes the current behavior.
Thanks, Stefan
+#ifndef WDT_DEFAULT_TIMEOUT +#define WDT_DEFAULT_TIMEOUT 60 +#endif
+static int initr_watchdog(void) +{
- u32 timeout = WDT_DEFAULT_TIMEOUT;
- /*
* Init watchdog: This will call the probe function of the
* watchdog driver, enabling the use of the device
*/
- if (uclass_get_device_by_seq(UCLASS_WDT, 0,
(struct udevice **)&gd->watchdog_dev)) {
debug("WDT: Not found by seq!\n");
if (uclass_get_device(UCLASS_WDT, 0,
(struct udevice **)&gd->watchdog_dev)) {
printf("WDT: Not found!\n");
return 0;
}
- }
- if (CONFIG_IS_ENABLED(OF_CONTROL)) {
timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec",
WDT_DEFAULT_TIMEOUT);
- }
- wdt_start(gd->watchdog_dev, timeout * 1000, 0);
- gd->flags |= GD_FLG_WDT_READY;
- printf("WDT: Started (%ds timeout)\n", timeout);
- return 0;
+} +#endif
- static int run_main_loop(void) { #ifdef CONFIG_SANDBOX
@@ -670,6 +707,9 @@ static init_fnc_t init_sequence_r[] = { #ifdef CONFIG_DM initr_dm, #endif +#if defined(CONFIG_WATCHDOG) && defined(CONFIG_WDT)
ditto.
- initr_watchdog,
+#endif #if defined(CONFIG_ARM) || defined(CONFIG_NDS32) || defined(CONFIG_RISCV) || \ defined(CONFIG_SANDBOX) board_init, /* Setup chipselects */ diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c index 23b7e3360d..bbfac4f0f9 100644 --- a/drivers/watchdog/wdt-uclass.c +++ b/drivers/watchdog/wdt-uclass.c @@ -10,6 +10,8 @@ #include <dm/device-internal.h> #include <dm/lists.h>
+DECLARE_GLOBAL_DATA_PTR;
- int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags) { const struct wdt_ops *ops = device_get_ops(dev);
@@ -63,6 +65,30 @@ int wdt_expire_now(struct udevice *dev, ulong flags) return ret; }
+#if defined(CONFIG_WATCHDOG) +/*
- Called by macro WATCHDOG_RESET. This function be called *very* early,
- so we need to make sure, that the watchdog driver is ready before using
- it in this function.
- */
+void watchdog_reset(void) +{
- static ulong next_reset;
- ulong now;
- /* Exit if GD is not ready or watchdog is not initialized yet */
- if (!gd || !(gd->flags & GD_FLG_WDT_READY))
return;
- /* Do not reset the watchdog too often */
- now = get_timer(0);
- if (now > next_reset) {
next_reset = now + 1000; /* reset every 1000ms */
wdt_reset(gd->watchdog_dev);
- }
+} +#endif
- static int wdt_post_bind(struct udevice *dev) { #if defined(CONFIG_NEEDS_MANUAL_RELOC)
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index 78dcf40bff..3cb583cbb1 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -133,6 +133,9 @@ typedef struct global_data { struct spl_handoff *spl_handoff; # endif #endif +#ifdef CONFIG_WATCHDOG
CONFIG_WDT here.
nit: don't know if you should use #if defined() or if CONFIG_IS_ENABLED here.
- struct udevice *watchdog_dev;
+#endif } gd_t; #endif
@@ -161,5 +164,6 @@ typedef struct global_data { #define GD_FLG_ENV_DEFAULT 0x02000 /* Default variable flag */ #define GD_FLG_SPL_EARLY_INIT 0x04000 /* Early SPL init is done */ #define GD_FLG_LOG_READY 0x08000 /* Log system is ready for use */ +#define GD_FLG_WDT_READY 0x10000 /* Watchdog is ready for use */
#endif /* __ASM_GENERIC_GBL_DATA_H */
M
Viele Grüße, Stefan