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);