[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/3] drivers: serial: add Qualcomm GENI-based serial driver
Hi Michal, Michal Orzel <michal.orzel@xxxxxxx> 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@xxxxxxxx> >> --- >> 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@xxxxxxxx> >> + * 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@xxxxxxxx> >> + * 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@xxxxxxxx> >> + * >> + * 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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |