[PATCH 0/5] serial: Add MMIO & SPCR support for AWS EC2 metal instances

[PATCH 0/5] serial: Add MMIO & SPCR support for AWS EC2 metal instances

[Benjamin Herrenschmidt]

(Apologies if that got sent twice, there was an issue with my setup
yesterday causing it to be sent with the wrong From: line)

This series adds support for the serial console of AWS EC2 "metal" x86
instances to grub. This requires two improvements:

- Support for MMIO accesses to the 8250 serial port. This adds the basic
plumbing, a function to register an MMIO based port, and support for
configuring an MMIO port by using the string "mmio" followed by a hex
address for the --port directive. [Q: Should we instead add a --mmio
directive ? Can we make two such options mutually exclusive with the
current infrastructure ?]

- Support for setting up a default port using the ACPI SPCR table if
present. This series will make this happen if the command "serial" is
used without arguments. In that case, SPCR will be used if present,
otherwise grub will revert to com0 with default settings as before.

This work started originally from Matthias Lange series
https://marc.info/?l=grub-devel&m=148775823217022&w=2

However, I ended up rewriting most of it using a different approach
which I felt was less invasive and simpler, as I don't expect we will
be collecting more 'backends' for ns8250.c among other things.

I did not keep Matthias original support for OXSEMI PCI uart. This can
be fairly easily added on top, however, I believe a better approach
would be to define a syntax to the "serial" command to define a PCI UART
by seg/bus/dev/fn with options to set the base clock.

We could also add a table of "known" ones as we go based in vid/did
similar to what Linux does. None of this was necessary for my purpose
and I lack the ability/time to test this setup, but it would be easy
to add it on top of this series.

This was tested using SPCR on an actual c5.metal instance, and using
explicit instanciation via serial -p mmioXXXXXXXX on a modified qemu
hacked to create MMIO PCI serial ports.

[PATCH 1/5] acpi: Export a generic grub_acpi_find_table

[Benjamin Herrenschmidt]

And convert grub_acpi_find_fadt to use it

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 grub-core/kern/acpi.c | 43 +++++++++++++++++++++++++------------------
 include/grub/acpi.h   |  3 +++
 2 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/grub-core/kern/acpi.c b/grub-core/kern/acpi.c
index 5746ac00c..10d3deb43 100644
--- a/grub-core/kern/acpi.c
+++ b/grub-core/kern/acpi.c
@@ -85,35 +85,42 @@ grub_acpi_xsdt_find_table (struct grub_acpi_table_header *xsdt, const char *sig)
   return 0;
 }
 
-struct grub_acpi_fadt *
-grub_acpi_find_fadt (void)
+void *
+grub_acpi_find_table (const char *sig)
 {
-  struct grub_acpi_fadt *fadt = 0;
+  struct grub_acpi_fadt *r = 0;
   struct grub_acpi_rsdp_v10 *rsdpv1;
   struct grub_acpi_rsdp_v20 *rsdpv2;
+
   rsdpv1 = grub_machine_acpi_get_rsdpv1 ();
   if (rsdpv1)
-    fadt = grub_acpi_rsdt_find_table ((struct grub_acpi_table_header *)
-				      (grub_addr_t) rsdpv1->rsdt_addr,
-				      GRUB_ACPI_FADT_SIGNATURE);
-  if (fadt)
-    return fadt;
+    r = grub_acpi_rsdt_find_table ((struct grub_acpi_table_header *)
+				   (grub_addr_t) rsdpv1->rsdt_addr,
+				   sig);
+  if (r)
+    return r;
   rsdpv2 = grub_machine_acpi_get_rsdpv2 ();
   if (rsdpv2)
-    fadt = grub_acpi_rsdt_find_table ((struct grub_acpi_table_header *)
-				      (grub_addr_t) rsdpv2->rsdpv1.rsdt_addr,
-				      GRUB_ACPI_FADT_SIGNATURE);
-  if (fadt)
-    return fadt;
+    r = grub_acpi_rsdt_find_table ((struct grub_acpi_table_header *)
+				   (grub_addr_t) rsdpv2->rsdpv1.rsdt_addr,
+				   sig);
+  if (r)
+    return r;
   if (rsdpv2
 #if GRUB_CPU_SIZEOF_VOID_P != 8
       && !(rsdpv2->xsdt_addr >> 32)
 #endif
       )
-    fadt = grub_acpi_xsdt_find_table ((struct grub_acpi_table_header *)
-				      (grub_addr_t) rsdpv2->xsdt_addr,
-				      GRUB_ACPI_FADT_SIGNATURE);
-  if (fadt)
-    return fadt;
+    r = grub_acpi_xsdt_find_table ((struct grub_acpi_table_header *)
+				   (grub_addr_t) rsdpv2->xsdt_addr,
+				   sig);
+  if (r)
+    return r;
   return 0;
 }
+
+struct grub_acpi_fadt *
+grub_acpi_find_fadt (void)
+{
+  return grub_acpi_find_table (GRUB_ACPI_FADT_SIGNATURE);
+}
diff --git a/include/grub/acpi.h b/include/grub/acpi.h
index 84f49487d..8c126b2b9 100644
--- a/include/grub/acpi.h
+++ b/include/grub/acpi.h
@@ -244,4 +244,7 @@ enum
 struct grub_acpi_fadt *
 EXPORT_FUNC(grub_acpi_find_fadt) (void);
 
+void *
+EXPORT_FUNC(grub_acpi_find_table) (const char *sig);
+
 #endif /* ! GRUB_ACPI_HEADER */
-- 
2.25.1

[PATCH 2/5] acpi: Add SPCR and generic address definitions

[Benjamin Herrenschmidt]

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 include/grub/acpi.h | 51 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/include/grub/acpi.h b/include/grub/acpi.h
index 8c126b2b9..17aadb802 100644
--- a/include/grub/acpi.h
+++ b/include/grub/acpi.h
@@ -179,6 +179,57 @@ enum
     GRUB_ACPI_MADT_ENTRY_SAPIC_FLAGS_ENABLED = 1
   };
 
+struct grub_acpi_genaddr {
+  grub_uint8_t space_id;
+#define GRUB_ACPI_GENADDR_MEM_SPACE 0x00
+#define GRUB_ACPI_GENADDR_IO_SPACE  0x01
+  grub_uint8_t bit_width;
+  grub_uint8_t bit_offset;
+  grub_uint8_t access_size;
+#define GRUB_ACPI_GENADDR_SIZE_LGCY  0x00
+#define GRUB_ACPI_GENADDR_SIZE_BYTE  0x01
+#define GRUB_ACPI_GENADDR_SIZE_WORD  0x02
+#define GRUB_ACPI_GENADDR_SIZE_DWORD 0x03
+#define GRUB_ACPI_GENADDR_SIZE_QWORD 0x04
+  grub_uint64_t addr;
+} GRUB_PACKED;
+
+#define GRUB_ACPI_SPCR_SIGNATURE "SPCR"
+
+struct grub_acpi_spcr {
+  struct grub_acpi_table_header hdr;
+  grub_uint8_t intf_type;
+#define GRUB_ACPI_SPCR_INTF_TYPE_16550  0x00
+#define GRUB_ACPI_SPCR_INTF_TYPE_16550X 0x01
+  grub_uint8_t reserved_0[3];
+  struct grub_acpi_genaddr base_addr;
+  grub_uint8_t interrupt_type;
+  grub_uint8_t irq;
+  grub_uint32_t gsi;
+  grub_uint8_t baud_rate;
+#define GRUB_ACPI_SPCR_BAUD_CURRENT  0x00
+#define GRUB_ACPI_SPCR_BAUD_9600     0x03
+#define GRUB_ACPI_SPCR_BAUD_19200    0x04
+#define GRUB_ACPI_SPCR_BAUD_57600    0x06
+#define GRUB_ACPI_SPCR_BAUD_115200   0x07
+  grub_uint8_t parity;
+  grub_uint8_t stop_bits;
+  grub_uint8_t flow_control;
+#define GRUB_ACPI_SPCR_FC_DCD_TX     (1 << 0)
+#define GRUB_ACPI_SPCR_FC_RTSCTS     (1 << 1)
+#define GRUB_ACPI_SPCR_FC_XONXOFF    (1 << 2)
+  grub_uint8_t terminal_type;
+  grub_uint8_t language;
+  grub_uint16_t	pci_device_id;
+  grub_uint16_t pci_vendor_id;
+  grub_uint8_t pci_bus;
+  grub_uint8_t pci_device;
+  grub_uint8_t pci_function;
+  grub_uint32_t pci_flags;
+  grub_uint8_t pci_segment;
+  grub_uint32_t reserved_1;
+} GRUB_PACKED;
+
 #ifndef GRUB_DSDT_TEST
 struct grub_acpi_rsdp_v10 *grub_acpi_get_rsdpv1 (void);
 struct grub_acpi_rsdp_v20 *grub_acpi_get_rsdpv2 (void);
