[PATCH v2 0/8] serial: Add MMIO & SPCR support

[PATCH v2 0/8] serial: Add MMIO & SPCR support

[Benjamin Herrenschmidt]

This series adds support for MMIO serial ports and auto-configuration
via ACPI SPCR.

This is necessary for the serial port to work on AWS EC2 "metal" x86
systems.

An MMIO serial port can be specified explicitely using the
"mmio," prefix for the --port argument to the 'serial' command,
the register access size can also be specified via a suffix,
and when 'serial' is used without arguments, it will try to use
an ACPI SPCR entry (if any) to add the default port and configure
it appropriately (speed, parity etc...)

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 and to create a SPCR ACPI table.

The insipiration was an original implementation by
Matthias Lange ! matthias.lange at kernkonzept.com
However, the code was rewritten pretty much from scratch.

v2: This version (hopefully) addresses all the review commments and
has been rebased on the latest master branch.

v3: - Remove change to grub_cmd_serial() in patch 3, it serves no
purpose at this stage in the series.
    - Rework string parsing so that patch 7 only affects new code
and goes back to using sizeof (...) - 1.
    - Fix MMIO port matching to search again without the acccess
size prefix before creating a new port
    - Move tentative changes to existing code to a separate series

[PATCH v3 1/8] 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 70898ddcd..5d5b54b72 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.34.1

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

[Benjamin Herrenschmidt]

This adds the definition of the two ACPI tables according to
the spec.

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

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

[Benjamin Herrenschmidt]

From: Benjamin Herrenschmidt <benh@amazon.com>

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.

In an ideal world, MMIO accessors would be generic and have architecture
specific memory barriers. However, existing drivers don't have them and
most of those "bare metal" drivers tend to be for x86 which doesn't need
them. If necessary, those can be added later.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 grub-core/term/ns8250.c | 80 ++++++++++++++++++++++++++++++++---------
 include/grub/serial.h   | 11 +++++-
 2 files changed, 74 insertions(+), 17 deletions(-)

diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
index 83b25990c..43b9b3719 100644
--- a/grub-core/term/ns8250.c
+++ b/grub-core/term/ns8250.c
@@ -43,6 +43,24 @@ static int dead_ports = 0;
 #define DEFAULT_BASE_CLOCK 115200
 #endif
 
+static grub_uint8_t
+ns8250_reg_read (struct grub_serial_port *port, grub_addr_t reg)
+{
+  asm volatile("" : : : "memory");
+  if (port->mmio)
+    return *((volatile grub_uint8_t *) (port->mmio_base + reg));
+  return grub_inb (port->port + reg);
+}
+
+static void
+ns8250_reg_write (struct grub_serial_port *port, grub_uint8_t, grub_addr_t reg)
+{
+  asm volatile("" : : : "memory");
+  if (port->mmio)
+    *((volatile grub_uint8_t *) (port->mmio_base + reg)) = value;
+  else
+    grub_outb (value, port->port + reg);
+}
 
 /* Convert speed to divisor.  */
 static unsigned short
@@ -93,43 +111,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;
@@ -145,8 +162,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;
 }
