[PATCH v5 0/5] Add support to load QUP SE firmware from

[PATCH v5 0/5] Add support to load QUP SE firmware from

[Viken Dadhaniya]

In Qualcomm SoCs, firmware loading for Serial Engines (SE) in the QUP
hardware has traditionally been managed by TrustZone (TZ). This setup
handled Serial Engines(SE) assignments and access control permissions,
ensuring a high level of security but limiting flexibility and
accessibility.
 
This limitation poses a significant challenge for developers who need more
flexibility to enable any protocol on any of the SEs within the QUP
hardware.
 
To address this, we are introducing a change that opens the firmware
loading mechanism to the Linux environment. This enhancement increases
flexibility and allows for more streamlined and efficient management. We
can now handle SE assignments and access control permissions directly
within Linux, eliminating the dependency on TZ.
 
We propose an alternative method for firmware loading and SE
ownership/transfer mode configuration based on device tree configuration.
This method does not rely on other execution environments, making it
accessible to all developers.
 
For SEs used prior to the kernel, their firmware will be loaded by the
respective image drivers (e.g., Debug UART, Secure or trusted SE).
Additionally, the GSI firmware, which is common to all SEs per QUPV3 core,
will not be loaded by Linux driver but TZ only. At the kernel level, only
the SE protocol driver should load the respective protocol firmware.
---
v4 -> v5:

- Added Reviewd-by tag.
- Resolved kernel test robot error by including the missing bitfield header file.
- Updated the SE firmware ELF structure name for consistency.
- Specified _leb4 format for the magic number definition.
- Updated the email domain from 'quic' to 'oss'.

v4 Link: https://lore.kernel.org/all/20250503111029.3583807-1-quic_vdadhani@quicinc.com/ 

v3 -> v4: 

- Drop patch 1 of the v3 series as it has been reviewed and merged.
- Update the qcom,gsi-dma-allowed property name to qcom,enable-gsi-dma.
- Remove the full stop from the title.
- Add a reference to the common schema YAML in the I2C, SPI, and SERIAL
  YAML files in a single patch and drop the individual patches for protocol YAML.
- Update the commit message.
- Resolve kernel test robot warnings.
- Add a multiline comment in the Copyright section.
- Remove valid_seg_size and geni_config_common_control functions and add the code inline.
- Rename read_elf function to geni_read_elf.
- Add a firmware size check.
- Assign *pelfseg after finding a match.
- Break one large condition check into multiple checks to improve code readability.
- Remove return type documentation for void functions.
- Update error messages to be more descriptive.
- Correct indentation.
- Rename geni_flash_fw_revision function to geni_write_fw_revision.
- Remove __func__ from all print statements.
- Move resource_on to the appropriate section after parsing the firmware file.
- Update variable names and function arguments as suggested.
- Use FIELD_GET, FIELD_PREP, and GENMASK.
- Use memcpy_toio() instead of memcpy.
- Remove duplicate registers and bitmask macros.
- Remove rsc struct and add required variables in geni_se struct.
- Add a patch dependency note.

v3 Link: https://lore.kernel.org/linux-arm-msm/20250303124349.3474185-1-quic_vdadhani@quicinc.com/ 

v2 -> v3:

- Add a new YAML file for QUP peripheral-specific properties for I2C, SPI, and SERIAL buses.
- Drop the 'qcom,xfer-mode' property and add the 'qcom,gsi-dma-allowed' property in protocol-specific YAML.
- Add a reference for the QUP peripheral shared YAML to protocol-specific YAML.
- Enhance error handling and remove redundant if conditions in the qcom-geni-se.c driver.
- Remove the ternary operator in the qup_fw_load function.
- Update function descriptions and use imperative mood in qcom-geni-se.c
- Load firmware during probe only if the protocol is invalid.

v2 Link: https://lore.kernel.org/linux-kernel/20250124105309.295769-1-quic_vdadhani@quicinc.com/ 
 
v1 -> v2:

- Drop the qcom,load-firmware property.
- Remove the fixed firmware path.
- Add the 'firmware-name' property in the QUP common driver.
- Add logic to read the firmware path from the device tree.
- Resolve kernel test robot warnings.
- Update the 'qcom,xfer-mode' property description.

v1 Link: https://lore.kernel.org/linux-kernel/20241204150326.1470749-1-quic_vdadhani@quicinc.com/ 
---
Viken Dadhaniya (5):
  dt-bindings: qcom: se-common: Add QUP Peripheral-specific properties
    for I2C, SPI, and SERIAL bus
  soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux
    subsystem
  i2c: qcom-geni: Load i2c qup Firmware from linux side
  spi: geni-qcom: Load spi qup Firmware from linux side
  serial: qcom-geni: Load UART qup Firmware from linux side

 .../bindings/i2c/qcom,i2c-geni-qcom.yaml      |   1 +
 .../serial/qcom,serial-geni-qcom.yaml         |   1 +
 .../soc/qcom/qcom,se-common-props.yaml        |  26 ++
 .../bindings/spi/qcom,spi-geni-qcom.yaml      |   1 +
 drivers/i2c/busses/i2c-qcom-geni.c            |   8 +-
 drivers/soc/qcom/qcom-geni-se.c               | 404 +++++++++++++++++-
 drivers/spi/spi-geni-qcom.c                   |   6 +
 drivers/tty/serial/qcom_geni_serial.c         |   8 +-
 include/linux/soc/qcom/geni-se.h              |  32 +-
 include/linux/soc/qcom/qup-fw-load.h          |  93 ++++
 10 files changed, 556 insertions(+), 24 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,se-common-props.yaml
 create mode 100644 include/linux/soc/qcom/qup-fw-load.h

-- 
2.34.1

[PATCH v5 1/5] dt-bindings: qcom: se-common: Add QUP Peripheral-specific properties for I2C, SPI, and SERIAL bus

[Viken Dadhaniya]

Introduce a new YAML schema for QUP-supported peripherals. Define common
properties used across QUP-supported peripherals.

Add property `qcom,enable-gsi-dma` to configure the Serial Engine (SE) for
QCOM GPI DMA mode.

Reference the common schema YAML in the GENI I2C, SPI, and SERIAL YAML
files.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Co-developed-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Signed-off-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Signed-off-by: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
---
v4 -> v5:

- Add Reviewed-by tag.
- Update the email domain from 'quic' to 'oss'.

v4 Link: https://lore.kernel.org/all/20250503111029.3583807-2-quic_vdadhani@quicinc.com/

v3 -> v4:

- Update qcom,gsi-dma-allowed property name to qcom,enable-gsi-dma.
- Remove full stop form title.
- Add reference of common schema YAML in the I2C, SPI, and SERIAL YAML files.

v3 Link: https://lore.kernel.org/linux-arm-msm/20250303124349.3474185-3-quic_vdadhani@quicinc.com/
---
---
 .../bindings/i2c/qcom,i2c-geni-qcom.yaml      |  1 +
 .../serial/qcom,serial-geni-qcom.yaml         |  1 +
 .../soc/qcom/qcom,se-common-props.yaml        | 26 +++++++++++++++++++
 .../bindings/spi/qcom,spi-geni-qcom.yaml      |  1 +
 4 files changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,se-common-props.yaml

diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
index 9f66a3bb1f80..51534953a69c 100644
--- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
+++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
@@ -75,6 +75,7 @@ required:
 
 allOf:
   - $ref: /schemas/i2c/i2c-controller.yaml#
+  - $ref: /schemas/soc/qcom/qcom,se-common-props.yaml#
   - if:
       properties:
         compatible:
diff --git a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
index dd33794b3534..ed7b3909d87d 100644
--- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
+++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
@@ -12,6 +12,7 @@ maintainers:
 
 allOf:
   - $ref: /schemas/serial/serial.yaml#
+  - $ref: /schemas/soc/qcom/qcom,se-common-props.yaml#
 
 properties:
   compatible:
diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,se-common-props.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,se-common-props.yaml
new file mode 100644
index 000000000000..6a34f05a07e8
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,se-common-props.yaml
@@ -0,0 +1,26 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/qcom/qcom,se-common-props.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: QUP Peripheral-specific properties for I2C, SPI and SERIAL bus
+
+description:
+  The Generic Interface (GENI) based Qualcomm Universal Peripheral (QUP) is
+  a programmable module that supports a wide range of serial interfaces
+  such as UART, SPI, I2C, I3C, etc. This defines the common properties used
+  across QUP-supported peripherals.
+
+maintainers:
+  - Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
+  - Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
+
+properties:
+  qcom,enable-gsi-dma:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Configure the Serial Engine (SE) to transfer data in QCOM GPI DMA mode.
+      By default, FIFO mode (PIO/CPU DMA) will be selected.
+
+additionalProperties: true
diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml b/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml
index 2e20ca313ec1..d12c5a060ed0 100644
--- a/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml
+++ b/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml
@@ -25,6 +25,7 @@ description:
 
 allOf:
   - $ref: /schemas/spi/spi-controller.yaml#
+  - $ref: /schemas/soc/qcom/qcom,se-common-props.yaml#
 
 properties:
   compatible:
-- 
2.34.1

[PATCH v5 2/5] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem

[Viken Dadhaniya]

In Qualcomm SoCs, firmware loading for Serial Engines (SE) within the QUP
hardware has traditionally been managed by TrustZone (TZ). This restriction
poses a significant challenge for developers, as it limits their ability to
enable various protocols on any of the SEs from the Linux side, reducing
flexibility.

Load the firmware to QUP SE based on the 'firmware-name' property specified
in devicetree at bootup time.

Co-developed-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Signed-off-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Signed-off-by: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
---
v4 -> v5:

- Resolved kernel test robot error by including the missing bitfield header file.
- Updated the SE firmware ELF structure name for consistency.
- Specified _leb4 format for the magic number definition.
- Updated the email domain from 'quic' to 'oss'.

v4 Link: https://lore.kernel.org/all/20250503111029.3583807-3-quic_vdadhani@quicinc.com/

v3 -> v4:

- Update the commit message.
- Resolve kernel test robot warnings.
- Add a multiline comment in the Copyright section.
- Remove valid_seg_size and geni_config_common_control functions, and add the code inline.
- Rename read_elf function to geni_read_elf.
- Add a firmware size check.
- Assign *pelfseg after finding a match.
- Break one large condition check into multiple checks to improve code readability.
- Remove return type documentation for void functions.
- Update error messages to be more descriptive.
- Correct indentation.
- Rename geni_flash_fw_revision function to geni_write_fw_revision.
- Remove __func__ from all print statements.
- Move resource_on to the appropriate section after parsing the firmware file.
- Update variable names and function arguments as suggested.
- Use FIELD_GET, FIELD_PREP, and GENMASK.
- Use memcpy_toio() instead of memcpy.
- Remove duplicate registers and bitmask macros.
- Remove rsc struct and add required variables in geni_se struct.

v3 Link: https://lore.kernel.org/linux-arm-msm/20250303124349.3474185-7-quic_vdadhani@quicinc.com/

v2 -> v3:

- Remove code related to the 'qcom,xfer-mode' property.
- Add logic to read the boolean property 'qcom,gsi-dma-allowed' and select the transfer mode.
- Hardcode FIFO mode for the serial driver as GSI mode is currently not supported.
- Update function descriptions as suggested.
- Enhance error handling and remove redundant if conditions.
- Drop the ternary operator.

v2 Link: https://lore.kernel.org/linux-arm-msm/20250124105309.295769-6-quic_vdadhani@quicinc.com/

v1 -> v2:

- Remove the fixed firmware path and add logic to read the path from the device tree.
- Remove code related to the 'qcom,load-firmware' property.
- Resolve kernel test robot warnings.
- Update the commit message.
- Update Copyright year.

v1 Link: https://lore.kernel.org/linux-kernel/20241204150326.1470749-5-quic_vdadhani@quicinc.com/
---
---
 drivers/soc/qcom/qcom-geni-se.c      | 407 +++++++++++++++++++++++++--
 include/linux/soc/qcom/geni-se.h     |  32 ++-
 include/linux/soc/qcom/qup-fw-load.h |  93 ++++++
 3 files changed, 510 insertions(+), 22 deletions(-)
 create mode 100644 include/linux/soc/qcom/qup-fw-load.h

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 4cb959106efa..ef4997a5fd63 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -1,11 +1,15 @@
 // SPDX-License-Identifier: GPL-2.0
-// Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+/*
+ *  Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ *  Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
 
 /* Disable MMIO tracing to prevent excessive logging of unwanted MMIO traces */
 #define __DISABLE_TRACE_MMIO__
 
 #include <linux/acpi.h>
 #include <linux/clk.h>
+#include <linux/firmware.h>
 #include <linux/slab.h>
 #include <linux/dma-mapping.h>
 #include <linux/io.h>
@@ -15,6 +19,7 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/soc/qcom/geni-se.h>
+#include <linux/soc/qcom/qup-fw-load.h>
 
 /**
  * DOC: Overview
@@ -80,9 +85,9 @@
  * common to all the serial engines and are independent of serial interfaces.
  */
 
-#define MAX_CLK_PERF_LEVEL 32
-#define MAX_CLKS 2
-
+#define MAX_CLK_PERF_LEVEL	32
+#define MAX_CLKS		2
+#define MAX_PROTOCOL		6
 /**
  * struct geni_wrapper - Data structure to represent the QUP Wrapper Core
  * @dev:		Device pointer of the QUP wrapper core
@@ -110,28 +115,23 @@ struct geni_se_desc {
 static const char * const icc_path_names[] = {"qup-core", "qup-config",
 						"qup-memory"};
 
+static const char * const protocol_name[MAX_PROTOCOL] = { "None", "SPI", "UART",
+							  "I2C", "I3C", "SPI SLAVE"};
+
 #define QUP_HW_VER_REG			0x4
 
 /* Common SE registers */
-#define GENI_INIT_CFG_REVISION		0x0
-#define GENI_S_INIT_CFG_REVISION	0x4
-#define GENI_OUTPUT_CTRL		0x24
-#define GENI_CGC_CTRL			0x28
 #define GENI_CLK_CTRL_RO		0x60
-#define GENI_FW_S_REVISION_RO		0x6c
 #define SE_GENI_BYTE_GRAN		0x254
 #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_GP_LENGTH		0x910
-#define SE_GENI_S_GP_LENGTH		0x914
 #define SE_DMA_TX_PTR_L			0xc30
 #define SE_DMA_TX_PTR_H			0xc34
 #define SE_DMA_TX_ATTR			0xc38
 #define SE_DMA_TX_LEN			0xc3c
 #define SE_DMA_TX_IRQ_EN		0xc48
-#define SE_DMA_TX_IRQ_EN_SET		0xc4c
 #define SE_DMA_TX_IRQ_EN_CLR		0xc50
 #define SE_DMA_TX_LEN_IN		0xc54
 #define SE_DMA_TX_MAX_BURST		0xc5c
