[PATCH 0/3] Add experimental support for Qualcomm SA8155P SoC

[PATCH 0/3] Add experimental support for Qualcomm SA8155P SoC

[Volodymyr Babchuk]

Hello,

This three patches are all what is needed to run Xen on Qualcomm
SA8155P. At the time of writing, I have a working setup with
(almost) mainline Linux kernel in Dom0, where basic features like UFS
and networking are working fine, but more advanced things like GPU are
not supported yet.

Volodymyr Babchuk (3):
  arm: smmu: allow SMMU to have more IRQs than context banks
  drivers: serial: add Qualcomm GENI-based serial driver
  arm: platform: qcom: add basic support SA8155P SoC

 xen/arch/arm/Kconfig.debug           |  19 +-
 xen/arch/arm/arm64/debug-qcom.inc    |  76 +++++++
 xen/arch/arm/include/asm/qcom-uart.h |  48 +++++
 xen/arch/arm/platforms/Makefile      |   1 +
 xen/arch/arm/platforms/qcom.c        |  77 +++++++
 xen/drivers/char/Kconfig             |   8 +
 xen/drivers/char/Makefile            |   1 +
 xen/drivers/char/qcom-uart.c         | 288 +++++++++++++++++++++++++++
 xen/drivers/passthrough/arm/smmu.c   |   2 +-
 9 files changed, 518 insertions(+), 2 deletions(-)
 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/arch/arm/platforms/qcom.c
 create mode 100644 xen/drivers/char/qcom-uart.c

-- 
2.43.0

[PATCH 1/3] arm: smmu: allow SMMU to have more IRQs than context banks

[Volodymyr Babchuk]

I encountered platform, namely Qualcomm SA8155P where SMMU-compatible
IO-MMU advertises more context IQRs than there are context banks. This
should not be an issue, we need to relax the check in the SMMU driver
to allow such configuration.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 xen/drivers/passthrough/arm/smmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 32e2ff279b..2dd3688f3b 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2550,7 +2550,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	parse_driver_options(smmu);
 
 	if (smmu->version > ARM_SMMU_V1 &&
-	    smmu->num_context_banks != smmu->num_context_irqs) {
+	    smmu->num_context_banks > smmu->num_context_irqs) {
 		dev_err(dev,
 			"found only %d context interrupt(s) but %d required\n",
 			smmu->num_context_irqs, smmu->num_context_banks);
-- 
2.43.0

Re: [PATCH 1/3] arm: smmu: allow SMMU to have more IRQs than context banks

[Michal Orzel]

Hello,

On 29/03/2024 01:08, Volodymyr Babchuk wrote:
> 
> 
> I encountered platform, namely Qualcomm SA8155P where SMMU-compatible
NIT: a commit msg should be written in imperative mood

> IO-MMU advertises more context IQRs than there are context banks. This
> should not be an issue, we need to relax the check in the SMMU driver
> to allow such configuration.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>  xen/drivers/passthrough/arm/smmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 32e2ff279b..2dd3688f3b 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2550,7 +2550,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>         parse_driver_options(smmu);
> 
>         if (smmu->version > ARM_SMMU_V1 &&
> -           smmu->num_context_banks != smmu->num_context_irqs) {
> +           smmu->num_context_banks > smmu->num_context_irqs) {
This was done in Linux by commit:
d1e20222d537 ("iommu/arm-smmu: Error out only if not enough context interrupts")

However, they also ignore superfluous interrupts. Shouldn't we do the same?

~Michal

Re: [PATCH 1/3] arm: smmu: allow SMMU to have more IRQs than context banks

[Julien Grall]

Hi,

On 02/04/2024 08:28, Michal Orzel wrote:
> Hello,
> 
> On 29/03/2024 01:08, Volodymyr Babchuk wrote:
>>
>>
>> I encountered platform, namely Qualcomm SA8155P where SMMU-compatible
> NIT: a commit msg should be written in imperative mood
> 
>> IO-MMU advertises more context IQRs than there are context banks. This
>> should not be an issue, we need to relax the check in the SMMU driver
>> to allow such configuration.
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>   xen/drivers/passthrough/arm/smmu.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
>> index 32e2ff279b..2dd3688f3b 100644
>> --- a/xen/drivers/passthrough/arm/smmu.c
>> +++ b/xen/drivers/passthrough/arm/smmu.c
>> @@ -2550,7 +2550,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>>          parse_driver_options(smmu);
>>
>>          if (smmu->version > ARM_SMMU_V1 &&
>> -           smmu->num_context_banks != smmu->num_context_irqs) {
>> +           smmu->num_context_banks > smmu->num_context_irqs) {
> This was done in Linux by commit:
> d1e20222d537 ("iommu/arm-smmu: Error out only if not enough context interrupts")
> 
> However, they also ignore superfluous interrupts. Shouldn't we do the same?

+1. It would be better to avoid allocating stating for IRQs that are 
never used.

Cheers,

-- 
Julien Grall

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

[Volodymyr Babchuk]

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.

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 +++++++
 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
+		select EARLY_UART_QCOM
+		bool "Early printk via Qualcomm debug UART"
+		help
+			Say Y here if you wish the early printk to direct their
+			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
 
 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.
+ */
+
+#include <asm/qcom-uart.h>
+
+.macro early_uart_init xb c
+        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 */
+        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
+ *
+ * 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.
+ */
+
+#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
+
+#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
+	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
 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.
+ */
+
+#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;
+    uint32_t reg;
+
+    while ( timeout_us ) {
+        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);
+    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)
+{
+    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);
+}
+
+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 )
+        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 */ },
+};
+
+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

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

