[PATCH v5 40/46] boot: Support IO UARTs for earlycon and console

[Simon Glass]

Hi Tom,

On Thu, 3 Apr 2025 at 08:57, Tom Rini <trini at konsulko.com> wrote:
>
> On Wed, Apr 02, 2025 at 01:50:33PM -0600, Tom Rini wrote:
> > On Thu, Apr 03, 2025 at 08:22:44AM +1300, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 3 Apr 2025 at 03:29, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Sat, Mar 15, 2025 at 02:26:00PM +0000, Simon Glass wrote:
> > > > > Update the string to take account of UARTs which are connected on I/O
> > > > > ports, as on x86.
> > > > >
> > > > > Fix a typo in an error message in the same command, while we are here.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > ---
> > > > >
> > > > > (no changes since v3)
> > > > >
> > > > > Changes in v3:
> > > > > - Add new patch to support IO UARTs for earlycon and console
> > > > >
> > > > >  boot/bootflow.c | 7 ++++---
> > > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > > > > index 58a1afa7a75..4054a966af8 100644
> > > > > --- a/boot/bootflow.c
> > > > > +++ b/boot/bootflow.c
> > > > > @@ -942,8 +942,9 @@ int bootflow_cmdline_auto(struct bootflow *bflow, const char *arg)
> > > > >       *buf = '\0';
> > > > >       if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_16550_COMPATIBLE) {
> > > > >               snprintf(buf, sizeof(buf),
> > > > > -                      "uart8250,mmio32,%#lx,%dn8", info.addr,
> > > > > -                      info.baudrate);
> > > > > +                      "uart8250,%s,%#lx,%dn8",
> > > > > +                      info.addr_space == SERIAL_ADDRESS_SPACE_IO ? "io" :
> > > > > +                      "mmio", info.addr, info.baudrate);
> > > > >       } else if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_PL01X) {
> > > > >               snprintf(buf, sizeof(buf),
> > > > >                        "pl011,mmio32,%#lx,%dn8", info.addr,
> > > >
> > > > I suppose we're well past the point where we can delete
> > > > bootflow_cmdline_auto() itself because that's just going to lead us to
> > > > trouble down the line (5 years from now when the kernel adopts a new
> > > > preferred way to pass this info) and grows every platform by some amount
> > > > of space every time we add something new here.
> > >
> > > Well firstly, why would you want to delete this command? It is very
> > > useful to be able to change the cmdline.
> > >
> > > This command is only available with BOOTSTD_FULL, which is less than
> > > 10% of boards.
> >
> > Because it's automatic non-obvious stuff. We should not be modifying the
> > command line at all. Is it even documented that we're doing this?

Yes, the command is documented. It only happens when you run the
command. Otherwise the command line is not modified.

As you know U-Boot has bootargs but that doesn't work with extlinux
and the like, so we have this command, which does.

>
> To be clearer, the more I review your changes, the more I see a blurred
> line that I do not this is good between "I found this handy while
> doing something" and "This is a good generic design / feature".
>
> That it can be annoying at times to add the debug uart information is
> not a new unique problem. It's something that's generally normally
> solved. I assume you hit this on x86 where it's more annoying than most.
> But a generic feature it should not have been.

I'm not even sure why you wrote this email, then.

U-Boot has all the information needed to set up the UART correctly and
it is extremely tedious to do otherwise. This is a bootloader, so it
needs to support kernel development. I actually think this is a really
cool feature.

At this point, I really would encourage you to look at what you are
trying to achieve with all these 'no' emails. Collected together they
create a substantial corpus of negativity. Why not just start saying
'yes' instead of 'no' for a while and see how things go?

Regards,
Simon