[PATCH 0/3] xen/arm: add i.MX lpuart and i.MX8QM initial support

[PATCH 0/3] xen/arm: add i.MX lpuart and i.MX8QM initial support

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Add i.MX lpuart driver and i.MX8QM platform support.
 - lpuart is the uart IP used in i.MX8QM/QXP/93.
 - Very basic i.MX8QM platform support.

Peng Fan (3):
  xen/arm: Add i.MX lpuart driver
  xen/arm: Add i.MX lpuart early printk support
  xen/arm: Add i.MX8QM platform support

 xen/arch/arm/Kconfig.debug              |  21 ++
 xen/arch/arm/arm64/debug-imx-lpuart.inc |  48 ++++
 xen/arch/arm/platforms/Makefile         |   1 +
 xen/arch/arm/platforms/imx8qm.c         |  44 ++++
 xen/drivers/char/Kconfig                |   8 +
 xen/drivers/char/Makefile               |   1 +
 xen/drivers/char/imx-lpuart.c           | 303 ++++++++++++++++++++++++
 xen/include/xen/imx-lpuart.h            |  64 +++++
 8 files changed, 490 insertions(+)
 create mode 100644 xen/arch/arm/arm64/debug-imx-lpuart.inc
 create mode 100644 xen/arch/arm/platforms/imx8qm.c
 create mode 100644 xen/drivers/char/imx-lpuart.c
 create mode 100644 xen/include/xen/imx-lpuart.h

-- 
2.30.0

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

[Peng Fan (OSS)]

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>
+#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:
+ */
-- 
2.30.0

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Peng Fan]

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

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

[Julien Grall]

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

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Peng Fan]

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

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

[Julien Grall]

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,

-- 
Julien Grall

[PATCH 2/3] xen/arm: Add i.MX lpuart early printk support

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 xen/arch/arm/Kconfig.debug              | 18 ++++++++++
 xen/arch/arm/arm64/debug-imx-lpuart.inc | 48 +++++++++++++++++++++++++
 2 files changed, 66 insertions(+)
 create mode 100644 xen/arch/arm/arm64/debug-imx-lpuart.inc

diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug
index 35ccd13273..9ecb446b3a 100644
--- a/xen/arch/arm/Kconfig.debug
+++ b/xen/arch/arm/Kconfig.debug
@@ -55,6 +55,20 @@ choice
 			selecting one of the platform specific options below if
 			you know the parameters for the port.
 
+			This option is preferred over the platform specific
+			options; the platform specific options are deprecated
+			and will soon be removed.
+	config EARLY_UART_CHOICE_IMX_LPUART
+		select EARLY_UART_IMX_LPUART
+		depends on ARM_64
+		bool "Early printk via i.MX LPUART"
+		help
+			Say Y here if you wish the early printk to direct their
+			output to a i.MX LPUART. You can use this option to
+			provide the parameters for the i.MX LPUART rather than
+			selecting one of the platform specific options below if
+			you know the parameters for the port.
+
 			This option is preferred over the platform specific
 			options; the platform specific options are deprecated
 			and will soon be removed.
@@ -186,6 +200,9 @@ config EARLY_UART_CADENCE
 config EARLY_UART_EXYNOS4210
 	select EARLY_PRINTK
 	bool
+config EARLY_UART_IMX_LPUART
+	select EARLY_PRINTK
+	bool
 config EARLY_UART_MESON
 	select EARLY_PRINTK
 	bool
@@ -283,6 +300,7 @@ config EARLY_PRINTK_INC
 	default "debug-8250.inc" if EARLY_UART_8250
 	default "debug-cadence.inc" if EARLY_UART_CADENCE
 	default "debug-exynos4210.inc" if EARLY_UART_EXYNOS4210
+	default "debug-imx-lpuart.inc" if EARLY_UART_IMX_LPUART
 	default "debug-meson.inc" if EARLY_UART_MESON
 	default "debug-mvebu.inc" if EARLY_UART_MVEBU
 	default "debug-pl011.inc" if EARLY_UART_PL011
