[PATCH v2] serial: mxc: Keep the original FIFO empty check

[PATCH v2] serial: mxc: Keep the original FIFO empty check

[Fabio Estevam]

From: Fabio Estevam <festevam@denx.de>

Tim Harvey reported that since commit c7878a0483c5 ("serial: mxc:
have putc use the TXFIFO"), console messages put inside board_init()
are no longer printed correctly.

This change added a check to handle the UART FIFO full condition and
removed the UART FIFO empty check, which causes the problem.

To avoid console corruption, add back the original UART FIFO empty
check so that when the FIFO is empty, mxc_serial_putc() returns -EAGAIN
to the core serial-uclass.c.

This way the serial core can properly handle the UART empty condition 
by not doing character transmission when -EAGAIN is returned.

Fixes: c7878a0483c5 ("serial: mxc: have putc use the TXFIFO")
Reported-by: Tim Harvey <tharvey@gateworks.com>
Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Changes since v1:
- Add the check for FIFO full and FIFO empty.

 drivers/serial/serial_mxc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
index 4cf79c1ca24f..d3fbe76065ec 100644
--- a/drivers/serial/serial_mxc.c
+++ b/drivers/serial/serial_mxc.c
@@ -311,7 +311,7 @@ static int mxc_serial_putc(struct udevice *dev, const char ch)
 	struct mxc_serial_plat *plat = dev_get_plat(dev);
 	struct mxc_uart *const uart = plat->reg;
 
-	if (readl(&uart->ts) & UTS_TXFULL)
+	if ((readl(&uart->ts) & UTS_TXFULL) || !(readl(&uart->ts) & UTS_TXEMPTY))
 		return -EAGAIN;
 
 	writel(ch, &uart->txd);
-- 
2.25.1

Re: [PATCH v2] serial: mxc: Keep the original FIFO empty check

[Tim Harvey]

On Tue, Oct 25, 2022 at 3:57 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> From: Fabio Estevam <festevam@denx.de>
>
> Tim Harvey reported that since commit c7878a0483c5 ("serial: mxc:
> have putc use the TXFIFO"), console messages put inside board_init()
> are no longer printed correctly.
>
> This change added a check to handle the UART FIFO full condition and
> removed the UART FIFO empty check, which causes the problem.
>
> To avoid console corruption, add back the original UART FIFO empty
> check so that when the FIFO is empty, mxc_serial_putc() returns -EAGAIN
> to the core serial-uclass.c.
>
> This way the serial core can properly handle the UART empty condition
> by not doing character transmission when -EAGAIN is returned.
>
> Fixes: c7878a0483c5 ("serial: mxc: have putc use the TXFIFO")
> Reported-by: Tim Harvey <tharvey@gateworks.com>
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
> Changes since v1:
> - Add the check for FIFO full and FIFO empty.
>
>  drivers/serial/serial_mxc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
> index 4cf79c1ca24f..d3fbe76065ec 100644
> --- a/drivers/serial/serial_mxc.c
> +++ b/drivers/serial/serial_mxc.c
> @@ -311,7 +311,7 @@ static int mxc_serial_putc(struct udevice *dev, const char ch)
>         struct mxc_serial_plat *plat = dev_get_plat(dev);
>         struct mxc_uart *const uart = plat->reg;
>
> -       if (readl(&uart->ts) & UTS_TXFULL)
> +       if ((readl(&uart->ts) & UTS_TXFULL) || !(readl(&uart->ts) & UTS_TXEMPTY))
>                 return -EAGAIN;
>
>         writel(ch, &uart->txd);
> --
> 2.25.1
>

Fabio,

This resolves the issue. Why would the kernel not suffer from this as
well? We are essentially saying if the FIFO is not full but also not
empty we need to wait.... so do we have a FIFO size of 1 or something
here as opposed to the kernel?

Best Regards,

Tim

Re: [PATCH v2] serial: mxc: Keep the original FIFO empty check

[Fabio Estevam]

Hi Tim,

On 25/10/2022 20:23, Tim Harvey wrote:

> Fabio,
> 
> This resolves the issue. Why would the kernel not suffer from this as
> well? We are essentially saying if the FIFO is not full but also not
> empty we need to wait.... so do we have a FIFO size of 1 or something
> here as opposed to the kernel?

The FIFO has a fixed size of 32 chars.

Kernel uses TXTL, which "controls the threshold at which a maskable 
interrupt is generated by the
TxFIFO".

We do not use interrupts in U-Boot. Prior to Johannes' patch, we only
checked for the TXFIFO empty condition, which avoided sending data when 
nothing was
available inside the FIFO (under-run condition).

Johannes' patch handles over-run condition.

My patch guarantees that both over-run and under-run conditions are 
handled.

When the number of chars in the FIFO is 1, 2, 3,...30, the transmission
can safely happen. Special cases are FIFO empty and FIFO full, which
returns -EAGAIN to the serial core.

Regards,

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

Re: [PATCH v2] serial: mxc: Keep the original FIFO empty check

[Michael Nazzareno Trimarchi]

Hi Fabio

On Wed, Oct 26, 2022 at 2:18 AM Fabio Estevam <festevam@denx.de> wrote:
>
> Hi Tim,
>
> On 25/10/2022 20:23, Tim Harvey wrote:
>
> > Fabio,
> >
> > This resolves the issue. Why would the kernel not suffer from this as
> > well? We are essentially saying if the FIFO is not full but also not
> > empty we need to wait.... so do we have a FIFO size of 1 or something
> > here as opposed to the kernel?
>
> The FIFO has a fixed size of 32 chars.
>
> Kernel uses TXTL, which "controls the threshold at which a maskable
> interrupt is generated by the
> TxFIFO".
>
> We do not use interrupts in U-Boot. Prior to Johannes' patch, we only
> checked for the TXFIFO empty condition, which avoided sending data when
> nothing was
> available inside the FIFO (under-run condition).
>
> Johannes' patch handles over-run condition.
>
> My patch guarantees that both over-run and under-run conditions are
> handled.
>
> When the number of chars in the FIFO is 1, 2, 3,...30, the transmission
> can safely happen. Special cases are FIFO empty and FIFO full, which
> returns -EAGAIN to the serial core.

Can you explain it better? If FIFO is empty there are no chars in the
serial, and
the transmission can happen only when you have at least one byte in the fifo.
The underrun condition in general is when the start of tx is enabled
but you have no bytes to send
at the time that bytes are needed. Was the case to limit the change on
the architecture that was tested?

Michael

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



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com