[Jan Beulich]

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

This last new file wants mentioning in ./MAINTAINERS'es "ARM (W/
VIRTUALISATION EXTENSIONS) ARCHITECTURE" section, I suppose.

Jan

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

[Michal Orzel]

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?

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


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

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

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

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

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

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

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

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

~Michal

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

[Volodymyr Babchuk]

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

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

[Michal Orzel]

On 02/04/2024 23:19, Volodymyr Babchuk wrote:
> 
> 
> 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...
Yes, please add a comment so that a person reading a code can understand the rationale.

[...]

>> 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.
Ok. It might make sense to mention this in commit msg (no vuart, no vpl011 for direct mapped domains)

> 
> Thank you for the review. I'll address all other comments as you
> suggested.
> 
> --
> WBR, Volodymyr

~Michal

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

[Julien Grall]

Hi,

On 02/04/2024 22:19, Volodymyr Babchuk wrote:
> 
> 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.
If you don't provide the early printk for arm32, then you need to add 
DEPENDS ON otherwise randconfig will fail on arm32.

[...]

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

AFAIU, this means we would potentially do one too many iteration. Does 
this have any implication to the runtime UART driver?

But why do we need to repeat step 2/3? Is this because both registers 
will be reset after the character is written?

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

Would you mind to explain what you mean with "just in case"? A pointer
to the corresponding Linux/U-boot code would be helpful.

[...]

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

Is it because there is no 32-bit HW from qualcomm that supports 
virtualization?

[...]

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

So I agree that enabling vUART is probably going to be difficult. But 
you should be able to implement the vpl011 as it only require a base and 
an IRQ.

That said, from the driver PoV, there is no differentiation today. We 
could add a extra parameter, but I don't think this needs to be handled 
in this series.

Cheers,

-- 
Julien Grall

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

[Michal Orzel]

Hi,

On 02/04/2024 12:25, Michal Orzel wrote:
> 
> 
> 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?
> 
>>
>> 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?
> 
> 
>>  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.
> 
>> +               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?
> 
>> +               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)
> 
>> +                       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 */
I chatted with Julien and it would be best to use GPL-2.0-only.

~Michal

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

[Julien Grall]

Hi,

On 29/03/2024 00: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.

If this is based on Linux/U-boot, then I assume you read the code and 
possibly copy/paste some of it. Therefore...

> 
> 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 +++++++
>   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
> +		select EARLY_UART_QCOM
> +		bool "Early printk via Qualcomm debug UART"
> +		help
> +			Say Y here if you wish the early printk to direct their
> +			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
>   
>   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.

... I don't think you can use GPLv2+ license here. It has to be the same 
as Linux/U-boot.