@@ -166,7 +183,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)
 	{
@@ -179,7 +196,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.
@@ -262,6 +279,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 = false;
 	err = grub_serial_config_defaults (&com_ports[i]);
 	if (err)
 	  grub_print_error ();
@@ -289,6 +307,7 @@ grub_serial_ns8250_add_port (grub_port_t port)
 {
   struct grub_serial_port *p;
   unsigned i;
+
   for (i = 0; i < GRUB_SERIAL_PORT_NUM; i++)
     if (com_ports[i].port == port)
       {
@@ -316,8 +335,37 @@ grub_serial_ns8250_add_port (grub_port_t port)
     }
   p->driver = &grub_ns8250_driver;
   grub_serial_config_defaults (p);
+  p->mmio = false;
   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 == NULL)
+    return NULL;
+  p->name = grub_xasprintf ("mmio,%llx", (unsigned long long) addr);
+  if (p->name == NULL)
+    {
+      grub_free (p);
+      return NULL;
+    }
+  p->driver = &grub_ns8250_driver;
+  grub_serial_config_defaults (p);
+  p->mmio = true;
+  p->mmio_base = addr;
+  grub_serial_register (p);
+
+  return p->name;
+}
diff --git a/include/grub/serial.h b/include/grub/serial.h
index 67379de45..5e008f0f5 100644
--- a/include/grub/serial.h
+++ b/include/grub/serial.h
@@ -86,9 +86,17 @@ struct grub_serial_port
    */
   union
   {
+    struct
+    {
+      bool 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 +186,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.34.1

[PATCH v3 4/8] ns8250: Move base clock definition to a header

[Benjamin Herrenschmidt]

And while at it, unify it as clock frequency in HZ, to match the
value in "struct grub_serial_config" and do the division by
16 in one common place.

This will simplify adding SPCR support.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 grub-core/term/ns8250.c | 15 ++++++++-------
 include/grub/ns8250.h   | 13 +++++++++++++
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
index 43b9b3719..9e0ebff5a 100644
--- a/grub-core/term/ns8250.c
+++ b/grub-core/term/ns8250.c
@@ -37,12 +37,6 @@ static const grub_port_t serial_hw_io_addr[] = GRUB_MACHINE_SERIAL_PORTS;
 
 static int dead_ports = 0;
 
-#ifdef GRUB_MACHINE_MIPS_LOONGSON
-#define DEFAULT_BASE_CLOCK (2 * 115200)
-#else
-#define DEFAULT_BASE_CLOCK 115200
-#endif
-
 static grub_uint8_t
 ns8250_reg_read (struct grub_serial_port *port, grub_addr_t reg)
 {
@@ -71,7 +65,14 @@ serial_get_divisor (const struct grub_serial_port *port __attribute__ ((unused))
   grub_uint32_t divisor;
   grub_uint32_t actual_speed, error;
 
-  base_clock = config->base_clock ? (config->base_clock >> 4) : DEFAULT_BASE_CLOCK;
+  /* Get the UART input clock frequency */
+  base_clock = config->base_clock ? config->base_clock : UART_DEFAULT_BASE_CLOCK;
+
+  /*
+   * The UART uses 16 times oversampling for the BRG, so adjust the value
+   * accordingly to calculate the divisor.
+   */
+  base_clock >>= 4;
 
   divisor = (base_clock + (config->speed / 2)) / config->speed;
   if (config->speed == 0)
diff --git a/include/grub/ns8250.h b/include/grub/ns8250.h
index 7947ba9c9..697637912 100644
--- a/include/grub/ns8250.h
+++ b/include/grub/ns8250.h
@@ -70,6 +70,19 @@
 /* Turn on DTR, RTS, and OUT2.  */
 #define UART_ENABLE_OUT2	0x08
 
+/*
+ * Default clock input of the UART (feeds the baud rate generator)
+ *
+ * The standard value here is 1.8432 Mhz, which corresponds to
+ * 115200 bauds * 16 (16 times oversampling).
+ *
+ */
+#ifdef GRUB_MACHINE_MIPS_LOONGSON
+#define UART_DEFAULT_BASE_CLOCK ((2 * 115200) << 4)
+#else
+#define UART_DEFAULT_BASE_CLOCK (115200 << 4)
+#endif
+
 #ifndef ASM_FILE
 #include <grub/cpu/io.h>
 
-- 
2.34.1

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

[Benjamin Herrenschmidt]

From: Benjamin Herrenschmidt <benh@amazon.com>

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 | 27 ++++++++++++++++++++-------
 grub-core/term/serial.c |  2 +-
 include/grub/serial.h   |  4 ++--
 3 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
index 9e0ebff5a..d9d93fcf8 100644
--- a/grub-core/term/ns8250.c
+++ b/grub-core/term/ns8250.c
@@ -304,7 +304,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;
@@ -314,6 +314,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 != NULL)
+	  grub_serial_port_configure (&com_ports[i], config);
 	return com_names[i];
       }
 
@@ -326,32 +329,39 @@ grub_serial_ns8250_add_port (grub_port_t port)
     return NULL;
 
   p = grub_malloc (sizeof (*p));
-  if (!p)
+  if (p == NULL)
     return NULL;
   p->name = grub_xasprintf ("port%lx", (unsigned long) port);
-  if (!p->name)
+  if (p->name == NULL)
     {
       grub_free (p);
       return NULL;
     }
   p->driver = &grub_ns8250_driver;
-  grub_serial_config_defaults (p);
   p->mmio = false;
   p->port = port;
+  if (config != NULL)
+    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 (config != NULL)
+          grub_serial_port_configure (&com_ports[i], config);
+        return com_names[i];
+      }
 
   p = grub_malloc (sizeof (*p));
   if (p == NULL)
@@ -363,9 +373,12 @@ grub_serial_ns8250_add_mmio (grub_addr_t addr)
       return NULL;
     }
   p->driver = &grub_ns8250_driver;
-  grub_serial_config_defaults (p);
   p->mmio = true;
   p->mmio_base = addr;
+  if (config != NULL)
+    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 842322b1c..cdbbc54e3 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 5e008f0f5..a94327140 100644
--- a/include/grub/serial.h
+++ b/include/grub/serial.h
@@ -185,8 +185,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.34.1

