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

[Chris Packham]

On Sat, Feb 16, 2019 at 12:23 AM Chris Packham <judge.packham at gmail.com> wrote:
>
>
>
> 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.
>

Spoke too soon. It's not SYS_TCLK but numerically it happens to be
SYS_TCLK / 10. I'd like to keep this as-is (I'll still send v2 with
the updated commit message).

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

I think this is actually what's needed. Right now orion_wdt justs
casts timeout_ms to u32 and uses it directly. It should figure out how
many timer ticks are needed (which will be a function of
CONFIG_SYS_TCLK) and figure out the value appropriately.

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