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

[Simon Glass]

Hi Simon,

On 17 October 2018 at 14:02, Simon Goldschmidt
<simon.k.r.goldschmidt at gmail.com> wrote:
>
>
> 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).

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