[U-Boot] [PATCH v2 2/3] arm: zynq: Wire watchdog internals
Lukasz Majewski
lukma at denx.de
Wed Feb 28 09:42:11 UTC 2018
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.
> 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.
During update the not serviced watchdog reset the board and you may end
up with brick.
> 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 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.
>
> Definitely please let me know if you see any issue with this patch.
>
> 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 at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180228/0e2f3212/attachment.sig>
More information about the U-Boot
mailing list