Also, as there is no public manual, can you provide some pointer to the 
code from both repo (including version)? This would help the reviewer to 
confirm you code.

Cheers,

-- 
Julien Grall

[PATCH 3/3] arm: platform: qcom: add basic support SA8155P SoC

[Volodymyr Babchuk]

Qualcomm SA8155P is the automotive variant of SM8150 aka Snapdragon
855.

This patch adds very basic support for the platform. We need to handle
Qualcomm-specific SMC to workaround quirk in the QCOM SCM driver in
the Linux kernel. Basically the driver tries multiple different SMCs
to determine which calling convention is supported by a SoC. If all
calls fail it decides that the SoC uses "legacy SMC" and tries to
communicate with SCM by issuing SMC with funcid = 1. Problem is that
Xen has own understanding on how such SMC should be handled. It
interprets this SMC as legacy PSCI_cpu_off and happily turns of Linux
boot CPU.

To workaround this, we pretend that we support
QCOM_SCM_INFO_IS_CALL_AVAIL, this will make the driver use the latest
calling convention. All subsequent calls will fail anyways and the
driver will terminate self gracefully. This is not a big deal, because
right now (with Linux 6.8) even on baremetal setup the driver fails
anyways, because it does not know how to work with this SoC.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 xen/arch/arm/platforms/Makefile |  1 +
 xen/arch/arm/platforms/qcom.c   | 77 +++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)
 create mode 100644 xen/arch/arm/platforms/qcom.c

diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
index 8632f4115f..6873735ef0 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) += qcom.o
 obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
 obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp-eemi.o
diff --git a/xen/arch/arm/platforms/qcom.c b/xen/arch/arm/platforms/qcom.c
new file mode 100644
index 0000000000..77e9c58649
--- /dev/null
+++ b/xen/arch/arm/platforms/qcom.c
@@ -0,0 +1,77 @@
+/*
+ * xen/arch/arm/platforms/qcom.c
+ *
+ * Qualcomm SoCs specific 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.
+ */
+
+#include <asm/platform.h>
+#include <public/arch-arm/smccc.h>
+#include <asm/smccc.h>
+
+#define SCM_SMC_FNID(s, c)	((((s) & 0xFF) << 8) | ((c) & 0xFF))
+#define QCOM_SCM_SVC_INFO		0x06
+#define QCOM_SCM_INFO_IS_CALL_AVAIL	0x01
+
+#define ARM_SMCCC_SIP_QCOM_SCM_IS_CALL_AVAIL                            \
+    ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,                             \
+                       ARM_SMCCC_CONV_64,                               \
+                       ARM_SMCCC_OWNER_SIP,                             \
+                       SCM_SMC_FNID(QCOM_SCM_SVC_INFO,                  \
+                                    QCOM_SCM_INFO_IS_CALL_AVAIL))
+
+static const char * const sa8155p_dt_compat[] __initconst =
+{
+    "qcom,sa8155p",
+    NULL
+};
+
+static bool sa8155p_smc(struct cpu_user_regs *regs)
+{
+    uint32_t funcid = get_user_reg(regs, 0);
+
+    switch ( funcid ) {
+    case ARM_SMCCC_SIP_QCOM_SCM_IS_CALL_AVAIL:
+        /*
+         * We need to implement this specific call only to make Linux
+         * counterpart happy: QCOM SCM driver in Linux tries to
+         * determine calling convention by issuing this particular
+         * SMC. If it receives an error it assumes that platform uses
+         * legacy calling convention and tries to issue SMC with
+         * funcid = 1. Xen interprets this as PSCI_cpu_off and turns
+         * off Linux boot vCPU.
+         */
+        set_user_reg(regs, 0, ARM_SMCCC_SUCCESS);
+        set_user_reg(regs, 1, 1);
+        return true;
+    default:
+        return false;
+    }
+}
+
+PLATFORM_START(sa8155p, "Qualcomm SA8155P")
+    .compatible = sa8155p_dt_compat,
+    .smc = sa8155p_smc
+PLATFORM_END
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.43.0

Re: [PATCH 3/3] arm: platform: qcom: add basic support SA8155P SoC

