From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1pQ8Hs-0005ft-OM for mharc-grub-devel@gnu.org; Thu, 09 Feb 2023 09:52:36 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pQ8Hr-0005f1-04 for grub-devel@gnu.org; Thu, 09 Feb 2023 09:52:35 -0500 Received: from dibed.net-space.pl ([84.10.22.86]) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:192) (Exim 4.90_1) (envelope-from ) id 1pQ8Ho-0002GA-3B for grub-devel@gnu.org; Thu, 09 Feb 2023 09:52:34 -0500 Received: from router-fw.i.net-space.pl ([192.168.52.1]:34964 "EHLO tomti.i.net-space.pl") by router-fw-old.i.net-space.pl with ESMTP id S2171834AbjBIOwW (ORCPT ); Thu, 9 Feb 2023 15:52:22 +0100 X-Comment: RFC 2476 MSA function at dibed.net-space.pl logged sender identity as: dkiper Date: Thu, 9 Feb 2023 15:52:20 +0100 From: Daniel Kiper To: Peter Zijlstra Cc: benh@amazon.com, benh@kernel.crashing.org, grub-devel@gnu.org, development@efficientek.com Subject: Re: [PATCH v2 2/3] term/serial: Add support for PCI serial devices Message-ID: <20230209145220.55gktnrf7oqxvewj@tomti.i.net-space.pl> References: <20220826110142.966628595@alderlake.programming.kicks-ass.net> <20220826111358.334758397@alderlake.programming.kicks-ass.net> <20221221125902.jw5kjwmxupeozt3b@tomti.i.net-space.pl> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Received-SPF: pass client-ip=84.10.22.86; envelope-from=dkiper@net-space.pl; helo=dibed.net-space.pl X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Feb 2023 14:52:35 -0000 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) > > > > --- > > > > > > > > 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- SERR- > 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) > > Tested-by: Glenn Washburn > > Reviewed-by: Glenn Washburn > > --- > > > > 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 . > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +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_} (serial terminal with explicit port selection), > > +@samp{pci_:.} (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_} (serial terminal with explicit port selection), > > +@samp{pci_:.} (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_} 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_:.} 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}.