[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/3] term/serial: Add support for PCI serial devices
From: |
Daniel Kiper |
Subject: |
Re: [PATCH v2 2/3] term/serial: Add support for PCI serial devices |
Date: |
Thu, 9 Feb 2023 15:52:20 +0100 |
User-agent: |
NeoMutt/20170113 (1.7.2) |
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}.