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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 01/06/2023 14:17, Julien Grall wrote:
>       
> 
> 
> 
> 
> On Thu, 1 Jun 2023 at 13:48, Michal Orzel <michal.orzel@xxxxxxx 
> <mailto:michal.orzel@xxxxxxx>> wrote:
> 
>     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> <mailto: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> <mailto: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> <mailto: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> <http://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?
> 
> 
> I don’t have the spec with me. I will have a look on Monday and reply back 
> here.
> 
> 
> 
>     >
>     >
>     >     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.
> 
> 
> It depends how you approach the problem. From my POV, a user that can’t see 
> logs will likely try to enable the early UART. Then they could debug the 
> runtime driver. With your proposal, this would even be broken.
> 
>     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.
> 
> 
> I am under the impression that the default behaviour on Linux is to use non 
> 32-big access. I would not want to diverge from 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.
> 
> 
> I don’t view it as a requirement. It would be OK to have it only available 
> for 64-but at the beginning. Same for the runtime.
> 
> 
>     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).
> 
> 
> I am aware of this issue but I don’t understand how this is related to this 
> discussion.
> 
> My only request is you don’t break the existing behavior of the early PL011 
> driver on arm64.
I understand your concern about legacy devices and that you do not want to 
break the existing code on arm64.
But please correct me if I'm wrong. These two lines:
str   w\c, [\xb, #LCR_H]
str   w\c, [\xb, #CR]

already make 32-bit accesses, meaning our early driver does not support legacy 
devices anyway.
You can say it is part of early_uart_init that is not often used but still you 
cannot really say
that our current code supports such devices.

Anyway, I will then first prepare a fix for the current code and then add 
support for mmio32 only.

~Michal