HI,
2018-02-28 9:55 GMT+01:00 Lukasz Majewski lukma@denx.de:
On Mon, 26 Feb 2018 10:09:55 +0100 Michal Simek michal.simek@xilinx.com wrote:
Watchdog is only enabled in full u-boot. Adoption for SPL should be also done because that's the right place where watchdog should be enabled.
Signed-off-by: Michal Simek michal.simek@xilinx.com
Changes in v2:
- Make watchdog_reset depends on CONFIG_WATCHDOG not WDT This will handle use cases where watchdog is cleared by OS
arch/arm/Kconfig | 1 + board/xilinx/zynq/board.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 2c52ff025a22..a66d04eadfcb 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -761,6 +761,7 @@ config ARCH_ZYNQ select SUPPORT_SPL select OF_CONTROL select SPL_BOARD_INIT if SPL
select BOARD_EARLY_INIT_F if WDT select SPL_OF_CONTROL if SPL select DM select DM_ETH if NETdiff --git a/board/xilinx/zynq/board.c b/board/xilinx/zynq/board.c index fb8eab07d768..838ac0f4c4ea 100644 --- a/board/xilinx/zynq/board.c +++ b/board/xilinx/zynq/board.c @@ -6,9 +6,11 @@ */
#include <common.h> +#include <dm/uclass.h> #include <fdtdec.h> #include <fpga.h> #include <mmc.h> +#include <wdt.h> #include <zynqpl.h> #include <asm/arch/hardware.h> #include <asm/arch/sys_proto.h> @@ -33,6 +35,22 @@ static xilinx_desc fpga045 = XILINX_XC7Z045_DESC(0x45); static xilinx_desc fpga100 = XILINX_XC7Z100_DESC(0x100); #endif
+#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT) +static struct udevice *watchdog_dev; +#endif
+#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_BOARD_EARLY_INIT_F) +int board_early_init_f(void) +{ +# if defined(CONFIG_WDT)
/* bss is not cleared at time when watchdog_reset() iscalled */
watchdog_dev = NULL;+# endif
return 0;+} +#endif
int board_init(void) { #if (defined(CONFIG_FPGA) && !defined(CONFIG_SPL_BUILD)) || \ @@ -75,6 +93,15 @@ int board_init(void) } #endif
+#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {puts("Watchdog: Not found!\n");} else {wdt_start(watchdog_dev, 0, 0);puts("Watchdog: Started\n");}+# endif
#if (defined(CONFIG_FPGA) && !defined(CONFIG_SPL_BUILD)) || \ (defined(CONFIG_SPL_FPGA_SUPPORT) && defined(CONFIG_SPL_BUILD)) fpga_init(); @@ -164,3 +191,25 @@ 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
I do have two questions if you don't mind:
- It seems like a lot of code added to a board file to provide WDT
support. Normally we just call a few functions - like hw_watchdog_init(); WATCHDOG_RESET();
Unfortunately I didn't find a way how to do it with less code. If you see a way to optimize it please let me know. hw_watchdog_init is not using DM. I used similar solution which is used by turris omnia.
Code needs to handle reference to watchdog_dev that's why the code is there.
- Is there any good reason for Watchdog_reset not being put into
driver, being wrapped, so it would provide WATCHDOG_RESET() macro, which is used to refresh WDT in several places?
The reason is that it depends how exactly you want to use it. Also adding dependency on WATCHDOG not WDT is reasonable because you can just start watchdog in u-boot but never service it. It should enable you and option to boot till certain time or reboot. I have tested that this setting is working. I haven't tested if Linux cadence driver can handle it but that's different topic.
Definitely please let me know if you see any issue with this patch.
Thanks, Michal