@@ -140,9 +140,7 @@ static const char * const icc_path_names[] = {"qup-core", "qup-config",
 #define SE_DMA_RX_ATTR			0xd38
 #define SE_DMA_RX_LEN			0xd3c
 #define SE_DMA_RX_IRQ_EN		0xd48
-#define SE_DMA_RX_IRQ_EN_SET		0xd4c
 #define SE_DMA_RX_IRQ_EN_CLR		0xd50
-#define SE_DMA_RX_LEN_IN		0xd54
 #define SE_DMA_RX_MAX_BURST		0xd5c
 #define SE_DMA_RX_FLUSH			0xd60
 #define SE_GSI_EVENT_EN			0xe18
@@ -179,7 +177,7 @@ static const char * const icc_path_names[] = {"qup-core", "qup-config",
 /* SE_DMA_GENERAL_CFG */
 #define DMA_RX_CLK_CGC_ON		BIT(0)
 #define DMA_TX_CLK_CGC_ON		BIT(1)
-#define DMA_AHB_SLV_CFG_ON		BIT(2)
+#define DMA_AHB_SLV_CLK_CGC_ON		BIT(2)
 #define AHB_SEC_SLV_CLK_CGC_ON		BIT(3)
 #define DUMMY_RX_NON_BUFFERABLE		BIT(4)
 #define RX_DMA_ZERO_PADDING_EN		BIT(5)
@@ -220,12 +218,12 @@ static void geni_se_io_init(void __iomem *base)
 {
 	u32 val;
 
-	val = readl_relaxed(base + GENI_CGC_CTRL);
+	val = readl_relaxed(base + SE_GENI_CGC_CTRL);
 	val |= DEFAULT_CGC_EN;
-	writel_relaxed(val, base + GENI_CGC_CTRL);
+	writel_relaxed(val, base + SE_GENI_CGC_CTRL);
 
 	val = readl_relaxed(base + SE_DMA_GENERAL_CFG);
-	val |= AHB_SEC_SLV_CLK_CGC_ON | DMA_AHB_SLV_CFG_ON;
+	val |= AHB_SEC_SLV_CLK_CGC_ON | DMA_AHB_SLV_CLK_CGC_ON;
 	val |= DMA_TX_CLK_CGC_ON | DMA_RX_CLK_CGC_ON;
 	writel_relaxed(val, base + SE_DMA_GENERAL_CFG);
 
@@ -891,6 +889,379 @@ int geni_icc_disable(struct geni_se *se)
 }
 EXPORT_SYMBOL_GPL(geni_icc_disable);
 
+/**
+ * geni_read_elf() - Read an ELF file.
+ * @se: Pointer to the SE resources structure.
+ * @fw: Pointer to the firmware buffer.
+ * @pelfseg: Pointer to the SE-specific ELF header.
+ *
+ * Read the ELF file and output a pointer to the header data, which
+ * contains the firmware data and any other details.
+ *
+ * Return: 0 if successful, otherwise return an error value.
+ */
+static int geni_read_elf(struct geni_se *se, const struct firmware *fw, struct se_fw_hdr **pelfseg)
+{
+	const struct elf32_hdr *ehdr;
+	struct elf32_phdr *phdrs, *phdr;
+	const struct se_fw_hdr *elfseg;
+	const u8 *addr;
+	int i;
+
+	if (!fw || fw->size < sizeof(struct elf32_hdr))
+		return -EINVAL;
+
+	ehdr = (struct elf32_hdr *)fw->data;
+	phdrs = (struct elf32_phdr *)(ehdr + 1);
+
+	if (ehdr->e_phnum < 2)
+		return -EINVAL;
+
+	for (i = 0; i < ehdr->e_phnum; i++) {
+		phdr = &phdrs[i];
+
+		if (fw->size < phdr->p_offset + phdr->p_filesz)
+			return -EINVAL;
+
+		if (phdr->p_type != PT_LOAD || !phdr->p_memsz)
+			continue;
+
+		if (MI_PBT_PAGE_MODE_VALUE(phdr->p_flags) != MI_PBT_NON_PAGED_SEGMENT ||
+		    MI_PBT_SEGMENT_TYPE_VALUE(phdr->p_flags) == MI_PBT_HASH_SEGMENT ||
+		    MI_PBT_ACCESS_TYPE_VALUE(phdr->p_flags) == MI_PBT_NOTUSED_SEGMENT ||
+		    MI_PBT_ACCESS_TYPE_VALUE(phdr->p_flags) == MI_PBT_SHARED_SEGMENT)
+			continue;
+
+		if (phdr->p_filesz < sizeof(struct se_fw_hdr))
+			continue;
+
+		addr = fw->data + phdr->p_offset;
+		elfseg = (const struct se_fw_hdr *)addr;
+
+		if (cpu_to_le32(elfseg->magic) != MAGIC_NUM_SE || elfseg->version != 1)
+			continue;
+
+		if (phdr->p_filesz < elfseg->fw_offset +
+				     elfseg->fw_size_in_items * sizeof(u32) ||
+		    phdr->p_filesz < elfseg->cfg_idx_offset +
+				     elfseg->cfg_size_in_items * sizeof(u8) ||
+		    phdr->p_filesz < elfseg->cfg_val_offset +
+				     elfseg->cfg_size_in_items * sizeof(u32))
+			continue;
+
+		if (elfseg->serial_protocol != se->protocol)
+			continue;
+
+		*pelfseg = (struct se_fw_hdr *)addr;
+		return 0;
+	}
+	return -EINVAL;
+}
+
+/**
+ * geni_configure_xfer_mode() - Set the transfer mode.
+ * @se: Pointer to a structure representing SE-related resources.
+ *
+ * Set the transfer mode to either FIFO or DMA according to the mode specified by the protocol
+ * driver.
+ *
+ * Return: 0 if successful, otherwise return an error value.
+ */
+static int geni_configure_xfer_mode(struct geni_se *se)
+{
+	/* Configure SE FIFO, DMA or GSI mode. */
+	switch (se->mode) {
+	case GENI_GPI_DMA:
+		geni_setbits32(se->base + SE_GENI_DMA_MODE_EN, GENI_DMA_MODE_EN);
+		writel_relaxed(0x0, se->base + SE_IRQ_EN);
+		writel_relaxed(DMA_RX_EVENT_EN | DMA_TX_EVENT_EN |
+			       GENI_M_EVENT_EN | GENI_S_EVENT_EN,
+			       se->base + SE_GSI_EVENT_EN);
+		break;
+
+	case GENI_SE_FIFO:
+		geni_clrbits32(se->base + SE_GENI_DMA_MODE_EN, GENI_DMA_MODE_EN);
+		writel_relaxed(DMA_RX_IRQ_EN | DMA_TX_IRQ_EN | GENI_M_IRQ_EN | GENI_S_IRQ_EN,
+			       se->base + SE_IRQ_EN);
+		writel_relaxed(0x0, se->base + SE_GSI_EVENT_EN);
+		break;
+
+	case GENI_SE_DMA:
+		geni_setbits32(se->base + SE_GENI_DMA_MODE_EN, GENI_DMA_MODE_EN);
+		writel_relaxed(DMA_RX_IRQ_EN | DMA_TX_IRQ_EN | GENI_M_IRQ_EN | GENI_S_IRQ_EN,
+			       se->base + SE_IRQ_EN);
+		writel_relaxed(0x0, se->base + SE_GSI_EVENT_EN);
+		break;
+
+	default:
+		dev_err(se->dev, "Invalid geni-se transfer mode: %d\n", se->mode);
+		return -EINVAL;
+	}
+	return 0;
+}
+
+/**
+ * geni_enable_interrupts() - Enable interrupts.
+ * @se: Pointer to a structure representing SE-related resources.
+ *
+ * Enable the required interrupts during the firmware load process.
+ *
+ */
+static void geni_enable_interrupts(struct geni_se *se)
+{
+	u32 reg_value;
+
+	/* Enable required interrupts. */
+	writel_relaxed(M_COMMON_GENI_M_IRQ_EN, se->base + SE_GENI_M_IRQ_EN);
+
+	reg_value = S_CMD_OVERRUN_EN | S_ILLEGAL_CMD_EN |
+		    S_CMD_CANCEL_EN | S_CMD_ABORT_EN |
+		    S_GP_IRQ_0_EN | S_GP_IRQ_1_EN |
+		    S_GP_IRQ_2_EN | S_GP_IRQ_3_EN |
+		    S_RX_FIFO_WR_ERR_EN | S_RX_FIFO_RD_ERR_EN;
+	writel_relaxed(reg_value, se->base + SE_GENI_S_IRQ_ENABLE);
+
+	/* DMA mode configuration. */
+	reg_value = RESET_DONE_EN | SBE_EN | DMA_DONE_EN;
+	writel_relaxed(reg_value, se->base + SE_DMA_TX_IRQ_EN_SET);
+	reg_value = FLUSH_DONE_EN | RESET_DONE_EN | SBE_EN | DMA_DONE_EN;
+	writel_relaxed(reg_value, se->base + SE_DMA_RX_IRQ_EN_SET);
+}
+
+/**
+ * geni_write_fw_revision() - Write the firmware revision.
+ * @se: Pointer to a structure representing SE-related resources.
+ * @serial_protocol: serial protocol type.
+ * @fw_version: QUP firmware version.
+ *
+ * Write the firmware revision and protocol into the respective register.
+ *
+ * Return: None.
+ */
+static void geni_write_fw_revision(struct geni_se *se, u16 serial_protocol, u16 fw_version)
+{
+	u32 reg_value;
+
+	reg_value = FIELD_PREP(FW_REV_PROTOCOL_MSK, serial_protocol);
+	reg_value |= FIELD_PREP(FW_REV_VERSION_MSK, fw_version);
+
+	writel_relaxed(reg_value, se->base + SE_GENI_FW_REVISION);
+	writel_relaxed(reg_value, se->base + SE_S_FW_REVISION);
+}
+
+/**
+ * geni_load_se_fw() - Load Serial Engine specific firmware.
+ * @se: Pointer to a structure representing SE-related resources.
+ * @fw: Pointer to the firmware structure.
+ *
+ * Load the protocol firmware into the IRAM of the Serial Engine.
+ *
+ * Return: 0 if successful, otherwise return an error value.
+ */
+static int geni_load_se_fw(struct geni_se *se, const struct firmware *fw)
+{
+	const u32 *fw_data, *cfg_val_arr;
+	const u8 *cfg_idx_arr;
+	u32 i, reg_value, ramn_cnt;
+	int ret;
+	struct se_fw_hdr *hdr;
+
+	ret = geni_read_elf(se, fw, &hdr);
+	if (ret) {
+		dev_err(se->dev, "ELF parsing failed ret: %d\n", ret);
+		return ret;
+	}
+
+	ramn_cnt = hdr->fw_size_in_items;
+	if (hdr->fw_size_in_items % 2 != 0)
+		ramn_cnt++;
+
+	if (ramn_cnt >= MAX_GENI_CFG_RAMn_CNT)
+		return -EINVAL;
+
+	ret = geni_icc_set_bw(se);
+	if (ret)
+		return ret;
+
+	ret = geni_icc_enable(se);
+	if (ret)
+		return ret;
+
+	ret = geni_se_resources_on(se);
+	if (ret)
+		goto err;
+
+	ramn_cnt = hdr->fw_size_in_items;
+	if (hdr->fw_size_in_items % 2 != 0)
+		ramn_cnt++;
+
+	if (ramn_cnt >= MAX_GENI_CFG_RAMn_CNT)
+		goto err_resource;
+
+	fw_data = (const u32 *)((u8 *)hdr + hdr->fw_offset);
+	cfg_idx_arr = (const u8 *)hdr + hdr->cfg_idx_offset;
+	cfg_val_arr = (const u32 *)((u8 *)hdr + hdr->cfg_val_offset);
+
+	/* Disable high priority interrupt until current low priority interrupts are handled. */
+	geni_setbits32(se->wrapper->base + QUPV3_COMMON_CFG, FAST_SWITCH_TO_HIGH_DISABLE);
+
+	/* Set AHB_M_CLK_CGC_ON to indicate hardware controls se-wrapper cgc clock. */
+	geni_setbits32(se->wrapper->base + QUPV3_SE_AHB_M_CFG, AHB_M_CLK_CGC_ON);
+
+	/* Let hardware to control common cgc. */
+	geni_setbits32(se->wrapper->base + QUPV3_COMMON_CGC_CTRL, COMMON_CSR_SLV_CLK_CGC_ON);
+
+	/* Allows to drive corresponding data according to hardware value. */
+	writel_relaxed(0x0, se->base + GENI_OUTPUT_CTRL);
+
+	/* Set SCLK and HCLK to program RAM */
+	geni_setbits32(se->base + SE_GENI_CGC_CTRL, PROG_RAM_SCLK_OFF | PROG_RAM_HCLK_OFF);
+	writel_relaxed(0x0, se->base + SE_GENI_CLK_CTRL);
+	geni_clrbits32(se->base + SE_GENI_CGC_CTRL, PROG_RAM_SCLK_OFF | PROG_RAM_HCLK_OFF);
+
+	/* Enable required clocks for DMA CSR, TX and RX. */
+	reg_value = AHB_SEC_SLV_CLK_CGC_ON | DMA_AHB_SLV_CLK_CGC_ON |
+		    DMA_TX_CLK_CGC_ON | DMA_RX_CLK_CGC_ON;
+	geni_setbits32(se->base + SE_DMA_GENERAL_CFG, reg_value);
+
+	/* Let hardware control CGC by default. */
+	writel_relaxed(DEFAULT_CGC_EN, se->base + SE_GENI_CGC_CTRL);
+
+	/* Set version of the configuration register part of firmware. */
+	writel_relaxed(hdr->cfg_version, se->base + SE_GENI_INIT_CFG_REVISION);
+	writel_relaxed(hdr->cfg_version, se->base + SE_GENI_S_INIT_CFG_REVISION);
+
+	/* Configure GENI primitive table. */
+	for (i = 0; i < hdr->cfg_size_in_items; i++)
+		writel_relaxed(cfg_val_arr[i],
+			       se->base + SE_GENI_CFG_REG0 + (cfg_idx_arr[i] * sizeof(u32)));
+
+	/* Configure condition for assertion of RX_RFR_WATERMARK condition. */
+	reg_value = geni_se_get_rx_fifo_depth(se);
+	writel_relaxed(reg_value - 2, se->base + SE_GENI_RX_RFR_WATERMARK_REG);
+
+	/* Let hardware control CGC */
+	geni_setbits32(se->base + GENI_OUTPUT_CTRL, DEFAULT_IO_OUTPUT_CTRL_MSK);
+
+	ret = geni_configure_xfer_mode(se);
+	if (ret)
+		goto err_resource;
+
+	geni_enable_interrupts(se);
+
+	geni_write_fw_revision(se, hdr->serial_protocol, hdr->fw_version);
+
+	ramn_cnt = hdr->fw_size_in_items;
+	if (hdr->fw_size_in_items % 2 != 0)
+		ramn_cnt++;
+
+	if (ramn_cnt >= MAX_GENI_CFG_RAMn_CNT)
+		goto err_resource;
+
+	/* Program RAM address space. */
+	memcpy_toio(se->base + SE_GENI_CFG_RAMN, fw_data, ramn_cnt * sizeof(u32));
+
+	/* Put default values on GENI's output pads. */
+	writel_relaxed(0x1, se->base + GENI_FORCE_DEFAULT_REG);
+
+	/* High to low SCLK and HCLK to finish RAM. */
+	geni_setbits32(se->base + SE_GENI_CGC_CTRL, PROG_RAM_SCLK_OFF | PROG_RAM_HCLK_OFF);
+	geni_setbits32(se->base + SE_GENI_CLK_CTRL, SER_CLK_SEL);
+	geni_clrbits32(se->base + SE_GENI_CGC_CTRL, PROG_RAM_SCLK_OFF | PROG_RAM_HCLK_OFF);
+
+	/* Serial engine DMA interface is enabled. */
+	geni_setbits32(se->base + SE_DMA_IF_EN, DMA_IF_EN);
+
+	/* Enable or disable FIFO interface of the serial engine. */
+	if (se->mode == GENI_SE_FIFO)
+		geni_clrbits32(se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
+	else
+		geni_setbits32(se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
+
+err_resource:
+	geni_se_resources_off(se);
+err:
+	geni_icc_disable(se);
+	return ret;
+}
+
+/**
+ * qup_fw_load() - Initiate firmware load.
+ * @se: Pointer to a structure representing SE-related resources.
+ * @fw_name: Name of the firmware.
+ *
+ * Load the firmware into a specific SE. Read the associated ELF file,
+ * copy the data into a buffer in kernel space using the request_firmware API, write the
+ * data into the SE's IRAM register, and then free the buffers. Handle firmware loading
+ * and parsing for a specific protocol.
+ *
+ * Return: 0 if successful, otherwise return an error value.
+ */
+static int qup_fw_load(struct geni_se *se, const char *fw_name)
+{
+	int ret;
+	const struct firmware *fw;
+	struct device *dev = se->dev;
+
+	ret = request_firmware(&fw, fw_name, dev);
+	if (ret) {
+		dev_err(dev, "request_firmware failed for %d: %d\n", se->protocol, ret);
+		return ret;
+	}
+
+	ret = geni_load_se_fw(se, fw);
+
+	release_firmware(fw);
+
+	return ret;
+}
+
+/**
+ * geni_load_se_firmware() - Initiate firmware loading.
+ * @se: Serial engine details.
+ * @protocol: Protocol (SPI, I2C, or UART) for which the firmware is to be loaded.
+ *
+ * If the device tree properties are configured to load QUP firmware and the firmware
+ * is not already loaded, start the firmware loading process. If the device tree properties
+ * are not defined, skip loading the firmware, assuming it is already loaded by TZ.
+ *
+ * Return: 0 if successful, otherwise return an error value.
+ */
+int geni_load_se_firmware(struct geni_se *se, enum geni_se_protocol_type protocol)
+{
+	const char *fw_name;
+	int ret;
+
+	if (protocol >= MAX_PROTOCOL) {
+		dev_err(se->dev, "Invalid geni-se protocol: %d", protocol);
+		return  -EINVAL;
+	}
+
+	ret = device_property_read_string(se->wrapper->dev, "firmware-name", &fw_name);
+	if (ret)
+		return  -EINVAL;
+
+	se->protocol = protocol;
+
+	if (of_property_read_bool(se->dev->of_node, "qcom,enable-gsi-dma"))
+		se->mode = GENI_GPI_DMA;
+	else
+		se->mode = GENI_SE_FIFO;
+
+	/* GSI mode is not supported by the UART driver; therefore, setting FIFO mode */
+	if (protocol == GENI_SE_UART)
+		se->mode = GENI_SE_FIFO;
+
+	ret = qup_fw_load(se, fw_name);
+	if (ret)
+		return ret;
+
+	dev_dbg(se->dev, "Firmware load for %s protocol is successful for xfer mode %d\n",
+		protocol_name[se->protocol], se->mode);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(geni_load_se_firmware);
+
 static int geni_se_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
index 2996a3c28ef3..6b75171d65ec 100644
--- a/include/linux/soc/qcom/geni-se.h
+++ b/include/linux/soc/qcom/geni-se.h
@@ -1,11 +1,13 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /*
  * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #ifndef _LINUX_QCOM_GENI_SE
 #define _LINUX_QCOM_GENI_SE
 
+#include <linux/bitfield.h>
 #include <linux/interconnect.h>
 
 /**
@@ -36,6 +38,7 @@ enum geni_se_protocol_type {
 	GENI_SE_I2C,
 	GENI_SE_I3C,
 	GENI_SE_SPI_SLAVE,
+	GENI_SE_INVALID_PROTO = 255,
 };
 
 struct geni_wrapper;
@@ -61,6 +64,8 @@ struct geni_icc_path {
  * @num_clk_levels:	Number of valid clock levels in clk_perf_tbl
  * @clk_perf_tbl:	Table of clock frequency input to serial engine clock
  * @icc_paths:		Array of ICC paths for SE
+ * @mode:		Transfer mode se fifo, dma or gsi.
+ * @protocol:		Protocol spi or i2c or serial.
  */
 struct geni_se {
 	void __iomem *base;
@@ -70,24 +75,32 @@ struct geni_se {
 	unsigned int num_clk_levels;
 	unsigned long *clk_perf_tbl;
 	struct geni_icc_path icc_paths[3];
+	enum geni_se_xfer_mode mode;
+	enum geni_se_protocol_type protocol;
 };
 
 /* Common SE registers */
+#define SE_GENI_INIT_CFG_REVISION	0x0
+#define SE_GENI_S_INIT_CFG_REVISION	0x4
 #define GENI_FORCE_DEFAULT_REG		0x20
 #define GENI_OUTPUT_CTRL		0x24
+#define SE_GENI_CGC_CTRL		0x28
 #define SE_GENI_STATUS			0x40
 #define GENI_SER_M_CLK_CFG		0x48
 #define GENI_SER_S_CLK_CFG		0x4c
 #define GENI_IF_DISABLE_RO		0x64
-#define GENI_FW_REVISION_RO		0x68
+#define SE_GENI_FW_REVISION_RO		0x68
+#define SE_GENI_S_FW_REVISION_RO	0x6c
 #define SE_GENI_CLK_SEL			0x7c
 #define SE_GENI_CFG_SEQ_START		0x84
+#define SE_GENI_CFG_REG0		0x100
 #define SE_GENI_DMA_MODE_EN		0x258
 #define SE_GENI_M_CMD0			0x600
 #define SE_GENI_M_CMD_CTRL_REG		0x604
 #define SE_GENI_M_IRQ_STATUS		0x610
 #define SE_GENI_M_IRQ_EN		0x614
 #define SE_GENI_M_IRQ_CLEAR		0x618
+#define SE_GENI_S_IRQ_ENABLE		0x644
 #define SE_GENI_M_IRQ_EN_SET		0x61c
 #define SE_GENI_M_IRQ_EN_CLEAR		0x620
 #define SE_GENI_S_CMD0			0x630
@@ -109,13 +122,22 @@ struct geni_se {
 #define SE_GENI_S_GP_LENGTH		0x914
 #define SE_DMA_TX_IRQ_STAT		0xc40
 #define SE_DMA_TX_IRQ_CLR		0xc44
+#define SE_DMA_TX_IRQ_EN_SET		0xc4c
 #define SE_DMA_TX_FSM_RST		0xc58
 #define SE_DMA_RX_IRQ_STAT		0xd40
 #define SE_DMA_RX_IRQ_CLR		0xd44
+#define SE_DMA_RX_IRQ_EN_SET		0xd4c
 #define SE_DMA_RX_LEN_IN		0xd54
 #define SE_DMA_RX_FSM_RST		0xd58
 #define SE_HW_PARAM_0			0xe24
 #define SE_HW_PARAM_1			0xe28
+#define SE_DMA_GENERAL_CFG		0xe30
+#define SE_GENI_FW_REVISION		0x1000
+#define SE_S_FW_REVISION		0x1004
+#define SE_GENI_CFG_RAMN		0x1010
+#define SE_GENI_CLK_CTRL		0x2000
+#define SE_DMA_IF_EN			0x2004
+#define SE_FIFO_IF_DISABLE		0x2008
 
 /* GENI_FORCE_DEFAULT_REG fields */
 #define FORCE_DEFAULT	BIT(0)
@@ -137,7 +159,7 @@ struct geni_se {
 
 /* GENI_FW_REVISION_RO fields */
 #define FW_REV_PROTOCOL_MSK		GENMASK(15, 8)
-#define FW_REV_PROTOCOL_SHFT		8
+#define FW_REV_VERSION_MSK		GENMASK(7, 0)
 
 /* GENI_CLK_SEL fields */
 #define CLK_SEL_MSK			GENMASK(2, 0)
@@ -325,9 +347,9 @@ static inline u32 geni_se_read_proto(struct geni_se *se)
 {
 	u32 val;
 
-	val = readl_relaxed(se->base + GENI_FW_REVISION_RO);
+	val = readl_relaxed(se->base + SE_GENI_FW_REVISION_RO);
 
-	return (val & FW_REV_PROTOCOL_MSK) >> FW_REV_PROTOCOL_SHFT;
+	return FIELD_GET(FW_REV_PROTOCOL_MSK, val);
 }
 
 /**
@@ -531,5 +553,7 @@ void geni_icc_set_tag(struct geni_se *se, u32 tag);
 int geni_icc_enable(struct geni_se *se);
 
 int geni_icc_disable(struct geni_se *se);
+
+int geni_load_se_firmware(struct geni_se *se, enum geni_se_protocol_type protocol);
 #endif
 #endif
diff --git a/include/linux/soc/qcom/qup-fw-load.h b/include/linux/soc/qcom/qup-fw-load.h
new file mode 100644
index 000000000000..94a6c4c10bcf
--- /dev/null
+++ b/include/linux/soc/qcom/qup-fw-load.h
@@ -0,0 +1,93 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+#ifndef _LINUX_QCOM_QUP_FW_LOAD
+#define _LINUX_QCOM_QUP_FW_LOAD
+
+#include <linux/kernel.h>
+
+/*Magic numbers*/
+#define MAGIC_NUM_SE			0x57464553
+
+#define MAX_GENI_CFG_RAMn_CNT		455
+
+#define MI_PBT_NON_PAGED_SEGMENT	0x0
+#define MI_PBT_HASH_SEGMENT		0x2
+#define MI_PBT_NOTUSED_SEGMENT		0x3
+#define MI_PBT_SHARED_SEGMENT		0x4
+
+#define MI_PBT_FLAG_PAGE_MODE		BIT(20)
+#define MI_PBT_FLAG_SEGMENT_TYPE	GENMASK(26, 24)
+#define MI_PBT_FLAG_ACCESS_TYPE		GENMASK(23, 21)
+
+#define MI_PBT_PAGE_MODE_VALUE(x) FIELD_GET(MI_PBT_FLAG_PAGE_MODE, x)
+
+#define MI_PBT_SEGMENT_TYPE_VALUE(x) FIELD_GET(MI_PBT_FLAG_SEGMENT_TYPE, x)
+
+#define MI_PBT_ACCESS_TYPE_VALUE(x) FIELD_GET(MI_PBT_FLAG_ACCESS_TYPE, x)
+
+#define M_COMMON_GENI_M_IRQ_EN	(GENMASK(6, 1) | \
+				M_IO_DATA_DEASSERT_EN | \
+				M_IO_DATA_ASSERT_EN | M_RX_FIFO_RD_ERR_EN | \
+				M_RX_FIFO_WR_ERR_EN | M_TX_FIFO_RD_ERR_EN | \
+				M_TX_FIFO_WR_ERR_EN)
+
+/* DMA_TX/RX_IRQ_EN fields */
+#define DMA_DONE_EN		BIT(0)
+#define SBE_EN			BIT(2)
+#define RESET_DONE_EN		BIT(3)
+#define FLUSH_DONE_EN		BIT(4)
+
+/* GENI_CLK_CTRL fields */
+#define SER_CLK_SEL		BIT(0)
+
+/* GENI_DMA_IF_EN fields */
+#define DMA_IF_EN		BIT(0)
+
+#define QUPV3_COMMON_CFG		0x120
+#define FAST_SWITCH_TO_HIGH_DISABLE	BIT(0)
+
+#define QUPV3_SE_AHB_M_CFG		0x118
+#define AHB_M_CLK_CGC_ON		BIT(0)
+
+#define QUPV3_COMMON_CGC_CTRL		0x21C
+#define COMMON_CSR_SLV_CLK_CGC_ON	BIT(0)
+
+/* access ports */
+#define geni_setbits32(_addr, _v) writel_relaxed(readl_relaxed(_addr) |  (_v), _addr)
+#define geni_clrbits32(_addr, _v) writel_relaxed(readl_relaxed(_addr) & ~(_v), _addr)
+
+/**
+ * struct se_fw_hdr - Serial Engine firmware configuration header
+ *
+ * This structure defines metadata for the Serial Engine (SE) firmware
+ * configuration. Although it is embedded within an ELF segment, it is
+ * not part of the ELF format itself.
+ *
+ * @magic: Set to 'SEFW'
+ * @version: Structure version number
+ * @core_version: QUPV3 hardware version
+ * @serial_protocol: Encoded in GENI_FW_REVISION
+ * @fw_version: Firmware version, from GENI_FW_REVISION
+ * @cfg_version: Configuration version, from GENI_INIT_CFG_REVISION
+ * @fw_size_in_items: Number of 32-bit words in GENI_FW_RAM
+ * @fw_offset: Byte offset to GENI_FW_RAM array
+ * @cfg_size_in_items: Number of GENI_FW_CFG index/value pairs
+ * @cfg_idx_offset: Byte offset to GENI_FW_CFG index array
+ * @cfg_val_offset: Byte offset to GENI_FW_CFG values array
+ */
+struct se_fw_hdr {
+	__le32 magic;
+	u32 version;
+	u32 core_version;
+	u16 serial_protocol;
+	u16 fw_version;
+	u16 cfg_version;
+	u16 fw_size_in_items;
+	u16 fw_offset;
+	u16 cfg_size_in_items;
+	u16 cfg_idx_offset;
+	u16 cfg_val_offset;
+};
+#endif /* _LINUX_QCOM_QUP_FW_LOAD */
-- 
2.34.1

[PATCH v5 3/5] i2c: qcom-geni: Load i2c qup Firmware from linux side

[Viken Dadhaniya]

Add provision to load firmware of Serial engine for I2C protocol from
Linux Execution Environment on running on APPS processor.

Co-developed-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Signed-off-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Signed-off-by: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
---
Dependencies:

This patch depends on patch 2 of this series.

v4 -> v5:

- Updated the email domain from 'quic' to 'oss'.

v4 Link: https://lore.kernel.org/all/20250503111029.3583807-4-quic_vdadhani@quicinc.com/

v3 - >v4:

- Add a patch dependency note.

v3 Link: https://lore.kernel.org/linux-arm-msm/20250303124349.3474185-8-quic_vdadhani@quicinc.com/

v2 -> v3:

- Load firmware only if the protocol is invalid.

v2 Link: https://lore.kernel.org/linux-arm-msm/20250124105309.295769-7-quic_vdadhani@quicinc.com/

v1 -> v2:

- No change.

v1 Link: https://lore.kernel.org/linux-arm-msm/20241204150326.1470749-6-quic_vdadhani@quicinc.com/
---
 drivers/i2c/busses/i2c-qcom-geni.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 13889f52b6f7..11f1089edf37 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -870,7 +870,13 @@ static int geni_i2c_probe(struct platform_device *pdev)
 		goto err_clk;
 	}
 	proto = geni_se_read_proto(&gi2c->se);
-	if (proto != GENI_SE_I2C) {
+	if (proto == GENI_SE_INVALID_PROTO) {
+		ret = geni_load_se_firmware(&gi2c->se, GENI_SE_I2C);
+		if (ret) {
+			dev_err_probe(dev, ret, "i2c firmware load failed ret: %d\n", ret);
+			goto err_resources;
+		}
+	} else if (proto != GENI_SE_I2C) {
 		ret = dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto);
 		goto err_resources;
 	}
-- 
2.34.1

[PATCH v5 4/5] spi: geni-qcom: Load spi qup Firmware from linux side

[Viken Dadhaniya]

Add provision to load firmware of Serial engine for SPI protocol from
Linux Execution Environment on running on APPS processor.

Co-developed-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Signed-off-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Signed-off-by: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>

---
Dependencies:

This patch depends on patch 2 of this series.

v4 -> v5:

- Updated the email domain from 'quic' to 'oss'.

v4 Link: https://lore.kernel.org/all/20250503111029.3583807-5-quic_vdadhani@quicinc.com/

v3 -> v4:

- Add a patch dependency note.

v3 Link: https://lore.kernel.org/linux-arm-msm/20250303124349.3474185-9-quic_vdadhani@quicinc.com/

v2 -> v3:

- Load firmware only if the protocol is invalid.

v2 Link: https://lore.kernel.org/linux-arm-msm/20250124105309.295769-8-quic_vdadhani@quicinc.com/

v1 -> v2:

- No change.

v1 Link: https://lore.kernel.org/linux-arm-msm/20241204150326.1470749-7-quic_vdadhani@quicinc.com/
---
---
 drivers/spi/spi-geni-qcom.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 768d7482102a..a0d8d3425c6c 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -671,6 +671,12 @@ static int spi_geni_init(struct spi_geni_master *mas)
 			goto out_pm;
 		}
 		spi_slv_setup(mas);
+	} else if (proto == GENI_SE_INVALID_PROTO) {
+		ret = geni_load_se_firmware(se, GENI_SE_SPI);
+		if (ret) {
+			dev_err(mas->dev, "spi master firmware load failed ret: %d\n", ret);
+			goto out_pm;
+		}
 	} else if (proto != GENI_SE_SPI) {
 		dev_err(mas->dev, "Invalid proto %d\n", proto);
 		goto out_pm;
-- 
2.34.1

[PATCH v5 5/5] serial: qcom-geni: Load UART qup Firmware from linux side

[Viken Dadhaniya]

Add provision to load firmware of Serial engine for UART protocol from
Linux Execution Environment on running on APPS processor.

Co-developed-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Signed-off-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Signed-off-by: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
---
Dependencies:

This patch depends on patch 2 of this series.

v4 -> v5:

- Updated the email domain from 'quic' to 'oss'.

v4 Link: https://lore.kernel.org/all/20250503111029.3583807-6-quic_vdadhani@quicinc.com/

v3 -> v4:

- Add a patch dependency note.

v3 Link: https://lore.kernel.org/linux-arm-msm/20250303124349.3474185-10-quic_vdadhani@quicinc.com/

v2 -> v3:

- Load firmware only if the protocol is invalid.

v2 Link: https://lore.kernel.org/linux-arm-msm/20250124105309.295769-9-quic_vdadhani@quicinc.com/

v1 -> v2:

- No change.

v1 Link: https://lore.kernel.org/linux-arm-msm/20241204150326.1470749-8-quic_vdadhani@quicinc.com/
---
---
 drivers/tty/serial/qcom_geni_serial.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 0293b6210aa6..9f905ae3c127 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1161,7 +1161,13 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
 	int ret;
 
 	proto = geni_se_read_proto(&port->se);
-	if (proto != GENI_SE_UART) {
+	if (proto == GENI_SE_INVALID_PROTO) {
+		ret = geni_load_se_firmware(&port->se, GENI_SE_UART);
+		if (ret) {
+			dev_err(uport->dev, "UART firmware load failed ret: %d\n", ret);
+			return ret;
+		}
+	} else if (proto != GENI_SE_UART) {
 		dev_err(uport->dev, "Invalid FW loaded, proto: %d\n", proto);
 		return -ENXIO;
 	}
-- 
2.34.1

Re: [PATCH v5 2/5] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem

[kernel test robot]

Hi Viken,

kernel test robot noticed the following build warnings:

[auto build test WARNING on andi-shyti/i2c/i2c-host]
[also build test WARNING on tty/tty-testing tty/tty-next tty/tty-linus broonie-spi/for-next linus/master v6.16-rc3 next-20250627]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Viken-Dadhaniya/dt-bindings-qcom-se-common-Add-QUP-Peripheral-specific-properties-for-I2C-SPI-and-SERIAL-bus/20250624-175308
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20250624095102.1587580-3-viken.dadhaniya%40oss.qualcomm.com
patch subject: [PATCH v5 2/5] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem
config: alpha-randconfig-r122-20250628 (https://download.01.org/0day-ci/archive/20250628/202506281152.eY0YQpxs-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 14.3.0
reproduce: (https://download.01.org/0day-ci/archive/20250628/202506281152.eY0YQpxs-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506281152.eY0YQpxs-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/soc/qcom/qcom-geni-se.c:941:21: sparse: sparse: cast from restricted __le32
>> drivers/soc/qcom/qcom-geni-se.c:941:21: sparse: sparse: restricted __le32 degrades to integer

vim +941 drivers/soc/qcom/qcom-geni-se.c

   891	
   892	/**
   893	 * geni_read_elf() - Read an ELF file.
   894	 * @se: Pointer to the SE resources structure.
   895	 * @fw: Pointer to the firmware buffer.
   896	 * @pelfseg: Pointer to the SE-specific ELF header.
   897	 *
   898	 * Read the ELF file and output a pointer to the header data, which
   899	 * contains the firmware data and any other details.
   900	 *
   901	 * Return: 0 if successful, otherwise return an error value.
   902	 */
   903	static int geni_read_elf(struct geni_se *se, const struct firmware *fw, struct se_fw_hdr **pelfseg)
   904	{
   905		const struct elf32_hdr *ehdr;
   906		struct elf32_phdr *phdrs, *phdr;
   907		const struct se_fw_hdr *elfseg;
   908		const u8 *addr;
   909		int i;
   910	
   911		if (!fw || fw->size < sizeof(struct elf32_hdr))
   912			return -EINVAL;
   913	
   914		ehdr = (struct elf32_hdr *)fw->data;
   915		phdrs = (struct elf32_phdr *)(ehdr + 1);
   916	
   917		if (ehdr->e_phnum < 2)
   918			return -EINVAL;
   919	
   920		for (i = 0; i < ehdr->e_phnum; i++) {
   921			phdr = &phdrs[i];
   922	
   923			if (fw->size < phdr->p_offset + phdr->p_filesz)
   924				return -EINVAL;
   925	
   926			if (phdr->p_type != PT_LOAD || !phdr->p_memsz)
   927				continue;
   928	
   929			if (MI_PBT_PAGE_MODE_VALUE(phdr->p_flags) != MI_PBT_NON_PAGED_SEGMENT ||
   930			    MI_PBT_SEGMENT_TYPE_VALUE(phdr->p_flags) == MI_PBT_HASH_SEGMENT ||
   931			    MI_PBT_ACCESS_TYPE_VALUE(phdr->p_flags) == MI_PBT_NOTUSED_SEGMENT ||
   932			    MI_PBT_ACCESS_TYPE_VALUE(phdr->p_flags) == MI_PBT_SHARED_SEGMENT)
   933				continue;
   934	
   935			if (phdr->p_filesz < sizeof(struct se_fw_hdr))
   936				continue;
   937	
   938			addr = fw->data + phdr->p_offset;
   939			elfseg = (const struct se_fw_hdr *)addr;
   940	
 > 941			if (cpu_to_le32(elfseg->magic) != MAGIC_NUM_SE || elfseg->version != 1)
   942				continue;
   943	
   944			if (phdr->p_filesz < elfseg->fw_offset +
   945					     elfseg->fw_size_in_items * sizeof(u32) ||
   946			    phdr->p_filesz < elfseg->cfg_idx_offset +
   947					     elfseg->cfg_size_in_items * sizeof(u8) ||
   948			    phdr->p_filesz < elfseg->cfg_val_offset +
   949					     elfseg->cfg_size_in_items * sizeof(u32))
   950				continue;
   951	
   952			if (elfseg->serial_protocol != se->protocol)
   953				continue;
   954	
   955			*pelfseg = (struct se_fw_hdr *)addr;
   956			return 0;
   957		}
   958		return -EINVAL;
   959	}
   960	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v5 3/5] i2c: qcom-geni: Load i2c qup Firmware from linux side

[Andi Shyti]

Hi Viken,

> Co-developed-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
> Signed-off-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
> Signed-off-by: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>

Acked-by: Andi Shyti <andi.shyti@kernel.org>

Thanks,
Andi

Re: [PATCH v5 2/5] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem

[Bjorn Andersson]

On Tue, Jun 24, 2025 at 03:20:59PM +0530, Viken Dadhaniya wrote:
> In Qualcomm SoCs, firmware loading for Serial Engines (SE) within the QUP
> hardware has traditionally been managed by TrustZone (TZ). This restriction
> poses a significant challenge for developers, as it limits their ability to
> enable various protocols on any of the SEs from the Linux side, reducing
> flexibility.
> 
> Load the firmware to QUP SE based on the 'firmware-name' property specified
> in devicetree at bootup time.
> 
> Co-developed-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
> Signed-off-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
> Signed-off-by: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
> ---
> v4 -> v5:
> 
> - Resolved kernel test robot error by including the missing bitfield header file.
> - Updated the SE firmware ELF structure name for consistency.
> - Specified _leb4 format for the magic number definition.
> - Updated the email domain from 'quic' to 'oss'.
> 
> v4 Link: https://lore.kernel.org/all/20250503111029.3583807-3-quic_vdadhani@quicinc.com/
> 
> v3 -> v4:
> 
> - Update the commit message.
> - Resolve kernel test robot warnings.
> - Add a multiline comment in the Copyright section.
> - Remove valid_seg_size and geni_config_common_control functions, and add the code inline.
> - Rename read_elf function to geni_read_elf.
> - Add a firmware size check.
> - Assign *pelfseg after finding a match.
> - Break one large condition check into multiple checks to improve code readability.
> - Remove return type documentation for void functions.
> - Update error messages to be more descriptive.
> - Correct indentation.
> - Rename geni_flash_fw_revision function to geni_write_fw_revision.
> - Remove __func__ from all print statements.
> - Move resource_on to the appropriate section after parsing the firmware file.
> - Update variable names and function arguments as suggested.
> - Use FIELD_GET, FIELD_PREP, and GENMASK.
> - Use memcpy_toio() instead of memcpy.
> - Remove duplicate registers and bitmask macros.
> - Remove rsc struct and add required variables in geni_se struct.
> 
> v3 Link: https://lore.kernel.org/linux-arm-msm/20250303124349.3474185-7-quic_vdadhani@quicinc.com/
> 
> v2 -> v3:
> 
> - Remove code related to the 'qcom,xfer-mode' property.
> - Add logic to read the boolean property 'qcom,gsi-dma-allowed' and select the transfer mode.
> - Hardcode FIFO mode for the serial driver as GSI mode is currently not supported.
> - Update function descriptions as suggested.
> - Enhance error handling and remove redundant if conditions.
> - Drop the ternary operator.
> 
> v2 Link: https://lore.kernel.org/linux-arm-msm/20250124105309.295769-6-quic_vdadhani@quicinc.com/
> 
> v1 -> v2:
> 
> - Remove the fixed firmware path and add logic to read the path from the device tree.
> - Remove code related to the 'qcom,load-firmware' property.
> - Resolve kernel test robot warnings.
> - Update the commit message.
> - Update Copyright year.
> 
> v1 Link: https://lore.kernel.org/linux-kernel/20241204150326.1470749-5-quic_vdadhani@quicinc.com/
> ---
> ---
>  drivers/soc/qcom/qcom-geni-se.c      | 407 +++++++++++++++++++++++++--
>  include/linux/soc/qcom/geni-se.h     |  32 ++-
>  include/linux/soc/qcom/qup-fw-load.h |  93 ++++++
>  3 files changed, 510 insertions(+), 22 deletions(-)
>  create mode 100644 include/linux/soc/qcom/qup-fw-load.h
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 4cb959106efa..ef4997a5fd63 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -1,11 +1,15 @@
>  // SPDX-License-Identifier: GPL-2.0
> -// Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> +/*
> + *  Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + *  Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.

Please use our updated Copyright header.

> + */
>  
>  /* Disable MMIO tracing to prevent excessive logging of unwanted MMIO traces */
>  #define __DISABLE_TRACE_MMIO__
>  
>  #include <linux/acpi.h>
>  #include <linux/clk.h>
> +#include <linux/firmware.h>
>  #include <linux/slab.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/io.h>
> @@ -15,6 +19,7 @@
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/soc/qcom/geni-se.h>
> +#include <linux/soc/qcom/qup-fw-load.h>
>  
>  /**
>   * DOC: Overview
> @@ -80,9 +85,9 @@
>   * common to all the serial engines and are independent of serial interfaces.
>   */
>  
> -#define MAX_CLK_PERF_LEVEL 32
> -#define MAX_CLKS 2
> -
> +#define MAX_CLK_PERF_LEVEL	32
> +#define MAX_CLKS		2
> +#define MAX_PROTOCOL		6
>  /**
>   * struct geni_wrapper - Data structure to represent the QUP Wrapper Core
>   * @dev:		Device pointer of the QUP wrapper core
> @@ -110,28 +115,23 @@ struct geni_se_desc {
>  static const char * const icc_path_names[] = {"qup-core", "qup-config",
>  						"qup-memory"};
>  
> +static const char * const protocol_name[MAX_PROTOCOL] = { "None", "SPI", "UART",
> +							  "I2C", "I3C", "SPI SLAVE"};
> +
>  #define QUP_HW_VER_REG			0x4
>  
>  /* Common SE registers */
> -#define GENI_INIT_CFG_REVISION		0x0
> -#define GENI_S_INIT_CFG_REVISION	0x4
> -#define GENI_OUTPUT_CTRL		0x24

It would be nice if you did the rename and register cleanup in one
"dumb" patch and then introduced the new functionality in a second
patch.

> -#define GENI_CGC_CTRL			0x28
>  #define GENI_CLK_CTRL_RO		0x60
> -#define GENI_FW_S_REVISION_RO		0x6c
>  #define SE_GENI_BYTE_GRAN		0x254
>  #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_GP_LENGTH		0x910
> -#define SE_GENI_S_GP_LENGTH		0x914
>  #define SE_DMA_TX_PTR_L			0xc30
>  #define SE_DMA_TX_PTR_H			0xc34
>  #define SE_DMA_TX_ATTR			0xc38
>  #define SE_DMA_TX_LEN			0xc3c
>  #define SE_DMA_TX_IRQ_EN		0xc48
> -#define SE_DMA_TX_IRQ_EN_SET		0xc4c
>  #define SE_DMA_TX_IRQ_EN_CLR		0xc50
>  #define SE_DMA_TX_LEN_IN		0xc54
>  #define SE_DMA_TX_MAX_BURST		0xc5c
> @@ -140,9 +140,7 @@ static const char * const icc_path_names[] = {"qup-core", "qup-config",
>  #define SE_DMA_RX_ATTR			0xd38
>  #define SE_DMA_RX_LEN			0xd3c
>  #define SE_DMA_RX_IRQ_EN		0xd48
> -#define SE_DMA_RX_IRQ_EN_SET		0xd4c
>  #define SE_DMA_RX_IRQ_EN_CLR		0xd50
> -#define SE_DMA_RX_LEN_IN		0xd54
>  #define SE_DMA_RX_MAX_BURST		0xd5c
>  #define SE_DMA_RX_FLUSH			0xd60
>  #define SE_GSI_EVENT_EN			0xe18
> @@ -179,7 +177,7 @@ static const char * const icc_path_names[] = {"qup-core", "qup-config",
>  /* SE_DMA_GENERAL_CFG */
>  #define DMA_RX_CLK_CGC_ON		BIT(0)
>  #define DMA_TX_CLK_CGC_ON		BIT(1)
> -#define DMA_AHB_SLV_CFG_ON		BIT(2)
> +#define DMA_AHB_SLV_CLK_CGC_ON		BIT(2)
>  #define AHB_SEC_SLV_CLK_CGC_ON		BIT(3)
>  #define DUMMY_RX_NON_BUFFERABLE		BIT(4)
>  #define RX_DMA_ZERO_PADDING_EN		BIT(5)
> @@ -220,12 +218,12 @@ static void geni_se_io_init(void __iomem *base)
>  {
>  	u32 val;
>  
> -	val = readl_relaxed(base + GENI_CGC_CTRL);
> +	val = readl_relaxed(base + SE_GENI_CGC_CTRL);

Per above comment, this change has nothing to do with firmware loading.

>  	val |= DEFAULT_CGC_EN;
> -	writel_relaxed(val, base + GENI_CGC_CTRL);
> +	writel_relaxed(val, base + SE_GENI_CGC_CTRL);
>  
>  	val = readl_relaxed(base + SE_DMA_GENERAL_CFG);
> -	val |= AHB_SEC_SLV_CLK_CGC_ON | DMA_AHB_SLV_CFG_ON;
> +	val |= AHB_SEC_SLV_CLK_CGC_ON | DMA_AHB_SLV_CLK_CGC_ON;
>  	val |= DMA_TX_CLK_CGC_ON | DMA_RX_CLK_CGC_ON;
>  	writel_relaxed(val, base + SE_DMA_GENERAL_CFG);
>  
> @@ -891,6 +889,379 @@ int geni_icc_disable(struct geni_se *se)
>  }
>  EXPORT_SYMBOL_GPL(geni_icc_disable);
>  
> +/**
> + * geni_read_elf() - Read an ELF file.

Not only does it "read an ELF"...

> + * @se: Pointer to the SE resources structure.
> + * @fw: Pointer to the firmware buffer.
> + * @pelfseg: Pointer to the SE-specific ELF header.
> + *
> + * Read the ELF file and output a pointer to the header data, which
> + * contains the firmware data and any other details.

...and not only does it output a pointer to _the_ header data. But it
actually finds the SE firmware for the protocol that has been requested
by the caller^4.

I think this function should be:

  static const struct se_fw_hdr *geni_find_protocol_fw(fw, protocol)

Which from just the prototype looks like a function that takes a
firmware pointer, and somehow find the relevant SE firmware header for
the requested protocol.

> + *
> + * Return: 0 if successful, otherwise return an error value.
> + */
> +static int geni_read_elf(struct geni_se *se, const struct firmware *fw, struct se_fw_hdr **pelfseg)
> +{
> +	const struct elf32_hdr *ehdr;
> +	struct elf32_phdr *phdrs, *phdr;
> +	const struct se_fw_hdr *elfseg;

This doesn't represent any ELF segment, but rather the SE firmware in an
ELF segment. So, I think it would be better if this was named sefw;

Same thing, you don't really return an "ELF segment", but rather a
pointer to the SE firmware.

> +	const u8 *addr;
> +	int i;
> +
> +	if (!fw || fw->size < sizeof(struct elf32_hdr))
> +		return -EINVAL;
> +
> +	ehdr = (struct elf32_hdr *)fw->data;
> +	phdrs = (struct elf32_phdr *)(ehdr + 1);

Sorry for being sloppy when I wrote the code that you copied this
from... This should be:

phdrs = (struct elf32_phdr *)(fw->data + ehdr->e_phoff);

> +
> +	if (ehdr->e_phnum < 2)

Why?

> +		return -EINVAL;
> +
> +	for (i = 0; i < ehdr->e_phnum; i++) {
> +		phdr = &phdrs[i];
> +
> +		if (fw->size < phdr->p_offset + phdr->p_filesz)
> +			return -EINVAL;
> +
> +		if (phdr->p_type != PT_LOAD || !phdr->p_memsz)
> +			continue;
> +
> +		if (MI_PBT_PAGE_MODE_VALUE(phdr->p_flags) != MI_PBT_NON_PAGED_SEGMENT ||
> +		    MI_PBT_SEGMENT_TYPE_VALUE(phdr->p_flags) == MI_PBT_HASH_SEGMENT ||
> +		    MI_PBT_ACCESS_TYPE_VALUE(phdr->p_flags) == MI_PBT_NOTUSED_SEGMENT ||
> +		    MI_PBT_ACCESS_TYPE_VALUE(phdr->p_flags) == MI_PBT_SHARED_SEGMENT)
> +			continue;
> +
> +		if (phdr->p_filesz < sizeof(struct se_fw_hdr))
> +			continue;
> +
> +		addr = fw->data + phdr->p_offset;
> +		elfseg = (const struct se_fw_hdr *)addr;

		elfseg = (const struct se_fw_hdr *)(fw->data + phdr->p_offset);

And then you can drop addr.

> +
> +		if (cpu_to_le32(elfseg->magic) != MAGIC_NUM_SE || elfseg->version != 1)

As LKP pointed out you're converting this __le32 in the wrong direction
(i.e. use le32_to_cpu()).

> +			continue;
> +
> +		if (phdr->p_filesz < elfseg->fw_offset +
> +				     elfseg->fw_size_in_items * sizeof(u32) ||
> +		    phdr->p_filesz < elfseg->cfg_idx_offset +
> +				     elfseg->cfg_size_in_items * sizeof(u8) ||
> +		    phdr->p_filesz < elfseg->cfg_val_offset +
> +				     elfseg->cfg_size_in_items * sizeof(u32))
> +			continue;

Even if this isn't the firmware for the protocol you're looking for, I'd
say that if you have determined that you've found a valid SE FW segment
and any of these checks doesn't pass, you have a truncated/corrupt
firmware file. I think a error print is warranted here.


Looks like you can squeeze these lines in under 100 characters if you
remove the line wrap - that will make this chunk easier to read, so
please unwrap them.


Stylistic, I prefer the variable to be on the left of the comparator and
the constant to the right; i.e. "elfseg->fw_offset + ... > phdr->p_filesz"

> +
> +		if (elfseg->serial_protocol != se->protocol)
> +			continue;

The key part of this loop is that you're looking for a SE firmware or
the right protocol.

The fact that the segment must be big enough etc is a secondary sanity
check to ensure you're not consuming bad firmware.

> +
> +		*pelfseg = (struct se_fw_hdr *)addr;

*pelfseg = elfseg;

Although, this function only has two "return paths", either it return
-EINVAL or it return 0 with *pelfseg assigned.

So, rather than using a pointer argument to return the result, make the
return value const struct *se_fw_hdr and return elfseg here.

> +		return 0;
> +	}
> +	return -EINVAL;
> +}
> +
> +/**
> + * geni_configure_xfer_mode() - Set the transfer mode.
> + * @se: Pointer to a structure representing SE-related resources.
> + *
> + * Set the transfer mode to either FIFO or DMA according to the mode specified by the protocol
> + * driver.

This can be wrapped to 80 characters without loss of readability.

> + *
> + * Return: 0 if successful, otherwise return an error value.
> + */
> +static int geni_configure_xfer_mode(struct geni_se *se)
> +{
> +	/* Configure SE FIFO, DMA or GSI mode. */
> +	switch (se->mode) {
> +	case GENI_GPI_DMA:
> +		geni_setbits32(se->base + SE_GENI_DMA_MODE_EN, GENI_DMA_MODE_EN);
> +		writel_relaxed(0x0, se->base + SE_IRQ_EN);
> +		writel_relaxed(DMA_RX_EVENT_EN | DMA_TX_EVENT_EN |
> +			       GENI_M_EVENT_EN | GENI_S_EVENT_EN,
> +			       se->base + SE_GSI_EVENT_EN);
> +		break;
> +
> +	case GENI_SE_FIFO:
> +		geni_clrbits32(se->base + SE_GENI_DMA_MODE_EN, GENI_DMA_MODE_EN);
> +		writel_relaxed(DMA_RX_IRQ_EN | DMA_TX_IRQ_EN | GENI_M_IRQ_EN | GENI_S_IRQ_EN,
> +			       se->base + SE_IRQ_EN);
> +		writel_relaxed(0x0, se->base + SE_GSI_EVENT_EN);
> +		break;
> +
> +	case GENI_SE_DMA:
> +		geni_setbits32(se->base + SE_GENI_DMA_MODE_EN, GENI_DMA_MODE_EN);
> +		writel_relaxed(DMA_RX_IRQ_EN | DMA_TX_IRQ_EN | GENI_M_IRQ_EN | GENI_S_IRQ_EN,
> +			       se->base + SE_IRQ_EN);
> +		writel_relaxed(0x0, se->base + SE_GSI_EVENT_EN);
> +		break;
> +
> +	default:
> +		dev_err(se->dev, "Invalid geni-se transfer mode: %d\n", se->mode);
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * geni_enable_interrupts() - Enable interrupts.
> + * @se: Pointer to a structure representing SE-related resources.
> + *
> + * Enable the required interrupts during the firmware load process.
> + *
> + */
> +static void geni_enable_interrupts(struct geni_se *se)
> +{
> +	u32 reg_value;

"val" has the same amount of information, but is shorter and easier to
read...

> +
> +	/* Enable required interrupts. */
> +	writel_relaxed(M_COMMON_GENI_M_IRQ_EN, se->base + SE_GENI_M_IRQ_EN);

Don't you need/want a wmb() before this (or use one writel()), to ensure
previous programming is performed before you enable IRQs?

> +
> +	reg_value = S_CMD_OVERRUN_EN | S_ILLEGAL_CMD_EN |
> +		    S_CMD_CANCEL_EN | S_CMD_ABORT_EN |
> +		    S_GP_IRQ_0_EN | S_GP_IRQ_1_EN |
> +		    S_GP_IRQ_2_EN | S_GP_IRQ_3_EN |
> +		    S_RX_FIFO_WR_ERR_EN | S_RX_FIFO_RD_ERR_EN;
> +	writel_relaxed(reg_value, se->base + SE_GENI_S_IRQ_ENABLE);
> +
> +	/* DMA mode configuration. */
> +	reg_value = RESET_DONE_EN | SBE_EN | DMA_DONE_EN;
> +	writel_relaxed(reg_value, se->base + SE_DMA_TX_IRQ_EN_SET);
> +	reg_value = FLUSH_DONE_EN | RESET_DONE_EN | SBE_EN | DMA_DONE_EN;
> +	writel_relaxed(reg_value, se->base + SE_DMA_RX_IRQ_EN_SET);
> +}
> +
> +/**
> + * geni_write_fw_revision() - Write the firmware revision.
> + * @se: Pointer to a structure representing SE-related resources.
> + * @serial_protocol: serial protocol type.
> + * @fw_version: QUP firmware version.
> + *
> + * Write the firmware revision and protocol into the respective register.
> + *
> + * Return: None.
> + */
> +static void geni_write_fw_revision(struct geni_se *se, u16 serial_protocol, u16 fw_version)
> +{
> +	u32 reg_value;
> +
> +	reg_value = FIELD_PREP(FW_REV_PROTOCOL_MSK, serial_protocol);
> +	reg_value |= FIELD_PREP(FW_REV_VERSION_MSK, fw_version);
> +
> +	writel_relaxed(reg_value, se->base + SE_GENI_FW_REVISION);
> +	writel_relaxed(reg_value, se->base + SE_S_FW_REVISION);
> +}
> +
> +/**
> + * geni_load_se_fw() - Load Serial Engine specific firmware.
> + * @se: Pointer to a structure representing SE-related resources.
> + * @fw: Pointer to the firmware structure.
> + *
> + * Load the protocol firmware into the IRAM of the Serial Engine.
> + *
> + * Return: 0 if successful, otherwise return an error value.
> + */
> +static int geni_load_se_fw(struct geni_se *se, const struct firmware *fw)
> +{
> +	const u32 *fw_data, *cfg_val_arr;
> +	const u8 *cfg_idx_arr;
> +	u32 i, reg_value, ramn_cnt;
> +	int ret;
> +	struct se_fw_hdr *hdr;
> +
> +	ret = geni_read_elf(se, fw, &hdr);
> +	if (ret) {
> +		dev_err(se->dev, "ELF parsing failed ret: %d\n", ret);

If you move this error print to geni_read_elf() you can make it more
useful - i.e. you can distinguish between not finding a sefw or the file
being truncated etc.

> +		return ret;
> +	}
> +
> +	ramn_cnt = hdr->fw_size_in_items;
> +	if (hdr->fw_size_in_items % 2 != 0)
> +		ramn_cnt++;
> +
> +	if (ramn_cnt >= MAX_GENI_CFG_RAMn_CNT)

I presume it's unlikely that there will be an odd number of
fw_size_in_items, that is less than MAX_GENI_CFG_RAMn_CNT, but the
validation in geni_read_elf() only accounted for that you will access
fw_size_in_items number of items from fw->data; not the rounded up
value.

Also, it would probably be helpful to the user if you provided an error
message about corrupt firmware here.

> +		return -EINVAL;
> +
> +	ret = geni_icc_set_bw(se);
> +	if (ret)
> +		return ret;
> +
> +	ret = geni_icc_enable(se);
> +	if (ret)
> +		return ret;
> +
> +	ret = geni_se_resources_on(se);
> +	if (ret)
> +		goto err;
> +
> +	ramn_cnt = hdr->fw_size_in_items;

These adjustments and checks was performed 19 lines above as well, but
before the hardware was spun up.

> +	if (hdr->fw_size_in_items % 2 != 0)
> +		ramn_cnt++;
> +
> +	if (ramn_cnt >= MAX_GENI_CFG_RAMn_CNT)
> +		goto err_resource;
> +
> +	fw_data = (const u32 *)((u8 *)hdr + hdr->fw_offset);
> +	cfg_idx_arr = (const u8 *)hdr + hdr->cfg_idx_offset;
> +	cfg_val_arr = (const u32 *)((u8 *)hdr + hdr->cfg_val_offset);
> +
> +	/* Disable high priority interrupt until current low priority interrupts are handled. */
> +	geni_setbits32(se->wrapper->base + QUPV3_COMMON_CFG, FAST_SWITCH_TO_HIGH_DISABLE);
> +
> +	/* Set AHB_M_CLK_CGC_ON to indicate hardware controls se-wrapper cgc clock. */
> +	geni_setbits32(se->wrapper->base + QUPV3_SE_AHB_M_CFG, AHB_M_CLK_CGC_ON);
> +
> +	/* Let hardware to control common cgc. */
> +	geni_setbits32(se->wrapper->base + QUPV3_COMMON_CGC_CTRL, COMMON_CSR_SLV_CLK_CGC_ON);
> +
> +	/* Allows to drive corresponding data according to hardware value. */

What does this sentence mean?

> +	writel_relaxed(0x0, se->base + GENI_OUTPUT_CTRL);
> +
> +	/* Set SCLK and HCLK to program RAM */
> +	geni_setbits32(se->base + SE_GENI_CGC_CTRL, PROG_RAM_SCLK_OFF | PROG_RAM_HCLK_OFF);
> +	writel_relaxed(0x0, se->base + SE_GENI_CLK_CTRL);
> +	geni_clrbits32(se->base + SE_GENI_CGC_CTRL, PROG_RAM_SCLK_OFF | PROG_RAM_HCLK_OFF);
> +
> +	/* Enable required clocks for DMA CSR, TX and RX. */
> +	reg_value = AHB_SEC_SLV_CLK_CGC_ON | DMA_AHB_SLV_CLK_CGC_ON |
> +		    DMA_TX_CLK_CGC_ON | DMA_RX_CLK_CGC_ON;
> +	geni_setbits32(se->base + SE_DMA_GENERAL_CFG, reg_value);
> +
> +	/* Let hardware control CGC by default. */
> +	writel_relaxed(DEFAULT_CGC_EN, se->base + SE_GENI_CGC_CTRL);
> +
> +	/* Set version of the configuration register part of firmware. */
> +	writel_relaxed(hdr->cfg_version, se->base + SE_GENI_INIT_CFG_REVISION);
> +	writel_relaxed(hdr->cfg_version, se->base + SE_GENI_S_INIT_CFG_REVISION);
> +
> +	/* Configure GENI primitive table. */
> +	for (i = 0; i < hdr->cfg_size_in_items; i++)
> +		writel_relaxed(cfg_val_arr[i],
> +			       se->base + SE_GENI_CFG_REG0 + (cfg_idx_arr[i] * sizeof(u32)));
> +
> +	/* Configure condition for assertion of RX_RFR_WATERMARK condition. */
> +	reg_value = geni_se_get_rx_fifo_depth(se);
> +	writel_relaxed(reg_value - 2, se->base + SE_GENI_RX_RFR_WATERMARK_REG);
> +
> +	/* Let hardware control CGC */
> +	geni_setbits32(se->base + GENI_OUTPUT_CTRL, DEFAULT_IO_OUTPUT_CTRL_MSK);
> +
> +	ret = geni_configure_xfer_mode(se);
> +	if (ret)
> +		goto err_resource;
> +
> +	geni_enable_interrupts(se);
> +
> +	geni_write_fw_revision(se, hdr->serial_protocol, hdr->fw_version);
> +
> +	ramn_cnt = hdr->fw_size_in_items;

hdr-.fw_size_in_items shouldn't have changed since you validated it 60
and 79 lines above, right?

> +	if (hdr->fw_size_in_items % 2 != 0)
> +		ramn_cnt++;
> +
> +	if (ramn_cnt >= MAX_GENI_CFG_RAMn_CNT)
> +		goto err_resource;

I suspect that you're going to remove this check and return, but at this
point you have enabled a bunch of hardware and you're exiting without
cleaning up...

> +
> +	/* Program RAM address space. */
> +	memcpy_toio(se->base + SE_GENI_CFG_RAMN, fw_data, ramn_cnt * sizeof(u32));
> +
> +	/* Put default values on GENI's output pads. */
> +	writel_relaxed(0x1, se->base + GENI_FORCE_DEFAULT_REG);
> +
> +	/* High to low SCLK and HCLK to finish RAM. */

"finish RAM <what>"?

> +	geni_setbits32(se->base + SE_GENI_CGC_CTRL, PROG_RAM_SCLK_OFF | PROG_RAM_HCLK_OFF);
> +	geni_setbits32(se->base + SE_GENI_CLK_CTRL, SER_CLK_SEL);
> +	geni_clrbits32(se->base + SE_GENI_CGC_CTRL, PROG_RAM_SCLK_OFF | PROG_RAM_HCLK_OFF);

All these IO accesses are relaxed, allowing the compiler and the CPU to
reorder them. Isn't there a point here where the QUP will start running
the newly loaded firmware - and you want to make sure that all the setup
(and the firmware loading) isn't reordered past that point?

Or is the ordering of access to all these registers govern by something
else?

> +
> +	/* Serial engine DMA interface is enabled. */
> +	geni_setbits32(se->base + SE_DMA_IF_EN, DMA_IF_EN);
> +
> +	/* Enable or disable FIFO interface of the serial engine. */
> +	if (se->mode == GENI_SE_FIFO)
> +		geni_clrbits32(se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
> +	else
> +		geni_setbits32(se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
> +
> +err_resource:

Rename this out_resources_off

> +	geni_se_resources_off(se);
> +err:

And this out_icc_disable

That way one can understand how the exit will look like without jumping
here.

> +	geni_icc_disable(se);
> +	return ret;
> +}
> +
> +/**
> + * qup_fw_load() - Initiate firmware load.
> + * @se: Pointer to a structure representing SE-related resources.
> + * @fw_name: Name of the firmware.
> + *
> + * Load the firmware into a specific SE. Read the associated ELF file,
> + * copy the data into a buffer in kernel space using the request_firmware API, write the
> + * data into the SE's IRAM register, and then free the buffers. Handle firmware loading
> + * and parsing for a specific protocol.
> + *

Wrap all your kernel-doc bodies to 80 characters, please.

> + * Return: 0 if successful, otherwise return an error value.
> + */
> +static int qup_fw_load(struct geni_se *se, const char *fw_name)

All other functions in this driver are prefixed "geni_". Please keep the
naming consistent.

That said, moving the content of this function into
geni_load_se_firmware() wouldn't worsen that function much and you
remove a dummy intermediate function. So, please inline this.

> +{
> +	int ret;
> +	const struct firmware *fw;
> +	struct device *dev = se->dev;
> +
> +	ret = request_firmware(&fw, fw_name, dev);
> +	if (ret) {
> +		dev_err(dev, "request_firmware failed for %d: %d\n", se->protocol, ret);
> +		return ret;
> +	}
> +
> +	ret = geni_load_se_fw(se, fw);
> +
> +	release_firmware(fw);
> +
> +	return ret;
> +}
> +
> +/**
> + * geni_load_se_firmware() - Initiate firmware loading.

Is this really "initiate firmware loading"? Isn't it actually completing
the firmware loading? I.e. this comment should be "Load SE firmware" or
such?

> + * @se: Serial engine details.
> + * @protocol: Protocol (SPI, I2C, or UART) for which the firmware is to be loaded.
> + *
> + * If the device tree properties are configured to load QUP firmware and the firmware
> + * is not already loaded, start the firmware loading process. If the device tree properties
> + * are not defined, skip loading the firmware, assuming it is already loaded by TZ.

This too talks about start firmware loading, but I don't see any code
that would "finish" it.

The short form comment for qup_fw_load() above also says "Initiate", but
the longer description indicates that it concludes the load.

> + *
> + * Return: 0 if successful, otherwise return an error value.
> + */
> +int geni_load_se_firmware(struct geni_se *se, enum geni_se_protocol_type protocol)
> +{
> +	const char *fw_name;
> +	int ret;
> +
> +	if (protocol >= MAX_PROTOCOL) {

The only place I can see where this is important is in below dev_dbg()
print, to avoid reading outside protocol_name[]. You can make that
clearer by comparing with ARRAY_SIZE(protocol_name) instead.

> +		dev_err(se->dev, "Invalid geni-se protocol: %d", protocol);
> +		return  -EINVAL;

There are two spaces between return and -EINVAL.

> +	}
> +
> +	ret = device_property_read_string(se->wrapper->dev, "firmware-name", &fw_name);
> +	if (ret)
> +		return  -EINVAL;

Two more spaces

> +
> +	se->protocol = protocol;

You assign se->protocol here, and it's read in geni_read_elf()...

> +
> +	if (of_property_read_bool(se->dev->of_node, "qcom,enable-gsi-dma"))
> +		se->mode = GENI_GPI_DMA;
> +	else
> +		se->mode = GENI_SE_FIFO;

...and you assign se->mode here and it's read in geni_configure_xfer_mode()
and geni_load_se_fw().

But the callstack looks like:
  geni_load_se_firmware()
    qup_fw_load()
      geni_load_se_fw()
        geni_read_elf()
        geni_configure_xfer_mode()

So, both protocol and mode are only used in the scope of this function.
Passing them around using struct geni_se only obfuscates this fact.

Please pass them as arguments instead to make this clear.

> +
> +	/* GSI mode is not supported by the UART driver; therefore, setting FIFO mode */
> +	if (protocol == GENI_SE_UART)
> +		se->mode = GENI_SE_FIFO;
> +
> +	ret = qup_fw_load(se, fw_name);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(se->dev, "Firmware load for %s protocol is successful for xfer mode %d\n",
> +		protocol_name[se->protocol], se->mode);

Use the protocol and mode from the stack.

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(geni_load_se_firmware);
> +
>  static int geni_se_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
> index 2996a3c28ef3..6b75171d65ec 100644
> --- a/include/linux/soc/qcom/geni-se.h
> +++ b/include/linux/soc/qcom/geni-se.h
> @@ -1,11 +1,13 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  /*
>   * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
>   */
>  
>  #ifndef _LINUX_QCOM_GENI_SE
>  #define _LINUX_QCOM_GENI_SE
>  
> +#include <linux/bitfield.h>
>  #include <linux/interconnect.h>
>  
>  /**
> @@ -36,6 +38,7 @@ enum geni_se_protocol_type {
>  	GENI_SE_I2C,
>  	GENI_SE_I3C,
>  	GENI_SE_SPI_SLAVE,
> +	GENI_SE_INVALID_PROTO = 255,
>  };
>  
>  struct geni_wrapper;
> @@ -61,6 +64,8 @@ struct geni_icc_path {
>   * @num_clk_levels:	Number of valid clock levels in clk_perf_tbl
>   * @clk_perf_tbl:	Table of clock frequency input to serial engine clock
>   * @icc_paths:		Array of ICC paths for SE
> + * @mode:		Transfer mode se fifo, dma or gsi.
> + * @protocol:		Protocol spi or i2c or serial.
>   */
>  struct geni_se {
>  	void __iomem *base;
> @@ -70,24 +75,32 @@ struct geni_se {
>  	unsigned int num_clk_levels;
>  	unsigned long *clk_perf_tbl;
>  	struct geni_icc_path icc_paths[3];
> +	enum geni_se_xfer_mode mode;
> +	enum geni_se_protocol_type protocol;
>  };
>  
>  /* Common SE registers */
> +#define SE_GENI_INIT_CFG_REVISION	0x0
> +#define SE_GENI_S_INIT_CFG_REVISION	0x4
>  #define GENI_FORCE_DEFAULT_REG		0x20
>  #define GENI_OUTPUT_CTRL		0x24
> +#define SE_GENI_CGC_CTRL		0x28
>  #define SE_GENI_STATUS			0x40
>  #define GENI_SER_M_CLK_CFG		0x48
>  #define GENI_SER_S_CLK_CFG		0x4c
>  #define GENI_IF_DISABLE_RO		0x64
> -#define GENI_FW_REVISION_RO		0x68
> +#define SE_GENI_FW_REVISION_RO		0x68
> +#define SE_GENI_S_FW_REVISION_RO	0x6c
>  #define SE_GENI_CLK_SEL			0x7c
>  #define SE_GENI_CFG_SEQ_START		0x84
> +#define SE_GENI_CFG_REG0		0x100
>  #define SE_GENI_DMA_MODE_EN		0x258
>  #define SE_GENI_M_CMD0			0x600
>  #define SE_GENI_M_CMD_CTRL_REG		0x604
>  #define SE_GENI_M_IRQ_STATUS		0x610
>  #define SE_GENI_M_IRQ_EN		0x614
>  #define SE_GENI_M_IRQ_CLEAR		0x618
> +#define SE_GENI_S_IRQ_ENABLE		0x644
>  #define SE_GENI_M_IRQ_EN_SET		0x61c
>  #define SE_GENI_M_IRQ_EN_CLEAR		0x620
>  #define SE_GENI_S_CMD0			0x630
> @@ -109,13 +122,22 @@ struct geni_se {
>  #define SE_GENI_S_GP_LENGTH		0x914
>  #define SE_DMA_TX_IRQ_STAT		0xc40
>  #define SE_DMA_TX_IRQ_CLR		0xc44
> +#define SE_DMA_TX_IRQ_EN_SET		0xc4c
>  #define SE_DMA_TX_FSM_RST		0xc58
>  #define SE_DMA_RX_IRQ_STAT		0xd40
>  #define SE_DMA_RX_IRQ_CLR		0xd44
> +#define SE_DMA_RX_IRQ_EN_SET		0xd4c
>  #define SE_DMA_RX_LEN_IN		0xd54
>  #define SE_DMA_RX_FSM_RST		0xd58
>  #define SE_HW_PARAM_0			0xe24
>  #define SE_HW_PARAM_1			0xe28
> +#define SE_DMA_GENERAL_CFG		0xe30
> +#define SE_GENI_FW_REVISION		0x1000
> +#define SE_S_FW_REVISION		0x1004
> +#define SE_GENI_CFG_RAMN		0x1010
> +#define SE_GENI_CLK_CTRL		0x2000
> +#define SE_DMA_IF_EN			0x2004
> +#define SE_FIFO_IF_DISABLE		0x2008
>  
>  /* GENI_FORCE_DEFAULT_REG fields */
>  #define FORCE_DEFAULT	BIT(0)
> @@ -137,7 +159,7 @@ struct geni_se {
>  
>  /* GENI_FW_REVISION_RO fields */
>  #define FW_REV_PROTOCOL_MSK		GENMASK(15, 8)
> -#define FW_REV_PROTOCOL_SHFT		8
> +#define FW_REV_VERSION_MSK		GENMASK(7, 0)
>  
>  /* GENI_CLK_SEL fields */
>  #define CLK_SEL_MSK			GENMASK(2, 0)
> @@ -325,9 +347,9 @@ static inline u32 geni_se_read_proto(struct geni_se *se)

It is assumed that this function will return an enum
geni_se_protocol_type.

I think you should follow up with a patch to not throw this around as a
u32.

>  {
>  	u32 val;
>  
> -	val = readl_relaxed(se->base + GENI_FW_REVISION_RO);
> +	val = readl_relaxed(se->base + SE_GENI_FW_REVISION_RO);
>  
> -	return (val & FW_REV_PROTOCOL_MSK) >> FW_REV_PROTOCOL_SHFT;
> +	return FIELD_GET(FW_REV_PROTOCOL_MSK, val);

Nice, but unrelated to firmware loading.

>  }
>  
>  /**
> @@ -531,5 +553,7 @@ void geni_icc_set_tag(struct geni_se *se, u32 tag);
>  int geni_icc_enable(struct geni_se *se);
>  
>  int geni_icc_disable(struct geni_se *se);
> +
> +int geni_load_se_firmware(struct geni_se *se, enum geni_se_protocol_type protocol);
>  #endif
>  #endif
> diff --git a/include/linux/soc/qcom/qup-fw-load.h b/include/linux/soc/qcom/qup-fw-load.h

As this is only going to be used from qcom-geni-se.c, I think you should
just put this content in that file directly.

> new file mode 100644
> index 000000000000..94a6c4c10bcf
> --- /dev/null
> +++ b/include/linux/soc/qcom/qup-fw-load.h
> @@ -0,0 +1,93 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +#ifndef _LINUX_QCOM_QUP_FW_LOAD
> +#define _LINUX_QCOM_QUP_FW_LOAD
> +
> +#include <linux/kernel.h>
> +
> +/*Magic numbers*/
> +#define MAGIC_NUM_SE			0x57464553

Please make "SE" the prefix, rather than suffix.

> +
> +#define MAX_GENI_CFG_RAMn_CNT		455
> +
> +#define MI_PBT_NON_PAGED_SEGMENT	0x0
> +#define MI_PBT_HASH_SEGMENT		0x2
> +#define MI_PBT_NOTUSED_SEGMENT		0x3
> +#define MI_PBT_SHARED_SEGMENT		0x4
> +
> +#define MI_PBT_FLAG_PAGE_MODE		BIT(20)
> +#define MI_PBT_FLAG_SEGMENT_TYPE	GENMASK(26, 24)
> +#define MI_PBT_FLAG_ACCESS_TYPE		GENMASK(23, 21)
> +
> +#define MI_PBT_PAGE_MODE_VALUE(x) FIELD_GET(MI_PBT_FLAG_PAGE_MODE, x)
> +
> +#define MI_PBT_SEGMENT_TYPE_VALUE(x) FIELD_GET(MI_PBT_FLAG_SEGMENT_TYPE, x)
> +
> +#define MI_PBT_ACCESS_TYPE_VALUE(x) FIELD_GET(MI_PBT_FLAG_ACCESS_TYPE, x)
> +
> +#define M_COMMON_GENI_M_IRQ_EN	(GENMASK(6, 1) | \
> +				M_IO_DATA_DEASSERT_EN | \

These constants are defined in geni-se.h, so if someone includes this
file before that one, this won't compile.

> +				M_IO_DATA_ASSERT_EN | M_RX_FIFO_RD_ERR_EN | \
> +				M_RX_FIFO_WR_ERR_EN | M_TX_FIFO_RD_ERR_EN | \
> +				M_TX_FIFO_WR_ERR_EN)
> +
> +/* DMA_TX/RX_IRQ_EN fields */
> +#define DMA_DONE_EN		BIT(0)

There's already a GENI_SE_DMA_DONE_EN define in qcom-geni-se.c, used for
the SE_DMA_TX_IRQ_EN_SET register.

> +#define SBE_EN			BIT(2)

And this bit is today called GENI_SE_DMA_AHB_ERR_EN. Did the name
change, does different hardware have different functionality for this
one bit? If so, how would one know that geni_se_tx_init_dma() and
geni_enable_interrupts() uses incompatible bit definitions?

This shows why it's important that you don't mix renames and functional
additions!

> +#define RESET_DONE_EN		BIT(3)
> +#define FLUSH_DONE_EN		BIT(4)
> +
> +/* GENI_CLK_CTRL fields */
> +#define SER_CLK_SEL		BIT(0)
> +
> +/* GENI_DMA_IF_EN fields */
> +#define DMA_IF_EN		BIT(0)
> +
> +#define QUPV3_COMMON_CFG		0x120

Group this with QUP_HW_VER_REG, to indicate that it belongs to that
register space and not the SE space.

> +#define FAST_SWITCH_TO_HIGH_DISABLE	BIT(0)
> +
> +#define QUPV3_SE_AHB_M_CFG		0x118
> +#define AHB_M_CLK_CGC_ON		BIT(0)
> +
> +#define QUPV3_COMMON_CGC_CTRL		0x21C
> +#define COMMON_CSR_SLV_CLK_CGC_ON	BIT(0)
> +
> +/* access ports */

There are no "ports"...

> +#define geni_setbits32(_addr, _v) writel_relaxed(readl_relaxed(_addr) |  (_v), _addr)
> +#define geni_clrbits32(_addr, _v) writel_relaxed(readl_relaxed(_addr) & ~(_v), _addr)
> +
> +/**
> + * struct se_fw_hdr - Serial Engine firmware configuration header
> + *
> + * This structure defines metadata for the Serial Engine (SE) firmware
> + * configuration. Although it is embedded within an ELF segment, it is
> + * not part of the ELF format itself.

Perhaps:

"""
This structure defines the SE firmware header, which together with the
firmware payload is stored in individual ELF segments.
"""

> + *
> + * @magic: Set to 'SEFW'
> + * @version: Structure version number
> + * @core_version: QUPV3 hardware version
> + * @serial_protocol: Encoded in GENI_FW_REVISION
> + * @fw_version: Firmware version, from GENI_FW_REVISION
> + * @cfg_version: Configuration version, from GENI_INIT_CFG_REVISION
> + * @fw_size_in_items: Number of 32-bit words in GENI_FW_RAM
> + * @fw_offset: Byte offset to GENI_FW_RAM array
> + * @cfg_size_in_items: Number of GENI_FW_CFG index/value pairs
> + * @cfg_idx_offset: Byte offset to GENI_FW_CFG index array
> + * @cfg_val_offset: Byte offset to GENI_FW_CFG values array
> + */
> +struct se_fw_hdr {
> +	__le32 magic;

Kudos for specifying the endianess of this member, but what about the
rest? How come magic is little endian but the other members are
magically native endian?

Preferable you define them all with proper endian annotation.

Regards,
Bjorn

> +	u32 version;
> +	u32 core_version;
> +	u16 serial_protocol;
> +	u16 fw_version;
> +	u16 cfg_version;
> +	u16 fw_size_in_items;
> +	u16 fw_offset;
> +	u16 cfg_size_in_items;
> +	u16 cfg_idx_offset;
> +	u16 cfg_val_offset;
> +};
> +#endif /* _LINUX_QCOM_QUP_FW_LOAD */
> -- 
> 2.34.1
> 

Re: [PATCH v5 2/5] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem

[Viken Dadhaniya]

On 7/17/2025 4:57 AM, Bjorn Andersson wrote:
> On Tue, Jun 24, 2025 at 03:20:59PM +0530, Viken Dadhaniya wrote:
>> In Qualcomm SoCs, firmware loading for Serial Engines (SE) within the QUP
>> hardware has traditionally been managed by TrustZone (TZ). This restriction
>> poses a significant challenge for developers, as it limits their ability to
>> enable various protocols on any of the SEs from the Linux side, reducing
>> flexibility.
>>
>> Load the firmware to QUP SE based on the 'firmware-name' property specified
>> in devicetree at bootup time.
>>
>> Co-developed-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
>> Signed-off-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
>> Signed-off-by: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
>> ---
>> v4 -> v5:
>>
>> - Resolved kernel test robot error by including the missing bitfield header file.
>> - Updated the SE firmware ELF structure name for consistency.
>> - Specified _leb4 format for the magic number definition.
>> - Updated the email domain from 'quic' to 'oss'.
>>
>> v4 Link: https://lore.kernel.org/all/20250503111029.3583807-3-quic_vdadhani@quicinc.com/
>>
>> v3 -> v4:
>>
>> - Update the commit message.
>> - Resolve kernel test robot warnings.
>> - Add a multiline comment in the Copyright section.
>> - Remove valid_seg_size and geni_config_common_control functions, and add the code inline.
>> - Rename read_elf function to geni_read_elf.
>> - Add a firmware size check.
>> - Assign *pelfseg after finding a match.
>> - Break one large condition check into multiple checks to improve code readability.
>> - Remove return type documentation for void functions.
>> - Update error messages to be more descriptive.
>> - Correct indentation.
>> - Rename geni_flash_fw_revision function to geni_write_fw_revision.
>> - Remove __func__ from all print statements.
>> - Move resource_on to the appropriate section after parsing the firmware file.
>> - Update variable names and function arguments as suggested.
>> - Use FIELD_GET, FIELD_PREP, and GENMASK.
>> - Use memcpy_toio() instead of memcpy.
>> - Remove duplicate registers and bitmask macros.
>> - Remove rsc struct and add required variables in geni_se struct.
>>
>> v3 Link: https://lore.kernel.org/linux-arm-msm/20250303124349.3474185-7-quic_vdadhani@quicinc.com/
>>
>> v2 -> v3:
>>
>> - Remove code related to the 'qcom,xfer-mode' property.
>> - Add logic to read the boolean property 'qcom,gsi-dma-allowed' and select the transfer mode.
>> - Hardcode FIFO mode for the serial driver as GSI mode is currently not supported.
>> - Update function descriptions as suggested.
>> - Enhance error handling and remove redundant if conditions.
>> - Drop the ternary operator.
>>
>> v2 Link: https://lore.kernel.org/linux-arm-msm/20250124105309.295769-6-quic_vdadhani@quicinc.com/
>>
>> v1 -> v2:
>>
>> - Remove the fixed firmware path and add logic to read the path from the device tree.
>> - Remove code related to the 'qcom,load-firmware' property.
>> - Resolve kernel test robot warnings.
>> - Update the commit message.
>> - Update Copyright year.
>>
>> v1 Link: https://lore.kernel.org/linux-kernel/20241204150326.1470749-5-quic_vdadhani@quicinc.com/
>> ---
>> ---
>>  drivers/soc/qcom/qcom-geni-se.c      | 407 +++++++++++++++++++++++++--
>>  include/linux/soc/qcom/geni-se.h     |  32 ++-
>>  include/linux/soc/qcom/qup-fw-load.h |  93 ++++++
>>  3 files changed, 510 insertions(+), 22 deletions(-)
>>  create mode 100644 include/linux/soc/qcom/qup-fw-load.h
>>
>> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
>> index 4cb959106efa..ef4997a5fd63 100644
>> --- a/drivers/soc/qcom/qcom-geni-se.c
>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>> @@ -1,11 +1,15 @@
>>  // SPDX-License-Identifier: GPL-2.0
>> -// Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>> +/*
>> + *  Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>> + *  Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
> 
> Please use our updated Copyright header.

Updated in v6.

> 
>> + */
>>  
>>  /* Disable MMIO tracing to prevent excessive logging of unwanted MMIO traces */
>>  #define __DISABLE_TRACE_MMIO__
>>  
>>  #include <linux/acpi.h>
>>  #include <linux/clk.h>
>> +#include <linux/firmware.h>
>>  #include <linux/slab.h>
>>  #include <linux/dma-mapping.h>
>>  #include <linux/io.h>
>> @@ -15,6 +19,7 @@
>>  #include <linux/pinctrl/consumer.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/soc/qcom/geni-se.h>
>> +#include <linux/soc/qcom/qup-fw-load.h>
>>  
>>  /**
>>   * DOC: Overview
>> @@ -80,9 +85,9 @@
>>   * common to all the serial engines and are independent of serial interfaces.
>>   */
>>  
>> -#define MAX_CLK_PERF_LEVEL 32
>> -#define MAX_CLKS 2
>> -
>> +#define MAX_CLK_PERF_LEVEL	32
>> +#define MAX_CLKS		2
>> +#define MAX_PROTOCOL		6
>>  /**
>>   * struct geni_wrapper - Data structure to represent the QUP Wrapper Core
>>   * @dev:		Device pointer of the QUP wrapper core
>> @@ -110,28 +115,23 @@ struct geni_se_desc {
>>  static const char * const icc_path_names[] = {"qup-core", "qup-config",
>>  						"qup-memory"};
>>  
>> +static const char * const protocol_name[MAX_PROTOCOL] = { "None", "SPI", "UART",
>> +							  "I2C", "I3C", "SPI SLAVE"};
>> +
>>  #define QUP_HW_VER_REG			0x4
>>  
>>  /* Common SE registers */
>> -#define GENI_INIT_CFG_REVISION		0x0
>> -#define GENI_S_INIT_CFG_REVISION	0x4
>> -#define GENI_OUTPUT_CTRL		0x24
> 
> It would be nice if you did the rename and register cleanup in one
> "dumb" patch and then introduced the new functionality in a second
> patch.
> 

Created a separate patch and posted v6.

>> -#define GENI_CGC_CTRL			0x28
>>  #define GENI_CLK_CTRL_RO		0x60
>> -#define GENI_FW_S_REVISION_RO		0x6c
>>  #define SE_GENI_BYTE_GRAN		0x254
>>  #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_GP_LENGTH		0x910
>> -#define SE_GENI_S_GP_LENGTH		0x914
>>  #define SE_DMA_TX_PTR_L			0xc30
>>  #define SE_DMA_TX_PTR_H			0xc34
>>  #define SE_DMA_TX_ATTR			0xc38
>>  #define SE_DMA_TX_LEN			0xc3c
>>  #define SE_DMA_TX_IRQ_EN		0xc48
>> -#define SE_DMA_TX_IRQ_EN_SET		0xc4c
>>  #define SE_DMA_TX_IRQ_EN_CLR		0xc50
>>  #define SE_DMA_TX_LEN_IN		0xc54
>>  #define SE_DMA_TX_MAX_BURST		0xc5c
>> @@ -140,9 +140,7 @@ static const char * const icc_path_names[] = {"qup-core", "qup-config",
>>  #define SE_DMA_RX_ATTR			0xd38
>>  #define SE_DMA_RX_LEN			0xd3c
>>  #define SE_DMA_RX_IRQ_EN		0xd48
>> -#define SE_DMA_RX_IRQ_EN_SET		0xd4c
>>  #define SE_DMA_RX_IRQ_EN_CLR		0xd50
>> -#define SE_DMA_RX_LEN_IN		0xd54
>>  #define SE_DMA_RX_MAX_BURST		0xd5c
>>  #define SE_DMA_RX_FLUSH			0xd60
>>  #define SE_GSI_EVENT_EN			0xe18
>> @@ -179,7 +177,7 @@ static const char * const icc_path_names[] = {"qup-core", "qup-config",
>>  /* SE_DMA_GENERAL_CFG */
>>  #define DMA_RX_CLK_CGC_ON		BIT(0)
>>  #define DMA_TX_CLK_CGC_ON		BIT(1)
>> -#define DMA_AHB_SLV_CFG_ON		BIT(2)
>> +#define DMA_AHB_SLV_CLK_CGC_ON		BIT(2)
>>  #define AHB_SEC_SLV_CLK_CGC_ON		BIT(3)
>>  #define DUMMY_RX_NON_BUFFERABLE		BIT(4)
>>  #define RX_DMA_ZERO_PADDING_EN		BIT(5)
>> @@ -220,12 +218,12 @@ static void geni_se_io_init(void __iomem *base)
>>  {
>>  	u32 val;
>>  
>> -	val = readl_relaxed(base + GENI_CGC_CTRL);
>> +	val = readl_relaxed(base + SE_GENI_CGC_CTRL);
> 
> Per above comment, this change has nothing to do with firmware loading.

Created a separate patch and posted v6.

> 
>>  	val |= DEFAULT_CGC_EN;
>> -	writel_relaxed(val, base + GENI_CGC_CTRL);
>> +	writel_relaxed(val, base + SE_GENI_CGC_CTRL);
>>  
>>  	val = readl_relaxed(base + SE_DMA_GENERAL_CFG);
>> -	val |= AHB_SEC_SLV_CLK_CGC_ON | DMA_AHB_SLV_CFG_ON;
>> +	val |= AHB_SEC_SLV_CLK_CGC_ON | DMA_AHB_SLV_CLK_CGC_ON;
>>  	val |= DMA_TX_CLK_CGC_ON | DMA_RX_CLK_CGC_ON;
>>  	writel_relaxed(val, base + SE_DMA_GENERAL_CFG);
>>  
>> @@ -891,6 +889,379 @@ int geni_icc_disable(struct geni_se *se)
>>  }
>>  EXPORT_SYMBOL_GPL(geni_icc_disable);
>>  
>> +/**
>> + * geni_read_elf() - Read an ELF file.
> 
> Not only does it "read an ELF"...

Updated in v6.

> 
>> + * @se: Pointer to the SE resources structure.
>> + * @fw: Pointer to the firmware buffer.
>> + * @pelfseg: Pointer to the SE-specific ELF header.
>> + *
>> + * Read the ELF file and output a pointer to the header data, which
>> + * contains the firmware data and any other details.
> 
> ...and not only does it output a pointer to _the_ header data. But it
> actually finds the SE firmware for the protocol that has been requested
> by the caller^4.
> 
> I think this function should be:
> 
>   static const struct se_fw_hdr *geni_find_protocol_fw(fw, protocol)
> 
> Which from just the prototype looks like a function that takes a
> firmware pointer, and somehow find the relevant SE firmware header for
> the requested protocol.
>

 
Updated in v6.

>> + *
>> + * Return: 0 if successful, otherwise return an error value.
>> + */
>> +static int geni_read_elf(struct geni_se *se, const struct firmware *fw, struct se_fw_hdr **pelfseg)
>> +{
>> +	const struct elf32_hdr *ehdr;
>> +	struct elf32_phdr *phdrs, *phdr;
>> +	const struct se_fw_hdr *elfseg;
> 
> This doesn't represent any ELF segment, but rather the SE firmware in an
> ELF segment. So, I think it would be better if this was named sefw;
> 
> Same thing, you don't really return an "ELF segment", but rather a
> pointer to the SE firmware.
> 

Updated in v6.

>> +	const u8 *addr;
>> +	int i;
>> +
>> +	if (!fw || fw->size < sizeof(struct elf32_hdr))
>> +		return -EINVAL;
>> +
>> +	ehdr = (struct elf32_hdr *)fw->data;
>> +	phdrs = (struct elf32_phdr *)(ehdr + 1);
> 
> Sorry for being sloppy when I wrote the code that you copied this
> from... This should be:
> 
> phdrs = (struct elf32_phdr *)(fw->data + ehdr->e_phoff);
> 
>> +
>> +	if (ehdr->e_phnum < 2)
> 
> Why?

The firmware is expected to have at least two program headers (segments),
One for metadata and the other for the actual protocol-specific firmware.

Added comment in v6.

> 
>> +		return -EINVAL;
>> +
>> +	for (i = 0; i < ehdr->e_phnum; i++) {
>> +		phdr = &phdrs[i];
>> +
>> +		if (fw->size < phdr->p_offset + phdr->p_filesz)
>> +			return -EINVAL;
>> +
>> +		if (phdr->p_type != PT_LOAD || !phdr->p_memsz)
>> +			continue;
>> +
>> +		if (MI_PBT_PAGE_MODE_VALUE(phdr->p_flags) != MI_PBT_NON_PAGED_SEGMENT ||
>> +		    MI_PBT_SEGMENT_TYPE_VALUE(phdr->p_flags) == MI_PBT_HASH_SEGMENT ||
>> +		    MI_PBT_ACCESS_TYPE_VALUE(phdr->p_flags) == MI_PBT_NOTUSED_SEGMENT ||
>> +		    MI_PBT_ACCESS_TYPE_VALUE(phdr->p_flags) == MI_PBT_SHARED_SEGMENT)
>> +			continue;
>> +
>> +		if (phdr->p_filesz < sizeof(struct se_fw_hdr))
>> +			continue;
>> +
>> +		addr = fw->data + phdr->p_offset;
>> +		elfseg = (const struct se_fw_hdr *)addr;
> 
> 		elfseg = (const struct se_fw_hdr *)(fw->data + phdr->p_offset);
> 
> And then you can drop addr.

Updated in v6.

> 
>> +
>> +		if (cpu_to_le32(elfseg->magic) != MAGIC_NUM_SE || elfseg->version != 1)
> 
> As LKP pointed out you're converting this __le32 in the wrong direction
> (i.e. use le32_to_cpu()).
> 
>> +			continue;
>> +
>> +		if (phdr->p_filesz < elfseg->fw_offset +
>> +				     elfseg->fw_size_in_items * sizeof(u32) ||
>> +		    phdr->p_filesz < elfseg->cfg_idx_offset +
>> +				     elfseg->cfg_size_in_items * sizeof(u8) ||
>> +		    phdr->p_filesz < elfseg->cfg_val_offset +
>> +				     elfseg->cfg_size_in_items * sizeof(u32))
>> +			continue;
> 
> Even if this isn't the firmware for the protocol you're looking for, I'd
> say that if you have determined that you've found a valid SE FW segment
> and any of these checks doesn't pass, you have a truncated/corrupt
> firmware file. I think a error print is warranted here.
> 
> 
> Looks like you can squeeze these lines in under 100 characters if you
> remove the line wrap - that will make this chunk easier to read, so
> please unwrap them.
> 
> 
> Stylistic, I prefer the variable to be on the left of the comparator and
> the constant to the right; i.e. "elfseg->fw_offset + ... > phdr->p_filesz"
> 
>> +
>> +		if (elfseg->serial_protocol != se->protocol)
>> +			continue;
> 
> The key part of this loop is that you're looking for a SE firmware or
> the right protocol.
> 
> The fact that the segment must be big enough etc is a secondary sanity
> check to ensure you're not consuming bad firmware.
> 
>> +
>> +		*pelfseg = (struct se_fw_hdr *)addr;
> 
> *pelfseg = elfseg;
> 
> Although, this function only has two "return paths", either it return
> -EINVAL or it return 0 with *pelfseg assigned.
> 
> So, rather than using a pointer argument to return the result, make the
> return value const struct *se_fw_hdr and return elfseg here.

Updated in v6.

> 
>> +		return 0;
>> +	}
>> +	return -EINVAL;
>> +}
>> +
>> +/**
>> + * geni_configure_xfer_mode() - Set the transfer mode.
>> + * @se: Pointer to a structure representing SE-related resources.
>> + *
>> + * Set the transfer mode to either FIFO or DMA according to the mode specified by the protocol
>> + * driver.
> 
> This can be wrapped to 80 characters without loss of readability.

Updated in v6.

> 
>> + *
>> + * Return: 0 if successful, otherwise return an error value.
>> + */
>> +static int geni_configure_xfer_mode(struct geni_se *se)
>> +{
>> +	/* Configure SE FIFO, DMA or GSI mode. */
>> +	switch (se->mode) {
>> +	case GENI_GPI_DMA:
>> +		geni_setbits32(se->base + SE_GENI_DMA_MODE_EN, GENI_DMA_MODE_EN);
>> +		writel_relaxed(0x0, se->base + SE_IRQ_EN);
>> +		writel_relaxed(DMA_RX_EVENT_EN | DMA_TX_EVENT_EN |
>> +			       GENI_M_EVENT_EN | GENI_S_EVENT_EN,
>> +			       se->base + SE_GSI_EVENT_EN);
>> +		break;
>> +
>> +	case GENI_SE_FIFO:
>> +		geni_clrbits32(se->base + SE_GENI_DMA_MODE_EN, GENI_DMA_MODE_EN);
>> +		writel_relaxed(DMA_RX_IRQ_EN | DMA_TX_IRQ_EN | GENI_M_IRQ_EN | GENI_S_IRQ_EN,
>> +			       se->base + SE_IRQ_EN);
>> +		writel_relaxed(0x0, se->base + SE_GSI_EVENT_EN);
>> +		break;
>> +
>> +	case GENI_SE_DMA:
>> +		geni_setbits32(se->base + SE_GENI_DMA_MODE_EN, GENI_DMA_MODE_EN);
>> +		writel_relaxed(DMA_RX_IRQ_EN | DMA_TX_IRQ_EN | GENI_M_IRQ_EN | GENI_S_IRQ_EN,
>> +			       se->base + SE_IRQ_EN);
>> +		writel_relaxed(0x0, se->base + SE_GSI_EVENT_EN);
>> +		break;
>> +
>> +	default:
>> +		dev_err(se->dev, "Invalid geni-se transfer mode: %d\n", se->mode);
>> +		return -EINVAL;
>> +	}
>> +	return 0;
>> +}
>> +
>> +/**
>> + * geni_enable_interrupts() - Enable interrupts.
>> + * @se: Pointer to a structure representing SE-related resources.
>> + *
>> + * Enable the required interrupts during the firmware load process.
>> + *
>> + */
>> +static void geni_enable_interrupts(struct geni_se *se)
>> +{
>> +	u32 reg_value;
> 
> "val" has the same amount of information, but is shorter and easier to
> read...

Updated in v6.

> 
>> +
>> +	/* Enable required interrupts. */
>> +	writel_relaxed(M_COMMON_GENI_M_IRQ_EN, se->base + SE_GENI_M_IRQ_EN);
> 
> Don't you need/want a wmb() before this (or use one writel()), to ensure
> previous programming is performed before you enable IRQs?

Updated in v6.

> 
>> +
>> +	reg_value = S_CMD_OVERRUN_EN | S_ILLEGAL_CMD_EN |
>> +		    S_CMD_CANCEL_EN | S_CMD_ABORT_EN |
>> +		    S_GP_IRQ_0_EN | S_GP_IRQ_1_EN |
>> +		    S_GP_IRQ_2_EN | S_GP_IRQ_3_EN |
>> +		    S_RX_FIFO_WR_ERR_EN | S_RX_FIFO_RD_ERR_EN;
>> +	writel_relaxed(reg_value, se->base + SE_GENI_S_IRQ_ENABLE);
>> +
>> +	/* DMA mode configuration. */
>> +	reg_value = RESET_DONE_EN | SBE_EN | DMA_DONE_EN;
>> +	writel_relaxed(reg_value, se->base + SE_DMA_TX_IRQ_EN_SET);
>> +	reg_value = FLUSH_DONE_EN | RESET_DONE_EN | SBE_EN | DMA_DONE_EN;
>> +	writel_relaxed(reg_value, se->base + SE_DMA_RX_IRQ_EN_SET);
>> +}
>> +
>> +/**
>> + * geni_write_fw_revision() - Write the firmware revision.
>> + * @se: Pointer to a structure representing SE-related resources.
>> + * @serial_protocol: serial protocol type.
>> + * @fw_version: QUP firmware version.
>> + *
>> + * Write the firmware revision and protocol into the respective register.
>> + *
>> + * Return: None.
>> + */
>> +static void geni_write_fw_revision(struct geni_se *se, u16 serial_protocol, u16 fw_version)
>> +{
>> +	u32 reg_value;
>> +
>> +	reg_value = FIELD_PREP(FW_REV_PROTOCOL_MSK, serial_protocol);
>> +	reg_value |= FIELD_PREP(FW_REV_VERSION_MSK, fw_version);
>> +
>> +	writel_relaxed(reg_value, se->base + SE_GENI_FW_REVISION);
>> +	writel_relaxed(reg_value, se->base + SE_S_FW_REVISION);
>> +}
>> +
>> +/**
>> + * geni_load_se_fw() - Load Serial Engine specific firmware.
>> + * @se: Pointer to a structure representing SE-related resources.
>> + * @fw: Pointer to the firmware structure.
>> + *
>> + * Load the protocol firmware into the IRAM of the Serial Engine.
>> + *
>> + * Return: 0 if successful, otherwise return an error value.
>> + */
>> +static int geni_load_se_fw(struct geni_se *se, const struct firmware *fw)
>> +{
>> +	const u32 *fw_data, *cfg_val_arr;
>> +	const u8 *cfg_idx_arr;
>> +	u32 i, reg_value, ramn_cnt;
>> +	int ret;
>> +	struct se_fw_hdr *hdr;
>> +
>> +	ret = geni_read_elf(se, fw, &hdr);
>> +	if (ret) {
>> +		dev_err(se->dev, "ELF parsing failed ret: %d\n", ret);
> 
> If you move this error print to geni_read_elf() you can make it more
> useful - i.e. you can distinguish between not finding a sefw or the file
> being truncated etc.

Updated in v6.

> 
>> +		return ret;
>> +	}
>> +
>> +	ramn_cnt = hdr->fw_size_in_items;
>> +	if (hdr->fw_size_in_items % 2 != 0)
>> +		ramn_cnt++;
>> +
>> +	if (ramn_cnt >= MAX_GENI_CFG_RAMn_CNT)
> 
> I presume it's unlikely that there will be an odd number of
> fw_size_in_items, that is less than MAX_GENI_CFG_RAMn_CNT, but the
> validation in geni_read_elf() only accounted for that you will access
> fw_size_in_items number of items from fw->data; not the rounded up
> value.
> 
> Also, it would probably be helpful to the user if you provided an error
> message about corrupt firmware here.

Updated in v6.

> 
>> +		return -EINVAL;
>> +
>> +	ret = geni_icc_set_bw(se);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = geni_icc_enable(se);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = geni_se_resources_on(se);
>> +	if (ret)
>> +		goto err;
>> +
>> +	ramn_cnt = hdr->fw_size_in_items;
> 
> These adjustments and checks was performed 19 lines above as well, but
> before the hardware was spun up.

Removed duplicate check and updated in v6.

> 
>> +	if (hdr->fw_size_in_items % 2 != 0)
>> +		ramn_cnt++;
>> +
>> +	if (ramn_cnt >= MAX_GENI_CFG_RAMn_CNT)
>> +		goto err_resource;
>> +
>> +	fw_data = (const u32 *)((u8 *)hdr + hdr->fw_offset);
>> +	cfg_idx_arr = (const u8 *)hdr + hdr->cfg_idx_offset;
>> +	cfg_val_arr = (const u32 *)((u8 *)hdr + hdr->cfg_val_offset);
>> +
>> +	/* Disable high priority interrupt until current low priority interrupts are handled. */
>> +	geni_setbits32(se->wrapper->base + QUPV3_COMMON_CFG, FAST_SWITCH_TO_HIGH_DISABLE);
>> +
>> +	/* Set AHB_M_CLK_CGC_ON to indicate hardware controls se-wrapper cgc clock. */
>> +	geni_setbits32(se->wrapper->base + QUPV3_SE_AHB_M_CFG, AHB_M_CLK_CGC_ON);
>> +
>> +	/* Let hardware to control common cgc. */
>> +	geni_setbits32(se->wrapper->base + QUPV3_COMMON_CGC_CTRL, COMMON_CSR_SLV_CLK_CGC_ON);
>> +
>> +	/* Allows to drive corresponding data according to hardware value. */
> 
> What does this sentence mean?

Enhanced comment and updated in v6.

> 
>> +	writel_relaxed(0x0, se->base + GENI_OUTPUT_CTRL);
>> +
>> +	/* Set SCLK and HCLK to program RAM */
>> +	geni_setbits32(se->base + SE_GENI_CGC_CTRL, PROG_RAM_SCLK_OFF | PROG_RAM_HCLK_OFF);
>> +	writel_relaxed(0x0, se->base + SE_GENI_CLK_CTRL);
>> +	geni_clrbits32(se->base + SE_GENI_CGC_CTRL, PROG_RAM_SCLK_OFF | PROG_RAM_HCLK_OFF);
>> +
>> +	/* Enable required clocks for DMA CSR, TX and RX. */
>> +	reg_value = AHB_SEC_SLV_CLK_CGC_ON | DMA_AHB_SLV_CLK_CGC_ON |
>> +		    DMA_TX_CLK_CGC_ON | DMA_RX_CLK_CGC_ON;
>> +	geni_setbits32(se->base + SE_DMA_GENERAL_CFG, reg_value);
>> +
>> +	/* Let hardware control CGC by default. */
>> +	writel_relaxed(DEFAULT_CGC_EN, se->base + SE_GENI_CGC_CTRL);
>> +
>> +	/* Set version of the configuration register part of firmware. */
>> +	writel_relaxed(hdr->cfg_version, se->base + SE_GENI_INIT_CFG_REVISION);
>> +	writel_relaxed(hdr->cfg_version, se->base + SE_GENI_S_INIT_CFG_REVISION);
>> +
>> +	/* Configure GENI primitive table. */
>> +	for (i = 0; i < hdr->cfg_size_in_items; i++)
>> +		writel_relaxed(cfg_val_arr[i],
>> +			       se->base + SE_GENI_CFG_REG0 + (cfg_idx_arr[i] * sizeof(u32)));
>> +
>> +	/* Configure condition for assertion of RX_RFR_WATERMARK condition. */
>> +	reg_value = geni_se_get_rx_fifo_depth(se);
>> +	writel_relaxed(reg_value - 2, se->base + SE_GENI_RX_RFR_WATERMARK_REG);
>> +
>> +	/* Let hardware control CGC */
>> +	geni_setbits32(se->base + GENI_OUTPUT_CTRL, DEFAULT_IO_OUTPUT_CTRL_MSK);
>> +
>> +	ret = geni_configure_xfer_mode(se);
>> +	if (ret)
>> +		goto err_resource;
>> +
>> +	geni_enable_interrupts(se);
>> +
>> +	geni_write_fw_revision(se, hdr->serial_protocol, hdr->fw_version);
>> +
>> +	ramn_cnt = hdr->fw_size_in_items;
> 
> hdr-.fw_size_in_items shouldn't have changed since you validated it 60
> and 79 lines above, right?

Removed duplicate check and updated in v6.

> 
>> +	if (hdr->fw_size_in_items % 2 != 0)
>> +		ramn_cnt++;
>> +
>> +	if (ramn_cnt >= MAX_GENI_CFG_RAMn_CNT)
>> +		goto err_resource;
> 
> I suspect that you're going to remove this check and return, but at this
> point you have enabled a bunch of hardware and you're exiting without
> cleaning up...

Moved this check to geni_find_protocol_fw and updated in v6.

> 
>> +
>> +	/* Program RAM address space. */
>> +	memcpy_toio(se->base + SE_GENI_CFG_RAMN, fw_data, ramn_cnt * sizeof(u32));
>> +
>> +	/* Put default values on GENI's output pads. */
>> +	writel_relaxed(0x1, se->base + GENI_FORCE_DEFAULT_REG);
>> +
>> +	/* High to low SCLK and HCLK to finish RAM. */
> 
> "finish RAM <what>"?

Enhanced comment and updated in v6.

> 
>> +	geni_setbits32(se->base + SE_GENI_CGC_CTRL, PROG_RAM_SCLK_OFF | PROG_RAM_HCLK_OFF);
>> +	geni_setbits32(se->base + SE_GENI_CLK_CTRL, SER_CLK_SEL);
>> +	geni_clrbits32(se->base + SE_GENI_CGC_CTRL, PROG_RAM_SCLK_OFF | PROG_RAM_HCLK_OFF);
> 
> All these IO accesses are relaxed, allowing the compiler and the CPU to
> reorder them. Isn't there a point here where the QUP will start running
> the newly loaded firmware - and you want to make sure that all the setup
> (and the firmware loading) isn't reordered past that point?
> 
> Or is the ordering of access to all these registers govern by something
> else?

Replaced writel_relaxed with writel in v6.

> 
>> +
>> +	/* Serial engine DMA interface is enabled. */
>> +	geni_setbits32(se->base + SE_DMA_IF_EN, DMA_IF_EN);
>> +
>> +	/* Enable or disable FIFO interface of the serial engine. */
>> +	if (se->mode == GENI_SE_FIFO)
>> +		geni_clrbits32(se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
>> +	else
>> +		geni_setbits32(se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
>> +
>> +err_resource:
> 
> Rename this out_resources_off

updated in v6.

> 
>> +	geni_se_resources_off(se);
>> +err:
> 
> And this out_icc_disable
> 
> That way one can understand how the exit will look like without jumping
> here
updated in v6.

> 
>> +	geni_icc_disable(se);
>> +	return ret;
>> +}
>> +
>> +/**
>> + * qup_fw_load() - Initiate firmware load.
>> + * @se: Pointer to a structure representing SE-related resources.
>> + * @fw_name: Name of the firmware.
>> + *
>> + * Load the firmware into a specific SE. Read the associated ELF file,
>> + * copy the data into a buffer in kernel space using the request_firmware API, write the
>> + * data into the SE's IRAM register, and then free the buffers. Handle firmware loading
>> + * and parsing for a specific protocol.
>> + *
> 
> Wrap all your kernel-doc bodies to 80 characters, please.

updated in v6.

> 
>> + * Return: 0 if successful, otherwise return an error value.
>> + */
>> +static int qup_fw_load(struct geni_se *se, const char *fw_name)
> 
> All other functions in this driver are prefixed "geni_". Please keep the
> naming consistent.
> 
> That said, moving the content of this function into
> geni_load_se_firmware() wouldn't worsen that function much and you
> remove a dummy intermediate function. So, please inline this.

updated in v6.

> 
>> +{
>> +	int ret;
>> +	const struct firmware *fw;
>> +	struct device *dev = se->dev;
>> +
>> +	ret = request_firmware(&fw, fw_name, dev);
>> +	if (ret) {
>> +		dev_err(dev, "request_firmware failed for %d: %d\n", se->protocol, ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = geni_load_se_fw(se, fw);
>> +
>> +	release_firmware(fw);
>> +
>> +	return ret;
>> +}
>> +
>> +/**
>> + * geni_load_se_firmware() - Initiate firmware loading.
> 
> Is this really "initiate firmware loading"? Isn't it actually completing
> the firmware loading? I.e. this comment should be "Load SE firmware" or
> such?

updated in v6.

> 
>> + * @se: Serial engine details.
>> + * @protocol: Protocol (SPI, I2C, or UART) for which the firmware is to be loaded.
>> + *
>> + * If the device tree properties are configured to load QUP firmware and the firmware
>> + * is not already loaded, start the firmware loading process. If the device tree properties
>> + * are not defined, skip loading the firmware, assuming it is already loaded by TZ.
> 
> This too talks about start firmware loading, but I don't see any code
> that would "finish" it.
> 
> The short form comment for qup_fw_load() above also says "Initiate", but
> the longer description indicates that it concludes the load.
> 
>> + *
>> + * Return: 0 if successful, otherwise return an error value.
>> + */
>> +int geni_load_se_firmware(struct geni_se *se, enum geni_se_protocol_type protocol)
>> +{
>> +	const char *fw_name;
>> +	int ret;
>> +
>> +	if (protocol >= MAX_PROTOCOL) {
> 
> The only place I can see where this is important is in below dev_dbg()
> print, to avoid reading outside protocol_name[]. You can make that
> clearer by comparing with ARRAY_SIZE(protocol_name) instead.

updated in v6.

> 
>> +		dev_err(se->dev, "Invalid geni-se protocol: %d", protocol);
>> +		return  -EINVAL;
> 
> There are two spaces between return and -EINVAL.

updated in v6.

> 
>> +	}
>> +
>> +	ret = device_property_read_string(se->wrapper->dev, "firmware-name", &fw_name);
>> +	if (ret)
>> +		return  -EINVAL;
> 
> Two more spaces

updated in v6.

> 
>> +
>> +	se->protocol = protocol;
> 
> You assign se->protocol here, and it's read in geni_read_elf()...

updated in v6.

> 
>> +
>> +	if (of_property_read_bool(se->dev->of_node, "qcom,enable-gsi-dma"))
>> +		se->mode = GENI_GPI_DMA;
>> +	else
>> +		se->mode = GENI_SE_FIFO;
> 
> ...and you assign se->mode here and it's read in geni_configure_xfer_mode()
> and geni_load_se_fw().
> 
> But the callstack looks like:
>   geni_load_se_firmware()
>     qup_fw_load()
>       geni_load_se_fw()
>         geni_read_elf()
>         geni_configure_xfer_mode()
> 
> So, both protocol and mode are only used in the scope of this function.
> Passing them around using struct geni_se only obfuscates this fact.
> 
> Please pass them as arguments instead to make this clear.

updated in v6.

> 
>> +
>> +	/* GSI mode is not supported by the UART driver; therefore, setting FIFO mode */
>> +	if (protocol == GENI_SE_UART)
>> +		se->mode = GENI_SE_FIFO;
>> +
>> +	ret = qup_fw_load(se, fw_name);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev_dbg(se->dev, "Firmware load for %s protocol is successful for xfer mode %d\n",
>> +		protocol_name[se->protocol], se->mode);
> 
> Use the protocol and mode from the stack.

updated in v6.

> 
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(geni_load_se_firmware);
>> +
>>  static int geni_se_probe(struct platform_device *pdev)
>>  {
>>  	struct device *dev = &pdev->dev;
>> diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
>> index 2996a3c28ef3..6b75171d65ec 100644
>> --- a/include/linux/soc/qcom/geni-se.h
>> +++ b/include/linux/soc/qcom/geni-se.h
>> @@ -1,11 +1,13 @@
>>  /* SPDX-License-Identifier: GPL-2.0 */
>>  /*
>>   * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
>>   */
>>  
>>  #ifndef _LINUX_QCOM_GENI_SE
>>  #define _LINUX_QCOM_GENI_SE
>>  
>> +#include <linux/bitfield.h>
>>  #include <linux/interconnect.h>
>>  
>>  /**
>> @@ -36,6 +38,7 @@ enum geni_se_protocol_type {
>>  	GENI_SE_I2C,
>>  	GENI_SE_I3C,
>>  	GENI_SE_SPI_SLAVE,
>> +	GENI_SE_INVALID_PROTO = 255,
>>  };
>>  
>>  struct geni_wrapper;
>> @@ -61,6 +64,8 @@ struct geni_icc_path {
>>   * @num_clk_levels:	Number of valid clock levels in clk_perf_tbl
>>   * @clk_perf_tbl:	Table of clock frequency input to serial engine clock
>>   * @icc_paths:		Array of ICC paths for SE
>> + * @mode:		Transfer mode se fifo, dma or gsi.
>> + * @protocol:		Protocol spi or i2c or serial.
>>   */
>>  struct geni_se {
>>  	void __iomem *base;
>> @@ -70,24 +75,32 @@ struct geni_se {
>>  	unsigned int num_clk_levels;
>>  	unsigned long *clk_perf_tbl;
>>  	struct geni_icc_path icc_paths[3];
>> +	enum geni_se_xfer_mode mode;
>> +	enum geni_se_protocol_type protocol;
>>  };
>>  
>>  /* Common SE registers */
>> +#define SE_GENI_INIT_CFG_REVISION	0x0
>> +#define SE_GENI_S_INIT_CFG_REVISION	0x4
>>  #define GENI_FORCE_DEFAULT_REG		0x20
>>  #define GENI_OUTPUT_CTRL		0x24
>> +#define SE_GENI_CGC_CTRL		0x28
>>  #define SE_GENI_STATUS			0x40
>>  #define GENI_SER_M_CLK_CFG		0x48
>>  #define GENI_SER_S_CLK_CFG		0x4c
>>  #define GENI_IF_DISABLE_RO		0x64
>> -#define GENI_FW_REVISION_RO		0x68
>> +#define SE_GENI_FW_REVISION_RO		0x68
>> +#define SE_GENI_S_FW_REVISION_RO	0x6c
>>  #define SE_GENI_CLK_SEL			0x7c
>>  #define SE_GENI_CFG_SEQ_START		0x84
>> +#define SE_GENI_CFG_REG0		0x100
>>  #define SE_GENI_DMA_MODE_EN		0x258
>>  #define SE_GENI_M_CMD0			0x600
>>  #define SE_GENI_M_CMD_CTRL_REG		0x604
>>  #define SE_GENI_M_IRQ_STATUS		0x610
>>  #define SE_GENI_M_IRQ_EN		0x614
>>  #define SE_GENI_M_IRQ_CLEAR		0x618
>> +#define SE_GENI_S_IRQ_ENABLE		0x644
>>  #define SE_GENI_M_IRQ_EN_SET		0x61c
>>  #define SE_GENI_M_IRQ_EN_CLEAR		0x620
>>  #define SE_GENI_S_CMD0			0x630
>> @@ -109,13 +122,22 @@ struct geni_se {
>>  #define SE_GENI_S_GP_LENGTH		0x914
>>  #define SE_DMA_TX_IRQ_STAT		0xc40
>>  #define SE_DMA_TX_IRQ_CLR		0xc44
>> +#define SE_DMA_TX_IRQ_EN_SET		0xc4c
>>  #define SE_DMA_TX_FSM_RST		0xc58
>>  #define SE_DMA_RX_IRQ_STAT		0xd40
>>  #define SE_DMA_RX_IRQ_CLR		0xd44
>> +#define SE_DMA_RX_IRQ_EN_SET		0xd4c
>>  #define SE_DMA_RX_LEN_IN		0xd54
>>  #define SE_DMA_RX_FSM_RST		0xd58
>>  #define SE_HW_PARAM_0			0xe24
>>  #define SE_HW_PARAM_1			0xe28
>> +#define SE_DMA_GENERAL_CFG		0xe30
>> +#define SE_GENI_FW_REVISION		0x1000
>> +#define SE_S_FW_REVISION		0x1004
>> +#define SE_GENI_CFG_RAMN		0x1010
>> +#define SE_GENI_CLK_CTRL		0x2000
>> +#define SE_DMA_IF_EN			0x2004
>> +#define SE_FIFO_IF_DISABLE		0x2008
>>  
>>  /* GENI_FORCE_DEFAULT_REG fields */
>>  #define FORCE_DEFAULT	BIT(0)
>> @@ -137,7 +159,7 @@ struct geni_se {
>>  
>>  /* GENI_FW_REVISION_RO fields */
>>  #define FW_REV_PROTOCOL_MSK		GENMASK(15, 8)
>> -#define FW_REV_PROTOCOL_SHFT		8
>> +#define FW_REV_VERSION_MSK		GENMASK(7, 0)
>>  
>>  /* GENI_CLK_SEL fields */
>>  #define CLK_SEL_MSK			GENMASK(2, 0)
>> @@ -325,9 +347,9 @@ static inline u32 geni_se_read_proto(struct geni_se *se)
> 
> It is assumed that this function will return an enum
> geni_se_protocol_type.
> 
> I think you should follow up with a patch to not throw this around as a
> u32.
> 
>>  {
>>  	u32 val;
>>  
>> -	val = readl_relaxed(se->base + GENI_FW_REVISION_RO);
>> +	val = readl_relaxed(se->base + SE_GENI_FW_REVISION_RO);
>>  
>> -	return (val & FW_REV_PROTOCOL_MSK) >> FW_REV_PROTOCOL_SHFT;
>> +	return FIELD_GET(FW_REV_PROTOCOL_MSK, val);
> 
> Nice, but unrelated to firmware loading.

updated in v6.

> 
>>  }
>>  
>>  /**
>> @@ -531,5 +553,7 @@ void geni_icc_set_tag(struct geni_se *se, u32 tag);
>>  int geni_icc_enable(struct geni_se *se);
>>  
>>  int geni_icc_disable(struct geni_se *se);
>> +
>> +int geni_load_se_firmware(struct geni_se *se, enum geni_se_protocol_type protocol);
>>  #endif
>>  #endif
>> diff --git a/include/linux/soc/qcom/qup-fw-load.h b/include/linux/soc/qcom/qup-fw-load.h
> 
> As this is only going to be used from qcom-geni-se.c, I think you should
> just put this content in that file directly.

updated in v6.

> 
>> new file mode 100644
>> index 000000000000..94a6c4c10bcf
>> --- /dev/null
>> +++ b/include/linux/soc/qcom/qup-fw-load.h
>> @@ -0,0 +1,93 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +#ifndef _LINUX_QCOM_QUP_FW_LOAD
>> +#define _LINUX_QCOM_QUP_FW_LOAD
>> +
>> +#include <linux/kernel.h>
>> +
>> +/*Magic numbers*/
>> +#define MAGIC_NUM_SE			0x57464553
> 
> Please make "SE" the prefix, rather than suffix.

updated in v6.

> 
>> +
>> +#define MAX_GENI_CFG_RAMn_CNT		455
>> +
>> +#define MI_PBT_NON_PAGED_SEGMENT	0x0
>> +#define MI_PBT_HASH_SEGMENT		0x2
>> +#define MI_PBT_NOTUSED_SEGMENT		0x3
>> +#define MI_PBT_SHARED_SEGMENT		0x4
>> +
>> +#define MI_PBT_FLAG_PAGE_MODE		BIT(20)
>> +#define MI_PBT_FLAG_SEGMENT_TYPE	GENMASK(26, 24)
>> +#define MI_PBT_FLAG_ACCESS_TYPE		GENMASK(23, 21)
>> +
>> +#define MI_PBT_PAGE_MODE_VALUE(x) FIELD_GET(MI_PBT_FLAG_PAGE_MODE, x)
>> +
>> +#define MI_PBT_SEGMENT_TYPE_VALUE(x) FIELD_GET(MI_PBT_FLAG_SEGMENT_TYPE, x)
>> +
>> +#define MI_PBT_ACCESS_TYPE_VALUE(x) FIELD_GET(MI_PBT_FLAG_ACCESS_TYPE, x)
>> +
>> +#define M_COMMON_GENI_M_IRQ_EN	(GENMASK(6, 1) | \
>> +				M_IO_DATA_DEASSERT_EN | \
> 
> These constants are defined in geni-se.h, so if someone includes this
> file before that one, this won't compile.

updated in v6.

> 
>> +				M_IO_DATA_ASSERT_EN | M_RX_FIFO_RD_ERR_EN | \
>> +				M_RX_FIFO_WR_ERR_EN | M_TX_FIFO_RD_ERR_EN | \
>> +				M_TX_FIFO_WR_ERR_EN)
>> +
>> +/* DMA_TX/RX_IRQ_EN fields */
>> +#define DMA_DONE_EN		BIT(0)
> 
> There's already a GENI_SE_DMA_DONE_EN define in qcom-geni-se.c, used for
> the SE_DMA_TX_IRQ_EN_SET register.

updated in v6.

> 
>> +#define SBE_EN			BIT(2)
> 
> And this bit is today called GENI_SE_DMA_AHB_ERR_EN. Did the name
> change, does different hardware have different functionality for this
> one bit? If so, how would one know that geni_se_tx_init_dma() and
> geni_enable_interrupts() uses incompatible bit definitions?
> 
> This shows why it's important that you don't mix renames and functional
> additions!

updated in v6.

> 
>> +#define RESET_DONE_EN		BIT(3)
>> +#define FLUSH_DONE_EN		BIT(4)
>> +
>> +/* GENI_CLK_CTRL fields */
>> +#define SER_CLK_SEL		BIT(0)
>> +
>> +/* GENI_DMA_IF_EN fields */
>> +#define DMA_IF_EN		BIT(0)
>> +
>> +#define QUPV3_COMMON_CFG		0x120
> 
> Group this with QUP_HW_VER_REG, to indicate that it belongs to that
> register space and not the SE space.

updated in v6.

> 
>> +#define FAST_SWITCH_TO_HIGH_DISABLE	BIT(0)
>> +
>> +#define QUPV3_SE_AHB_M_CFG		0x118
>> +#define AHB_M_CLK_CGC_ON		BIT(0)
>> +
>> +#define QUPV3_COMMON_CGC_CTRL		0x21C
>> +#define COMMON_CSR_SLV_CLK_CGC_ON	BIT(0)
>> +
>> +/* access ports */
> 
> There are no "ports"...

updated in v6.

> 
>> +#define geni_setbits32(_addr, _v) writel_relaxed(readl_relaxed(_addr) |  (_v), _addr)
>> +#define geni_clrbits32(_addr, _v) writel_relaxed(readl_relaxed(_addr) & ~(_v), _addr)
>> +
>> +/**
>> + * struct se_fw_hdr - Serial Engine firmware configuration header
>> + *
>> + * This structure defines metadata for the Serial Engine (SE) firmware
>> + * configuration. Although it is embedded within an ELF segment, it is
>> + * not part of the ELF format itself.
> 
> Perhaps:
> 
> """
> This structure defines the SE firmware header, which together with the
> firmware payload is stored in individual ELF segments.

updated in v6.

> """
> 
>> + *
>> + * @magic: Set to 'SEFW'
>> + * @version: Structure version number
>> + * @core_version: QUPV3 hardware version
>> + * @serial_protocol: Encoded in GENI_FW_REVISION
>> + * @fw_version: Firmware version, from GENI_FW_REVISION
>> + * @cfg_version: Configuration version, from GENI_INIT_CFG_REVISION
>> + * @fw_size_in_items: Number of 32-bit words in GENI_FW_RAM
>> + * @fw_offset: Byte offset to GENI_FW_RAM array
>> + * @cfg_size_in_items: Number of GENI_FW_CFG index/value pairs
>> + * @cfg_idx_offset: Byte offset to GENI_FW_CFG index array
>> + * @cfg_val_offset: Byte offset to GENI_FW_CFG values array
>> + */
>> +struct se_fw_hdr {
>> +	__le32 magic;
> 
> Kudos for specifying the endianess of this member, but what about the
> rest? How come magic is little endian but the other members are
> magically native endian?
> 
> Preferable you define them all with proper endian annotation.

updated in v6.

> 
> Regards,
> Bjorn
> 
>> +	u32 version;
>> +	u32 core_version;
>> +	u16 serial_protocol;
>> +	u16 fw_version;
>> +	u16 cfg_version;
>> +	u16 fw_size_in_items;
>> +	u16 fw_offset;
>> +	u16 cfg_size_in_items;
>> +	u16 cfg_idx_offset;
>> +	u16 cfg_val_offset;
>> +};
>> +#endif /* _LINUX_QCOM_QUP_FW_LOAD */
>> -- 
>> 2.34.1
>>