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

["Pali Rohár" <pali@kernel.org>]

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