[PATCH v3 6/8] ns8250: Use ACPI SPCR table when available to configure serial

[Benjamin Herrenschmidt]

From: Benjamin Herrenschmidt <benh@amazon.com>

"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>
---
 docs/grub.texi               |  9 ++++
 grub-core/Makefile.core.def  |  1 +
 grub-core/term/ns8250-spcr.c | 83 ++++++++++++++++++++++++++++++++++++
 grub-core/term/serial.c      | 21 ++++++---
 include/grub/serial.h        |  1 +
 5 files changed, 110 insertions(+), 5 deletions(-)
 create mode 100644 grub-core/term/ns8250-spcr.c

diff --git a/docs/grub.texi b/docs/grub.texi
index 50c811a88..fd22a69fa 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -2719,6 +2719,10 @@ you want to use COM2, you must specify @samp{--unit=1} instead. This
 command accepts many other options, so please refer to @ref{serial},
 for more details.
 
+Without argument or with @samp{--port=auto}, GRUB will attempt to use
+ACPI when available to auto-detect the default serial port and its
+configuration.
+
 The commands @command{terminal_input} (@pxref{terminal_input}) and
 @command{terminal_output} (@pxref{terminal_output}) choose which type of
 terminal you want to use. In the case above, the terminal will be a
@@ -4172,6 +4176,11 @@ be in the range 5-8 and stop bits must be 1 or 2. Default is 8 data
 bits and one stop bit. @var{parity} is one of @samp{no}, @samp{odd},
 @samp{even} and defaults to @samp{no}.
 
+If passed no @var{unit} nor @var{port}, or if @var{port} is set to
+@samp{auto} then GRUB will attempt to use ACPI to automatically detect
+the system default serial port and its configuration. If this information
+is not available, it will default to @var{unit} 0.
+
 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}).
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 95942fc8c..b656bdb44 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -2048,6 +2048,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..0b4417a30
--- /dev/null
+++ b/grub-core/term/ns8250-spcr.c
@@ -0,0 +1,83 @@
+/*
+ *  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/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 == NULL)
+    return NULL;
+  if (spcr->hdr.revision < 2)
+    return NULL;
+  if (spcr->intf_type != GRUB_ACPI_SPCR_INTF_TYPE_16550 &&
+      spcr->intf_type != GRUB_ACPI_SPCR_INTF_TYPE_16550X)
+    return NULL;
+  /* 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 NULL;
+  config.word_len = 8;
+  config.parity = GRUB_SERIAL_PARITY_NONE;
+  config.stop_bits = GRUB_SERIAL_STOP_BITS_1;
+  config.base_clock = UART_DEFAULT_BASE_CLOCK;
+  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 NULL;
+    };
+}
diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
index cdbbc54e3..d49261b48 100644
--- a/grub-core/term/serial.c
+++ b/grub-core/term/serial.c
@@ -157,12 +157,23 @@ grub_serial_find (const char *name)
     {
       name = grub_serial_ns8250_add_port (grub_strtoul (&name[sizeof ("port") - 1],
 							0, 16), NULL);
-      if (!name)
-	return NULL;
+      if (name == NULL)
+        return NULL;
 
       FOR_SERIAL_PORTS (port)
-	if (grub_strcmp (port->name, name) == 0)
-	  break;
+        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 == NULL)
+        name = "com0";
+
+      FOR_SERIAL_PORTS (port)
+        if (grub_strcmp (port->name, name) == 0)
+          break;
     }
 #endif
 
@@ -210,7 +221,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 a94327140..8d6ed56a3 100644
--- a/include/grub/serial.h
+++ b/include/grub/serial.h
@@ -185,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_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.34.1

[PATCH v3 7/8] ns8250: Support more MMIO access sizes

[Benjamin Herrenschmidt]

From: Benjamin Herrenschmidt <benh@amazon.com>

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      | 67 ++++++++++++++++++++++++++++++------
 include/grub/serial.h        | 10 ++++--
 3 files changed, 67 insertions(+), 13 deletions(-)

diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
index 0b4417a30..f4b718833 100644
--- a/grub-core/term/ns8250-spcr.c
+++ b/grub-core/term/ns8250-spcr.c
@@ -74,7 +74,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 d9d93fcf8..98f0b3bc3 100644
--- a/grub-core/term/ns8250.c
+++ b/grub-core/term/ns8250.c
@@ -42,16 +42,59 @@ ns8250_reg_read (struct grub_serial_port *port, grub_addr_t reg)
 {
   asm volatile("" : : : "memory");
   if (port->mmio)
-    return *((volatile grub_uint8_t *) (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:
+          /* ACPI tables occasionally uses "0" (legacy) as equivalent to "1" (byte) */
+        case 1:
+          return *((volatile grub_uint8_t *) (port->mmio_base + reg));
+        case 2:
+          return grub_le_to_cpu16 (*((volatile grub_uint16_t *) (port->mmio_base + (reg << 1))));
+        case 3:
+          return grub_le_to_cpu32 (*((volatile grub_uint32_t *) (port->mmio_base + (reg << 2))));
+        case 4:
+          /*
+           * This will only work properly on 64-bit systems since 64-bit
+           * accessors aren't atomic on 32-bit hardware. Thankfully the
+           * case of a UART with a 64-bit register spacing on 32-bit
+           * also probably doesn't exist.
+           */
+          return grub_le_to_cpu64 (*((volatile grub_uint64_t *) (port->mmio_base + (reg << 3))));
+        }
+    }
   return grub_inb (port->port + reg);
 }
 
 static void
-ns8250_reg_write (struct grub_serial_port *port, grub_uint8_t, grub_addr_t reg)
+ns8250_reg_write (struct grub_serial_port *port, grub_uint8_t value, grub_addr_t reg)
 {
   asm volatile("" : : : "memory");
   if (port->mmio)
-    *((volatile grub_uint8_t *) (port->mmio_base + reg)) = value;
+    {
+      switch(port->access_size)
+        {
+        default:
+          /* ACPI tables occasionally uses "0" (legacy) as equivalent to "1" (byte) */
+        case 1:
+          *((volatile grub_uint8_t *) (port->mmio_base + reg)) = value;
+          break;
+        case 2:
+          *((volatile grub_uint16_t *) (port->mmio_base + (reg << 1))) = grub_cpu_to_le16 (value);
+          break;
+        case 3:
+          *((volatile grub_uint32_t *) (port->mmio_base + (reg << 2))) = grub_cpu_to_le32 (value);
+          break;
+        case 4:
+          /* See commment in ns8250_reg_read() */
+          *((volatile grub_uint64_t *) (port->mmio_base + (reg << 3))) = grub_cpu_to_le64 (value);
+          break;
+        }
+    }
   else
     grub_outb (value, port->port + reg);
 }
@@ -286,6 +329,7 @@ grub_ns8250_init (void)
 	  grub_print_error ();
 
 	grub_serial_register (&com_ports[i]);
+	com_ports[i].access_size = 1;
       }
 }
 
@@ -312,12 +356,12 @@ grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config *config
   for (i = 0; i < GRUB_SERIAL_PORT_NUM; i++)
     if (com_ports[i].port == port)
       {
-	if (dead_ports & (1 << i))
-	  return NULL;
-	/* give the opportunity for SPCR to configure a default com port */
-	if (config != NULL)
-	  grub_serial_port_configure (&com_ports[i], config);
-	return com_names[i];
+        if (dead_ports & (1 << i))
+          return NULL;
+        /* give the opportunity for SPCR to configure a default com port */
+        if (config != NULL)
+          grub_serial_port_configure (&com_ports[i], config);
+        return com_names[i];
       }
 
   grub_outb (0x5a, port + UART_SR);
@@ -340,6 +384,7 @@ grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config *config
   p->driver = &grub_ns8250_driver;
   p->mmio = false;
   p->port = port;
+  p->access_size = 1;
   if (config != NULL)
     grub_serial_port_configure (p, config);
   else
@@ -350,7 +395,8 @@ 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;
@@ -375,6 +421,7 @@ grub_serial_ns8250_add_mmio (grub_addr_t addr, struct grub_serial_config *config
   p->driver = &grub_ns8250_driver;
   p->mmio = true;
   p->mmio_base = addr;
+  p->access_size = acc_size;
   if (config != NULL)
     grub_serial_port_configure (p, config);
   else
diff --git a/include/grub/serial.h b/include/grub/serial.h
index 8d6ed56a3..65ccab4ff 100644
--- a/include/grub/serial.h
+++ b/include/grub/serial.h
@@ -94,7 +94,12 @@ struct grub_serial_port
 #if defined(__mips__) || defined (__i386__) || defined (__x86_64__)
         grub_port_t port;
 #endif
-        grub_addr_t mmio_base;
+        struct
+        {
+          grub_addr_t mmio_base;
+          /* Access size uses ACPI definition */
+          grub_uint8_t access_size;
+        };
       };
     };
     struct
@@ -187,7 +192,8 @@ 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);
-- 
2.34.1

[PATCH v3 8/8] serial: Add ability to specify MMIO ports via 'serial'

