Re: [PATCH 2/3] drivers: serial: add Qualcomm GENI-based serial driver

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Hi Michal,

Michal Orzel <michal.orzel@amd.com> writes:

> Hello,
>
> On 29/03/2024 01:08, Volodymyr Babchuk wrote:
>> 
>> 
>> Generic Interface (GENI) is a newer interface for low speed interfaces
>> like UART, I2C or SPI. This patch adds the simple driver for the UART
>> instance of GENI. Code is based on similar drivers in U-Boot and Linux
>> kernel.
> Do you have a link to a manual?

I wish I had. Qualcomm provides HW manuals only under very strict
NDAs. At the time of writing I don't have access to the manual at
all. Those patches are based solely on similar drivers in U-Boot and
Linux kernel.

>> 
>> This driver implements only simple synchronous mode, because although
>> GENI supports FIFO mode, it needs to know number of
>> characters **before** starting TX transaction. This is a stark
>> contrast when compared to other UART peripherals, which allow adding
>> characters to a FIFO while TX operation is running.
>> 
>> The patch adds both normal UART driver and earlyprintk version.
>> 
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>  xen/arch/arm/Kconfig.debug           |  19 +-
>>  xen/arch/arm/arm64/debug-qcom.inc    |  76 +++++++
> Shouldn't all the files (+ other places) have geni in their names? Could qcom refer to other type of serial device?

AFAIK, there is an older type of serial device. Both Linux and U-Boot
use "msm" instead of "qcom" in drivers names for those devices.

But I can add "geni" to the names, no big deal.

>
>>  xen/arch/arm/include/asm/qcom-uart.h |  48 +++++
>>  xen/drivers/char/Kconfig             |   8 +
>>  xen/drivers/char/Makefile            |   1 +
>>  xen/drivers/char/qcom-uart.c         | 288 +++++++++++++++++++++++++++
>>  6 files changed, 439 insertions(+), 1 deletion(-)
>>  create mode 100644 xen/arch/arm/arm64/debug-qcom.inc
>>  create mode 100644 xen/arch/arm/include/asm/qcom-uart.h
>>  create mode 100644 xen/drivers/char/qcom-uart.c
>> 
>> diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug
>> index eec860e88e..f6ab0bb30e 100644
>> --- a/xen/arch/arm/Kconfig.debug
>> +++ b/xen/arch/arm/Kconfig.debug
>> @@ -119,6 +119,19 @@ 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_QCOM
> The list is sorted alphabetically. Please adhere to that.
>

Sure

>> +               select EARLY_UART_QCOM
>> +               bool "Early printk via Qualcomm debug UART"
> Shouldn't you add depends on ARM_64? Isn't this device Arm64 specific like in Linux?
>

In linux it depends on ARCH_QCOM which can be enabled both for arm and
arm64. But for Xen case... I believe it is better to make ARM_64
dependency.

>> +               help
>> +                       Say Y here if you wish the early printk to direct their
> help text here should be indented by 2 tabs and 2 spaces (do not take example from surrounding code)
>

Would anyone mind if I'll send patch that aligns surrounding code as well?

>> +                       output to a Qualcomm debug UART. You can use this option to
>> +                       provide the parameters for the debug UART 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.
>> @@ -211,6 +224,9 @@ config EARLY_UART_PL011
>>  config EARLY_UART_SCIF
>>         select EARLY_PRINTK
>>         bool
>> +config EARLY_UART_QCOM
>> +       select EARLY_PRINTK
>> +       bool
> The list is sorted alphabetically. Please adhere to that.
>
>> 
>>  config EARLY_PRINTK
>>         bool
>> @@ -261,7 +277,7 @@ config EARLY_UART_PL011_MMIO32
>>           will be done using 32-bit only accessors.
>> 
>>  config EARLY_UART_INIT
>> -       depends on EARLY_UART_PL011 && EARLY_UART_PL011_BAUD_RATE != 0
>> +       depends on (EARLY_UART_PL011 && EARLY_UART_PL011_BAUD_RATE != 0) || EARLY_UART_QCOM
>>         def_bool y
>> 
>>  config EARLY_UART_8250_REG_SHIFT
>> @@ -308,3 +324,4 @@ config EARLY_PRINTK_INC
>>         default "debug-mvebu.inc" if EARLY_UART_MVEBU
>>         default "debug-pl011.inc" if EARLY_UART_PL011
>>         default "debug-scif.inc" if EARLY_UART_SCIF
>> +       default "debug-qcom.inc" if EARLY_UART_QCOM
>> diff --git a/xen/arch/arm/arm64/debug-qcom.inc b/xen/arch/arm/arm64/debug-qcom.inc
>> new file mode 100644
>> index 0000000000..130d08d6e9
>> --- /dev/null
>> +++ b/xen/arch/arm/arm64/debug-qcom.inc
>> @@ -0,0 +1,76 @@
>> +/*
>> + * xen/arch/arm/arm64/debug-qcom.inc
>> + *
>> + * Qualcomm debug UART specific debug code
>> + *
>> + * Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> + * Copyright (C) 2024, EPAM Systems.
>> + *
>> + * 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.
> No need for the license text. You should use SPDX identifier instead at the top of the file:
> /* SPDX-License-Identifier: GPL-2.0-or-later */
>
>> + */
>> +
>> +#include <asm/qcom-uart.h>
>> +
>> +.macro early_uart_init xb c
> Separate macro parameters with comma (here and elsewhere) and please add a comment on top clarifying the use
> Also, do we really need to initialize uart every time? What if firmware does that?
>

You see, this code is working inside-out. early printk code in Xen is
written with assumption that early_uart_transmit don't need a scratch
register. But this is not true for GENI... To send one character we
must write two registers beforehand: TX_TRANS_LEN and CMD0. Only after
that we can write something to FIFO. But early_uart_transmit has no
scratch register to prepare values for TX_TRANS_LEN and CMD0. So we
write what we do here

1. Reset the state machine with ABORT command
2. Write 1 to TX_TRANS_LEN
3. Write START_TX to CMD0

Now early_uart_transmit can write "wt" to the FIFO and after that it can
use "wt" as a scratch register to repeat steps 2 and 3.

Probably I need this as a comment to into the .inc file...

