[v3,1/2] serial: mxc: Wait for TX completion before reset

The u-boot console may show some corrupted characters when
printing in board_init() due to reset of the UART (probe)
before the TX FIFO has been completely drained.

To fix this issue, and in case UART is still running, we now
try to flush the FIFO before proceeding to UART reinitialization.
For this we're waiting for Transmitter Complete bit, indicating
that the FIFO and the shift register are empty.

flushing has a 4ms timeout guard, which is normally more than
enough to consume the FIFO @ low baudrate (9600bps).

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
Tested-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 v2: Add this commit to the series
 v3: Fix typo & reordering commits for good bisectability 

 drivers/serial/serial_mxc.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

[Adding Johannes and Tim]

On Tue, Jan 10, 2023 at 4:24 PM Loic Poulain <loic.poulain@linaro.org> wrote:
>
> The u-boot console may show some corrupted characters when
> printing in board_init() due to reset of the UART (probe)
> before the TX FIFO has been completely drained.
>
> To fix this issue, and in case UART is still running, we now
> try to flush the FIFO before proceeding to UART reinitialization.
> For this we're waiting for Transmitter Complete bit, indicating
> that the FIFO and the shift register are empty.
>
> flushing has a 4ms timeout guard, which is normally more than
> enough to consume the FIFO @ low baudrate (9600bps).
>
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> Tested-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  v2: Add this commit to the series
>  v3: Fix typo & reordering commits for good bisectability

Reviewed-by: Fabio Estevam <festevam@denx.de>

Thanks

On Tuesday 10 January 2023 20:24:06 Loic Poulain wrote:
> The u-boot console may show some corrupted characters when
> printing in board_init() due to reset of the UART (probe)
> before the TX FIFO has been completely drained.
> 
> To fix this issue, and in case UART is still running, we now
> try to flush the FIFO before proceeding to UART reinitialization.
> For this we're waiting for Transmitter Complete bit, indicating
> that the FIFO and the shift register are empty.
> 
> flushing has a 4ms timeout guard, which is normally more than
> enough to consume the FIFO @ low baudrate (9600bps).
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> Tested-by: Lothar Waßmann <LW@KARO-electronics.de>

Hello! Last time when I looked at this driver I was in impression that
also _mxc_serial_setbrg() function requires calling some flush function
at the beginning. Could you please check if it is needed or not? I'm
really not sure.

