RE: [PATCH 1/3] xen/arm: Add i.MX lpuart driver

[Peng Fan <peng.fan@xxxxxxx>]

Hi Julien,

> Subject: Re: [PATCH 1/3] xen/arm: Add i.MX lpuart driver
> 
> Hi Peng,
> 
> On 28/02/2022 01:07, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@xxxxxxx>
> >
> > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > ---
> >   xen/drivers/char/Kconfig      |   8 +
> >   xen/drivers/char/Makefile     |   1 +
> >   xen/drivers/char/imx-lpuart.c | 303
> ++++++++++++++++++++++++++++++++++
> >   xen/include/xen/imx-lpuart.h  |  64 +++++++
> >   4 files changed, 376 insertions(+)
> >   create mode 100644 xen/drivers/char/imx-lpuart.c
> >   create mode 100644 xen/include/xen/imx-lpuart.h
> >
> > diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig index
> > 2ff5b288e2..0efdb2128f 100644
> > --- a/xen/drivers/char/Kconfig
> > +++ b/xen/drivers/char/Kconfig
> > @@ -13,6 +13,14 @@ config HAS_CADENCE_UART
> >       This selects the Xilinx Zynq Cadence UART. If you have a Xilinx
> Zynq
> >       based board, say Y.
> >
> > +config HAS_IMX_LPUART
> > +   bool "i.MX LPUART driver"
> > +   default y
> > +   depends on ARM_64
> > +   help
> > +     This selects the i.MX LPUART. If you have a i.MX8QM 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 7c646d771c..14e67cf072 100644
> > --- a/xen/drivers/char/Makefile
> > +++ b/xen/drivers/char/Makefile
> > @@ -8,6 +8,7 @@ obj-$(CONFIG_HAS_MVEBU) += mvebu-uart.o
> >   obj-$(CONFIG_HAS_OMAP) += omap-uart.o
> >   obj-$(CONFIG_HAS_SCIF) += scif-uart.o
> >   obj-$(CONFIG_HAS_EHCI) += ehci-dbgp.o
> > +obj-$(CONFIG_HAS_IMX_LPUART) += imx-lpuart.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-lpuart.c b/xen/drivers/char/imx-lpuart.c new
> > file mode 100644 index 0000000000..2a30e3f21a
> > --- /dev/null
> > +++ b/xen/drivers/char/imx-lpuart.c
> > @@ -0,0 +1,303 @@
> > +/*
> > + * xen/drivers/char/imx-lpuart.c
> > + *
> > + * Driver for i.MX LPUART.
> > + *
> > + * Peng Fan <peng.fan@xxxxxxx>
> > + * Copyright 2022 NXP
> > + *
> > + * 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/console.h>
> 
> This should not be necessary.h

Will drop in v2.

> 
> > +#include <xen/serial.h>
> > +#include <xen/imx-lpuart.h>
> > +#include <xen/init.h>
> > +#include <xen/irq.h>
> > +#include <xen/errno.h>
> > +#include <xen/mm.h>
> 
> Please order the <xen/*> alphabetically.h

Fix in V2.

> 
> > +#include <asm/device.h>
> > +#include <asm/io.h>
> > +
> > +#define imx_lpuart_read(uart, off)       readl((uart)->regs + off)
> > +#define imx_lpuart_write(uart, off, val) writel((val), (uart)->regs +
> > +off)
> > +
> > +static struct imx_lpuart {
> > +    unsigned int baud, clock_hz, data_bits, parity, stop_bits, fifo_size;
> > +    unsigned int irq;
> > +    char __iomem *regs;
> > +    struct irqaction irqaction;
> > +    struct vuart_info vuart;
> > +} imx8_com = {0};
> 
> This will be initialized to 0 by default. So I would drop {0}.

Fix in V2.

> 
> > +
> > +static void imx_lpuart_interrupt(int irq, void *data,
> > +                                  struct cpu_user_regs *regs)
> 
> Coding style: 'struct' should be aligned with 'int'.

Fix in V2.

> 
> > +{
> > +    struct serial_port *port = data;
> > +    struct imx_lpuart *uart = port->uart;
> > +    unsigned int sts, rxcnt;
> > +
> > +    sts = imx_lpuart_read(uart, UARTSTAT);
> > +    rxcnt = imx_lpuart_read(uart, UARTWATER) >>
> UARTWATER_RXCNT_OFF;
> > +
> > +    if ((sts & UARTSTAT_RDRF) || (rxcnt > 0)) {
> 
> Coding style:
> 
> if ( ... )
> {
> 
> But for single line block, we tend to avoid {}.

Fix in V2.

> 
> > +       serial_rx_interrupt(port, regs);
> > +    }
> > +
> > +    if ((sts & UARTSTAT_TDRE) &&
> > +        !(imx_lpuart_read(uart, UARTBAUD) & UARTBAUD_TDMAE))
> 
> Looking at imx_lpuart_init_preirq(), you will always clear UARTBAUD_TDMAE.
> So it is necessary to check the value for every interrupt?

Just for safe, but may not needed check, let me check more, if not needed,
I'll drop in V2.

> 
> > +       serial_tx_interrupt(port, regs);
> > +
> > +    imx_lpuart_write(uart, UARTSTAT, sts); }
> > +
> > +static void __init imx_lpuart_init_preirq(struct serial_port *port) {
> > +    struct imx_lpuart *uart = port->uart;
> > +    u32 sbr, osr;
> > +    u32 ctrl, old_ctrl, bd;
> > +    u32 baud;
> 
> In Xen we are phasing out the use of u* in favor of uint*_t. Can you convert
> your code to use uint*_t?

Fix in V2.

> 
> > +
> > +    ctrl = old_ctrl = imx_lpuart_read(uart, UARTCTRL);
> > +    ctrl = (old_ctrl & ~UARTCTRL_M) | UARTCTRL_TE | UARTCTRL_RE;
> > +    bd = imx_lpuart_read(uart, UARTBAUD);
> > +    baud = uart->baud;
> > +
> > +    while ( !(imx_lpuart_read(uart, UARTSTAT) & UARTSTAT_TC))
> 
> Coding style: missing space before the last ).

Fix in V2.

> 
> > +       barrier();
> 
> I think this wants to be cpu_relax(). At the moment, it is implemented as a
> barrier() but this may change in the future.

Fix in V2.

> 
> > +
> > +    /* Disable trasmit and receive */
> 
> Typo: s/trasmit/transmit/

