[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,
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.
Cheers,
--
Julien Grall
|
