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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 16/11/2022 14:41, Julien Grall wrote:
> 
> 
> 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.
I would then prefer to add a follow-up patch as this one is already acked
and created for a different reason.

> 
>>
>> 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).
sounds good.

> 
>> 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).
I do not really agree because the current behavior was done on purpose.
At the moment early_uart_init is called only if EARLY_UART_PL011_BAUD_RATE is 
set to a value != 0.
This is done in order to have flexibility to either stick to what 
firmware/bootloader configured or to change this
configuration by specifying the EARLY_UART_PL011_BAUD_RATE (useful when you do 
not know what
the firmware configured).

> 
> Cheers,
> 
> --
> Julien Grall

~Michal