diff mbox series

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

Message ID 20181007175211.6727-1-simon.k.r.goldschmidt@gmail.com
State Rejected
Delegated to: Simon Glass
Headers show
Series [U-Boot] debug uart: don't print before initialization | expand

Commit Message

Simon Goldschmidt Oct. 7, 2018, 5:52 p.m. UTC
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.

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@gmail.com>
---

 include/asm-generic/global_data.h |  3 +++
 include/debug_uart.h              | 19 ++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

Comments

Simon Glass Oct. 9, 2018, 3:40 a.m. UTC | #1
Hi,

On 7 October 2018 at 11:52, Simon Goldschmidt
<simon.k.r.goldschmidt@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.

>
> 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@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
Simon Goldschmidt Oct. 9, 2018, 5:06 a.m. UTC | #2
On Tue, Oct 9, 2018 at 5:41 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi,
>
> On 7 October 2018 at 11:52, Simon Goldschmidt
> <simon.k.r.goldschmidt@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/

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@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
Simon Goldschmidt Oct. 10, 2018, 1:28 p.m. UTC | #3
+ 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@chromium.org> wrote:
>> Hi,
>>
>> On 7 October 2018 at 11:52, Simon Goldschmidt
>> <simon.k.r.goldschmidt@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).

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@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
Simon Glass Oct. 10, 2018, 8:03 p.m. UTC | #4
Hi Simon,

On 10 October 2018 at 07:28, Simon Goldschmidt
<simon.k.r.goldschmidt@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@chromium.org> wrote:
>>>
>>> Hi,
>>>
>>> On 7 October 2018 at 11:52, Simon Goldschmidt
>>> <simon.k.r.goldschmidt@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@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
>
>
>
Simon Goldschmidt Oct. 10, 2018, 8:16 p.m. UTC | #5
On Wed, Oct 10, 2018 at 10:03 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Simon,
>
> On 10 October 2018 at 07:28, Simon Goldschmidt
> <simon.k.r.goldschmidt@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@chromium.org> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On 7 October 2018 at 11:52, Simon Goldschmidt
> >>> <simon.k.r.goldschmidt@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@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
> >
> >
> >
Simon Goldschmidt Oct. 17, 2018, 8:02 p.m. UTC | #6
Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> schrieb am Mi., 10.
Okt. 2018, 22:16:

> On Wed, Oct 10, 2018 at 10:03 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Simon,
> >
> > On 10 October 2018 at 07:28, Simon Goldschmidt
> > <simon.k.r.goldschmidt@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@chromium.org> wrote:
> > >>>
> > >>> Hi,
> > >>>
> > >>> On 7 October 2018 at 11:52, Simon Goldschmidt
> > >>> <simon.k.r.goldschmidt@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@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
> > >
> > >
>

> >
>
Simon Glass Oct. 19, 2018, 3:25 a.m. UTC | #7
Hi Simon,

On 17 October 2018 at 14:02, Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> wrote:
>
>
> Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> schrieb am Mi., 10. Okt.
> 2018, 22:16:
>>
>> On Wed, Oct 10, 2018 at 10:03 PM Simon Glass <sjg@chromium.org> wrote:
>> >
>> > Hi Simon,
>> >
>> > On 10 October 2018 at 07:28, Simon Goldschmidt
>> > <simon.k.r.goldschmidt@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@chromium.org> wrote:
>> > >>>
>> > >>> Hi,
>> > >>>
>> > >>> On 7 October 2018 at 11:52, Simon Goldschmidt
>> > >>> <simon.k.r.goldschmidt@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).

Don't put your debug UART on device that needs lots of init. Only
Intel does that :-)

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

I think so.

Regards,
Simon
diff mbox series

Patch

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
 } gd_t;
 #endif
 
diff --git a/include/debug_uart.h b/include/debug_uart.h
index 34e8b2fc81..4133c5612c 100644
--- a/include/debug_uart.h
+++ b/include/debug_uart.h
@@ -128,9 +128,12 @@  void printhex8(uint value);
 \
 	static inline void _printch(int ch) \
 	{ \
-		if (ch == '\n') \
-			_debug_uart_putc('\r'); \
-		_debug_uart_putc(ch); \
+		DECLARE_GLOBAL_DATA_PTR; \
+		if (gd->debug_uart_initialized) { \
+			if (ch == '\n') \
+				_debug_uart_putc('\r'); \
+			_debug_uart_putc(ch); \
+		} \
 	} \
 \
 	void printch(int ch) \
@@ -146,8 +149,12 @@  void printhex8(uint value);
 \
 	static inline void printhex1(uint digit) \
 	{ \
-		digit &= 0xf; \
-		_debug_uart_putc(digit > 9 ? digit - 10 + 'a' : digit + '0'); \
+		DECLARE_GLOBAL_DATA_PTR; \
+		if (gd->debug_uart_initialized) { \
+			digit &= 0xf; \
+			_debug_uart_putc(digit > 9 ? digit - 10 + 'a' : \
+					 digit + '0'); \
+		} \
 	} \
 \
 	static inline void printhex(uint value, int digits) \
@@ -173,8 +180,10 @@  void printhex8(uint value);
 \
 	void debug_uart_init(void) \
 	{ \
+		DECLARE_GLOBAL_DATA_PTR; \
 		board_debug_uart_init(); \
 		_debug_uart_init(); \
+		gd->debug_uart_initialized = 1; \
 		_DEBUG_UART_ANNOUNCE \
 	} \