[PATCH 03/17] x86: Allow coreboot serial driver to guess the UART

[Bin Meng]

Hi Simon,

On Fri, Apr 30, 2021 at 12:10 AM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Bin,
>
> On Sun, 25 Apr 2021 at 18:21, Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Sun, 25 Apr 2021 at 13:49, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass <sjg at chromium.org> wrote:
> > > > >
> > > > > Hi Bin,
> > > > >
> > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg at chromium.org> wrote:
> > > > > > >
> > > > > > > At present this driver relies on coreboot to provide information about
> > > > > > > the console UART. However if coreboot is not compiled with the UART
> > > > > > > enabled, the information is left out. This configuration is quite
> > > > > > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > > > > > >
> > > > > > > Add a way to determine the UART settings in this case, using a
> > > > > > > hard-coded list of PCI IDs.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > > ---
> > > > > > >
> > > > > > >  drivers/serial/serial_coreboot.c | 68 ++++++++++++++++++++++++++++----
> > > > > > >  include/pci_ids.h                |  1 +
> > > > > > >  2 files changed, 61 insertions(+), 8 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/serial/serial_coreboot.c b/drivers/serial/serial_coreboot.c
> > > > > > > index de09c8681f5..4b4619432d8 100644
> > > > > > > --- a/drivers/serial/serial_coreboot.c
> > > > > > > +++ b/drivers/serial/serial_coreboot.c
> > > > > > > @@ -11,19 +11,71 @@
> > > > > > >  #include <serial.h>
> > > > > > >  #include <asm/cb_sysinfo.h>
> > > > > > >
> > > > > > > +static const struct pci_device_id ids[] = {
> > > > > > > +       { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_APL_UART2) },
> > > > > > > +       {},
> > > > > > > +};
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Coreboot only sets up the UART if it uses it and doesn't bother to put the
> > > > > > > + * details in sysinfo if it doesn't. Try to guess in that case, using devices
> > > > > > > + * we know about
> > > > > > > + *
> > > > > > > + * @plat: Platform data to fill in
> > > > > > > + * @return 0 if found, -ve if no UART was found
> > > > > > > + */
> > > > > > > +static int guess_uart(struct ns16550_plat *plat)
> > > > > >
> > > > > > This is really not a guess, but use a pre-configured platform data.
> > > > > > Also this only work for Apollo Lake board, and will break other boards
> > > > > > if they don't have cbinfo available.
> > > > >
> > > > > Which bit of it breaks other boards?
> > > >
> > > > I see it does not return the error code to the caller, that's okay ...
> > > >
> > > > >
> > > > > >
> > > > > > Why not just simply put a serial node in the device tree and we are all done?
> > > > >
> > > > > See my other email...I am trying to make this boot on any board that
> > > > > coreboot supports.
> > > >
> > > > But this solution does not scale. One has to put all known UARTs into
> > > > serial_coreboot.c.
> > >
> > > Yes that's right...but this is only for when coreboot does not enable
> > > serial. Also the number of new platforms is not that great.
> > >
> > > >
> > > > Why not patch coreboot instead? Why coreboot does not provide a cbinfo
> > > > with serial?
> > >
> > > Because it does not even set up the serial device in that case, so
> > > doesn't know the details. The driver is completely missing.
> >
> > Sigh. Is it possible to upstream a patch to coreboot to enable that?
>
> Well I'm not even sure upstream coreboot boots on the various
> Chromebooks I am targetting. If it does, then serial is probablt
> enabled. But certainly for chromebooks, it is not. My goal is to have
> U-Boot boot on a chromebook in altfw mode with serial enabled.
>
> >
> > I don't like the current approach because it ends up duplicating all
> > UART IDs/info in C.
>
> Yes. Do you think it would be better to put it in the devicetree? I
> suppose we could add some more stuff to the compatible string,
> although U-Boot does not support the PCI compatible strings at
> present.

Putting it in the device tree also looks odd, because it only matters
on a dedicated board.

> What do you suggest?

Or parse the ACPI table coreboot has set up? But that might be another
huge monster :(

Regards,
Bin