[PATCH 0/3] board: dragonboard410c: Fix some minor issues

[PATCH 0/3] board: dragonboard410c: Fix some minor issues

[Stephan Gerhold]

I spent some more time testing the "U-Boot without LK" changes [1] on DB410c
last week and noticed some minor issues that are fixed in this patch series.

Ideally they should be applied on top of [1]. However, actually PATCH 1/3
and PATCH 2/3 are completely unrelated to [1] and could be also applied
independently. PATCH 3/3 is likely directly related to [1].

[1]: https://lore.kernel.org/u-boot/20210707090602.32515-1-stephan@gerhold.net/


Stephan Gerhold (3):
  board: dragonboard410c: Fix PHYS_SDRAM_1_SIZE
  board: dragonboard410c: Fix fastboot
  serial: serial_msm: Delay initialization to let pins stabilize

 board/qualcomm/dragonboard410c/dragonboard410c.c | 3 +--
 configs/dragonboard410c_defconfig                | 1 +
 drivers/serial/serial_msm.c                      | 4 ++++
 include/configs/dragonboard410c.h                | 4 ++--
 4 files changed, 8 insertions(+), 4 deletions(-)

-- 
2.32.0

[PATCH 1/3] board: dragonboard410c: Fix PHYS_SDRAM_1_SIZE

[Stephan Gerhold]

The DragonBoard 410c has proprietary firmware from Qualcomm that
reserves 8 MiB of memory for tz/smem/hyp/rmtfs/rfsa from 0x86000000
to 0x86800000. I'm not aware of any ATF (ARM Trusted Firmware) port
for DB410c that would reserve 30 MiB of memory at the end of RAM.
I suspect the comment might have been copied from hikey.h which has
a very similar comment (and which actually does have an ATF port).

Reducing the memory size just prevents U-Boot from using the end of
the RAM, not the reserved region inbetween. Therefore we might as well
display the correct DRAM size (1 GiB) instead of strange 986 MiB.

Fixes: 626f048bbc14 ("board: Add Qualcomm Dragonboard 410C support")
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---

 include/configs/dragonboard410c.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/configs/dragonboard410c.h b/include/configs/dragonboard410c.h
index 1f08508c4d..6474e57b29 100644
--- a/include/configs/dragonboard410c.h
+++ b/include/configs/dragonboard410c.h
@@ -16,8 +16,8 @@
 
 /* Physical Memory Map */
 #define PHYS_SDRAM_1			0x80000000
-/* 1008 MB (the last ~30Mb are secured for TrustZone by ATF*/
-#define PHYS_SDRAM_1_SIZE		0x3da00000
+/* Note: 8 MiB (0x86000000 - 0x86800000) are reserved for tz/smem/hyp/rmtfs/rfsa */
+#define PHYS_SDRAM_1_SIZE		SZ_1G
 #define CONFIG_SYS_SDRAM_BASE		PHYS_SDRAM_1
 #define CONFIG_SYS_INIT_SP_ADDR		(CONFIG_SYS_SDRAM_BASE + 0x7fff0)
 #define CONFIG_SYS_LOAD_ADDR		(CONFIG_SYS_SDRAM_BASE + 0x80000)
-- 
2.32.0

Re: [PATCH 1/3] board: dragonboard410c: Fix PHYS_SDRAM_1_SIZE

[Ramon Fried]

