* [U-Boot] [PATCH 1/1] serial/ns16550: check UART mode for TEMT value @ 2012-12-21 9:21 Javier Martinez Canillas 2012-12-22 2:45 ` Scott Wood 0 siblings, 1 reply; 6+ messages in thread From: Javier Martinez Canillas @ 2012-12-21 9:21 UTC (permalink / raw) To: u-boot 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); -- 1.7.7.6 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 1/1] serial/ns16550: check UART mode for TEMT value 2012-12-21 9:21 [U-Boot] [PATCH 1/1] serial/ns16550: check UART mode for TEMT value Javier Martinez Canillas @ 2012-12-22 2:45 ` Scott Wood 2012-12-22 3:40 ` Javier Martinez Canillas 0 siblings, 1 reply; 6+ messages in thread From: Scott Wood @ 2012-12-22 2:45 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 1/1] serial/ns16550: check UART mode for TEMT value 2012-12-22 2:45 ` Scott Wood @ 2012-12-22 3:40 ` Javier Martinez Canillas 0 siblings, 0 replies; 6+ messages in thread From: Javier Martinez Canillas @ 2012-12-22 3:40 UTC (permalink / raw) To: u-boot 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 [1]:http://www.ti.com/lit/ds/symlink/pc16550d.pdf ^ permalink raw reply related [flat|nested] 6+ messages in thread
[parent not found: <CAAwP0s0y=hPh3FrTSLptd_5UnbYGTV47s=6hVkR8aL0-Z_Ejeg@mail.gmail.com>]
* [U-Boot] [PATCH 1/1] serial/ns16550: check UART mode for TEMT value [not found] <CAAwP0s0y=hPh3FrTSLptd_5UnbYGTV47s=6hVkR8aL0-Z_Ejeg@mail.gmail.com> @ 2013-01-03 2:58 ` Scott Wood 2013-01-03 21:04 ` Tom Rini 0 siblings, 1 reply; 6+ messages in thread From: Scott Wood @ 2013-01-03 2:58 UTC (permalink / raw) To: u-boot On 12/23/2012 05:17:25 PM, Javier Martinez Canillas wrote: > On Sat, Dec 22, 2012 at 4:40 AM, Javier Martinez Canillas > <javier.martinez@collabora.co.uk> wrote: > > > > 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. > > > > Hello, > > I tested with an Beagleboard which is another OMAP3 board that has the > same PC16550D UART than my OMAP3 IGEPv2 and the board does not hang > while waiting for TEMT. I've also tested with an OMAP4 pandaboard and > it boots correctly too. > > But the hung issue seems to not be an issue with my IGEPv2 board but > with any IGEPv2 board since another user reported that his board hangs > with the latest U-Boot too. Reverting cb55b332 (serial/ns16550: wait > for TEMT before initializing) solved the issue for him. > > Will a patch like this be an acceptable solution? > > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c > index bbd91ca..8106a9a 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) > { > +#if (CONFIG_MACH_TYPE != MACH_TYPE_IGEP0020) > while (!(serial_in(&com_port->lsr) & UART_LSR_TEMT)) > ; > +#endif Probably better to have your board config file define CONFIG_SYS_NS16550_BROKEN_TEMT or similar (and document the symbol in README). -Scott ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 1/1] serial/ns16550: check UART mode for TEMT value 2013-01-03 2:58 ` Scott Wood @ 2013-01-03 21:04 ` Tom Rini 2013-01-07 11:37 ` Javier Martinez Canillas 0 siblings, 1 reply; 6+ messages in thread From: Tom Rini @ 2013-01-03 21:04 UTC (permalink / raw) To: u-boot On Wed, Jan 2, 2013 at 9:58 PM, Scott Wood <scottwood@freescale.com> wrote: > On 12/23/2012 05:17:25 PM, Javier Martinez Canillas wrote: >> >> On Sat, Dec 22, 2012 at 4:40 AM, Javier Martinez Canillas >> <javier.martinez@collabora.co.uk> wrote: >> > >> > 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. >> > >> >> Hello, >> >> I tested with an Beagleboard which is another OMAP3 board that has the >> same PC16550D UART than my OMAP3 IGEPv2 and the board does not hang >> while waiting for TEMT. I've also tested with an OMAP4 pandaboard and >> it boots correctly too. >> >> But the hung issue seems to not be an issue with my IGEPv2 board but >> with any IGEPv2 board since another user reported that his board hangs >> with the latest U-Boot too. Reverting cb55b332 (serial/ns16550: wait >> for TEMT before initializing) solved the issue for him. >> >> Will a patch like this be an acceptable solution? >> >> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c >> index bbd91ca..8106a9a 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) >> { >> +#if (CONFIG_MACH_TYPE != MACH_TYPE_IGEP0020) >> while (!(serial_in(&com_port->lsr) & UART_LSR_TEMT)) >> ; >> +#endif > > > Probably better to have your board config file define > CONFIG_SYS_NS16550_BROKEN_TEMT or similar (and document the symbol in > README). To be clear, yes, please, that way. Thanks! -- Tom ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 1/1] serial/ns16550: check UART mode for TEMT value 2013-01-03 21:04 ` Tom Rini @ 2013-01-07 11:37 ` Javier Martinez Canillas 0 siblings, 0 replies; 6+ messages in thread From: Javier Martinez Canillas @ 2013-01-07 11:37 UTC (permalink / raw) To: u-boot On Thu, Jan 3, 2013 at 10:04 PM, Tom Rini <trini@ti.com> wrote: > On Wed, Jan 2, 2013 at 9:58 PM, Scott Wood <scottwood@freescale.com> wrote: >> On 12/23/2012 05:17:25 PM, Javier Martinez Canillas wrote: >>> >>> On Sat, Dec 22, 2012 at 4:40 AM, Javier Martinez Canillas >>> <javier.martinez@collabora.co.uk> wrote: >>> > >>> > 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. >>> > >>> >>> Hello, >>> >>> I tested with an Beagleboard which is another OMAP3 board that has the >>> same PC16550D UART than my OMAP3 IGEPv2 and the board does not hang >>> while waiting for TEMT. I've also tested with an OMAP4 pandaboard and >>> it boots correctly too. >>> >>> But the hung issue seems to not be an issue with my IGEPv2 board but >>> with any IGEPv2 board since another user reported that his board hangs >>> with the latest U-Boot too. Reverting cb55b332 (serial/ns16550: wait >>> for TEMT before initializing) solved the issue for him. >>> >>> Will a patch like this be an acceptable solution? >>> >>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c >>> index bbd91ca..8106a9a 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) >>> { >>> +#if (CONFIG_MACH_TYPE != MACH_TYPE_IGEP0020) >>> while (!(serial_in(&com_port->lsr) & UART_LSR_TEMT)) >>> ; >>> +#endif >> >> >> Probably better to have your board config file define >> CONFIG_SYS_NS16550_BROKEN_TEMT or similar (and document the symbol in >> README). > > To be clear, yes, please, that way. Thanks! > > -- > Tom Ok, thanks a lot for the suggestion. I just sent a patch-set that adds this config option and uses it on IGEP board config file. Best regards, Javier ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-01-07 11:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-21 9:21 [U-Boot] [PATCH 1/1] serial/ns16550: check UART mode for TEMT value Javier Martinez Canillas
2012-12-22 2:45 ` Scott Wood
2012-12-22 3:40 ` Javier Martinez Canillas
[not found] <CAAwP0s0y=hPh3FrTSLptd_5UnbYGTV47s=6hVkR8aL0-Z_Ejeg@mail.gmail.com>
2013-01-03 2:58 ` Scott Wood
2013-01-03 21:04 ` Tom Rini
2013-01-07 11:37 ` Javier Martinez Canillas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox