[U-Boot] [PATCH] debug uart: don't print before initialization

[Simon Glass]

Hi Simon,

On 10 October 2018 at 07:28, Simon Goldschmidt
<simon.k.r.goldschmidt at gmail.com> wrote:
>
> + Marek (as he commented on the original patch http://patchwork.ozlabs.org/patch/955765/)
>
> On 09.10.2018 07:06, Simon Goldschmidt wrote:
>>
>> On Tue, Oct 9, 2018 at 5:41 AM Simon Glass <sjg at chromium.org> wrote:
>>>
>>> Hi,
>>>
>>> On 7 October 2018 at 11:52, Simon Goldschmidt
>>> <simon.k.r.goldschmidt at gmail.com> wrote:
>>>>
>>>> At least on socfpga gen5, _debug_uart_putc() can be called
>>>> before debug_uart_init(), which leaves us stuck in an
>>>> infinite loop in the ns16550 debug uart driver.
>>>
>>> Can you fix that? That is a bug.
>>
>> I already posted a patch for that but it was rejected:
>> http://patchwork.ozlabs.org/patch/955765/
>
>
> I'd have to add I still thing that that patch is good to fix this:
> It checks that the baudrate divisor is set, which effectively is
> an 'enable' bit for this hardware.
>
> There were comments about the style (we can talk about that)
> and Marek rejected it because he wanted a generic solution.
> But honestly, given your idea that some platforms init the debug
> uart before setting up gd, I don't think I can find a solution for
> this.
>
> So I'd really like to get my original patch applied (see above).

Yes I think your patch is best - it is specific to the hardware which
is the only way you can safely detect whether the UART is enabled.

We cannot rely on state like global_data to tell if the debug UART is
enabled. We could use a flag in the DATA section on some devices, but
that is also pretty nasty and we've been trying to avoid adding such
things and using global_data instead.

So I am OK with the original patch.

However before going further I'd like to understand your thoughts on
this question: I feel that you are checking for something that should
not happen - i.e. the debug UART must be inited before use, just like
any other device. We don't in general put these sorts of checks into
other drivers. Why is the debug UART different?

Regards,
Simon

>
> Simon
>
>
>>
>> As patman automatically choses the CC addresses, you weren't
>> on the CC list back then, since that patch covered different filfes.
>>
>>
>> Simon
>>
>>>> Since this prevents debugging startup problems instead
>>>> of helping, let's add a field to 'gd' that prevents
>>>> calling the _debug_uart_putc() until debug_uart_init()
>>>> has been called.
>>>>
>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
>>>> ---
>>>>
>>>>   include/asm-generic/global_data.h |  3 +++
>>>>   include/debug_uart.h              | 19 ++++++++++++++-----
>>>>   2 files changed, 17 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
>>>> index c83fc01b76..9de7f48476 100644
>>>> --- a/include/asm-generic/global_data.h
>>>> +++ b/include/asm-generic/global_data.h
>>>> @@ -122,6 +122,9 @@ typedef struct global_data {
>>>>          struct list_head log_head;      /* List of struct log_device */
>>>>          int log_fmt;                    /* Mask containing log format info */
>>>>   #endif
>>>> +#ifdef CONFIG_DEBUG_UART
>>>> +       int debug_uart_initialized;     /* No print before debug_uart_init */
>>>> +#endif
>>>
>>> There is no requirement that gd be set up before the debug UART is
>>> running. It certainly isn't on the few platforms I know about.
>>>
>>> Regards,
>>> Simon
>
>
>