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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxx>
> >> ---
> >> 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@xxxxxxxxxx>