[Benjamin Herrenschmidt]

This adds the ability to explicitely add an MMIO based serial port
via the 'serial' command. The syntax is:

serial --port=mmio,<hex_address>{.b,.w,.l,.q}

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

# Conflicts:
#	grub-core/term/serial.c
---
 docs/grub.texi          | 26 +++++++++++++--
 grub-core/term/serial.c | 71 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 93 insertions(+), 4 deletions(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index fd22a69fa..502ca2ef7 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -4168,8 +4168,24 @@ Commands usable anywhere in the menu and in the command-line.
 @deffn Command serial [@option{--unit=unit}] [@option{--port=port}] [@option{--speed=speed}] [@option{--word=word}] [@option{--parity=parity}] [@option{--stop=stop}]
 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}.
+the port often called COM1.
+
+@var{port} is the I/O port where the UART is to be found or, if prefixed
+with @samp{mmio,}, the MMIO address of the UART. If specified it takes
+precedence over @var{unit}.
+
+Additionally, an MMIO address can be suffixed with:
+@itemize @bullet
+@item
+@samp{.b} for bytes access (default)
+@item
+@samp{.w} for 16-bit word access
+@item
+@samp{.l} for 32-bit long word access or
+@item
+@samp{.q} for 64-bit long long word access
+@end itemize
+
 @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
@@ -4185,6 +4201,12 @@ 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}).
 
+Examples:
+@example
+serial --port=3f8 --speed=9600
+serial --port=mmio,fefb0000.l --speed=115200
+@end example
+
 See also @ref{Serial terminal}.
 @end deffn
 
diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
index d49261b48..d161e1d36 100644
--- a/grub-core/term/serial.c
+++ b/grub-core/term/serial.c
@@ -164,6 +164,70 @@ grub_serial_find (const char *name)
         if (grub_strcmp (port->name, name) == 0)
            break;
     }
+  if (!port && grub_strncmp (name, "mmio,", sizeof ("mmio,") - 1) == 0
+      && grub_isxdigit (name [sizeof ("mmio,") - 1]))
+    {
+      const char *p1, *p = &name[sizeof ("mmio,") - 1];
+      grub_addr_t addr = grub_strtoul (p, &p1, 16);
+      unsigned int acc_size = 1;
+      unsigned int nlen = p1 - p;
+
+      /*
+       * If we reach here, we know there's a digit after "mmio,", so
+       * all we need to check is the validity of the character following
+       * the number, which should be a termination, or a dot followed by
+       * an access size
+       */
+      if (p1[0] != '\0' && p1[0] != '.')
+        {
+          grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Incorrect MMIO address syntax"));
+          return NULL;
+        }
+      if (p1[0] == '.')
+        switch(p1[1])
+          {
+          case 'w':
+            acc_size = 2;
+            break;
+          case 'l':
+            acc_size = 3;
+            break;
+          case 'q':
+            acc_size = 4;
+            break;
+          case 'b':
+            acc_size = 1;
+            break;
+          default:
+            /*
+             * Should we abort for an unknown size ? Let's just default
+             * to 1 byte, it would increase the chance that the user who
+             * did a typo can actually see the console.
+             */
+            grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Incorrect MMIO access size"));
+          }
+
+      /*
+       * Specifying the access size is optional an grub_serial_ns8250_add_mmio()
+       * will not add it to the name. So the original loop trying to match an
+       * existing port above might have missed this one. Let's do another
+       * search ignoring the access size part of the string. At this point
+       * nlen contains the length of the name up to the end of the address.
+       */
+      FOR_SERIAL_PORTS (port)
+        if (grub_strncmp (port->name, name, nlen) == 0) {
+          break;
+        }
+
+      name = grub_serial_ns8250_add_mmio (addr, acc_size, NULL);
+      if (name == NULL)
+        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 */
@@ -212,8 +276,11 @@ grub_cmd_serial (grub_extcmd_context_t ctxt, int argc, char **args)
 
   if (state[OPTION_PORT].set)
     {
-      grub_snprintf (pname, sizeof (pname), "port%lx",
-		     grub_strtoul (state[1].arg, 0, 0));
+      if (grub_strncmp (state[OPTION_PORT].arg, "mmio,", sizeof ("mmio,") - 1) == 0)
+          grub_snprintf (pname, sizeof (pname), "%s", state[1].arg);
+      else
+          grub_snprintf (pname, sizeof (pname), "port%lx",
+                         grub_strtoul (state[1].arg, 0, 0));
       name = pname;
     }
 
-- 
2.34.1

Re: [PATCH v2 0/8] serial: Add MMIO & SPCR support

[Daniel Kiper]

On Fri, Dec 23, 2022 at 12:47:51PM +1100, Benjamin Herrenschmidt wrote:
> This series adds support for MMIO serial ports and auto-configuration
> via ACPI SPCR.
>
> This is necessary for the serial port to work on AWS EC2 "metal" x86
> systems.
>
> An MMIO serial port can be specified explicitely using the
> "mmio," prefix for the --port argument to the 'serial' command,
> the register access size can also be specified via a suffix,
> and when 'serial' is used without arguments, it will try to use
> an ACPI SPCR entry (if any) to add the default port and configure
> it appropriately (speed, parity etc...)
>
> 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 and to create a SPCR ACPI table.
>
> The insipiration was an original implementation by
> Matthias Lange ! matthias.lange at kernkonzept.com
> However, the code was rewritten pretty much from scratch.

For all the patches Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...

Thank you for adding this feature!

Daniel

Re: [PATCH v2 0/8] serial: Add MMIO & SPCR support

[Daniel Kiper]

On Thu, Jan 12, 2023 at 01:51:56PM +0100, Daniel Kiper wrote:
> On Fri, Dec 23, 2022 at 12:47:51PM +1100, Benjamin Herrenschmidt wrote:
> > This series adds support for MMIO serial ports and auto-configuration
> > via ACPI SPCR.
> >
> > This is necessary for the serial port to work on AWS EC2 "metal" x86
> > systems.
> >
> > An MMIO serial port can be specified explicitely using the
> > "mmio," prefix for the --port argument to the 'serial' command,
> > the register access size can also be specified via a suffix,
> > and when 'serial' is used without arguments, it will try to use
> > an ACPI SPCR entry (if any) to add the default port and configure
> > it appropriately (speed, parity etc...)
> >
> > 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 and to create a SPCR ACPI table.
> >
> > The insipiration was an original implementation by
> > Matthias Lange ! matthias.lange at kernkonzept.com
> > However, the code was rewritten pretty much from scratch.
>
> For all the patches Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...

Sadly your patch set broke various builds including x86-ieee1275 one. I had
to do some tweaks to make it work again. You can find most important ones
below. Please double check I have not messed up something.

Daniel

diff --git a/grub-core/term/ieee1275/serial.c b/grub-core/term/ieee1275/serial.c
index afbe8dcda..0e4cac4c4 100644
--- a/grub-core/term/ieee1275/serial.c
+++ b/grub-core/term/ieee1275/serial.c
@@ -228,7 +228,7 @@ add_port (struct ofserial_hash_ent *ent)
     return NULL;
 
   FOR_SERIAL_PORTS (port)
-    if (port->ent == ent)
+    if (port->elem == ent)
       return port;
 
   port = grub_zalloc (sizeof (*port));
@@ -252,7 +252,7 @@ add_port (struct ofserial_hash_ent *ent)
   return port;
 }
 
-const struct grub_serial_port *
+struct grub_serial_port *
 grub_ofserial_add_port (const char *path)
 {
   struct ofserial_hash_ent *ent;
diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
index 01533d969..d101bffb5 100644
--- a/grub-core/term/ns8250-spcr.c
+++ b/grub-core/term/ns8250-spcr.c
@@ -16,6 +16,8 @@
  *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#if !defined(GRUB_MACHINE_IEEE1275) && !defined(GRUB_MACHINE_QEMU)
+
 #include <grub/serial.h>
 #include <grub/ns8250.h>
 #include <grub/types.h>
@@ -82,3 +84,5 @@ grub_ns8250_spcr_init (void)
         return NULL;
     };
 }
+
+#endif
diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
index 81abd570c..c65ffc63c 100644
--- a/grub-core/term/serial.c
+++ b/grub-core/term/serial.c
@@ -209,6 +209,8 @@ grub_serial_find (const char *name)
       if (port != NULL)
         return port;
     }
+
+#if (defined(__i386__) || defined(__x86_64__)) && !defined(GRUB_MACHINE_IEEE1275) && !defined(GRUB_MACHINE_QEMU)
   if (grub_strcmp (name, "auto") == 0)
     {
       /* Look for an SPCR if any. If not, default to com0. */
@@ -220,6 +222,7 @@ grub_serial_find (const char *name)
           return port;
     }
 #endif
+#endif
 
 #ifdef GRUB_MACHINE_IEEE1275
   if (grub_strncmp (name, "ieee1275/", sizeof ("ieee1275/") - 1) == 0)
diff --git a/include/grub/ieee1275/console.h b/include/grub/ieee1275/console.h
index bdd98fe06..a8094d381 100644
--- a/include/grub/ieee1275/console.h
+++ b/include/grub/ieee1275/console.h
@@ -28,7 +28,7 @@ void grub_console_init_lately (void);
 /* Finish the console system.  */
 void grub_console_fini (void);
 