>> +        mov   w\c, #M_GENI_CMD_ABORT
>> +        str   w\c, [\xb, #SE_GENI_M_CMD_CTRL_REG]
>> +1:
>> +        ldr   w\c, [\xb, #SE_GENI_M_IRQ_STATUS]   /* Load IRQ status */
>> +        tst   w\c, #M_CMD_ABORT_EN         /* Check TX_FIFI_WATERMARK_EN bit */
> The comment does not correspond to the code. Shouldn't this be the same as in early_uart_ready?
>

We need to reset the state machine with ABORT command just in case. So
code is correct, but the comment is wrong.

>> +        beq   1b                          /* Wait for the UART to be ready */
>> +        mov   w\c, #M_CMD_ABORT_EN
>> +        orr   w\c, w\c, #M_CMD_DONE_EN
>> +        str   w\c, [\xb, #SE_GENI_M_IRQ_CLEAR]
>> +
>> +        mov   w\c, #1
>> +        str   w\c, [\xb, #SE_UART_TX_TRANS_LEN]         /* write len */
>> +
>> +        mov   w\c, #(UART_START_TX << M_OPCODE_SHFT)    /* Prepare cmd  */
>> +        str   w\c, [\xb, #SE_GENI_M_CMD0]               /* write cmd */
>> +.endm
>> +/*
>> + * wait for UART to be ready to transmit
>> + * xb: register which contains the UART base address
>> + * c: scratch register
>> + */
>> +.macro early_uart_ready xb c
>> +1:
>> +        ldr   w\c, [\xb, #SE_GENI_M_IRQ_STATUS] /* Load IRQ status */
>> +        tst   w\c, #M_TX_FIFO_WATERMARK_EN  /* Check TX_FIFI_WATERMARK_EN bit */
>> +        beq    1b                           /* Wait for the UART to be ready */
>> +.endm
>> +
>> +/*
>> + * UART transmit character
>> + * xb: register which contains the UART base address
>> + * wt: register which contains the character to transmit
>> + */
>> +.macro early_uart_transmit xb wt
>> +        str   \wt, [\xb, #SE_GENI_TX_FIFOn]             /* Put char to FIFO */
>> +        mov   \wt, #M_TX_FIFO_WATERMARK_EN              /* Prepare to FIFO */
>> +        str   \wt, [\xb, #SE_GENI_M_IRQ_CLEAR]          /* Kick FIFO */
>> +95:
>> +        ldr   \wt, [\xb, #SE_GENI_M_IRQ_STATUS]         /* Load IRQ status */
>> +        tst   \wt, #M_CMD_DONE_EN           /* Check TX_FIFO_WATERMARK_EN bit */
>> +        beq   95b                           /* Wait for the UART to be ready */
>> +        mov   \wt, #M_CMD_DONE_EN
>> +        str   \wt, [\xb, #SE_GENI_M_IRQ_CLEAR]
>> +
>> +        mov   \wt, #(UART_START_TX << M_OPCODE_SHFT)    /* Prepare next cmd */
>> +        str   \wt, [\xb, #SE_GENI_M_CMD0]               /* write cmd */
>> +.endm
>> +
>> +/*
>> + * Local variables:
>> + * mode: ASM
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/include/asm/qcom-uart.h b/xen/arch/arm/include/asm/qcom-uart.h
>> new file mode 100644
>> index 0000000000..dc9579374c
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/qcom-uart.h
>> @@ -0,0 +1,48 @@
>> +/*
>> + * xen/include/asm-arm/qcom-uart.h
> What's the use of this pseudo path? I would drop it or provide a correct path.
>
>> + *
>> + * Common constant definition between early printk and the UART driver
>> + * for the Qualcomm debug UART
>> + *
>> + * Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> + * Copyright (C) 2024, EPAM Systems.
>> + *
>> + * 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.
> No need for the license text. You should use SPDX identifier instead at the top of the file:
> /* SPDX-License-Identifier: GPL-2.0-or-later */
>
>> + */
>> +
>> +#ifndef __ASM_ARM_QCOM_UART_H
>> +#define __ASM_ARM_QCOM_UART_H
>> +
>> +#define SE_UART_TX_TRANS_LEN            0x270
>> +#define SE_GENI_M_CMD0                  0x600
>> +#define   UART_START_TX                 0x1
>> +#define   M_OPCODE_SHFT                 27
>> +
>> +#define SE_GENI_M_CMD_CTRL_REG          0x604
>> +#define   M_GENI_CMD_ABORT              BIT(1, U)
>> +#define SE_GENI_M_IRQ_STATUS            0x610
>> +#define   M_CMD_DONE_EN                 BIT(0, U)
>> +#define   M_CMD_ABORT_EN                BIT(5, U)
>> +#define   M_TX_FIFO_WATERMARK_EN        BIT(30, U)
>> +#define SE_GENI_M_IRQ_CLEAR             0x618
>> +#define SE_GENI_TX_FIFOn                0x700
>> +#define SE_GENI_TX_WATERMARK_REG        0x80c
> AFAICT, in this header you listed only regs used both by assembly and c code.
> However, SE_GENI_TX_WATERMARK_REG is not used in assembly.
> Also, my personal opinion is that it would make more sense to list here all the regs.

Okay, I'll move all the register to the header.

>> +
>> +#endif /* __ASM_ARM_QCOM_UART_H */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
>> index e18ec3788c..52c3934d06 100644
>> --- a/xen/drivers/char/Kconfig
>> +++ b/xen/drivers/char/Kconfig
>> @@ -68,6 +68,14 @@ config HAS_SCIF
>>           This selects the SuperH SCI(F) UART. If you have a SuperH based board,
>>           or Renesas R-Car Gen 2/3 based board say Y.
>> 
>> +config HAS_QCOM_UART
>> +       bool "Qualcomm GENI UART driver"
>> +       default y
>> +       depends on ARM
> Isn't is Arm64 specific device?
>

Probably yes. I believe it is safe to assume that it is Arm64-specific
in Xen case.

>> +       help
>> +         This selects the Qualcomm GENI-based UART driver. If you
>> +         have a Qualcomm-based board board say Y here.
>> +
>>  config HAS_EHCI
>>         bool
>>         depends on X86
>> diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
>> index e7e374775d..698ad0578c 100644
>> --- a/xen/drivers/char/Makefile
>> +++ b/xen/drivers/char/Makefile
>> @@ -7,6 +7,7 @@ obj-$(CONFIG_HAS_MESON) += meson-uart.o
>>  obj-$(CONFIG_HAS_MVEBU) += mvebu-uart.o
>>  obj-$(CONFIG_HAS_OMAP) += omap-uart.o
>>  obj-$(CONFIG_HAS_SCIF) += scif-uart.o
>> +obj-$(CONFIG_HAS_QCOM_UART) += qcom-uart.o
> Q comes before S
>
>>  obj-$(CONFIG_HAS_EHCI) += ehci-dbgp.o
>>  obj-$(CONFIG_XHCI) += xhci-dbc.o
>>  obj-$(CONFIG_HAS_IMX_LPUART) += imx-lpuart.o
>> diff --git a/xen/drivers/char/qcom-uart.c b/xen/drivers/char/qcom-uart.c
>> new file mode 100644
>> index 0000000000..2614051ca0
>> --- /dev/null
>> +++ b/xen/drivers/char/qcom-uart.c
>> @@ -0,0 +1,288 @@
>> +/*
>> + * xen/drivers/char/qcom-uart.c
>> + *
>> + * Driver for Qualcomm GENI-based UART interface
>> + *
>> + * Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> + *
>> + * Copyright (C) EPAM Systems 2024
>> + *
>> + * 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.
> No need for the license text. You should use SPDX identifier instead at the top of the file:
> /* SPDX-License-Identifier: GPL-2.0-or-later */
>
>> + */
>> +
>> +#include <xen/console.h>
>> +#include <xen/const.h>
>> +#include <xen/errno.h>
>> +#include <xen/serial.h>
>> +#include <xen/init.h>
>> +#include <xen/irq.h>
>> +#include <xen/mm.h>
>> +#include <xen/delay.h>
>> +#include <asm/device.h>
>> +#include <asm/qcom-uart.h>
>> +#include <asm/io.h>
>> +
>> +#define GENI_FORCE_DEFAULT_REG          0x20
>> +#define   FORCE_DEFAULT                 BIT(0, U)
>> +#define   DEF_TX_WM                     2
>> +#define SE_GENI_TX_PACKING_CFG0         0x260
>> +#define SE_GENI_TX_PACKING_CFG1         0x264
>> +#define SE_GENI_RX_PACKING_CFG0         0x284
>> +#define SE_GENI_RX_PACKING_CFG1         0x288
>> +#define SE_GENI_M_IRQ_EN                0x614
>> +#define   M_SEC_IRQ_EN                  BIT(31, U)
>> +#define   M_RX_FIFO_WATERMARK_EN        BIT(26, U)
>> +#define   M_RX_FIFO_LAST_EN             BIT(27, U)
>> +#define SE_GENI_S_CMD0                  0x630
>> +#define   UART_START_READ               0x1
>> +#define   S_OPCODE_SHFT                 27
>> +#define SE_GENI_S_CMD_CTRL_REG          0x634
>> +#define   S_GENI_CMD_ABORT              BIT(1, U)
>> +#define SE_GENI_S_IRQ_STATUS            0x640
>> +#define SE_GENI_S_IRQ_EN                0x644
>> +#define   S_RX_FIFO_LAST_EN             BIT(27, U)
>> +#define   S_RX_FIFO_WATERMARK_EN        BIT(26, U)
>> +#define   S_CMD_ABORT_EN                BIT(5, U)
>> +#define   S_CMD_DONE_EN                 BIT(0, U)
>> +#define SE_GENI_S_IRQ_CLEAR             0x648
>> +#define SE_GENI_RX_FIFOn                0x780
>> +#define SE_GENI_TX_FIFO_STATUS          0x800
>> +#define   TX_FIFO_WC                    GENMASK(27, 0)
>> +#define SE_GENI_RX_FIFO_STATUS          0x804
>> +#define   RX_LAST                       BIT(31, U)
>> +#define   RX_LAST_BYTE_VALID_MSK        GENMASK(30, 28)
>> +#define   RX_LAST_BYTE_VALID_SHFT       28
>> +#define   RX_FIFO_WC_MSK                GENMASK(24, 0)
>> +#define SE_GENI_TX_WATERMARK_REG        0x80c
>> +
>> +static struct qcom_uart {
>> +    unsigned int irq;
>> +    char __iomem *regs;
>> +    struct irqaction irqaction;
>> +} qcom_com = {0};
>> +
>> +static bool qcom_uart_poll_bit(void *addr, uint32_t mask, bool set)
>> +{
>> +    unsigned long timeout_us = 20000;
> Why UL and not U?
>
>> +    uint32_t reg;
>> +
>> +    while ( timeout_us ) {
> Brace should be placed on its own line
>
>> +        reg = readl(addr);
>> +        if ( (bool)(reg & mask) == set )
>> +            return true;
>> +        udelay(10);
>> +        timeout_us -= 10;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +static void __init qcom_uart_init_preirq(struct serial_port *port)
>> +{
>> +    struct qcom_uart *uart = port->uart;
>> +
>> +    /* Stop anything in TX that earlyprintk configured and clear all errors */
>> +    writel(M_GENI_CMD_ABORT, uart->regs + SE_GENI_M_CMD_CTRL_REG);
> It would be easier to creare wrappers that would improve readability:
> #define qcom_write(uart, off, val) writel((val), (uart)->regs + (off))
> #define qcom_read(uart, off) readl((uart)->regs + (off))
>
>> +    qcom_uart_poll_bit(uart->regs + SE_GENI_M_IRQ_STATUS, M_CMD_ABORT_EN,
>> +                       true);
>> +    writel(M_CMD_ABORT_EN, uart->regs + SE_GENI_M_IRQ_CLEAR);
>> +
>> +    /*
>> +     * Configure FIFO length: 1 byte per FIFO entry. This is terribly
>> +     * ineffective, as it is possible to cram 4 bytes per FIFO word,
>> +     * like Linux does. But using one byte per FIFO entry makes this
>> +     * driver much simpler.
>> +     */
>> +    writel(0xf, uart->regs + SE_GENI_TX_PACKING_CFG0);
>> +    writel(0x0, uart->regs + SE_GENI_TX_PACKING_CFG1);
>> +    writel(0xf, uart->regs + SE_GENI_RX_PACKING_CFG0);
>> +    writel(0x0, uart->regs + SE_GENI_RX_PACKING_CFG1);
>> +
>> +    /* Reset RX state machine */
>> +    writel(S_GENI_CMD_ABORT, uart->regs + SE_GENI_S_CMD_CTRL_REG);
>> +    qcom_uart_poll_bit(uart->regs + SE_GENI_S_CMD_CTRL_REG,
>> +                       S_GENI_CMD_ABORT, false);
>> +    writel(S_CMD_DONE_EN | S_CMD_ABORT_EN, uart->regs + SE_GENI_S_IRQ_CLEAR);
>> +    writel(FORCE_DEFAULT, uart->regs + GENI_FORCE_DEFAULT_REG);
>> +}
>> +
>> +static void qcom_uart_interrupt(int irq, void *data, struct cpu_user_regs *regs)
> serial_rx_interrupt has been modified not to take regs as parameter. Your patch breaks the build here.
> You need to remove regs from here and ...
>
>> +{
>> +    struct serial_port *port = data;
>> +    struct qcom_uart *uart = port->uart;
>> +    uint32_t m_irq_status, s_irq_status;
>> +
>> +    m_irq_status = readl(uart->regs + SE_GENI_M_IRQ_STATUS);
>> +    s_irq_status = readl(uart->regs + SE_GENI_S_IRQ_STATUS);
>> +    writel(m_irq_status, uart->regs + SE_GENI_M_IRQ_CLEAR);
>> +    writel(s_irq_status, uart->regs + SE_GENI_S_IRQ_CLEAR);
>> +
>> +    if ( s_irq_status & (S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN) )
>> +        serial_rx_interrupt(port, regs);
> here.
>
>> +}
>> +
>> +static void __init qcom_uart_init_postirq(struct serial_port *port)
>> +{
>> +    struct qcom_uart *uart = port->uart;
>> +    int rc;
>> +    uint32_t val;
>> +
>> +    uart->irqaction.handler = qcom_uart_interrupt;
>> +    uart->irqaction.name    = "qcom_uart";
>> +    uart->irqaction.dev_id  = port;
>> +
>> +    if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
> Value assigned to rc does not seem to be used at all
>
>> +        dprintk(XENLOG_ERR, "Failed to allocated qcom_uart IRQ %d\n",
>> +                uart->irq);
>> +
>> +    /* Enable TX/RX and Error Interrupts  */
>> +    writel(S_GENI_CMD_ABORT, uart->regs + SE_GENI_S_CMD_CTRL_REG);
>> +    qcom_uart_poll_bit(uart->regs + SE_GENI_S_CMD_CTRL_REG,
>> +                       S_GENI_CMD_ABORT, false);
>> +    writel(S_CMD_DONE_EN | S_CMD_ABORT_EN, uart->regs + SE_GENI_S_IRQ_CLEAR);
>> +    writel(FORCE_DEFAULT, uart->regs + GENI_FORCE_DEFAULT_REG);
>> +
>> +    val = readl(uart->regs + SE_GENI_S_IRQ_EN);
>> +    val = S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN;
>> +    writel(val, uart->regs + SE_GENI_S_IRQ_EN);
>> +
>> +    val = readl(uart->regs + SE_GENI_M_IRQ_EN);
>> +    val = M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN;
>> +    writel(val, uart->regs + SE_GENI_M_IRQ_EN);
>> +
>> +    /* Send RX command */
>> +    writel(UART_START_READ << S_OPCODE_SHFT, uart->regs + SE_GENI_S_CMD0);
>> +    qcom_uart_poll_bit(uart->regs + SE_GENI_M_IRQ_STATUS, M_SEC_IRQ_EN,
>> +                       true);
>> +}
>> +
>> +static void qcom_uart_putc(struct serial_port *port, char c)
>> +{
>> +    struct qcom_uart *uart = port->uart;
>> +    uint32_t irq_clear = M_CMD_DONE_EN;
>> +    uint32_t m_cmd;
>> +    bool done;
>> +
>> +    /* Setup TX */
>> +    writel(1, uart->regs + SE_UART_TX_TRANS_LEN);
>> +
>> +    writel(DEF_TX_WM, uart->regs + SE_GENI_TX_WATERMARK_REG);
>> +
>> +    m_cmd = UART_START_TX << M_OPCODE_SHFT;
>> +    writel(m_cmd, uart->regs + SE_GENI_M_CMD0);
>> +
>> +    qcom_uart_poll_bit(uart->regs + SE_GENI_M_IRQ_STATUS,
>> +                       M_TX_FIFO_WATERMARK_EN, true);
>> +
>> +    writel(c, uart->regs + SE_GENI_TX_FIFOn);
>> +    writel(M_TX_FIFO_WATERMARK_EN, uart->regs + SE_GENI_M_IRQ_CLEAR);
>> +
>> +    /* Check for TX done */
>> +    done = qcom_uart_poll_bit(uart->regs + SE_GENI_M_IRQ_STATUS, M_CMD_DONE_EN,
>> +                              true);
>> +    if ( !done )
>> +    {
>> +        writel(M_GENI_CMD_ABORT, uart->regs + SE_GENI_M_CMD_CTRL_REG);
>> +        irq_clear |= M_CMD_ABORT_EN;
>> +        qcom_uart_poll_bit(uart->regs + SE_GENI_M_IRQ_STATUS, M_CMD_ABORT_EN,
>> +                           true);
>> +
>> +    }
>> +    writel(irq_clear, uart->regs + SE_GENI_M_IRQ_CLEAR);
>> +}
>> +
>> +static int qcom_uart_getc(struct serial_port *port, char *pc)
>> +{
>> +    struct qcom_uart *uart = port->uart;
>> +
>> +    if ( !readl(uart->regs + SE_GENI_RX_FIFO_STATUS) )
>> +        return 0;
>> +
>> +    *pc = readl(uart->regs + SE_GENI_RX_FIFOn) & 0xFF;
>> +
>> +    writel(UART_START_READ << S_OPCODE_SHFT, uart->regs + SE_GENI_S_CMD0);
>> +    qcom_uart_poll_bit(uart->regs + SE_GENI_M_IRQ_STATUS, M_SEC_IRQ_EN,
>> +                       true);
>> +
>> +    return 1;
>> +
>> +}
>> +
>> +static struct uart_driver __read_mostly qcom_uart_driver = {
>> +    .init_preirq  = qcom_uart_init_preirq,
>> +    .init_postirq = qcom_uart_init_postirq,
>> +    .putc         = qcom_uart_putc,
>> +    .getc         = qcom_uart_getc,
>> +};
>> +
>> +static const struct dt_device_match qcom_uart_dt_match[] __initconst =
>> +{
>> +    { .compatible = "qcom,geni-debug-uart"},
>> +    { /* sentinel */ },
>> +};
> Can you move it right before DT_DEVICE_START?.
>
>> +
>> +static int __init qcom_uart_init(struct dt_device_node *dev,
>> +                                 const void *data)
>> +{
>> +    const char *config = data;
>> +    struct qcom_uart *uart;
>> +    int res;
>> +    paddr_t addr, size;
>> +
>> +    if ( strcmp(config, "") )
>> +        printk("WARNING: UART configuration is not supported\n");
>> +
>> +    uart = &qcom_com;
>> +
>> +    res = dt_device_get_paddr(dev, 0, &addr, &size);
>> +    if ( res )
>> +    {
>> +        printk("qcom-uart: Unable to retrieve the base"
>> +               " address of the UART\n");
>> +        return res;
>> +    }
>> +
>> +    res = platform_get_irq(dev, 0);
>> +    if ( res < 0 )
>> +    {
>> +        printk("qcom-uart: Unable to retrieve the IRQ\n");
>> +        return res;
>> +    }
>> +    uart->irq = res;
>> +
>> +    uart->regs = ioremap_nocache(addr, size);
>> +    if ( !uart->regs )
>> +    {
>> +        printk("qcom-uart: Unable to map the UART memory\n");
>> +        return -ENOMEM;
>> +    }
>> +
>> +    /* Register with generic serial driver */
>> +    serial_register_uart(SERHND_DTUART, &qcom_uart_driver, uart);
>> +
>> +    dt_device_set_used_by(dev, DOMID_XEN);
>> +
>> +    return 0;
>> +}
>> +
>> +DT_DEVICE_START(qcom_uart, "QCOM UART", DEVICE_SERIAL)
>> +    .dt_match = qcom_uart_dt_match,
>> +    .init = qcom_uart_init,
>> +DT_DEVICE_END
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> --
>> 2.43.0
>
> What about vUART? You don't seem to set vuart information that is used by vuart.c and vpl011.c
>

I am not sure that it is possible to emulate GENI UART with simple vuart
API. vuart makes assumption that there is one simple status register and
FIFO register operates on per-character basis, while even earlycon part
of the driver in Linux tries to pack 4 characters to one FIFO write.

Thank you for the review. I'll address all other comments as you
suggested. 

-- 
WBR, Volodymyr