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

[Lukasz Majewski <lukma@denx.de>]

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

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 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/f547ba8b/attachment.sig>