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

[Peng Fan <peng.fan@nxp.com>]

> Subject: Re: [PATCH 2/3] xen/arm: Add i.MX lpuart early printk support
> 
> Hi Peng,
> 
> On 28/02/2022 01:07, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >   xen/arch/arm/Kconfig.debug              | 18 ++++++++++
> >   xen/arch/arm/arm64/debug-imx-lpuart.inc | 48
> +++++++++++++++++++++++++
> >   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..9ecb446b3a 100644
> > --- a/xen/arch/arm/Kconfig.debug
> > +++ b/xen/arch/arm/Kconfig.debug
> > @@ -55,6 +55,20 @@ choice
> >   			selecting one of the platform specific options below if
> >   			you know the parameters for the port.
> >
> > +			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.
> 
> Plaform specific early printk are deprecated. So I would rather prefer we are
> not introducing new one. Can you adjust the description to remove any
> mention of platform specific options?

Sure, fix in v2.

> 
> > +
> >   			This option is preferred over the platform specific
> >   			options; the platform specific options are deprecated
> >   			and will soon be removed.
> > @@ -186,6 +200,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 +300,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..7510210d46
> > --- /dev/null
> > +++ b/xen/arch/arm/arm64/debug-imx-lpuart.inc
> > @@ -0,0 +1,48 @@
> > +/*
> > + * xen/arch/arm/arm64/debug-imx8qm.inc
> > + *
> > + * i.MX8QM specific debug code
> > + *
> > + * Peng Fan <peng.fan@nxp.com>
> > + * Copyright (C) 2016 Freescale Inc.
> > + *
> > + * 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 <xen/imx-lpuart.h>
> > +
> > +.macro early_uart_init wb wc wd
> > +/* Already initialized in bootloader */ .endm
> 
> NIT: I would add a newline to separate with this macro from next one.

Fix in v2.

> 
> > +/* i.MX8QM wait LPUART to be ready to transmit
> > + * rb: register which contains the UART base address
> > + * rc: scratch register
> > + */
> 
> The coding style for multi-lines comment is:

Fix in v2. Thanks.

> 
> /*
>   * Foo
>   * Bar
>   */
> 
> > +.macro early_uart_ready xb, c
> > +1:
> > +        ldr   w\c, [\xb, #UARTSTAT]   /* <- Flag register */
> > +        tst   w\c, #UARTSTAT_TDRE     /* Check FIFO EMPTY bit */
> > +        beq   1b                      /* Wait for the UART to be
> ready */
> > +.endm
> > +
> > +/* i.MX8QM LPUART transmit character
> > + * rb: register which contains the UART base address
> > + * rt: register which contains the character to transmit */
> 
> Coding style:

Fix in V2.

Thanks,
Peng.
> 
> /*
>   * Foo
>   * Bar
>   */
> 
> > +.macro early_uart_transmit xb, wt
> > +        str   \wt, [\xb, #UARTDATA]  /* -> Data Register */
> > +.endm
> > +
> > +/*
> > + * Local variables:
> > + * mode: ASM
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> 
> Cheers,
> 
> --
> Julien Grall