Re: [PATCH 1/3] xen/char: add classic i.MX UART driver

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 14-Aug-26 18:25, Wig Cheng wrote:
> Add a console driver for the classic i.MX UART IP ("fsl,imx6q-uart"
> compatible), as found on the i.MX6/7/8M families.  Baudrate and pin
You often mention i.MX 6 and 7 but guard the driver on Arm64. Please do not
mention them if you only intend to support/test i.MX 8.

> configuration are inherited from the bootloader; the driver only
> enables the transmitter/receiver and wires up the RX/TX interrupts,
> mirroring the existing imx-lpuart driver.
> 
> This is needed for the i.MX8M family, whose UART IP differs from the
> LPUART used on i.MX8QM/8QXP.
> 
> Signed-off-by: Wig Cheng <onlywig@xxxxxxxxx>
> ---
>  xen/arch/arm/include/asm/imx-uart.h |  62 ++++++++
>  xen/drivers/char/Kconfig            |   8 +
>  xen/drivers/char/Makefile           |   1 +
>  xen/drivers/char/imx-uart.c         | 227 ++++++++++++++++++++++++++++
Please add entry to MAINTAINERS for this file under ARM

>  4 files changed, 298 insertions(+)
>  create mode 100644 xen/arch/arm/include/asm/imx-uart.h
>  create mode 100644 xen/drivers/char/imx-uart.c
> 
> 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..ad4b0b06ff
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/imx-uart.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
Can this be GPL-2.0 only?
> +/*
> + * xen/arch/arm/include/asm/imx-uart.h
This can go stale. Please drop.

> + *
> + * Register definitions for the classic i.MX UART IP
> + * ("fsl,imx6q-uart" compatible, used on i.MX6/7/8M families).
> + *
> + * Register layout taken from Linux drivers/tty/serial/imx.c.
> + *
> + * Copyright 2026 Open-EP (E-Paper) Community
> + */
> +
> +#ifndef __ASM_ARM_IMX_UART_H__
Should be ASM_IMX_UART_H

> +#define __ASM_ARM_IMX_UART_H__
> +
> +#define URXD0           0x00   /* Receiver Register */
> +#define URTX0           0x40   /* Transmitter Register */
> +#define UCR1            0x80   /* Control Register 1 */
> +#define UCR2            0x84   /* Control Register 2 */
> +#define UCR3            0x88   /* Control Register 3 */
Given that this is not a verbatim 1:1 copy from Linux (no need for it to be),
please do not define macros that are unused.

> +#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 UTS             0xb4   /* Test Register */
> +
> +#define URXD_CHARRDY    (1U << 15)
Please use BIT(n, U)

