[U-Boot] [PATCH 5/6] serial: ns16550: fix debug uart putc called before init

[Marek Vasut]

On 08/09/2018 11:13 PM, Adam Ford wrote:
> On Thu, Aug 9, 2018 at 2:08 PM Simon Goldschmidt
> <simon.k.r.goldschmidt at gmail.com> wrote:
>>
>> If _debug_uart_putc() is called before _debug_uart_init(), the
>> ns16550 debug uart driver hangs in a tight loop waiting for the
>> tx FIFO to get empty.
>>
>> As this can happen via a printf sneaking in before the port calls
>> debug_uart_init(), let's rather ignore characters before the debug
>> uart is initialized.
>>
>> This is done by reading the baudrate divisor and aborting if is zero.
>>
>> Tested on socfpga_cyclone5_socrates.
>>
>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
>> ---
>> v2:
>>  - this patch is new in v2 of the series. It replaces the printf/debug
>>   change in reset_manager_gen5.c from v1
>>
>>  drivers/serial/ns16550.c | 18 ++++++++++++++++--
>>  1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>> index 9c80090aa7..475075c03c 100644
>> --- a/drivers/serial/ns16550.c
>> +++ b/drivers/serial/ns16550.c
>> @@ -266,12 +266,26 @@ static inline void _debug_uart_init(void)
>>         serial_dout(&com_port->lcr, UART_LCRVAL);
>>  }
>>
>> +static inline int NS16550_read_baud_divisor(struct NS16550 *com_port)
>> +{
>> +       int ret;
>> +
>> +       serial_dout(&com_port->lcr, UART_LCR_BKSE | UART_LCRVAL);
>> +       ret = serial_din(&com_port->dll) & 0xff;
>> +       ret |= (serial_din(&com_port->dlm) & 0xff) << 8;
>> +       serial_dout(&com_port->lcr, UART_LCRVAL);
>> +
>> +       return ret;
>> +}
>> +
>>  static inline void _debug_uart_putc(int ch)
>>  {
>>         struct NS16550 *com_port = (struct NS16550 *)CONFIG_DEBUG_UART_BASE;
>>
>> -       while (!(serial_din(&com_port->lsr) & UART_LSR_THRE))
>> -               ;
>> +       while (!(serial_din(&com_port->lsr) & UART_LSR_THRE)) {
>> +               if (!NS16550_read_baud_divisor(com_port))
> 
> Unless there is a change that the read_baud_divisor will change while
> we're waiting for the character, could we move this check before the
> while statement?  This would reduce the check for the divisor to 1x
> and the while statement would only have one comparison to do.  I
> realize it's rather trivial, but the way I see it, there is no reason
> to do the while statement at all if the read_baud_divisor fails and
> there if there is a baud divisor, we should only need to check it
> once.

This looks like a massive hack -- what about having a flag which says
that the debug uart was/was not inited somewhere ?

-- 
Best regards,
Marek Vasut