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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4276 bytes --]

On Thu, Apr 03, 2025 at 12:54:42PM +1300, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 3 Apr 2025 at 08:57, Tom Rini <trini@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@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@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.

Because this is another little thing I'm being flexible on and not
naking, I just think it's wrong-headed. And you seem to think I'm never
being flexible. So I started this by saying I'm not naking it, I just
think it's wrong.

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

It's "cool" until it breaks. And grows platforms for features they
can't/won't support.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]