Re: [PATCH 2/2] xen/arm: Add i.MX UART driver

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Oleksandr,

On 02/04/2024 14:05, Oleksandr Tyshchenko wrote:
> 
> 
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> 
> The i.MX UART Documentation:
> https://www.nxp.com/webapp/Download?colCode=IMX8MMRM
> Chapter 16.2 Universal Asynchronous Receiver/Transmitter (UART)
> 
> Tested on i.MX 8M Mini only, but I guess, it should be
imperative mood

> suitable for other i.MX8M* SoCs (those UART device tree nodes
> contain "fsl,imx6q-uart" compatible string).
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> ---
> I used the "earlycon=ec_imx6q,0x30890000" cmd arg and
> selected CONFIG_SERIAL_IMX_EARLYCON in Linux for enabling vUART.
> ---
> ---
>  MAINTAINERS                 |   1 +
>  xen/drivers/char/Kconfig    |   7 +
>  xen/drivers/char/Makefile   |   1 +
>  xen/drivers/char/imx-uart.c | 299 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 308 insertions(+)
>  create mode 100644 xen/drivers/char/imx-uart.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1bd22fd75f..bd4084fd20 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -249,6 +249,7 @@ F:  xen/drivers/char/arm-uart.c
>  F:     xen/drivers/char/cadence-uart.c
>  F:     xen/drivers/char/exynos4210-uart.c
>  F:     xen/drivers/char/imx-lpuart.c
> +F:     xen/drivers/char/imx-uart.c
>  F:     xen/drivers/char/meson-uart.c
>  F:     xen/drivers/char/mvebu-uart.c
>  F:     xen/drivers/char/omap-uart.c
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index e18ec3788c..f51a1f596a 100644
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -20,6 +20,13 @@ 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 i.MX UART. If you have i.MX8M* 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 e7e374775d..147530a1ed 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_ARM) += arm-uart.o
>  obj-y += serial.o
>  obj-$(CONFIG_XEN_GUEST) += xen_pv_console.o
> diff --git a/xen/drivers/char/imx-uart.c b/xen/drivers/char/imx-uart.c
> new file mode 100644
> index 0000000000..13bb189063
> --- /dev/null
> +++ b/xen/drivers/char/imx-uart.c
> @@ -0,0 +1,299 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
Can it be GPL-2.0-only? Some companies are worried about v3.

> +/*
> + * xen/drivers/char/imx-uart.c
> + *
> + * Driver for i.MX UART.
> + *
> + * Based on Linux's drivers/tty/serial/imx.c
> + *
> + * Copyright (C) 2024 EPAM Systems Inc.
> + */
> +
> +#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 baud, clock_hz, data_bits, parity, stop_bits, fifo_size;
What's the use of these variables? AFAICT they are set but never used.

> +    uint32_t irq;
unsigned int

