On Fri, Dec 02, 2022 at 10:05:25AM +1100, Benjamin Herrenschmidt wrote:
> From: Benjamin Herrenschmidt <b...@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 <b...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
