[U-Boot] [PATCH 1/1] serial/ns16550: check UART mode for TEMT value

[Scott Wood]

On 12/21/2012 03:21:46 AM, Javier Martinez Canillas wrote:
> On ns16550, the Transmitter Empty (TEMT) Bit is used to
> indicate when the Transmitter Holding Register (THR) and
> the Transmitter Shift Register (TSR) are both empty.
> 
> But ns16550 UART has two operation modes (16450 and FIFO)
> and the TEMT bit logic value set is different on each mode.
> 
> On 16450, the TEMT bit is set to 1 when both THR and TSR are
> empty and is set to 0 on FIFO mode.

When you say "on 16450", do you mean "in 16450 mode"?

> So, checking the TEMT value without checking the current mode
> and assuming a logical value of 1, can lead to U-Boot to hang
> forever if the UART is initialized on FIFO mode by default.

 From the 16550 docs:

   Bit 6 This bit is the Transmitter Empty (TEMT) indicator Bit 6 is set
   to a logic 1 whenever the Transmitter Holding Regis- ter (THR) and  
the
   Transmitter Shift Register (TSR) are both empty It is reset to a  
logic
   0 whenever either the THR or TSR contains a data character In the  
FIFO
   mode this bit is set to one whenever the transmitter FIFO and shift
   register are both empty

Maybe the 16550 implementation you're using is doing something wrong?

> Signed-off-by: Javier Martinez Canillas  
> <javier.martinez at collabora.co.uk>
> ---
>  drivers/serial/ns16550.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index bbd91ca..d75d814 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -36,7 +36,9 @@
> 
>  void NS16550_init(NS16550_t com_port, int baud_divisor)
>  {
> -	while (!(serial_in(&com_port->lsr) & UART_LSR_TEMT))
> +	int mode = serial_in(&com_port->fcr) & UART_FCR_FIFO_EN;
> +
> +	while (!(serial_in(&com_port->lsr) & UART_LSR_TEMT ^ mode))
>  		;

Please don't make the reader know the relative prededence of & and ^,  
and
don't use ^ in obfuscatory ways.  It's not even any different from | as
used above -- except that | wouldn't break if TEMT and FIFO_EN had the  
same
value -- so why do you need the exclusive form?

BTW, when I saw the problem that necessitated this, FIFO was enabled, so
this is no better than reverting the patch.

-Scott