Xen project Mailing List

On Thu, 1 Jun 2023 at 12:31, Michal Orzel <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> 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>
>> ---
>> 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

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