Re: [PATCH] xen/arm: debug-pl011.inc: Use macros instead of hardcoded values

[Julien Grall <julien@xxxxxxx>]

On 16/11/2022 08:05, Michal Orzel wrote:
> On 16/11/2022 00:10, Julien Grall wrote:
> > Hi Michal,
> >
> > On 24/10/2022 11:05, Michal Orzel wrote:
> > > Make use of the macros defined in asm/pl011-uart.h instead of hardcoding
> > > the values. Also, take the opportunity to fix the file extension in a
> > > top-level comment.
> > >
> > > No functional change intended.
> > >
> > > Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> > With one comment below:
> >
> > Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > > ---
> > >    xen/arch/arm/arm64/debug-pl011.inc | 20 +++++++++++---------
> > >    1 file changed, 11 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/arm64/debug-pl011.inc 
> > > b/xen/arch/arm/arm64/debug-pl011.inc
> > > index 1928a2e3ffbb..d82f2f1de197 100644
> > > --- a/xen/arch/arm/arm64/debug-pl011.inc
> > > +++ b/xen/arch/arm/arm64/debug-pl011.inc
> > > @@ -1,5 +1,5 @@
> > >    /*
> > > - * xen/arch/arm/arm64/debug-pl011.S
> > > + * xen/arch/arm/arm64/debug-pl011.inc
> > >     *
> > >     * PL011 specific debug code
> > >     *
> > > @@ -16,6 +16,8 @@
> > >     * GNU General Public License for more details.
> > >     */
> > >
> > > + #include <asm/pl011-uart.h>
> > > +
> > >    /*
> > >     * PL011 UART initialization
> > >     * xb: register which containts the UART base address
> > > @@ -23,13 +25,13 @@
> > >     */
> > >    .macro early_uart_init xb, c
> > >            mov   x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE % 16)
> > > -        strh  w\c, [\xb, #0x28]      /* -> UARTFBRD (Baud divisor fraction) */
> > > +        strh  w\c, [\xb, #FBRD]      /* -> UARTFBRD (Baud divisor fraction) */
> > >            mov   x\c, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE / 16)
> > > -        strh  w\c, [\xb, #0x24]      /* -> UARTIBRD (Baud divisor integer) */
> > > +        strh  w\c, [\xb, #IBRD]      /* -> UARTIBRD (Baud divisor integer) */
> > >            mov   x\c, #0x60             /* 8n1 */
> > Can we introduce macro/define for 0x60?
> We could but I think this should not be part of this patch.
> The reason being, the arm32 code also uses hardcoded 0x60 so it should be 
> changed as well.
> I can create a prerequisite patch introducing the macro and changing the arm32 
> code first unless you prefer to have everything in a single patch.
I am fine with either prerequisite or a follow-up to define a macro and 
use it in both arm32/arm64.
> As for the macro itself, because 8n1 only requires setting bits for WLEN (1 
> stop bit and no parity are 0 by default), we can do
> the following in pl011-uart.h:
> #define WLEN_8 0x60
I think it would be clearer and easier to check the spec if the value is 
(_AC(0x3, U) << 5).
> and use it in the pl011-debug files (+ there is a question whether we should 
> define WLEN_7-5 for completeness).
I would not define WLEN_7-5. That said, I wonder if we really need to 
set the baud rate & co here?
AFAICT the runtime driver never touch them. The reasoning is the 
firmware is responsible to configure the serial. Therefore, I would 
consider to drop the code (setting UARTCR might still be necessary).
Cheers,

--
Julien Grall