[PATCH u-boot-mvebu 0/3] turris_omnia: Disable MCU watchdog in SPL when booting over UART

[PATCH u-boot-mvebu 0/3] turris_omnia: Disable MCU watchdog in SPL when booting over UART

[Marek Behún]

Hello Stefan,

while debugging Omnia, when sending U-Boot over UART, some USB-TTY
controllers are too slow and Omnia's MCU watchdog resets the board
before U-Boot proper is sent.

This patch series changes Omnia board code so that when booting via
UART, we disable MCU watchdog in SPL, prior loading U-Boot proper.

Marek

Marek Behún (3):
  arm: mvebu: Move get_boot_device() to cpu.c and make visible
  arm: mvebu: turris_omnia: don't guard by CONFIG_SPL_BUILD macro
  arm: mvebu: turris_omnia: disable MCU watchdog in SPL when booting
    over UART

 arch/arm/mach-mvebu/cpu.c                | 60 ++++++++++++++++++
 arch/arm/mach-mvebu/include/mach/cpu.h   |  2 +
 arch/arm/mach-mvebu/spl.c                | 77 +++---------------------
 board/CZ.NIC/turris_omnia/turris_omnia.c | 25 +++++---
 configs/turris_omnia_defconfig           |  1 +
 5 files changed, 88 insertions(+), 77 deletions(-)

-- 
2.31.1

[PATCH u-boot-mvebu 1/3] arm: mvebu: Move get_boot_device() to cpu.c and make visible

[Marek Behún]

Move the function get_boot_device() from spl.c to cpu.c.

Make it visible, so that it may be used from other files.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 arch/arm/mach-mvebu/cpu.c              | 60 ++++++++++++++++++++
 arch/arm/mach-mvebu/include/mach/cpu.h |  2 +
 arch/arm/mach-mvebu/spl.c              | 77 +++-----------------------
 3 files changed, 71 insertions(+), 68 deletions(-)

diff --git a/arch/arm/mach-mvebu/cpu.c b/arch/arm/mach-mvebu/cpu.c
index 0b935c46fb..daf8bd66a0 100644
--- a/arch/arm/mach-mvebu/cpu.c
+++ b/arch/arm/mach-mvebu/cpu.c
@@ -14,6 +14,7 @@
 #include <asm/pl310.h>
 #include <asm/arch/cpu.h>
 #include <asm/arch/soc.h>
+#include <asm/spl.h>
 #include <sdhci.h>
 
 #define DDR_BASE_CS_OFF(n)	(0x0000 + ((n) << 3))
@@ -80,6 +81,65 @@ int mvebu_soc_family(void)
 	return MVEBU_SOC_UNKNOWN;
 }
 
+u32 get_boot_device(void)
+{
+	u32 val;
+	u32 boot_device;
+
+	/*
+	  * First check, if UART boot-mode is active. This can only
+	  * be done, via the bootrom error register. Here the
+	  * MSB marks if the UART mode is active.
+	  */
+	val = readl(CONFIG_BOOTROM_ERR_REG);
+	boot_device = (val & BOOTROM_ERR_MODE_MASK) >> BOOTROM_ERR_MODE_OFFS;
+	debug("BOOTROM_REG=0x%08x boot_device=0x%x\n", val, boot_device);
+	if (boot_device == BOOTROM_ERR_MODE_UART)
+		return BOOT_DEVICE_UART;
+
+#ifdef CONFIG_ARMADA_38X
+	/*
+	  * If the bootrom error code contains any other than zeros it's an
+	  * error condition and the bootROM has fallen back to UART boot
+	  */
+	boot_device = (val & BOOTROM_ERR_CODE_MASK) >> BOOTROM_ERR_CODE_OFFS;
+	if (boot_device)
+		return BOOT_DEVICE_UART;
+#endif
+
+	/*
+	  * Now check the SAR register for the strapped boot-device
+	  */
+	val = readl(CONFIG_SAR_REG);	/* SAR - Sample At Reset */
+	boot_device = (val & BOOT_DEV_SEL_MASK) >> BOOT_DEV_SEL_OFFS;
+	debug("SAR_REG=0x%08x boot_device=0x%x\n", val, boot_device);
+	switch (boot_device) {
+#ifdef BOOT_FROM_NAND
+	case BOOT_FROM_NAND:
+		return BOOT_DEVICE_NAND;
+#endif
+#ifdef BOOT_FROM_MMC
+	case BOOT_FROM_MMC:
+	case BOOT_FROM_MMC_ALT:
+		return BOOT_DEVICE_MMC1;
+#endif
+	case BOOT_FROM_UART:
+#ifdef BOOT_FROM_UART_ALT
+	case BOOT_FROM_UART_ALT:
+#endif
+		return BOOT_DEVICE_UART;
+#ifdef BOOT_FROM_SATA
+	case BOOT_FROM_SATA:
+	case BOOT_FROM_SATA_ALT:
+		return BOOT_DEVICE_SATA;
+#endif
+	case BOOT_FROM_SPI:
+		return BOOT_DEVICE_SPI;
+	default:
+		return BOOT_DEVICE_BOOTROM;
+	};
+}
+
 #if defined(CONFIG_DISPLAY_CPUINFO)
 
 #if defined(CONFIG_ARMADA_375)
