[PATCH 0/4] xen/arm: pl011: Use correct accessors

[PATCH 0/4] xen/arm: pl011: Use correct accessors

[Michal Orzel]

At the moment we have a real mix of accessors used for PL011 registers across
early printk and runtime driver resulting in a) not spec compliant behavior
and b) inconsistency. This series:
 - switches to use the 8/16-bit accessors in normal case,
 - adds support to use 32-bit only accessors.

The behavior is now the same as in Linux.

The last patch adding support for SBSA UART in dt boot was added to make
the merge process easier. Also, arm,sbsa-uart property is usually present
next to arm,pl011 which would casue the pl011 driver to believe it is driving
PL011 and not SBSA, therefore resulting in accessing the registers that shall
not be touched (as oppose to just failing).

Discussion:
https://lore.kernel.org/xen-devel/b31a9f06-1ad8-b882-2fb0-84a84a1accb8@xen.org/T/#

Michal Orzel (4):
  xen/arm: debug-pl011: Use correct accessors
  xen/arm: debug-pl011: Add support for 32-bit only MMIO
  xen/arm: pl011: Use correct accessors
  xen/arm: pl011: Add SBSA UART device-tree support

 docs/misc/arm/early-printk.txt        |  3 ++
 xen/arch/arm/Kconfig.debug            |  7 ++++
 xen/arch/arm/arm32/debug-pl011.inc    | 12 +++---
 xen/arch/arm/arm64/debug-pl011.inc    | 12 +++---
 xen/arch/arm/include/asm/pl011-uart.h | 19 +++++++++
 xen/drivers/char/pl011.c              | 57 ++++++++++++++++++++++++---
 6 files changed, 92 insertions(+), 18 deletions(-)

-- 
2.25.1

[PATCH 1/4] xen/arm: debug-pl011: Use correct accessors

[Michal Orzel]

Although most PL011 UARTs can cope with 32-bit accesses, some of the old
legacy ones might not. PL011 registers are 8/16-bit wide and this shall
be perceived as the normal behavior.

Modify early printk pl011 code for arm32/arm64 to use the correct
accessors depending on the register size (refer ARM DDI 0183G, Table 3.1).

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
Next patch will override strX,ldrX with macros but I prefer to keep the
history clean (+ possibiltity for a backport if needed).
---
 xen/arch/arm/arm32/debug-pl011.inc | 12 ++++++------
 xen/arch/arm/arm64/debug-pl011.inc |  6 +++---
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/arm32/debug-pl011.inc b/xen/arch/arm/arm32/debug-pl011.inc
index c527f1d4424d..9fe0c2503831 100644
--- a/xen/arch/arm/arm32/debug-pl011.inc
+++ b/xen/arch/arm/arm32/debug-pl011.inc
@@ -26,13 +26,13 @@
  */
 .macro early_uart_init rb, rc, rd
         mov   \rc, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE % 16)
