| Message ID | 20230607092727.19654-2-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 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
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 >
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
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>
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 /*
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(-)