| Message ID | 20230607092727.19654-4-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: pl011: Use correct accessors | expand |
Hi Michal, > -----Original Message----- > Subject: [PATCH 3/4] xen/arm: pl011: Use correct accessors > > At the moment, we use 32-bit only accessors (i.e. readl/writel) to match > the SBSA v2.x requirement. This should not be the default case for normal > PL011 where accesses shall be 8/16-bit (max register size is 16-bit). > There are however implementations of this UART that can only handle 32-bit > MMIO. This is advertised by dt property "reg-io-width" set to 4. > > Introduce new struct pl011 member mmio32 and replace pl011_{read/write} > macros with static inline helpers that use 32-bit or 16-bit accessors > (largest-common not to end up using different ones depending on the actual > register size) according to mmio32 value. By default this property is set > to false, unless: > - reg-io-width is specified with value 4, > - SBSA UART is in use. > > For now, no changes done for ACPI due to lack of testing possibilities > (i.e. current behavior maintained resulting in 32-bit accesses). > > Signed-off-by: Michal Orzel <michal.orzel@amd.com> I've tested this patch on top of today's staging on FVP arm32 and arm64 and confirm this patch will not break existing functionality. So: Tested-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry
On Wed, 7 Jun 2023, Michal Orzel wrote: > At the moment, we use 32-bit only accessors (i.e. readl/writel) to match > the SBSA v2.x requirement. This should not be the default case for normal > PL011 where accesses shall be 8/16-bit (max register size is 16-bit). > There are however implementations of this UART that can only handle 32-bit > MMIO. This is advertised by dt property "reg-io-width" set to 4. > > Introduce new struct pl011 member mmio32 and replace pl011_{read/write} > macros with static inline helpers that use 32-bit or 16-bit accessors > (largest-common not to end up using different ones depending on the actual > register size) according to mmio32 value. By default this property is set > to false, unless: > - reg-io-width is specified with value 4, > - SBSA UART is in use. > > For now, no changes done for ACPI due to lack of testing possibilities > (i.e. current behavior maintained resulting in 32-bit accesses). > > Signed-off-by: Michal Orzel <michal.orzel@amd.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/drivers/char/pl011.c | 53 +++++++++++++++++++++++++++++++++++----- > 1 file changed, 47 insertions(+), 6 deletions(-) > > diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c > index 052a6512515c..403b1ac06551 100644 > --- a/xen/drivers/char/pl011.c > +++ b/xen/drivers/char/pl011.c > @@ -41,6 +41,7 @@ static struct pl011 { > /* unsigned int timeout_ms; */ > /* bool_t probing, intr_works; */ > bool sbsa; /* ARM SBSA generic interface */ > + bool mmio32; /* 32-bit only MMIO */ > } pl011_com = {0}; > > /* These parity settings can be ORed directly into the LCR. */ > @@ -50,9 +51,30 @@ static struct pl011 { > #define PARITY_MARK (PEN|SPS) > #define PARITY_SPACE (PEN|EPS|SPS) > > -/* SBSA v2.x document requires, all reads/writes must be 32-bit accesses */ > -#define pl011_read(uart, off) readl((uart)->regs + (off)) > -#define pl011_write(uart, off,val) writel((val), (uart)->regs + (off)) > +/* > + * By default, PL011 accesses shall be done using 8/16-bit accessors to > + * support legacy devices that cannot cope with 32-bit. On the other hand, > + * there are implementations of PL011 that can only handle 32-bit MMIO. Also, > + * SBSA v2.x requires 32-bit accesses. Note that for default case, we use > + * largest-common accessors (i.e. 16-bit) not to end up using different ones > + * depending on the actual register size. > + */ > +static inline void > +pl011_write(struct pl011 *uart, unsigned int offset, unsigned int val) > +{ > + if ( uart->mmio32 ) > + writel(val, uart->regs + offset); > + else > + writew(val, uart->regs + offset); > +} > + > +static inline unsigned int pl011_read(struct pl011 *uart, unsigned int offset) > +{ > + if ( uart->mmio32 ) > + return readl(uart->regs + offset); > + > + return readw(uart->regs + offset); > +} > > static unsigned int pl011_intr_status(struct pl011 *uart) > { > @@ -222,7 +244,8 @@ static struct uart_driver __read_mostly pl011_driver = { > .vuart_info = pl011_vuart, > }; > > -static int __init pl011_uart_init(int irq, paddr_t addr, paddr_t size, bool sbsa) > +static int __init > +pl011_uart_init(int irq, paddr_t addr, paddr_t size, bool sbsa, bool mmio32) > { > struct pl011 *uart; > > @@ -233,6 +256,9 @@ static int __init pl011_uart_init(int irq, paddr_t addr, paddr_t size, bool sbsa > uart->stop_bits = 1; > uart->sbsa = sbsa; > > + /* Set 32-bit MMIO also for SBSA since v2.x requires it */ > + uart->mmio32 = (mmio32 || sbsa); > + > uart->regs = ioremap_nocache(addr, size); > if ( !uart->regs ) > { > @@ -259,6 +285,8 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev, > const char *config = data; > int res; > paddr_t addr, size; > + uint32_t io_width; > + bool mmio32 = false; > > if ( strcmp(config, "") ) > { > @@ -280,7 +308,19 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev, > return -EINVAL; > } > > - res = pl011_uart_init(res, addr, size, false); > + /* See linux Documentation/devicetree/bindings/serial/pl011.yaml */ > + if ( dt_property_read_u32(dev, "reg-io-width", &io_width) ) > + { > + if ( io_width == 4 ) > + mmio32 = true; > + else if ( io_width != 1 ) > + { > + printk("pl011: Unsupported reg-io-width (%"PRIu32")\n", io_width); > + return -EINVAL; > + } > + } > + > + res = pl011_uart_init(res, addr, size, false, mmio32); > if ( res < 0 ) > { > printk("pl011: Unable to initialize\n"); > @@ -328,8 +368,9 @@ static int __init pl011_acpi_uart_init(const void *data) > /* trigger/polarity information is not available in spcr */ > irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH); > > + /* TODO - mmio32 proper handling (for now set to true) */ > res = pl011_uart_init(spcr->interrupt, spcr->serial_port.address, > - PAGE_SIZE, sbsa); > + PAGE_SIZE, sbsa, true); > if ( res < 0 ) > { > printk("pl011: Unable to initialize\n"); > -- > 2.25.1 >
diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c index 052a6512515c..403b1ac06551 100644 --- a/xen/drivers/char/pl011.c +++ b/xen/drivers/char/pl011.c @@ -41,6 +41,7 @@ static struct pl011 { /* unsigned int timeout_ms; */ /* bool_t probing, intr_works; */ bool sbsa; /* ARM SBSA generic interface */ + bool mmio32; /* 32-bit only MMIO */ } pl011_com = {0}; /* These parity settings can be ORed directly into the LCR. */ @@ -50,9 +51,30 @@ static struct pl011 { #define PARITY_MARK (PEN|SPS) #define PARITY_SPACE (PEN|EPS|SPS) -/* SBSA v2.x document requires, all reads/writes must be 32-bit accesses */ -#define pl011_read(uart, off) readl((uart)->regs + (off)) -#define pl011_write(uart, off,val) writel((val), (uart)->regs + (off)) +/* + * By default, PL011 accesses shall be done using 8/16-bit accessors to + * support legacy devices that cannot cope with 32-bit. On the other hand, + * there are implementations of PL011 that can only handle 32-bit MMIO. Also, + * SBSA v2.x requires 32-bit accesses. Note that for default case, we use + * largest-common accessors (i.e. 16-bit) not to end up using different ones + * depending on the actual register size. + */ +static inline void +pl011_write(struct pl011 *uart, unsigned int offset, unsigned int val) +{ + if ( uart->mmio32 ) + writel(val, uart->regs + offset); + else + writew(val, uart->regs + offset); +} + +static inline unsigned int pl011_read(struct pl011 *uart, unsigned int offset) +{ + if ( uart->mmio32 ) + return readl(uart->regs + offset); + + return readw(uart->regs + offset); +} static unsigned int pl011_intr_status(struct pl011 *uart) { @@ -222,7 +244,8 @@ static struct uart_driver __read_mostly pl011_driver = { .vuart_info = pl011_vuart, }; -static int __init pl011_uart_init(int irq, paddr_t addr, paddr_t size, bool sbsa) +static int __init +pl011_uart_init(int irq, paddr_t addr, paddr_t size, bool sbsa, bool mmio32) { struct pl011 *uart; @@ -233,6 +256,9 @@ static int __init pl011_uart_init(int irq, paddr_t addr, paddr_t size, bool sbsa uart->stop_bits = 1; uart->sbsa = sbsa; + /* Set 32-bit MMIO also for SBSA since v2.x requires it */ + uart->mmio32 = (mmio32 || sbsa); + uart->regs = ioremap_nocache(addr, size); if ( !uart->regs ) { @@ -259,6 +285,8 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev, const char *config = data; int res; paddr_t addr, size; + uint32_t io_width; + bool mmio32 = false; if ( strcmp(config, "") ) { @@ -280,7 +308,19 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev, return -EINVAL; } - res = pl011_uart_init(res, addr, size, false); + /* See linux Documentation/devicetree/bindings/serial/pl011.yaml */ + if ( dt_property_read_u32(dev, "reg-io-width", &io_width) ) + { + if ( io_width == 4 ) + mmio32 = true; + else if ( io_width != 1 ) + { + printk("pl011: Unsupported reg-io-width (%"PRIu32")\n", io_width); + return -EINVAL; + } + } + + res = pl011_uart_init(res, addr, size, false, mmio32); if ( res < 0 ) { printk("pl011: Unable to initialize\n"); @@ -328,8 +368,9 @@ static int __init pl011_acpi_uart_init(const void *data) /* trigger/polarity information is not available in spcr */ irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH); + /* TODO - mmio32 proper handling (for now set to true) */ res = pl011_uart_init(spcr->interrupt, spcr->serial_port.address, - PAGE_SIZE, sbsa); + PAGE_SIZE, sbsa, true); if ( res < 0 ) { printk("pl011: Unable to initialize\n");
At the moment, we use 32-bit only accessors (i.e. readl/writel) to match the SBSA v2.x requirement. This should not be the default case for normal PL011 where accesses shall be 8/16-bit (max register size is 16-bit). There are however implementations of this UART that can only handle 32-bit MMIO. This is advertised by dt property "reg-io-width" set to 4. Introduce new struct pl011 member mmio32 and replace pl011_{read/write} macros with static inline helpers that use 32-bit or 16-bit accessors (largest-common not to end up using different ones depending on the actual register size) according to mmio32 value. By default this property is set to false, unless: - reg-io-width is specified with value 4, - SBSA UART is in use. For now, no changes done for ACPI due to lack of testing possibilities (i.e. current behavior maintained resulting in 32-bit accesses). Signed-off-by: Michal Orzel <michal.orzel@amd.com> --- xen/drivers/char/pl011.c | 53 +++++++++++++++++++++++++++++++++++----- 1 file changed, 47 insertions(+), 6 deletions(-)