On Wed, Jul 14, 2021 at 11:56 AM Stephan Gerhold <stephan@gerhold.net> wrote:
>
> The DragonBoard 410c has proprietary firmware from Qualcomm that
> reserves 8 MiB of memory for tz/smem/hyp/rmtfs/rfsa from 0x86000000
> to 0x86800000. I'm not aware of any ATF (ARM Trusted Firmware) port
> for DB410c that would reserve 30 MiB of memory at the end of RAM.
> I suspect the comment might have been copied from hikey.h which has
> a very similar comment (and which actually does have an ATF port).
>
> Reducing the memory size just prevents U-Boot from using the end of
> the RAM, not the reserved region inbetween. Therefore we might as well
> display the correct DRAM size (1 GiB) instead of strange 986 MiB.
>
> Fixes: 626f048bbc14 ("board: Add Qualcomm Dragonboard 410C support")
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>
>  include/configs/dragonboard410c.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/configs/dragonboard410c.h b/include/configs/dragonboard410c.h
> index 1f08508c4d..6474e57b29 100644
> --- a/include/configs/dragonboard410c.h
> +++ b/include/configs/dragonboard410c.h
> @@ -16,8 +16,8 @@
>
>  /* Physical Memory Map */
>  #define PHYS_SDRAM_1                   0x80000000
> -/* 1008 MB (the last ~30Mb are secured for TrustZone by ATF*/
> -#define PHYS_SDRAM_1_SIZE              0x3da00000
> +/* Note: 8 MiB (0x86000000 - 0x86800000) are reserved for tz/smem/hyp/rmtfs/rfsa */
> +#define PHYS_SDRAM_1_SIZE              SZ_1G
>  #define CONFIG_SYS_SDRAM_BASE          PHYS_SDRAM_1
>  #define CONFIG_SYS_INIT_SP_ADDR                (CONFIG_SYS_SDRAM_BASE + 0x7fff0)
>  #define CONFIG_SYS_LOAD_ADDR           (CONFIG_SYS_SDRAM_BASE + 0x80000)
> --
> 2.32.0
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 1/3] board: dragonboard410c: Fix PHYS_SDRAM_1_SIZE

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 938 bytes --]

On Wed, Jul 14, 2021 at 10:56:24AM +0200, Stephan Gerhold wrote:

> The DragonBoard 410c has proprietary firmware from Qualcomm that
> reserves 8 MiB of memory for tz/smem/hyp/rmtfs/rfsa from 0x86000000
> to 0x86800000. I'm not aware of any ATF (ARM Trusted Firmware) port
> for DB410c that would reserve 30 MiB of memory at the end of RAM.
> I suspect the comment might have been copied from hikey.h which has
> a very similar comment (and which actually does have an ATF port).
> 
> Reducing the memory size just prevents U-Boot from using the end of
> the RAM, not the reserved region inbetween. Therefore we might as well
> display the correct DRAM size (1 GiB) instead of strange 986 MiB.
> 
> Fixes: 626f048bbc14 ("board: Add Qualcomm Dragonboard 410C support")
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 2/3] board: dragonboard410c: Fix fastboot

[Stephan Gerhold]

At the moment pressing the volume down key does not actually launch
fastboot. This is because setting "bootdelay" to "-1" actually
disables autoboot and drops to the U-Boot console. It does not execute
the "bootcmd".

The correct value for "bootdelay" here would be "-2", which disables
the delay and key checking and would immediately execute the "bootcmd".

However, even better in this case is using "preboot" to trigger Fastboot.
The advantage is that running "fastboot continue" will actually continue
the autoboot process instead of ending up in the U-Boot shell.

Also make sure to unset "preboot" again immediately in case the user
saves the environment after triggering fastboot.

Cc: Ramon Fried <rfried.dev@gmail.com>
Fixes: aa043ee91a47 ("db410c: automatically launch fastboot")
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---

 board/qualcomm/dragonboard410c/dragonboard410c.c | 3 +--
 configs/dragonboard410c_defconfig                | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/board/qualcomm/dragonboard410c/dragonboard410c.c b/board/qualcomm/dragonboard410c/dragonboard410c.c
index 3b71881cac..371b3262f8 100644
--- a/board/qualcomm/dragonboard410c/dragonboard410c.c
+++ b/board/qualcomm/dragonboard410c/dragonboard410c.c
@@ -132,8 +132,7 @@ int misc_init_r(void)
 	}
 
 	if (dm_gpio_get_value(&resin)) {
-		env_set("bootdelay", "-1");
-		env_set("bootcmd", "fastboot 0");
+		env_set("preboot", "setenv preboot; fastboot 0");
 		printf("key_vol_down pressed - Starting fastboot.\n");
 	}
 
diff --git a/configs/dragonboard410c_defconfig b/configs/dragonboard410c_defconfig
index dc9d0d893c..a46283f980 100644
--- a/configs/dragonboard410c_defconfig
+++ b/configs/dragonboard410c_defconfig
@@ -10,6 +10,7 @@ CONFIG_DISTRO_DEFAULTS=y
 # CONFIG_ANDROID_BOOT_IMAGE is not set
 CONFIG_FIT=y
 CONFIG_OF_BOARD_SETUP=y
