Re: [U-Boot] [PATCH 1/1 v2] omap3_beagle: Enabling UART3 first allows the Transmitter to be empty
Am 28.03.2013 09:45, schrieb Andreas Bießmann: Dear Manfred Huber, On 03/28/2013 07:06 AM, Manfred Huber wrote: On 2013-03-27 14:37, Andreas Bießmann wrote: On 03/25/2013 11:02 PM, Manfred Huber wrote: +serial_out(UART_LCR_DLAB, &com_port->lcr); +serial_out(baud_divisor & 0xff, &com_port->dll); +serial_out((baud_divisor >> 8) & 0xff, &com_port->dlm); +serial_out(UART_LCRVAL, &com_port->lcr); +serial_out(0, &com_port->mdr1); I wonder which use-case requires UART flushing in u-boot context before initializing the UART for u-boot correctly. Can someone explain this to me? Shouldn't we always start here from the very beginning and setup UART as configured? Beagleboard has several ways to boot (NAND, SD/MMC, UART, ...). For the boot mode with UART, Beagleboard configures the UART and ends with a non empty transmitter. In a booting sequence where UART is before NAND, SD/MMC or wherever SPL starts from, we have not a clean UART. It's not critical. So I guess it's not needed for this release. Well, if there are boards in the field that will not boot with the next release I think it is critical. We do have some omap3 (omap35xx and am37xx) based boards here. I can recall a situation where some few boards did not boot from sd-card while serial debug cable was attached (AFAIR this was not the case when booting from NAND). The root cause was never investigated, so maybe we suffered exactly this bug. Can you test this boars with my patch? Best regards Andreas Bießmann ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/1 v2] omap3_beagle: Enabling UART3 first allows the Transmitter to be empty
On 2013-03-27 10:29, Javier Martinez Canillas wrote: On Wed, Mar 27, 2013 at 5:50 AM, Manfred Huber wrote: I still think that we should keep CONFIG_SYS_NS16550_BROKEN_TEMT or something similar instead of just checking for CONFIG_OMAP34XX. Since we don't know if this problem is also present on other TI OMAP platforms (or any other platform). I would just change CONFIG_SYS_NS16550_BROKEN_TEMT semantics once is confirmed that this also fixes the issue on IGEP boards. If we continue using CONFIG_SYS_NS16550_BROKEN_TEMT, there is no benefit by my patch. It works with the define alone. Also, if you are removing CONFIG_SYS_NS16550_BROKEN_TEMT then you have to update the README too since it is explained there what this config option does. And also in igep00x0.h Thank a lot and best regards, Javier On 2013-03-25 23:02, Manfred Huber wrote: ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/1 v2] omap3_beagle: Enabling UART3 first allows the Transmitter to be empty
On Wed, Mar 27, 2013 at 05:50:17AM +0100, Manfred Huber wrote: > Please test the Patch. It is very simple on a Beagleboard. I guess you > have flashed the actual SPL and u-boot and Beagleboard boots correctly. > Now press and hold 'User' button and connect power. SPL should hang. > You can see some symbols on the console from the ROM code. I cannot reproduce this problem on my Beagleboard C5. I note this only so it's clear I tried things out. I'm still supportive of the changes overall as they also don't break anything here. I'm also still waiting to hear back from some folks that might know what the ROM code does or doesn't do. -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/1 v2] omap3_beagle: Enabling UART3 first allows the Transmitter to be empty
On Thu, Mar 28, 2013 at 10:50:44AM +0100, Andreas Bie??mann wrote: > Hi Javier, > > On 03/28/2013 10:11 AM, Javier Martinez Canillas wrote: > > On Thu, Mar 28, 2013 at 9:45 AM, Andreas Bie??mann > > wrote: > >> Dear Manfred Huber, > >> > >> On 03/28/2013 07:06 AM, Manfred Huber wrote: > >>> On 2013-03-27 14:37, Andreas Bie??mann wrote: > > > > >>> The reason to setup the baud is for the shift register. It only works > >>> with programmed baud registers. A soft reset would also work, but as > >>> Scott Wood said it would corrupt the last character. On the other hand > >>> the character should be corrupted by disabling the UART. I have no > >>> preferred solution: programming the UART or a soft reset. Maybe someone > >>> wants to decide. > >> > >> Well, I think also that re-programming the UART will destroy the last > >> character in shift register anyway. > >> I wonder which use-case requires UART flushing in u-boot context before > >> initializing the UART for u-boot correctly. Can someone explain this to > >> me? Shouldn't we always start here from the very beginning and setup > >> UART as configured? We shouldn't be concerned with what is destroyed here as it's going to be related to ROM trying (and then failing and moving on from) to load via UART the next program. We've already started and are running, so the UART is ours to do with as we need. If ROM did load us over UART it really should already be in a clean state. > > > > When I first hit this bug I tried removing the serial debug cable and > > this made my IGEPv2 to boot correctly. Then I looked at the latest > > changes to the serial ns16550 driver and found that cb55b332 > > "serial/ns16550: wait for TEMT before initializing" was the culprit > > commit that made my board to not boot. > > Thanks for clarification. So there is a need to flush UART before > initializing it in u-boot context as said by Scott's comment in > cb55b332, at least for some systems. > > I think Manfred's proposed solution in this patch is ok. It fixes a bug > when transiting from (some) OMAP ROM code to SPL, therefore we should > have the code conditionally on SPL and OMAP. Maybe we find out later, > that this also matches other combinations. But for now I think we should > just take Manfred's solution in this release, Tom? Agreed. > Many thanks to you Manfred for forcing attention on this bug and > providing a solution. And also very much agreed! -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/1 v2] omap3_beagle: Enabling UART3 first allows the Transmitter to be empty
Hi Javier, On 03/28/2013 10:11 AM, Javier Martinez Canillas wrote: > On Thu, Mar 28, 2013 at 9:45 AM, Andreas Bießmann > wrote: >> Dear Manfred Huber, >> >> On 03/28/2013 07:06 AM, Manfred Huber wrote: >>> On 2013-03-27 14:37, Andreas Bießmann wrote: >>> The reason to setup the baud is for the shift register. It only works >>> with programmed baud registers. A soft reset would also work, but as >>> Scott Wood said it would corrupt the last character. On the other hand >>> the character should be corrupted by disabling the UART. I have no >>> preferred solution: programming the UART or a soft reset. Maybe someone >>> wants to decide. >> >> Well, I think also that re-programming the UART will destroy the last >> character in shift register anyway. >> I wonder which use-case requires UART flushing in u-boot context before >> initializing the UART for u-boot correctly. Can someone explain this to >> me? Shouldn't we always start here from the very beginning and setup >> UART as configured? > When I first hit this bug I tried removing the serial debug cable and > this made my IGEPv2 to boot correctly. Then I looked at the latest > changes to the serial ns16550 driver and found that cb55b332 > "serial/ns16550: wait for TEMT before initializing" was the culprit > commit that made my board to not boot. Thanks for clarification. So there is a need to flush UART before initializing it in u-boot context as said by Scott's comment in cb55b332, at least for some systems. I think Manfred's proposed solution in this patch is ok. It fixes a bug when transiting from (some) OMAP ROM code to SPL, therefore we should have the code conditionally on SPL and OMAP. Maybe we find out later, that this also matches other combinations. But for now I think we should just take Manfred's solution in this release, Tom? Many thanks to you Manfred for forcing attention on this bug and providing a solution. Best regards Andreas Bießmann ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/1 v2] omap3_beagle: Enabling UART3 first allows the Transmitter to be empty
On Thu, Mar 28, 2013 at 9:45 AM, Andreas Bießmann wrote: > Dear Manfred Huber, > > On 03/28/2013 07:06 AM, Manfred Huber wrote: >> On 2013-03-27 14:37, Andreas Bießmann wrote: > > > >>> On 03/25/2013 11:02 PM, Manfred Huber wrote: > > > +serial_out(UART_LCR_DLAB, &com_port->lcr); +serial_out(baud_divisor & 0xff, &com_port->dll); +serial_out((baud_divisor >> 8) & 0xff, &com_port->dlm); +serial_out(UART_LCRVAL, &com_port->lcr); +serial_out(0, &com_port->mdr1); >>> >>> Do we need to setup baud a.s.o. here? Isn't it enough to issue an soft >>> reset of the UART? I'm not in this material, I just wonder if we can >>> omit some of the lines here cause we set e.g. the BAUD later on. >>> >> >> The reason to setup the baud is for the shift register. It only works >> with programmed baud registers. A soft reset would also work, but as >> Scott Wood said it would corrupt the last character. On the other hand >> the character should be corrupted by disabling the UART. I have no >> preferred solution: programming the UART or a soft reset. Maybe someone >> wants to decide. > > Well, I think also that re-programming the UART will destroy the last > character in shift register anyway. > I wonder which use-case requires UART flushing in u-boot context before > initializing the UART for u-boot correctly. Can someone explain this to > me? Shouldn't we always start here from the very beginning and setup > UART as configured? > >> +} +#endif + 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)) || \ >>> >>> I managed to apply your patch anyhow. A short test on a tricorder board >>> showed no harm to the boot process. So please get your patch clean and >>> resend, then I will add my tested-by. >>> >>> As Javier pointed out please think about using the >>> CONFIG_SYS_NS16550_BROKEN_TEMT instead of SPL && OMAP34XX. >>> Another solution could be to have this TEMT | THRE check in >>> unconditionally, this however would require a lot more testing. >>> Especially with the release date in mind. >> >> It's not critical. So I guess it's not needed for this release. > > Well, if there are boards in the field that will not boot with the next > release I think it is critical. > We do have some omap3 (omap35xx and am37xx) based boards here. I can > recall a situation where some few boards did not boot from sd-card while > serial debug cable was attached (AFAIR this was not the case when > booting from NAND). The root cause was never investigated, so maybe we > suffered exactly this bug. > > Best regards > > Andreas Bießmann Hi Andreas, When I first hit this bug I tried removing the serial debug cable and this made my IGEPv2 to boot correctly. Then I looked at the latest changes to the serial ns16550 driver and found that cb55b332 "serial/ns16550: wait for TEMT before initializing" was the culprit commit that made my board to not boot. So, if I remember correctly, it seems as you hit the exact same bug (I didn't try to boot from NAND back then though) Best regards, Javier ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/1 v2] omap3_beagle: Enabling UART3 first allows the Transmitter to be empty
Dear Manfred Huber, On 03/28/2013 07:06 AM, Manfred Huber wrote: > On 2013-03-27 14:37, Andreas Bießmann wrote: >> On 03/25/2013 11:02 PM, Manfred Huber wrote: >>> +serial_out(UART_LCR_DLAB, &com_port->lcr); >>> +serial_out(baud_divisor & 0xff, &com_port->dll); >>> +serial_out((baud_divisor >> 8) & 0xff, &com_port->dlm); >>> +serial_out(UART_LCRVAL, &com_port->lcr); >>> +serial_out(0, &com_port->mdr1); >> >> Do we need to setup baud a.s.o. here? Isn't it enough to issue an soft >> reset of the UART? I'm not in this material, I just wonder if we can >> omit some of the lines here cause we set e.g. the BAUD later on. >> > > The reason to setup the baud is for the shift register. It only works > with programmed baud registers. A soft reset would also work, but as > Scott Wood said it would corrupt the last character. On the other hand > the character should be corrupted by disabling the UART. I have no > preferred solution: programming the UART or a soft reset. Maybe someone > wants to decide. Well, I think also that re-programming the UART will destroy the last character in shift register anyway. I wonder which use-case requires UART flushing in u-boot context before initializing the UART for u-boot correctly. Can someone explain this to me? Shouldn't we always start here from the very beginning and setup UART as configured? > >>> +} >>> +#endif >>> + >>> 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)) || \ >> >> I managed to apply your patch anyhow. A short test on a tricorder board >> showed no harm to the boot process. So please get your patch clean and >> resend, then I will add my tested-by. >> >> As Javier pointed out please think about using the >> CONFIG_SYS_NS16550_BROKEN_TEMT instead of SPL && OMAP34XX. >> Another solution could be to have this TEMT | THRE check in >> unconditionally, this however would require a lot more testing. >> Especially with the release date in mind. > > It's not critical. So I guess it's not needed for this release. Well, if there are boards in the field that will not boot with the next release I think it is critical. We do have some omap3 (omap35xx and am37xx) based boards here. I can recall a situation where some few boards did not boot from sd-card while serial debug cable was attached (AFAIR this was not the case when booting from NAND). The root cause was never investigated, so maybe we suffered exactly this bug. Best regards Andreas Bießmann ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/1 v2] omap3_beagle: Enabling UART3 first allows the Transmitter to be empty
On 2013-03-27 14:37, Andreas Bießmann wrote:
Dear Manfred Huber,
---8<---
abiessmann@punisher % pwclient get 230994
Saved patch to
U-Boot-1-1-v2-omap3_beagle-Enabling-UART3-first-allows-the-Transmitter-to-be-empty.patch
abiessmann@punisher % git am
U-Boot-1-1-v2-omap3_beagle-Enabling-UART3-first-allows-the-Transmitter-to-be-empty.patch
Patch does not have a valid e-mail address.
abiessmann@punisher % ./tools/checkpatch.pl
U-Boot-1-1-v2-omap3_beagle-Enabling-UART3-first-allows-the-Transmitter-to-be-empty.patch
ERROR: trailing whitespace
#39: FILE: drivers/serial/ns16550.c:40:
+^Iif ((serial_in(&com_port->lsr) & (UART_LSR_TEMT | UART_LSR_THRE)) == $
ERROR: patch seems to be corrupt (line wrapped?)
#40: FILE: drivers/serial/ns16550.c:40:
UART_LSR_THRE) {
total: 2 errors, 0 warnings, 20 lines checked
NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
scripts/cleanfile
NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
MULTISTATEMENT_MACRO_USE_DO_WHILE
U-Boot-1-1-v2-omap3_beagle-Enabling-UART3-first-allows-the-Transmitter-to-be-empty.patch
has style problems, please review.
If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
--->8---
Can you please fix these errors?
I will do it.
On 03/25/2013 11:02 PM, Manfred Huber wrote:
From: Manfred Huber
Due to a Bug in the ROM code of some OMAP3 devices, the TEMT bit is not
set if UART3 is configured before (only THRE is set). Reason is the
disabling of UART3 even though the Transmitter is not empty. Enabling
UART3 allows the Transmitter to be empty.
Signed-off-by: Manfred Huber
---
drivers/serial/ns16550.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index b2da8b3..24ff84f 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -36,10 +36,18 @@
void NS16550_init(NS16550_t com_port, int baud_divisor)
{
-#if (!defined(CONFIG_SYS_NS16550_BROKEN_TEMT))
+#if (defined(CONFIG_SPL_BUILD) && defined(CONFIG_OMAP34XX))
I think a comment here would be good.
I will do it.
+if ((serial_in(&com_port->lsr) & (UART_LSR_TEMT | UART_LSR_THRE))
== UART_LSR_THRE) {
The patch is broken here, even if you fix the wrapping you will get an
'over 80 char' error here.
+serial_out(UART_LCR_DLAB, &com_port->lcr);
+serial_out(baud_divisor & 0xff, &com_port->dll);
+serial_out((baud_divisor >> 8) & 0xff, &com_port->dlm);
+serial_out(UART_LCRVAL, &com_port->lcr);
+serial_out(0, &com_port->mdr1);
Do we need to setup baud a.s.o. here? Isn't it enough to issue an soft
reset of the UART? I'm not in this material, I just wonder if we can
omit some of the lines here cause we set e.g. the BAUD later on.
The reason to setup the baud is for the shift register. It only works
with programmed baud registers. A soft reset would also work, but as
Scott Wood said it would corrupt the last character. On the other hand
the character should be corrupted by disabling the UART. I have no
preferred solution: programming the UART or a soft reset. Maybe someone
wants to decide.
+}
+#endif
+
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)) || \
I managed to apply your patch anyhow. A short test on a tricorder board
showed no harm to the boot process. So please get your patch clean and
resend, then I will add my tested-by.
As Javier pointed out please think about using the
CONFIG_SYS_NS16550_BROKEN_TEMT instead of SPL && OMAP34XX.
Another solution could be to have this TEMT | THRE check in
unconditionally, this however would require a lot more testing.
Especially with the release date in mind.
It's not critical. So I guess it's not needed for this release.
Best regards
Andreas Bießmann
Best regards,
Manfred
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/1 v2] omap3_beagle: Enabling UART3 first allows the Transmitter to be empty
On 2013-03-27 10:29, Javier Martinez Canillas wrote:
On Wed, Mar 27, 2013 at 5:50 AM, Manfred Huber wrote:
Please test the Patch. It is very simple on a Beagleboard. I guess you
have flashed the actual SPL and u-boot and Beagleboard boots correctly.
Now press and hold 'User' button and connect power. SPL should hang.
You can see some symbols on the console from the ROM code.
Hi Manfred,
I don't have access to my IGEP board right now but I'll test your
patch as soon as posible.
Install the Patch, compile it and flash the NAND. Beagleboard still
boots correctly. Now press and hold 'User' button again and Beagleboard
should also boot correctly. The Patch works.
I suspect the IGEP board has the same bug. If so, the Patch should work
on this board too and we don't need CONFIG_SYS_NS16550_BROKEN_TEMT
anymore.
I still think that we should keep CONFIG_SYS_NS16550_BROKEN_TEMT or
something similar instead of just checking for CONFIG_OMAP34XX. Since
we don't know if this problem is also present on other TI OMAP
platforms (or any other platform). I would just change
CONFIG_SYS_NS16550_BROKEN_TEMT semantics once is confirmed that this
also fixes the issue on IGEP boards.
Also, if you are removing CONFIG_SYS_NS16550_BROKEN_TEMT then you have
to update the README too since it is explained there what this config
option does.
If you don't want a patch for this bug please let me know. I will not
bother you again.
Thanks a lot for working on this. Some people work on mainline U-Boot
on their free time (like myself) so it can take a few days until you
get a response about a patch. Please be patient :-)
Hi Javier,
Also like me. The patch is not for me, because I have written my own
serial driver. So this comment was only not to waste your and my time if
no one needs this patch.
Best regards,
Manfred
Best regards,
Manfred
Thank a lot and best regards,
Javier
On 2013-03-25 23:02, Manfred Huber wrote:
From: Manfred Huber
Due to a Bug in the ROM code of some OMAP3 devices, the TEMT bit is not
set if UART3 is configured before (only THRE is set). Reason is the
disabling of UART3 even though the Transmitter is not empty. Enabling
UART3 allows the Transmitter to be empty.
Signed-off-by: Manfred Huber
---
drivers/serial/ns16550.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index b2da8b3..24ff84f 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -36,10 +36,18 @@
void NS16550_init(NS16550_t com_port, int baud_divisor)
{
-#if (!defined(CONFIG_SYS_NS16550_BROKEN_TEMT))
+#if (defined(CONFIG_SPL_BUILD) && defined(CONFIG_OMAP34XX))
+if ((serial_in(&com_port->lsr) & (UART_LSR_TEMT | UART_LSR_THRE))
== UART_LSR_THRE) {
+serial_out(UART_LCR_DLAB, &com_port->lcr);
+serial_out(baud_divisor & 0xff, &com_port->dll);
+serial_out((baud_divisor >> 8) & 0xff, &com_port->dlm);
+serial_out(UART_LCRVAL, &com_port->lcr);
+serial_out(0, &com_port->mdr1);
+}
+#endif
+
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)) || \
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/1 v2] omap3_beagle: Enabling UART3 first allows the Transmitter to be empty
On Wed, Mar 27, 2013 at 2:37 PM, Andreas Bießmann
wrote:
> Dear Manfred Huber,
>
> ---8<---
> abiessmann@punisher % pwclient get 230994
> Saved patch to
> U-Boot-1-1-v2-omap3_beagle-Enabling-UART3-first-allows-the-Transmitter-to-be-empty.patch
> abiessmann@punisher % git am
> U-Boot-1-1-v2-omap3_beagle-Enabling-UART3-first-allows-the-Transmitter-to-be-empty.patch
> Patch does not have a valid e-mail address.
> abiessmann@punisher % ./tools/checkpatch.pl
> U-Boot-1-1-v2-omap3_beagle-Enabling-UART3-first-allows-the-Transmitter-to-be-empty.patch
> ERROR: trailing whitespace
> #39: FILE: drivers/serial/ns16550.c:40:
> +^Iif ((serial_in(&com_port->lsr) & (UART_LSR_TEMT | UART_LSR_THRE)) == $
>
> ERROR: patch seems to be corrupt (line wrapped?)
> #40: FILE: drivers/serial/ns16550.c:40:
> UART_LSR_THRE) {
>
> total: 2 errors, 0 warnings, 20 lines checked
>
> NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
> scripts/cleanfile
>
> NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
> MULTISTATEMENT_MACRO_USE_DO_WHILE
>
> U-Boot-1-1-v2-omap3_beagle-Enabling-UART3-first-allows-the-Transmitter-to-be-empty.patch
> has style problems, please review.
>
> If any of these errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
> --->8---
>
> Can you please fix these errors?
>
> On 03/25/2013 11:02 PM, Manfred Huber wrote:
>> From: Manfred Huber
>>
>> Due to a Bug in the ROM code of some OMAP3 devices, the TEMT bit is not
>> set if UART3 is configured before (only THRE is set). Reason is the
>> disabling of UART3 even though the Transmitter is not empty. Enabling
>> UART3 allows the Transmitter to be empty.
>>
>> Signed-off-by: Manfred Huber
>> ---
>> drivers/serial/ns16550.c | 12 ++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>> index b2da8b3..24ff84f 100644
>> --- a/drivers/serial/ns16550.c
>> +++ b/drivers/serial/ns16550.c
>> @@ -36,10 +36,18 @@
>>
>> void NS16550_init(NS16550_t com_port, int baud_divisor)
>> {
>> -#if (!defined(CONFIG_SYS_NS16550_BROKEN_TEMT))
>> +#if (defined(CONFIG_SPL_BUILD) && defined(CONFIG_OMAP34XX))
>
> I think a comment here would be good.
>
>> +if ((serial_in(&com_port->lsr) & (UART_LSR_TEMT | UART_LSR_THRE))
>> == UART_LSR_THRE) {
>
> The patch is broken here, even if you fix the wrapping you will get an
> 'over 80 char' error here.
>
>> +serial_out(UART_LCR_DLAB, &com_port->lcr);
>> +serial_out(baud_divisor & 0xff, &com_port->dll);
>> +serial_out((baud_divisor >> 8) & 0xff, &com_port->dlm);
>> +serial_out(UART_LCRVAL, &com_port->lcr);
>> +serial_out(0, &com_port->mdr1);
>
> Do we need to setup baud a.s.o. here? Isn't it enough to issue an soft
> reset of the UART? I'm not in this material, I just wonder if we can
> omit some of the lines here cause we set e.g. the BAUD later on.
>
>> +}
>> +#endif
>> +
>> 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)) || \
>
> I managed to apply your patch anyhow. A short test on a tricorder board
> showed no harm to the boot process. So please get your patch clean and
> resend, then I will add my tested-by.
>
> As Javier pointed out please think about using the
> CONFIG_SYS_NS16550_BROKEN_TEMT instead of SPL && OMAP34XX.
> Another solution could be to have this TEMT | THRE check in
> unconditionally, this however would require a lot more testing.
> Especially with the release date in mind.
>
> Best regards
>
> Andreas Bießmann
Hi Manfred,
I just tested your patch and my IGEPv2 boots correctly.
So, after addressing the issues pointed out by Andreas feel free to
add my Tested-by too.
Thanks a lot and best regards,
Javier
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/1 v2] omap3_beagle: Enabling UART3 first allows the Transmitter to be empty
On Wed, Mar 27, 2013 at 10:29:00AM +0100, Javier Martinez Canillas wrote: > On Wed, Mar 27, 2013 at 5:50 AM, Manfred Huber wrote: > > Please test the Patch. It is very simple on a Beagleboard. I guess you > > have flashed the actual SPL and u-boot and Beagleboard boots correctly. > > Now press and hold 'User' button and connect power. SPL should hang. > > You can see some symbols on the console from the ROM code. > > > > Hi Manfred, > > I don't have access to my IGEP board right now but I'll test your > patch as soon as posible. > > > Install the Patch, compile it and flash the NAND. Beagleboard still > > boots correctly. Now press and hold 'User' button again and Beagleboard > > should also boot correctly. The Patch works. > > > > I suspect the IGEP board has the same bug. If so, the Patch should work > > on this board too and we don't need CONFIG_SYS_NS16550_BROKEN_TEMT > > anymore. > > > > I still think that we should keep CONFIG_SYS_NS16550_BROKEN_TEMT or > something similar instead of just checking for CONFIG_OMAP34XX. Since > we don't know if this problem is also present on other TI OMAP > platforms (or any other platform). I would just change > CONFIG_SYS_NS16550_BROKEN_TEMT semantics once is confirmed that this > also fixes the issue on IGEP boards. I'm attempting to see if this ROM bug is (a) known and (b) is or was fixed at some point. Based on what I know (or at least think I know) of how the ROM code lived for "OMAP3", given that you're seeing this on beagle and igep, it's a valid assumption that OMAP34XX is buggy. The question I have (and hope for resolution on) is if it's a problem on am335x/omap4 or later as well. If I follow how to reproduce this issue (and I think I do), assuming I can I'll setup an am335x platform in a similar position. > > If you don't want a patch for this bug please let me know. I will not > > bother you again. > > > > Thanks a lot for working on this. Some people work on mainline U-Boot > on their free time (like myself) so it can take a few days until you > get a response about a patch. Please be patient :-) Yes, thanks for working this issue through, I also appreciate it! -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/1 v2] omap3_beagle: Enabling UART3 first allows the Transmitter to be empty
Dear Manfred Huber,
---8<---
abiessmann@punisher % pwclient get 230994
Saved patch to
U-Boot-1-1-v2-omap3_beagle-Enabling-UART3-first-allows-the-Transmitter-to-be-empty.patch
abiessmann@punisher % git am
U-Boot-1-1-v2-omap3_beagle-Enabling-UART3-first-allows-the-Transmitter-to-be-empty.patch
Patch does not have a valid e-mail address.
abiessmann@punisher % ./tools/checkpatch.pl
U-Boot-1-1-v2-omap3_beagle-Enabling-UART3-first-allows-the-Transmitter-to-be-empty.patch
ERROR: trailing whitespace
#39: FILE: drivers/serial/ns16550.c:40:
+^Iif ((serial_in(&com_port->lsr) & (UART_LSR_TEMT | UART_LSR_THRE)) == $
ERROR: patch seems to be corrupt (line wrapped?)
#40: FILE: drivers/serial/ns16550.c:40:
UART_LSR_THRE) {
total: 2 errors, 0 warnings, 20 lines checked
NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
scripts/cleanfile
NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
MULTISTATEMENT_MACRO_USE_DO_WHILE
U-Boot-1-1-v2-omap3_beagle-Enabling-UART3-first-allows-the-Transmitter-to-be-empty.patch
has style problems, please review.
If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
--->8---
Can you please fix these errors?
On 03/25/2013 11:02 PM, Manfred Huber wrote:
> From: Manfred Huber
>
> Due to a Bug in the ROM code of some OMAP3 devices, the TEMT bit is not
> set if UART3 is configured before (only THRE is set). Reason is the
> disabling of UART3 even though the Transmitter is not empty. Enabling
> UART3 allows the Transmitter to be empty.
>
> Signed-off-by: Manfred Huber
> ---
> drivers/serial/ns16550.c | 12 ++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index b2da8b3..24ff84f 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -36,10 +36,18 @@
>
> void NS16550_init(NS16550_t com_port, int baud_divisor)
> {
> -#if (!defined(CONFIG_SYS_NS16550_BROKEN_TEMT))
> +#if (defined(CONFIG_SPL_BUILD) && defined(CONFIG_OMAP34XX))
I think a comment here would be good.
> +if ((serial_in(&com_port->lsr) & (UART_LSR_TEMT | UART_LSR_THRE))
> == UART_LSR_THRE) {
The patch is broken here, even if you fix the wrapping you will get an
'over 80 char' error here.
> +serial_out(UART_LCR_DLAB, &com_port->lcr);
> +serial_out(baud_divisor & 0xff, &com_port->dll);
> +serial_out((baud_divisor >> 8) & 0xff, &com_port->dlm);
> +serial_out(UART_LCRVAL, &com_port->lcr);
> +serial_out(0, &com_port->mdr1);
Do we need to setup baud a.s.o. here? Isn't it enough to issue an soft
reset of the UART? I'm not in this material, I just wonder if we can
omit some of the lines here cause we set e.g. the BAUD later on.
> +}
> +#endif
> +
> 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)) || \
I managed to apply your patch anyhow. A short test on a tricorder board
showed no harm to the boot process. So please get your patch clean and
resend, then I will add my tested-by.
As Javier pointed out please think about using the
CONFIG_SYS_NS16550_BROKEN_TEMT instead of SPL && OMAP34XX.
Another solution could be to have this TEMT | THRE check in
unconditionally, this however would require a lot more testing.
Especially with the release date in mind.
Best regards
Andreas Bießmann
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/1 v2] omap3_beagle: Enabling UART3 first allows the Transmitter to be empty
On Wed, Mar 27, 2013 at 5:50 AM, Manfred Huber wrote:
> Please test the Patch. It is very simple on a Beagleboard. I guess you
> have flashed the actual SPL and u-boot and Beagleboard boots correctly.
> Now press and hold 'User' button and connect power. SPL should hang.
> You can see some symbols on the console from the ROM code.
>
Hi Manfred,
I don't have access to my IGEP board right now but I'll test your
patch as soon as posible.
> Install the Patch, compile it and flash the NAND. Beagleboard still
> boots correctly. Now press and hold 'User' button again and Beagleboard
> should also boot correctly. The Patch works.
>
> I suspect the IGEP board has the same bug. If so, the Patch should work
> on this board too and we don't need CONFIG_SYS_NS16550_BROKEN_TEMT
> anymore.
>
I still think that we should keep CONFIG_SYS_NS16550_BROKEN_TEMT or
something similar instead of just checking for CONFIG_OMAP34XX. Since
we don't know if this problem is also present on other TI OMAP
platforms (or any other platform). I would just change
CONFIG_SYS_NS16550_BROKEN_TEMT semantics once is confirmed that this
also fixes the issue on IGEP boards.
Also, if you are removing CONFIG_SYS_NS16550_BROKEN_TEMT then you have
to update the README too since it is explained there what this config
option does.
> If you don't want a patch for this bug please let me know. I will not
> bother you again.
>
Thanks a lot for working on this. Some people work on mainline U-Boot
on their free time (like myself) so it can take a few days until you
get a response about a patch. Please be patient :-)
> Best regards,
> Manfred
>
>
Thank a lot and best regards,
Javier
>
> On 2013-03-25 23:02, Manfred Huber wrote:
>>
>> From: Manfred Huber
>>
>> Due to a Bug in the ROM code of some OMAP3 devices, the TEMT bit is not
>> set if UART3 is configured before (only THRE is set). Reason is the
>> disabling of UART3 even though the Transmitter is not empty. Enabling
>> UART3 allows the Transmitter to be empty.
>>
>> Signed-off-by: Manfred Huber
>> ---
>> drivers/serial/ns16550.c | 12 ++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>> index b2da8b3..24ff84f 100644
>> --- a/drivers/serial/ns16550.c
>> +++ b/drivers/serial/ns16550.c
>> @@ -36,10 +36,18 @@
>>
>> void NS16550_init(NS16550_t com_port, int baud_divisor)
>> {
>> -#if (!defined(CONFIG_SYS_NS16550_BROKEN_TEMT))
>> +#if (defined(CONFIG_SPL_BUILD) && defined(CONFIG_OMAP34XX))
>> +if ((serial_in(&com_port->lsr) & (UART_LSR_TEMT | UART_LSR_THRE))
>> == UART_LSR_THRE) {
>> +serial_out(UART_LCR_DLAB, &com_port->lcr);
>> +serial_out(baud_divisor & 0xff, &com_port->dll);
>> +serial_out((baud_divisor >> 8) & 0xff, &com_port->dlm);
>> +serial_out(UART_LCRVAL, &com_port->lcr);
>> +serial_out(0, &com_port->mdr1);
>> +}
>> +#endif
>> +
>> 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)) || \
>> ___
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>
>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/1 v2] omap3_beagle: Enabling UART3 first allows the Transmitter to be empty
Please test the Patch. It is very simple on a Beagleboard. I guess you
have flashed the actual SPL and u-boot and Beagleboard boots correctly.
Now press and hold 'User' button and connect power. SPL should hang.
You can see some symbols on the console from the ROM code.
Install the Patch, compile it and flash the NAND. Beagleboard still
boots correctly. Now press and hold 'User' button again and Beagleboard
should also boot correctly. The Patch works.
I suspect the IGEP board has the same bug. If so, the Patch should work
on this board too and we don't need CONFIG_SYS_NS16550_BROKEN_TEMT
anymore.
If you don't want a patch for this bug please let me know. I will not
bother you again.
Best regards,
Manfred
On 2013-03-25 23:02, Manfred Huber wrote:
From: Manfred Huber
Due to a Bug in the ROM code of some OMAP3 devices, the TEMT bit is not
set if UART3 is configured before (only THRE is set). Reason is the
disabling of UART3 even though the Transmitter is not empty. Enabling
UART3 allows the Transmitter to be empty.
Signed-off-by: Manfred Huber
---
drivers/serial/ns16550.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index b2da8b3..24ff84f 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -36,10 +36,18 @@
void NS16550_init(NS16550_t com_port, int baud_divisor)
{
-#if (!defined(CONFIG_SYS_NS16550_BROKEN_TEMT))
+#if (defined(CONFIG_SPL_BUILD) && defined(CONFIG_OMAP34XX))
+if ((serial_in(&com_port->lsr) & (UART_LSR_TEMT | UART_LSR_THRE))
== UART_LSR_THRE) {
+serial_out(UART_LCR_DLAB, &com_port->lcr);
+serial_out(baud_divisor & 0xff, &com_port->dll);
+serial_out((baud_divisor >> 8) & 0xff, &com_port->dlm);
+serial_out(UART_LCRVAL, &com_port->lcr);
+serial_out(0, &com_port->mdr1);
+}
+#endif
+
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)) || \
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
