[PATCH v2 0/3] term/serial: Add PCI-serial device support

[PATCH v2 0/3] term/serial: Add PCI-serial device support

[peterz]

Hi!

A small collection of patches that adds PCI serial support.

The first patch is by Glenn and adds GRUB_HAS_PCI -- it is included so that it
is a complete posting.

The second patch is the actual PCI serial support, and the last patch a cleanup
requested by Glenn.

Much thanks to Glenn for all the feedback so far.

[PATCH v2 1/3] configure: Add -DGRUB_HAS_PCI when compiling C/C++ files on targets that support PCI

[peterz]

From: Glenn Washburn <development@efficientek.com>

The list of targets that support PCI is in gentpl.py. However, there is no
support for generating makefile script from a .def file that will apply
globally to the makefile, but on a per target basis. So instead, use
gentpl.py in configure to get the list of targets and check if the current
build target is one of them. If it is, set the automake conditional
COND_HAVE_PCI. Then in conf/Makefile.common add -DGRUB_HAS_PCI for the
platform if COND_HAVE_PCI is true.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 conf/Makefile.common | 3 +++
 configure.ac         | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/conf/Makefile.common b/conf/Makefile.common
index 2d8f1bf2e8..f8faa92e92 100644
--- a/conf/Makefile.common
+++ b/conf/Makefile.common
@@ -20,6 +20,9 @@ endif
 if COND_powerpc_ieee1275
   CFLAGS_PLATFORM += -mcpu=powerpc
 endif
+if COND_HAVE_PCI
+  CFLAGS_PLATFORM += -DGRUB_HAS_PCI
+endif
 
 # Other options
 
diff --git a/configure.ac b/configure.ac
index 90f686f799..1348b06a98 100644
--- a/configure.ac
+++ b/configure.ac
@@ -375,6 +375,11 @@ AM_GNU_GETTEXT([external])
 AM_GNU_GETTEXT_VERSION([0.18.3])
 AC_SYS_LARGEFILE
 
+PLATFORMS_PCI=" $(PYTHONPATH="${srcdir}" $PYTHON -c 'import gentpl; print(" ".join(gentpl.GROUPS[["pci"]]))') "
+if test x"${PLATFORMS_PCI##* ${target_cpu}_${platform} *}" = x ; then
+  have_pci=y
+fi
+
 # Identify characteristics of the host architecture.
 unset ac_cv_c_bigendian
 
@@ -2042,6 +2047,7 @@ AM_CONDITIONAL([COND_HAVE_CXX], [test x$HAVE_CXX = xyes])
 AM_CONDITIONAL([COND_HAVE_ASM_USCORE], [test x$HAVE_ASM_USCORE = x1])
 AM_CONDITIONAL([COND_STARFIELD], [test "x$starfield_excuse" = x])
 AM_CONDITIONAL([COND_HAVE_EXEC], [test "x$have_exec" = xy])
+AM_CONDITIONAL([COND_HAVE_PCI], [test "x$have_pci" = xy])
 
 test "x$prefix" = xNONE && prefix="$ac_default_prefix"
 test "x$exec_prefix" = xNONE && exec_prefix="${prefix}"
-- 
2.34.1

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

[peterz]

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"
GRUB_TERMINAL="serial console"

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---

