On 01/06/2023 14:17, Julien Grall wrote: > > > > > > On Thu, 1 Jun 2023 at 13:48, Michal Orzel <michal.or...@amd.com > <mailto:michal.or...@amd.com>> 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.or...@amd.com > <mailto:michal.or...@amd.com> <mailto:michal.or...@amd.com > <mailto:michal.or...@amd.com>>> wrote: > > > > Hi Bertrand, > > > > On 01/06/2023 12:19, Bertrand Marquis wrote: > > > > > > > > > Hi Michal, > > > > > >> On 1 Jun 2023, at 10:50, Michal Orzel <michal.or...@amd.com > <mailto:michal.or...@amd.com> <mailto:michal.or...@amd.com > <mailto:michal.or...@amd.com>>> 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.or...@amd.com > <mailto:michal.or...@amd.com> <mailto:michal.or...@amd.com > <mailto:michal.or...@amd.com>>> > > >> --- > > >> 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