-const char *
+struct grub_serial_port *
 grub_ofserial_add_port (const char *name);
 
 #endif /* ! GRUB_CONSOLE_MACHINE_HEADER */

Re: [PATCH v2 0/8] serial: Add MMIO & SPCR support

[Benjamin Herrenschmidt]

On Thu, 2023-01-19 at 18:31 +0100, Daniel Kiper wrote:
> 
> Sadly your patch set broke various builds including x86-ieee1275 one. I had
> to do some tweaks to make it work again. You can find most important ones
> below. Please double check I have not messed up something.

Oh, sorry about that. I didn't know there even was such a thing as x86-
ieee1275. Is there an easy way to run / test all those builds ? A
script or service somewhere ?

> Daniel
> 
> diff --git a/grub-core/term/ieee1275/serial.c b/grub-core/term/ieee1275/serial.c
> index afbe8dcda..0e4cac4c4 100644
> --- a/grub-core/term/ieee1275/serial.c
> +++ b/grub-core/term/ieee1275/serial.c
> @@ -228,7 +228,7 @@ add_port (struct ofserial_hash_ent *ent)
>      return NULL;
>  
>    FOR_SERIAL_PORTS (port)
> -    if (port->ent == ent)
> +    if (port->elem == ent)
>        return port;

Right.

>    port = grub_zalloc (sizeof (*port));
> @@ -252,7 +252,7 @@ add_port (struct ofserial_hash_ent *ent)
>    return port;
>  }
>  
> -const struct grub_serial_port *
> +struct grub_serial_port *
>  grub_ofserial_add_port (const char *path)
>  {
>    struct ofserial_hash_ent *ent;
> diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
> index 01533d969..d101bffb5 100644
> --- a/grub-core/term/ns8250-spcr.c
> +++ b/grub-core/term/ns8250-spcr.c
> @@ -16,6 +16,8 @@
>   *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#if !defined(GRUB_MACHINE_IEEE1275) && !defined(GRUB_MACHINE_QEMU)
> +
>  #include <grub/serial.h>
>  #include <grub/ns8250.h>
>  #include <grub/types.h>
> @@ -82,3 +84,5 @@ grub_ns8250_spcr_init (void)
>          return NULL;
>      };
>  }

This is the rule for ACPI ? Or is something else problematic ? No
particular objection here, just wondering.

> +
> +#endif
> diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
> index 81abd570c..c65ffc63c 100644
> --- a/grub-core/term/serial.c
> +++ b/grub-core/term/serial.c
> @@ -209,6 +209,8 @@ grub_serial_find (const char *name)
>        if (port != NULL)
>          return port;
>      }
> +
> +#if (defined(__i386__) || defined(__x86_64__)) && !defined(GRUB_MACHINE_IEEE1275) && !defined(GRUB_MACHINE_QEMU)

Ok.

>    if (grub_strcmp (name, "auto") == 0)
>      {
>        /* Look for an SPCR if any. If not, default to com0. */
> @@ -220,6 +222,7 @@ grub_serial_find (const char *name)
>            return port;
>      }
>  #endif
> +#endif
>  
>  #ifdef GRUB_MACHINE_IEEE1275
>    if (grub_strncmp (name, "ieee1275/", sizeof ("ieee1275/") - 1) == 0)
> diff --git a/include/grub/ieee1275/console.h b/include/grub/ieee1275/console.h
> index bdd98fe06..a8094d381 100644
> --- a/include/grub/ieee1275/console.h
> +++ b/include/grub/ieee1275/console.h
> @@ -28,7 +28,7 @@ void grub_console_init_lately (void);
>  /* Finish the console system.  */
>  void grub_console_fini (void);
>  
> -const char *
> +struct grub_serial_port *
>  grub_ofserial_add_port (const char *name);
> 
Yeah, I didn't try building ieee1275, my fault. Sorry about that. Your
changes seem fine to me.

Cheers,
Ben.

Re: [PATCH v2 0/8] serial: Add MMIO & SPCR support

[Daniel Kiper]

On Fri, Jan 20, 2023 at 05:15:48PM +1100, Benjamin Herrenschmidt wrote:
> On Thu, 2023-01-19 at 18:31 +0100, Daniel Kiper wrote:
> >
> > Sadly your patch set broke various builds including x86-ieee1275 one. I had
> > to do some tweaks to make it work again. You can find most important ones
> > below. Please double check I have not messed up something.
>
> Oh, sorry about that. I didn't know there even was such a thing as x86-
> ieee1275. Is there an easy way to run / test all those builds ? A

