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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Oleksandr,

On 02/04/2024 14:05, Oleksandr Tyshchenko wrote:
> 
> 
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> 
> Tested on i.MX 8M Mini only, but I guess, it should be
> suitable for other i.MX8M* SoCs (those UART device tree nodes
> contain "fsl,imx6q-uart" compatible string).
Please use imperative mood in commit msg. I would mention also that you are 
adding
macros that will be used by the runtime driver.

> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> ---
> I selected the following configs for enabling early printk:
> 
>  CONFIG_EARLY_UART_CHOICE_IMX_UART=y
>  CONFIG_EARLY_UART_IMX_UART=y
>  CONFIG_EARLY_PRINTK=y
>  CONFIG_EARLY_UART_BASE_ADDRESS=0x30890000
>  CONFIG_EARLY_PRINTK_INC="debug-imx-uart.inc"
> ---
> ---
>  xen/arch/arm/Kconfig.debug            | 14 +++++
>  xen/arch/arm/arm64/debug-imx-uart.inc | 38 ++++++++++++++
>  xen/arch/arm/include/asm/imx-uart.h   | 76 +++++++++++++++++++++++++++
>  3 files changed, 128 insertions(+)
>  create mode 100644 xen/arch/arm/arm64/debug-imx-uart.inc
>  create mode 100644 xen/arch/arm/include/asm/imx-uart.h
> 
> diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug
> index eec860e88e..a15d08f214 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_IMX_UART
> +               select EARLY_UART_IMX_UART
> +               depends on ARM_64
> +               bool "Early printk via i.MX UART"
> +               help
> +                       Say Y here if you wish the early printk to direct 
> their
Do not take example from surrounding code. help text should be indented by 2 
tabs and 2 spaces here.

> +                       output to a i.MX UART. You can use this option to
> +                       provide the parameters for the i.MX 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_IMX_UART
> +       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-imx-uart.inc" if EARLY_UART_IMX_UART
>         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-uart.inc 
> b/xen/arch/arm/arm64/debug-imx-uart.inc
> new file mode 100644
> index 0000000000..27a68b1ed5
> --- /dev/null
> +++ b/xen/arch/arm/arm64/debug-imx-uart.inc
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/arch/arm/arm64/debug-imx-uart.inc
> + *
> + * i.MX8M* specific debug code
> + *
> + * Copyright (C) 2024 EPAM Systems Inc.
> + */
> +
> +#include <asm/imx-uart.h>
> +
> +/*
> + * Wait UART to be ready to transmit
> + * rb: register which contains the UART base address
> + * rc: scratch register
> + */
> +.macro early_uart_ready xb, c
> +1:
> +        ldr   w\c, [\xb, #IMX21_UTS] /* <- Test register */
> +        tst   w\c, #UTS_TXFULL       /* Check TxFIFO FULL bit */
> +        bne   1b                     /* Wait for the UART to be ready */
> +.endm
> +
> +/*
> + * UART transmit character
> + * rb: register which contains the UART base address
> + * rt: register which contains the character to transmit
> + */
> +.macro early_uart_transmit xb, wt
> +        str   \wt, [\xb, #URTX0] /* -> Transmitter Register */
> +.endm
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/include/asm/imx-uart.h 
> b/xen/arch/arm/include/asm/imx-uart.h
> new file mode 100644
> index 0000000000..413a81dd44
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/imx-uart.h
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/arch/arm/include/asm/imx-uart.h
> + *
> + * Common constant definition between early printk and the UART driver
> + *
> + * Copyright (C) 2024 EPAM Systems Inc.
> + */
> +
> +#ifndef __ASM_ARM_IMX_UART_H__
> +#define __ASM_ARM_IMX_UART_H__
> +
> +/* 32-bit register definition */
> +#define URXD0        (0x00) /* Receiver Register */
There is no need to surround these values

> +#define URTX0        (0x40) /* Transmitter Register */
> +#define UCR1         (0x80) /* Control Register 1 */
> +#define UCR2         (0x84) /* Control Register 2 */
> +#define UCR3         (0x88) /* Control Register 3 */

> +#define UCR4         (0x8c) /* Control Register 4 */
> +#define UFCR         (0x90) /* FIFO Control Register */
> +#define USR1         (0x94) /* Status Register 1 */
> +#define USR2         (0x98) /* Status Register 2 */
> +#define IMX21_UTS    (0xb4) /* Test Register */
> +
> +#define URXD_ERR        BIT(14, UL) /* Error detect */
> +#define URXD_RX_DATA    GENMASK(7, 0) /* Received data mask */
> +
> +#define UCR1_TRDYEN      BIT(13, UL) /* Transmitter ready interrupt enable */
> +#define UCR1_RRDYEN      BIT(9, UL) /* Receiver ready interrupt enable */
NIT: please align comments within a block

Other than that:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal