[U-Boot] [PATCH 2/2] arm: mvebu: x530: Enable watchdog in SPL and U-Boot

[Chris Packham]

On Fri, 15 Feb 2019 21:46 Stefan Roese <sr at denx.de wrote:

> Hi Chris,
>
> On 15.02.19 03:12, Chris Packham wrote:
> > Enable the hardware watchdog to guard against system lock ups when
> > running in the SPL or U-Boot. Stop the watchdog just before booting so
> > that the OS.
> >
> > Signed-off-by: Chris Packham <judge.packham at gmail.com>
> > ---
> >
> >   arch/arm/dts/armada-385-atl-x530-u-boot.dtsi |  4 ++
> >   board/alliedtelesis/x530/x530.c              | 48 ++++++++++++++++++++
> >   configs/x530_defconfig                       |  5 ++
> >   3 files changed, 57 insertions(+)
> >
> > diff --git a/arch/arm/dts/armada-385-atl-x530-u-boot.dtsi
> b/arch/arm/dts/armada-385-atl-x530-u-boot.dtsi
> > index 7074a73537fa..79b694cb84bc 100644
> > --- a/arch/arm/dts/armada-385-atl-x530-u-boot.dtsi
> > +++ b/arch/arm/dts/armada-385-atl-x530-u-boot.dtsi
> > @@ -11,3 +11,7 @@
> >   &uart0 {
> >       u-boot,dm-pre-reloc;
> >   };
> > +
> > +&watchdog {
> > +     u-boot,dm-pre-reloc;
> > +};
> > diff --git a/board/alliedtelesis/x530/x530.c
> b/board/alliedtelesis/x530/x530.c
> > index d7d1942fe686..1b22b6307cd2 100644
> > --- a/board/alliedtelesis/x530/x530.c
> > +++ b/board/alliedtelesis/x530/x530.c
> > @@ -7,6 +7,7 @@
> >   #include <command.h>
> >   #include <dm.h>
> >   #include <i2c.h>
> > +#include <wdt.h>
> >   #include <asm/gpio.h>
> >   #include <linux/mbus.h>
> >   #include <linux/io.h>
> > @@ -24,6 +25,10 @@ DECLARE_GLOBAL_DATA_PTR;
> >   #define CONFIG_NVS_LOCATION         0xf4800000
> >   #define CONFIG_NVS_SIZE                     (512 << 10)
> >
> > +#ifdef CONFIG_WATCHDOG
> > +static struct udevice *watchdog_dev;
> > +#endif
> > +
> >   static struct serdes_map board_serdes_map[] = {
> >       {PEX0, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
> >       {DEFAULT_SERDES, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
> > @@ -75,6 +80,10 @@ struct mv_ddr_topology_map
> *mv_ddr_topology_map_get(void)
> >
> >   int board_early_init_f(void)
> >   {
> > +#ifdef CONFIG_WATCHDOG
> > +     watchdog_dev = NULL;
> > +#endif
> > +
> >       /* Configure MPP */
> >       writel(0x00001111, MVEBU_MPP_BASE + 0x00);
> >       writel(0x00000000, MVEBU_MPP_BASE + 0x04);
> > @@ -88,6 +97,17 @@ int board_early_init_f(void)
> >       return 0;
> >   }
> >
> > +void spl_board_init(void)
> > +{
> > +#ifdef CONFIG_WATCHDOG
> > +     int ret;
> > +
> > +     ret = uclass_get_device(UCLASS_WDT, 0, &watchdog_dev);
> > +     if (!ret)
> > +             wdt_start(watchdog_dev, 25000000ULL * 120, 0);
>
> This is CONFIG_SYS_TCLK? Would it make sense to use this macro
> instead here?
>

Yes it would. I'll send a v2 with this change.

Ideally I'd specify the value in ms and have orion_wdt deal with the
details of SYS_TCLK.


> > +#endif
> > +}
> > +
> >   int board_init(void)
> >   {
> >       /* address of boot parameters */
> > @@ -100,9 +120,37 @@ int board_init(void)
> >       /* DEV_READYn is not needed for NVS, ignore it when accessing CS1
> */
> >       writel(0x00004001, MVEBU_DEV_BUS_BASE + 0xc8);
> >
> > +     spl_board_init();
> > +
> >       return 0;
> >   }
> >
> > +void arch_preboot_os(void)
> > +{
> > +#ifdef CONFIG_WATCHDOG
> > +     wdt_stop(watchdog_dev);
> > +#endif
> > +}
>
> So you are not using the WDT in the OS (Linux)?
>
> > +
> > +#ifdef CONFIG_WATCHDOG
> > +void watchdog_reset(void)
> > +{
> > +     static ulong next_reset = 0;
> > +     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
>
> When I recently implemented the watchdog support the MT7688, I
> wondered why there is so much code necessary, that each board
> and platform needs to copy to get this real watchdog working.
> We should definitely rework this at some time, so make this more
> generic with less board specific code that could be shared.
>
> You don't need to change this here. I just wanted to express my
> thoughts, as I'm stumbling over this code again.
>
> Other than this (and your commit text change):
>
> Reviewed-by: Stefan Roese <sr at denx.de>
>
> Thanks,
> Stefan
>