Yeah, I was surprised too when I saw it first time... :-)

> script or service somewhere ?

./configure --target=i386 --with-platform=ieee1275 ...

Sadly we do not have any script yet. However, the INSTALL file has quite
good description how to setup a cross-platform build environment.

> > diff --git a/grub-core/term/ieee1275/serial.c b/grub-core/term/ieee1275/serial.c
> > index afbe8dcda..0e4cac4c4 100644
> > --- a/grub-core/term/ieee1275/serial.c
> > +++ b/grub-core/term/ieee1275/serial.c
> > @@ -228,7 +228,7 @@ add_port (struct ofserial_hash_ent *ent)
> >      return NULL;
> >  
> >    FOR_SERIAL_PORTS (port)
> > -    if (port->ent == ent)
> > +    if (port->elem == ent)
> >        return port;
>
> Right.
>
> >    port = grub_zalloc (sizeof (*port));
> > @@ -252,7 +252,7 @@ add_port (struct ofserial_hash_ent *ent)
> >    return port;
> >  }
> >  
> > -const struct grub_serial_port *
> > +struct grub_serial_port *
> >  grub_ofserial_add_port (const char *path)
> >  {
> >    struct ofserial_hash_ent *ent;
> > diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
> > index 01533d969..d101bffb5 100644
> > --- a/grub-core/term/ns8250-spcr.c
> > +++ b/grub-core/term/ns8250-spcr.c
> > @@ -16,6 +16,8 @@
> >   *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> >   */
> >  
> > +#if !defined(GRUB_MACHINE_IEEE1275) && !defined(GRUB_MACHINE_QEMU)
> > +
> >  #include <grub/serial.h>
> >  #include <grub/ns8250.h>
> >  #include <grub/types.h>
> > @@ -82,3 +84,5 @@ grub_ns8250_spcr_init (void)
> >          return NULL;
> >      };
> >  }
>
> This is the rule for ACPI ? Or is something else problematic ? No
> particular objection here, just wondering.

IIRC this is due to lack of support for ACPI on some platforms.

> > +
> > +#endif
> > diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
> > index 81abd570c..c65ffc63c 100644
> > --- a/grub-core/term/serial.c
> > +++ b/grub-core/term/serial.c
> > @@ -209,6 +209,8 @@ grub_serial_find (const char *name)
> >        if (port != NULL)
> >          return port;
> >      }
> > +
> > +#if (defined(__i386__) || defined(__x86_64__)) && !defined(GRUB_MACHINE_IEEE1275) && !defined(GRUB_MACHINE_QEMU)
>
> Ok.
>
> >    if (grub_strcmp (name, "auto") == 0)
> >      {
> >        /* Look for an SPCR if any. If not, default to com0. */
> > @@ -220,6 +222,7 @@ grub_serial_find (const char *name)
> >            return port;
> >      }
> >  #endif
> > +#endif
> >  
> >  #ifdef GRUB_MACHINE_IEEE1275
> >    if (grub_strncmp (name, "ieee1275/", sizeof ("ieee1275/") - 1) == 0)
> > diff --git a/include/grub/ieee1275/console.h b/include/grub/ieee1275/console.h
> > index bdd98fe06..a8094d381 100644
> > --- a/include/grub/ieee1275/console.h
> > +++ b/include/grub/ieee1275/console.h
> > @@ -28,7 +28,7 @@ void grub_console_init_lately (void);
> >  /* Finish the console system.  */
> >  void grub_console_fini (void);
> >  
> > -const char *
> > +struct grub_serial_port *
> >  grub_ofserial_add_port (const char *name);
> >
> Yeah, I didn't try building ieee1275, my fault. Sorry about that. Your
> changes seem fine to me.

No problem. Thanks a lot for checking this.

Daniel

[PATCH v2 0/8] serial: Add MMIO & SPCR support

[Benjamin Herrenschmidt]

This series adds support for MMIO serial ports and auto-configuration
via ACPI SPCR.

This is necessary for the serial port to work on AWS EC2 "metal" x86
systems.

An MMIO serial port can be specified explicitely using the
"mmio," prefix for the --port argument to the 'serial' command,
the register access size can also be specified via a suffix,
and when 'serial' is used without arguments, it will try to use
an ACPI SPCR entry (if any) to add the default port and configure
it appropriately (speed, parity etc...)

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 and to create a SPCR ACPI table.

The insipiration was an original implementation by
Matthias Lange ! matthias.lange at kernkonzept.com
However, the code was rewritten pretty much from scratch.

v2: This version (hopefully) addresses all the review commments and
has been rebased on the latest master branch.