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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 05/06/2023 20:34, Julien Grall wrote:
> 
> 
> Hi,
> 
> On 01/06/2023 13:38, Michal Orzel wrote:
>>
>> 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.
> 
> I had another look at this. You are right, "str" can work with wN or xN.
> I got confused because in the past the issue we had was with the asm
> volatile with 'msr' (see 57e761b60dc9 "xen/arm64: Remove
> READ/WRITE_SYSREG32 helper macros").
> 
> [...]
> 
>>> 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]
> 
> Hmmm... Looking at the spec, LCR_H is meant to be 8-bit and CR 16-bit.
> So I think we need to modify both of them. I am happy to send a patch
> separately for that either before or after your patch.

There are other cases like FBRD being 6bit so we could use strb instead of strh.
In any case, I will handle all of that in a consistent manner by sending a 
series that should not take much time.

I can think of the following patches:
1) Use correct accessors for early pl011 on arm32 and arm64
- this would use the accessors depending on the given register size (ldrh, 
strh, strb)
- this would switch arm32 from 32-bit only to the same behavior as arm64

2) Support for 32-bit only PL011 in early pl011 on arm32 and arm64:
- this would result in overwriting the changes done in patch 1 (but I'd prefer 
not to do all in one patch for better history/backporting)
as I'd have to introduce some macros e.g. PL011_STRH that would be defined as 
strh in normal case and str in mmio32 case, etc. I could also
just duplicate all the early functions and use ifdefery if that's what would be 
preferred.

3) Use correct accessors for runtime pl011:
I would prefer to do what Linux does meaning using the largest-common accessor 
in normal-case (i.e. readw/writew) so that we can have a generic helpers
(i.e. use readl/writel in mmio32 case or readw/writew in normal case). 
Otherwise we would have to use the accessors depending on the given register 
size
(like in early printk) which destroys the idea of generic helpers. Linux for 
earlycon also uses per-register size accessor and in runtime - largest-common.

Let me know your thoughts.

~Michal