diff --git a/arch/arm/mach-mvebu/include/mach/cpu.h b/arch/arm/mach-mvebu/include/mach/cpu.h
index 79858858c2..a7a62c7e7d 100644
--- a/arch/arm/mach-mvebu/include/mach/cpu.h
+++ b/arch/arm/mach-mvebu/include/mach/cpu.h
@@ -148,6 +148,8 @@ void __noreturn return_to_bootrom(void);
 int mv_sdh_init(unsigned long regbase, u32 max_clk, u32 min_clk, u32 quirks);
 #endif
 
+u32 get_boot_device(void);
+
 void get_sar_freq(struct sar_freq_modes *sar_freq);
 
 /*
diff --git a/arch/arm/mach-mvebu/spl.c b/arch/arm/mach-mvebu/spl.c
index f0cf60bb14..8d6d4902f6 100644
--- a/arch/arm/mach-mvebu/spl.c
+++ b/arch/arm/mach-mvebu/spl.c
@@ -171,74 +171,6 @@ int spl_parse_board_header(struct spl_image_info *spl_image,
 	return 0;
 }
 
-static u32 get_boot_device(void)
-{
-	u32 val;
-	u32 boot_device;
-
-	/*
-	 * First check, if UART boot-mode is active. This can only
-	 * be done, via the bootrom error register. Here the
-	 * MSB marks if the UART mode is active.
-	 */
-	val = readl(CONFIG_BOOTROM_ERR_REG);
-	boot_device = (val & BOOTROM_ERR_MODE_MASK) >> BOOTROM_ERR_MODE_OFFS;
-	debug("BOOTROM_REG=0x%08x boot_device=0x%x\n", val, boot_device);
-	if (boot_device == BOOTROM_ERR_MODE_UART)
-		return BOOT_DEVICE_UART;
-
-#ifdef CONFIG_ARMADA_38X
-	/*
-	 * If the bootrom error code contains any other than zeros it's an
-	 * error condition and the bootROM has fallen back to UART boot
-	 */
-	boot_device = (val & BOOTROM_ERR_CODE_MASK) >> BOOTROM_ERR_CODE_OFFS;
-	if (boot_device)
-		return BOOT_DEVICE_UART;
-#endif
-
-	/*
-	 * Now check the SAR register for the strapped boot-device
-	 */
-	val = readl(CONFIG_SAR_REG);	/* SAR - Sample At Reset */
-	boot_device = (val & BOOT_DEV_SEL_MASK) >> BOOT_DEV_SEL_OFFS;
-	debug("SAR_REG=0x%08x boot_device=0x%x\n", val, boot_device);
-	switch (boot_device) {
-#ifdef BOOT_FROM_NAND
-	case BOOT_FROM_NAND:
-		return BOOT_DEVICE_NAND;
-#endif
-#ifdef BOOT_FROM_MMC
-	case BOOT_FROM_MMC:
-	case BOOT_FROM_MMC_ALT:
-		return BOOT_DEVICE_MMC1;
-#endif
-	case BOOT_FROM_UART:
-#ifdef BOOT_FROM_UART_ALT
-	case BOOT_FROM_UART_ALT:
-#endif
-		return BOOT_DEVICE_UART;
-#ifdef BOOT_FROM_SATA
-	case BOOT_FROM_SATA:
-	case BOOT_FROM_SATA_ALT:
-		return BOOT_DEVICE_SATA;
-#endif
-	case BOOT_FROM_SPI:
-		return BOOT_DEVICE_SPI;
-	default:
-		return BOOT_DEVICE_BOOTROM;
-	};
-}
-
-#else
-
-static u32 get_boot_device(void)
-{
-	return BOOT_DEVICE_BOOTROM;
-}
-
-#endif
-
 u32 spl_boot_device(void)
 {
 	u32 boot_device = get_boot_device();
@@ -285,6 +217,15 @@ u32 spl_boot_device(void)
 	}
 }
 
