Re: [PATCH v2 2/3] term/serial: Add support for PCI serial devices

[Glenn Washburn]

On Mon, 29 Aug 2022 12:53:54 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Aug 26, 2022 at 01:32:25PM -0500, Glenn Washburn wrote:
> > On Fri, 26 Aug 2022 13:01:44 +0200
> > peterz@infradead.org wrote:
> > 
> > > Loosely based on early_pci_serial_init() from Linux, allow GRUB to make
> > > use of PCI serial devices.
> > > 
> > > Specifically, my Alderlake NUC exposes the Intel AMT SoL UART as a PCI
> > > enumerated device but doesn't include it in the EFI tables.
> > > 
> > > Tested and confirmed working on a "Lenovo P360 Tiny" with Intel AMT
> > > enabled. This specific machine has (from lspci -vv):
> > > 
> > > 00:16.3 Serial controller: Intel Corporation Device 7aeb (rev 11) 
> > > (prog-if 02 [16550])
> > >         DeviceName: Onboard - Other
> > >         Subsystem: Lenovo Device 330e
> > >         Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- 
> > > ParErr- Stepping- SERR- FastB2B- DisINTx-
> > >         Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- 
> > > <TAbort- <MAbort- >SERR- <PERR- INTx-
> > >         Interrupt: pin D routed to IRQ 19
> > >         Region 0: I/O ports at 40a0 [size=8]
> > >         Region 1: Memory at b4224000 (32-bit, non-prefetchable) [size=4K]
> > >         Capabilities: [40] MSI: Enable- Count=1/1 Maskable- 64bit+
> > >                 Address: 0000000000000000  Data: 0000
> > >         Capabilities: [50] Power Management version 3
> > >                 Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA 
> > > PME(D0-,D1-,D2-,D3hot-,D3cold-)
> > >                 Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
> > >         Kernel driver in use: serial
> > > 
> > > >From which the following config (/etc/default/grub) gets a working
> > > serial setup:
> > > 
> > > GRUB_CMDLINE_LINUX="console=tty0 earlyprintk=pciserial,00:16.3,115200 
> > > console=ttyS0,115200"
> > > GRUB_SERIAL_COMMAND="serial --port=0x40a0 --speed=115200"
> > 
> > Forgive my ignorance, I'm a bit confused why the above line does not
> > work without the patch, or does it? Is it because the line
> > "grub_pci_write (cmd_addr, cmdreg | GRUB_PCI_COMMAND_IO_ENABLED);" was
> > needed to enable that IO port?
> 
> Correct. Without that the IO port doesn't function and life is sad :-)
> 
> > Either way, I think it would be better to example would be something
> > like:
> > 
> >   GRUB_SERIAL_COMMAND="serial --speed=115200 pci0"
> 
> This made me think the below delta might be a better option.
> Then we could write:
> 
>    GRUB_SERIAL_COMMAND="serial --speed=115200 pci,00:16.3"
> 
> Hmm?

Hmm, I like this idea because its more descriptive for the user. I
don't particularly like the comma separator. Perhaps dash, underscore,
or forward slash are better or maybe no separator at all. I also don't
feel that strongly about it.

Glenn

> 
> ---
> 
> Index: grub/grub-core/term/pci/serial.c
> ===================================================================
> --- grub.orig/grub-core/term/pci/serial.c
> +++ grub/grub-core/term/pci/serial.c
> @@ -30,10 +30,10 @@ find_pciserial (grub_pci_device_t dev, g
>    struct grub_serial_port *port;
>    grub_uint32_t class, bar;
>    grub_uint16_t cmdreg;
> -  int *port_num = data;
>    grub_err_t err;
>  
>    (void)pciid;
> +  (void)data;
>  
>    cmd_addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
>    cmdreg = grub_pci_read (cmd_addr);
> @@ -57,7 +57,10 @@ find_pciserial (grub_pci_device_t dev, g
>    if (port == NULL)
>      return 0;
>  
> -  port->name = grub_xasprintf ("pci%d", (*port_num));
> +  port->name = grub_xasprintf ("pci,%02x:%02x.%x",
> +                            grub_pci_get_bus (dev),
> +                            grub_pci_get_device (dev),
> +                            grub_pci_get_function (dev));
>    if (port->name == NULL)
>      goto error;
>  
> @@ -76,8 +79,6 @@ find_pciserial (grub_pci_device_t dev, g
>    if (err != GRUB_ERR_NONE)
>      goto error;
>  
> -  (*port_num)++;
> -
>    return 0;
>  
>  error:
> @@ -89,7 +90,5 @@ error:
>  void
>  grub_pciserial_init (void)
>  {
> -  int port_num;
> -
> -  grub_pci_iterate (find_pciserial, &port_num);
> +  grub_pci_iterate (find_pciserial, NULL);
>  }