-- 
2.25.1

[PATCH 3/5] ns8250: Add base support for MMIO UARTs

[Benjamin Herrenschmidt]

This adds the ability for the driver to access UARTs via MMIO instead
of PIO selectively at runtime, and exposes a new function to add an
MMIO port.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 docs/grub.texi          |  3 +-
 grub-core/term/ns8250.c | 78 ++++++++++++++++++++++++++++++++---------
 grub-core/term/serial.c | 22 ++++++++++--
 include/grub/serial.h   | 10 +++++-
 4 files changed, 92 insertions(+), 21 deletions(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index eeb3118eb..4a3287119 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -3878,7 +3878,8 @@ Commands usable anywhere in the menu and in the command-line.
 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. @var{port} is the I/O port where the UART
-is to be found; if specified it takes precedence over @var{unit}.
+is to be found; if specified it takes precedence over @var{unit}. It can
+also be ``mmio'' followed by the MMIO address of the port in hexadecimal.
 @var{speed} is the transmission speed; default is 9600. @var{word} and
 @var{stop} are the number of data bits and stop bits. Data bits must
 be in the range 5-8 and stop bits must be 1 or 2. Default is 8 data
diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
index 39809d042..183e14b3b 100644
--- a/grub-core/term/ns8250.c
+++ b/grub-core/term/ns8250.c
@@ -44,6 +44,24 @@ static int dead_ports = 0;
 #define DEFAULT_BASE_CLOCK 115200
 #endif
 
+static inline unsigned char
+ns8250_reg_read (struct grub_serial_port *port, grub_addr_t reg)
+{
+  asm volatile("" : : : "memory");
+  if (port->mmio)
+    return *((volatile unsigned char *)(port->mmio_base + reg));
+  return grub_inb (port->port + reg);
+}
+
+static inline void
+ns8250_reg_write (struct grub_serial_port *port, unsigned char value, grub_addr_t reg)
+{
+  asm volatile("" : : : "memory");
+  if (port->mmio)
+    *((volatile char *)(port->mmio_base + reg)) = value;
+  else
+    grub_outb (value, port->port + reg);
+}
 
 /* Convert speed to divisor.  */
 static unsigned short
@@ -94,43 +112,42 @@ do_real_config (struct grub_serial_port *port)
   divisor = serial_get_divisor (port, &port->config);
 
   /* Turn off the interrupt.  */
-  grub_outb (0, port->port + UART_IER);
+  ns8250_reg_write (port, 0, UART_IER);
 
   /* Set DLAB.  */
-  grub_outb (UART_DLAB, port->port + UART_LCR);
+  ns8250_reg_write (port, UART_DLAB, UART_LCR);
 
-  /* Set the baud rate.  */
-  grub_outb (divisor & 0xFF, port->port + UART_DLL);
-  grub_outb (divisor >> 8, port->port + UART_DLH);
+  ns8250_reg_write (port, divisor & 0xFF, UART_DLL);
+  ns8250_reg_write (port, divisor >> 8, UART_DLH);
 
   /* Set the line status.  */
   status |= (parities[port->config.parity]
 	     | (port->config.word_len - 5)
 	     | stop_bits[port->config.stop_bits]);
-  grub_outb (status, port->port + UART_LCR);
+  ns8250_reg_write (port, status, UART_LCR);
 
   if (port->config.rtscts)
     {
       /* Enable the FIFO.  */
-      grub_outb (UART_ENABLE_FIFO_TRIGGER1, port->port + UART_FCR);
+      ns8250_reg_write (port, UART_ENABLE_FIFO_TRIGGER1, UART_FCR);
 
       /* Turn on DTR and RTS.  */
-      grub_outb (UART_ENABLE_DTRRTS, port->port + UART_MCR);
+      ns8250_reg_write (port, UART_ENABLE_DTRRTS, UART_MCR);
     }
   else
     {
       /* Enable the FIFO.  */
-      grub_outb (UART_ENABLE_FIFO_TRIGGER14, port->port + UART_FCR);
+      ns8250_reg_write (port, UART_ENABLE_FIFO_TRIGGER14, UART_FCR);
 
       /* Turn on DTR, RTS, and OUT2.  */
-      grub_outb (UART_ENABLE_DTRRTS | UART_ENABLE_OUT2, port->port + UART_MCR);
+      ns8250_reg_write (port, UART_ENABLE_DTRRTS | UART_ENABLE_OUT2, UART_MCR);
     }
 
   /* Drain the input buffer.  */
   endtime = grub_get_time_ms () + 1000;
-  while (grub_inb (port->port + UART_LSR) & UART_DATA_READY)
+  while (ns8250_reg_read (port, UART_LSR) & UART_DATA_READY)
     {
-      grub_inb (port->port + UART_RX);
+      ns8250_reg_read (port, UART_RX);
       if (grub_get_time_ms () > endtime)
 	{
 	  port->broken = 1;
@@ -146,8 +163,8 @@ static int
 serial_hw_fetch (struct grub_serial_port *port)
 {
   do_real_config (port);
-  if (grub_inb (port->port + UART_LSR) & UART_DATA_READY)
-    return grub_inb (port->port + UART_RX);
+  if (ns8250_reg_read (port, UART_LSR) & UART_DATA_READY)
+    return ns8250_reg_read (port, UART_RX);
 
   return -1;
 }
@@ -167,7 +184,7 @@ serial_hw_put (struct grub_serial_port *port, const int c)
   else
     endtime = grub_get_time_ms () + 200;
   /* Wait until the transmitter holding register is empty.  */
-  while ((grub_inb (port->port + UART_LSR) & UART_EMPTY_TRANSMITTER) == 0)
+  while ((ns8250_reg_read (port, UART_LSR) & UART_EMPTY_TRANSMITTER) == 0)
     {
       if (grub_get_time_ms () > endtime)
 	{
@@ -180,7 +197,7 @@ serial_hw_put (struct grub_serial_port *port, const int c)
   if (port->broken)
     port->broken--;
 
-  grub_outb (c, port->port + UART_TX);
+  ns8250_reg_write (port, c, UART_TX);
 }
 
 /* Initialize a serial device. PORT is the port number for a serial device.
@@ -260,6 +277,7 @@ grub_ns8250_init (void)
 	com_ports[i].name = com_names[i];
 	com_ports[i].driver = &grub_ns8250_driver;
 	com_ports[i].port = serial_hw_io_addr[i];
+	com_ports[i].mmio = 0;
 	err = grub_serial_config_defaults (&com_ports[i]);
 	if (err)
 	  grub_print_error ();
@@ -311,8 +329,36 @@ grub_serial_ns8250_add_port (grub_port_t port)
     }
   p->driver = &grub_ns8250_driver;
   grub_serial_config_defaults (p);
+  p->mmio = 0;
   p->port = port;
   grub_serial_register (p);  
 
   return p->name;
 }
+
+char *
+grub_serial_ns8250_add_mmio(grub_addr_t addr)
+{
+  struct grub_serial_port *p;
+  unsigned i;
+  for (i = 0; i < GRUB_SERIAL_PORT_NUM; i++)
+    if (com_ports[i].mmio &&  com_ports[i].mmio_base == addr)
+	return com_names[i];
+
+  p = grub_malloc (sizeof (*p));
+  if (!p)
+    return NULL;
+  p->name = grub_xasprintf ("mmio%llx", (unsigned long long) addr);
+  if (!p->name)
+    {
+      grub_free (p);
+      return NULL;
+    }
+  p->driver = &grub_ns8250_driver;
+  grub_serial_config_defaults (p);
+  p->mmio = 1;
+  p->mmio_base = addr;
+  grub_serial_register (p);
+
+  return p->name;
+}
diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
index f9271b092..7d4dbb2de 100644
--- a/grub-core/term/serial.c
+++ b/grub-core/term/serial.c
@@ -160,6 +160,18 @@ grub_serial_find (const char *name)
       if (!name)
 	return NULL;
 
+      FOR_SERIAL_PORTS (port)
+	if (grub_strcmp (port->name, name) == 0)
+	  break;
+    }
+  if (!port && grub_memcmp (name, "mmio", sizeof ("mmio") - 1) == 0
+      && grub_isxdigit (name [sizeof ("mmio") - 1]))
+    {
+      name = grub_serial_ns8250_add_mmio (grub_strtoul (&name[sizeof ("mmio") - 1],
+							0, 16), NULL);
+      if (!name)
+	return NULL;
+
       FOR_SERIAL_PORTS (port)
 	if (grub_strcmp (port->name, name) == 0)
 	  break;
@@ -195,14 +207,18 @@ grub_cmd_serial (grub_extcmd_context_t ctxt, int argc, char **args)
   if (state[OPTION_UNIT].set)
     {
       grub_snprintf (pname, sizeof (pname), "com%ld",
-		     grub_strtoul (state[0].arg, 0, 0));
+		     grub_strtoul (state[OPTION_UNIT].arg, 0, 0));
       name = pname;
     }
 
   if (state[OPTION_PORT].set)
     {
-      grub_snprintf (pname, sizeof (pname), "port%lx",
-		     grub_strtoul (state[1].arg, 0, 0));
+      if (grub_memcmp (state[OPTION_PORT].arg, "mmio", 4) == 0)
+          grub_snprintf(pname, sizeof (pname), "%s", state[OPTION_PORT].arg);
+      else
+          grub_snprintf (pname, sizeof (pname), "port%lx",
+                         grub_strtoul (state[OPTION_PORT].arg, 0, 0));
+
       name = pname;
     }
 
diff --git a/include/grub/serial.h b/include/grub/serial.h
index 67379de45..a5756cd25 100644
--- a/include/grub/serial.h
+++ b/include/grub/serial.h
@@ -86,9 +86,16 @@ struct grub_serial_port
    */
   union
   {
+    struct
+    {
+      int mmio;
+      union {
 #if defined(__mips__) || defined (__i386__) || defined (__x86_64__)
-    grub_port_t port;
+        grub_port_t port;
 #endif
+        grub_addr_t mmio_base;
+      };
+    };
     struct
     {
       grub_usb_device_t usbdev;
@@ -178,6 +185,7 @@ grub_serial_config_defaults (struct grub_serial_port *port)
 #if defined(__mips__) || defined (__i386__) || defined (__x86_64__)
 void grub_ns8250_init (void);
 char *grub_serial_ns8250_add_port (grub_port_t port);
+char *grub_serial_ns8250_add_mmio(grub_addr_t addr);
 #endif
 #ifdef GRUB_MACHINE_IEEE1275
 void grub_ofserial_init (void);
-- 
2.25.1

Re: [PATCH 3/5] ns8250: Add base support for MMIO UARTs

[Glenn Washburn]

On Fri, 19 Mar 2021 09:07:26 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> This adds the ability for the driver to access UARTs via MMIO instead
> of PIO selectively at runtime, and exposes a new function to add an
> MMIO port.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  docs/grub.texi          |  3 +-
>  grub-core/term/ns8250.c | 78
> ++++++++++++++++++++++++++++++++--------- grub-core/term/serial.c |
> 22 ++++++++++-- include/grub/serial.h   | 10 +++++-
>  4 files changed, 92 insertions(+), 21 deletions(-)
> 
> diff --git a/docs/grub.texi b/docs/grub.texi
> index eeb3118eb..4a3287119 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3878,7 +3878,8 @@ Commands usable anywhere in the menu and in the
> command-line. 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. @var{port} is the
> I/O port where the UART 

Perhaps instead of
  "@var{port} is the I/O port where the UART is to be found"
it should be changed to
  "@var{port} is the I/O port or MMIO address where the UART is to be
  found" 

> -is to be found; if specified it takes precedence over @var{unit}. 
> +is to be found; if specified it takes precedence over @var{unit}. It
>can 
> +also be ``mmio'' followed by the MMIO address of the port in
> hexadecimal. 

This was unclear to me without reading the source. "It can also be
``mmio''" -- perhaps "It" should be "The port argument can also be an
MMIO address in hexadecimal prefixed by ``mmio''".

Or if the change in my first comment is made the whole sentence could
instead read:
  "The form of the MMIO address port argument is ``mmio'' followed by
  the MMIO address in hexadecimal." 

> @var{speed} is the
> transmission speed; default is 9600. @var{word} and @var{stop} are
> the number of data bits and stop bits. Data bits must be in the range
> 5-8 and stop bits must be 1 or 2. Default is 8 data diff --git
> a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c index
> 39809d042..183e14b3b 100644 --- a/grub-core/term/ns8250.c
> +++ b/grub-core/term/ns8250.c
> @@ -44,6 +44,24 @@ static int dead_ports = 0;
>  #define DEFAULT_BASE_CLOCK 115200
>  #endif
>  
> +static inline unsigned char
> +ns8250_reg_read (struct grub_serial_port *port, grub_addr_t reg)
> +{
> +  asm volatile("" : : : "memory");
> +  if (port->mmio)
> +    return *((volatile unsigned char *)(port->mmio_base + reg));
> +  return grub_inb (port->port + reg);
> +}
> +
> +static inline void
> +ns8250_reg_write (struct grub_serial_port *port, unsigned char
> value, grub_addr_t reg) +{
> +  asm volatile("" : : : "memory");
> +  if (port->mmio)
> +    *((volatile char *)(port->mmio_base + reg)) = value;
> +  else
> +    grub_outb (value, port->port + reg);
> +}
>  
>  /* Convert speed to divisor.  */
>  static unsigned short
> @@ -94,43 +112,42 @@ do_real_config (struct grub_serial_port *port)
>    divisor = serial_get_divisor (port, &port->config);
>  
>    /* Turn off the interrupt.  */
> -  grub_outb (0, port->port + UART_IER);
> +  ns8250_reg_write (port, 0, UART_IER);
>  
>    /* Set DLAB.  */
> -  grub_outb (UART_DLAB, port->port + UART_LCR);
> +  ns8250_reg_write (port, UART_DLAB, UART_LCR);
>  
> -  /* Set the baud rate.  */
> -  grub_outb (divisor & 0xFF, port->port + UART_DLL);
> -  grub_outb (divisor >> 8, port->port + UART_DLH);
> +  ns8250_reg_write (port, divisor & 0xFF, UART_DLL);
> +  ns8250_reg_write (port, divisor >> 8, UART_DLH);
>  
>    /* Set the line status.  */
>    status |= (parities[port->config.parity]
>  	     | (port->config.word_len - 5)
>  	     | stop_bits[port->config.stop_bits]);
> -  grub_outb (status, port->port + UART_LCR);
> +  ns8250_reg_write (port, status, UART_LCR);
>  
>    if (port->config.rtscts)
>      {
>        /* Enable the FIFO.  */
> -      grub_outb (UART_ENABLE_FIFO_TRIGGER1, port->port + UART_FCR);
> +      ns8250_reg_write (port, UART_ENABLE_FIFO_TRIGGER1, UART_FCR);
>  
>        /* Turn on DTR and RTS.  */
> -      grub_outb (UART_ENABLE_DTRRTS, port->port + UART_MCR);
> +      ns8250_reg_write (port, UART_ENABLE_DTRRTS, UART_MCR);
>      }
>    else
>      {
>        /* Enable the FIFO.  */
> -      grub_outb (UART_ENABLE_FIFO_TRIGGER14, port->port + UART_FCR);
> +      ns8250_reg_write (port, UART_ENABLE_FIFO_TRIGGER14, UART_FCR);
>  
>        /* Turn on DTR, RTS, and OUT2.  */
> -      grub_outb (UART_ENABLE_DTRRTS | UART_ENABLE_OUT2, port->port +
> UART_MCR);
> +      ns8250_reg_write (port, UART_ENABLE_DTRRTS | UART_ENABLE_OUT2,
> UART_MCR); }
>  
>    /* Drain the input buffer.  */
>    endtime = grub_get_time_ms () + 1000;
> -  while (grub_inb (port->port + UART_LSR) & UART_DATA_READY)
> +  while (ns8250_reg_read (port, UART_LSR) & UART_DATA_READY)
>      {
> -      grub_inb (port->port + UART_RX);
> +      ns8250_reg_read (port, UART_RX);
>        if (grub_get_time_ms () > endtime)
>  	{
>  	  port->broken = 1;
> @@ -146,8 +163,8 @@ static int
>  serial_hw_fetch (struct grub_serial_port *port)
>  {
>    do_real_config (port);
> -  if (grub_inb (port->port + UART_LSR) & UART_DATA_READY)
> -    return grub_inb (port->port + UART_RX);
> +  if (ns8250_reg_read (port, UART_LSR) & UART_DATA_READY)
> +    return ns8250_reg_read (port, UART_RX);
>  
>    return -1;
>  }
> @@ -167,7 +184,7 @@ serial_hw_put (struct grub_serial_port *port,
> const int c) else
>      endtime = grub_get_time_ms () + 200;
>    /* Wait until the transmitter holding register is empty.  */
> -  while ((grub_inb (port->port + UART_LSR) & UART_EMPTY_TRANSMITTER)
> == 0)
> +  while ((ns8250_reg_read (port, UART_LSR) & UART_EMPTY_TRANSMITTER)
> == 0) {
>        if (grub_get_time_ms () > endtime)
>  	{
> @@ -180,7 +197,7 @@ serial_hw_put (struct grub_serial_port *port,
> const int c) if (port->broken)
>      port->broken--;
>  
> -  grub_outb (c, port->port + UART_TX);
> +  ns8250_reg_write (port, c, UART_TX);
>  }
>  
>  /* Initialize a serial device. PORT is the port number for a serial
> device. @@ -260,6 +277,7 @@ grub_ns8250_init (void)
>  	com_ports[i].name = com_names[i];
>  	com_ports[i].driver = &grub_ns8250_driver;
>  	com_ports[i].port = serial_hw_io_addr[i];
> +	com_ports[i].mmio = 0;
>  	err = grub_serial_config_defaults (&com_ports[i]);
>  	if (err)
>  	  grub_print_error ();
> @@ -311,8 +329,36 @@ grub_serial_ns8250_add_port (grub_port_t port)
>      }
>    p->driver = &grub_ns8250_driver;
>    grub_serial_config_defaults (p);
> +  p->mmio = 0;
>    p->port = port;
>    grub_serial_register (p);  
>  
>    return p->name;
>  }
> +
> +char *
> +grub_serial_ns8250_add_mmio(grub_addr_t addr)
> +{
> +  struct grub_serial_port *p;
> +  unsigned i;
> +  for (i = 0; i < GRUB_SERIAL_PORT_NUM; i++)
> +    if (com_ports[i].mmio &&  com_ports[i].mmio_base == addr)
> +	return com_names[i];
> +
> +  p = grub_malloc (sizeof (*p));
> +  if (!p)
> +    return NULL;
> +  p->name = grub_xasprintf ("mmio%llx", (unsigned long long) addr);
> +  if (!p->name)
> +    {
> +      grub_free (p);
> +      return NULL;
> +    }
> +  p->driver = &grub_ns8250_driver;
> +  grub_serial_config_defaults (p);
> +  p->mmio = 1;
> +  p->mmio_base = addr;
> +  grub_serial_register (p);
> +
> +  return p->name;
> +}
> diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
> index f9271b092..7d4dbb2de 100644
> --- a/grub-core/term/serial.c
> +++ b/grub-core/term/serial.c
> @@ -160,6 +160,18 @@ grub_serial_find (const char *name)
>        if (!name)
>  	return NULL;
>  
> +      FOR_SERIAL_PORTS (port)
> +	if (grub_strcmp (port->name, name) == 0)
> +	  break;
> +    }
> +  if (!port && grub_memcmp (name, "mmio", sizeof ("mmio") - 1) == 0
> +      && grub_isxdigit (name [sizeof ("mmio") - 1]))

It would look nicer if the prefix were "mmio:" to more clearly mark it
as a prefix.

> +    {
> +      name = grub_serial_ns8250_add_mmio (grub_strtoul (&name[sizeof
> ("mmio") - 1],
> +							0, 16),
> NULL);
> +      if (!name)
> +	return NULL;
> +
>        FOR_SERIAL_PORTS (port)
>  	if (grub_strcmp (port->name, name) == 0)
>  	  break;
> @@ -195,14 +207,18 @@ grub_cmd_serial (grub_extcmd_context_t ctxt,
> int argc, char **args) if (state[OPTION_UNIT].set)
>      {
>        grub_snprintf (pname, sizeof (pname), "com%ld",
> -		     grub_strtoul (state[0].arg, 0, 0));
> +		     grub_strtoul (state[OPTION_UNIT].arg, 0, 0));
>        name = pname;
>      }
>  
>    if (state[OPTION_PORT].set)
>      {
> -      grub_snprintf (pname, sizeof (pname), "port%lx",
> -		     grub_strtoul (state[1].arg, 0, 0));
> +      if (grub_memcmp (state[OPTION_PORT].arg, "mmio", 4) == 0)
> +          grub_snprintf(pname, sizeof (pname), "%s",
> state[OPTION_PORT].arg);
> +      else
> +          grub_snprintf (pname, sizeof (pname), "port%lx",
> +                         grub_strtoul (state[OPTION_PORT].arg, 0,
> 0)); +
>        name = pname;
>      }
>  
> diff --git a/include/grub/serial.h b/include/grub/serial.h
> index 67379de45..a5756cd25 100644
> --- a/include/grub/serial.h
> +++ b/include/grub/serial.h
> @@ -86,9 +86,16 @@ struct grub_serial_port
>     */
>    union
>    {
> +    struct
> +    {
> +      int mmio;
> +      union {
>  #if defined(__mips__) || defined (__i386__) || defined (__x86_64__)
> -    grub_port_t port;
> +        grub_port_t port;
>  #endif
> +        grub_addr_t mmio_base;
> +      };
> +    };
>      struct
>      {
>        grub_usb_device_t usbdev;
> @@ -178,6 +185,7 @@ grub_serial_config_defaults (struct
> grub_serial_port *port) #if defined(__mips__) || defined (__i386__)
> || defined (__x86_64__) void grub_ns8250_init (void);
>  char *grub_serial_ns8250_add_port (grub_port_t port);
> +char *grub_serial_ns8250_add_mmio(grub_addr_t addr);
>  #endif
>  #ifdef GRUB_MACHINE_IEEE1275
>  void grub_ofserial_init (void);

Glenn

Re: [PATCH 3/5] ns8250: Add base support for MMIO UARTs

[Benjamin Herrenschmidt]

On Fri, 2021-03-19 at 12:16 -0500, Glenn Washburn wrote:
> diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
> 
> > index f9271b092..7d4dbb2de 100644
> > --- a/grub-core/term/serial.c
> > +++ b/grub-core/term/serial.c
> > @@ -160,6 +160,18 @@ grub_serial_find (const char *name)
> >         if (!name)
> >        return NULL;
> >   
> > +      FOR_SERIAL_PORTS (port)
> > +     if (grub_strcmp (port->name, name) == 0)
> > +       break;
> > +    }
> > +  if (!port && grub_memcmp (name, "mmio", sizeof ("mmio") - 1) == 0
> > +      && grub_isxdigit (name [sizeof ("mmio") - 1]))
> 
> 
> It would look nicer if the prefix were "mmio:" to more clearly mark it
> as a prefix.

It would but it would diverge from what is done above for port IO
already.

If you look at the caller(s) of grub_serial_find, they basically build
a portNNN string and pass that, unless I misread the code dramatically.

So I wanted to keep this consistent. This is exposed to the user via
the gdb commands, it's not only an internal representation, so if we
want to change *both*, we would have to hack the gdb code to convert
portNNN to port:NNN

Cheers,
Ben.

Re: [PATCH 3/5] ns8250: Add base support for MMIO UARTs

[Sven Anderson]

[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]

Hi all,

Am Do., 18. März 2021 um 23:08 Uhr schrieb Benjamin Herrenschmidt <
benh@kernel.crashing.org>:

> This adds the ability for the driver to access UARTs via MMIO instead
> of PIO selectively at runtime, and exposes a new function to add an
> MMIO port.
>

I had a couple of sleepless nights trying to find out why this didn't work
for my MMIO UART (Intel Cannon Lake PCH Intel C246), so I thought I would
share my findings with others in a similar situation. (See below.)


> diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
> index 39809d042..183e14b3b 100644
> --- a/grub-core/term/ns8250.c
> +++ b/grub-core/term/ns8250.c
> @@ -44,6 +44,24 @@ static int dead_ports = 0;
>  #define DEFAULT_BASE_CLOCK 115200
>  #endif
>
> +static inline unsigned char
> +ns8250_reg_read (struct grub_serial_port *port, grub_addr_t reg)
> +{
> +  asm volatile("" : : : "memory");
> +  if (port->mmio)
> +    return *((volatile unsigned char *)(port->mmio_base + reg));
> +  return grub_inb (port->port + reg);
> +}
> +
> +static inline void
> +ns8250_reg_write (struct grub_serial_port *port, unsigned char value,
> grub_addr_t reg)
> +{
> +  asm volatile("" : : : "memory");
> +  if (port->mmio)
> +    *((volatile char *)(port->mmio_base + reg)) = value;
> +  else
> +    grub_outb (value, port->port + reg);
> +}
>

This code assumes that the registers are only 8 bit wide. Apparently for my
chipset they are 32 bits wide, so it only (finally) worked when I rewrote
this code to address and write full grub_uint32_t values, like this:

------
  volatile grub_uint32_t* p = (void*)(port->mmio_base);
  *(p + reg) = (grub_uint32_t)(value);
------

Cheers

Sven

[-- Attachment #2: Type: text/html, Size: 2517 bytes --]

Re: [PATCH 3/5] ns8250: Add base support for MMIO UARTs

[Benjamin Herrenschmidt]

On Mon, 2022-05-23 at 19:11 +0200, Sven Anderson wrote:
> Hi all,
> 
> Am Do., 18. März 2021 um 23:08 Uhr schrieb Benjamin Herrenschmidt <benh@kernel.crashing.org>:
> > This adds the ability for the driver to access UARTs via MMIO instead
> > of PIO selectively at runtime, and exposes a new function to add an
> > MMIO port.
> 
> I had a couple of sleepless nights trying to find out why this didn't
> work for my MMIO UART (Intel Cannon Lake PCH Intel C246), so I
> thought I would share my findings with others in a similar situation.
> (See below.)

Thanks ! I'm overdue to re-submit those patches :-)

> > diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
> > index 39809d042..183e14b3b 100644
> > --- a/grub-core/term/ns8250.c
> > +++ b/grub-core/term/ns8250.c
> > @@ -44,6 +44,24 @@ static int dead_ports = 0;
> >  #define DEFAULT_BASE_CLOCK 115200
> >  #endif
> > 
> > +static inline unsigned char
> > +ns8250_reg_read (struct grub_serial_port *port, grub_addr_t reg)
> > +{
> > +  asm volatile("" : : : "memory");
> > +  if (port->mmio)
> > +    return *((volatile unsigned char *)(port->mmio_base + reg));
> > +  return grub_inb (port->port + reg);
> > +}
> > +
> > +static inline void
> > +ns8250_reg_write (struct grub_serial_port *port, unsigned char value, grub_addr_t reg)
> > +{
> > +  asm volatile("" : : : "memory");
> > +  if (port->mmio)
> > +    *((volatile char *)(port->mmio_base + reg)) = value;
> > +  else
> > +    grub_outb (value, port->port + reg);
> > +}
> 
> This code assumes that the registers are only 8 bit wide. Apparently
> for my chipset they are 32 bits wide, so it only (finally) worked
> when I rewrote this code to address and write full grub_uint32_t
> values, like this: 

Ah yes, there are all sort of "interesting" variations of MMIO UART
layout and my patch didn't add >8 bits as I didn't need it. The
information is available via SPCR though, so when I finally get a
chance to respin this series, I'll try to add something and ask you to
test if you don't mind ?

PS. I'm quite swamped this week, in absence of news from me in a couple
of weeks, feel free to poke for a heads up !

Cheers,
Ben.

Re: [PATCH 3/5] ns8250: Add base support for MMIO UARTs

[Benjamin Herrenschmidt]

On Mon, 2022-05-23 at 19:11 +0200, Sven Anderson wrote:
> 
> I had a couple of sleepless nights trying to find out why this didn't
> work for my MMIO UART (Intel Cannon Lake PCH Intel C246), so I
> thought I would share my findings with others in a similar situation.
> (See below.)
>  
 .../...
> 
> This code assumes that the registers are only 8 bit wide. Apparently
> for my chipset they are 32 bits wide, so it only (finally) worked
> when I rewrote this code to address and write full grub_uint32_t
> values, like this: 
> 
> ------
>   volatile grub_uint32_t* p = (void*)(port->mmio_base);
>   *(p + reg) = (grub_uint32_t)(value);
> ------

Sorry, I dopped the ball on this for a while. Getting back to it and
will re-submit the patches.

Can you send me a dump of your SPCR table ? I'd like to see if it
properly says "32-bit" there to auto-detect this.

Cheers,
Ben.

Re: [PATCH 3/5] ns8250: Add base support for MMIO UARTs

[Benjamin Herrenschmidt]

On Wed, 2022-08-31 at 15:56 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2022-05-23 at 19:11 +0200, Sven Anderson wrote:
> > 
> > I had a couple of sleepless nights trying to find out why this
> > didn't
> > work for my MMIO UART (Intel Cannon Lake PCH Intel C246), so I
> > thought I would share my findings with others in a
> > similar situation.
> > (See below.)
> >  
>  .../...
> > 
> > This code assumes that the registers are only 8 bit wide.
> > Apparently
> > for my chipset they are 32 bits wide, so it only (finally) worked
> > when I rewrote this code to address and write full grub_uint32_t
> > values, like this: 
> > 
> > ------
> >   volatile grub_uint32_t* p = (void*)(port->mmio_base);
> >   *(p + reg) = (grub_uint32_t)(value);
> > ------
> 
> Sorry, I dopped the ball on this for a while. Getting back to it and
> will re-submit the patches.
> 
> Can you send me a dump of your SPCR table ? I'd like to see if it
> properly says "32-bit" there to auto-detect this.

Does this help ? I'll resend the whole series after a bit more testing.

From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Benjamin Herrenschmidt <benh@amazon.com>
Date: Wed, 31 Aug 2022 17:19:13 +1000
Subject: [PATCH] ns8250: Support more MMIO access sizes

It is common for PCI based UARTs to use larger than one byte access
sizes. This adds support for this and uses the information present
in SPCR accordingly.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 grub-core/term/ns8250-spcr.c |  3 ++-
 grub-core/term/ns8250.c      | 45 +++++++++++++++++++++++++++++++++++++++++---
 include/grub/serial.h        |  4 +++-
 3 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
index 0786aee1b..dd589af60 100644
--- a/grub-core/term/ns8250-spcr.c
+++ b/grub-core/term/ns8250-spcr.c
@@ -73,7 +73,8 @@ grub_ns8250_spcr_init (void)
   switch (spcr->base_addr.space_id)
     {
       case GRUB_ACPI_GENADDR_MEM_SPACE:
-        return grub_serial_ns8250_add_mmio(spcr->base_addr.addr, &config);
+        return grub_serial_ns8250_add_mmio(spcr->base_addr.addr,
+                                           spcr->base_addr.access_size, &config);
       case GRUB_ACPI_GENADDR_IO_SPACE:
         return grub_serial_ns8250_add_port(spcr->base_addr.addr, &config);
       default:
diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
index d783c2897..57d4d6d38 100644
--- a/grub-core/term/ns8250.c
+++ b/grub-core/term/ns8250.c
@@ -49,7 +49,26 @@ ns8250_reg_read (struct grub_serial_port *port, grub_addr_t reg)
 {
   asm volatile("" : : : "memory");
   if (port->mmio)
-    return *((volatile unsigned char *)(port->mmio_base + reg));
+    {
+      /* Note: we assume MMIO UARTs are little endian. This is not true of all
+       * embedded platforms but will do for now
+       */
+      switch(port->access_size)
+        {
+        default:
+        case 1:
+          return *((volatile unsigned char  *)(port->mmio_base + reg));
+        case 2:
+          return grub_le_to_cpu16(*((volatile unsigned short *)(port->mmio_base + (reg << 1))));
+        case 3:
+          return grub_le_to_cpu32(*((volatile unsigned long *)(port->mmio_base + (reg << 2))));
+        case 4:
+          /* This will only work properly on 64-bit systems, thankfully it
+           * also probably doesn't exist
+           */
+          return grub_le_to_cpu64(*((volatile unsigned long long *)(port->mmio_base + (reg << 3))));
+        }
+    }
   return grub_inb (port->port + reg);
 }
 
@@ -58,7 +77,24 @@ ns8250_reg_write (struct grub_serial_port *port, unsigned char value, grub_addr_
 {
   asm volatile("" : : : "memory");
   if (port->mmio)
-    *((volatile char *)(port->mmio_base + reg)) = value;
+    {
+      switch(port->access_size)
+        {
+        default:
+        case 1:
+          *((volatile unsigned char *)(port->mmio_base + reg)) = value;
+          break;
+        case 2:
+          *((volatile unsigned short *)(port->mmio_base + (reg << 1))) = grub_cpu_to_le16(value);
+          break;
+        case 3:
+          *((volatile unsigned long *)(port->mmio_base + (reg << 2))) = grub_cpu_to_le32(value);
+          break;
+        case 4:
+          *((volatile unsigned long long *)(port->mmio_base + (reg << 3))) = grub_cpu_to_le64(value);
+          break;
+        }
+    }
   else
     grub_outb (value, port->port + reg);
 }
@@ -283,6 +319,7 @@ grub_ns8250_init (void)
 	  grub_print_error ();
 
 	grub_serial_register (&com_ports[i]);
+	com_ports[i].access_size = 1;
       }
 }
 
@@ -333,6 +370,7 @@ grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config *config
   p->driver = &grub_ns8250_driver;
   p->mmio = 0;
   p->port = port;
+  p->access_size = 1;
   if (config)
     grub_serial_port_configure (p, config);
   else
@@ -343,7 +381,7 @@ grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config *config
 }
 
 char *
-grub_serial_ns8250_add_mmio(grub_addr_t addr, struct grub_serial_config *config)
+grub_serial_ns8250_add_mmio(grub_addr_t addr, unsigned int acc_size, struct grub_serial_config *config)
 {
   struct grub_serial_port *p;
   unsigned i;
@@ -367,6 +405,7 @@ grub_serial_ns8250_add_mmio(grub_addr_t addr, struct grub_serial_config *config)
   p->driver = &grub_ns8250_driver;
   p->mmio = 1;
   p->mmio_base = addr;
+  p->access_size = acc_size;
   if (config)
     grub_serial_port_configure (p, config);
   else
diff --git a/include/grub/serial.h b/include/grub/serial.h
index 588797768..56a80bb8b 100644
--- a/include/grub/serial.h
+++ b/include/grub/serial.h
@@ -94,6 +94,8 @@ struct grub_serial_port
         grub_port_t port;
 #endif
         grub_addr_t mmio_base;
+        /* Access size uses ACPI definition */
+        unsigned int access_size;
       };
     };
     struct
@@ -186,7 +188,7 @@ grub_serial_config_defaults (struct grub_serial_port *port)
 void grub_ns8250_init (void);
 char * grub_ns8250_spcr_init (void);
 char *grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config *config);
-char *grub_serial_ns8250_add_mmio(grub_addr_t addr, struct grub_serial_config *config);
+char *grub_serial_ns8250_add_mmio(grub_addr_t addr, unsigned int acc_size, struct grub_serial_config *config);
 #endif
 #ifdef GRUB_MACHINE_IEEE1275
 void grub_ofserial_init (void);

Re: [PATCH 3/5] ns8250: Add base support for MMIO UARTs

[Benjamin Herrenschmidt]

On Wed, 2022-08-31 at 18:26 +1000, Benjamin Herrenschmidt wrote:
>        grub_addr_t mmio_base;
> +        /* Access size uses ACPI definition */
> +        unsigned int access_size;
>        };
>      };
>  

This is buggy, access_size needs to be out of the union. I'll resend
the whole series with that patch fixed up once I'm done testing,
probably later today.

Cheers,
Ben.

[PATCH 4/5] ns8250: Add configuration parameter when adding ports

[Benjamin Herrenschmidt]

This will allow ports to be added with a pre-set configuration

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 grub-core/term/ns8250.c | 25 +++++++++++++++++++------
 grub-core/term/serial.c |  2 +-
 include/grub/serial.h   |  4 ++--
 3 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
index 183e14b3b..d783c2897 100644
--- a/grub-core/term/ns8250.c
+++ b/grub-core/term/ns8250.c
@@ -298,7 +298,7 @@ grub_ns8250_hw_get_port (const unsigned int unit)
 }
 
 char *
-grub_serial_ns8250_add_port (grub_port_t port)
+grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config *config)
 {
   struct grub_serial_port *p;
   unsigned i;
@@ -307,6 +307,9 @@ grub_serial_ns8250_add_port (grub_port_t port)
       {
 	if (dead_ports & (1 << i))
 	  return NULL;
+	/* give the opportunity for SPCR to configure a default com port */
+	if (config)
+	  grub_serial_port_configure (&com_ports[i], config);
 	return com_names[i];
       }
 
@@ -328,22 +331,29 @@ grub_serial_ns8250_add_port (grub_port_t port)
       return NULL;
     }
   p->driver = &grub_ns8250_driver;
-  grub_serial_config_defaults (p);
   p->mmio = 0;
   p->port = port;
+  if (config)
+    grub_serial_port_configure (p, config);
+  else
+    grub_serial_config_defaults (p);
   grub_serial_register (p);  
 
   return p->name;
 }
 
 char *
-grub_serial_ns8250_add_mmio(grub_addr_t addr)
+grub_serial_ns8250_add_mmio(grub_addr_t addr, struct grub_serial_config *config)
 {
   struct grub_serial_port *p;
   unsigned i;
   for (i = 0; i < GRUB_SERIAL_PORT_NUM; i++)
-    if (com_ports[i].mmio &&  com_ports[i].mmio_base == addr)
-	return com_names[i];
+    if (com_ports[i].mmio && com_ports[i].mmio_base == addr)
+      {
+        if (config)
+          grub_serial_port_configure (&com_ports[i], config);
+        return com_names[i];
+      }
 
   p = grub_malloc (sizeof (*p));
   if (!p)
@@ -355,9 +365,12 @@ grub_serial_ns8250_add_mmio(grub_addr_t addr)
       return NULL;
     }
   p->driver = &grub_ns8250_driver;
-  grub_serial_config_defaults (p);
   p->mmio = 1;
   p->mmio_base = addr;
+  if (config)
+    grub_serial_port_configure (p, config);
+  else
+    grub_serial_config_defaults (p);
   grub_serial_register (p);
 
   return p->name;
diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
index 7d4dbb2de..313341f53 100644
--- a/grub-core/term/serial.c
+++ b/grub-core/term/serial.c
@@ -156,7 +156,7 @@ grub_serial_find (const char *name)
       && grub_isxdigit (name [sizeof ("port") - 1]))
     {
       name = grub_serial_ns8250_add_port (grub_strtoul (&name[sizeof ("port") - 1],
-							0, 16));
+							0, 16), NULL);
       if (!name)
 	return NULL;
 
diff --git a/include/grub/serial.h b/include/grub/serial.h
index a5756cd25..5677dae33 100644
--- a/include/grub/serial.h
+++ b/include/grub/serial.h
@@ -184,8 +184,8 @@ grub_serial_config_defaults (struct grub_serial_port *port)
 
 #if defined(__mips__) || defined (__i386__) || defined (__x86_64__)
 void grub_ns8250_init (void);
-char *grub_serial_ns8250_add_port (grub_port_t port);
-char *grub_serial_ns8250_add_mmio(grub_addr_t addr);
+char *grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config *config);
+char *grub_serial_ns8250_add_mmio(grub_addr_t addr, struct grub_serial_config *config);
 #endif
 #ifdef GRUB_MACHINE_IEEE1275
 void grub_ofserial_init (void);
