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