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

[Manfred Huber <man.huber@arcor.de>]

On 2013-03-27 14:37, Andreas Bie?mann wrote:
> Dear Manfred Huber,
>
> ---8<---
> abiessmann at 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 at 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 at 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