Hi Michal,
Hi,
2018-02-28 10:42 GMT+01:00 Lukasz Majewski lukma@denx.de:
On Wed, 28 Feb 2018 10:29:24 +0100 Michal Simek monstr@monstr.eu wrote:
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.
I think that the problem is not with the code optimization - I'm a bit concern about reusability.
Probably this code could be move out of board file to more generic location but several things needs to be considered. Especially how to handle system with more watchdogs.
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.
Isn't this a bit dangerous? For example the end user stop booting the board because wants to upload and flash some data.
It is user decision when this should be done. If you do this from Linux it should be fine because Linux service it.
Ok.
During update the not serviced watchdog reset the board and you may end up with brick.
Depends on your setup and needs. If you have board with only one watchdog when you boot from qspi but reading variables from qspi failed (for whatever reason). it means u-boot ends in prompt watchdog is serviced properly and it will never expire.
The most WDT use cases is to reboot when the target device aborts (hangs) at program execution.
( As a remedy one can put reset if bootx fails ).
However, I do get your point.
But in this case this is broken boot. In next reset variable read can be correct.
It should enable you and option to boot till certain time or reboot.
You can enable WDT in SPL and "refresh" it in u-boot if needed.
I didn't try SPL but I will look at it. Again depends on use case you have. Also which board you are using. If you have one or multiple watchdogs. I can imagine if you have 2 watchdogs available that one will cover full boot till linux and one just u-boot issues.
I'm not sure if u-boot now supports two watchdogs in a generic way...
I have tested that this setting is working. I haven't tested if Linux cadence driver can handle it but that's different topic.
There are several options in Linux (with iMX6 at least).
You may disable WDT when driver is correctly setup, refresh it or leave as is, so user space program can take over the WDT control.
I expect this behaviour but I just didn't test that with cadence driver. Also initial u-boot setup can be different compare to OS setup.
Yes, agree. It is use case dependent.
Thanks, Michal
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de