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

Message ID 50D52BB9.8050408@collabora.co.uk
State Rejected
Delegated to: Tom Rini
Headers show

Commit Message

Javier Martinez Canillas Dec. 22, 2012, 3:40 a.m.
Hi Scott, thank you for your feedback.

On 12/22/2012 03:45 AM, Scott Wood wrote:
> 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"?

Yes, that's what I meant. I'm not a native English speaker so sorry if I have
some grammatic errors in my comments.

>> 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  
>    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?

Could be, I'm using a board with an TI OMAP3 SoC which has an NS16650 UART.

Now I read again the NS16650 documentation [1] and you are right. I
misunderstood and thought that on FIFO mode the TEMT bit was set to 0 when the
both THR and TSR were empty and that this was causing U-Boot to hang.

BTW I realized that this is only an issue for SPL, if you build that check for
the non-SPL build, it works.

With this patch my board boots:

>> Signed-off-by: Javier Martinez Canillas  
>> <javier.martinez@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?

Ok, sorry. I didn't think that knowing the precedence of bitwise operator was
obfuscated and the ^ was because I misunderstood the NS16550 docs and thought
that UART_LSR_TEMT would be 0 in FIFO mode.

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

Either way works for me, I just want to boot my board :-)

But if I'm the only one having this issue maybe is just my hardware behaving
badly. I'll ask other OMAP3 users if they can boot with mainline U-Boot to
confirm this.

Best regards,



diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index bbd91ca..a3ef8a5 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -36,8 +36,10 @@ 

 void NS16550_init(NS16550_t com_port, int baud_divisor)
        while (!(serial_in(&com_port->lsr) & UART_LSR_TEMT))

        serial_out(CONFIG_SYS_NS16550_IER, &com_port->ier);
 #if (defined(CONFIG_OMAP) && !defined(CONFIG_OMAP3_ZOOM2)) || \