-- 
2.25.1

[PATCH 5/5] ns8250: Use ACPI SPCR table when available to configure serial

[Benjamin Herrenschmidt]

"serial auto" is now equivalent to just "serial" and will use the
SPCR to discover the port if present, otherwise defaults to "com0"
as before.

This allows to support MMIO ports specified by ACPI which is needed
on AWS EC2 "metal" instances, and will enable grub to pickup the
port configuration specified by ACPI in other cases.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 grub-core/Makefile.core.def  |  1 +
 grub-core/term/ns8250-spcr.c | 82 ++++++++++++++++++++++++++++++++++++
 grub-core/term/serial.c      | 14 +++++-
 include/grub/serial.h        |  1 +
 4 files changed, 96 insertions(+), 2 deletions(-)
 create mode 100644 grub-core/term/ns8250-spcr.c

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 8022e1c0a..2830a1e85 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -2033,6 +2033,7 @@ module = {
   name = serial;
   common = term/serial.c;
   x86 = term/ns8250.c;
+  x86 = term/ns8250-spcr.c;
   ieee1275 = term/ieee1275/serial.c;
   mips_arc = term/arc/serial.c;
   efi = term/efi/serial.c;
diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
new file mode 100644
index 000000000..63a67d07c
--- /dev/null
+++ b/grub-core/term/ns8250-spcr.c
@@ -0,0 +1,82 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2000,2001,2002,2003,2004,2005,2007,2008,2009,2010  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/ns8250.h>
+#include <grub/types.h>
+#include <grub/dl.h>
+#include <grub/acpi.h>
+
+char *
+grub_ns8250_spcr_init (void)
+{
+  struct grub_acpi_spcr *spcr;
+  struct grub_serial_config config;
+
+  spcr = grub_acpi_find_table (GRUB_ACPI_SPCR_SIGNATURE);
+  if (!spcr)
+    return 0;
+  if (spcr->hdr.revision < 2)
+    return 0;
+  if (spcr->intf_type != GRUB_ACPI_SPCR_INTF_TYPE_16550 &&
+      spcr->intf_type != GRUB_ACPI_SPCR_INTF_TYPE_16550X)
+    return 0;
+  /* For now, we only support byte accesses */
+  if (spcr->base_addr.access_size != GRUB_ACPI_GENADDR_SIZE_BYTE &&
+      spcr->base_addr.access_size != GRUB_ACPI_GENADDR_SIZE_LGCY)
+    return 0;
+  config.word_len = 8;
+  config.parity = GRUB_SERIAL_PARITY_NONE;
+  config.stop_bits = GRUB_SERIAL_STOP_BITS_1;
+  config.base_clock = 1843200;
+  if (spcr->flow_control & GRUB_ACPI_SPCR_FC_RTSCTS)
+    config.rtscts = 1;
+  else
+    config.rtscts = 0;
+  switch (spcr->baud_rate)
+    {
+      case GRUB_ACPI_SPCR_BAUD_9600:
+        config.speed = 9600;
+	break;
+      case GRUB_ACPI_SPCR_BAUD_19200:
+        config.speed = 19200;
+	break;
+      case GRUB_ACPI_SPCR_BAUD_57600:
+        config.speed = 57600;
+	break;
+      case GRUB_ACPI_SPCR_BAUD_115200:
+        config.speed = 115200;
+	break;
+      case GRUB_ACPI_SPCR_BAUD_CURRENT:
+      default:
+       /* We don't (yet) have a way to read the currently
+	* configured speed in HW, so let's use a sane default
+	*/
+        config.speed = 115200;
+	break;
+    };
+  switch (spcr->base_addr.space_id)
+    {
+      case GRUB_ACPI_GENADDR_MEM_SPACE:
+        return grub_serial_ns8250_add_mmio(spcr->base_addr.addr, &config);
+      case GRUB_ACPI_GENADDR_IO_SPACE:
+        return grub_serial_ns8250_add_port(spcr->base_addr.addr, &config);
+      default:
+	return 0;
+    };
+}
diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
index 313341f53..b966b1854 100644
--- a/grub-core/term/serial.c
+++ b/grub-core/term/serial.c
@@ -171,11 +171,21 @@ grub_serial_find (const char *name)
 							0, 16), NULL);
       if (!name)
 	return NULL;