Fix in V2.

> 
> > +    imx_lpuart_write(uart, UARTCTRL, old_ctrl & ~(UARTCTRL_TE |
> > + UARTCTRL_RE));
> > +
> > +    osr = (bd >> UARTBAUD_OSR_SHIFT) & UARTBAUD_OSR_MASK;
> > +    sbr = uart->clock_hz / (baud * (osr + 1));
> 
> For earlyprintk() patch you rely on the baud rate set by the firmware.
> Looking at the code below, you will also hardocode the baud rate. So couldn't
> we simply reply on the firmware to set the baud correctly?

It should be ok. let me check more and drop if it works.

> 
> > +
> > +    bd &= ~ UARTBAUD_SBR_MASK;
> > +    bd |= sbr & UARTBAUD_SBR_MASK;
> > +    bd |= UARTBAUD_BOTHEDGE;
> 
> In the Linux driver, the bit will only be set when osr is between 3 and 8.
> Shouldn't we do the same?

Thanks for spotting this, fix in v2.

> 
> > +    bd &= ~(UARTBAUD_TDMAE | UARTBAUD_RDMAE);
> 
> Can you document why we clear the flag?

To avoid uart dma interrupt.

> 
> > +
> > +    imx_lpuart_write(uart, UARTMODIR, 0);
> > +    imx_lpuart_write(uart, UARTBAUD, bd);
> > +    imx_lpuart_write(uart, UARTCTRL, ctrl); }
> > +
> > +static void __init imx_lpuart_init_postirq(struct serial_port *port)
> > +{
> > +    struct imx_lpuart *uart = port->uart;
> > +    unsigned int temp;
> > +
> > +    uart->irqaction.handler = imx_lpuart_interrupt;
> > +    uart->irqaction.name = "imx_lpuart";
> > +    uart->irqaction.dev_id = port;
> > +
> > +    if ( setup_irq(uart->irq, 0, &uart->irqaction) != 0 )
> > +    {
> > +        dprintk(XENLOG_ERR, "Failed to allocate imx_lpuart IRQ %d\n",
> > +                uart->irq);
> > +        return;
> > +    }
> > +
> > +    /* Enable interrupte */
> 
> Typo: s/interrupte/interrupts/

Fix in v2.

> 
> > +    temp = imx_lpuart_read(uart, UARTCTRL);
> > +    temp |= (UARTCTRL_RIE | UARTCTRL_TIE);
> > +    temp |= UARTCTRL_ILIE;
> > +    imx_lpuart_write(uart, UARTCTRL, temp); }
> > +
> > +static void imx_lpuart_suspend(struct serial_port *port) {
> > +    BUG();
> > +}
> > +
> > +static void imx_lpuart_resume(struct serial_port *port) {
> > +    BUG();
> > +}
> > +
> > +static int imx_lpuart_tx_ready(struct serial_port *port) {
> > +    struct imx_lpuart *uart = port->uart;
> > +
> > +    return (imx_lpuart_read(uart, UARTSTAT) & UARTSTAT_TC) ? 1 : 0;
> 
> This can be simply:
> 
> return imx_lpuart_read(uart, UARTSTAT) & UARTSTAT_TC;

Fix in V2.

> 
> > +}
> > +
> > +static void imx_lpuart_putc(struct serial_port *port, char c) {
> > +    struct imx_lpuart *uart = port->uart;
> > +
> > +    while ( !(imx_lpuart_read(uart, UARTSTAT) & UARTSTAT_TDRE))
> > +        barrier();
> 
> Same remark about the barrier.

Fix in v2.

> 
> > +
> > +    imx_lpuart_write(uart, UARTDATA, c); }
> > +
> > +static int imx_lpuart_getc(struct serial_port *port, char *pc) {
> > +    struct imx_lpuart *uart = port->uart;
> > +    int ch;
> > +
> > +    while ( !(imx_lpuart_read(uart, UARTSTAT) & UARTSTAT_RDRF))
> > +        barrier();
> 
> Same remark about the barrier.
> 
> However, rather than waiting, shouldn't we check the watermark instead and
> return 0 if there are no character to read?

We normally check status bit to check whether there are data to read.

> 
> > +
> > +    ch = imx_lpuart_read(uart, UARTDATA);
> > +    *pc = ch & 0xff;
> > +
> > +    if (imx_lpuart_read(uart, UARTSTAT) &  UARTSTAT_OR)
> > +        imx_lpuart_write(uart, UARTSTAT, UARTSTAT_OR);
> > +
> > +    return 1;
> > +}
> > +
> > +static int __init imx_lpuart_irq(struct serial_port *port) {
> > +    struct imx_lpuart *uart = port->uart;
> > +
> > +    return ((uart->irq >0) ? uart->irq : -1);
> 
> Coding style: Missing space after >.

Fix in V2.

For the following coding style issue, all will be fixed in v2.

Thanks,
Peng.

> 
> > +}
> > +
> > +static const struct vuart_info *imx_lpuart_vuart_info(struct
> > +serial_port *port) {
> > +    struct imx_lpuart *uart = port->uart;
> > +
> > +    return &uart->vuart;
> > +}
> > +
> > +static void imx_lpuart_start_tx(struct serial_port *port) {
> > +    struct imx_lpuart *uart = port->uart;
> > +    unsigned int temp;
> > +
> > +    temp = imx_lpuart_read(uart, UARTSTAT);
> > +    /* Wait until empty */
> > +    while (!(temp & UARTSTAT_TDRE))
> 
> Coding style: while ( ... )
> 
> > +       barrier();
> 
> Same remark about the barrier.
> 
> > +
> > +    temp = imx_lpuart_read(uart, UARTCTRL);
> > +    imx_lpuart_write(uart, UARTCTRL, (temp | UARTCTRL_TIE));
> > +
> > +    return;
> 
> There is no need for an explicit return here.
> 
> > +}
> > +
> > +static void imx_lpuart_stop_tx(struct serial_port *port) {
> > +    struct imx_lpuart *uart = port->uart;
> > +    unsigned int temp;
> > +
> > +    temp = imx_lpuart_read(uart, UARTCTRL);
> > +    temp &= ~(UARTCTRL_TIE | UARTCTRL_TCIE);
> > +    imx_lpuart_write(uart, UARTCTRL, temp);
> > +
> > +    return;
> 
> There is no need for an explicit return here.
> 
> > +}
> > +
> > +static struct uart_driver __read_mostly imx_lpuart_driver = {
> > +    .init_preirq = imx_lpuart_init_preirq,
> > +    .init_postirq = imx_lpuart_init_postirq,
> > +    .endboot = NULL,
> > +    .suspend = imx_lpuart_suspend,
> > +    .resume = imx_lpuart_resume,
> > +    .tx_ready = imx_lpuart_tx_ready,
> > +    .putc = imx_lpuart_putc,
> > +    .getc = imx_lpuart_getc,
> > +    .irq = imx_lpuart_irq,
> > +    .start_tx = imx_lpuart_start_tx,
> > +    .stop_tx = imx_lpuart_stop_tx,
> > +    .vuart_info = imx_lpuart_vuart_info, };
> > +
> > +static int __init imx_lpuart_init(struct dt_device_node *dev,
> > +                                     const void *data) {
> > +    const char *config = data;
> > +    struct imx_lpuart *uart;
> > +    u32 clkspec;
> > +    int res;
> > +    u64 addr, size;
> > +
> > +    if ( strcmp(config, "") )
> > +        printk("WARNING: UART configuration is not supported\n");
> > +
> > +    uart = &imx8_com;
> > +
> > +    res = dt_property_read_u32(dev, "clock-frequency", &clkspec);
> > +    if ( !res )
> > +    {
> > +   res = dt_property_read_u32(dev, "assigned-clock-rates", &clkspec);
> > +   if ( !res )
> > +   {
> > +           printk("imx-uart: Unable to retrieve the clock frequency\n");
> > +           return -EINVAL;
> > +   }
> > +    }
> > +
> > +    uart->clock_hz = clkspec;
> > +    uart->baud = 115200;
> > +    uart->data_bits = 8;
> > +    uart->parity = 0;
> > +    uart->stop_bits = 1;
> > +
> > +    res = dt_device_get_address(dev, 0, &addr, &size);
> > +    if ( res )
> > +    {
> > +        printk("imx8-lpuart: Unable to retrieve the base"
> > +               " address of the UART\n");
> > +        return res;
> > +    }
> > +
> > +    res = platform_get_irq(dev, 0);
> > +    if ( res < 0 )
> > +    {
> > +        printk("imx8-lpuart: Unable to retrieve the IRQ\n");
> > +        return -EINVAL;
> > +    }
> > +    uart->irq = res;
> > +
> > +    uart->regs = ioremap_nocache(addr, size);
> > +    if ( !uart->regs )
> > +    {
> > +        printk("imx8-lpuart: Unable to map the UART memory\n");
> > +        return -ENOMEM;
> > +    }
> > +
> > +    uart->vuart.base_addr = addr;
> > +    uart->vuart.size = size;
> > +    uart->vuart.data_off = UARTDATA;
> > +    /* tmp from uboot */
> > +    uart->vuart.status_off = UARTSTAT;
> > +    uart->vuart.status = UARTSTAT_TDRE;
> > +
> > +    /* Register with generic serial driver */
> > +    serial_register_uart(SERHND_DTUART, &imx_lpuart_driver, uart);
> > +
> > +    dt_device_set_used_by(dev, DOMID_XEN);
> > +
> > +    return 0;
> > +}
> > +
> > +static const struct dt_device_match imx_lpuart_dt_compat[]
> > +__initconst = {
> > +    DT_MATCH_COMPATIBLE("fsl,imx8qm-lpuart"),
> > +    {},
> > +};
> > +
> > +DT_DEVICE_START(imx_lpuart, "i.MX LPUART", DEVICE_SERIAL)
> > +    .dt_match = imx_lpuart_dt_compat,
> > +    .init = imx_lpuart_init,
> > +DT_DEVICE_END
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/xen/include/xen/imx-lpuart.h
> > b/xen/include/xen/imx-lpuart.h new file mode 100644 index
> > 0000000000..945ab1c4fa
> > --- /dev/null
> > +++ b/xen/include/xen/imx-lpuart.h
> > @@ -0,0 +1,64 @@
> > +/*
> > + * xen/include/asm-arm/imx-lpuart.h
> > + *
> > + * Common constant definition between early printk and the LPUART
> > +driver
> > + *
> > + * Peng Fan <peng.fan@xxxxxxx>
> > + * Copyright 2022 NXP
> > + *
> > + * 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.
> > + */
> > +
> > +#ifndef __ASM_ARM_IMX_LPUART_H
> > +#define __ASM_ARM_IMX_LPUART_H
> > +
> > +/* 32-bit register definition */
> > +#define UARTBAUD          (0x10)
> > +#define UARTSTAT          (0x14)
> > +#define UARTCTRL          (0x18)
> > +#define UARTDATA          (0x1C)
> > +#define UARTMATCH         (0x20)
> > +#define UARTMODIR         (0x24)
> > +#define UARTFIFO          (0x28)
> > +#define UARTWATER         (0x2c)
> > +
> > +#define UARTSTAT_TDRE     (1 << 23)
> > +#define UARTSTAT_TC       (1 << 22)
> > +#define UARTSTAT_RDRF     (1 << 21)
> > +#define UARTSTAT_OR       (1 << 19)
> > +
> > +#define UARTBAUD_OSR_SHIFT (24)
> > +#define UARTBAUD_OSR_MASK (0x1f)
> > +#define UARTBAUD_SBR_MASK (0x1fff)
> > +#define UARTBAUD_BOTHEDGE (0x00020000)
> > +#define UARTBAUD_TDMAE    (0x00800000)
> > +#define UARTBAUD_RDMAE    (0x00200000)
> 
> NIT: For single bit, I find easier to reason when using shift. I.e:
> 
> 1U << X
> 
> or
> 
> BIT(X).
> 
> > +
> > +#define UARTCTRL_TIE      (1 << 23)
> > +#define UARTCTRL_TCIE     (1 << 22)
> > +#define UARTCTRL_RIE      (1 << 21)
> > +#define UARTCTRL_ILIE     (1 << 20)
> > +#define UARTCTRL_TE       (1 << 19)
> > +#define UARTCTRL_RE       (1 << 18)
> > +#define UARTCTRL_M        (1 << 4)
> > +
> > +#define UARTWATER_RXCNT_OFF     24
> > +
> > +#endif /* __ASM_ARM_IMX_LPUART_H */
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> 
> Cheers,
> 
> --
> Julien Grall