> ---
>  v2: Add this commit to the series
>  v3: Fix typo & reordering commits for good bisectability 
> 
>  drivers/serial/serial_mxc.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
> index 82c0d84628..3c89781f2c 100644
> --- a/drivers/serial/serial_mxc.c
> +++ b/drivers/serial/serial_mxc.c
> @@ -13,6 +13,7 @@
>  #include <dm/platform_data/serial_mxc.h>
>  #include <serial.h>
>  #include <linux/compiler.h>
> +#include <linux/delay.h>
>  
>  /* UART Control Register Bit Fields.*/
>  #define URXD_CHARRDY	(1<<15)
> @@ -144,8 +145,22 @@ struct mxc_uart {
>  	u32 ts;
>  };
>  
> +static void _mxc_serial_flush(struct mxc_uart *base)
> +{
> +	unsigned int timeout = 4000;
> +
> +	if (!(readl(&base->cr1) & UCR1_UARTEN) ||
> +	    !(readl(&base->cr2) & UCR2_TXEN))
> +		return;
> +
> +	while (!(readl(&base->sr2) & USR2_TXDC) && --timeout)
> +		udelay(1);
> +}
> +
>  static void _mxc_serial_init(struct mxc_uart *base, int use_dte)
>  {
> +	_mxc_serial_flush(base);
> +
>  	writel(0, &base->cr1);
>  	writel(0, &base->cr2);
>  
> @@ -252,10 +267,17 @@ static int mxc_serial_init(void)
>  	return 0;
>  }
>  
> +static int mxc_serial_stop(void)
> +{
> +	_mxc_serial_flush(mxc_base);
> +
> +	return 0;
> +}
> +
>  static struct serial_device mxc_serial_drv = {
>  	.name	= "mxc_serial",
>  	.start	= mxc_serial_init,
> -	.stop	= NULL,
> +	.stop	= mxc_serial_stop,
>  	.setbrg	= mxc_serial_setbrg,
>  	.putc	= mxc_serial_putc,
>  	.puts	= default_serial_puts,

Anyway, this code touches _only_ non-DM version of the driver. So same
fix should be implemented also for DM version because non-DM is now
legacy and will be removed in the future from U-Boot. Please look at the
DM version too.

> -- 
> 2.34.1
>

On Wed, 11 Jan 2023 at 00:53, Pali Rohár <pali@kernel.org> wrote:
>
> On Tuesday 10 January 2023 20:24:06 Loic Poulain wrote:
> > The u-boot console may show some corrupted characters when
> > printing in board_init() due to reset of the UART (probe)
> > before the TX FIFO has been completely drained.
> >
> > To fix this issue, and in case UART is still running, we now
> > try to flush the FIFO before proceeding to UART reinitialization.
> > For this we're waiting for Transmitter Complete bit, indicating
> > that the FIFO and the shift register are empty.
> >
> > flushing has a 4ms timeout guard, which is normally more than
> > enough to consume the FIFO @ low baudrate (9600bps).
> >
> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > Tested-by: Lothar Waßmann <LW@KARO-electronics.de>
>
> Hello! Last time when I looked at this driver I was in impression that
> also _mxc_serial_setbrg() function requires calling some flush function
> at the beginning. Could you please check if it is needed or not? I'm
> really not sure.

_mxc_serial_setbrg is usually called after init, which now includes that flush.

>
> > ---
> >  v2: Add this commit to the series
> >  v3: Fix typo & reordering commits for good bisectability
> >
> >  drivers/serial/serial_mxc.c | 24 +++++++++++++++++++++++-
> >  1 file changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
> > index 82c0d84628..3c89781f2c 100644
> > --- a/drivers/serial/serial_mxc.c
> > +++ b/drivers/serial/serial_mxc.c
> > @@ -13,6 +13,7 @@
> >  #include <dm/platform_data/serial_mxc.h>
> >  #include <serial.h>
> >  #include <linux/compiler.h>
> > +#include <linux/delay.h>
> >
> >  /* UART Control Register Bit Fields.*/
> >  #define URXD_CHARRDY (1<<15)
> > @@ -144,8 +145,22 @@ struct mxc_uart {
> >       u32 ts;
> >  };
> >
> > +static void _mxc_serial_flush(struct mxc_uart *base)
> > +{
> > +     unsigned int timeout = 4000;
> > +
> > +     if (!(readl(&base->cr1) & UCR1_UARTEN) ||
> > +         !(readl(&base->cr2) & UCR2_TXEN))
> > +             return;
> > +
> > +     while (!(readl(&base->sr2) & USR2_TXDC) && --timeout)
> > +             udelay(1);
> > +}
> > +
> >  static void _mxc_serial_init(struct mxc_uart *base, int use_dte)
> >  {
> > +     _mxc_serial_flush(base);
> > +
> >       writel(0, &base->cr1);
> >       writel(0, &base->cr2);
> >
> > @@ -252,10 +267,17 @@ static int mxc_serial_init(void)
> >       return 0;
> >  }
> >
> > +static int mxc_serial_stop(void)
> > +{
> > +     _mxc_serial_flush(mxc_base);
> > +
> > +     return 0;
> > +}
> > +
> >  static struct serial_device mxc_serial_drv = {
> >       .name   = "mxc_serial",
> >       .start  = mxc_serial_init,
> > -     .stop   = NULL,
> > +     .stop   = mxc_serial_stop,
> >       .setbrg = mxc_serial_setbrg,
> >       .putc   = mxc_serial_putc,
> >       .puts   = default_serial_puts,
>
> Anyway, this code touches _only_ non-DM version of the driver. So same
> fix should be implemented also for DM version because non-DM is now
> legacy and will be removed in the future from U-Boot. Please look at the
> DM version too.

This code impacts both DM and non-DM, as _mxc_serial_init is a common version,
and was the main issue (several init() leading to corrupted char).

Regards,
Loic

On Wednesday 11 January 2023 08:53:30 Loic Poulain wrote:
> On Wed, 11 Jan 2023 at 00:53, Pali Rohár <pali@kernel.org> wrote:
> >
> > On Tuesday 10 January 2023 20:24:06 Loic Poulain wrote:
> > > The u-boot console may show some corrupted characters when
> > > printing in board_init() due to reset of the UART (probe)
> > > before the TX FIFO has been completely drained.
> > >
> > > To fix this issue, and in case UART is still running, we now
> > > try to flush the FIFO before proceeding to UART reinitialization.
> > > For this we're waiting for Transmitter Complete bit, indicating
> > > that the FIFO and the shift register are empty.
> > >
> > > flushing has a 4ms timeout guard, which is normally more than
> > > enough to consume the FIFO @ low baudrate (9600bps).
> > >
> > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > > Tested-by: Lothar Waßmann <LW@KARO-electronics.de>
> >
> > Hello! Last time when I looked at this driver I was in impression that
> > also _mxc_serial_setbrg() function requires calling some flush function
> > at the beginning. Could you please check if it is needed or not? I'm
> > really not sure.
> 
> _mxc_serial_setbrg is usually called after init, which now includes that flush.

I'm looking at it and this function is called also from
mxc_serial_setbrg() callback, which is used by u-boot to change baudrate
after the init.

> >
> > > ---
> > >  v2: Add this commit to the series
> > >  v3: Fix typo & reordering commits for good bisectability
> > >
> > >  drivers/serial/serial_mxc.c | 24 +++++++++++++++++++++++-
> > >  1 file changed, 23 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
> > > index 82c0d84628..3c89781f2c 100644
> > > --- a/drivers/serial/serial_mxc.c
> > > +++ b/drivers/serial/serial_mxc.c
> > > @@ -13,6 +13,7 @@
> > >  #include <dm/platform_data/serial_mxc.h>
> > >  #include <serial.h>
> > >  #include <linux/compiler.h>
> > > +#include <linux/delay.h>
> > >
> > >  /* UART Control Register Bit Fields.*/
> > >  #define URXD_CHARRDY (1<<15)
> > > @@ -144,8 +145,22 @@ struct mxc_uart {
> > >       u32 ts;
> > >  };
> > >
> > > +static void _mxc_serial_flush(struct mxc_uart *base)
> > > +{
> > > +     unsigned int timeout = 4000;
> > > +
> > > +     if (!(readl(&base->cr1) & UCR1_UARTEN) ||
> > > +         !(readl(&base->cr2) & UCR2_TXEN))
> > > +             return;
> > > +
> > > +     while (!(readl(&base->sr2) & USR2_TXDC) && --timeout)
> > > +             udelay(1);
> > > +}
> > > +
> > >  static void _mxc_serial_init(struct mxc_uart *base, int use_dte)
> > >  {
> > > +     _mxc_serial_flush(base);
> > > +
> > >       writel(0, &base->cr1);
> > >       writel(0, &base->cr2);
> > >
> > > @@ -252,10 +267,17 @@ static int mxc_serial_init(void)
> > >       return 0;
> > >  }
> > >
> > > +static int mxc_serial_stop(void)
> > > +{
> > > +     _mxc_serial_flush(mxc_base);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  static struct serial_device mxc_serial_drv = {
> > >       .name   = "mxc_serial",
> > >       .start  = mxc_serial_init,
> > > -     .stop   = NULL,
> > > +     .stop   = mxc_serial_stop,
> > >       .setbrg = mxc_serial_setbrg,
> > >       .putc   = mxc_serial_putc,
> > >       .puts   = default_serial_puts,
> >
> > Anyway, this code touches _only_ non-DM version of the driver. So same
> > fix should be implemented also for DM version because non-DM is now
> > legacy and will be removed in the future from U-Boot. Please look at the
> > DM version too.
> 
> This code impacts both DM and non-DM, as _mxc_serial_init is a common version,
> and was the main issue (several init() leading to corrupted char).
> 
> Regards,
> Loic

Yea, now I figured out.

On Wed, 11 Jan 2023 at 09:08, Pali Rohár <pali@kernel.org> wrote:
>
> On Wednesday 11 January 2023 08:53:30 Loic Poulain wrote:
> > On Wed, 11 Jan 2023 at 00:53, Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Tuesday 10 January 2023 20:24:06 Loic Poulain wrote:
> > > > The u-boot console may show some corrupted characters when
> > > > printing in board_init() due to reset of the UART (probe)
> > > > before the TX FIFO has been completely drained.
> > > >
> > > > To fix this issue, and in case UART is still running, we now
> > > > try to flush the FIFO before proceeding to UART reinitialization.
> > > > For this we're waiting for Transmitter Complete bit, indicating
> > > > that the FIFO and the shift register are empty.
> > > >
> > > > flushing has a 4ms timeout guard, which is normally more than
> > > > enough to consume the FIFO @ low baudrate (9600bps).
> > > >
> > > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > > > Tested-by: Lothar Waßmann <LW@KARO-electronics.de>
> > >
> > > Hello! Last time when I looked at this driver I was in impression that
> > > also _mxc_serial_setbrg() function requires calling some flush function
> > > at the beginning. Could you please check if it is needed or not? I'm
> > > really not sure.
> >
> > _mxc_serial_setbrg is usually called after init, which now includes that flush.
>
> I'm looking at it and this function is called also from
> mxc_serial_setbrg() callback, which is used by u-boot to change baudrate
> after the init.

Ok, good point, then it makes sense to add this guard here as well.