-
       FOR_SERIAL_PORTS (port)
 	if (grub_strcmp (port->name, name) == 0)
 	  break;
     }
+  if (!port && grub_strcmp (name, "auto") == 0)
+    {
+      /* Look for an SPCR if any. If not, default to com0 */
+      name = grub_ns8250_spcr_init();
+      if (!name)
+        name = "com0";
+
+      FOR_SERIAL_PORTS (port)
+        if (grub_strcmp (port->name, name) == 0)
+          break;
+    }
 #endif
 
 #ifdef GRUB_MACHINE_IEEE1275
@@ -226,7 +236,7 @@ grub_cmd_serial (grub_extcmd_context_t ctxt, int argc, char **args)
     name = args[0];
 
   if (!name)
-    name = "com0";
+    name = "auto";
 
   port = grub_serial_find (name);
   if (!port)
diff --git a/include/grub/serial.h b/include/grub/serial.h
index 5677dae33..0fbda220f 100644
--- a/include/grub/serial.h
+++ b/include/grub/serial.h
@@ -184,6 +184,7 @@ grub_serial_config_defaults (struct grub_serial_port *port)
 
 #if defined(__mips__) || defined (__i386__) || defined (__x86_64__)
 void grub_ns8250_init (void);
+char * grub_ns8250_spcr_init (void);
 char *grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config *config);
 char *grub_serial_ns8250_add_mmio(grub_addr_t addr, struct grub_serial_config *config);
 #endif