+#else
+
+u32 spl_boot_device(void)
+{
+	return BOOT_DEVICE_BOOTROM;
+}
+
+#endif
+
 int board_return_to_bootrom(struct spl_image_info *spl_image,
 			    struct spl_boot_device *bootdev)
 {
-- 
2.31.1

[PATCH u-boot-mvebu 2/3] arm: mvebu: turris_omnia: don't guard by CONFIG_SPL_BUILD macro

[Marek Behún]

We do not need to guard code in board_init() and board_late_init()
functions with the CONFIG_SPL_BUILD macro, since these functions are not
called in SPL.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 board/CZ.NIC/turris_omnia/turris_omnia.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c
index a7e5f56eed..a84a409f43 100644
--- a/board/CZ.NIC/turris_omnia/turris_omnia.c
+++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
@@ -129,7 +129,6 @@ static int omnia_mcu_read(u8 cmd, void *buf, int len)
 	return dm_i2c_read(chip, cmd, buf, len);
 }
 
-#ifndef CONFIG_SPL_BUILD
 static int omnia_mcu_write(u8 cmd, const void *buf, int len)
 {
 	struct udevice *chip;
@@ -158,7 +157,6 @@ static bool disable_mcu_watchdog(void)
 
 	return true;
 }
-#endif
 
 static bool omnia_detect_sata(void)
 {
@@ -325,7 +323,6 @@ struct mv_ddr_topology_map *mv_ddr_topology_map_get(void)
 		return &board_topology_map_1g;
 }
 
-#ifndef CONFIG_SPL_BUILD
 static int set_regdomain(void)
 {
 	struct omnia_eeprom oep;
@@ -394,7 +391,6 @@ static void handle_reset_button(void)
 		}
 	}
 }
-#endif
 
 int board_early_init_f(void)
 {
@@ -428,19 +424,15 @@ int board_init(void)
 	/* address of boot parameters */
 	gd->bd->bi_boot_params = mvebu_sdram_bar(0) + 0x100;
 
-#ifndef CONFIG_SPL_BUILD
 	disable_mcu_watchdog();
-#endif
 
 	return 0;
 }
 
 int board_late_init(void)
 {
-#ifndef CONFIG_SPL_BUILD
 	set_regdomain();
 	handle_reset_button();
-#endif
 	pci_init();
 
 	return 0;
-- 
2.31.1

[PATCH u-boot-mvebu 3/3] arm: mvebu: turris_omnia: disable MCU watchdog in SPL when booting over UART

[Marek Behún]

When booting over UART, sending U-Boot proper may take too much time and
MCU watchdog will reset the board before U-Boot proper is loaded.

Better disable MCU watchdog in SPL when booting over UART.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 board/CZ.NIC/turris_omnia/turris_omnia.c | 17 ++++++++++++++++-
 configs/turris_omnia_defconfig           |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c
index a84a409f43..b0391c973d 100644
--- a/board/CZ.NIC/turris_omnia/turris_omnia.c
+++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
@@ -419,12 +419,27 @@ int board_early_init_f(void)
 	return 0;
 }
 
