On Fri, Dec 02, 2022 at 10:05:22AM +1100, Benjamin Herrenschmidt wrote:
> From: Benjamin Herrenschmidt <b...@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.
>
> Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
> ---
> grub-core/term/ns8250.c | 78 ++++++++++++++++++++++++++++++++---------
> grub-core/term/serial.c | 8 +++--
> include/grub/serial.h | 11 +++++-
> 3 files changed, 78 insertions(+), 19 deletions(-)
>
> diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
> index 83b25990c..b640508d0 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 inline unsigned char
Please drop inline from here. Then compiler will be free to optimize out
or not this function.
> +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
Ditto...
> +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;
Casts require space like this: (volatile char *) (port->mmio_base + reg).
Please fix casts in other places/patches too.
> + 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 = 0;
> err = grub_serial_config_defaults (&com_ports[i]);
> if (err)
> grub_print_error ();
> @@ -316,8 +334,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;
Please add en empty line here.
> + 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)
I prefer "p == NULL" instead of "!p". If you could fix that here and in
the other places/patches that will be nice.
> + 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 842322b1c..c8f5f02db 100644
> --- a/grub-core/term/serial.c
> +++ b/grub-core/term/serial.c
> @@ -201,8 +201,12 @@ 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_memcmp (state[OPTION_PORT].arg, "mmio", 4) == 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;
> }
>
> diff --git a/include/grub/serial.h b/include/grub/serial.h
> index 67379de45..ccf464d41 100644
> --- a/include/grub/serial.h
> +++ b/include/grub/serial.h
> @@ -86,9 +86,17 @@ struct grub_serial_port
> */
> union
> {
> + struct
> + {
> + int mmio;
Could you use bool here and true/false in the code?
> + 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);
Please do not drop space before "(". I saw similar mistakes in the other
patches.
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
