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