[PATCH] serial: ns16550: Wait in debug_uart_init until tx buffer is empty

[PATCH] serial: ns16550: Wait in debug_uart_init until tx buffer is empty

[Pali Rohár]

Commit d293759d55cc ("serial: ns16550: Add support for
SPL_DEBUG_UART_BASE") fixed support for setting correct early debug UART
base address in SPL.

But after this commit, output from Marvell A385 BootROM is truncated or
lost and not fully present on serial console.

Debugging this issue showed that BootROM just put bytes into UART HW output
buffer and does not wait until UART HW transmit all characters. U-Boot
ns16550 early debug is initialized very early and during its initialization
is resetting UART HW and flushing remaining transmit buffer (which still
contains BootROM output).

Fix this issue by waiting in init function prior resetting UART HW until
TxEmpty bit in UART Line Status Register is set. TxEmpty is set when all
remaining bytes from HW buffer are transmitted.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/serial/ns16550.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 78bfe6281ce3..13418004e2b0 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -328,6 +328,8 @@ static inline void _debug_uart_init(void)
 	struct ns16550 *com_port = (struct ns16550 *)CONFIG_VAL(DEBUG_UART_BASE);
 	int baud_divisor;
 
+	while (!(serial_din(&com_port->lsr) & UART_LSR_TEMT));
+
 	/*
 	 * We copy the code from above because it is already horribly messy.
 	 * Trying to refactor to nicely remove the duplication doesn't seem
-- 
2.20.1

Re: [PATCH] serial: ns16550: Wait in debug_uart_init until tx buffer is empty

[Stefan Roese]

On 23.06.22 14:13, Pali Rohár wrote:
> Commit d293759d55cc ("serial: ns16550: Add support for
> SPL_DEBUG_UART_BASE") fixed support for setting correct early debug UART
> base address in SPL.
> 
> But after this commit, output from Marvell A385 BootROM is truncated or
> lost and not fully present on serial console.
> 
> Debugging this issue showed that BootROM just put bytes into UART HW output
> buffer and does not wait until UART HW transmit all characters. U-Boot
> ns16550 early debug is initialized very early and during its initialization
> is resetting UART HW and flushing remaining transmit buffer (which still
> contains BootROM output).
> 
> Fix this issue by waiting in init function prior resetting UART HW until
> TxEmpty bit in UART Line Status Register is set. TxEmpty is set when all
> remaining bytes from HW buffer are transmitted.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Just checking, do you need a "Fixes" tag as well here?

> ---
>   drivers/serial/ns16550.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index 78bfe6281ce3..13418004e2b0 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -328,6 +328,8 @@ static inline void _debug_uart_init(void)
>   	struct ns16550 *com_port = (struct ns16550 *)CONFIG_VAL(DEBUG_UART_BASE);
>   	int baud_divisor;
>   
> +	while (!(serial_din(&com_port->lsr) & UART_LSR_TEMT));

I personally prefer the ";" in a separate line instead. Not sure what
the official coding style mentions to such constructs.

Anyways:

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

Re: [PATCH] serial: ns16550: Wait in debug_uart_init until tx buffer is empty

[Pali Rohár]

On Friday 01 July 2022 09:21:34 Stefan Roese wrote:
> On 23.06.22 14:13, Pali Rohár wrote:
> > Commit d293759d55cc ("serial: ns16550: Add support for
> > SPL_DEBUG_UART_BASE") fixed support for setting correct early debug UART
> > base address in SPL.
> > 
> > But after this commit, output from Marvell A385 BootROM is truncated or
> > lost and not fully present on serial console.
> > 
> > Debugging this issue showed that BootROM just put bytes into UART HW output
> > buffer and does not wait until UART HW transmit all characters. U-Boot
> > ns16550 early debug is initialized very early and during its initialization
> > is resetting UART HW and flushing remaining transmit buffer (which still
> > contains BootROM output).
> > 
> > Fix this issue by waiting in init function prior resetting UART HW until
> > TxEmpty bit in UART Line Status Register is set. TxEmpty is set when all
> > remaining bytes from HW buffer are transmitted.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> Just checking, do you need a "Fixes" tag as well here?

Well, ns16550_init() function already contains that waiting for
UART_LSR_TEMT bit. This was introduced in following commit years ago:
https://source.denx.de/u-boot/u-boot/-/commit/cb55b3320014b7f6014416c556fe506efbf0a84b

So probably the "fixes" tag could contain commit which introduced
_debug_uart_init() function. It was in this commit years ago:
https://source.denx.de/u-boot/u-boot/-/commit/21d004368fc8a4da07147c58dfe9a4e16d4ab761

Support for SPL_DEBUG_UART_BASE in my recent commit just caused to hit
issue that in _debug_uart_init() is missing waiting until TxEmpty.

> > ---
> >   drivers/serial/ns16550.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> > index 78bfe6281ce3..13418004e2b0 100644
> > --- a/drivers/serial/ns16550.c
> > +++ b/drivers/serial/ns16550.c
> > @@ -328,6 +328,8 @@ static inline void _debug_uart_init(void)
> >   	struct ns16550 *com_port = (struct ns16550 *)CONFIG_VAL(DEBUG_UART_BASE);
> >   	int baud_divisor;
> > +	while (!(serial_din(&com_port->lsr) & UART_LSR_TEMT));
> 
> I personally prefer the ";" in a separate line instead. Not sure what
> the official coding style mentions to such constructs.

I have no problem with this change. If you prefer it, feel free to move
";" to next separate line.

> Anyways:
> 
> Reviewed-by: Stefan Roese <sr@denx.de>
> 
> Thanks,
> Stefan

Re: [PATCH] serial: ns16550: Wait in debug_uart_init until tx buffer is empty

[Tom Rini]

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

On Thu, Jun 23, 2022 at 02:13:56PM +0200, Pali Rohár wrote:

> Commit d293759d55cc ("serial: ns16550: Add support for
> SPL_DEBUG_UART_BASE") fixed support for setting correct early debug UART
> base address in SPL.
> 
> But after this commit, output from Marvell A385 BootROM is truncated or
> lost and not fully present on serial console.
> 
> Debugging this issue showed that BootROM just put bytes into UART HW output
> buffer and does not wait until UART HW transmit all characters. U-Boot
> ns16550 early debug is initialized very early and during its initialization
> is resetting UART HW and flushing remaining transmit buffer (which still
> contains BootROM output).
> 
> Fix this issue by waiting in init function prior resetting UART HW until
> TxEmpty bit in UART Line Status Register is set. TxEmpty is set when all
> remaining bytes from HW buffer are transmitted.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Stefan Roese <sr@denx.de>

Applied to u-boot/next, thanks!

-- 
Tom

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