| Message ID | 20230601085001.1782-1-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: debug-pl011: Use 32-bit accessors for broader compatibility | expand |
Hi Michal, > -----Original Message----- > Subject: [PATCH] xen/arm: debug-pl011: Use 32-bit accessors for broader > compatibility > > 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@amd.com> Reviewed-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry
Hi Michal, > On 1 Jun 2023, at 10:50, Michal Orzel <michal.orzel@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.orzel@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 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 ? Regards Bertrand > .endm > > /* > -- > 2.25.1 >
Hi Bertrand, On 01/06/2023 12:19, Bertrand Marquis wrote: > > > Hi Michal, > >> On 1 Jun 2023, at 10:50, Michal Orzel <michal.orzel@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.orzel@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 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). 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) - this way we support broader list of PL011s consistently (i.e. both early and runtime driver works as oppose to only runtime) 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
Hi, Sorry for the formatting. On Thu, 1 Jun 2023 at 12:31, Michal Orzel <michal.orzel@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.orzel@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.orzel@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 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 >
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@amd.com <mailto:michal.orzel@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.orzel@amd.com <mailto:michal.orzel@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.orzel@amd.com <mailto:michal.orzel@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> 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? > > > 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. 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. 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. 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). But if this is what you require, I'm somewhat forced to do so just so that our devices can be supported. ~Michal
On Thu, 1 Jun 2023 at 13:48, Michal Orzel <michal.orzel@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.orzel@amd.com <mailto: > michal.orzel@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.orzel@amd.com > <mailto:michal.orzel@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.orzel@amd.com <mailto: > michal.orzel@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> 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 >
On 01/06/2023 14:17, Julien Grall wrote: > > > > > > On Thu, 1 Jun 2023 at 13:48, Michal Orzel <michal.orzel@amd.com <mailto:michal.orzel@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.orzel@amd.com <mailto:michal.orzel@amd.com> <mailto:michal.orzel@amd.com <mailto:michal.orzel@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.orzel@amd.com <mailto:michal.orzel@amd.com> <mailto:michal.orzel@amd.com <mailto:michal.orzel@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.orzel@amd.com <mailto:michal.orzel@amd.com> <mailto:michal.orzel@amd.com <mailto:michal.orzel@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
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@amd.com <mailto:michal.orzel@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.orzel@amd.com <mailto:michal.orzel@amd.com> <mailto:michal.orzel@amd.com <mailto:michal.orzel@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.orzel@amd.com <mailto:michal.orzel@amd.com> <mailto:michal.orzel@amd.com <mailto:michal.orzel@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.orzel@amd.com <mailto:michal.orzel@amd.com> <mailto:michal.orzel@amd.com <mailto:michal.orzel@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. 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. Cheers,
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@amd.com <mailto:michal.orzel@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.orzel@amd.com <mailto:michal.orzel@amd.com> <mailto:michal.orzel@amd.com <mailto:michal.orzel@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.orzel@amd.com <mailto:michal.orzel@amd.com> <mailto:michal.orzel@amd.com <mailto:michal.orzel@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.orzel@amd.com <mailto:michal.orzel@amd.com> <mailto:michal.orzel@amd.com <mailto:michal.orzel@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. > > 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
On 06/06/2023 08:04, Michal Orzel wrote: > Hi Julien, Hi Michal, > On 05/06/2023 20:34, Julien Grall wrote: > 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. I would be fine with introduce PL011_STRH & co. > > 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. Make senses. Cheers,
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) */ .endm /*
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@amd.com> --- xen/arch/arm/arm64/debug-pl011.inc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)