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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

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.

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
and use it in the pl011-debug files (+ there is a question whether we should 
define WLEN_7-5 for completeness).


> 
>> -        str   w\c, [\xb, #0x2C]      /* -> UARTLCR_H (Line control) */
>> -        ldr   x\c, =0x00000301       /* RXE | TXE | UARTEN */
>> -        str   w\c, [\xb, #0x30]      /* -> UARTCR (Control Register) */
>> +        str   w\c, [\xb, #LCR_H]     /* -> UARTLCR_H (Line control) */
>> +        ldr   x\c, =(RXE | TXE | UARTEN)
>> +        str   w\c, [\xb, #CR]        /* -> UARTCR (Control Register) */
>>   .endm
> 
> Cheers,
> 
> --
> Julien Grall

~Michal