diff mbox series

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

Message ID 20220228010711.11566-2-peng.fan@oss.nxp.com (mailing list archive)
State Superseded
Headers show
Series xen/arm: add i.MX lpuart and i.MX8QM initial support | expand

Commit Message

Peng Fan (OSS) Feb. 28, 2022, 1:07 a.m. UTC 
From: Peng Fan <peng.fan@nxp.com>

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 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

Comments

Julien Grall Feb. 28, 2022, 9:19 a.m. UTC | #1 
Hi Peng,

On 28/02/2022 01:07, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>

Can you give me a link to the specification and/or a similar driver in 
Linux?

This would help to review this patch.

Cheers,
Peng Fan Feb. 28, 2022, 9:27 a.m. UTC | #2 
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@nxp.com>
> 
> Can you give me a link to the specification and/or a similar driver in Linux?

https://www.nxp.com/webapp/Download?colCode=IMX8QMIEC 
Chatper 13.6 Low Power Universal Asynchronous Receiver/
Transmitter (LPUART)
But this requires registration to access.

Linux driver:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/fsl_lpuart.c


Thanks,
Peng.

> 
> This would help to review this patch.
> 
> Cheers,
> 
> --
> Julien Grall
Julien Grall March 8, 2022, 6:29 p.m. UTC | #3 
On 28/02/2022 09:27, Peng Fan wrote:
> Hi Julien,

Hi Peng,

>> 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@nxp.com>
>>
>> Can you give me a link to the specification and/or a similar driver in Linux?
> 
> https://www.nxp.com/webapp/Download?colCode=IMX8QMIEC
> Chatper 13.6 Low Power Universal Asynchronous Receiver/
> Transmitter (LPUART)
> But this requires registration to access.
Ok. I think it would still be valuable to add the link of to the spec in 
the commit message.

> 
> Linux driver:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/fsl_lpuart.c

I would also add a link to the Linux code just as a reference (if one 
doesn't want to register).

Cheers,
Julien Grall March 8, 2022, 6:52 p.m. UTC | #4 
Hi Peng,

On 28/02/2022 01:07, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>   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@nxp.com>
> + * 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

> +#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

> +#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}.

> +
> +static void imx_lpuart_interrupt(int irq, void *data,
> +                                  struct cpu_user_regs *regs)

Coding style: 'struct' should be aligned with 'int'.

> +{
> +    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 {}.

> +	    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?

> +	    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?

> +
> +    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 ).

> +	    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.

> +
> +    /* Disable trasmit and receive */

Typo: s/trasmit/transmit/

> +    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?

> +
> +    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?

> +    bd &= ~(UARTBAUD_TDMAE | UARTBAUD_RDMAE);

Can you document why we clear the flag?

> +
> +    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/

> +    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;

> +}
> +
> +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.

> +
> +    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?

> +
> +    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 >.

> +}
> +
> +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@nxp.com>
> + * 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,
Peng Fan March 18, 2022, 5:23 a.m. UTC | #5 
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@nxp.com>
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >   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@nxp.com>
> > + * 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@nxp.com>
> > + * 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
Julien Grall March 21, 2022, 7:39 p.m. UTC | #6 
Hi Peng,

On 18/03/2022 05:23, Peng Fan wrote:
>> Subject: Re: [PATCH 1/3] xen/arm: Add i.MX lpuart driver
>> On 28/02/2022 01:07, Peng Fan (OSS) wrote:
>>> From: Peng Fan <peng.fan@nxp.com>
>>> +
>>> +    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.

I guess by "we", you mean the caller. In which case, there is at least 
one (i.e. serial_getc()) that will not check the status before calling 
getc(). Instead, it will wait if the helper returns 0.

Looking at the other UART driver, none of them seem to busy loop. So, at 
least for consistency, we should avoid the busy loop here.

Cheers,
diff mbox series

Patch

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@nxp.com>
+ * 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>
+#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>
+#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};
+
+static void imx_lpuart_interrupt(int irq, void *data,
+                                  struct cpu_user_regs *regs)
+{
+    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)) {
+	    serial_rx_interrupt(port, regs);
+    }
+
+    if ((sts & UARTSTAT_TDRE) &&
+        !(imx_lpuart_read(uart, UARTBAUD) & UARTBAUD_TDMAE))
+	    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;
+
+    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))
+	    barrier();
+
+    /* Disable trasmit and receive */
+    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));
+
+    bd &= ~ UARTBAUD_SBR_MASK;
+    bd |= sbr & UARTBAUD_SBR_MASK;
+    bd |= UARTBAUD_BOTHEDGE;
+    bd &= ~(UARTBAUD_TDMAE | UARTBAUD_RDMAE);
+
+    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 */
+    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;
+}
+
+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();
+
+    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();
+
+    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);
+}
+
+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))
+	    barrier();
+
+    temp = imx_lpuart_read(uart, UARTCTRL);
+    imx_lpuart_write(uart, UARTCTRL, (temp | UARTCTRL_TIE));
+
+    return;
+}
+
+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;
+}
+
+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@nxp.com>
+ * 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)
+
+#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:
+ */