-- 
2.25.1

Re: [PATCH 0/5] serial: Add MMIO & SPCR support for AWS EC2 metal instances

[Glenn Washburn]

Hey Ben,

On Fri, 19 Mar 2021 09:07:23 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> (Apologies if that got sent twice, there was an issue with my setup
> yesterday causing it to be sent with the wrong From: line)
> 
> This series adds support for the serial console of AWS EC2 "metal" x86
> instances to grub. This requires two improvements:
> 
> - Support for MMIO accesses to the 8250 serial port. This adds the
> basic plumbing, a function to register an MMIO based port, and
> support for configuring an MMIO port by using the string "mmio"
> followed by a hex address for the --port directive. [Q: Should we
> instead add a --mmio directive ? Can we make two such options
> mutually exclusive with the current infrastructure ?]
> 
> - Support for setting up a default port using the ACPI SPCR table if
> present. This series will make this happen if the command "serial" is
> used without arguments. In that case, SPCR will be used if present,
> otherwise grub will revert to com0 with default settings as before.
> 
> This work started originally from Matthias Lange series
> https://marc.info/?l=grub-devel&m=148775823217022&w=2
> 
> However, I ended up rewriting most of it using a different approach
> which I felt was less invasive and simpler, as I don't expect we will
> be collecting more 'backends' for ns8250.c among other things.
> 
> I did not keep Matthias original support for OXSEMI PCI uart. This can
> be fairly easily added on top, however, I believe a better approach
> would be to define a syntax to the "serial" command to define a PCI
> UART by seg/bus/dev/fn with options to set the base clock.
> 
> We could also add a table of "known" ones as we go based in vid/did
> similar to what Linux does. None of this was necessary for my purpose
> and I lack the ability/time to test this setup, but it would be easy
> to add it on top of this series.
> 
> This was tested using SPCR on an actual c5.metal instance, and using
> explicit instanciation via serial -p mmioXXXXXXXX on a modified qemu
> hacked to create MMIO PCI serial ports.

