On Sat, Feb 16, 2019 at 12:23 AM Chris Packham <judge.pack...@gmail.com> wrote: > > > > On Fri, 15 Feb 2019 21:46 Stefan Roese <s...@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.pack...@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 <s...@denx.de> >> >> Thanks, >> Stefan _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot