[U-Boot] [PATCH 0/2] x530: Enable watchdog

[U-Boot] [PATCH 0/2] x530: Enable watchdog

[Chris Packham]

We've seen some issues with the x530 under extreme conditions where the
DDR gets into a bad state. Generally this results in an application
crash, kernel panic then a lock-up in u-boot (generally just as the SPL
hands off to u-boot proper).

Enabling the watchdog prevents the lock up and will let the DDR training
have another go. Sometimes this recovers but even a reboot loop is
better than a complete lockup.

Chris Packham (2):
  watchdog: orion_wdt: support SPL usage
  arm: mvebu: x530: Enable watchdog in SPL and U-Boot

 arch/arm/dts/armada-385-atl-x530-u-boot.dtsi |  4 ++
 board/alliedtelesis/x530/x530.c              | 48 ++++++++++++++++++++
 configs/x530_defconfig                       |  5 ++
 drivers/watchdog/orion_wdt.c                 |  4 +-
 4 files changed, 58 insertions(+), 3 deletions(-)

-- 
2.20.1

[U-Boot] [PATCH 1/2] watchdog: orion_wdt: support SPL usage

[Chris Packham]

When run from the SPL the mvebu targets are using the hardware default
offset for the SoC peripherals. devfdt_get_addr_size_index() understands
how to deal with this via dm_get_translation_offset() so use this
instead of fdtdec_get_addr_size_auto_noparent().

Signed-off-by: Chris Packham <judge.packham@gmail.com>
---

 drivers/watchdog/orion_wdt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index a0df02d10382..c1add3e7c121 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -114,9 +114,7 @@ static inline bool save_reg_from_ofdata(struct udevice *dev, int index,
 	fdt_addr_t addr;
 	fdt_size_t off;
 
-	addr = fdtdec_get_addr_size_auto_noparent(
-		gd->fdt_blob, dev_of_offset(dev), "reg", index, &off, true);
-
+	addr = devfdt_get_addr_size_index(dev, index, &off);
 	if (addr == FDT_ADDR_T_NONE)
 		return false;
 
-- 
2.20.1

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

[Chris Packham]

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@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);
+#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
+}
+
+#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
+
 static int led_7seg_init(unsigned int segments)
 {
 	int node;
diff --git a/configs/x530_defconfig b/configs/x530_defconfig
index 25b9e885d8e6..3bc37b9bee11 100644
--- a/configs/x530_defconfig
+++ b/configs/x530_defconfig
@@ -19,6 +19,8 @@ CONFIG_SILENT_CONSOLE=y
 CONFIG_SILENT_U_BOOT_ONLY=y
 CONFIG_SILENT_CONSOLE_UPDATE_ON_RELOC=y
 CONFIG_MISC_INIT_R=y
+CONFIG_SPL_BOARD_INIT=y
+CONFIG_SPL_WATCHDOG_SUPPORT=y
 CONFIG_CMD_MEMINFO=y
 # CONFIG_CMD_FLASH is not set
 CONFIG_CMD_GPIO=y
@@ -70,3 +72,6 @@ CONFIG_USB_STORAGE=y
 CONFIG_USB_HOST_ETHER=y
 CONFIG_USB_ETHER_ASIX=y
 CONFIG_USB_ETHER_ASIX88179=y
+CONFIG_WATCHDOG=y
+CONFIG_WDT=y
+CONFIG_WDT_ORION=y
-- 
2.20.1

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

[Chris Packham]

On Fri, Feb 15, 2019 at 3:12 PM Chris Packham <judge.packham@gmail.com> 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.

D'oh  managed to cut off the sentence.

so that the OS can re-enable it if needed.

>
> Signed-off-by: Chris Packham <judge.packham@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);
> +#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
> +}
> +
> +#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
> +
>  static int led_7seg_init(unsigned int segments)
>  {
>         int node;
> diff --git a/configs/x530_defconfig b/configs/x530_defconfig
> index 25b9e885d8e6..3bc37b9bee11 100644
> --- a/configs/x530_defconfig
> +++ b/configs/x530_defconfig
> @@ -19,6 +19,8 @@ CONFIG_SILENT_CONSOLE=y
>  CONFIG_SILENT_U_BOOT_ONLY=y
>  CONFIG_SILENT_CONSOLE_UPDATE_ON_RELOC=y
>  CONFIG_MISC_INIT_R=y
> +CONFIG_SPL_BOARD_INIT=y
> +CONFIG_SPL_WATCHDOG_SUPPORT=y
>  CONFIG_CMD_MEMINFO=y
>  # CONFIG_CMD_FLASH is not set
>  CONFIG_CMD_GPIO=y
> @@ -70,3 +72,6 @@ CONFIG_USB_STORAGE=y
>  CONFIG_USB_HOST_ETHER=y
>  CONFIG_USB_ETHER_ASIX=y
>  CONFIG_USB_ETHER_ASIX88179=y
> +CONFIG_WATCHDOG=y
> +CONFIG_WDT=y
> +CONFIG_WDT_ORION=y
> --
> 2.20.1
>

[U-Boot] [PATCH 1/2] watchdog: orion_wdt: support SPL usage

[Stefan Roese]

On 15.02.19 03:12, Chris Packham wrote:
> When run from the SPL the mvebu targets are using the hardware default
> offset for the SoC peripherals. devfdt_get_addr_size_index() understands
> how to deal with this via dm_get_translation_offset() so use this
> instead of fdtdec_get_addr_size_auto_noparent().
> 
> Signed-off-by: Chris Packham <judge.packham@gmail.com>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

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

[Stefan Roese]

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

> +#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@denx.de>

Thanks,
Stefan

[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@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@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@denx.de>
>
> Thanks,
> Stefan
>

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

[Chris Packham]

On Fri, 15 Feb 2019, 9:46 PM Stefan Roese <sr@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@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?
>
> > +#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)?
>

In our production systems we are. But I wanted to allow a kernel built
without the driver to boot and not have it reset on me (e.g. if I'm using a
debugger).


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

Yes it's no coincidence that this looks a lot like the code from one of the
turris boards. I was kind of surprised i needed to get the device and start
it but it kind of makes sense as I've decided that my board needs to do it
in the spl where as others might want to use it only to ensure that their
os boots succesfully.

Implementing watchdog_reset was a surprise but again generic code would
need to iterate over all instantiated UCLASS_WDT devices.

May be there's some middle ground with some mvebu specific code that can't
be completely generic but can avoid the repetition.


> Other than this (and your commit text change):
>
> Reviewed-by: Stefan Roese <sr@denx.de>
>
> Thanks,
> Stefan
>

[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@gmail.com> wrote:
>
>
>
> On Fri, 15 Feb 2019 21:46 Stefan Roese <sr@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@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@denx.de>
>>
>> Thanks,
>> Stefan