When you say a modified qemu, was that a source level change? I'm
curious how hard it would be to add this test to the current GRUB make
check tests (many of which already use qemu). Of course, if they source
of qemu was modified, then its probably a deal breaker (until it could
be accepted upstream). 

Also I haven't looked into it, but seems like it might not be hard to
add a separate test for the part using the SPCR table via qemu (perhaps
using the "-acpitable" arg).

Also, could you add to the documentation on the usage of these changes?
New functionality should in general come with new tests (when feasible)
and additions to the documentation.

Thanks for contributing,
Glenn

Re: [PATCH 0/5] serial: Add MMIO & SPCR support for AWS EC2 metal instances

[Benjamin Herrenschmidt]

On Thu, 2021-03-18 at 19:03 -0500, Glenn Washburn wrote:
> > This was tested using SPCR on an actual c5.metal instance, and using
> > explicit instanciation via serial -p mmioXXXXXXXX on a modified qemu
> > hacked to create MMIO PCI serial ports.
> 
> 
> When you say a modified qemu, was that a source level change? I'm
> curious how hard it would be to add this test to the current GRUB make
> check tests (many of which already use qemu). Of course, if they source
> of qemu was modified, then its probably a deal breaker (until it could
> be accepted upstream). 

Yes, I added an "mmio" option to pci-serial that makes register with
the Amazon UART vid/did and use an MMIO BAR :-) It's a bit of a hack
but I can try upstreaming it.

