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

[Andreas Bießmann]

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?

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