+void spl_board_init(void)
+{
+	/*
+	 * If booting from UART, disable MCU watchdog in SPL, since uploading
+	 * U-Boot proper can take too much time and trigger it.
+	 */
+	if (get_boot_device() == BOOT_DEVICE_UART)
+		disable_mcu_watchdog();
+}
+
 int board_init(void)
 {
 	/* address of boot parameters */
 	gd->bd->bi_boot_params = mvebu_sdram_bar(0) + 0x100;
 
-	disable_mcu_watchdog();
+	/*
+	 * If not booting from UART, MCU watchdog was not disabled in SPL,
+	 * disable it now.
+	 */
+	if (get_boot_device() != BOOT_DEVICE_UART)
+		disable_mcu_watchdog();
 
 	return 0;
 }
diff --git a/configs/turris_omnia_defconfig b/configs/turris_omnia_defconfig
index cd443ceb30..67f8bdadaf 100644
--- a/configs/turris_omnia_defconfig
+++ b/configs/turris_omnia_defconfig
@@ -33,6 +33,7 @@ CONFIG_SYS_CONSOLE_INFO_QUIET=y
 # CONFIG_DISPLAY_BOARDINFO is not set
 CONFIG_DISPLAY_BOARDINFO_LATE=y
 CONFIG_MISC_INIT_R=y
+CONFIG_SPL_BOARD_INIT=y
 CONFIG_SPL_I2C=y
 CONFIG_CMD_MEMTEST=y
 CONFIG_SYS_ALT_MEMTEST=y
-- 
2.31.1

Re: [PATCH u-boot-mvebu 3/3] arm: mvebu: turris_omnia: disable MCU watchdog in SPL when booting over UART

[Stefan Roese]

Hi Marek,

On 16.08.21 14:27, Marek Behún wrote:
> When booting over UART, sending U-Boot proper may take too much time and
> MCU watchdog will reset the board before U-Boot proper is loaded.
> 
> Better disable MCU watchdog in SPL when booting over UART.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>   board/CZ.NIC/turris_omnia/turris_omnia.c | 17 ++++++++++++++++-
>   configs/turris_omnia_defconfig           |  1 +
>   2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c
> index a84a409f43..b0391c973d 100644
> --- a/board/CZ.NIC/turris_omnia/turris_omnia.c
> +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
> @@ -419,12 +419,27 @@ int board_early_init_f(void)
>   	return 0;
>   }
>   
> +void spl_board_init(void)
> +{
> +	/*
> +	 * If booting from UART, disable MCU watchdog in SPL, since uploading
> +	 * U-Boot proper can take too much time and trigger it.
> +	 */
> +	if (get_boot_device() == BOOT_DEVICE_UART)
> +		disable_mcu_watchdog();
> +}
> +
>   int board_init(void)
>   {
>   	/* address of boot parameters */
>   	gd->bd->bi_boot_params = mvebu_sdram_bar(0) + 0x100;
>   
> -	disable_mcu_watchdog();
> +	/*
> +	 * If not booting from UART, MCU watchdog was not disabled in SPL,
> +	 * disable it now.
> +	 */
> +	if (get_boot_device() != BOOT_DEVICE_UART)
> +		disable_mcu_watchdog();

Why do you disable the MCU watchdog here in U-Boot proper?

Thanks,
Stefan

>   
>   	return 0;
>   }
> diff --git a/configs/turris_omnia_defconfig b/configs/turris_omnia_defconfig
> index cd443ceb30..67f8bdadaf 100644
> --- a/configs/turris_omnia_defconfig
> +++ b/configs/turris_omnia_defconfig
> @@ -33,6 +33,7 @@ CONFIG_SYS_CONSOLE_INFO_QUIET=y
>   # CONFIG_DISPLAY_BOARDINFO is not set
>   CONFIG_DISPLAY_BOARDINFO_LATE=y
>   CONFIG_MISC_INIT_R=y
> +CONFIG_SPL_BOARD_INIT=y
>   CONFIG_SPL_I2C=y
>   CONFIG_CMD_MEMTEST=y
>   CONFIG_SYS_ALT_MEMTEST=y
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-mvebu 3/3] arm: mvebu: turris_omnia: disable MCU watchdog in SPL when booting over UART

[Marek Behún]

On Mon, 16 Aug 2021 14:50:15 +0200
Stefan Roese <sr@denx.de> wrote:

> > -	disable_mcu_watchdog();
> > +	/*
> > +	 * If not booting from UART, MCU watchdog was not disabled
> > in SPL,
> > +	 * disable it now.
> > +	 */
> > +	if (get_boot_device() != BOOT_DEVICE_UART)
> > +		disable_mcu_watchdog();  
> 
> Why do you disable the MCU watchdog here in U-Boot proper?