> Also I haven't looked into it, but seems like it might not be hard to
> add a separate test for the part using the SPCR table via qemu (perhaps
> using the "-acpitable" arg).

My experience with ACPI is extremely limited, I've never done such
things as build tables etc... but I can certainly try to look into it
as time permits.

> Also, could you add to the documentation on the usage of these changes?

I added some documentation to the "serial" command syntax change for
MMIO. I didn't add anything about SPCR indeed, where do you suggest I
add it ? Same spot ?

> New functionality should in general come with new tests (when feasible)

Yes, I understand, it's just that in this specific case it's rather
hard ... :-) I'll see what I can do with qemu but the best case
scenario would involve upstreaming something there and then depending
on that change trickling down to be able to use it in the grub tests
which would be ... tricky....

> and additions to the documentation.

Right, I did a bit, I can look into adding more, let me know if there
are other parts of the doc you wish me to update.

Cheers,
Ben.

Re: [PATCH 0/5] serial: Add MMIO & SPCR support for AWS EC2 metal instances

[Glenn Washburn]

On Fri, 19 Mar 2021 12:27:15 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Thu, 2021-03-18 at 19:03 -0500, Glenn Washburn wrote:
> > > This was tested using SPCR on an actual c5.metal instance, and
> > > using explicit instanciation via serial -p mmioXXXXXXXX on a
> > > modified qemu hacked to create MMIO PCI serial ports.
> > 
> > 
> > When you say a modified qemu, was that a source level change? I'm
> > curious how hard it would be to add this test to the current GRUB
> > make check tests (many of which already use qemu). Of course, if
> > they source of qemu was modified, then its probably a deal breaker
> > (until it could be accepted upstream). 
> 
> Yes, I added an "mmio" option to pci-serial that makes register with
> the Amazon UART vid/did and use an MMIO BAR :-) It's a bit of a hack
> but I can try upstreaming it.

