serial/ns16550: check UART mode for TEMT value
PATCH 1/1 serial/ns16550: check UART mode for TEMT value
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.
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.
--- drivers/serial/ns16550.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
drivers/serial/ns16550.c +3 -1
@@ -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)) ;serial_out (CONFIG_SYS_NS16550_IER , &com_port->ier);
Signature · 1 line
1.7.7.6
PATCH 1/1 serial/ns16550: check UART mode for TEMT value
Quoted 9 lines · Javier Martinez Canillas
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"?
Quoted 3 lines
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?
Quoted 11 lines
Signed-off-by: Javier Martinez Canillas
<[email protected]>--- drivers/serial/ns16550.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)drivers/serial/ns16550.c +0
-0
drivers/serial/ns16550.c line 42
Quoted 8 lines
drivers/serial/ns16550.c +3
-1@@ -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
PATCH 1/1 serial/ns16550: check UART mode for TEMT value
Hi Scott, thank you for your feedback.
Quoted 5 lines · Scott Wood
Quoted 4 lines · Javier Martinez Canillas
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.
Quoted 8 lines
Quoted 5 lines
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.
Quoted 16 lines
Quoted 3 lines
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
Quoted 2 lines
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:
drivers/serial/ns16550.c +2 -0
@@ -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)) || \
Quoted 12 lines
Quoted 12 lines
Signed-off-by: Javier Martinez Canillas
<[email protected]>--- drivers/serial/ns16550.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)drivers/serial/ns16550.c +0
-0@@ -36,7 +36,9 @@
Quoted 15 lines
drivers/serial/ns16550.c line 42
Quoted 7 lines
drivers/serial/ns16550.c +3
-1void 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 andFIFO_ENhad 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.
Quoted 5 lines
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