Hi Stefan,

this is just where it was before.
But looking at the prompt
   SoC:   MV88F6820-A0 at 1600 MHz
   DRAM:  2 GiB (800 MHz, 32-bit, 2T, ECC not enabled)
   Disabling MCU watchdog... disabled
   WDT:   Started with servicing (60s timeout)

it looks like U-Boot's watchdog is being enabled after MCU watchdog.
It would be better to enable U-Boot's WDT before disabling MCU watchdog.
I will send another patch that moves it to another function,
board_late_init for example.

Shall I resend this series with this new patch?

Marek

Re: [PATCH u-boot-mvebu 3/3] arm: mvebu: turris_omnia: disable MCU watchdog in SPL when booting over UART

[Stefan Roese]

On 16.08.21 15:01, Marek Behún wrote:
> On Mon, 16 Aug 2021 14:50:15 +0200
> Stefan Roese <sr@denx.de> wrote:
> 
>>> -	disable_mcu_watchdog();
>>> +	/*
>>> +	 * If not booting from UART, MCU watchdog was not disabled
>>> in SPL,
>>> +	 * disable it now.
>>> +	 */
>>> +	if (get_boot_device() != BOOT_DEVICE_UART)
>>> +		disable_mcu_watchdog();
>>
>> Why do you disable the MCU watchdog here in U-Boot proper?
> 
> Hi Stefan,
> 
> this is just where it was before.
> But looking at the prompt
>     SoC:   MV88F6820-A0 at 1600 MHz
>     DRAM:  2 GiB (800 MHz, 32-bit, 2T, ECC not enabled)
>     Disabling MCU watchdog... disabled
>     WDT:   Started with servicing (60s timeout)
> 
> it looks like U-Boot's watchdog is being enabled after MCU watchdog.
> It would be better to enable U-Boot's WDT before disabling MCU watchdog.
> I will send another patch that moves it to another function,
> board_late_init for example.

So the MCU watchdog is some kind of very early WD and only used until
the SoC WD is used / enabled?

> Shall I resend this series with this new patch?

Yes, please do.

Thanks,
Stefan

Re: [PATCH u-boot-mvebu 3/3] arm: mvebu: turris_omnia: disable MCU watchdog in SPL when booting over UART

[Marek Behún]

On Mon, 16 Aug 2021 15:07:33 +0200
Stefan Roese <sr@denx.de> wrote:

> So the MCU watchdog is some kind of very early WD and only used until
> the SoC WD is used / enabled?

Yes, this is how it was first thought of for Omnia. Looking at the MCU
code it seems that the watchdog can be started again after being
disabled, so it could theoretically be used as a normal watchdog. The
problem is that it cannot be pinged to reset the counter. It has to be
stopped and started again, meaning there is a small window when it is
disabled. If the system used this watchdog, this small window would
make it vulnerable, so I don't think we should use it.

>> Shall I resend this series with this new patch?  

> Yes, please do.

Will do.

Marek

Re: [PATCH u-boot-mvebu 3/3] arm: mvebu: turris_omnia: disable MCU watchdog in SPL when booting over UART

[Stefan Roese]

On 16.08.21 15:11, Marek Behún wrote:
> On Mon, 16 Aug 2021 15:07:33 +0200
> Stefan Roese <sr@denx.de> wrote:
> 
>> So the MCU watchdog is some kind of very early WD and only used until
>> the SoC WD is used / enabled?
> 
> Yes, this is how it was first thought of for Omnia. Looking at the MCU
> code it seems that the watchdog can be started again after being
> disabled, so it could theoretically be used as a normal watchdog. The
> problem is that it cannot be pinged to reset the counter. It has to be
> stopped and started again, meaning there is a small window when it is
> disabled. If the system used this watchdog, this small window would
> make it vulnerable, so I don't think we should use it.

Agreed. With such a WD concept including this "window of vulnerability",
it really shouldn't be used.

Thanks,
Stefan

>>> Shall I resend this series with this new patch?
> 
>> Yes, please do.
> 
> Will do.
> 
> Marek
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de