Re: [PATCH V4 2/2] xen/arm: Add i.MX lpuart early printk support

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 11.04.2022 12:23, Julien Grall wrote:
> Hi,
> 
> On 11/04/2022 07:24, Michal Orzel wrote:
>> On 08.04.2022 16:09, Peng Fan (OSS) wrote:
>>> From: Peng Fan <peng.fan@xxxxxxx>
>>>
>>> Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
>>> ---
>>>   xen/arch/arm/Kconfig.debug              | 14 +++++++
>>>   xen/arch/arm/arm64/debug-imx-lpuart.inc | 52 +++++++++++++++++++++++++
>>>   2 files changed, 66 insertions(+)
>>>   create mode 100644 xen/arch/arm/arm64/debug-imx-lpuart.inc
>>>
>>> diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug
>>> index 35ccd13273..842d768280 100644
>>> --- a/xen/arch/arm/Kconfig.debug
>>> +++ b/xen/arch/arm/Kconfig.debug
>>> @@ -58,6 +58,16 @@ choice
>>>               This option is preferred over the platform specific
>>>               options; the platform specific options are deprecated
>>>               and will soon be removed.
>>> +    config EARLY_UART_CHOICE_IMX_LPUART
>>> +        select EARLY_UART_IMX_LPUART
>>> +        depends on ARM_64
>>> +        bool "Early printk via i.MX LPUART"
>>> +        help
>>> +            Say Y here if you wish the early printk to direct their
>>> +            output to a i.MX LPUART. You can use this option to
>>> +            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_MESON
>>>           select EARLY_UART_MESON
>>>           depends on ARM_64
>>> @@ -186,6 +196,9 @@ config EARLY_UART_CADENCE
>>>   config EARLY_UART_EXYNOS4210
>>>       select EARLY_PRINTK
>>>       bool
>>> +config EARLY_UART_IMX_LPUART
>>> +    select EARLY_PRINTK
>>> +    bool
>>>   config EARLY_UART_MESON
>>>       select EARLY_PRINTK
>>>       bool
>>> @@ -283,6 +296,7 @@ config EARLY_PRINTK_INC
>>>       default "debug-8250.inc" if EARLY_UART_8250
>>>       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-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-imx-lpuart.inc 
>>> b/xen/arch/arm/arm64/debug-imx-lpuart.inc
>>> new file mode 100644
>>> index 0000000000..f68252da86
>>> --- /dev/null
>>> +++ b/xen/arch/arm/arm64/debug-imx-lpuart.inc
>>> @@ -0,0 +1,52 @@
>>> +/*
>>> + * xen/arch/arm/arm64/debug-imx-lpuart.inc
>>> + *
>>> + * i.MX8QM specific debug code
>>> + *
>>> + * Peng Fan <peng.fan@xxxxxxx>
>>> + * Copyright 2022 NXP
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <asm/imx-lpuart.h>
>>> +
>>> +.macro early_uart_init wb wc wd
>>> +/* Already initialized in bootloader */
>>> +.endm
>>> +
>> FWICS, early_uart_init call is protected with CONFIG_EARLY_UART_INIT that is 
>> set
>> only for PL011 UART. So I think there is no need to implement this empty 
>> macro,
>> unless we want to keep the same macros across the files no matter whether in 
>> use or not.
>> I think you need to ask Julien.
> 
> The call to early_uart_init is indeed protected by #ifdef 
> CONFIG_EARLY_UART_INIT. The goal was to avoid having all debug-*.inc to 
> implement dummy helper.
> 
> So I would prefer if early_uart_init is not implemented for 
> debug-imx-lpuart.inc.
> 
> Cheers,
> 

I agree.
As a side note, do we still need to support platform specific options for early 
printk?
If not, I am happy to send a patch removing them.

Cheers,
Michal