[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] xen/arm: debug-pl011: Use 32-bit accessors for broader compatibility
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?
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.
But if this is what you require, I'm somewhat forced to do so just so that our devices can be supported.
See above.
~Michal
|
