[U-Boot] omap3_beagle: Enable CONFIG_SYS_NS16550_BROKEN_TEMT

From: Manfred Huber

Beagleboard UART (ns16550) doesn't set the Transmitter Empty (TEMT) Bit 
in SPL. Only Transmitter Hold Register Empty (THRE) Bit is set. This 
makes SPL to hang while waiting for TEMT. Adding the 
CONFIG_SYS_NS16550_BROKEN_TEMT config option and waiting for THRE avoid 
this issue.

Signed-off-by: Manfred Huber <man.huber@arcor.de>
---
  drivers/serial/ns16550.c       |    5 ++++-
  include/configs/omap3_beagle.h |    3 +++
  2 files changed, 7 insertions(+), 1 deletion(-)

Hi Manfred,

On Thu, Mar 21, 2013 at 8:03 PM, Manfred Huber <man.huber@arcor.de> wrote:
> From: Manfred Huber
>
> Beagleboard UART (ns16550) doesn't set the Transmitter Empty (TEMT) Bit in
> SPL. Only Transmitter Hold Register Empty (THRE) Bit is set. This makes SPL
> to hang while waiting for TEMT. Adding the CONFIG_SYS_NS16550_BROKEN_TEMT
> config option and waiting for THRE avoid this issue.
>
> Signed-off-by: Manfred Huber <man.huber@arcor.de>
> ---
>  drivers/serial/ns16550.c       |    5 ++++-
>  include/configs/omap3_beagle.h |    3 +++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index b2da8b3..6379bcc 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -36,7 +36,10 @@
>
>  void NS16550_init(NS16550_t com_port, int baud_divisor)
>  {
> -#if (!defined(CONFIG_SYS_NS16550_BROKEN_TEMT))
> +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_SYS_NS16550_BROKEN_TEMT)
> +       while (!(serial_in(&com_port->lsr) & UART_LSR_THRE))
> +               ;
> +#else

I'm not sure to add this behavior under CONFIG_SYS_NS16550_BROKEN_TEMT
since this option just means:

"On some broken platforms the Transmitter Empty (TEMT) bit is not set
in SPL making U-Boot to hang while waiting for it"

According to your findings it seems that some OMAP3 platforms (at
least DM3730 and 3530) set THRE but I wonder if other broken platforms
will behave the same.

The current CONFIG_SYS_NS16550_BROKEN_TEMT just skips this test so it
can be reused by other platforms, but now your change makes it less
generic since it will only work on platforms that set THRE.

So I would just leave CONFIG_SYS_NS16550_BROKEN_TEMT as is now and be
able to reuse it on other platforms or add a new config option
CONFIG_SYS_NS16550_WAIT_THRE or something that would test for THRE
instead of TEMT and use that for OMAP3 based boards.

I do like your change that adds a test for CONFIG_SPL_BUILD since even
in the README it says that the issue is only present in SPL. So I just
forgot to add this when added the config option.

>         while (!(serial_in(&com_port->lsr) & UART_LSR_TEMT))
>                 ;
>  #endif
> diff --git a/include/configs/omap3_beagle.h b/include/configs/omap3_beagle.h
> index 48ce4c0..6ab46d5 100644
> --- a/include/configs/omap3_beagle.h
> +++ b/include/configs/omap3_beagle.h
> @@ -82,6 +82,9 @@
>  #define CONFIG_SYS_NS16550_REG_SIZE    (-4)
>  #define CONFIG_SYS_NS16550_CLK         V_NS16550_CLK
>
> +/* define to avoid U-Boot to hang while waiting for TEMT */
> +#define CONFIG_SYS_NS16550_BROKEN_TEMT
> +
>  /*
>   * select serial console configuration
>   */
>

This part of your patch looks good to me but you should split your
changes in two patches. One to change the ns16550 drvier and another
one to add this config option for the Beagleboard.

Thanks a lot and best regards,
Javier

On Thu, Mar 21, 2013 at 08:03:59PM +0100, Manfred Huber wrote:
> From: Manfred Huber
> 
> Beagleboard UART (ns16550) doesn't set the Transmitter Empty (TEMT)
> Bit in SPL. Only Transmitter Hold Register Empty (THRE) Bit is set.
> This makes SPL to hang while waiting for TEMT. Adding the
> CONFIG_SYS_NS16550_BROKEN_TEMT config option and waiting for THRE
> avoid this issue.
> 
> Signed-off-by: Manfred Huber <man.huber@arcor.de>
> ---
>  drivers/serial/ns16550.c       |    5 ++++-
>  include/configs/omap3_beagle.h |    3 +++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index b2da8b3..6379bcc 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -36,7 +36,10 @@
> 
>  void NS16550_init(NS16550_t com_port, int baud_divisor)
>  {
> -#if (!defined(CONFIG_SYS_NS16550_BROKEN_TEMT))
> +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_SYS_NS16550_BROKEN_TEMT)
> +	while (!(serial_in(&com_port->lsr) & UART_LSR_THRE))
> +		;
> +#else
>  	while (!(serial_in(&com_port->lsr) & UART_LSR_TEMT))
>  		;
>  #endif

Scott, do you still have access to the failing systems that made us
introduce this change to start with?  Could we perhaps go with the THRE
test instead in all cases?  Thanks!