Hi Michal,
Michal Orzel <michal.or...@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_babc...@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_babc...@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_babc...@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_babc...@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
