[U-Boot] [PATCH v2 2/3] arm: zynq: Wire watchdog internals
Michal Simek
monstr at monstr.eu
Wed Feb 28 09:58:08 UTC 2018
Hi,
2018-02-28 10:42 GMT+01:00 Lukasz Majewski <lukma at denx.de>:
> On Wed, 28 Feb 2018 10:29:24 +0100
> Michal Simek <monstr at monstr.eu> wrote:
>
> > HI,
> >
> > 2018-02-28 9:55 GMT+01:00 Lukasz Majewski <lukma at denx.de>:
> >
> > > On Mon, 26 Feb 2018 10:09:55 +0100
> > > Michal Simek <michal.simek at 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 at 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 NET
> > > > diff --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() is
> > > > called */
> > > > + 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:
> > >
> > > 1. 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.
> >
> >
> > >
> > > 2. 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.
>
> 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. 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 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.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
More information about the U-Boot
mailing list