diff mbox series

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

Message ID 20210714085626.5961-4-stephan@gerhold.net
State Accepted
Commit ad7e967738a9c639e07cf50b83ffccdf9a8537b0
Delegated to: Tom Rini
Headers show
Series board: dragonboard410c: Fix some minor issues | expand

Commit Message

Stephan Gerhold July 14, 2021, 8:56 a.m. UTC
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(+)

Comments

Ramon Fried July 14, 2021, 8:39 p.m. UTC | #1
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 ?
Stephan Gerhold July 15, 2021, 6:15 a.m. UTC | #2
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
Tom Rini July 24, 2021, 8:40 p.m. UTC | #3
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!
diff mbox series

Patch

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);