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

[Javier Martinez Canillas]

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

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:

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)
 {
+#ifndef CONFIG_SPL_BUILD
        while (!(serial_in(&com_port->lsr) & UART_LSR_TEMT))
                ;
+#endif

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

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

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,
Javier

[1]:http://www.ti.com/lit/ds/symlink/pc16550d.pdf