Hi, Sorry for the formatting.
On Thu, 1 Jun 2023 at 12:31, Michal Orzel <michal.or...@amd.com> wrote: > Hi Bertrand, > > On 01/06/2023 12:19, Bertrand Marquis wrote: > > > > > > Hi Michal, > > > >> On 1 Jun 2023, at 10:50, Michal Orzel <michal.or...@amd.com> wrote: > >> > >> There are implementations of the PL011 that can only handle 32-bit > >> accesses (i.e. no 16-bit or 8-bit), usually advertised by 'reg-io-width' > >> dt property set to 4. On such UARTs, the current early printk code for > >> arm64 does not work. To fix this issue, make all the accesses to be > 32-bit > >> by using ldr, str without a size field. This makes it possible to use > >> early printk on such platforms, while all the other implementations > should > >> generally cope with 32-bit accesses. In case they do not, they would > >> already fail as we explicitly use writel/readl in the runtime driver to > >> maintain broader compatibility and to be SBSAv2 compliant. Therefore, > this > >> change makes the runtime/early handling consistent (also it matches the > >> arm32 debug-pl011 code). > >> > >> Signed-off-by: Michal Orzel <michal.or...@amd.com> > >> --- > >> xen/arch/arm/arm64/debug-pl011.inc | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/xen/arch/arm/arm64/debug-pl011.inc > b/xen/arch/arm/arm64/debug-pl011.inc > >> index 6d60e78c8ba3..80eb8fdc1ec7 100644 > >> --- a/xen/arch/arm/arm64/debug-pl011.inc > >> +++ b/xen/arch/arm/arm64/debug-pl011.inc > >> @@ -25,9 +25,9 @@ > >> */ > >> .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) */ > >> + str 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) */ > >> + str w\c, [\xb, #IBRD] /* -> UARTIBRD (Baud divisor > integer) */ > >> mov x\c, #WLEN_8 /* 8n1 */ > >> str w\c, [\xb, #LCR_H] /* -> UARTLCR_H (Line control) */ > >> ldr x\c, =(RXE | TXE | UARTEN) > >> @@ -41,7 +41,7 @@ > >> */ > >> .macro early_uart_ready xb, c > >> 1: > >> - ldrh w\c, [\xb, #FR] /* <- UARTFR (Flag register) */ > >> + ldr 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) */ > >> + str \wt, [\xb, #DR] /* -> UARTDR (Data Register) */ > > > > Is it really ok to drop the 8bit access here ? > It is not only ok, it is necessary. Otherwise it won't work on the above > mentioned UARTs (they can only perform 32-bit access). IIRC some compilers will complain because you use wN with “str”. > And following to what I wrote in commit msg: > - we use str already in arm32 which results in 32-bit access > - we use reald/writel that end up as str/ldr in runtime driver > - we are down to SBSAv2 spec that runtime driver follows (meaning we can > use early printk for SBSA too) The runtime driver is meant to follow the PL011 spec first and may have some adaptation for SBSA. > - this way we support broader list of PL011s consistently (i.e. both early > and runtime driver works as oppose to only runtime) I am not sure I agree here. You are focussing on HW that only support 32-bit access. And, AFAICT this shouldn’t be the norm. I think it would be best if we have an option to select the width supported and modify the code accordingly. And yes, I know that there might be some issues in the runtime driver. But they can be handled separately. With that we don’t promote a behaviour that AFAICT is not meant to be normal. > > Also, before every early_uart_transmit we use ldrb (to convert to char) > which means that the rest of the \wt register (8:31) is zero extended. > > ~Michal >