-        str   \rc, [\rb, #FBRD]     /* -> UARTFBRD (Baud divisor fraction) */
+        strb  \rc, [\rb, #FBRD]     /* -> UARTFBRD (Baud divisor fraction) */
         mov   \rc, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE / 16)
-        str   \rc, [\rb, #IBRD]     /* -> UARTIBRD (Baud divisor integer) */
+        strh  \rc, [\rb, #IBRD]     /* -> UARTIBRD (Baud divisor integer) */
         mov   \rc, #WLEN_8          /* 8n1 */
-        str   \rc, [\rb, #LCR_H]     /* -> UARTLCR_H (Line control) */
+        strb  \rc, [\rb, #LCR_H]     /* -> UARTLCR_H (Line control) */
         ldr   \rc, =(RXE | TXE | UARTEN)      /* RXE | TXE | UARTEN */
-        str   \rc, [\rb, #CR]     /* -> UARTCR (Control Register) */
+        strh  \rc, [\rb, #CR]     /* -> UARTCR (Control Register) */
 .endm
 
 /*
@@ -42,7 +42,7 @@
  */
 .macro early_uart_ready rb, rc
 1:
-        ldr   \rc, [\rb, #FR]       /* <- UARTFR (Flag register) */
+        ldrh  \rc, [\rb, #FR]       /* <- UARTFR (Flag register) */
         tst   \rc, #BUSY             /* Check BUSY bit */
         bne   1b                    /* Wait for the UART to be ready */
 .endm
@@ -53,7 +53,7 @@
  * rt: register which contains the character to transmit
  */
 .macro early_uart_transmit rb, rt
-        str   \rt, [\rb, #DR]            /* -> UARTDR (Data Register) */
+        strb  \rt, [\rb, #DR]            /* -> UARTDR (Data Register) */
 .endm
 
 /*
diff --git a/xen/arch/arm/arm64/debug-pl011.inc b/xen/arch/arm/arm64/debug-pl011.inc
index 6d60e78c8ba3..df713eff4922 100644
--- a/xen/arch/arm/arm64/debug-pl011.inc
+++ b/xen/arch/arm/arm64/debug-pl011.inc
@@ -25,13 +25,13 @@
  */
 .macro early_uart_init xb, c
         mov   x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE % 16)
-        strh  w\c, [\xb, #FBRD]      /* -> UARTFBRD (Baud divisor fraction) */
+        strb  w\c, [\xb, #FBRD]      /* -> UARTFBRD (Baud divisor fraction) */
         mov   x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE / 16)
         strh  w\c, [\xb, #IBRD]      /* -> UARTIBRD (Baud divisor integer) */
         mov   x\c, #WLEN_8           /* 8n1 */
-        str   w\c, [\xb, #LCR_H]     /* -> UARTLCR_H (Line control) */
+        strb  w\c, [\xb, #LCR_H]     /* -> UARTLCR_H (Line control) */
         ldr   x\c, =(RXE | TXE | UARTEN)
-        str   w\c, [\xb, #CR]        /* -> UARTCR (Control Register) */
+        strh  w\c, [\xb, #CR]        /* -> UARTCR (Control Register) */
 .endm
 
 /*
-- 
2.25.1

[PATCH 2/4] xen/arm: debug-pl011: Add support for 32-bit only MMIO

[Michal Orzel]

There are implementations of PL011 that can only handle 32-bit accesses
as oppose to the normal behavior where accesses are 8/16-bit wide. This
is usually advertised by setting a dt property 'reg-io-width' to 4.

Introduce CONFIG_EARLY_UART_PL011_MMIO32 Kconfig option to be able to
enable the use of 32-bit only accessors in PL011 early printk code.
Define macros PL011_{STRH,STRB,LDRH} to distinguish accessors for normal
case from 32-bit MMIO one and use them in arm32/arm64 pl011 early printk
code.

Update documentation accordingly.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
I might want to align the indentation of operands but doing so in this patch
is rather no go as it would limit the visibility of the scope of this patch.
Something to do in the future.
---
 docs/misc/arm/early-printk.txt        |  3 +++
 xen/arch/arm/Kconfig.debug            |  7 +++++++
 xen/arch/arm/arm32/debug-pl011.inc    | 12 ++++++------
 xen/arch/arm/arm64/debug-pl011.inc    | 12 ++++++------
 xen/arch/arm/include/asm/pl011-uart.h | 19 +++++++++++++++++++
 5 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/docs/misc/arm/early-printk.txt b/docs/misc/arm/early-printk.txt
index aa22826075a4..bc2d65aa2ea3 100644
--- a/docs/misc/arm/early-printk.txt
+++ b/docs/misc/arm/early-printk.txt
@@ -26,6 +26,9 @@ Other options depends on the driver selected:
       If CONFIG_EARLY_UART_PL011_BAUD_RATE  is set to 0 then the code will
       not try to initialize the UART, so that bootloader or firmware
       settings can be used for maximum compatibility.
+
+    - CONFIG_EARLY_UART_PL011_MMIO32 is, optionally, used to enable 32-bit
+      only accesses to registers.
   - scif
     - CONFIG_EARLY_UART_SCIF_VERSION_* is, optionally, the interface version
       of the UART. Default to version NONE.
diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug
index 842d768280c4..eec860e88e0b 100644
--- a/xen/arch/arm/Kconfig.debug
+++ b/xen/arch/arm/Kconfig.debug
@@ -253,6 +253,13 @@ config EARLY_UART_PL011_BAUD_RATE
 	default 115200 if EARLY_PRINTK_FASTMODEL
 	default 0
 
+config EARLY_UART_PL011_MMIO32
+	bool "32-bit only MMIO for PL011 early printk"
+	depends on EARLY_UART_PL011
+	help
+	  If specified, all accesses to PL011 registers made from early printk code
+	  will be done using 32-bit only accessors.
+
 config EARLY_UART_INIT
 	depends on EARLY_UART_PL011 && EARLY_UART_PL011_BAUD_RATE != 0
 	def_bool y
diff --git a/xen/arch/arm/arm32/debug-pl011.inc b/xen/arch/arm/arm32/debug-pl011.inc
index 9fe0c2503831..5833da2a235c 100644
--- a/xen/arch/arm/arm32/debug-pl011.inc
+++ b/xen/arch/arm/arm32/debug-pl011.inc
@@ -26,13 +26,13 @@
  */
 .macro early_uart_init rb, rc, rd
         mov   \rc, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE % 16)
-        strb  \rc, [\rb, #FBRD]     /* -> UARTFBRD (Baud divisor fraction) */
+        PL011_STRB  \rc, [\rb, #FBRD]  /* -> UARTFBRD (Baud divisor fraction) */
         mov   \rc, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE / 16)
-        strh  \rc, [\rb, #IBRD]     /* -> UARTIBRD (Baud divisor integer) */
+        PL011_STRH  \rc, [\rb, #IBRD]  /* -> UARTIBRD (Baud divisor integer) */
         mov   \rc, #WLEN_8          /* 8n1 */
-        strb  \rc, [\rb, #LCR_H]     /* -> UARTLCR_H (Line control) */
+        PL011_STRB  \rc, [\rb, #LCR_H] /* -> UARTLCR_H (Line control) */
         ldr   \rc, =(RXE | TXE | UARTEN)      /* RXE | TXE | UARTEN */
-        strh  \rc, [\rb, #CR]     /* -> UARTCR (Control Register) */
+        PL011_STRH  \rc, [\rb, #CR]    /* -> UARTCR (Control Register) */
 .endm
 
 /*
@@ -42,7 +42,7 @@
  */
 .macro early_uart_ready rb, rc
 1:
-        ldrh  \rc, [\rb, #FR]       /* <- UARTFR (Flag register) */
+        PL011_LDRH  \rc, [\rb, #FR] /* <- UARTFR (Flag register) */
         tst   \rc, #BUSY             /* Check BUSY bit */
         bne   1b                    /* Wait for the UART to be ready */
 .endm
@@ -53,7 +53,7 @@
  * rt: register which contains the character to transmit
  */
 .macro early_uart_transmit rb, rt
-        strb  \rt, [\rb, #DR]            /* -> UARTDR (Data Register) */
+        PL011_STRB  \rt, [\rb, #DR]      /* -> UARTDR (Data Register) */
 .endm
 
 /*
diff --git a/xen/arch/arm/arm64/debug-pl011.inc b/xen/arch/arm/arm64/debug-pl011.inc
index df713eff4922..430594610b2c 100644
--- a/xen/arch/arm/arm64/debug-pl011.inc
+++ b/xen/arch/arm/arm64/debug-pl011.inc
@@ -25,13 +25,13 @@
  */
 .macro early_uart_init xb, c
         mov   x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE % 16)
-        strb  w\c, [\xb, #FBRD]      /* -> UARTFBRD (Baud divisor fraction) */
+        PL011_STRB  w\c, [\xb, #FBRD]  /* -> UARTFBRD (Baud divisor fraction) */
         mov   x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE / 16)
-        strh  w\c, [\xb, #IBRD]      /* -> UARTIBRD (Baud divisor integer) */
+        PL011_STRH  w\c, [\xb, #IBRD]  /* -> UARTIBRD (Baud divisor integer) */
         mov   x\c, #WLEN_8           /* 8n1 */
-        strb  w\c, [\xb, #LCR_H]     /* -> UARTLCR_H (Line control) */
+        PL011_STRB  w\c, [\xb, #LCR_H] /* -> UARTLCR_H (Line control) */
         ldr   x\c, =(RXE | TXE | UARTEN)
-        strh  w\c, [\xb, #CR]        /* -> UARTCR (Control Register) */
+        PL011_STRH  w\c, [\xb, #CR]    /* -> UARTCR (Control Register) */
 .endm
 
 /*
@@ -41,7 +41,7 @@
  */
 .macro early_uart_ready xb, c
 1:
-        ldrh  w\c, [\xb, #FR]        /* <- UARTFR (Flag register) */
+        PL011_LDRH  w\c, [\xb, #FR]  /* <- UARTFR (Flag register) */
         tst   w\c, #BUSY             /* Check BUSY bit */
         b.ne  1b                     /* Wait for the UART to be ready */
 .endm
@@ -52,7 +52,7 @@
  * wt: register which contains the character to transmit
  */
 .macro early_uart_transmit xb, wt
-        strb  \wt, [\xb, #DR]        /* -> UARTDR (Data Register) */
+        PL011_STRB  \wt, [\xb, #DR]  /* -> UARTDR (Data Register) */
 .endm
 
 /*
diff --git a/xen/arch/arm/include/asm/pl011-uart.h b/xen/arch/arm/include/asm/pl011-uart.h
index 5bb563ec0814..27c9bfa444cb 100644
--- a/xen/arch/arm/include/asm/pl011-uart.h
+++ b/xen/arch/arm/include/asm/pl011-uart.h
@@ -21,6 +21,25 @@
 #ifndef __ASM_ARM_PL011_H
 #define __ASM_ARM_PL011_H
 
+#ifdef __ASSEMBLY__
+
+/*
+ * PL011 registers are 8/16-bit wide. However, there are implementations that
+ * can only handle 32-bit accesses. The following macros used in early printk
+ * are defined to distinguish accessors for normal case from 32-bit MMIO one.
+ */
+#ifdef CONFIG_EARLY_UART_PL011_MMIO32
+#define PL011_STRH str
+#define PL011_STRB str
+#define PL011_LDRH ldr
+#else
+#define PL011_STRH strh
+#define PL011_STRB strb
+#define PL011_LDRH ldrh
+#endif
+
+#endif /* __ASSEMBLY__ */
+
 /* PL011 register addresses */
 #define DR     (0x00)
 #define RSR    (0x04)
-- 
2.25.1

[PATCH 3/4] xen/arm: pl011: Use correct accessors

[Michal Orzel]

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

[PATCH 4/4] xen/arm: pl011: Add SBSA UART device-tree support

[Michal Orzel]

We already have all the bits necessary in PL011 driver to support SBSA
UART thanks to commit 032ea8c736d10f02672863c6e369338f948f7ed8 that
enabled it for ACPI. Plumb in the remaining part for device-tree boot:
 - add arm,sbsa-uart compatible to pl011_dt_match (no need for a separate
   struct and DT_DEVICE_START as SBSA is a subset of PL011),
 - from pl011_dt_uart_init(), check for SBSA UART compatible to determine
   the UART type in use.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
After this series the last thing not to be in spec for newer UARTs (well,
for rev1.5 introduced in 2007 I believe) is incorrect FIFO size. We hardcode it
to 16 but in r1.5 it is 32. This requires checking the peripheral ID register
or using arm,primecell-periphid dt property for overriding HW. Something to
be done in the future (at least 16 is not harmful).
---
 xen/drivers/char/pl011.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
index 403b1ac06551..f7bf3ad117af 100644
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -286,7 +286,7 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev,
     int res;
     paddr_t addr, size;
     uint32_t io_width;
-    bool mmio32 = false;
+    bool mmio32 = false, sbsa;
 
     if ( strcmp(config, "") )
     {
@@ -320,7 +320,9 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev,
         }
     }
 
-    res = pl011_uart_init(res, addr, size, false, mmio32);
+    sbsa = dt_device_is_compatible(dev, "arm,sbsa-uart");
+
+    res = pl011_uart_init(res, addr, size, sbsa, mmio32);
     if ( res < 0 )
     {
         printk("pl011: Unable to initialize\n");
@@ -335,6 +337,8 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev,
 static const struct dt_device_match pl011_dt_match[] __initconst =
 {
     DT_MATCH_COMPATIBLE("arm,pl011"),
+    /* No need for a separate struct as SBSA UART is a subset of PL011 */
+    DT_MATCH_COMPATIBLE("arm,sbsa-uart"),
     { /* sentinel */ },
 };
 
-- 
2.25.1

RE: [PATCH 1/4] xen/arm: debug-pl011: Use correct accessors

[Henry Wang]

Hi Michal,

> -----Original Message-----
> Subject: [PATCH 1/4] xen/arm: debug-pl011: Use correct accessors
> 
> Although most PL011 UARTs can cope with 32-bit accesses, some of the old
> legacy ones might not. PL011 registers are 8/16-bit wide and this shall
> be perceived as the normal behavior.
> 
> Modify early printk pl011 code for arm32/arm64 to use the correct
> accessors depending on the register size (refer ARM DDI 0183G, Table 3.1).
> 
> 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

RE: [PATCH 2/4] xen/arm: debug-pl011: Add support for 32-bit only MMIO

[Henry Wang]

Hi Michal,

> -----Original Message-----
> Subject: [PATCH 2/4] xen/arm: debug-pl011: Add support for 32-bit only
> MMIO
> 
> There are implementations of PL011 that can only handle 32-bit accesses
> as oppose to the normal behavior where accesses are 8/16-bit wide. This
> is usually advertised by setting a dt property 'reg-io-width' to 4.
> 
> Introduce CONFIG_EARLY_UART_PL011_MMIO32 Kconfig option to be able to
> enable the use of 32-bit only accessors in PL011 early printk code.
> Define macros PL011_{STRH,STRB,LDRH} to distinguish accessors for normal
> case from 32-bit MMIO one and use them in arm32/arm64 pl011 early printk
> code.
> 
> Update documentation accordingly.
> 
> 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

RE: [PATCH 3/4] xen/arm: pl011: Use correct accessors

[Henry Wang]

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

RE: [PATCH 4/4] xen/arm: pl011: Add SBSA UART device-tree support

[Henry Wang]

Hi Michal,

> -----Original Message-----
> Subject: [PATCH 4/4] xen/arm: pl011: Add SBSA UART device-tree support
> 
> We already have all the bits necessary in PL011 driver to support SBSA
> UART thanks to commit 032ea8c736d10f02672863c6e369338f948f7ed8 that
> enabled it for ACPI. Plumb in the remaining part for device-tree boot:
>  - add arm,sbsa-uart compatible to pl011_dt_match (no need for a separate
>    struct and DT_DEVICE_START as SBSA is a subset of PL011),
>  - from pl011_dt_uart_init(), check for SBSA UART compatible to determine
>    the UART type in use.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

I've also 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

Re: [PATCH 1/4] xen/arm: debug-pl011: Use correct accessors

[Stefano Stabellini]

On Wed, 7 Jun 2023, Michal Orzel wrote:
> Although most PL011 UARTs can cope with 32-bit accesses, some of the old
> legacy ones might not. PL011 registers are 8/16-bit wide and this shall
> be perceived as the normal behavior.
> 
> Modify early printk pl011 code for arm32/arm64 to use the correct
> accessors depending on the register size (refer ARM DDI 0183G, Table 3.1).
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
> Next patch will override strX,ldrX with macros but I prefer to keep the
> history clean (+ possibiltity for a backport if needed).
> ---
>  xen/arch/arm/arm32/debug-pl011.inc | 12 ++++++------
>  xen/arch/arm/arm64/debug-pl011.inc |  6 +++---
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/debug-pl011.inc b/xen/arch/arm/arm32/debug-pl011.inc
> index c527f1d4424d..9fe0c2503831 100644
> --- a/xen/arch/arm/arm32/debug-pl011.inc
> +++ b/xen/arch/arm/arm32/debug-pl011.inc
> @@ -26,13 +26,13 @@
>   */
>  .macro early_uart_init rb, rc, rd
>          mov   \rc, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE % 16)
> -        str   \rc, [\rb, #FBRD]     /* -> UARTFBRD (Baud divisor fraction) */
> +        strb  \rc, [\rb, #FBRD]     /* -> UARTFBRD (Baud divisor fraction) */
>          mov   \rc, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE / 16)
> -        str   \rc, [\rb, #IBRD]     /* -> UARTIBRD (Baud divisor integer) */
> +        strh  \rc, [\rb, #IBRD]     /* -> UARTIBRD (Baud divisor integer) */
>          mov   \rc, #WLEN_8          /* 8n1 */
> -        str   \rc, [\rb, #LCR_H]     /* -> UARTLCR_H (Line control) */
> +        strb  \rc, [\rb, #LCR_H]     /* -> UARTLCR_H (Line control) */
>          ldr   \rc, =(RXE | TXE | UARTEN)      /* RXE | TXE | UARTEN */
> -        str   \rc, [\rb, #CR]     /* -> UARTCR (Control Register) */
> +        strh  \rc, [\rb, #CR]     /* -> UARTCR (Control Register) */
>  .endm
>  
>  /*
> @@ -42,7 +42,7 @@
>   */
>  .macro early_uart_ready rb, rc
>  1:
> -        ldr   \rc, [\rb, #FR]       /* <- UARTFR (Flag register) */
> +        ldrh  \rc, [\rb, #FR]       /* <- UARTFR (Flag register) */
>          tst   \rc, #BUSY             /* Check BUSY bit */
>          bne   1b                    /* Wait for the UART to be ready */
>  .endm
> @@ -53,7 +53,7 @@
>   * rt: register which contains the character to transmit
>   */
>  .macro early_uart_transmit rb, rt
> -        str   \rt, [\rb, #DR]            /* -> UARTDR (Data Register) */
> +        strb  \rt, [\rb, #DR]            /* -> UARTDR (Data Register) */

Isn't UARTDR potentially 12-bit? I am not sure if we should use strb or
strh here...

Everything else checks out.


>  .endm
>  
>  /*
> diff --git a/xen/arch/arm/arm64/debug-pl011.inc b/xen/arch/arm/arm64/debug-pl011.inc
> index 6d60e78c8ba3..df713eff4922 100644
> --- a/xen/arch/arm/arm64/debug-pl011.inc
> +++ b/xen/arch/arm/arm64/debug-pl011.inc
> @@ -25,13 +25,13 @@
>   */
>  .macro early_uart_init xb, c
>          mov   x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE % 16)
> -        strh  w\c, [\xb, #FBRD]      /* -> UARTFBRD (Baud divisor fraction) */
> +        strb  w\c, [\xb, #FBRD]      /* -> UARTFBRD (Baud divisor fraction) */
>          mov   x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE / 16)
>          strh  w\c, [\xb, #IBRD]      /* -> UARTIBRD (Baud divisor integer) */
>          mov   x\c, #WLEN_8           /* 8n1 */
> -        str   w\c, [\xb, #LCR_H]     /* -> UARTLCR_H (Line control) */
> +        strb  w\c, [\xb, #LCR_H]     /* -> UARTLCR_H (Line control) */
>          ldr   x\c, =(RXE | TXE | UARTEN)
> -        str   w\c, [\xb, #CR]        /* -> UARTCR (Control Register) */
> +        strh  w\c, [\xb, #CR]        /* -> UARTCR (Control Register) */
>  .endm
>  
>  /*
> -- 
> 2.25.1
> 

Re: [PATCH 2/4] xen/arm: debug-pl011: Add support for 32-bit only MMIO

[Stefano Stabellini]

On Wed, 7 Jun 2023, Michal Orzel wrote:
> There are implementations of PL011 that can only handle 32-bit accesses
> as oppose to the normal behavior where accesses are 8/16-bit wide. This
> is usually advertised by setting a dt property 'reg-io-width' to 4.
> 
> Introduce CONFIG_EARLY_UART_PL011_MMIO32 Kconfig option to be able to
> enable the use of 32-bit only accessors in PL011 early printk code.
> Define macros PL011_{STRH,STRB,LDRH} to distinguish accessors for normal
> case from 32-bit MMIO one and use them in arm32/arm64 pl011 early printk
> code.
> 
> Update documentation accordingly.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

With the caveat of the potential change to patch #1 that would affect
this patch too


> ---
> I might want to align the indentation of operands but doing so in this patch
> is rather no go as it would limit the visibility of the scope of this patch.
> Something to do in the future.
> ---
>  docs/misc/arm/early-printk.txt        |  3 +++
>  xen/arch/arm/Kconfig.debug            |  7 +++++++
>  xen/arch/arm/arm32/debug-pl011.inc    | 12 ++++++------
>  xen/arch/arm/arm64/debug-pl011.inc    | 12 ++++++------
>  xen/arch/arm/include/asm/pl011-uart.h | 19 +++++++++++++++++++
>  5 files changed, 41 insertions(+), 12 deletions(-)
> 
> diff --git a/docs/misc/arm/early-printk.txt b/docs/misc/arm/early-printk.txt
> index aa22826075a4..bc2d65aa2ea3 100644
> --- a/docs/misc/arm/early-printk.txt
> +++ b/docs/misc/arm/early-printk.txt
> @@ -26,6 +26,9 @@ Other options depends on the driver selected:
>        If CONFIG_EARLY_UART_PL011_BAUD_RATE  is set to 0 then the code will
>        not try to initialize the UART, so that bootloader or firmware
>        settings can be used for maximum compatibility.
> +
> +    - CONFIG_EARLY_UART_PL011_MMIO32 is, optionally, used to enable 32-bit
> +      only accesses to registers.
>    - scif
>      - CONFIG_EARLY_UART_SCIF_VERSION_* is, optionally, the interface version
>        of the UART. Default to version NONE.
> diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug
> index 842d768280c4..eec860e88e0b 100644
> --- a/xen/arch/arm/Kconfig.debug
> +++ b/xen/arch/arm/Kconfig.debug
> @@ -253,6 +253,13 @@ config EARLY_UART_PL011_BAUD_RATE
>  	default 115200 if EARLY_PRINTK_FASTMODEL
>  	default 0
>  
> +config EARLY_UART_PL011_MMIO32
> +	bool "32-bit only MMIO for PL011 early printk"
> +	depends on EARLY_UART_PL011
> +	help
> +	  If specified, all accesses to PL011 registers made from early printk code
> +	  will be done using 32-bit only accessors.
> +
>  config EARLY_UART_INIT
>  	depends on EARLY_UART_PL011 && EARLY_UART_PL011_BAUD_RATE != 0
>  	def_bool y
> diff --git a/xen/arch/arm/arm32/debug-pl011.inc b/xen/arch/arm/arm32/debug-pl011.inc
> index 9fe0c2503831..5833da2a235c 100644
> --- a/xen/arch/arm/arm32/debug-pl011.inc
> +++ b/xen/arch/arm/arm32/debug-pl011.inc
> @@ -26,13 +26,13 @@
>   */
>  .macro early_uart_init rb, rc, rd
>          mov   \rc, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE % 16)
> -        strb  \rc, [\rb, #FBRD]     /* -> UARTFBRD (Baud divisor fraction) */
> +        PL011_STRB  \rc, [\rb, #FBRD]  /* -> UARTFBRD (Baud divisor fraction) */
>          mov   \rc, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE / 16)
> -        strh  \rc, [\rb, #IBRD]     /* -> UARTIBRD (Baud divisor integer) */
> +        PL011_STRH  \rc, [\rb, #IBRD]  /* -> UARTIBRD (Baud divisor integer) */
>          mov   \rc, #WLEN_8          /* 8n1 */
> -        strb  \rc, [\rb, #LCR_H]     /* -> UARTLCR_H (Line control) */
> +        PL011_STRB  \rc, [\rb, #LCR_H] /* -> UARTLCR_H (Line control) */
>          ldr   \rc, =(RXE | TXE | UARTEN)      /* RXE | TXE | UARTEN */
> -        strh  \rc, [\rb, #CR]     /* -> UARTCR (Control Register) */
> +        PL011_STRH  \rc, [\rb, #CR]    /* -> UARTCR (Control Register) */
>  .endm
>  
>  /*
> @@ -42,7 +42,7 @@
>   */
>  .macro early_uart_ready rb, rc
>  1:
> -        ldrh  \rc, [\rb, #FR]       /* <- UARTFR (Flag register) */
> +        PL011_LDRH  \rc, [\rb, #FR] /* <- UARTFR (Flag register) */
>          tst   \rc, #BUSY             /* Check BUSY bit */
>          bne   1b                    /* Wait for the UART to be ready */
>  .endm
> @@ -53,7 +53,7 @@
>   * rt: register which contains the character to transmit
>   */
>  .macro early_uart_transmit rb, rt
> -        strb  \rt, [\rb, #DR]            /* -> UARTDR (Data Register) */
> +        PL011_STRB  \rt, [\rb, #DR]      /* -> UARTDR (Data Register) */
>  .endm
>  
>  /*
> diff --git a/xen/arch/arm/arm64/debug-pl011.inc b/xen/arch/arm/arm64/debug-pl011.inc
> index df713eff4922..430594610b2c 100644
> --- a/xen/arch/arm/arm64/debug-pl011.inc
> +++ b/xen/arch/arm/arm64/debug-pl011.inc
> @@ -25,13 +25,13 @@
>   */
>  .macro early_uart_init xb, c
>          mov   x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE % 16)
> -        strb  w\c, [\xb, #FBRD]      /* -> UARTFBRD (Baud divisor fraction) */
> +        PL011_STRB  w\c, [\xb, #FBRD]  /* -> UARTFBRD (Baud divisor fraction) */
>          mov   x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE / 16)
> -        strh  w\c, [\xb, #IBRD]      /* -> UARTIBRD (Baud divisor integer) */
> +        PL011_STRH  w\c, [\xb, #IBRD]  /* -> UARTIBRD (Baud divisor integer) */
>          mov   x\c, #WLEN_8           /* 8n1 */
> -        strb  w\c, [\xb, #LCR_H]     /* -> UARTLCR_H (Line control) */
> +        PL011_STRB  w\c, [\xb, #LCR_H] /* -> UARTLCR_H (Line control) */
>          ldr   x\c, =(RXE | TXE | UARTEN)
> -        strh  w\c, [\xb, #CR]        /* -> UARTCR (Control Register) */
> +        PL011_STRH  w\c, [\xb, #CR]    /* -> UARTCR (Control Register) */
>  .endm
>  
>  /*
> @@ -41,7 +41,7 @@
>   */
>  .macro early_uart_ready xb, c
>  1:
> -        ldrh  w\c, [\xb, #FR]        /* <- UARTFR (Flag register) */
> +        PL011_LDRH  w\c, [\xb, #FR]  /* <- UARTFR (Flag register) */
>          tst   w\c, #BUSY             /* Check BUSY bit */
>          b.ne  1b                     /* Wait for the UART to be ready */
>  .endm
> @@ -52,7 +52,7 @@
>   * wt: register which contains the character to transmit
>   */
>  .macro early_uart_transmit xb, wt
> -        strb  \wt, [\xb, #DR]        /* -> UARTDR (Data Register) */
> +        PL011_STRB  \wt, [\xb, #DR]  /* -> UARTDR (Data Register) */
>  .endm
>  
>  /*
> diff --git a/xen/arch/arm/include/asm/pl011-uart.h b/xen/arch/arm/include/asm/pl011-uart.h
> index 5bb563ec0814..27c9bfa444cb 100644
> --- a/xen/arch/arm/include/asm/pl011-uart.h
> +++ b/xen/arch/arm/include/asm/pl011-uart.h
> @@ -21,6 +21,25 @@
>  #ifndef __ASM_ARM_PL011_H
>  #define __ASM_ARM_PL011_H
>  
> +#ifdef __ASSEMBLY__
> +
> +/*
> + * PL011 registers are 8/16-bit wide. However, there are implementations that
> + * can only handle 32-bit accesses. The following macros used in early printk
> + * are defined to distinguish accessors for normal case from 32-bit MMIO one.
> + */
> +#ifdef CONFIG_EARLY_UART_PL011_MMIO32
> +#define PL011_STRH str
> +#define PL011_STRB str
> +#define PL011_LDRH ldr
> +#else
> +#define PL011_STRH strh
> +#define PL011_STRB strb
> +#define PL011_LDRH ldrh
> +#endif
> +
> +#endif /* __ASSEMBLY__ */
> +
>  /* PL011 register addresses */
>  #define DR     (0x00)
>  #define RSR    (0x04)
> -- 
> 2.25.1
> 

Re: [PATCH 3/4] xen/arm: pl011: Use correct accessors

[Stefano Stabellini]

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
> 

Re: [PATCH 4/4] xen/arm: pl011: Add SBSA UART device-tree support

[Stefano Stabellini]

On Wed, 7 Jun 2023, Michal Orzel wrote:
> We already have all the bits necessary in PL011 driver to support SBSA
> UART thanks to commit 032ea8c736d10f02672863c6e369338f948f7ed8 that
> enabled it for ACPI. Plumb in the remaining part for device-tree boot:
>  - add arm,sbsa-uart compatible to pl011_dt_match (no need for a separate
>    struct and DT_DEVICE_START as SBSA is a subset of PL011),
>  - from pl011_dt_uart_init(), check for SBSA UART compatible to determine
>    the UART type in use.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> After this series the last thing not to be in spec for newer UARTs (well,
> for rev1.5 introduced in 2007 I believe) is incorrect FIFO size. We hardcode it
> to 16 but in r1.5 it is 32. This requires checking the peripheral ID register
> or using arm,primecell-periphid dt property for overriding HW. Something to
> be done in the future (at least 16 is not harmful).
> ---
>  xen/drivers/char/pl011.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
> index 403b1ac06551..f7bf3ad117af 100644
> --- a/xen/drivers/char/pl011.c
> +++ b/xen/drivers/char/pl011.c
> @@ -286,7 +286,7 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev,
>      int res;
>      paddr_t addr, size;
>      uint32_t io_width;
> -    bool mmio32 = false;
> +    bool mmio32 = false, sbsa;
>  
>      if ( strcmp(config, "") )
>      {
> @@ -320,7 +320,9 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev,
>          }
>      }
>  
> -    res = pl011_uart_init(res, addr, size, false, mmio32);
> +    sbsa = dt_device_is_compatible(dev, "arm,sbsa-uart");
> +
> +    res = pl011_uart_init(res, addr, size, sbsa, mmio32);
>      if ( res < 0 )
>      {
>          printk("pl011: Unable to initialize\n");
> @@ -335,6 +337,8 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev,
>  static const struct dt_device_match pl011_dt_match[] __initconst =
>  {
>      DT_MATCH_COMPATIBLE("arm,pl011"),
> +    /* No need for a separate struct as SBSA UART is a subset of PL011 */
> +    DT_MATCH_COMPATIBLE("arm,sbsa-uart"),
>      { /* sentinel */ },
>  };
>  
> -- 
> 2.25.1
> 

Re: [PATCH 1/4] xen/arm: debug-pl011: Use correct accessors

[Michal Orzel]

Hi Stefano,

On 15/06/2023 03:36, Stefano Stabellini wrote:
> 
> 
> On Wed, 7 Jun 2023, Michal Orzel wrote:
>> Although most PL011 UARTs can cope with 32-bit accesses, some of the old
>> legacy ones might not. PL011 registers are 8/16-bit wide and this shall
>> be perceived as the normal behavior.
>>
>> Modify early printk pl011 code for arm32/arm64 to use the correct
>> accessors depending on the register size (refer ARM DDI 0183G, Table 3.1).
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>> Next patch will override strX,ldrX with macros but I prefer to keep the
>> history clean (+ possibiltity for a backport if needed).
>> ---
>>  xen/arch/arm/arm32/debug-pl011.inc | 12 ++++++------
>>  xen/arch/arm/arm64/debug-pl011.inc |  6 +++---
>>  2 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/debug-pl011.inc b/xen/arch/arm/arm32/debug-pl011.inc
>> index c527f1d4424d..9fe0c2503831 100644
>> --- a/xen/arch/arm/arm32/debug-pl011.inc
>> +++ b/xen/arch/arm/arm32/debug-pl011.inc
>> @@ -26,13 +26,13 @@
>>   */
>>  .macro early_uart_init rb, rc, rd
>>          mov   \rc, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE % 16)
>> -        str   \rc, [\rb, #FBRD]     /* -> UARTFBRD (Baud divisor fraction) */
>> +        strb  \rc, [\rb, #FBRD]     /* -> UARTFBRD (Baud divisor fraction) */
>>          mov   \rc, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE / 16)
>> -        str   \rc, [\rb, #IBRD]     /* -> UARTIBRD (Baud divisor integer) */
>> +        strh  \rc, [\rb, #IBRD]     /* -> UARTIBRD (Baud divisor integer) */
>>          mov   \rc, #WLEN_8          /* 8n1 */
>> -        str   \rc, [\rb, #LCR_H]     /* -> UARTLCR_H (Line control) */
>> +        strb  \rc, [\rb, #LCR_H]     /* -> UARTLCR_H (Line control) */
>>          ldr   \rc, =(RXE | TXE | UARTEN)      /* RXE | TXE | UARTEN */
>> -        str   \rc, [\rb, #CR]     /* -> UARTCR (Control Register) */
>> +        strh  \rc, [\rb, #CR]     /* -> UARTCR (Control Register) */
>>  .endm
>>
>>  /*
>> @@ -42,7 +42,7 @@
>>   */
>>  .macro early_uart_ready rb, rc
>>  1:
>> -        ldr   \rc, [\rb, #FR]       /* <- UARTFR (Flag register) */
>> +        ldrh  \rc, [\rb, #FR]       /* <- UARTFR (Flag register) */
>>          tst   \rc, #BUSY             /* Check BUSY bit */
>>          bne   1b                    /* Wait for the UART to be ready */
>>  .endm
>> @@ -53,7 +53,7 @@
>>   * rt: register which contains the character to transmit
>>   */
>>  .macro early_uart_transmit rb, rt
>> -        str   \rt, [\rb, #DR]            /* -> UARTDR (Data Register) */
>> +        strb  \rt, [\rb, #DR]            /* -> UARTDR (Data Register) */
> 
> Isn't UARTDR potentially 12-bit? I am not sure if we should use strb or
> strh here...
UARTDR is 16-bit register where bits 15:12 are reserved and 11:8 are for indicating errors during READ.
Here, we perform WRITE meaning the actual register width is 8 bytes. This is also indicated by the PL011 TRM
which specifies width as "12/8" meaning 12 for READ and 8 for WRITE.

~Michal

Re: [PATCH 1/4] xen/arm: debug-pl011: Use correct accessors

[Stefano Stabellini]

On Thu, 15 Jun 2023, Michal Orzel wrote:
> Hi Stefano,
> 
> On 15/06/2023 03:36, Stefano Stabellini wrote:
> > 
> > 
> > On Wed, 7 Jun 2023, Michal Orzel wrote:
> >> Although most PL011 UARTs can cope with 32-bit accesses, some of the old
> >> legacy ones might not. PL011 registers are 8/16-bit wide and this shall
> >> be perceived as the normal behavior.
> >>
> >> Modify early printk pl011 code for arm32/arm64 to use the correct
> >> accessors depending on the register size (refer ARM DDI 0183G, Table 3.1).
> >>
> >> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> >> ---
> >> Next patch will override strX,ldrX with macros but I prefer to keep the
> >> history clean (+ possibiltity for a backport if needed).
> >> ---
> >>  xen/arch/arm/arm32/debug-pl011.inc | 12 ++++++------
> >>  xen/arch/arm/arm64/debug-pl011.inc |  6 +++---
> >>  2 files changed, 9 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/arm32/debug-pl011.inc b/xen/arch/arm/arm32/debug-pl011.inc
> >> index c527f1d4424d..9fe0c2503831 100644
> >> --- a/xen/arch/arm/arm32/debug-pl011.inc
> >> +++ b/xen/arch/arm/arm32/debug-pl011.inc
> >> @@ -26,13 +26,13 @@
> >>   */
> >>  .macro early_uart_init rb, rc, rd
> >>          mov   \rc, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE % 16)
> >> -        str   \rc, [\rb, #FBRD]     /* -> UARTFBRD (Baud divisor fraction) */
> >> +        strb  \rc, [\rb, #FBRD]     /* -> UARTFBRD (Baud divisor fraction) */
> >>          mov   \rc, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE / 16)
> >> -        str   \rc, [\rb, #IBRD]     /* -> UARTIBRD (Baud divisor integer) */
> >> +        strh  \rc, [\rb, #IBRD]     /* -> UARTIBRD (Baud divisor integer) */
> >>          mov   \rc, #WLEN_8          /* 8n1 */
> >> -        str   \rc, [\rb, #LCR_H]     /* -> UARTLCR_H (Line control) */
> >> +        strb  \rc, [\rb, #LCR_H]     /* -> UARTLCR_H (Line control) */
> >>          ldr   \rc, =(RXE | TXE | UARTEN)      /* RXE | TXE | UARTEN */
> >> -        str   \rc, [\rb, #CR]     /* -> UARTCR (Control Register) */
> >> +        strh  \rc, [\rb, #CR]     /* -> UARTCR (Control Register) */
> >>  .endm
> >>
> >>  /*
> >> @@ -42,7 +42,7 @@
> >>   */
> >>  .macro early_uart_ready rb, rc
> >>  1:
> >> -        ldr   \rc, [\rb, #FR]       /* <- UARTFR (Flag register) */
> >> +        ldrh  \rc, [\rb, #FR]       /* <- UARTFR (Flag register) */
> >>          tst   \rc, #BUSY             /* Check BUSY bit */
> >>          bne   1b                    /* Wait for the UART to be ready */
> >>  .endm
> >> @@ -53,7 +53,7 @@
> >>   * rt: register which contains the character to transmit
> >>   */
> >>  .macro early_uart_transmit rb, rt
> >> -        str   \rt, [\rb, #DR]            /* -> UARTDR (Data Register) */
> >> +        strb  \rt, [\rb, #DR]            /* -> UARTDR (Data Register) */
> > 
> > Isn't UARTDR potentially 12-bit? I am not sure if we should use strb or
> > strh here...
> UARTDR is 16-bit register where bits 15:12 are reserved and 11:8 are for indicating errors during READ.
> Here, we perform WRITE meaning the actual register width is 8 bytes. This is also indicated by the PL011 TRM
> which specifies width as "12/8" meaning 12 for READ and 8 for WRITE.

That makes sense

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>