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