> +    char __iomem *regs;
> +    struct irqaction irqaction;
> +    struct vuart_info vuart;
> +} imx_com;
> +
> +static void imx_uart_interrupt(int irq, void *data)
> +{
> +    struct serial_port *port = data;
> +    struct imx_uart *uart = port->uart;
> +    uint32_t usr1, usr2;
> +
> +    usr1 = imx_uart_read(uart, USR1);
> +    usr2 = imx_uart_read(uart, USR2);
> +
> +    if ( usr1 & (USR1_RRDY | USR1_AGTIM) )
> +    {
> +        imx_uart_write(uart, USR1, USR1_AGTIM);
> +        serial_rx_interrupt(port);
> +    }
> +
> +    if ( (usr1 & USR1_TRDY) || (usr2 & USR2_TXDC) )
> +        serial_tx_interrupt(port);
> +}
> +
> +static void imx_uart_clear_rx_errors(struct serial_port *port)
> +{
> +    struct imx_uart *uart = port->uart;
> +    uint32_t usr1, usr2;
> +
> +    usr1 = imx_uart_read(uart, USR1);
> +    usr2 = imx_uart_read(uart, USR2);
> +
> +    if ( usr2 & USR2_BRCD )
> +        imx_uart_write(uart, USR2, USR2_BRCD);
> +    else if ( usr1 & USR1_FRAMERR )
> +        imx_uart_write(uart, USR1, USR1_FRAMERR);
> +    else if ( usr1 & USR1_PARITYERR )
> +        imx_uart_write(uart, USR1, USR1_PARITYERR);
> +
> +    if ( usr2 & USR2_ORE )
> +        imx_uart_write(uart, USR2, USR2_ORE);
> +}
> +
> +static void __init imx_uart_init_preirq(struct serial_port *port)
> +{
> +    struct imx_uart *uart = port->uart;
> +    uint32_t reg;
> +
> +    /*
> +     * Wait for the transmission to complete. This is needed for a smooth
> +     * transition when we come from early printk.
> +     */
> +    while ( !(imx_uart_read(uart, USR2) & USR2_TXDC) )
> +        cpu_relax();
> +
> +    /* Set receiver/transmitter trigger level */
> +    reg = imx_uart_read(uart, UFCR);
> +    reg &= (UFCR_RFDIV | UFCR_DCEDTE);
> +    reg |= TXTL_DEFAULT << UFCR_TXTL_SHF | RXTL_DEFAULT;
please surround TXTL_DEFAULT << UFCR_TXTL_SHF with parentheses

> +    imx_uart_write(uart, UFCR, reg);
> +
> +    /* Enable UART and disable interrupts/DMA */
> +    reg = imx_uart_read(uart, UCR1);
> +    reg |= UCR1_UARTEN;
> +    reg &= ~(UCR1_TRDYEN | UCR1_TXMPTYEN | UCR1_RTSDEN | UCR1_RRDYEN |
> +             UCR1_RXDMAEN | UCR1_TXDMAEN | UCR1_ATDMAEN);
> +    imx_uart_write(uart, UCR1, reg);
> +
> +    /* Enable receiver/transmitter and disable Aging Timer */
> +    reg = imx_uart_read(uart, UCR2);
> +    reg |= UCR2_RXEN | UCR2_TXEN;
> +    reg &= ~UCR2_ATEN;
> +    imx_uart_write(uart, UCR2, reg);
> +
> +    /* Disable interrupts */
> +    reg = imx_uart_read(uart, UCR4);
> +    reg &= ~(UCR4_TCEN | UCR4_DREN);
> +    imx_uart_write(uart, UCR4, reg);
> +}
> +
> +static void __init imx_uart_init_postirq(struct serial_port *port)
> +{
> +    struct imx_uart *uart = port->uart;
> +
> +    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);
Wrong format specifier for unsigned, use %u
Also, why dprintk?

> +        return;
> +    }
> +
> +    /* Clear possible receiver errors */
> +    imx_uart_clear_rx_errors(port);
> +
> +    /* Enable interrupts */
> +    imx_uart_write(uart, UCR1, imx_uart_read(uart, UCR1) |
> +                   UCR1_RRDYEN | UCR1_TRDYEN);
> +    imx_uart_write(uart, UCR2, imx_uart_read(uart, UCR2) | UCR2_ATEN);
> +}
> +
> +static void imx_uart_suspend(struct serial_port *port)
> +{
> +    BUG();
> +}
no need for this function

> +
> +static void imx_uart_resume(struct serial_port *port)
> +{
> +    BUG();
> +}
no need for this function

