Re: [PATCH 6/7] ns8250: Support more MMIO access sizes

[Daniel Kiper]

On Fri, Dec 02, 2022 at 10:05:25AM +1100, Benjamin Herrenschmidt wrote:
> 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      | 45 +++++++++++++++++++++++++++++++++---
>  include/grub/serial.h        |  9 ++++++--
>  3 files changed, 51 insertions(+), 6 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 08dfb99da..76f68c037 100644
> --- a/grub-core/term/ns8250.c
> +++ b/grub-core/term/ns8250.c
> @@ -48,7 +48,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));

s/unsigned char/grub_uint8_t/a

And now I think ns8250_reg_read()/ns8250_reg_write() in patch #3 probably
should use grub_uint8_t instead of "unsigned char" everywhere...

> +        case 2:
> +          return grub_le_to_cpu16(*((volatile unsigned short 
> *)(port->mmio_base + (reg << 1))));

s/unsigned short/grub_uint16_t/

> +        case 3:
> +          return grub_le_to_cpu32(*((volatile unsigned long 
> *)(port->mmio_base + (reg << 2))));

s/unsigned long/grub_uint32_t/

I do not mention cast coding style issue...

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

s/unsigned long long/grub_uint64_t/

Then maybe comment after "case 4" can be dropped...

> +        }
> +    }
>    return grub_inb (port->port + reg);
>  }
>
> @@ -57,7 +76,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;

s/unsigned char/grub_uint8_t/a

And please fix the similar problems accordingly in this and other patches...

> +          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);
>  }
> @@ -285,6 +321,7 @@ grub_ns8250_init (void)
>         grub_print_error ();
>
>       grub_serial_register (&com_ports[i]);
> +     com_ports[i].access_size = 1;
>        }
>  }
>
> @@ -338,6 +375,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
> @@ -348,7 +386,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;
> @@ -372,6 +410,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 646bdf1bb..8875eaf82 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 */
> +          unsigned int access_size;

Is this 32-bit value? If yes then s/unsigned int/grub_uint32_t/...

> +        };
>        };
>      };
>      struct
> @@ -187,7 +192,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);

Please fix coding style...

Daniel