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

[Simon Goldschmidt]

Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com> schrieb am Mi., 10.
Okt. 2018, 22:16:

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

So, without further answers, can we push the original patch? (
http://patchwork.ozlabs.org/patch/955765/ )


Simon



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

> >
>