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

[Simon Goldschmidt]

On Wed, Oct 10, 2018 at 10:03 PM Simon Glass <sjg at chromium.org> wrote:
>
> 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?

On this specific platform (socfpga cylcone5/gen5), the preloader calls
a function before initializing the debug uart that is also called later and
that should emit an error in one case. And if this is the case (which it
can be depending on the boot source), then the system will hang when
debug uart is enabled (loop forever in the debug uart output code).

Now you can say we can initialize the debug uart earlier on this platform,
but that doesn't really work as it could be that the above function enables
a bus that is required for the debug uart (on socfpga, I could well imagine
the debug being implemented in FPGA and the offending function
enables the FPGA bridge).

Then you could argue to remove the offending printf, but I feel that debug
uart should help in debugging, not prevent it by entering an infinite loop
when printf is called at the wrong time.

I just feel it's better to lose some printf messages at the start than adding
a hang that's not present without debug uart.

And as much as I'd love to solve this in a general way, the multitudes of
different startup code does not seem to leave an easy way to solve this
other than I did...?

Simon

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