> +
> +static int imx_uart_tx_ready(struct serial_port *port)
> +{
> +    struct imx_uart *uart = port->uart;
can be const

> +    uint32_t reg;
> +
> +    reg = imx_uart_read(uart, IMX21_UTS);
> +    if ( reg & UTS_TXEMPTY )
> +        return TX_FIFO_SIZE;
empty line here please

> +    if ( reg & UTS_TXFULL )
> +        return 0;
> +
> +    /*
> +     * If the FIFO is neither full nor empty then there is a space for
> +     * one char at least.
> +     */
> +    return 1;
> +}
> +
> +static void imx_uart_putc(struct serial_port *port, char c)
> +{
> +    struct imx_uart *uart = port->uart;
can be const

> +
> +    while ( imx_uart_read(uart, IMX21_UTS) & UTS_TXFULL )
> +        cpu_relax();
Looking at pl011, cdns, putc contains only a write to data register. This is 
because
->putc always follows ->tx_ready which contains the check for how many chars 
can be written.
Therefore, AFAICT this should be dropped.

> +
> +    imx_uart_write(uart, URTX0, c);
> +}
> +
> +static int imx_uart_getc(struct serial_port *port, char *pc)
> +{
> +    struct imx_uart *uart = port->uart;
can be const

> +    uint32_t data;
> +
> +    if ( !(imx_uart_read(uart, USR2) & USR2_RDR) )
> +        return 0;
> +
> +    data = imx_uart_read(uart, URXD0);
> +    *pc = data & URXD_RX_DATA;
> +
> +    if ( unlikely(data & URXD_ERR) )
> +        imx_uart_clear_rx_errors(port);
> +
> +    return 1;
> +}
> +
> +static int __init imx_uart_irq(struct serial_port *port)
> +{
> +    struct imx_uart *uart = port->uart;
can be const

> +
> +    return ((uart->irq > 0) ? uart->irq : -1);
> +}
> +
> +static const struct vuart_info *imx_uart_vuart_info(struct serial_port *port)
> +{
> +    struct imx_uart *uart = port->uart;
can be const

> +
> +    return &uart->vuart;
> +}
> +
> +static void imx_uart_start_tx(struct serial_port *port)
> +{
> +    struct imx_uart *uart = port->uart;
> +
> +    imx_uart_write(uart, UCR1, imx_uart_read(uart, UCR1) | UCR1_TRDYEN);
> +}
> +
> +static void imx_uart_stop_tx(struct serial_port *port)
> +{
> +    struct imx_uart *uart = port->uart;
> +
> +    imx_uart_write(uart, UCR1, imx_uart_read(uart, UCR1) & ~UCR1_TRDYEN);
> +}
> +
> +static struct uart_driver __read_mostly imx_uart_driver = {
> +    .init_preirq = imx_uart_init_preirq,
> +    .init_postirq = imx_uart_init_postirq,
> +    .endboot = NULL,
> +    .suspend = imx_uart_suspend,
> +    .resume = imx_uart_resume,
no need to assign a value for these 3 members

> +    .tx_ready = imx_uart_tx_ready,
> +    .putc = imx_uart_putc,
> +    .getc = imx_uart_getc,
> +    .irq = imx_uart_irq,
> +    .start_tx = imx_uart_start_tx,
> +    .stop_tx = imx_uart_stop_tx,
> +    .vuart_info = imx_uart_vuart_info,
> +};
> +
> +static int __init imx_uart_init(struct dt_device_node *dev, const void *data)
> +{
> +    const char *config = data;
> +    struct imx_uart *uart;
> +    int res;
> +    paddr_t addr, size;
> +
> +    if ( strcmp(config, "") )
> +        printk("WARNING: UART configuration is not supported\n");
> +
> +    uart = &imx_com;
> +
> +    uart->baud = 115200;
> +    uart->data_bits = 8;
> +    uart->parity = 0;
> +    uart->stop_bits = 1;
They are set but never used

> +
> +    res = dt_device_get_paddr(dev, 0, &addr, &size);
> +    if ( res )
> +    {
> +        printk("imx-uart: Unable to retrieve the base address of the 
> UART\n");
> +        return res;
> +    }
> +
> +    res = platform_get_irq(dev, 0);
> +    if ( res < 0 )
> +    {
> +        printk("imx-uart: Unable to retrieve the IRQ\n");
> +        return -EINVAL;
> +    }
> +    uart->irq = res;
> +
> +    uart->regs = ioremap_nocache(addr, size);
> +    if ( !uart->regs )
> +    {
> +        printk("imx-uart: Unable to map the UART memory\n");
> +        return -ENOMEM;
> +    }
> +
> +    uart->vuart.base_addr = addr;
> +    uart->vuart.size = size;
> +    uart->vuart.data_off = URTX0;
> +    uart->vuart.status_off = IMX21_UTS;
> +    uart->vuart.status = UTS_TXEMPTY;
> +
> +    /* Register with generic serial driver */
> +    serial_register_uart(SERHND_DTUART, &imx_uart_driver, uart);
> +
> +    dt_device_set_used_by(dev, DOMID_XEN);
> +
> +    return 0;
> +}
> +
> +static const struct dt_device_match imx_uart_dt_compat[] __initconst =
> +{
> +    DT_MATCH_COMPATIBLE("fsl,imx6q-uart"),
> +    { /* sentinel */ },
> +};
> +
> +DT_DEVICE_START(imx_uart, "i.MX UART", DEVICE_SERIAL)
> +    .dt_match = imx_uart_dt_compat,
> +    .init = imx_uart_init,
> +DT_DEVICE_END
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> --
> 2.34.1
> 

~Michal