Index: grub/grub-core/Makefile.core.def
===================================================================
--- grub.orig/grub-core/Makefile.core.def
+++ grub/grub-core/Makefile.core.def
@@ -2047,6 +2047,7 @@ module = {
   ieee1275 = term/ieee1275/serial.c;
   mips_arc = term/arc/serial.c;
   efi = term/efi/serial.c;
+  pci = term/pci/serial.c;
 
   enable = terminfomodule;
   enable = ieee1275;
Index: grub/grub-core/term/pci/serial.c
===================================================================
--- /dev/null
+++ grub/grub-core/term/pci/serial.c
@@ -0,0 +1,95 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2022  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/serial.h>
+#include <grub/term.h>
+#include <grub/types.h>
+#include <grub/pci.h>
+#include <grub/mm.h>
+#include <grub/misc.h>
+
+static int
+find_pciserial (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
+{
+  grub_pci_address_t cmd_addr, class_addr, bar_addr;
+  struct grub_serial_port *port;
+  grub_uint32_t class, bar;
+  grub_uint16_t cmdreg;
+  int *port_num = data;
+  grub_err_t err;
+
+  (void)pciid;
+
+  cmd_addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
+  cmdreg = grub_pci_read (cmd_addr);
+
+  class_addr = grub_pci_make_address (dev, GRUB_PCI_REG_REVISION);
+  class = grub_pci_read (class_addr);
+
+  bar_addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
+  bar = grub_pci_read (bar_addr);
+
+  /* 16550 compattible MODEM or SERIAL */
+  if (((class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_MODEM &&
+       (class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_SERIAL) ||
+      ((class >> 8) & 0xff) != GRUB_PCI_SERIAL_16550_COMPATIBLE)
+    return 0;
+
+  if ((bar & GRUB_PCI_ADDR_SPACE_MASK) != GRUB_PCI_ADDR_SPACE_IO)
+    return 0;
+
+  port = grub_zalloc (sizeof (*port));
+  if (port == NULL)
+    return 0;
+
+  port->name = grub_xasprintf ("pci%d", (*port_num));
+  if (port->name == NULL)
+    goto error;
+
+  grub_pci_write (cmd_addr, cmdreg | GRUB_PCI_COMMAND_IO_ENABLED);
+
+  port->driver = &grub_ns8250_driver;
+  port->port = bar & GRUB_PCI_ADDR_IO_MASK;
+  err = grub_serial_config_defaults (port);
+  if (err != GRUB_ERR_NONE)
+    {
+      grub_print_error ();
+      goto error;
+    }
+
+  err = grub_serial_register (port);
+  if (err != GRUB_ERR_NONE)
+    goto error;
+
+  (*port_num)++;
+
+  return 0;
+
+error:
+  grub_free (port->name);
+  grub_free (port);
+  return 0;
+}
+
+void
+grub_pciserial_init (void)
+{
+  int port_num;
+
+  grub_pci_iterate (find_pciserial, &port_num);
+}
Index: grub/grub-core/term/serial.c
===================================================================
--- grub.orig/grub-core/term/serial.c
+++ grub/grub-core/term/serial.c
@@ -448,6 +448,9 @@ GRUB_MOD_INIT(serial)
 #ifdef GRUB_MACHINE_ARC
   grub_arcserial_init ();
 #endif
+#ifdef GRUB_HAS_PCI
+  grub_pciserial_init ();
+#endif
 }
 
 GRUB_MOD_FINI(serial)
Index: grub/include/grub/pci.h
===================================================================
--- grub.orig/include/grub/pci.h
+++ grub/include/grub/pci.h
@@ -80,8 +80,13 @@
 
 #define  GRUB_PCI_STATUS_DEVSEL_TIMING_SHIFT 9
 #define  GRUB_PCI_STATUS_DEVSEL_TIMING_MASK 0x0600
+
 #define  GRUB_PCI_CLASS_SUBCLASS_VGA  0x0300
 #define  GRUB_PCI_CLASS_SUBCLASS_USB  0x0c03
+#define  GRUB_PCI_CLASS_COMMUNICATION_SERIAL  0x0700
+#define  GRUB_PCI_CLASS_COMMUNICATION_MODEM   0x0703
+
+#define  GRUB_PCI_SERIAL_16550_COMPATIBLE 0x02
 
 #ifndef ASM_FILE
 
Index: grub/include/grub/serial.h
===================================================================
--- grub.orig/include/grub/serial.h
+++ grub/include/grub/serial.h
@@ -193,6 +193,10 @@ const char *
 grub_arcserial_add_port (const char *path);
 #endif
 
+#ifdef GRUB_HAS_PCI
+void grub_pciserial_init (void);
+#endif
+
 struct grub_serial_port *grub_serial_find (const char *name);
 extern struct grub_serial_driver grub_ns8250_driver;
 void EXPORT_FUNC(grub_serial_unregister_driver) (struct grub_serial_driver *driver);

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

[Glenn Washburn]

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?

Either way, I think it would be better to example would be something
like:

  GRUB_SERIAL_COMMAND="serial --speed=115200 pci0"

Otherwise, LGTM.

Reviewed-by: Glenn Washburn <development@efficientek.com>

Glenn

> GRUB_TERMINAL="serial console"
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> 
> Index: grub/grub-core/Makefile.core.def
> ===================================================================
> --- grub.orig/grub-core/Makefile.core.def
> +++ grub/grub-core/Makefile.core.def
> @@ -2047,6 +2047,7 @@ module = {
>    ieee1275 = term/ieee1275/serial.c;
>    mips_arc = term/arc/serial.c;
>    efi = term/efi/serial.c;
> +  pci = term/pci/serial.c;
>  
>    enable = terminfomodule;
>    enable = ieee1275;
> Index: grub/grub-core/term/pci/serial.c
> ===================================================================
> --- /dev/null
> +++ grub/grub-core/term/pci/serial.c
> @@ -0,0 +1,95 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2022  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/serial.h>
> +#include <grub/term.h>
> +#include <grub/types.h>
> +#include <grub/pci.h>
> +#include <grub/mm.h>
> +#include <grub/misc.h>
> +
> +static int
> +find_pciserial (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
> +{
> +  grub_pci_address_t cmd_addr, class_addr, bar_addr;
> +  struct grub_serial_port *port;
> +  grub_uint32_t class, bar;
> +  grub_uint16_t cmdreg;
> +  int *port_num = data;
> +  grub_err_t err;
> +
> +  (void)pciid;
> +
> +  cmd_addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
> +  cmdreg = grub_pci_read (cmd_addr);
> +
> +  class_addr = grub_pci_make_address (dev, GRUB_PCI_REG_REVISION);
> +  class = grub_pci_read (class_addr);
> +
> +  bar_addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
> +  bar = grub_pci_read (bar_addr);
> +
> +  /* 16550 compattible MODEM or SERIAL */
> +  if (((class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_MODEM &&
> +       (class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_SERIAL) ||
> +      ((class >> 8) & 0xff) != GRUB_PCI_SERIAL_16550_COMPATIBLE)
> +    return 0;
> +
> +  if ((bar & GRUB_PCI_ADDR_SPACE_MASK) != GRUB_PCI_ADDR_SPACE_IO)
> +    return 0;
> +
> +  port = grub_zalloc (sizeof (*port));
> +  if (port == NULL)
> +    return 0;
> +
> +  port->name = grub_xasprintf ("pci%d", (*port_num));
> +  if (port->name == NULL)
> +    goto error;
> +
> +  grub_pci_write (cmd_addr, cmdreg | GRUB_PCI_COMMAND_IO_ENABLED);
> +
> +  port->driver = &grub_ns8250_driver;
> +  port->port = bar & GRUB_PCI_ADDR_IO_MASK;
> +  err = grub_serial_config_defaults (port);
> +  if (err != GRUB_ERR_NONE)
> +    {
> +      grub_print_error ();
> +      goto error;
> +    }
> +
> +  err = grub_serial_register (port);
> +  if (err != GRUB_ERR_NONE)
> +    goto error;
> +
> +  (*port_num)++;
> +
> +  return 0;
> +
> +error:
> +  grub_free (port->name);
> +  grub_free (port);
> +  return 0;
> +}
> +
> +void
> +grub_pciserial_init (void)
> +{
> +  int port_num;
> +
> +  grub_pci_iterate (find_pciserial, &port_num);
> +}
> Index: grub/grub-core/term/serial.c
> ===================================================================
> --- grub.orig/grub-core/term/serial.c
> +++ grub/grub-core/term/serial.c
> @@ -448,6 +448,9 @@ GRUB_MOD_INIT(serial)
>  #ifdef GRUB_MACHINE_ARC
>    grub_arcserial_init ();
>  #endif
> +#ifdef GRUB_HAS_PCI
> +  grub_pciserial_init ();
> +#endif
>  }
>  
>  GRUB_MOD_FINI(serial)
> Index: grub/include/grub/pci.h
> ===================================================================
> --- grub.orig/include/grub/pci.h
> +++ grub/include/grub/pci.h
> @@ -80,8 +80,13 @@
>  
>  #define  GRUB_PCI_STATUS_DEVSEL_TIMING_SHIFT 9
>  #define  GRUB_PCI_STATUS_DEVSEL_TIMING_MASK 0x0600
> +
>  #define  GRUB_PCI_CLASS_SUBCLASS_VGA  0x0300
>  #define  GRUB_PCI_CLASS_SUBCLASS_USB  0x0c03
> +#define  GRUB_PCI_CLASS_COMMUNICATION_SERIAL  0x0700
> +#define  GRUB_PCI_CLASS_COMMUNICATION_MODEM   0x0703
> +
> +#define  GRUB_PCI_SERIAL_16550_COMPATIBLE 0x02
>  
>  #ifndef ASM_FILE
>  
> Index: grub/include/grub/serial.h
> ===================================================================
> --- grub.orig/include/grub/serial.h
> +++ grub/include/grub/serial.h
> @@ -193,6 +193,10 @@ const char *
>  grub_arcserial_add_port (const char *path);
>  #endif
>  
> +#ifdef GRUB_HAS_PCI
> +void grub_pciserial_init (void);
> +#endif
> +
>  struct grub_serial_port *grub_serial_find (const char *name);
>  extern struct grub_serial_driver grub_ns8250_driver;
>  void EXPORT_FUNC(grub_serial_unregister_driver) (struct grub_serial_driver *driver);
> 
> 

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

[Peter Zijlstra]

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

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

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

[Daniel Kiper]

Sorry for late reply...

May I ask you to send the patches using "git send-email"?

On Fri, Aug 26, 2022 at 01:01:44PM +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"

Please add a blurb how to use this driver to the docs/grub.texi file.

> GRUB_TERMINAL="serial console"
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>
> Index: grub/grub-core/Makefile.core.def
> ===================================================================
> --- grub.orig/grub-core/Makefile.core.def
> +++ grub/grub-core/Makefile.core.def
> @@ -2047,6 +2047,7 @@ module = {
>    ieee1275 = term/ieee1275/serial.c;
>    mips_arc = term/arc/serial.c;
>    efi = term/efi/serial.c;
> +  pci = term/pci/serial.c;

I think this should be "x86 = term/pci/serial.c".

>
>    enable = terminfomodule;
>    enable = ieee1275;
> Index: grub/grub-core/term/pci/serial.c
> ===================================================================
> --- /dev/null
> +++ grub/grub-core/term/pci/serial.c
> @@ -0,0 +1,95 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2022  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/serial.h>
> +#include <grub/term.h>
> +#include <grub/types.h>
> +#include <grub/pci.h>
> +#include <grub/mm.h>
> +#include <grub/misc.h>
> +
> +static int
> +find_pciserial (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)

grub_pci_id_t pciid __attribute__ ((unused))

> +{
> +  grub_pci_address_t cmd_addr, class_addr, bar_addr;
> +  struct grub_serial_port *port;
> +  grub_uint32_t class, bar;
> +  grub_uint16_t cmdreg;
> +  int *port_num = data;
> +  grub_err_t err;
> +
> +  (void)pciid;

... and please drop this...

> +  cmd_addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
> +  cmdreg = grub_pci_read (cmd_addr);
> +
> +  class_addr = grub_pci_make_address (dev, GRUB_PCI_REG_REVISION);
> +  class = grub_pci_read (class_addr);
> +
> +  bar_addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
> +  bar = grub_pci_read (bar_addr);
> +
> +  /* 16550 compattible MODEM or SERIAL */
> +  if (((class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_MODEM &&
> +       (class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_SERIAL) ||
> +      ((class >> 8) & 0xff) != GRUB_PCI_SERIAL_16550_COMPATIBLE)
> +    return 0;
> +
> +  if ((bar & GRUB_PCI_ADDR_SPACE_MASK) != GRUB_PCI_ADDR_SPACE_IO)
> +    return 0;
> +
> +  port = grub_zalloc (sizeof (*port));
> +  if (port == NULL)
> +    return 0;
> +
> +  port->name = grub_xasprintf ("pci%d", (*port_num));
> +  if (port->name == NULL)
> +    goto error;
> +
> +  grub_pci_write (cmd_addr, cmdreg | GRUB_PCI_COMMAND_IO_ENABLED);
> +
> +  port->driver = &grub_ns8250_driver;
> +  port->port = bar & GRUB_PCI_ADDR_IO_MASK;
> +  err = grub_serial_config_defaults (port);
> +  if (err != GRUB_ERR_NONE)
> +    {
> +      grub_print_error ();
> +      goto error;
> +    }
> +
> +  err = grub_serial_register (port);
> +  if (err != GRUB_ERR_NONE)
> +    goto error;
> +
> +  (*port_num)++;
> +
> +  return 0;
> +
> +error:

s/error/fail/

And please add a space before label...

> +  grub_free (port->name);
> +  grub_free (port);
> +  return 0;
> +}

Daniel

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

[Peter Zijlstra]

On Wed, Dec 21, 2022 at 01:59:02PM +0100, Daniel Kiper wrote:
> Sorry for late reply...
> 
> May I ask you to send the patches using "git send-email"?

I'll try -- I'm one of the few quilt holdouts in Linux-land so I don't
have much experience with it.


> Please add a blurb how to use this driver to the docs/grub.texi file.
> 
> > GRUB_TERMINAL="serial console"
> >
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >
> > Index: grub/grub-core/Makefile.core.def
> > ===================================================================
> > --- grub.orig/grub-core/Makefile.core.def
> > +++ grub/grub-core/Makefile.core.def
> > @@ -2047,6 +2047,7 @@ module = {
> >    ieee1275 = term/ieee1275/serial.c;
> >    mips_arc = term/arc/serial.c;
> >    efi = term/efi/serial.c;
> > +  pci = term/pci/serial.c;
> 
> I think this should be "x86 = term/pci/serial.c".

I thought the whole purpose of Glenn's patch was to allow doing the 'pci
=' thing, so that all platforms that support PCI get to have the
benefit.

But if you really want I can make it 'x86 =' ofcourse.


> > +static int
> > +find_pciserial (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
> 
> grub_pci_id_t pciid __attribute__ ((unused))
> 
> > +{
> > +  grub_pci_address_t cmd_addr, class_addr, bar_addr;
> > +  struct grub_serial_port *port;
> > +  grub_uint32_t class, bar;
> > +  grub_uint16_t cmdreg;
> > +  int *port_num = data;
> > +  grub_err_t err;
> > +
> > +  (void)pciid;
> 
> ... and please drop this...
> 

> > +error:
> 
> s/error/fail/
> 
> And please add a space before label...

All done, except the Makefile thing, find the updated version below.
I'll attempt to git-send-email both patches once we agree on the
Makefile hunk.


---
Subject: term/serial: Add support for PCI serial devices

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 --speed=115200 pci_00:16.3"
GRUB_TERMINAL="serial console"

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Glenn Washburn <development@efficientek.com>
---

Index: grub/grub-core/Makefile.core.def
===================================================================
--- grub.orig/grub-core/Makefile.core.def
+++ grub/grub-core/Makefile.core.def
@@ -2057,6 +2057,7 @@ module = {
   ieee1275 = term/ieee1275/serial.c;
   mips_arc = term/arc/serial.c;
   efi = term/efi/serial.c;
+  pci = term/pci/serial.c;
 
   enable = terminfomodule;
   enable = ieee1275;
Index: grub/grub-core/term/pci/serial.c
===================================================================
--- /dev/null
+++ grub/grub-core/term/pci/serial.c
@@ -0,0 +1,93 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2022  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/serial.h>
+#include <grub/term.h>
+#include <grub/types.h>
+#include <grub/pci.h>
+#include <grub/mm.h>
+#include <grub/misc.h>
+
+static int
+find_pciserial (grub_pci_device_t dev,
+		grub_pci_id_t pciid __attribute__ ((unused)),
+		void *data __attribute__ ((unused)))
+{
+  grub_pci_address_t cmd_addr, class_addr, bar_addr;
+  struct grub_serial_port *port;
+  grub_uint32_t class, bar;
+  grub_uint16_t cmdreg;
+  grub_err_t err;
+
+  cmd_addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
+  cmdreg = grub_pci_read (cmd_addr);
+
+  class_addr = grub_pci_make_address (dev, GRUB_PCI_REG_REVISION);
+  class = grub_pci_read (class_addr);
+
+  bar_addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
+  bar = grub_pci_read (bar_addr);
+
+  /* 16550 compatible MODEM or SERIAL */
+  if (((class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_MODEM &&
+       (class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_SERIAL) ||
+      ((class >> 8) & 0xff) != GRUB_PCI_SERIAL_16550_COMPATIBLE)
+    return 0;
+
+  if ((bar & GRUB_PCI_ADDR_SPACE_MASK) != GRUB_PCI_ADDR_SPACE_IO)
+    return 0;
+
+  port = grub_zalloc (sizeof (*port));
+  if (port == NULL)
+    return 0;
+
+  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 fail;
+
+  grub_pci_write (cmd_addr, cmdreg | GRUB_PCI_COMMAND_IO_ENABLED);
+
+  port->driver = &grub_ns8250_driver;
+  port->port = bar & GRUB_PCI_ADDR_IO_MASK;
+  err = grub_serial_config_defaults (port);
+  if (err != GRUB_ERR_NONE)
+    {
+      grub_print_error ();
+      goto fail;
+    }
+
+  err = grub_serial_register (port);
+  if (err != GRUB_ERR_NONE)
+    goto fail;
+
+  return 0;
+
+ fail:
+  grub_free (port->name);
+  grub_free (port);
+  return 0;
+}
+
+void
+grub_pciserial_init (void)
+{
+  grub_pci_iterate (find_pciserial, NULL);
+}
Index: grub/grub-core/term/serial.c
===================================================================
--- grub.orig/grub-core/term/serial.c
+++ grub/grub-core/term/serial.c
@@ -505,6 +505,9 @@ GRUB_MOD_INIT(serial)
 #ifdef GRUB_MACHINE_ARC
   grub_arcserial_init ();
 #endif
+#ifdef GRUB_HAS_PCI
+  grub_pciserial_init ();
+#endif
 }
 
 GRUB_MOD_FINI(serial)
Index: grub/include/grub/pci.h
===================================================================
--- grub.orig/include/grub/pci.h
+++ grub/include/grub/pci.h
@@ -80,8 +80,13 @@
 
 #define  GRUB_PCI_STATUS_DEVSEL_TIMING_SHIFT 9
 #define  GRUB_PCI_STATUS_DEVSEL_TIMING_MASK 0x0600
+
 #define  GRUB_PCI_CLASS_SUBCLASS_VGA  0x0300
 #define  GRUB_PCI_CLASS_SUBCLASS_USB  0x0c03
+#define  GRUB_PCI_CLASS_COMMUNICATION_SERIAL  0x0700
+#define  GRUB_PCI_CLASS_COMMUNICATION_MODEM   0x0703
+
+#define  GRUB_PCI_SERIAL_16550_COMPATIBLE 0x02
 
 #ifndef ASM_FILE
 
Index: grub/include/grub/serial.h
===================================================================
--- grub.orig/include/grub/serial.h
+++ grub/include/grub/serial.h
@@ -210,6 +210,10 @@ grub_arcserial_init (void);
 struct grub_serial_port *grub_arcserial_add_port (const char *path);
 #endif
 
+#ifdef GRUB_HAS_PCI
+void grub_pciserial_init (void);
+#endif
+
 struct grub_serial_port *grub_serial_find (const char *name);
 extern struct grub_serial_driver grub_ns8250_driver;
 void EXPORT_FUNC(grub_serial_unregister_driver) (struct grub_serial_driver *driver);
Index: grub/docs/grub.texi
===================================================================
--- grub.orig/docs/grub.texi
+++ grub/docs/grub.texi
@@ -1386,6 +1386,7 @@ separated by spaces.
 Valid terminal input names depend on the platform, but may include
 @samp{console} (native platform console), @samp{serial} (serial terminal),
 @samp{serial_<port>} (serial terminal with explicit port selection),
+@samp{pci_<bus>:<device>.<function>} (PCI serial with explicit location),
 @samp{at_keyboard} (PC AT keyboard), or @samp{usb_keyboard} (USB keyboard
 using the HID Boot Protocol, for cases where the firmware does not handle
 this).
@@ -1399,6 +1400,7 @@ separated by spaces.
 Valid terminal output names depend on the platform, but may include
 @samp{console} (native platform console), @samp{serial} (serial terminal),
 @samp{serial_<port>} (serial terminal with explicit port selection),
+@samp{pci_<bus>:<device>.<function>} (PCI serial with explicit location),
 @samp{gfxterm} (graphics-mode output), @samp{vga_text} (VGA text output),
 @samp{mda_text} (MDA text output), @samp{morse} (Morse-coding using system
 beeper) or @samp{spkmodem} (simple data protocol using system speaker).
@@ -4165,7 +4167,7 @@ Commands usable anywhere in the menu and
 @node serial
 @subsection serial
 
-@deffn Command serial [@option{--unit=unit}] [@option{--port=port}] [@option{--speed=speed}] [@option{--word=word}] [@option{--parity=parity}] [@option{--stop=stop}]
+@deffn Command serial [@option{--unit=unit}] [@option{--port=port}] [@option{--speed=speed}] [@option{--word=word}] [@option{--parity=parity}] [@option{--stop=stop}] [@option{name}]
 Initialize a serial device. @var{unit} is a number in the range 0-3
 specifying which serial port to use; default is 0, which corresponds to
 the port often called COM1.
@@ -4197,6 +4199,19 @@ If passed no @var{unit} nor @var{port},
 the system default serial port and its configuration. If this information
 is not available, it will default to @var{unit} 0.
 
+If used @var{name}, a machine and driver specific name given during probing,
+will be used to indicate which serial port to use. Possible forms include:
+@itemize @bullet
+@item
+@samp{serial_<port>} as an alternative to --port
+@item
+@samp{usbN} the N'th USB serial detected
+@item
+@samp{efiN} the N'th EFI serial detected
+@item
+@samp{pci_<bus>:<device>.<func>} PCI serial at the specified location
+@end itemize
+
 The serial port is not used as a communication channel unless the
 @command{terminal_input} or @command{terminal_output} command is used
 (@pxref{terminal_input}, @pxref{terminal_output}).
@@ -4205,6 +4220,7 @@ Examples:
 @example
 serial --port=3f8 --speed=9600
 serial --port=mmio,fefb0000.l --speed=115200
+serial --speed=115200 pci_00:16.3
 @end example
 
 See also @ref{Serial terminal}.

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

[Glenn Washburn]

On Thu, 26 Jan 2023 10:37:35 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Dec 21, 2022 at 01:59:02PM +0100, Daniel Kiper wrote:
> > Sorry for late reply...
> > 
> > May I ask you to send the patches using "git send-email"?
> 
> I'll try -- I'm one of the few quilt holdouts in Linux-land so I don't
> have much experience with it.
> 
> 
> > Please add a blurb how to use this driver to the docs/grub.texi
> > file.
> > 
> > > GRUB_TERMINAL="serial console"
> > >
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > ---
> > >
> > > Index: grub/grub-core/Makefile.core.def
> > > ===================================================================
> > > --- grub.orig/grub-core/Makefile.core.def
> > > +++ grub/grub-core/Makefile.core.def
> > > @@ -2047,6 +2047,7 @@ module = {
> > >    ieee1275 = term/ieee1275/serial.c;
> > >    mips_arc = term/arc/serial.c;
> > >    efi = term/efi/serial.c;
> > > +  pci = term/pci/serial.c;
> > 
> > I think this should be "x86 = term/pci/serial.c".
> 
> I thought the whole purpose of Glenn's patch was to allow doing the
> 'pci =' thing, so that all platforms that support PCI get to have the
> benefit.
> 
> But if you really want I can make it 'x86 =' ofcourse.

To throw in my two cents, I believe this is correct the way it is with
'pci =' as Peter mentioned. Am I also missing something here?

Glenn

> > > +static int
> > > +find_pciserial (grub_pci_device_t dev, grub_pci_id_t pciid, void
> > > *data)
> > 
> > grub_pci_id_t pciid __attribute__ ((unused))
> > 
> > > +{
> > > +  grub_pci_address_t cmd_addr, class_addr, bar_addr;
> > > +  struct grub_serial_port *port;
> > > +  grub_uint32_t class, bar;
> > > +  grub_uint16_t cmdreg;
> > > +  int *port_num = data;
> > > +  grub_err_t err;
> > > +
> > > +  (void)pciid;
> > 
> > ... and please drop this...
> > 
> 
> > > +error:
> > 
> > s/error/fail/
> > 
> > And please add a space before label...
> 
> All done, except the Makefile thing, find the updated version below.
> I'll attempt to git-send-email both patches once we agree on the
> Makefile hunk.
> 
> 
> ---
> Subject: term/serial: Add support for PCI serial devices
> 
> 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 --speed=115200
> pci_00:16.3" GRUB_TERMINAL="serial console"
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Tested-by: Glenn Washburn <development@efficientek.com>
> Reviewed-by: Glenn Washburn <development@efficientek.com>
> ---
> 
> Index: grub/grub-core/Makefile.core.def
> ===================================================================
> --- grub.orig/grub-core/Makefile.core.def
> +++ grub/grub-core/Makefile.core.def
> @@ -2057,6 +2057,7 @@ module = {
>    ieee1275 = term/ieee1275/serial.c;
>    mips_arc = term/arc/serial.c;
>    efi = term/efi/serial.c;
> +  pci = term/pci/serial.c;
>  
>    enable = terminfomodule;
>    enable = ieee1275;
> Index: grub/grub-core/term/pci/serial.c
> ===================================================================
> --- /dev/null
> +++ grub/grub-core/term/pci/serial.c
> @@ -0,0 +1,93 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2022  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as
> published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/serial.h>
> +#include <grub/term.h>
> +#include <grub/types.h>
> +#include <grub/pci.h>
> +#include <grub/mm.h>
> +#include <grub/misc.h>
> +
> +static int
> +find_pciserial (grub_pci_device_t dev,
> +		grub_pci_id_t pciid __attribute__ ((unused)),
> +		void *data __attribute__ ((unused)))
> +{
> +  grub_pci_address_t cmd_addr, class_addr, bar_addr;
> +  struct grub_serial_port *port;
> +  grub_uint32_t class, bar;
> +  grub_uint16_t cmdreg;
> +  grub_err_t err;
> +
> +  cmd_addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
> +  cmdreg = grub_pci_read (cmd_addr);
> +
> +  class_addr = grub_pci_make_address (dev, GRUB_PCI_REG_REVISION);
> +  class = grub_pci_read (class_addr);
> +
> +  bar_addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
> +  bar = grub_pci_read (bar_addr);
> +
> +  /* 16550 compatible MODEM or SERIAL */
> +  if (((class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_MODEM &&
> +       (class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_SERIAL) ||
> +      ((class >> 8) & 0xff) != GRUB_PCI_SERIAL_16550_COMPATIBLE)
> +    return 0;
> +
> +  if ((bar & GRUB_PCI_ADDR_SPACE_MASK) != GRUB_PCI_ADDR_SPACE_IO)
> +    return 0;
> +
> +  port = grub_zalloc (sizeof (*port));
> +  if (port == NULL)
> +    return 0;
> +
> +  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 fail;
> +
> +  grub_pci_write (cmd_addr, cmdreg | GRUB_PCI_COMMAND_IO_ENABLED);
> +
> +  port->driver = &grub_ns8250_driver;
> +  port->port = bar & GRUB_PCI_ADDR_IO_MASK;
> +  err = grub_serial_config_defaults (port);
> +  if (err != GRUB_ERR_NONE)
> +    {
> +      grub_print_error ();
> +      goto fail;
> +    }
> +
> +  err = grub_serial_register (port);
> +  if (err != GRUB_ERR_NONE)
> +    goto fail;
> +
> +  return 0;
> +
> + fail:
> +  grub_free (port->name);
> +  grub_free (port);
> +  return 0;
> +}
> +
> +void
> +grub_pciserial_init (void)
> +{
> +  grub_pci_iterate (find_pciserial, NULL);
> +}
> Index: grub/grub-core/term/serial.c
> ===================================================================
> --- grub.orig/grub-core/term/serial.c
> +++ grub/grub-core/term/serial.c
> @@ -505,6 +505,9 @@ GRUB_MOD_INIT(serial)
>  #ifdef GRUB_MACHINE_ARC
>    grub_arcserial_init ();
>  #endif
> +#ifdef GRUB_HAS_PCI
> +  grub_pciserial_init ();
> +#endif
>  }
>  
>  GRUB_MOD_FINI(serial)
> Index: grub/include/grub/pci.h
> ===================================================================
> --- grub.orig/include/grub/pci.h
> +++ grub/include/grub/pci.h
> @@ -80,8 +80,13 @@
>  
>  #define  GRUB_PCI_STATUS_DEVSEL_TIMING_SHIFT 9
>  #define  GRUB_PCI_STATUS_DEVSEL_TIMING_MASK 0x0600
> +
>  #define  GRUB_PCI_CLASS_SUBCLASS_VGA  0x0300
>  #define  GRUB_PCI_CLASS_SUBCLASS_USB  0x0c03
> +#define  GRUB_PCI_CLASS_COMMUNICATION_SERIAL  0x0700
> +#define  GRUB_PCI_CLASS_COMMUNICATION_MODEM   0x0703
> +
> +#define  GRUB_PCI_SERIAL_16550_COMPATIBLE 0x02
>  
>  #ifndef ASM_FILE
>  
> Index: grub/include/grub/serial.h
> ===================================================================
> --- grub.orig/include/grub/serial.h
> +++ grub/include/grub/serial.h
> @@ -210,6 +210,10 @@ grub_arcserial_init (void);
>  struct grub_serial_port *grub_arcserial_add_port (const char *path);
>  #endif
>  
> +#ifdef GRUB_HAS_PCI
> +void grub_pciserial_init (void);
> +#endif
> +
>  struct grub_serial_port *grub_serial_find (const char *name);
>  extern struct grub_serial_driver grub_ns8250_driver;
>  void EXPORT_FUNC(grub_serial_unregister_driver) (struct
> grub_serial_driver *driver); Index: grub/docs/grub.texi
> ===================================================================
> --- grub.orig/docs/grub.texi
> +++ grub/docs/grub.texi
> @@ -1386,6 +1386,7 @@ separated by spaces.
>  Valid terminal input names depend on the platform, but may include
>  @samp{console} (native platform console), @samp{serial} (serial
> terminal), @samp{serial_<port>} (serial terminal with explicit port
> selection), +@samp{pci_<bus>:<device>.<function>} (PCI serial with
> explicit location), @samp{at_keyboard} (PC AT keyboard), or
> @samp{usb_keyboard} (USB keyboard using the HID Boot Protocol, for
> cases where the firmware does not handle this).
> @@ -1399,6 +1400,7 @@ separated by spaces.
>  Valid terminal output names depend on the platform, but may include
>  @samp{console} (native platform console), @samp{serial} (serial
> terminal), @samp{serial_<port>} (serial terminal with explicit port
> selection), +@samp{pci_<bus>:<device>.<function>} (PCI serial with
> explicit location), @samp{gfxterm} (graphics-mode output),
> @samp{vga_text} (VGA text output), @samp{mda_text} (MDA text output),
> @samp{morse} (Morse-coding using system beeper) or @samp{spkmodem}
> (simple data protocol using system speaker). @@ -4165,7 +4167,7 @@
> Commands usable anywhere in the menu and @node serial
>  @subsection serial
>  
> -@deffn Command serial [@option{--unit=unit}] [@option{--port=port}]
> [@option{--speed=speed}] [@option{--word=word}]
> [@option{--parity=parity}] [@option{--stop=stop}] +@deffn Command
> serial [@option{--unit=unit}] [@option{--port=port}]
> [@option{--speed=speed}] [@option{--word=word}]
> [@option{--parity=parity}] [@option{--stop=stop}] [@option{name}]
> Initialize a serial device. @var{unit} is a number in the range 0-3
> specifying which serial port to use; default is 0, which corresponds
> to the port often called COM1. @@ -4197,6 +4199,19 @@ If passed no
> @var{unit} nor @var{port}, the system default serial port and its
> configuration. If this information is not available, it will default
> to @var{unit} 0. +If used @var{name}, a machine and driver specific
> name given during probing, +will be used to indicate which serial
> port to use. Possible forms include: +@itemize @bullet +@item
> +@samp{serial_<port>} as an alternative to --port +@item
> +@samp{usbN} the N'th USB serial detected
> +@item
> +@samp{efiN} the N'th EFI serial detected
> +@item
> +@samp{pci_<bus>:<device>.<func>} PCI serial at the specified location
> +@end itemize
> +
>  The serial port is not used as a communication channel unless the
>  @command{terminal_input} or @command{terminal_output} command is used
>  (@pxref{terminal_input}, @pxref{terminal_output}).
> @@ -4205,6 +4220,7 @@ Examples:
>  @example
>  serial --port=3f8 --speed=9600
>  serial --port=mmio,fefb0000.l --speed=115200
> +serial --speed=115200 pci_00:16.3
>  @end example
>  
>  See also @ref{Serial terminal}.

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

[Peter Zijlstra]

On Thu, Jan 26, 2023 at 10:37:35AM +0100, Peter Zijlstra wrote:
> On Wed, Dec 21, 2022 at 01:59:02PM +0100, Daniel Kiper wrote:
> > Sorry for late reply...
> > 
> > May I ask you to send the patches using "git send-email"?
> 
> I'll try -- I'm one of the few quilt holdouts in Linux-land so I don't
> have much experience with it.
> 
> 
> > Please add a blurb how to use this driver to the docs/grub.texi file.
> > 
> > > GRUB_TERMINAL="serial console"
> > >
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > ---
> > >
> > > Index: grub/grub-core/Makefile.core.def
> > > ===================================================================
> > > --- grub.orig/grub-core/Makefile.core.def
> > > +++ grub/grub-core/Makefile.core.def
> > > @@ -2047,6 +2047,7 @@ module = {
> > >    ieee1275 = term/ieee1275/serial.c;
> > >    mips_arc = term/arc/serial.c;
> > >    efi = term/efi/serial.c;
> > > +  pci = term/pci/serial.c;
> > 
> > I think this should be "x86 = term/pci/serial.c".
> 
> I thought the whole purpose of Glenn's patch was to allow doing the 'pci
> =' thing, so that all platforms that support PCI get to have the
> benefit.
> 
> But if you really want I can make it 'x86 =' ofcourse.

Ben, you've lived a very long time in !x86 land, can you elucidate us on
this? My thinking is that if someone sticks a 16550 on a PCI board, this
driver should work on any PCI enabled platform, not just x86, rite?

> 
> 
> > > +static int
> > > +find_pciserial (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
> > 
> > grub_pci_id_t pciid __attribute__ ((unused))
> > 
> > > +{
> > > +  grub_pci_address_t cmd_addr, class_addr, bar_addr;
> > > +  struct grub_serial_port *port;
> > > +  grub_uint32_t class, bar;
> > > +  grub_uint16_t cmdreg;
> > > +  int *port_num = data;
> > > +  grub_err_t err;
> > > +
> > > +  (void)pciid;
> > 
> > ... and please drop this...
> > 
> 
> > > +error:
> > 
> > s/error/fail/
> > 
> > And please add a space before label...
> 
> All done, except the Makefile thing, find the updated version below.
> I'll attempt to git-send-email both patches once we agree on the
> Makefile hunk.
> 
> 
> ---
> Subject: term/serial: Add support for PCI serial devices
> 
> 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 --speed=115200 pci_00:16.3"
> GRUB_TERMINAL="serial console"
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Tested-by: Glenn Washburn <development@efficientek.com>
> Reviewed-by: Glenn Washburn <development@efficientek.com>
> ---
> 
> Index: grub/grub-core/Makefile.core.def
> ===================================================================
> --- grub.orig/grub-core/Makefile.core.def
> +++ grub/grub-core/Makefile.core.def
> @@ -2057,6 +2057,7 @@ module = {
>    ieee1275 = term/ieee1275/serial.c;
>    mips_arc = term/arc/serial.c;
>    efi = term/efi/serial.c;
> +  pci = term/pci/serial.c;
>  
>    enable = terminfomodule;
>    enable = ieee1275;
> Index: grub/grub-core/term/pci/serial.c
> ===================================================================
> --- /dev/null
> +++ grub/grub-core/term/pci/serial.c
> @@ -0,0 +1,93 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2022  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/serial.h>
> +#include <grub/term.h>
> +#include <grub/types.h>
> +#include <grub/pci.h>
> +#include <grub/mm.h>
> +#include <grub/misc.h>
> +
> +static int
> +find_pciserial (grub_pci_device_t dev,
> +		grub_pci_id_t pciid __attribute__ ((unused)),
> +		void *data __attribute__ ((unused)))
> +{
> +  grub_pci_address_t cmd_addr, class_addr, bar_addr;
> +  struct grub_serial_port *port;
> +  grub_uint32_t class, bar;
> +  grub_uint16_t cmdreg;
> +  grub_err_t err;
> +
> +  cmd_addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
> +  cmdreg = grub_pci_read (cmd_addr);
> +
> +  class_addr = grub_pci_make_address (dev, GRUB_PCI_REG_REVISION);
> +  class = grub_pci_read (class_addr);
> +
> +  bar_addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
> +  bar = grub_pci_read (bar_addr);
> +
> +  /* 16550 compatible MODEM or SERIAL */
> +  if (((class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_MODEM &&
> +       (class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_SERIAL) ||
> +      ((class >> 8) & 0xff) != GRUB_PCI_SERIAL_16550_COMPATIBLE)
> +    return 0;
> +
> +  if ((bar & GRUB_PCI_ADDR_SPACE_MASK) != GRUB_PCI_ADDR_SPACE_IO)
> +    return 0;
> +
> +  port = grub_zalloc (sizeof (*port));
> +  if (port == NULL)
> +    return 0;
> +
> +  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 fail;
> +
> +  grub_pci_write (cmd_addr, cmdreg | GRUB_PCI_COMMAND_IO_ENABLED);
> +
> +  port->driver = &grub_ns8250_driver;
> +  port->port = bar & GRUB_PCI_ADDR_IO_MASK;
> +  err = grub_serial_config_defaults (port);
> +  if (err != GRUB_ERR_NONE)
> +    {
> +      grub_print_error ();
> +      goto fail;
> +    }
> +
> +  err = grub_serial_register (port);
> +  if (err != GRUB_ERR_NONE)
> +    goto fail;
> +
> +  return 0;
> +
> + fail:
> +  grub_free (port->name);
> +  grub_free (port);
> +  return 0;
> +}
> +
> +void
> +grub_pciserial_init (void)
> +{
> +  grub_pci_iterate (find_pciserial, NULL);
> +}
> Index: grub/grub-core/term/serial.c
> ===================================================================
> --- grub.orig/grub-core/term/serial.c
> +++ grub/grub-core/term/serial.c
> @@ -505,6 +505,9 @@ GRUB_MOD_INIT(serial)
>  #ifdef GRUB_MACHINE_ARC
>    grub_arcserial_init ();
>  #endif
> +#ifdef GRUB_HAS_PCI
> +  grub_pciserial_init ();
> +#endif
>  }
>  
>  GRUB_MOD_FINI(serial)
> Index: grub/include/grub/pci.h
> ===================================================================
> --- grub.orig/include/grub/pci.h
> +++ grub/include/grub/pci.h
> @@ -80,8 +80,13 @@
>  
>  #define  GRUB_PCI_STATUS_DEVSEL_TIMING_SHIFT 9
>  #define  GRUB_PCI_STATUS_DEVSEL_TIMING_MASK 0x0600
> +
>  #define  GRUB_PCI_CLASS_SUBCLASS_VGA  0x0300
>  #define  GRUB_PCI_CLASS_SUBCLASS_USB  0x0c03
> +#define  GRUB_PCI_CLASS_COMMUNICATION_SERIAL  0x0700
> +#define  GRUB_PCI_CLASS_COMMUNICATION_MODEM   0x0703
> +
> +#define  GRUB_PCI_SERIAL_16550_COMPATIBLE 0x02
>  
>  #ifndef ASM_FILE
>  
> Index: grub/include/grub/serial.h
> ===================================================================
> --- grub.orig/include/grub/serial.h
> +++ grub/include/grub/serial.h
> @@ -210,6 +210,10 @@ grub_arcserial_init (void);
>  struct grub_serial_port *grub_arcserial_add_port (const char *path);
>  #endif
>  
> +#ifdef GRUB_HAS_PCI
> +void grub_pciserial_init (void);
> +#endif
> +
>  struct grub_serial_port *grub_serial_find (const char *name);
>  extern struct grub_serial_driver grub_ns8250_driver;
>  void EXPORT_FUNC(grub_serial_unregister_driver) (struct grub_serial_driver *driver);
> Index: grub/docs/grub.texi
> ===================================================================
> --- grub.orig/docs/grub.texi
> +++ grub/docs/grub.texi
> @@ -1386,6 +1386,7 @@ separated by spaces.
>  Valid terminal input names depend on the platform, but may include
>  @samp{console} (native platform console), @samp{serial} (serial terminal),
>  @samp{serial_<port>} (serial terminal with explicit port selection),
> +@samp{pci_<bus>:<device>.<function>} (PCI serial with explicit location),
>  @samp{at_keyboard} (PC AT keyboard), or @samp{usb_keyboard} (USB keyboard
>  using the HID Boot Protocol, for cases where the firmware does not handle
>  this).
> @@ -1399,6 +1400,7 @@ separated by spaces.
>  Valid terminal output names depend on the platform, but may include
>  @samp{console} (native platform console), @samp{serial} (serial terminal),
>  @samp{serial_<port>} (serial terminal with explicit port selection),
> +@samp{pci_<bus>:<device>.<function>} (PCI serial with explicit location),
>  @samp{gfxterm} (graphics-mode output), @samp{vga_text} (VGA text output),
>  @samp{mda_text} (MDA text output), @samp{morse} (Morse-coding using system
>  beeper) or @samp{spkmodem} (simple data protocol using system speaker).
> @@ -4165,7 +4167,7 @@ Commands usable anywhere in the menu and
>  @node serial
>  @subsection serial
>  
> -@deffn Command serial [@option{--unit=unit}] [@option{--port=port}] [@option{--speed=speed}] [@option{--word=word}] [@option{--parity=parity}] [@option{--stop=stop}]
> +@deffn Command serial [@option{--unit=unit}] [@option{--port=port}] [@option{--speed=speed}] [@option{--word=word}] [@option{--parity=parity}] [@option{--stop=stop}] [@option{name}]
>  Initialize a serial device. @var{unit} is a number in the range 0-3
>  specifying which serial port to use; default is 0, which corresponds to
>  the port often called COM1.
> @@ -4197,6 +4199,19 @@ If passed no @var{unit} nor @var{port},
>  the system default serial port and its configuration. If this information
>  is not available, it will default to @var{unit} 0.
>  
> +If used @var{name}, a machine and driver specific name given during probing,
> +will be used to indicate which serial port to use. Possible forms include:
> +@itemize @bullet
> +@item
> +@samp{serial_<port>} as an alternative to --port
> +@item
> +@samp{usbN} the N'th USB serial detected
> +@item
> +@samp{efiN} the N'th EFI serial detected
> +@item
> +@samp{pci_<bus>:<device>.<func>} PCI serial at the specified location
> +@end itemize
> +
>  The serial port is not used as a communication channel unless the
>  @command{terminal_input} or @command{terminal_output} command is used
>  (@pxref{terminal_input}, @pxref{terminal_output}).
> @@ -4205,6 +4220,7 @@ Examples:
>  @example
>  serial --port=3f8 --speed=9600
>  serial --port=mmio,fefb0000.l --speed=115200
> +serial --speed=115200 pci_00:16.3
>  @end example
>  
>  See also @ref{Serial terminal}.

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

[Daniel Kiper]

Adding another Ben's address...

On Thu, Feb 09, 2023 at 11:36:46AM +0100, Peter Zijlstra wrote:
> On Thu, Jan 26, 2023 at 10:37:35AM +0100, Peter Zijlstra wrote:
> > On Wed, Dec 21, 2022 at 01:59:02PM +0100, Daniel Kiper wrote:
> > > Sorry for late reply...
> > >
> > > May I ask you to send the patches using "git send-email"?
> >
> > I'll try -- I'm one of the few quilt holdouts in Linux-land so I don't
> > have much experience with it.
> >
> >
> > > Please add a blurb how to use this driver to the docs/grub.texi file.
> > >
> > > > GRUB_TERMINAL="serial console"
> > > >
> > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > > ---
> > > >
> > > > Index: grub/grub-core/Makefile.core.def
> > > > ===================================================================
> > > > --- grub.orig/grub-core/Makefile.core.def
> > > > +++ grub/grub-core/Makefile.core.def
> > > > @@ -2047,6 +2047,7 @@ module = {
> > > >    ieee1275 = term/ieee1275/serial.c;
> > > >    mips_arc = term/arc/serial.c;
> > > >    efi = term/efi/serial.c;
> > > > +  pci = term/pci/serial.c;
> > >
> > > I think this should be "x86 = term/pci/serial.c".
> >
> > I thought the whole purpose of Glenn's patch was to allow doing the 'pci
> > =' thing, so that all platforms that support PCI get to have the
> > benefit.
> >
> > But if you really want I can make it 'x86 =' ofcourse.
>
> Ben, you've lived a very long time in !x86 land, can you elucidate us on
> this? My thinking is that if someone sticks a 16550 on a PCI board, this
> driver should work on any PCI enabled platform, not just x86, rite?

Ben, here ^^^ is the question for you...

Daniel

> > > > +static int
> > > > +find_pciserial (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
> > >
> > > grub_pci_id_t pciid __attribute__ ((unused))
> > >
> > > > +{
> > > > +  grub_pci_address_t cmd_addr, class_addr, bar_addr;
> > > > +  struct grub_serial_port *port;
> > > > +  grub_uint32_t class, bar;
> > > > +  grub_uint16_t cmdreg;
> > > > +  int *port_num = data;
> > > > +  grub_err_t err;
> > > > +
> > > > +  (void)pciid;
> > >
> > > ... and please drop this...
> > >
> >
> > > > +error:
> > >
> > > s/error/fail/
> > >
> > > And please add a space before label...
> >
> > All done, except the Makefile thing, find the updated version below.
> > I'll attempt to git-send-email both patches once we agree on the
> > Makefile hunk.
> >
> >
> > ---
> > Subject: term/serial: Add support for PCI serial devices
> >
> > 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 --speed=115200 pci_00:16.3"
> > GRUB_TERMINAL="serial console"
> >
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Tested-by: Glenn Washburn <development@efficientek.com>
> > Reviewed-by: Glenn Washburn <development@efficientek.com>
> > ---
> >
> > Index: grub/grub-core/Makefile.core.def
> > ===================================================================
> > --- grub.orig/grub-core/Makefile.core.def
> > +++ grub/grub-core/Makefile.core.def
> > @@ -2057,6 +2057,7 @@ module = {
> >    ieee1275 = term/ieee1275/serial.c;
> >    mips_arc = term/arc/serial.c;
> >    efi = term/efi/serial.c;
> > +  pci = term/pci/serial.c;
> >
> >    enable = terminfomodule;
> >    enable = ieee1275;
> > Index: grub/grub-core/term/pci/serial.c
> > ===================================================================
> > --- /dev/null
> > +++ grub/grub-core/term/pci/serial.c
> > @@ -0,0 +1,93 @@
> > +/*
> > + *  GRUB  --  GRand Unified Bootloader
> > + *  Copyright (C) 2022  Free Software Foundation, Inc.
> > + *
> > + *  GRUB is free software: you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License as published by
> > + *  the Free Software Foundation, either version 3 of the License, or
> > + *  (at your option) any later version.
> > + *
> > + *  GRUB is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + *
> > + *  You should have received a copy of the GNU General Public License
> > + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <grub/serial.h>
> > +#include <grub/term.h>
> > +#include <grub/types.h>
> > +#include <grub/pci.h>
> > +#include <grub/mm.h>
> > +#include <grub/misc.h>
> > +
> > +static int
> > +find_pciserial (grub_pci_device_t dev,
> > +		grub_pci_id_t pciid __attribute__ ((unused)),
> > +		void *data __attribute__ ((unused)))
> > +{
> > +  grub_pci_address_t cmd_addr, class_addr, bar_addr;
> > +  struct grub_serial_port *port;
> > +  grub_uint32_t class, bar;
> > +  grub_uint16_t cmdreg;
> > +  grub_err_t err;
> > +
> > +  cmd_addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
> > +  cmdreg = grub_pci_read (cmd_addr);
> > +
> > +  class_addr = grub_pci_make_address (dev, GRUB_PCI_REG_REVISION);
> > +  class = grub_pci_read (class_addr);
> > +
> > +  bar_addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
> > +  bar = grub_pci_read (bar_addr);
> > +
> > +  /* 16550 compatible MODEM or SERIAL */
> > +  if (((class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_MODEM &&
> > +       (class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_SERIAL) ||
> > +      ((class >> 8) & 0xff) != GRUB_PCI_SERIAL_16550_COMPATIBLE)
> > +    return 0;
> > +
> > +  if ((bar & GRUB_PCI_ADDR_SPACE_MASK) != GRUB_PCI_ADDR_SPACE_IO)
> > +    return 0;
> > +
> > +  port = grub_zalloc (sizeof (*port));
> > +  if (port == NULL)
> > +    return 0;
> > +
> > +  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 fail;
> > +
> > +  grub_pci_write (cmd_addr, cmdreg | GRUB_PCI_COMMAND_IO_ENABLED);
> > +
> > +  port->driver = &grub_ns8250_driver;
> > +  port->port = bar & GRUB_PCI_ADDR_IO_MASK;
> > +  err = grub_serial_config_defaults (port);
> > +  if (err != GRUB_ERR_NONE)
> > +    {
> > +      grub_print_error ();
> > +      goto fail;
> > +    }
> > +
> > +  err = grub_serial_register (port);
> > +  if (err != GRUB_ERR_NONE)
> > +    goto fail;
> > +
> > +  return 0;
> > +
> > + fail:
> > +  grub_free (port->name);
> > +  grub_free (port);
> > +  return 0;
> > +}
> > +
> > +void
> > +grub_pciserial_init (void)
> > +{
> > +  grub_pci_iterate (find_pciserial, NULL);
> > +}
> > Index: grub/grub-core/term/serial.c
> > ===================================================================
> > --- grub.orig/grub-core/term/serial.c
> > +++ grub/grub-core/term/serial.c
> > @@ -505,6 +505,9 @@ GRUB_MOD_INIT(serial)
> >  #ifdef GRUB_MACHINE_ARC
> >    grub_arcserial_init ();
> >  #endif
> > +#ifdef GRUB_HAS_PCI
> > +  grub_pciserial_init ();
> > +#endif
> >  }
> >
> >  GRUB_MOD_FINI(serial)
> > Index: grub/include/grub/pci.h
> > ===================================================================
> > --- grub.orig/include/grub/pci.h
> > +++ grub/include/grub/pci.h
> > @@ -80,8 +80,13 @@
> >
> >  #define  GRUB_PCI_STATUS_DEVSEL_TIMING_SHIFT 9
> >  #define  GRUB_PCI_STATUS_DEVSEL_TIMING_MASK 0x0600
> > +
> >  #define  GRUB_PCI_CLASS_SUBCLASS_VGA  0x0300
> >  #define  GRUB_PCI_CLASS_SUBCLASS_USB  0x0c03
> > +#define  GRUB_PCI_CLASS_COMMUNICATION_SERIAL  0x0700
> > +#define  GRUB_PCI_CLASS_COMMUNICATION_MODEM   0x0703
> > +
> > +#define  GRUB_PCI_SERIAL_16550_COMPATIBLE 0x02
> >
> >  #ifndef ASM_FILE
> >
> > Index: grub/include/grub/serial.h
> > ===================================================================
> > --- grub.orig/include/grub/serial.h
> > +++ grub/include/grub/serial.h
> > @@ -210,6 +210,10 @@ grub_arcserial_init (void);
> >  struct grub_serial_port *grub_arcserial_add_port (const char *path);
> >  #endif
> >
> > +#ifdef GRUB_HAS_PCI
> > +void grub_pciserial_init (void);
> > +#endif
> > +
> >  struct grub_serial_port *grub_serial_find (const char *name);
> >  extern struct grub_serial_driver grub_ns8250_driver;
> >  void EXPORT_FUNC(grub_serial_unregister_driver) (struct grub_serial_driver *driver);
> > Index: grub/docs/grub.texi
> > ===================================================================
> > --- grub.orig/docs/grub.texi
> > +++ grub/docs/grub.texi
> > @@ -1386,6 +1386,7 @@ separated by spaces.
> >  Valid terminal input names depend on the platform, but may include
> >  @samp{console} (native platform console), @samp{serial} (serial terminal),
> >  @samp{serial_<port>} (serial terminal with explicit port selection),
> > +@samp{pci_<bus>:<device>.<function>} (PCI serial with explicit location),
> >  @samp{at_keyboard} (PC AT keyboard), or @samp{usb_keyboard} (USB keyboard
> >  using the HID Boot Protocol, for cases where the firmware does not handle
> >  this).
> > @@ -1399,6 +1400,7 @@ separated by spaces.
> >  Valid terminal output names depend on the platform, but may include
> >  @samp{console} (native platform console), @samp{serial} (serial terminal),
> >  @samp{serial_<port>} (serial terminal with explicit port selection),
> > +@samp{pci_<bus>:<device>.<function>} (PCI serial with explicit location),
> >  @samp{gfxterm} (graphics-mode output), @samp{vga_text} (VGA text output),
> >  @samp{mda_text} (MDA text output), @samp{morse} (Morse-coding using system
> >  beeper) or @samp{spkmodem} (simple data protocol using system speaker).
> > @@ -4165,7 +4167,7 @@ Commands usable anywhere in the menu and
> >  @node serial
> >  @subsection serial
> >
> > -@deffn Command serial [@option{--unit=unit}] [@option{--port=port}] [@option{--speed=speed}] [@option{--word=word}] [@option{--parity=parity}] [@option{--stop=stop}]
> > +@deffn Command serial [@option{--unit=unit}] [@option{--port=port}] [@option{--speed=speed}] [@option{--word=word}] [@option{--parity=parity}] [@option{--stop=stop}] [@option{name}]
> >  Initialize a serial device. @var{unit} is a number in the range 0-3
> >  specifying which serial port to use; default is 0, which corresponds to
> >  the port often called COM1.
> > @@ -4197,6 +4199,19 @@ If passed no @var{unit} nor @var{port},
> >  the system default serial port and its configuration. If this information
> >  is not available, it will default to @var{unit} 0.
> >
> > +If used @var{name}, a machine and driver specific name given during probing,
> > +will be used to indicate which serial port to use. Possible forms include:
> > +@itemize @bullet
> > +@item
> > +@samp{serial_<port>} as an alternative to --port
> > +@item
> > +@samp{usbN} the N'th USB serial detected
> > +@item
> > +@samp{efiN} the N'th EFI serial detected
> > +@item
> > +@samp{pci_<bus>:<device>.<func>} PCI serial at the specified location
> > +@end itemize
> > +
> >  The serial port is not used as a communication channel unless the
> >  @command{terminal_input} or @command{terminal_output} command is used
> >  (@pxref{terminal_input}, @pxref{terminal_output}).
> > @@ -4205,6 +4220,7 @@ Examples:
> >  @example
> >  serial --port=3f8 --speed=9600
> >  serial --port=mmio,fefb0000.l --speed=115200
> > +serial --speed=115200 pci_00:16.3
> >  @end example
> >
> >  See also @ref{Serial terminal}.

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

[Benjamin Herrenschmidt]

On Thu, 2023-02-09 at 15:52 +0100, Daniel Kiper wrote:
> Adding another Ben's address...

Ah ! I saw the previous one before breakfast the other day, and forgot
to reply once properly caffeinated :-)

> > Ben, you've lived a very long time in !x86 land, can you elucidate us on
> > this? My thinking is that if someone sticks a 16550 on a PCI board, this
> > driver should work on any PCI enabled platform, not just x86, rite?
> 
> Ben, here ^^^ is the question for you...

It's ... complicated :-)

On most non-x86 platforms you don't have a 1:1 mapping between PCI BAR
value and the corresponding CPU address. Now I think with ACPI and UEFI
there are ways to convert but I don't remember them off the top of my
mind, there are similar ways with Open Firmware or flat device-trees.
But grub doesn't have the infrastructure for that.

Also non-x86 don't have "IO ports" per se, they are mapped somewhere in
the MMIO address space, and how that is done is very platform specific.

So at this point it makes sense to leave that x86 only. Once can add
support for other archs/platforms later with the appopriate address
conversion trickery.

Another issue is that grub  doesn't have any form of MMIO abstraction
layer providing things like ordering enforcement (memory barriers)
etc... so doing any kind of direct device access in grub on most non-
x86 is chancy at best.

Cheers,
Ben.

> 
> Daniel
> 
> > > > > +static int
> > > > > +find_pciserial (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
> > > > 
> > > > grub_pci_id_t pciid __attribute__ ((unused))
> > > > 
> > > > > +{
> > > > > +  grub_pci_address_t cmd_addr, class_addr, bar_addr;
> > > > > +  struct grub_serial_port *port;
> > > > > +  grub_uint32_t class, bar;
> > > > > +  grub_uint16_t cmdreg;
> > > > > +  int *port_num = data;
> > > > > +  grub_err_t err;
> > > > > +
> > > > > +  (void)pciid;
> > > > 
> > > > ... and please drop this...
> > > > 
> > > 
> > > > > +error:
> > > > 
> > > > s/error/fail/
> > > > 
> > > > And please add a space before label...
> > > 
> > > All done, except the Makefile thing, find the updated version below.
> > > I'll attempt to git-send-email both patches once we agree on the
> > > Makefile hunk.
> > > 
> > > 
> > > ---
> > > Subject: term/serial: Add support for PCI serial devices
> > > 
> > > 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 --speed=115200 pci_00:16.3"
> > > GRUB_TERMINAL="serial console"
> > > 
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > Tested-by: Glenn Washburn <development@efficientek.com>
> > > Reviewed-by: Glenn Washburn <development@efficientek.com>
> > > ---
> > > 
> > > Index: grub/grub-core/Makefile.core.def
> > > ===================================================================
> > > --- grub.orig/grub-core/Makefile.core.def
> > > +++ grub/grub-core/Makefile.core.def
> > > @@ -2057,6 +2057,7 @@ module = {
> > >    ieee1275 = term/ieee1275/serial.c;
> > >    mips_arc = term/arc/serial.c;
> > >    efi = term/efi/serial.c;
> > > +  pci = term/pci/serial.c;
> > > 
> > >    enable = terminfomodule;
> > >    enable = ieee1275;
> > > Index: grub/grub-core/term/pci/serial.c
> > > ===================================================================
> > > --- /dev/null
> > > +++ grub/grub-core/term/pci/serial.c
> > > @@ -0,0 +1,93 @@
> > > +/*
> > > + *  GRUB  --  GRand Unified Bootloader
> > > + *  Copyright (C) 2022  Free Software Foundation, Inc.
> > > + *
> > > + *  GRUB is free software: you can redistribute it and/or modify
> > > + *  it under the terms of the GNU General Public License as published by
> > > + *  the Free Software Foundation, either version 3 of the License, or
> > > + *  (at your option) any later version.
> > > + *
> > > + *  GRUB is distributed in the hope that it will be useful,
> > > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + *  GNU General Public License for more details.
> > > + *
> > > + *  You should have received a copy of the GNU General Public License
> > > + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> > > + */
> > > +
> > > +#include <grub/serial.h>
> > > +#include <grub/term.h>
> > > +#include <grub/types.h>
> > > +#include <grub/pci.h>
> > > +#include <grub/mm.h>
> > > +#include <grub/misc.h>
> > > +
> > > +static int
> > > +find_pciserial (grub_pci_device_t dev,
> > > +               grub_pci_id_t pciid __attribute__ ((unused)),
> > > +               void *data __attribute__ ((unused)))
> > > +{
> > > +  grub_pci_address_t cmd_addr, class_addr, bar_addr;
> > > +  struct grub_serial_port *port;
> > > +  grub_uint32_t class, bar;
> > > +  grub_uint16_t cmdreg;
> > > +  grub_err_t err;
> > > +
> > > +  cmd_addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
> > > +  cmdreg = grub_pci_read (cmd_addr);
> > > +
> > > +  class_addr = grub_pci_make_address (dev, GRUB_PCI_REG_REVISION);
> > > +  class = grub_pci_read (class_addr);
> > > +
> > > +  bar_addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
> > > +  bar = grub_pci_read (bar_addr);
> > > +
> > > +  /* 16550 compatible MODEM or SERIAL */
> > > +  if (((class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_MODEM &&
> > > +       (class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_SERIAL) ||
> > > +      ((class >> 8) & 0xff) != GRUB_PCI_SERIAL_16550_COMPATIBLE)
> > > +    return 0;
> > > +
> > > +  if ((bar & GRUB_PCI_ADDR_SPACE_MASK) != GRUB_PCI_ADDR_SPACE_IO)
> > > +    return 0;
> > > +
> > > +  port = grub_zalloc (sizeof (*port));
> > > +  if (port == NULL)
> > > +    return 0;
> > > +
> > > +  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 fail;
> > > +
> > > +  grub_pci_write (cmd_addr, cmdreg | GRUB_PCI_COMMAND_IO_ENABLED);
> > > +
> > > +  port->driver = &grub_ns8250_driver;
> > > +  port->port = bar & GRUB_PCI_ADDR_IO_MASK;
> > > +  err = grub_serial_config_defaults (port);
> > > +  if (err != GRUB_ERR_NONE)
> > > +    {
> > > +      grub_print_error ();
> > > +      goto fail;
> > > +    }
> > > +
> > > +  err = grub_serial_register (port);
> > > +  if (err != GRUB_ERR_NONE)
> > > +    goto fail;
> > > +
> > > +  return 0;
> > > +
> > > + fail:
> > > +  grub_free (port->name);
> > > +  grub_free (port);
> > > +  return 0;
> > > +}
> > > +
> > > +void
> > > +grub_pciserial_init (void)
> > > +{
> > > +  grub_pci_iterate (find_pciserial, NULL);
> > > +}
> > > Index: grub/grub-core/term/serial.c
> > > ===================================================================
> > > --- grub.orig/grub-core/term/serial.c
> > > +++ grub/grub-core/term/serial.c
> > > @@ -505,6 +505,9 @@ GRUB_MOD_INIT(serial)
> > >  #ifdef GRUB_MACHINE_ARC
> > >    grub_arcserial_init ();
> > >  #endif
> > > +#ifdef GRUB_HAS_PCI
> > > +  grub_pciserial_init ();
> > > +#endif
> > >  }
> > > 
> > >  GRUB_MOD_FINI(serial)
> > > Index: grub/include/grub/pci.h
> > > ===================================================================
> > > --- grub.orig/include/grub/pci.h
> > > +++ grub/include/grub/pci.h
> > > @@ -80,8 +80,13 @@
> > > 
> > >  #define  GRUB_PCI_STATUS_DEVSEL_TIMING_SHIFT 9
> > >  #define  GRUB_PCI_STATUS_DEVSEL_TIMING_MASK 0x0600
> > > +
> > >  #define  GRUB_PCI_CLASS_SUBCLASS_VGA  0x0300
> > >  #define  GRUB_PCI_CLASS_SUBCLASS_USB  0x0c03
> > > +#define  GRUB_PCI_CLASS_COMMUNICATION_SERIAL  0x0700
> > > +#define  GRUB_PCI_CLASS_COMMUNICATION_MODEM   0x0703
> > > +
> > > +#define  GRUB_PCI_SERIAL_16550_COMPATIBLE 0x02
> > > 
> > >  #ifndef ASM_FILE
> > > 
> > > Index: grub/include/grub/serial.h
> > > ===================================================================
> > > --- grub.orig/include/grub/serial.h
> > > +++ grub/include/grub/serial.h
> > > @@ -210,6 +210,10 @@ grub_arcserial_init (void);
> > >  struct grub_serial_port *grub_arcserial_add_port (const char *path);
> > >  #endif
> > > 
> > > +#ifdef GRUB_HAS_PCI
> > > +void grub_pciserial_init (void);
> > > +#endif
> > > +
> > >  struct grub_serial_port *grub_serial_find (const char *name);
> > >  extern struct grub_serial_driver grub_ns8250_driver;
> > >  void EXPORT_FUNC(grub_serial_unregister_driver) (struct grub_serial_driver *driver);
> > > Index: grub/docs/grub.texi
> > > ===================================================================
> > > --- grub.orig/docs/grub.texi
> > > +++ grub/docs/grub.texi
> > > @@ -1386,6 +1386,7 @@ separated by spaces.
> > >  Valid terminal input names depend on the platform, but may include
> > >  @samp{console} (native platform console), @samp{serial} (serial terminal),
> > >  @samp{serial_<port>} (serial terminal with explicit port selection),
> > > +@samp{pci_<bus>:<device>.<function>} (PCI serial with explicit location),
> > >  @samp{at_keyboard} (PC AT keyboard), or @samp{usb_keyboard} (USB keyboard
> > >  using the HID Boot Protocol, for cases where the firmware does not handle
> > >  this).
> > > @@ -1399,6 +1400,7 @@ separated by spaces.
> > >  Valid terminal output names depend on the platform, but may include
> > >  @samp{console} (native platform console), @samp{serial} (serial terminal),
> > >  @samp{serial_<port>} (serial terminal with explicit port selection),
> > > +@samp{pci_<bus>:<device>.<function>} (PCI serial with explicit location),
> > >  @samp{gfxterm} (graphics-mode output), @samp{vga_text} (VGA text output),
> > >  @samp{mda_text} (MDA text output), @samp{morse} (Morse-coding using system
> > >  beeper) or @samp{spkmodem} (simple data protocol using system speaker).
> > > @@ -4165,7 +4167,7 @@ Commands usable anywhere in the menu and
> > >  @node serial
> > >  @subsection serial
> > > 
> > > -@deffn Command serial [@option{--unit=unit}] [@option{--port=port}] [@option{--speed=speed}] [@option{--word=word}] [@option{--parity=parity}] [@option{--stop=stop}]
> > > +@deffn Command serial [@option{--unit=unit}] [@option{--port=port}] [@option{--speed=speed}] [@option{--word=word}] [@option{--parity=parity}] [@option{--stop=stop}] [@option{name}]
> > >  Initialize a serial device. @var{unit} is a number in the range 0-3
> > >  specifying which serial port to use; default is 0, which corresponds to
> > >  the port often called COM1.
> > > @@ -4197,6 +4199,19 @@ If passed no @var{unit} nor @var{port},
> > >  the system default serial port and its configuration. If this information
> > >  is not available, it will default to @var{unit} 0.
> > > 
> > > +If used @var{name}, a machine and driver specific name given during probing,
> > > +will be used to indicate which serial port to use. Possible forms include:
> > > +@itemize @bullet
> > > +@item
> > > +@samp{serial_<port>} as an alternative to --port
> > > +@item
> > > +@samp{usbN} the N'th USB serial detected
> > > +@item
> > > +@samp{efiN} the N'th EFI serial detected
> > > +@item
> > > +@samp{pci_<bus>:<device>.<func>} PCI serial at the specified location
> > > +@end itemize
> > > +
> > >  The serial port is not used as a communication channel unless the
> > >  @command{terminal_input} or @command{terminal_output} command is used
> > >  (@pxref{terminal_input}, @pxref{terminal_output}).
> > > @@ -4205,6 +4220,7 @@ Examples:
> > >  @example
> > >  serial --port=3f8 --speed=9600
> > >  serial --port=mmio,fefb0000.l --speed=115200
> > > +serial --speed=115200 pci_00:16.3
> > >  @end example
> > > 
> > >  See also @ref{Serial terminal}.

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

[Daniel Kiper]

On Mon, Feb 13, 2023 at 08:23:39AM +1100, Benjamin Herrenschmidt wrote:
> On Thu, 2023-02-09 at 15:52 +0100, Daniel Kiper wrote:
> > Adding another Ben's address...
>
> Ah ! I saw the previous one before breakfast the other day, and forgot
> to reply once properly caffeinated :-)
>
> > > Ben, you've lived a very long time in !x86 land, can you elucidate us on
> > > this? My thinking is that if someone sticks a 16550 on a PCI board, this
> > > driver should work on any PCI enabled platform, not just x86, rite?
> >
> > Ben, here ^^^ is the question for you...
>
> It's ... complicated :-)
>
> On most non-x86 platforms you don't have a 1:1 mapping between PCI BAR
> value and the corresponding CPU address. Now I think with ACPI and UEFI
> there are ways to convert but I don't remember them off the top of my
> mind, there are similar ways with Open Firmware or flat device-trees.
> But grub doesn't have the infrastructure for that.
>
> Also non-x86 don't have "IO ports" per se, they are mapped somewhere in
> the MMIO address space, and how that is done is very platform specific.
>
> So at this point it makes sense to leave that x86 only. Once can add
> support for other archs/platforms later with the appopriate address
> conversion trickery.
>
> Another issue is that grub  doesn't have any form of MMIO abstraction
> layer providing things like ordering enforcement (memory barriers)
> etc... so doing any kind of direct device access in grub on most non-
> x86 is chancy at best.

Ben, thank you for explanation. Taking into account above...

> --- grub.orig/grub-core/Makefile.core.def
> +++ grub/grub-core/Makefile.core.def
> @@ -2047,6 +2047,7 @@ module = {
>    ieee1275 = term/ieee1275/serial.c;
>    mips_arc = term/arc/serial.c;
>    efi = term/efi/serial.c;
> +  pci = term/pci/serial.c;

... I still think this should be "x86 = term/pci/serial.c".

Until you push me to leave it as "pci = term/pci/serial.c".
However, if you want to do that please bring in good arguments... :-)

Daniel

[PATCH v2 3/3] pci: Rename GRUB_PCI_CLASS_*

[peterz]

Glenn suggested to rename the existing PCI_CLASS defines to have
explicit class and subclass names.

Suggested-by: Glenn Washburn <development@efficientek.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
Index: grub/grub-core/kern/i386/qemu/init.c
===================================================================
--- grub.orig/grub-core/kern/i386/qemu/init.c
+++ grub/grub-core/kern/i386/qemu/init.c
@@ -168,11 +168,11 @@ enable_cards (grub_pci_device_t dev,
 
   class = (grub_pci_read (addr) >> 16) & 0xffff;
 
-  if (class == GRUB_PCI_CLASS_SUBCLASS_VGA)
+  if (class == GRUB_PCI_CLASS_DISPLAY_VGA)
     cmd |= GRUB_PCI_COMMAND_IO_ENABLED
       | GRUB_PCI_COMMAND_MEM_ENABLED;
 
-  if (class == GRUB_PCI_CLASS_SUBCLASS_USB)
+  if (class == GRUB_PCI_CLASS_SERIAL_USB)
     return 0;
 
   addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
Index: grub/grub-core/video/efi_uga.c
===================================================================
--- grub.orig/grub-core/video/efi_uga.c
+++ grub/grub-core/video/efi_uga.c
@@ -100,7 +100,7 @@ find_card (grub_pci_device_t dev, grub_p
   addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
   subclass = (grub_pci_read (addr) >> 16) & 0xffff;
 
-  if (subclass != GRUB_PCI_CLASS_SUBCLASS_VGA)
+  if (subclass != GRUB_PCI_CLASS_DISPLAY_VGA)
     return 0;
 
   /* Enable MEM address spaces */
Index: grub/grub-core/video/radeon_fuloong2e.c
===================================================================
--- grub.orig/grub-core/video/radeon_fuloong2e.c
+++ grub/grub-core/video/radeon_fuloong2e.c
@@ -72,7 +72,7 @@ find_card (grub_pci_device_t dev, grub_p
   addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
   class = grub_pci_read (addr);
 
-  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA
+  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_DISPLAY_VGA
       || pciid != 0x515a1002)
     return 0;
 
Index: grub/grub-core/video/radeon_yeeloong3a.c
===================================================================
--- grub.orig/grub-core/video/radeon_yeeloong3a.c
+++ grub/grub-core/video/radeon_yeeloong3a.c
@@ -71,7 +71,7 @@ find_card (grub_pci_device_t dev, grub_p
   addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
   class = grub_pci_read (addr);
 
-  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA
+  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_DISPLAY_VGA
       || pciid != 0x96151002)
     return 0;
 
Index: grub/grub-core/video/sis315pro.c
===================================================================
--- grub.orig/grub-core/video/sis315pro.c
+++ grub/grub-core/video/sis315pro.c
@@ -100,7 +100,7 @@ find_card (grub_pci_device_t dev, grub_p
   addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
   class = grub_pci_read (addr);
 
-  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA
+  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_DISPLAY_VGA
       || pciid != GRUB_SIS315PRO_PCIID)
     return 0;
 
Index: grub/grub-core/video/sm712.c
===================================================================
--- grub.orig/grub-core/video/sm712.c
+++ grub/grub-core/video/sm712.c
@@ -372,7 +372,7 @@ find_card (grub_pci_device_t dev, grub_p
   addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
   class = grub_pci_read (addr);
 
-  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA
+  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_DISPLAY_VGA
       || pciid != GRUB_SM712_PCIID)
     return 0;
 
Index: grub/include/grub/pci.h
===================================================================
--- grub.orig/include/grub/pci.h
+++ grub/include/grub/pci.h
@@ -81,8 +81,8 @@
 #define  GRUB_PCI_STATUS_DEVSEL_TIMING_SHIFT 9
 #define  GRUB_PCI_STATUS_DEVSEL_TIMING_MASK 0x0600
 
-#define  GRUB_PCI_CLASS_SUBCLASS_VGA  0x0300
-#define  GRUB_PCI_CLASS_SUBCLASS_USB  0x0c03
+#define  GRUB_PCI_CLASS_DISPLAY_VGA           0x0300
+#define  GRUB_PCI_CLASS_SERIAL_USB            0x0c03
 #define  GRUB_PCI_CLASS_COMMUNICATION_SERIAL  0x0700
 #define  GRUB_PCI_CLASS_COMMUNICATION_MODEM   0x0703
 

Re: [PATCH v2 3/3] pci: Rename GRUB_PCI_CLASS_*

[Daniel Kiper]

On Fri, Aug 26, 2022 at 01:01:45PM +0200, peterz@infradead.org wrote:
> Glenn suggested to rename the existing PCI_CLASS defines to have
> explicit class and subclass names.
>
> Suggested-by: Glenn Washburn <development@efficientek.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v2 0/3] term/serial: Add PCI-serial device support

[Herrenschmidt, Benjamin]

On Fri, 2022-08-26 at 13:01 +0200, peterz@infradead.org wrote:
> Hi!
> 
> A small collection of patches that adds PCI serial support.
> 
> The first patch is by Glenn and adds GRUB_HAS_PCI -- it is included so that it
> is a complete posting.
> 
> The second patch is the actual PCI serial support, and the last patch a cleanup
> requested by Glenn.
> 
> Much thanks to Glenn for all the feedback so far.

Ah fun ! I have old patches I was about to revive (we carry them in
Amazon Linux) that add SPCR support and MMIO support for our MMIO PCI
UARTs on metal instances.

Cheers,
Ben.

Re: [PATCH v2 0/3] term/serial: Add PCI-serial device support

[Glenn Washburn]

On Fri, 26 Aug 2022 13:01:42 +0200
peterz@infradead.org wrote:

> Hi!
> 
> A small collection of patches that adds PCI serial support.
> 
> The first patch is by Glenn and adds GRUB_HAS_PCI -- it is included
> so that it is a complete posting.
> 
> The second patch is the actual PCI serial support, and the last patch
> a cleanup requested by Glenn.
> 
> Much thanks to Glenn for all the feedback so far.
> 

I've had to occasion to play with AMT recently, so I've tested this
patch series and can confirm that it works as advertised. I added a
reviewed by for one of the patches in this series many months ago, and
I'll now formally add it to the whole series. The first patch has
already been accepted, but I don't see why that should cause an issue
in reviewed and merging the other two. Or does it?

Tested-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Glenn Washburn <development@efficientek.com>

Glenn