diff --git a/xen/arch/arm/arm64/debug-imx-lpuart.inc b/xen/arch/arm/arm64/debug-imx-lpuart.inc
new file mode 100644
index 0000000000..7510210d46
--- /dev/null
+++ b/xen/arch/arm/arm64/debug-imx-lpuart.inc
@@ -0,0 +1,48 @@
+/*
+ * xen/arch/arm/arm64/debug-imx8qm.inc
+ *
+ * i.MX8QM specific debug code
+ *
+ * Peng Fan <peng.fan@nxp.com>
+ * Copyright (C) 2016 Freescale Inc.
+ *
+ * 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/imx-lpuart.h>
+
+.macro early_uart_init wb wc wd
+/* Already initialized in bootloader */
+.endm
+/* i.MX8QM wait LPUART to be ready to transmit
+ * rb: register which contains the UART base address
+ * rc: scratch register
+ */
+.macro early_uart_ready xb, c
+1:
+        ldr   w\c, [\xb, #UARTSTAT]   /* <- Flag register */
+        tst   w\c, #UARTSTAT_TDRE     /* Check FIFO EMPTY bit */
+        beq   1b                      /* Wait for the UART to be ready */
+.endm
+
+/* i.MX8QM LPUART transmit character
+ * rb: register which contains the UART base address
+ * rt: register which contains the character to transmit */
+.macro early_uart_transmit xb, wt
+        str   \wt, [\xb, #UARTDATA]  /* -> Data Register */
+.endm
+
+/*
+ * Local variables:
+ * mode: ASM
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.30.0

Re: [PATCH 2/3] xen/arm: Add i.MX lpuart early printk support

[Julien Grall]

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/arch/arm/Kconfig.debug              | 18 ++++++++++
>   xen/arch/arm/arm64/debug-imx-lpuart.inc | 48 +++++++++++++++++++++++++
>   2 files changed, 66 insertions(+)
>   create mode 100644 xen/arch/arm/arm64/debug-imx-lpuart.inc
> 
> diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug
> index 35ccd13273..9ecb446b3a 100644
> --- a/xen/arch/arm/Kconfig.debug
> +++ b/xen/arch/arm/Kconfig.debug
> @@ -55,6 +55,20 @@ choice
>   			selecting one of the platform specific options below if
>   			you know the parameters for the port.
>   
> +			This option is preferred over the platform specific
> +			options; the platform specific options are deprecated
> +			and will soon be removed.
> +	config EARLY_UART_CHOICE_IMX_LPUART
> +		select EARLY_UART_IMX_LPUART
> +		depends on ARM_64
> +		bool "Early printk via i.MX LPUART"
> +		help
> +			Say Y here if you wish the early printk to direct their
> +			output to a i.MX LPUART. You can use this option to
> +			provide the parameters for the i.MX LPUART rather than
> +			selecting one of the platform specific options below if
> +			you know the parameters for the port.

Plaform specific early printk are deprecated. So I would rather prefer 
we are not introducing new one. Can you adjust the description to remove 
any mention of platform specific options?

> +
>   			This option is preferred over the platform specific
>   			options; the platform specific options are deprecated
>   			and will soon be removed.
> @@ -186,6 +200,9 @@ config EARLY_UART_CADENCE
>   config EARLY_UART_EXYNOS4210
>   	select EARLY_PRINTK
>   	bool
> +config EARLY_UART_IMX_LPUART
> +	select EARLY_PRINTK
> +	bool
>   config EARLY_UART_MESON
>   	select EARLY_PRINTK
>   	bool
> @@ -283,6 +300,7 @@ config EARLY_PRINTK_INC
>   	default "debug-8250.inc" if EARLY_UART_8250
>   	default "debug-cadence.inc" if EARLY_UART_CADENCE
>   	default "debug-exynos4210.inc" if EARLY_UART_EXYNOS4210
> +	default "debug-imx-lpuart.inc" if EARLY_UART_IMX_LPUART
>   	default "debug-meson.inc" if EARLY_UART_MESON
>   	default "debug-mvebu.inc" if EARLY_UART_MVEBU
>   	default "debug-pl011.inc" if EARLY_UART_PL011
> diff --git a/xen/arch/arm/arm64/debug-imx-lpuart.inc b/xen/arch/arm/arm64/debug-imx-lpuart.inc
> new file mode 100644
> index 0000000000..7510210d46
> --- /dev/null
> +++ b/xen/arch/arm/arm64/debug-imx-lpuart.inc
> @@ -0,0 +1,48 @@
> +/*
> + * xen/arch/arm/arm64/debug-imx8qm.inc
> + *
> + * i.MX8QM specific debug code
> + *
> + * Peng Fan <peng.fan@nxp.com>
> + * Copyright (C) 2016 Freescale Inc.
> + *
> + * 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/imx-lpuart.h>
> +
> +.macro early_uart_init wb wc wd
> +/* Already initialized in bootloader */
> +.endm

NIT: I would add a newline to separate with this macro from next one.

> +/* i.MX8QM wait LPUART to be ready to transmit
> + * rb: register which contains the UART base address
> + * rc: scratch register
> + */

The coding style for multi-lines comment is:

/*
  * Foo
  * Bar
  */

> +.macro early_uart_ready xb, c
> +1:
> +        ldr   w\c, [\xb, #UARTSTAT]   /* <- Flag register */
> +        tst   w\c, #UARTSTAT_TDRE     /* Check FIFO EMPTY bit */
> +        beq   1b                      /* Wait for the UART to be ready */
> +.endm
> +
> +/* i.MX8QM LPUART transmit character
> + * rb: register which contains the UART base address
> + * rt: register which contains the character to transmit */

Coding style:

/*
  * Foo
  * Bar
  */

> +.macro early_uart_transmit xb, wt
> +        str   \wt, [\xb, #UARTDATA]  /* -> Data Register */
> +.endm
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */

Cheers,

-- 
Julien Grall

RE: [PATCH 2/3] xen/arm: Add i.MX lpuart early printk support

[Peng Fan]

> Subject: Re: [PATCH 2/3] xen/arm: Add i.MX lpuart early printk support
> 
> 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/arch/arm/Kconfig.debug              | 18 ++++++++++
> >   xen/arch/arm/arm64/debug-imx-lpuart.inc | 48
> +++++++++++++++++++++++++
> >   2 files changed, 66 insertions(+)
> >   create mode 100644 xen/arch/arm/arm64/debug-imx-lpuart.inc
> >
> > diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug
> > index 35ccd13273..9ecb446b3a 100644
> > --- a/xen/arch/arm/Kconfig.debug
> > +++ b/xen/arch/arm/Kconfig.debug
> > @@ -55,6 +55,20 @@ choice
> >   			selecting one of the platform specific options below if
> >   			you know the parameters for the port.
> >
> > +			This option is preferred over the platform specific
> > +			options; the platform specific options are deprecated
> > +			and will soon be removed.
> > +	config EARLY_UART_CHOICE_IMX_LPUART
> > +		select EARLY_UART_IMX_LPUART
> > +		depends on ARM_64
> > +		bool "Early printk via i.MX LPUART"
> > +		help
> > +			Say Y here if you wish the early printk to direct their
> > +			output to a i.MX LPUART. You can use this option to
> > +			provide the parameters for the i.MX LPUART rather than
> > +			selecting one of the platform specific options below if
> > +			you know the parameters for the port.
> 
> Plaform specific early printk are deprecated. So I would rather prefer we are
> not introducing new one. Can you adjust the description to remove any
> mention of platform specific options?

Sure, fix in v2.

> 
> > +
> >   			This option is preferred over the platform specific
> >   			options; the platform specific options are deprecated
> >   			and will soon be removed.
> > @@ -186,6 +200,9 @@ config EARLY_UART_CADENCE
> >   config EARLY_UART_EXYNOS4210
> >   	select EARLY_PRINTK
> >   	bool
> > +config EARLY_UART_IMX_LPUART
> > +	select EARLY_PRINTK
> > +	bool
> >   config EARLY_UART_MESON
> >   	select EARLY_PRINTK
> >   	bool
> > @@ -283,6 +300,7 @@ config EARLY_PRINTK_INC
> >   	default "debug-8250.inc" if EARLY_UART_8250
> >   	default "debug-cadence.inc" if EARLY_UART_CADENCE
> >   	default "debug-exynos4210.inc" if EARLY_UART_EXYNOS4210
> > +	default "debug-imx-lpuart.inc" if EARLY_UART_IMX_LPUART
> >   	default "debug-meson.inc" if EARLY_UART_MESON
> >   	default "debug-mvebu.inc" if EARLY_UART_MVEBU
> >   	default "debug-pl011.inc" if EARLY_UART_PL011 diff --git
> > a/xen/arch/arm/arm64/debug-imx-lpuart.inc
> > b/xen/arch/arm/arm64/debug-imx-lpuart.inc
> > new file mode 100644
> > index 0000000000..7510210d46
> > --- /dev/null
> > +++ b/xen/arch/arm/arm64/debug-imx-lpuart.inc
> > @@ -0,0 +1,48 @@
> > +/*
> > + * xen/arch/arm/arm64/debug-imx8qm.inc
> > + *
> > + * i.MX8QM specific debug code
> > + *
> > + * Peng Fan <peng.fan@nxp.com>
> > + * Copyright (C) 2016 Freescale Inc.
> > + *
> > + * 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/imx-lpuart.h>
> > +
> > +.macro early_uart_init wb wc wd
> > +/* Already initialized in bootloader */ .endm
> 
> NIT: I would add a newline to separate with this macro from next one.

Fix in v2.

> 
> > +/* i.MX8QM wait LPUART to be ready to transmit
> > + * rb: register which contains the UART base address
> > + * rc: scratch register
> > + */
> 
> The coding style for multi-lines comment is:

Fix in v2. Thanks.

> 
> /*
>   * Foo
>   * Bar
>   */
> 
> > +.macro early_uart_ready xb, c
> > +1:
> > +        ldr   w\c, [\xb, #UARTSTAT]   /* <- Flag register */
> > +        tst   w\c, #UARTSTAT_TDRE     /* Check FIFO EMPTY bit */
> > +        beq   1b                      /* Wait for the UART to be
> ready */
> > +.endm
> > +
> > +/* i.MX8QM LPUART transmit character
> > + * rb: register which contains the UART base address
> > + * rt: register which contains the character to transmit */
> 
> Coding style:

Fix in V2.

Thanks,
Peng.
> 
> /*
>   * Foo
>   * Bar
>   */
> 
> > +.macro early_uart_transmit xb, wt
> > +        str   \wt, [\xb, #UARTDATA]  /* -> Data Register */
> > +.endm
> > +
> > +/*
> > + * Local variables:
> > + * mode: ASM
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> 
> Cheers,
> 
> --
> Julien Grall

[PATCH 3/3] xen/arm: Add i.MX8QM platform support

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 xen/arch/arm/Kconfig.debug      |  3 +++
 xen/arch/arm/platforms/Makefile |  1 +
 xen/arch/arm/platforms/imx8qm.c | 44 +++++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+)
 create mode 100644 xen/arch/arm/platforms/imx8qm.c

diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug
index 9ecb446b3a..43ccd8fe62 100644
--- a/xen/arch/arm/Kconfig.debug
+++ b/xen/arch/arm/Kconfig.debug
@@ -143,6 +143,9 @@ choice
 	config EARLY_PRINTK_HIKEY960
 		bool "Early printk with pl011 with Hikey 960"
 		select EARLY_UART_PL011
+	config EARLY_PRINTK_IMX8QM
+		bool "Early printk with i.MX LPUART with i.MX8QM"
+		select EARLY_UART_IMX_LPUART
 	config EARLY_PRINTK_JUNO
 		bool "Early printk with pl011 on Juno platform"
 		select EARLY_UART_PL011
diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
index 8632f4115f..bec6e55d1f 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -9,5 +9,6 @@ obj-$(CONFIG_ALL_PLAT)   += sunxi.o
 obj-$(CONFIG_ALL64_PLAT) += thunderx.o
 obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o
 obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o
+obj-$(CONFIG_ALL64_PLAT) += imx8qm.o
 obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
 obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp-eemi.o
diff --git a/xen/arch/arm/platforms/imx8qm.c b/xen/arch/arm/platforms/imx8qm.c
new file mode 100644
index 0000000000..289c18e5f9
--- /dev/null
+++ b/xen/arch/arm/platforms/imx8qm.c
@@ -0,0 +1,44 @@
+/*
+ * xen/arch/arm/platforms/imx8qm.c
+ *
+ * i.MX 8QM setup
+ *
+ * Copyright 2022 NXP
+ *
+ * Peng Fan <peng.fan@nxp.com>
+ *
+ * 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/delay.h>
+#include <xen/mm.h>
+#include <xen/vmap.h>
+#include <asm/io.h>
+#include <asm/platform.h>
+
+static const char * const imx8qm_dt_compat[] __initconst =
+{
+    "fsl,imx8qm",
+    NULL
+};
+
+PLATFORM_START(imx8qm, "i.MX 8")
+    .compatible = imx8qm_dt_compat,
+PLATFORM_END
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.30.0

Re: [PATCH 3/3] xen/arm: Add i.MX8QM platform support

[Julien Grall]

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/arch/arm/Kconfig.debug      |  3 +++
>   xen/arch/arm/platforms/Makefile |  1 +
>   xen/arch/arm/platforms/imx8qm.c | 44 +++++++++++++++++++++++++++++++++
>   3 files changed, 48 insertions(+)
>   create mode 100644 xen/arch/arm/platforms/imx8qm.c
> 
> diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug
> index 9ecb446b3a..43ccd8fe62 100644
> --- a/xen/arch/arm/Kconfig.debug
> +++ b/xen/arch/arm/Kconfig.debug
> @@ -143,6 +143,9 @@ choice
>   	config EARLY_PRINTK_HIKEY960
>   		bool "Early printk with pl011 with Hikey 960"
>   		select EARLY_UART_PL011
> +	config EARLY_PRINTK_IMX8QM
> +		bool "Early printk with i.MX LPUART with i.MX8QM"
> +		select EARLY_UART_IMX_LPUART

The goal of platform specific early printk is to select to UART address 
(see EARLY_UART_BASE_ADDRESS).

However, we have deprecated them. So we should avoid adding new ones.

>   	config EARLY_PRINTK_JUNO
>   		bool "Early printk with pl011 on Juno platform"
>   		select EARLY_UART_PL011
> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> index 8632f4115f..bec6e55d1f 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -9,5 +9,6 @@ obj-$(CONFIG_ALL_PLAT)   += sunxi.o
>   obj-$(CONFIG_ALL64_PLAT) += thunderx.o
>   obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o
>   obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o
> +obj-$(CONFIG_ALL64_PLAT) += imx8qm.o
>   obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
>   obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp-eemi.o
> diff --git a/xen/arch/arm/platforms/imx8qm.c b/xen/arch/arm/platforms/imx8qm.c
> new file mode 100644
> index 0000000000..289c18e5f9
> --- /dev/null
> +++ b/xen/arch/arm/platforms/imx8qm.c
> @@ -0,0 +1,44 @@
> +/*
> + * xen/arch/arm/platforms/imx8qm.c
> + *
> + * i.MX 8QM setup
> + *
> + * Copyright 2022 NXP
> + *
> + * Peng Fan <peng.fan@nxp.com>
> + *
> + * 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/delay.h>
> +#include <xen/mm.h>
> +#include <xen/vmap.h>
> +#include <asm/io.h>
> +#include <asm/platform.h>
> +
> +static const char * const imx8qm_dt_compat[] __initconst =
> +{
> +    "fsl,imx8qm",
> +    NULL
> +};
> +
> +PLATFORM_START(imx8qm, "i.MX 8")
> +    .compatible = imx8qm_dt_compat,
> +PLATFORM_END

We are only adding new platform definition when quirks are necessary. Do 
you need specific quirks for the i.MX8QM?

A somewhat related question, is this series enough to boot Xen upstream 
on the board?

Cheers,

-- 
Julien Grall

RE: [PATCH 3/3] xen/arm: Add i.MX8QM platform support

[Peng Fan]

> Subject: Re: [PATCH 3/3] xen/arm: Add i.MX8QM platform support
> 
> 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/arch/arm/Kconfig.debug      |  3 +++
> >   xen/arch/arm/platforms/Makefile |  1 +
> >   xen/arch/arm/platforms/imx8qm.c | 44
> +++++++++++++++++++++++++++++++++
> >   3 files changed, 48 insertions(+)
> >   create mode 100644 xen/arch/arm/platforms/imx8qm.c
> >
> > diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug
> > index 9ecb446b3a..43ccd8fe62 100644
> > --- a/xen/arch/arm/Kconfig.debug
> > +++ b/xen/arch/arm/Kconfig.debug
> > @@ -143,6 +143,9 @@ choice
> >   	config EARLY_PRINTK_HIKEY960
> >   		bool "Early printk with pl011 with Hikey 960"
> >   		select EARLY_UART_PL011
> > +	config EARLY_PRINTK_IMX8QM
> > +		bool "Early printk with i.MX LPUART with i.MX8QM"
> > +		select EARLY_UART_IMX_LPUART
> 
> The goal of platform specific early printk is to select to UART address (see
> EARLY_UART_BASE_ADDRESS).
> 
> However, we have deprecated them. So we should avoid adding new ones.
> 
> >   	config EARLY_PRINTK_JUNO
> >   		bool "Early printk with pl011 on Juno platform"
> >   		select EARLY_UART_PL011
> > diff --git a/xen/arch/arm/platforms/Makefile
> > b/xen/arch/arm/platforms/Makefile index 8632f4115f..bec6e55d1f 100644
> > --- a/xen/arch/arm/platforms/Makefile
> > +++ b/xen/arch/arm/platforms/Makefile
> > @@ -9,5 +9,6 @@ obj-$(CONFIG_ALL_PLAT)   += sunxi.o
> >   obj-$(CONFIG_ALL64_PLAT) += thunderx.o
> >   obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o
> >   obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o
> > +obj-$(CONFIG_ALL64_PLAT) += imx8qm.o
> >   obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
> >   obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp-eemi.o diff --git
> > a/xen/arch/arm/platforms/imx8qm.c b/xen/arch/arm/platforms/imx8qm.c
> > new file mode 100644 index 0000000000..289c18e5f9
> > --- /dev/null
> > +++ b/xen/arch/arm/platforms/imx8qm.c
> > @@ -0,0 +1,44 @@
> > +/*
> > + * xen/arch/arm/platforms/imx8qm.c
> > + *
> > + * i.MX 8QM setup
> > + *
> > + * Copyright 2022 NXP
> > + *
> > + * Peng Fan <peng.fan@nxp.com>
> > + *
> > + * 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/delay.h>
> > +#include <xen/mm.h>
> > +#include <xen/vmap.h>
> > +#include <asm/io.h>
> > +#include <asm/platform.h>
> > +
> > +static const char * const imx8qm_dt_compat[] __initconst = {
> > +    "fsl,imx8qm",
> > +    NULL
> > +};
> > +
> > +PLATFORM_START(imx8qm, "i.MX 8")
> > +    .compatible = imx8qm_dt_compat,
> > +PLATFORM_END
> 
> We are only adding new platform definition when quirks are necessary. Do
> you need specific quirks for the i.MX8QM?
> 
> A somewhat related question, is this series enough to boot Xen upstream on
> the board?

Boot xen upstream, no need specific quirk.

Thanks,
Peng.

> 
> Cheers,
> 
> --
> Julien Grall

Re: [PATCH 3/3] xen/arm: Add i.MX8QM platform support

[Julien Grall]

Hi Peng,

On 18/03/2022 05:24, Peng Fan wrote:
>> Subject: Re: [PATCH 3/3] xen/arm: Add i.MX8QM platform support
>> On 28/02/2022 01:07, Peng Fan (OSS) wrote:
>>> +static const char * const imx8qm_dt_compat[] __initconst = {
>>> +    "fsl,imx8qm",
>>> +    NULL
>>> +};
>>> +
>>> +PLATFORM_START(imx8qm, "i.MX 8")
>>> +    .compatible = imx8qm_dt_compat,
>>> +PLATFORM_END
>>
>> We are only adding new platform definition when quirks are necessary. Do
>> you need specific quirks for the i.MX8QM?
>>
>> A somewhat related question, is this series enough to boot Xen upstream on
>> the board?
> 
> Boot xen upstream, no need specific quirk.

That's great to hear! Then this file should not be necessary at all. 
Please drop it.

Cheers,

-- 
Julien Grall

Re: [PATCH 0/3] xen/arm: add i.MX lpuart and i.MX8QM initial support

[Jan Beulich]

On 28.02.2022 02:07, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Add i.MX lpuart driver and i.MX8QM platform support.
>  - lpuart is the uart IP used in i.MX8QM/QXP/93.
>  - Very basic i.MX8QM platform support.
> 
> Peng Fan (3):
>   xen/arm: Add i.MX lpuart driver
>   xen/arm: Add i.MX lpuart early printk support
>   xen/arm: Add i.MX8QM platform support
> 
>  xen/arch/arm/Kconfig.debug              |  21 ++
>  xen/arch/arm/arm64/debug-imx-lpuart.inc |  48 ++++
>  xen/arch/arm/platforms/Makefile         |   1 +
>  xen/arch/arm/platforms/imx8qm.c         |  44 ++++
>  xen/drivers/char/Kconfig                |   8 +
>  xen/drivers/char/Makefile               |   1 +
>  xen/drivers/char/imx-lpuart.c           | 303 ++++++++++++++++++++++++
>  xen/include/xen/imx-lpuart.h            |  64 +++++
>  8 files changed, 490 insertions(+)
>  create mode 100644 xen/arch/arm/arm64/debug-imx-lpuart.inc
>  create mode 100644 xen/arch/arm/platforms/imx8qm.c
>  create mode 100644 xen/drivers/char/imx-lpuart.c
>  create mode 100644 xen/include/xen/imx-lpuart.h

I guess the latter two additions want to be accompanied by an update to
./MAINTAINERS' ARM section.

Jan

RE: [PATCH 0/3] xen/arm: add i.MX lpuart and i.MX8QM initial support

[Peng Fan]

> Subject: Re: [PATCH 0/3] xen/arm: add i.MX lpuart and i.MX8QM initial
> support
> 
> On 28.02.2022 02:07, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Add i.MX lpuart driver and i.MX8QM platform support.
> >  - lpuart is the uart IP used in i.MX8QM/QXP/93.
> >  - Very basic i.MX8QM platform support.
> >
> > Peng Fan (3):
> >   xen/arm: Add i.MX lpuart driver
> >   xen/arm: Add i.MX lpuart early printk support
> >   xen/arm: Add i.MX8QM platform support
> >
> >  xen/arch/arm/Kconfig.debug              |  21 ++
> >  xen/arch/arm/arm64/debug-imx-lpuart.inc |  48 ++++
> >  xen/arch/arm/platforms/Makefile         |   1 +
> >  xen/arch/arm/platforms/imx8qm.c         |  44 ++++
> >  xen/drivers/char/Kconfig                |   8 +
> >  xen/drivers/char/Makefile               |   1 +
> >  xen/drivers/char/imx-lpuart.c           | 303
> ++++++++++++++++++++++++
> >  xen/include/xen/imx-lpuart.h            |  64 +++++
> >  8 files changed, 490 insertions(+)
> >  create mode 100644 xen/arch/arm/arm64/debug-imx-lpuart.inc
> >  create mode 100644 xen/arch/arm/platforms/imx8qm.c  create mode
> > 100644 xen/drivers/char/imx-lpuart.c  create mode 100644
> > xen/include/xen/imx-lpuart.h
> 
> I guess the latter two additions want to be accompanied by an update
> to ./MAINTAINERS' ARM section.

I'll address this in v2. Let me wait more comments and address together.

Thanks,
Peng.

> 
> Jan