> +#define URXD_RX_DATA    0xff
> +
> +#define UCR1_UARTEN     (1U << 0)
> +#define UCR1_RRDYEN     (1U << 9)   /* Receiver ready interrupt enable */
> +#define UCR1_TRDYEN     (1U << 13)  /* Transmitter ready interrupt enable */
> +#define UCR1_RXDMAEN    (1U << 8)
> +#define UCR1_TXDMAEN    (1U << 3)
> +#define UCR1_ATDMAEN    (1U << 2)
> +
> +#define UCR2_SRST       (1U << 0)   /* 0 = issue software reset */
> +#define UCR2_RXEN       (1U << 1)
> +#define UCR2_TXEN       (1U << 2)
> +
> +#define USR1_RRDY       (1U << 9)   /* Receiver ready */
> +#define USR1_TRDY       (1U << 13)  /* Transmitter ready */
> +
> +#define USR2_RDR        (1U << 0)   /* Receive data ready */
> +#define USR2_ORE        (1U << 1)   /* Overrun error */
> +#define USR2_TXDC       (1U << 3)   /* Transmission complete */
> +#define USR2_TXFE       (1U << 14)  /* Transmit FIFO empty */
> +
> +#define UTS_TXFULL      (1U << 4)
> +#define UTS_RXEMPTY     (1U << 5)
> +#define UTS_TXEMPTY     (1U << 6)
> +
> +#endif /* __ASM_ARM_IMX_UART_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index 8e49a52c73..f237c0220d 100644
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -30,6 +30,14 @@ config HAS_IMX_LPUART
>       help
>         This selects the i.MX LPUART. If you have i.MX8QM based board, say Y.
>  
> +config HAS_IMX_UART
> +     bool "i.MX UART driver"
> +     default y
> +     depends on ARM_64
> +     help
> +       This selects the classic i.MX UART. If you have an i.MX8M family
> +       based board, say Y.
> +
>  config HAS_MVEBU
>       bool "Marvell MVEBU UART driver"
>       default y
> diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
> index 8cbbffdca8..039f566926 100644
> --- a/xen/drivers/char/Makefile
> +++ b/xen/drivers/char/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_HAS_SCIF) += scif-uart.o
>  obj-$(CONFIG_HAS_EHCI) += ehci-dbgp.o
>  obj-$(CONFIG_XHCI) += xhci-dbc.o
>  obj-$(CONFIG_HAS_IMX_LPUART) += imx-lpuart.o
> +obj-$(CONFIG_HAS_IMX_UART) += imx-uart.o
>  obj-$(CONFIG_HAS_LINFLEX) += linflex-uart.o
>  obj-$(CONFIG_GENERIC_UART_INIT) += uart-init.o
>  obj-y += serial.o
> diff --git a/xen/drivers/char/imx-uart.c b/xen/drivers/char/imx-uart.c
> new file mode 100644
> index 0000000000..5ae8c13c40
> --- /dev/null
> +++ b/xen/drivers/char/imx-uart.c
> @@ -0,0 +1,227 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/drivers/char/imx-uart.c
This can go stale. Please drop.

> + *
> + * Driver for the classic i.MX UART IP ("fsl,imx6q-uart"), as found on
> + * the i.MX6/7/8M families (e.g. i.MX8MP).
> + *
> + * Baudrate and pin configuration are inherited from the bootloader.
> + *
> + * Copyright 2026 Open-EP (E-Paper) Community
> + */
> +
> +#include <xen/errno.h>
> +#include <xen/init.h>
> +#include <xen/irq.h>
> +#include <xen/mm.h>
> +#include <xen/serial.h>
> +#include <asm/device.h>
> +#include <asm/imx-uart.h>
> +#include <asm/io.h>
> +
> +#define imx_uart_read(uart, off)       readl((uart)->regs + (off))
> +#define imx_uart_write(uart, off, val) writel((val), (uart)->regs + (off))
> +
> +static struct imx_uart {
> +    uint32_t irq;
> +    char __iomem *regs;
> +    struct irqaction irqaction;
> +    struct vuart_info vuart;
> +} imx8m_com;
> +
> +static void imx_uart_interrupt(int irq, void *data)
> +{
> +    struct serial_port *port = data;
> +    struct imx_uart *uart = port->uart;
> +
> +    if ( imx_uart_read(uart, USR2) & USR2_RDR )
> +        serial_rx_interrupt(port);
> +
> +    if ( imx_uart_read(uart, USR1) & USR1_TRDY )
> +        serial_tx_interrupt(port);
> +}
> +
> +static void __init imx_uart_init_preirq(struct serial_port *port)
> +{
> +    struct imx_uart *uart = port->uart;
> +    uint32_t ucr1, ucr2;
> +
> +    /*
> +     * Reuse the bootloader baudrate/format settings: only make sure the
> +     * UART and both directions are enabled, DMA and interrupts are off.
> +     */
> +    ucr1 = imx_uart_read(uart, UCR1);
> +    ucr1 &= ~(UCR1_RRDYEN | UCR1_TRDYEN | UCR1_RXDMAEN | UCR1_TXDMAEN |
> +              UCR1_ATDMAEN);
What about TXMPTYEN?

> +    ucr1 |= UCR1_UARTEN;
> +    imx_uart_write(uart, UCR1, ucr1);
Only UCR1's enables are cleared, while other UCRs interrupt enables keep
whatever the bootloader left. Either mention UCR1 only or clear others too.

~Michal