[U-Boot] [PATCH v2 2/3] arm: zynq: Wire watchdog internals

[Michal Simek]

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.
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. 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

-- 
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