Re: [PATCH v1 2/5] xen/arm: Add NXP LINFlexD UART early printk support

[Andrei Cherechesu <andrei.cherechesu@xxxxxxxxxxx>]

On 11/09/2024 00:58, Julien Grall wrote:
> Hi,
>
> On 10/09/2024 15:34, Andrei Cherechesu (OSS) wrote:
>> From: Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
>>
>> This adds support for early printk debug via the NXP LINFlexD
>> UART controller.
>>
>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
>> Signed-off-by: Peter van der Perk <peter.vander.perk@xxxxxxx>
>> ---
>>   xen/arch/arm/Kconfig.debug           | 14 +++++++
>>   xen/arch/arm/arm64/debug-linflex.inc | 58 ++++++++++++++++++++++++++++
>>   2 files changed, 72 insertions(+)
>>   create mode 100644 xen/arch/arm/arm64/debug-linflex.inc
>>
>> diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug
>> index eec860e88e..a309f67f90 100644
>> --- a/xen/arch/arm/Kconfig.debug
>> +++ b/xen/arch/arm/Kconfig.debug
>> @@ -68,6 +68,16 @@ choice
>>               provide the parameters for the i.MX LPUART rather than
>>               selecting one of the platform specific options below if
>>               you know the parameters for the port.
>> +    config EARLY_UART_CHOICE_LINFLEX
>> +        select EARLY_UART_LINFLEX
>> +        depends on ARM_64
>> +        bool "Early printk via NXP LINFlexD UART"
>> +        help
>> +            Say Y here if you wish the early printk to direct their
>> +            output to an NXP LINFlexD UART. You can use this option to
>> +            provide the parameters for the NXP LINFlexD UART rather than
>> +            selecting one of the platform specific options below if
>> +            you know the parameters for the port.
>>       config EARLY_UART_CHOICE_MESON
>>           select EARLY_UART_MESON
>>           depends on ARM_64
>> @@ -199,6 +209,9 @@ config EARLY_UART_EXYNOS4210
>>   config EARLY_UART_IMX_LPUART
>>       select EARLY_PRINTK
>>       bool
>> +config EARLY_UART_LINFLEX
>> +    select EARLY_PRINTK
>> +    bool
>>   config EARLY_UART_MESON
>>       select EARLY_PRINTK
>>       bool
>> @@ -304,6 +317,7 @@ config EARLY_PRINTK_INC
>>       default "debug-cadence.inc" if EARLY_UART_CADENCE
>>       default "debug-exynos4210.inc" if EARLY_UART_EXYNOS4210
>>       default "debug-imx-lpuart.inc" if EARLY_UART_IMX_LPUART
>> +    default "debug-linflex.inc" if EARLY_UART_LINFLEX
>>       default "debug-meson.inc" if EARLY_UART_MESON
>>       default "debug-mvebu.inc" if EARLY_UART_MVEBU
>>       default "debug-pl011.inc" if EARLY_UART_PL011
>> diff --git a/xen/arch/arm/arm64/debug-linflex.inc 
>> b/xen/arch/arm/arm64/debug-linflex.inc
>> new file mode 100644
>> index 0000000000..6ed704e104
>> --- /dev/null
>> +++ b/xen/arch/arm/arm64/debug-linflex.inc
>> @@ -0,0 +1,58 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>
> GPL-2.0 is deprecated and replaced by GPL-2.0-only.
Will fix in v2.
>
>> +/*
>> + * xen/arch/arm/arm64/debug-linflex.inc
>> + *
>> + * NXP LINFlexD UART specific debug code
>> + *
>> + * Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
>> + * Copyright 2018, 2021, 2023-2024 NXP
>> + */
>> +
>> +#include <asm/asm_defns.h>
>> +#include <asm/linflex-uart.h>
>> +
>> +/*
>> + * wait LINFlexD UART to be ready to transmit
>> + * xb: register which contains the UART base address
>> + * c: scratch register number
>> + */
>> +.macro early_uart_ready xb, c
>> +    ldr   w\c, [\xb, #UARTCR]       /* <= Control Register */
>> +    and   w\c, w\c, #UARTCR_TFBM    /* Check Buffer/FIFO (0/1) Mode */
>> +    cbz   w\c, 2f                   /* Buffer Mode => return */
>> +1:
>> +    ldrb  w\c, [\xb, #UARTSR]       /* <= Status Register */
>> +    tst   w\c, #UARTSR_DTFTFF       /* FIFO Mode => Check DTF bit */
>> +    b.ne  1b
>> +2:
>> +.endm
>> +
>> +/*
>> + * LINFlexD UART transmit character
>> + * xb: register which contains the UART base address
>> + * wt: register which contains the character to transmit
>> + */
>> +.macro early_uart_transmit xb, wt
>> +    strb  \wt, [\xb, #BDRL]
>> +
>> +    ldr   \wt, [\xb, #UARTCR]       /* <= Control Register */
>> +    and   \wt, \wt, #UARTCR_TFBM    /* Check Buffer/FIFO (0/1) Mode */
>> +    cbnz  \wt, 2f                   /* FIFO Mode => goto exit */
>> +
>> +3:  /* Buffer Mode */
>> +    ldrb  \wt, [\xb, #UARTSR]       /* <= Status Register */
>> +    and   \wt, \wt, #UARTSR_DTFTFF  /* Check Transmission Completed */
>> +    cbz   \wt, 3b
>> +
>> +    ldr   \wt, [\xb, #UARTSR]       /* <= Status Register */
>> +    orr   \wt, \wt, #UARTSR_DTFTFF  /* Clear DTF bit */
>> +    str   \wt, [\xb, #UARTSR]
>> +2:
>> +.endm
>> +
>> +/*
>> + * Local variables:
>> + * mode: ASM
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> \ No newline at end of file
>
> The file should end with a newline.
>
Will fix in v2.
> I haven't looked at the specifics of the UART. But the code integration in 
> Xen LGTM. So with the two comments addressed:
>
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
>
> Cheers,
>
Thanks for the review!

Regards,
Andrei C