[U-Boot,1/1,v2] omap3_beagle: Enabling UART3 first allows the Transmitter to be empty

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 <man.huber@arcor.de>
---
  drivers/serial/ns16550.c |   12 ++++++++++--
  1 file changed, 10 insertions(+), 2 deletions(-)

  #if (defined(CONFIG_OMAP) && !defined(CONFIG_OMAP3_ZOOM2)) || \

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 <man.huber@arcor.de>
> ---
>   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

On Wed, Mar 27, 2013 at 5:50 AM, Manfred Huber <man.huber@arcor.de> 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 <man.huber@arcor.de>
>> ---
>>   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
>
>

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 <man.huber@arcor.de>
> ---
>  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

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 <man.huber@arcor.de> 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!

On Wed, Mar 27, 2013 at 2:37 PM, Andreas Bießmann
<andreas.devel@googlemail.com> 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 <man.huber@arcor.de>
>> ---
>>  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

On 2013-03-27 10:29, Javier Martinez Canillas wrote:
> On Wed, Mar 27, 2013 at 5:50 AM, Manfred Huber <man.huber@arcor.de> 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 <man.huber@arcor.de>
>>> ---
>>>    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
>>
>>

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 <man.huber@arcor.de>
>> ---
>>   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

Dear Manfred Huber,

On 03/28/2013 07:06 AM, Manfred Huber wrote:
> On 2013-03-27 14:37, Andreas Bießmann wrote:

<snip>

>> On 03/25/2013 11:02 PM, Manfred Huber wrote:

<snip>

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

On Thu, Mar 28, 2013 at 9:45 AM, Andreas Bießmann
<andreas.devel@googlemail.com> wrote:
> Dear Manfred Huber,
>
> On 03/28/2013 07:06 AM, Manfred Huber wrote:
>> On 2013-03-27 14:37, Andreas Bießmann wrote:
>
> <snip>
>
>>> On 03/25/2013 11:02 PM, Manfred Huber wrote:
>
> <snip>
>
>>>> +        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

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
> <andreas.devel@googlemail.com> wrote:
>> Dear Manfred Huber,
>>
>> On 03/28/2013 07:06 AM, Manfred Huber wrote:
>>> On 2013-03-27 14:37, Andreas Bießmann wrote:

<snip>

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

<snip>

> 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

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
> > <andreas.devel@googlemail.com> wrote:
> >> Dear Manfred Huber,
> >>
> >> On 03/28/2013 07:06 AM, Manfred Huber wrote:
> >>> On 2013-03-27 14:37, Andreas Bie??mann wrote:
> 
> <snip>
> 
> >>> 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.

> <snip>
> 
> > 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!

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.

On 2013-03-27 10:29, Javier Martinez Canillas wrote:
> On Wed, Mar 27, 2013 at 5:50 AM, Manfred Huber <man.huber@arcor.de> wrote:
<snip>
>
> 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

<snip>
>
> Thank a lot and best regards,
> Javier
>
>>
>> On 2013-03-25 23:02, Manfred Huber wrote:

<snip>

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:
>
> <snip>
>
>>> On 03/25/2013 11:02 PM, Manfred Huber wrote:
>
> <snip>
>
>>>> +        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);
>>>
<snip>
> 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.
>
<snip>

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