[U-Boot] [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.
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)) ;
serial_out(CONFIG_SYS_NS16550_IER, &com_port->ier);
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@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
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@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
participants (2)
-
Javier Martinez Canillas -
Scott Wood