[U-Boot] [PATCH v2 3/4] tegra: Implement pre-console putc() for fdt warning

[Simon Glass]

+Graeme

Hi Wolfgang,

On Sat, Mar 10, 2012 at 12:08 PM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ1=KK9UNy2fyk9WJ-gcyYE5JgwqO_cT8igXjD-OFP_h6w at mail.gmail.com> you wrote:
>>
>> >> +void board_pre_console_putc(int ch)
>> >> +{
>> > ...
>> >> +     for (uart_addr = uart_reg_addr; *uart_addr; uart_addr++) {
>> >> +             NS16550_t regs = (NS16550_t)*uart_addr;
>> >> +
>> >> +             NS16550_init(regs, divisor);
>> >> +             NS16550_putc(regs, ch);
>> >> +             if (ch == '\n')
>> >> +                     NS16550_putc(regs, '\r');
>> >> +             NS16550_drain(regs);
>> >
>> > Why is this needed for every output character?
>> >
>> > Actually, why is it needed at all?
>>
>> Of course in this case the init could be done for each UART at the
>> start of the function rather than in the loop, by looping through the
>> UARTs twice.
>
> Both the init() and the drain() should never be used in a character
> output loop.

Yes I agree, but I can't move them out with the API as it is. If we
move back to a puts() type function then I could do this.

>
>> After requests on the list for general purpose pre-console output
>> function (the purpose of which I didn't necessary see) I changed this
>> to a putc() mechanism. This means that we need to set up the UARTs
>> each time it is called. We can't really add a flag to global data
>> since this might be called before that is even set up. There might be
>> a better way, but I'm not sure what it is.
>>
>> For SPL I would like to be able use this same mechanism to call
>> panic() when something goes wrong, and use the same mechanism to get a
>> message to the user. Again, this avoids a bricked unit with no failure
>> indication.
>
> I understand your intention, but I disagree with the design, and with
> the implementation.

It's not great, but let's work out something that is better.

>
> I think outputting data to all "potential" console ports is a really
> bad thing, as you cannot know how your users are using the hardware.
> They may have attached hardware to the UARTs, and the data you send to
> the port causes a mis-function of the device.  I guess you don't add
> to your documentation a warning like: select one port as console, and
> leave allother ports unused, because we may send random date to all
> ports any time?

Only on a fatal error. Unfortunately this idea of 'fatal error' was
lost in the conversion from board_panic_no_console() to
board_pre_console_putc(). I would be keen to move back to that
original plan, so that the idea of a fatal error is captured. In a
fatal error situation there is no prospect of the board working and
the only hope is that we can alert the user.

>
> If you do not know which UART port to use, then the only consequence
> can be not to use any UART prt at all.  Use a LED with blink codes or
> whatever your hardwar ehas, but do not mess with random ports.

I agres with the sentiment and this is a very ugly corner case, but in
practice people want to know what happened, not just be presented with
a brick.

>
> I also cannot understand why you think you must init() and drain() the
> UART for each character printed - ok, the latter is probably a
> consequence of re-initializing the port for each character.
> Eventually this will be not needed once you get rid of the re-init.

OK so how about moving to a puts()-type interface? Then I can remove this.

Regards,
Simon

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> "Consistency requires you to be as ignorant today as you were a  year
> ago."                                              - Bernard Berenson