[Michal Orzel]

Hello,

On 29/03/2024 01:08, Volodymyr Babchuk wrote:
> 
> 
> Qualcomm SA8155P is the automotive variant of SM8150 aka Snapdragon
> 855.
> 
> This patch adds very basic support for the platform. We need to handle
> Qualcomm-specific SMC to workaround quirk in the QCOM SCM driver in
> the Linux kernel. Basically the driver tries multiple different SMCs
> to determine which calling convention is supported by a SoC. If all
> calls fail it decides that the SoC uses "legacy SMC" and tries to
> communicate with SCM by issuing SMC with funcid = 1. Problem is that
> Xen has own understanding on how such SMC should be handled. It
> interprets this SMC as legacy PSCI_cpu_off and happily turns of Linux
> boot CPU.
> 
> To workaround this, we pretend that we support
> QCOM_SCM_INFO_IS_CALL_AVAIL, this will make the driver use the latest
> calling convention. All subsequent calls will fail anyways and the
> driver will terminate self gracefully. This is not a big deal, because
> right now (with Linux 6.8) even on baremetal setup the driver fails
> anyways, because it does not know how to work with this SoC.
Therefore I would consider adding a Kconfig option and placing it under experimental

> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>  xen/arch/arm/platforms/Makefile |  1 +
>  xen/arch/arm/platforms/qcom.c   | 77 +++++++++++++++++++++++++++++++++
>  2 files changed, 78 insertions(+)
>  create mode 100644 xen/arch/arm/platforms/qcom.c
> 
> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> index 8632f4115f..6873735ef0 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) += qcom.o
>  obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
>  obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp-eemi.o
> diff --git a/xen/arch/arm/platforms/qcom.c b/xen/arch/arm/platforms/qcom.c
> new file mode 100644
> index 0000000000..77e9c58649
> --- /dev/null
> +++ b/xen/arch/arm/platforms/qcom.c
> @@ -0,0 +1,77 @@
> +/*
> + * xen/arch/arm/platforms/qcom.c
> + *
> + * Qualcomm SoCs specific 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.
Please use SPDX identifier instead of license text

> + */
> +
> +#include <asm/platform.h>
> +#include <public/arch-arm/smccc.h>
no need for this public header

> +#include <asm/smccc.h>
> +
> +#define SCM_SMC_FNID(s, c)     ((((s) & 0xFF) << 8) | ((c) & 0xFF))
spaces instead of tabs

> +#define QCOM_SCM_SVC_INFO              0x06
spaces instead of tabs