+CONFIG_USE_PREBOOT=y
 # CONFIG_DISPLAY_CPUINFO is not set
 # CONFIG_DISPLAY_BOARDINFO is not set
 CONFIG_MISC_INIT_R=y
-- 
2.32.0

Re: [PATCH 2/3] board: dragonboard410c: Fix fastboot

[Ramon Fried]

On Wed, Jul 14, 2021 at 11:56 AM Stephan Gerhold <stephan@gerhold.net> wrote:
>
> At the moment pressing the volume down key does not actually launch
> fastboot. This is because setting "bootdelay" to "-1" actually
> disables autoboot and drops to the U-Boot console. It does not execute
> the "bootcmd".
>
> The correct value for "bootdelay" here would be "-2", which disables
> the delay and key checking and would immediately execute the "bootcmd".
>
> However, even better in this case is using "preboot" to trigger Fastboot.
> The advantage is that running "fastboot continue" will actually continue
> the autoboot process instead of ending up in the U-Boot shell.
>
> Also make sure to unset "preboot" again immediately in case the user
> saves the environment after triggering fastboot.
>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Fixes: aa043ee91a47 ("db410c: automatically launch fastboot")
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>
>  board/qualcomm/dragonboard410c/dragonboard410c.c | 3 +--
>  configs/dragonboard410c_defconfig                | 1 +
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/board/qualcomm/dragonboard410c/dragonboard410c.c b/board/qualcomm/dragonboard410c/dragonboard410c.c
> index 3b71881cac..371b3262f8 100644
> --- a/board/qualcomm/dragonboard410c/dragonboard410c.c
> +++ b/board/qualcomm/dragonboard410c/dragonboard410c.c
> @@ -132,8 +132,7 @@ int misc_init_r(void)
>         }
>
>         if (dm_gpio_get_value(&resin)) {
> -               env_set("bootdelay", "-1");
> -               env_set("bootcmd", "fastboot 0");
> +               env_set("preboot", "setenv preboot; fastboot 0");
>                 printf("key_vol_down pressed - Starting fastboot.\n");
>         }
>
> diff --git a/configs/dragonboard410c_defconfig b/configs/dragonboard410c_defconfig
> index dc9d0d893c..a46283f980 100644
> --- a/configs/dragonboard410c_defconfig
> +++ b/configs/dragonboard410c_defconfig
> @@ -10,6 +10,7 @@ CONFIG_DISTRO_DEFAULTS=y
>  # CONFIG_ANDROID_BOOT_IMAGE is not set
>  CONFIG_FIT=y
>  CONFIG_OF_BOARD_SETUP=y
> +CONFIG_USE_PREBOOT=y
>  # CONFIG_DISPLAY_CPUINFO is not set
>  # CONFIG_DISPLAY_BOARDINFO is not set
>  CONFIG_MISC_INIT_R=y
> --
> 2.32.0
>
I also noticed that a few days ago, it's strange because it used to
work, maybe something changed since I introduced the fastboot support.
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 2/3] board: dragonboard410c: Fix fastboot

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1066 bytes --]

On Wed, Jul 14, 2021 at 10:56:25AM +0200, Stephan Gerhold wrote:

> At the moment pressing the volume down key does not actually launch
> fastboot. This is because setting "bootdelay" to "-1" actually
> disables autoboot and drops to the U-Boot console. It does not execute
> the "bootcmd".
> 
> The correct value for "bootdelay" here would be "-2", which disables
> the delay and key checking and would immediately execute the "bootcmd".
> 
> However, even better in this case is using "preboot" to trigger Fastboot.
> The advantage is that running "fastboot continue" will actually continue
> the autoboot process instead of ending up in the U-Boot shell.
> 
> Also make sure to unset "preboot" again immediately in case the user
> saves the environment after triggering fastboot.
> 
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Fixes: aa043ee91a47 ("db410c: automatically launch fastboot")
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 3/3] serial: serial_msm: Delay initialization to let pins stabilize

