Re: [PATCH] xen/arm: debug-pl011: Use 32-bit accessors for broader compatibility

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 01/06/2023 13:12, Julien Grall wrote:
>       
> 
> 
> Hi,
> 
> Sorry for the formatting.
> 
> On Thu, 1 Jun 2023 at 12:31, Michal Orzel <michal.orzel@xxxxxxx 
> <mailto:michal.orzel@xxxxxxx>> wrote:
> 
>     Hi Bertrand,
> 
>     On 01/06/2023 12:19, Bertrand Marquis wrote:
>     >
>     >
>     > Hi Michal,
>     >
>     >> On 1 Jun 2023, at 10:50, Michal Orzel <michal.orzel@xxxxxxx 
> <mailto:michal.orzel@xxxxxxx>> 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.orzel@xxxxxxx 
> <mailto:michal.orzel@xxxxxxx>>
>     >> ---
>     >> 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 <http://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”.
Hmm, I would expect it to be totally ok as the size is determined by the reg 
name. Any reference?

> 
> 
>     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'm focusing on supporting wider range of devices.
At the moment Xen PL011 runtime makes 32-bit accesses while early code makes 
8/16-bit accesses (arm32 uses 32-bit only as well).
So my patch can only improve things and not make them worse. In case of some 
very old legacy device that cannot cope with 32-bit accesses,
such device would not work anyway with the runtime driver. Also, while I'm 
aware of platforms with 32-bit only UART and the normal one
that can cope with 32-bit as well, I'm not aware of any legacy one that cannot 
do that.

Adding a config option like EARLY_UART_PL011_MMIO32 would be ok but it would 
require to also modify arm32 early printk and runtime driver.
Not mentioning things that we somehow do not want to look at like hardcoded 
7372800 HZ frequency for early_uart_init we can just pray
to match the HW UART clock or other not PL011 spec things (i.e. incorrect FIFO 
size for most modern UARTs).

But if this is what you require, I'm somewhat forced to do so just so that our 
devices can be supported.

~Michal