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

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

On 18-Aug-26 04:52, Wig Cheng wrote:
> Add a console driver for the classic i.MX UART IP ("fsl,imx6q-uart"
> compatible), used as the console UART on the i.MX8M family.  Baudrate
> and pin 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.
> 
> The i.MX8M family's UART IP differs from the LPUART used on
> i.MX8QM/8QXP, so a separate driver is needed.
> 
> Signed-off-by: Wig Cheng <onlywig@gmail.com>
> ---
>  xen/arch/arm/include/asm/imx-uart.h |  57 +++++++
>  xen/drivers/char/Kconfig            |   8 +
>  xen/drivers/char/Makefile           |   1 +
>  xen/drivers/char/imx-uart.c         | 226 ++++++++++++++++++++++++++++
You should add an entry to the MAINTAINERS file for imx-uart.c so that it falls
down under ARM maintainership. Your last patch makes you a reviewer but we still
need to be maintainers of it. See how it was done for IMX8QM.

>  4 files changed, 292 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..a3892020e6
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/imx-uart.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Register definitions for the classic i.MX UART IP
> + * ("fsl,imx6q-uart" compatible), used as the console UART on the
> + * i.MX8M family.
> + *
> + * Register layout taken from Linux drivers/tty/serial/imx.c.
> + *
> + * Copyright 2026 Open-EP (E-Paper) Community
> + */
> +
> +#ifndef ASM_IMX_UART_H
> +#define ASM_IMX_UART_H
> +
> +#include <xen/const.h>
> +
> +#define URXD0           0x00   /* Receiver Register */
> +#define URTX0           0x40   /* Transmitter Register */
> +#define UCR1            0x80   /* Control Register 1 */
> +#define UCR2            0x84   /* Control Register 2 */
> +#define USR1            0x94   /* Status Register 1 */
> +#define USR2            0x98   /* Status Register 2 */
> +#define UTS             0xb4   /* Test Register */
> +
> +#define URXD_RX_DATA    0xff
> +
> +#define UCR1_UARTEN     BIT(0, U)   /* UART enable */
> +#define UCR1_ATDMAEN    BIT(2, U)   /* Aging DMA timer enable */
> +#define UCR1_TXDMAEN    BIT(3, U)   /* Transmitter ready DMA enable */
> +#define UCR1_TXMPTYEN   BIT(6, U)   /* Transmitter empty interrupt enable */
> +#define UCR1_RXDMAEN    BIT(8, U)   /* Receiver ready DMA enable */
> +#define UCR1_RRDYEN     BIT(9, U)   /* Receiver ready interrupt enable */
> +#define UCR1_TRDYEN     BIT(13, U)  /* Transmitter ready interrupt enable */
> +
> +#define UCR2_SRST       BIT(0, U)   /* 0 = issue software reset */
> +#define UCR2_RXEN       BIT(1, U)   /* Receiver enable */
> +#define UCR2_TXEN       BIT(2, U)   /* Transmitter enable */
> +
> +#define USR1_TRDY       BIT(13, U)  /* Transmitter ready */
> +
> +#define USR2_RDR        BIT(0, U)   /* Receive data ready */
> +#define USR2_ORE        BIT(1, U)   /* Overrun error */
> +
> +#define UTS_TXFULL      BIT(4, U)   /* TX FIFO full */
> +#define UTS_RXEMPTY     BIT(5, U)   /* RX FIFO empty */
> +#define UTS_TXEMPTY     BIT(6, U)   /* TX FIFO empty */
> +
> +#endif /* ASM_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..fd34b0cd11
> --- /dev/null
> +++ b/xen/drivers/char/imx-uart.c
> @@ -0,0 +1,226 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Driver for the classic i.MX UART IP ("fsl,imx6q-uart"), used as the
> + * console UART on the i.MX8M family (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 )
Looking at Linux's imx.c you should clear TRDY if TRDEN is not enabled to
prevent RX interrupt entering serial_tx_interrupt as TRDY is a raw status register.

> +        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 settings; only enable the UART and both
> +     * directions.  The console uses UCR1 interrupts (RRDYEN/TRDYEN)
> +     * exclusively, so just clear UCR1's interrupt and DMA enables.
> +     */
> +    ucr1 = imx_uart_read(uart, UCR1);
> +    ucr1 &= ~(UCR1_RRDYEN | UCR1_TRDYEN | UCR1_TXMPTYEN | UCR1_RXDMAEN |
> +              UCR1_TXDMAEN | UCR1_ATDMAEN);
> +    ucr1 |= UCR1_UARTEN;
> +    imx_uart_write(uart, UCR1, ucr1);
> +
> +    ucr2 = imx_uart_read(uart, UCR2);
> +    ucr2 |= UCR2_SRST | UCR2_RXEN | UCR2_TXEN;
> +    imx_uart_write(uart, UCR2, ucr2);
> +}
> +
> +static void __init imx_uart_init_postirq(struct serial_port *port)
> +{
> +    struct imx_uart *uart = port->uart;
> +    uint32_t ucr1;
> +
> +    uart->irqaction.handler = imx_uart_interrupt;
> +    uart->irqaction.name = "imx_uart";
> +    uart->irqaction.dev_id = port;
> +
> +    if ( setup_irq(uart->irq, 0, &uart->irqaction) != 0 )
> +    {
> +        dprintk(XENLOG_ERR, "Failed to allocate imx_uart IRQ %d\n", uart->irq);
uart->irq is unsigned, so s/%d/%u.

~Michal