| Message ID | 20230110192407.1874044-1-loic.poulain@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/2] serial: mxc: Wait for TX completion before reset | expand |
[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.
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,