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(-)

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

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
>

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