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

[Peter Zijlstra <peterz@infradead.org>]

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?

---

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);
 }