I may have been confusing above. When I said deal breaker, I didn't
mean for the patch series. I meant if it was source changes to qemu (as
it is), it would be a deal breaker to require that test. It would be
ideal to try to upstream the change to qemu and then get a test working
here, but it shouldn't be a requirement to include this patch series.

> > Also I haven't looked into it, but seems like it might not be hard
> > to add a separate test for the part using the SPCR table via qemu
> > (perhaps using the "-acpitable" arg).
> 
> My experience with ACPI is extremely limited, I've never done such
> things as build tables etc... but I can certainly try to look into it
> as time permits.

Ok, I was hoping it might be low hanging fruit. As it seems like it
might not be, I don't think not having this test should be a blocker.
But it would be nice if you could take a quick look at it to see if it
would in fact be easy.

> > Also, could you add to the documentation on the usage of these
> > changes?
> 
> I added some documentation to the "serial" command syntax change for
> MMIO. I didn't add anything about SPCR indeed, where do you suggest I
> add it ? Same spot ?

Yes, at the end of that paragraph an additional sentence seems like a
good place. I'll add a comment on some changes to the --mmio doc change
in that patch.

> > New functionality should in general come with new tests (when
> > feasible)
> 
> Yes, I understand, it's just that in this specific case it's rather
> hard ... :-) I'll see what I can do with qemu but the best case
> scenario would involve upstreaming something there and then depending
> on that change trickling down to be able to use it in the grub tests
> which would be ... tricky....

Yep, I'm not suggesting we add any tests that rely on changes not in
a qemu release. 

> > and additions to the documentation.
> 
> Right, I did a bit, I can look into adding more, let me know if there
> are other parts of the doc you wish me to update.

Ok, so to sum up, I don't think these changes should be blocked by not
having tests. There still needs to be a proper code review, which I'm
no comfortable doing since its not really my area.

Glenn

Re: [PATCH 0/5] serial: Add MMIO & SPCR support for AWS EC2 metal instances

[Daniel Kiper]

Hi Benjamin,

On Fri, Mar 19, 2021 at 09:07:23AM +1100, Benjamin Herrenschmidt wrote:
> (Apologies if that got sent twice, there was an issue with my setup
> yesterday causing it to be sent with the wrong From: line)
>
> This series adds support for the serial console of AWS EC2 "metal" x86
> instances to grub. This requires two improvements:

Thank you for your patches. Sadly we are in code freeze stage before
release. So, your patch set does not count as a release material at this
point. I will take a look at it after release. Sorry for the inconvenience.

Daniel

Re: [PATCH 0/5] serial: Add MMIO & SPCR support for AWS EC2 metal instances

[Benjamin Herrenschmidt]

On Tue, 2021-03-23 at 19:04 +0100, Daniel Kiper wrote:
> Hi Benjamin,
> 
> On Fri, Mar 19, 2021 at 09:07:23AM +1100, Benjamin Herrenschmidt wrote:
> > (Apologies if that got sent twice, there was an issue with my setup
> > yesterday causing it to be sent with the wrong From: line)
> > 
> > This series adds support for the serial console of AWS EC2 "metal" x86
> > instances to grub. This requires two improvements:
> 
> Thank you for your patches. Sadly we are in code freeze stage before
> release. So, your patch set does not count as a release material at this
> point. I will take a look at it after release. Sorry for the inconvenience.

That's fine ! I already got some feedback that I will address and post
a v2, we can get things reviewed in the meantime :)

Cheers,
Ben.