[Stephan Gerhold]

For some reason, the DragonBoard 410c aborts autoboot immediately if
U-Boot is started without LK. It looks like it picks up a single broken
character via serial and therefore believes a key was pressed to abort
autoboot.

After some debugging, it seems like adding some delay after pinctrl
setup but before UART initialization fixes the issue. It's also worth
mentioning that unlike when booting from LK, the pinctrl setup is
actually necessary when booting U-Boot without LK since UART is broken
if the pinctrl line is removed.

I suspect that reconfiguring the pins might take some time to stabilize
and if the UART controller is enabled too quickly it will pick up some
random noise. Adding a few milliseconds of delay fixes the issue and
shouldn't have any other negative side effects.

3ms seems to be the minimum delay required in my tests, use 5ms instead
just to be sure.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---

 drivers/serial/serial_msm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c
index d8dd5c1104..ac4d0824b9 100644
--- a/drivers/serial/serial_msm.c
+++ b/drivers/serial/serial_msm.c
@@ -18,6 +18,7 @@
 #include <asm/global_data.h>
 #include <asm/io.h>
 #include <linux/compiler.h>
+#include <linux/delay.h>
 #include <dm/pinctrl.h>
 
 /* Serial registers - this driver works in uartdm mode*/
@@ -193,6 +194,9 @@ static int msm_uart_clk_init(struct udevice *dev)
 
 static void uart_dm_init(struct msm_serial_data *priv)
 {
+	/* Delay initialization for a bit to let pins stabilize if necessary */
+	mdelay(5);
+
 	writel(priv->clk_bit_rate, priv->base + UARTDM_CSR);
 	writel(0x0, priv->base + UARTDM_MR1);
 	writel(MSM_BOOT_UART_DM_8_N_1_MODE, priv->base + UARTDM_MR2);
-- 
2.32.0

Re: [PATCH 3/3] serial: serial_msm: Delay initialization to let pins stabilize

[Ramon Fried]

On Wed, Jul 14, 2021 at 11:56 AM Stephan Gerhold <stephan@gerhold.net> wrote:
>
> For some reason, the DragonBoard 410c aborts autoboot immediately if
> U-Boot is started without LK. It looks like it picks up a single broken
> character via serial and therefore believes a key was pressed to abort
> autoboot.
>
> After some debugging, it seems like adding some delay after pinctrl
> setup but before UART initialization fixes the issue. It's also worth
> mentioning that unlike when booting from LK, the pinctrl setup is
> actually necessary when booting U-Boot without LK since UART is broken
> if the pinctrl line is removed.
>
> I suspect that reconfiguring the pins might take some time to stabilize
> and if the UART controller is enabled too quickly it will pick up some
> random noise. Adding a few milliseconds of delay fixes the issue and
> shouldn't have any other negative side effects.
>
> 3ms seems to be the minimum delay required in my tests, use 5ms instead
> just to be sure.
>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>
>  drivers/serial/serial_msm.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c
> index d8dd5c1104..ac4d0824b9 100644
> --- a/drivers/serial/serial_msm.c
> +++ b/drivers/serial/serial_msm.c
> @@ -18,6 +18,7 @@
>  #include <asm/global_data.h>
>  #include <asm/io.h>
>  #include <linux/compiler.h>
> +#include <linux/delay.h>
>  #include <dm/pinctrl.h>
>
>  /* Serial registers - this driver works in uartdm mode*/
> @@ -193,6 +194,9 @@ static int msm_uart_clk_init(struct udevice *dev)
>
>  static void uart_dm_init(struct msm_serial_data *priv)
>  {
> +       /* Delay initialization for a bit to let pins stabilize if necessary */
> +       mdelay(5);
> +
>         writel(priv->clk_bit_rate, priv->base + UARTDM_CSR);
>         writel(0x0, priv->base + UARTDM_MR1);
>         writel(MSM_BOOT_UART_DM_8_N_1_MODE, priv->base + UARTDM_MR2);
> --
> 2.32.0
>
Eh...this is not a nice fix, perhaps just clearing the RX buffer after
initialization will work better ?

Re: [PATCH 3/3] serial: serial_msm: Delay initialization to let pins stabilize

[Stephan Gerhold]

On Wed, Jul 14, 2021 at 11:39:34PM +0300, Ramon Fried wrote:
> On Wed, Jul 14, 2021 at 11:56 AM Stephan Gerhold <stephan@gerhold.net> wrote:
> >
> > For some reason, the DragonBoard 410c aborts autoboot immediately if
> > U-Boot is started without LK. It looks like it picks up a single broken
> > character via serial and therefore believes a key was pressed to abort
> > autoboot.
> >
> > After some debugging, it seems like adding some delay after pinctrl
> > setup but before UART initialization fixes the issue. It's also worth
> > mentioning that unlike when booting from LK, the pinctrl setup is
> > actually necessary when booting U-Boot without LK since UART is broken
> > if the pinctrl line is removed.
> >
> > I suspect that reconfiguring the pins might take some time to stabilize
> > and if the UART controller is enabled too quickly it will pick up some
> > random noise. Adding a few milliseconds of delay fixes the issue and
> > shouldn't have any other negative side effects.
> >
> > 3ms seems to be the minimum delay required in my tests, use 5ms instead
> > just to be sure.
> >
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> >
> >  drivers/serial/serial_msm.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c
> > index d8dd5c1104..ac4d0824b9 100644
> > --- a/drivers/serial/serial_msm.c
> > +++ b/drivers/serial/serial_msm.c
> > @@ -18,6 +18,7 @@
> >  #include <asm/global_data.h>
> >  #include <asm/io.h>
> >  #include <linux/compiler.h>
> > +#include <linux/delay.h>
> >  #include <dm/pinctrl.h>
> >
> >  /* Serial registers - this driver works in uartdm mode*/
> > @@ -193,6 +194,9 @@ static int msm_uart_clk_init(struct udevice *dev)
> >
> >  static void uart_dm_init(struct msm_serial_data *priv)
> >  {
> > +       /* Delay initialization for a bit to let pins stabilize if necessary */
> > +       mdelay(5);
> > +
> >         writel(priv->clk_bit_rate, priv->base + UARTDM_CSR);
> >         writel(0x0, priv->base + UARTDM_MR1);
> >         writel(MSM_BOOT_UART_DM_8_N_1_MODE, priv->base + UARTDM_MR2);
> > --
> > 2.32.0
> >
> Eh...this is not a nice fix, perhaps just clearing the RX buffer after
> initialization will work better ?

I don't like it much either but this is the best I came up with even
after spending several hours trying to track down the problem. I also
tried porting more register initialization from LK, probably even
tried clearing the RX buffer again after initialization.

The problem is: In my tests there was no way to get rid of the delay,
because the character doesn't show up until a few milliseconds after
initialization. What you suggest would only work like this:

  <initialize UART>
  mdelay(5); // Wait for the weird character to show up
  <clear RX buffer>

Which is not much different (and in fact will take longer) than:

  mdelay(5);
  <initialize UART>

So unfortunately I can't think of a way to get rid of the delay. :(

I'm not sure if this is some strange problem of my DB410c, I'd actually
be curious if the problem can also be reproduced on others.

Stephan

Re: [PATCH 3/3] serial: serial_msm: Delay initialization to let pins stabilize

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1112 bytes --]

On Wed, Jul 14, 2021 at 10:56:26AM +0200, Stephan Gerhold wrote:

> For some reason, the DragonBoard 410c aborts autoboot immediately if
> U-Boot is started without LK. It looks like it picks up a single broken
> character via serial and therefore believes a key was pressed to abort
> autoboot.
> 
> After some debugging, it seems like adding some delay after pinctrl
> setup but before UART initialization fixes the issue. It's also worth
> mentioning that unlike when booting from LK, the pinctrl setup is
> actually necessary when booting U-Boot without LK since UART is broken
> if the pinctrl line is removed.
> 
> I suspect that reconfiguring the pins might take some time to stabilize
> and if the UART controller is enabled too quickly it will pick up some
> random noise. Adding a few milliseconds of delay fixes the issue and
> shouldn't have any other negative side effects.
> 
> 3ms seems to be the minimum delay required in my tests, use 5ms instead
> just to be sure.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]