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

[Marek Vasut]

On 08/10/2018 01:37 PM, Simon Goldschmidt wrote:
> On 10.08.2018 11:51, Marek Vasut wrote:
>> On 08/10/2018 07:22 AM, Simon Goldschmidt wrote:
>>> On 10.08.2018 00:41, Marek Vasut wrote:
>>>> On 08/10/2018 12:35 AM, Andy Shevchenko wrote:
>>>>> On Fri, Aug 10, 2018 at 12:45 AM, Marek Vasut <marex at denx.de> wrote:
>>>>>> 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.
>>>>>>>>    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)) {
>>>>>>>> +               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 ?
>>>>> Agree, why not to cache divisor value, for example, instead of doing
>>>>> slow I/O?
>>>> But why do we care about the divisor at all ?
>>> Because if the divisor is zero, the UART is disabled.
>>>
>>>> The real problem I believe
>>>> is that someone can call debug UART print/read functions before it is
>>>> inited.
>>>>
>>> I know this is a hack. I did it like that because I need something like
>>> this to get debug uart to work on socfpga gen5 (there always is a printf
>>> before debug uart init is possible).
>>>
>>> A generic solution for all debug uarts would be better of course, but
>>> given the point in SPL runtime, we might have to add a field to 'gd' for
>>> that, or does a global variable work at that point already?
>> GD field might be needed indeed.
>>
> Right. I'll drop this patch in the next version of the series and
> instead I'll try to work out something that works for all debug uarts
> drivers using a gd field.

Thanks, much appreciated :)

-- 
Best regards,
Marek Vasut