> +#define QCOM_SCM_INFO_IS_CALL_AVAIL    0x01
> +
> +#define ARM_SMCCC_SIP_QCOM_SCM_IS_CALL_AVAIL                            \
> +    ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,                             \
> +                       ARM_SMCCC_CONV_64,                               \
> +                       ARM_SMCCC_OWNER_SIP,                             \
> +                       SCM_SMC_FNID(QCOM_SCM_SVC_INFO,                  \
> +                                    QCOM_SCM_INFO_IS_CALL_AVAIL))
> +
> +static const char * const sa8155p_dt_compat[] __initconst =
> +{
> +    "qcom,sa8155p",
> +    NULL
> +};
> +
> +static bool sa8155p_smc(struct cpu_user_regs *regs)
> +{
> +    uint32_t funcid = get_user_reg(regs, 0);
> +
> +    switch ( funcid ) {
brace should be placed on a new line

~Michal

Re: [PATCH 3/3] arm: platform: qcom: add basic support SA8155P SoC

[Julien Grall]

Hi Volodymyr,

On 29/03/2024 00:08, Volodymyr Babchuk wrote:
> Qualcomm SA8155P is the automotive variant of SM8150 aka Snapdragon
> 855.
> 
> This patch adds very basic support for the platform. We need to handle
> Qualcomm-specific SMC to workaround quirk in the QCOM SCM driver in
> the Linux kernel. Basically the driver tries multiple different SMCs
> to determine which calling convention is supported by a SoC. If all
> calls fail it decides that the SoC uses "legacy SMC" and tries to
> communicate with SCM by issuing SMC with funcid = 1. Problem is that
> Xen has own understanding on how such SMC should be handled. It
> interprets this SMC as legacy PSCI_cpu_off and happily turns of Linux
> boot CPU.
> 
> To workaround this, we pretend that we support
> QCOM_SCM_INFO_IS_CALL_AVAIL, this will make the driver use the latest
> calling convention. All subsequent calls will fail anyways and the
> driver will terminate self gracefully. This is not a big deal, because
> right now (with Linux 6.8) even on baremetal setup the driver fails
> anyways, because it does not know how to work with this SoC.

Is there any patches that are not yet merged and/or on a Qualcomm 
specific tree? I just want to make sure that we have a code that will 
still work in the near future.

> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>   xen/arch/arm/platforms/Makefile |  1 +
>   xen/arch/arm/platforms/qcom.c   | 77 +++++++++++++++++++++++++++++++++
>   2 files changed, 78 insertions(+)
>   create mode 100644 xen/arch/arm/platforms/qcom.c
> 
> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> index 8632f4115f..6873735ef0 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) += qcom.o
>   obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
>   obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp-eemi.o
> diff --git a/xen/arch/arm/platforms/qcom.c b/xen/arch/arm/platforms/qcom.c
> new file mode 100644
> index 0000000000..77e9c58649
> --- /dev/null
> +++ b/xen/arch/arm/platforms/qcom.c
> @@ -0,0 +1,77 @@
> +/*
> + * xen/arch/arm/platforms/qcom.c
> + *
> + * Qualcomm SoCs specific 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.

Did you intend to license the code as GPLv2+? See [1], for some context.

> + *
> + * 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 <asm/platform.h>
> +#include <public/arch-arm/smccc.h>
> +#include <asm/smccc.h>
> +
> +#define SCM_SMC_FNID(s, c)	((((s) & 0xFF) << 8) | ((c) & 0xFF))
> +#define QCOM_SCM_SVC_INFO		0x06
> +#define QCOM_SCM_INFO_IS_CALL_AVAIL	0x01
> +
> +#define ARM_SMCCC_SIP_QCOM_SCM_IS_CALL_AVAIL                            \

I find this name a little bit too long. Can we remove ARM_SMCCC as I 
don't think it adds any value?

> +    ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,                             \
> +                       ARM_SMCCC_CONV_64,                               \
> +                       ARM_SMCCC_OWNER_SIP,                             \
> +                       SCM_SMC_FNID(QCOM_SCM_SVC_INFO,                  \
> +                                    QCOM_SCM_INFO_IS_CALL_AVAIL))
> +
> +static const char * const sa8155p_dt_compat[] __initconst =
> +{
> +    "qcom,sa8155p",
> +    NULL
> +};
> +
> +static bool sa8155p_smc(struct cpu_user_regs *regs)
> +{
> +    uint32_t funcid = get_user_reg(regs, 0);
> +

The function will also called for guests. But only the hardware 
domain/dom0 is using the machine qcom,sa8155p, so I think you want to 
check the domain.

> +    switch ( funcid ) {
> +    case ARM_SMCCC_SIP_QCOM_SCM_IS_CALL_AVAIL:
> +        /*
> +         * We need to implement this specific call only to make Linux
> +         * counterpart happy: QCOM SCM driver in Linux tries to
> +         * determine calling convention by issuing this particular
> +         * SMC. If it receives an error it assumes that platform uses
> +         * legacy calling convention and tries to issue SMC with
> +         * funcid = 1. Xen interprets this as PSCI_cpu_off and turns
> +         * off Linux boot vCPU.
> +         */
> +        set_user_reg(regs, 0, ARM_SMCCC_SUCCESS);
> +        set_user_reg(regs, 1, 1);
> +        return true;
> +    default:
> +        return false;
> +    }
> +}
> +
> +PLATFORM_START(sa8155p, "Qualcomm SA8155P")
> +    .compatible = sa8155p_dt_compat,
> +    .smc = sa8155p_smc
> +PLATFORM_END
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

Cheers,

[1] https://lore.kernel.org/1272bb77-8a93-4ca2-af0d-4c1c36729307@xen.org

-- 
Julien Grall