[PATCH v3 0/9] Add support to load QUP SE firmware from

[PATCH v3 0/9] 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.
---
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 (9):
  dt-bindings: qcom: geni-se: Add 'firmware-name' property for firmware
    loading
  dt-bindings: qcom: se-common: Add QUP Peripheral-specific properties
    for I2C, SPI, and SERIAL bus
  dt-bindings: i2c: qcom,i2c-geni: document qcom,gsi-dma-allowed
  spi: dt-bindings: document qcom,gsi-dma-allowed
  dt-bindings: serial: document qcom,gsi-dma-allowed
  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      |   3 +
 .../serial/qcom,serial-geni-qcom.yaml         |   3 +
 .../bindings/soc/qcom/qcom,geni-se.yaml       |   5 +
 .../soc/qcom/qcom,se-common-props.yaml        |  26 ++
 .../bindings/spi/qcom,spi-geni-qcom.yaml      |   3 +
 drivers/i2c/busses/i2c-qcom-geni.c            |   8 +-
 drivers/soc/qcom/qcom-geni-se.c               | 423 ++++++++++++++++++
 drivers/spi/spi-geni-qcom.c                   |   6 +
 drivers/tty/serial/qcom_geni_serial.c         |   8 +-
 include/linux/soc/qcom/geni-se.h              |  18 +
 include/linux/soc/qcom/qup-fw-load.h          | 179 ++++++++
 11 files changed, 680 insertions(+), 2 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 v3 1/9] dt-bindings: qcom: geni-se: Add 'firmware-name' property for firmware loading

[Viken Dadhaniya]

Document the 'firmware-name' property in the device tree bindings to
support loading SE (Serial Engine) firmware from the protocol driver,
allowing for more flexible firmware management.

Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
---
v2 -> v3:

- Remove ref and add maxItem element.

v2 Link: https://lore.kernel.org/linux-arm-msm/20250124105309.295769-2-quic_vdadhani@quicinc.com/
---
---
 Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.yaml
index 7b031ef09669..54cd585f19e3 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.yaml
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.yaml
@@ -54,6 +54,10 @@ properties:
 
   dma-coherent: true
 
+  firmware-name:
+    maxItems: 1
+    description: Specify the name of the QUP firmware to load.
+
 required:
   - compatible
   - reg
@@ -135,6 +139,7 @@ examples:
             #address-cells = <2>;
             #size-cells = <2>;
             ranges;
+            firmware-name = "qcom/sa8775p/qupv3fw.elf";
 
             i2c0: i2c@a94000 {
                 compatible = "qcom,geni-i2c";
-- 
2.34.1

Re: [PATCH v3 1/9] dt-bindings: qcom: geni-se: Add 'firmware-name' property for firmware loading

[Krzysztof Kozlowski]

On Mon, Mar 03, 2025 at 06:13:41PM +0530, Viken Dadhaniya wrote:
> Document the 'firmware-name' property in the device tree bindings to
> support loading SE (Serial Engine) firmware from the protocol driver,
> allowing for more flexible firmware management.
> 
> Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
> ---
> v2 -> v3:
> 
> - Remove ref and add maxItem element.
> 
> v2 Link: https://lore.kernel.org/linux-arm-msm/20250124105309.295769-2-quic_vdadhani@quicinc.com/
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v3 2/9] 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,gsi-dma-allowed` to configure the Serial Engine (SE) for
QCOM GPI DMA mode.

Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
---
 .../soc/qcom/qcom,se-common-props.yaml        | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,se-common-props.yaml

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..a111e51bb1c4
--- /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 <quic_msavaliy@quicinc.com>
+  - Viken Dadhaniya <quic_vdadhani@quicinc.com>
+
+properties:
+  qcom,gsi-dma-allowed:
+    $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
-- 
2.34.1

Re: [PATCH v3 2/9] dt-bindings: qcom: se-common: Add QUP Peripheral-specific properties for I2C, SPI, and SERIAL bus

[Krzysztof Kozlowski]

On Mon, Mar 03, 2025 at 06:13:42PM +0530, Viken Dadhaniya wrote:
> Introduce a new YAML schema for QUP-supported peripherals. Define common
> properties used across QUP-supported peripherals.
> 
> Add property `qcom,gsi-dma-allowed` to configure the Serial Engine (SE) for
> QCOM GPI DMA mode.
> 
> Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
> ---
>  .../soc/qcom/qcom,se-common-props.yaml        | 26 +++++++++++++++++++
>  1 file changed, 26 insertions(+)

You miss here any user of this.

>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,se-common-props.yaml
> 
> 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..a111e51bb1c4
> --- /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.

Drop full stop. Titles never have them.

> +
> +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 <quic_msavaliy@quicinc.com>
> +  - Viken Dadhaniya <quic_vdadhani@quicinc.com>
> +
> +properties:
> +  qcom,gsi-dma-allowed:

Based on description: qcom,enable-gsi-dma
(or description is not accurate)

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

Best regards,
Krzysztof

Re: [PATCH v3 2/9] dt-bindings: qcom: se-common: Add QUP Peripheral-specific properties for I2C, SPI, and SERIAL bus

[Krzysztof Kozlowski]

On 04/03/2025 09:10, Krzysztof Kozlowski wrote:
> On Mon, Mar 03, 2025 at 06:13:42PM +0530, Viken Dadhaniya wrote:
>> Introduce a new YAML schema for QUP-supported peripherals. Define common
>> properties used across QUP-supported peripherals.
>>
>> Add property `qcom,gsi-dma-allowed` to configure the Serial Engine (SE) for
>> QCOM GPI DMA mode.
>>
>> Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
>> ---
>>  .../soc/qcom/qcom,se-common-props.yaml        | 26 +++++++++++++++++++
>>  1 file changed, 26 insertions(+)
> 
> You miss here any user of this.

I see now you have users in other patches, so it is fine.

Best regards,
Krzysztof

[PATCH v3 3/9] dt-bindings: i2c: qcom,i2c-geni: document qcom,gsi-dma-allowed

[Viken Dadhaniya]

Data transfer mode is fixed by TrustZone (TZ), which currently restricts
developers from modifying the transfer mode from the APPS side.

Document the 'qcom,gsi-dma-allowed' property to select the data transfer
mode to GPI DMA (Generic Packet Interface). If not set, FIFO mode
(PIO/CPU DMA) will be selected by default.

Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
---
v2 -> v3:

- Drop the 'qcom,xfer-mode' property and add the 'qcom,gsi-dma-allowed' property.
- Add a reference for the QUP peripheral shared YAML.
- Update commit log.

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

v1 -> v2:

- Drop 'qcom,load-firmware' property and add 'firmware-name' property in
  qup common driver.
- Update commit log.

v1 Link: https://lore.kernel.org/linux-kernel/20241204150326.1470749-2-quic_vdadhani@quicinc.com/
---
---
 Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
index 9f66a3bb1f80..8c2675c65455 100644
--- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
+++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
@@ -60,6 +60,8 @@ properties:
   power-domains:
     maxItems: 1
 
+  qcom,gsi-dma-allowed: true
+
   reg:
     maxItems: 1
 
@@ -75,6 +77,7 @@ required:
 
 allOf:
   - $ref: /schemas/i2c/i2c-controller.yaml#
+  - $ref: /schemas/soc/qcom/qcom,se-common-props.yaml#
   - if:
       properties:
         compatible:
-- 
2.34.1

Re: [PATCH v3 3/9] dt-bindings: i2c: qcom,i2c-geni: document qcom,gsi-dma-allowed

[Krzysztof Kozlowski]

On Mon, Mar 03, 2025 at 06:13:43PM +0530, Viken Dadhaniya wrote:
> Data transfer mode is fixed by TrustZone (TZ), which currently restricts
> developers from modifying the transfer mode from the APPS side.
> 
> Document the 'qcom,gsi-dma-allowed' property to select the data transfer
> mode to GPI DMA (Generic Packet Interface). If not set, FIFO mode
> (PIO/CPU DMA) will be selected by default.
> 
> Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
> ---
> v2 -> v3:
> 

To be clear: this cannot be merged via i2c because of hidden dependency.

In the future: please clearly explain the dependencies so maintainers
know what to do with this.

> - Drop the 'qcom,xfer-mode' property and add the 'qcom,gsi-dma-allowed' property.
> - Add a reference for the QUP peripheral shared YAML.
> - Update commit log.
> 
> v2 Link: https://lore.kernel.org/linux-arm-msm/20250124105309.295769-3-quic_vdadhani@quicinc.com/
> 
> v1 -> v2:
> 
> - Drop 'qcom,load-firmware' property and add 'firmware-name' property in
>   qup common driver.
> - Update commit log.
> 
> v1 Link: https://lore.kernel.org/linux-kernel/20241204150326.1470749-2-quic_vdadhani@quicinc.com/
> ---
> ---
>  Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> index 9f66a3bb1f80..8c2675c65455 100644
> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> @@ -60,6 +60,8 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  qcom,gsi-dma-allowed: true

Drop

Best regards,
Krzysztof

[PATCH v3 4/9] spi: dt-bindings: document qcom,gsi-dma-allowed

[Viken Dadhaniya]

Data transfer mode is fixed by TrustZone (TZ), which currently restricts
developers from modifying the transfer mode from the APPS side.

Document the 'qcom,gsi-dma-allowed' property to select the data transfer
mode to GPI DMA (Generic Packet Interface). If not set, FIFO mode
(PIO/CPU DMA) will be selected by default.

Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
---
v2 -> v3:

- Drop the 'qcom,xfer-mode' property and add the 'qcom,gsi-dma-allowed' property.
- Add a reference for the QUP peripheral shared YAML.
- Update commit log.

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

v1 -> v2:

- Drop 'qcom,load-firmware' property and add 'firmware-name' property in
  qup common driver.
- Update commit log.

v1 Link: https://lore.kernel.org/linux-kernel/20241204150326.1470749-3-quic_vdadhani@quicinc.com/
---
---
 Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml b/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml
index 2e20ca313ec1..012cb7aecb3d 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:
@@ -63,6 +64,8 @@ properties:
   power-domains:
     maxItems: 1
 
+  qcom,gsi-dma-allowed: true
+
   reg:
     maxItems: 1
 
-- 
2.34.1

Re: [PATCH v3 4/9] spi: dt-bindings: document qcom,gsi-dma-allowed

[Krzysztof Kozlowski]

On Mon, Mar 03, 2025 at 06:13:44PM +0530, Viken Dadhaniya wrote:
> Data transfer mode is fixed by TrustZone (TZ), which currently restricts
> developers from modifying the transfer mode from the APPS side.
> 
> Document the 'qcom,gsi-dma-allowed' property to select the data transfer
> mode to GPI DMA (Generic Packet Interface). If not set, FIFO mode
> (PIO/CPU DMA) will be selected by default.
> 
> Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
> ---
> v2 -> v3:
> 

Missing dependency explanation.

> - Drop the 'qcom,xfer-mode' property and add the 'qcom,gsi-dma-allowed' property.
> - Add a reference for the QUP peripheral shared YAML.
> - Update commit log.
> 
> v2 Link: https://lore.kernel.org/linux-arm-msm/20250124105309.295769-4-quic_vdadhani@quicinc.com/
> 
> v1 -> v2:
> 
> - Drop 'qcom,load-firmware' property and add 'firmware-name' property in
>   qup common driver.
> - Update commit log.
> 
> v1 Link: https://lore.kernel.org/linux-kernel/20241204150326.1470749-3-quic_vdadhani@quicinc.com/
> ---
> ---
>  Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml b/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml
> index 2e20ca313ec1..012cb7aecb3d 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:
> @@ -63,6 +64,8 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  qcom,gsi-dma-allowed: true

Other comments also apply here probably.

Best regards,
Krzysztof

[PATCH v3 5/9] dt-bindings: serial: document qcom,gsi-dma-allowed

[Viken Dadhaniya]

Data transfer mode is fixed by TrustZone (TZ), which currently restricts
developers from modifying the transfer mode from the APPS side.

Document the 'qcom,gsi-dma-allowed' property to select the data transfer
mode to GPI DMA (Generic Packet Interface). If not set, FIFO mode
(PIO/CPU DMA) will be selected by default.

Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
---
v2 -> v3:

- Drop the 'qcom,xfer-mode' property and add the 'qcom,gsi-dma-allowed' property.
- Add a reference for the QUP peripheral shared YAML.
- Update commit log.

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

v1 -> v2:

- Drop 'qcom,load-firmware' property and add 'firmware-name' property in
  qup common driver.
- Update commit log.

v1 Link: https://lore.kernel.org/linux-kernel/20241204150326.1470749-4-quic_vdadhani@quicinc.com/
---
---
 .../devicetree/bindings/serial/qcom,serial-geni-qcom.yaml      | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
index dd33794b3534..254fab9f2341 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:
@@ -53,6 +54,8 @@ properties:
   power-domains:
     maxItems: 1
 
+  qcom,gsi-dma-allowed: true
+
   reg:
     maxItems: 1
 
-- 
2.34.1

Re: [PATCH v3 5/9] dt-bindings: serial: document qcom,gsi-dma-allowed

[Krzysztof Kozlowski]

On Mon, Mar 03, 2025 at 06:13:45PM +0530, Viken Dadhaniya wrote:
> Data transfer mode is fixed by TrustZone (TZ), which currently restricts
> developers from modifying the transfer mode from the APPS side.
> 
> Document the 'qcom,gsi-dma-allowed' property to select the data transfer
> mode to GPI DMA (Generic Packet Interface). If not set, FIFO mode
> (PIO/CPU DMA) will be selected by default.
> 
> Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
> ---
> v2 -> v3:

Missing dependency.

> 
> - Drop the 'qcom,xfer-mode' property and add the 'qcom,gsi-dma-allowed' property.
> - Add a reference for the QUP peripheral shared YAML.
> - Update commit log.
> 
> v2 Link: https://lore.kernel.org/linux-arm-msm/20250124105309.295769-5-quic_vdadhani@quicinc.com/
> 
> v1 -> v2:
> 
> - Drop 'qcom,load-firmware' property and add 'firmware-name' property in
>   qup common driver.
> - Update commit log.
> 
> v1 Link: https://lore.kernel.org/linux-kernel/20241204150326.1470749-4-quic_vdadhani@quicinc.com/
> ---
> ---
>  .../devicetree/bindings/serial/qcom,serial-geni-qcom.yaml      | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
> index dd33794b3534..254fab9f2341 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:
> @@ -53,6 +54,8 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  qcom,gsi-dma-allowed: true

And this also should be investigated, just like for I2C and SPI.

Best regards,
Krzysztof

[PATCH v3 6/9] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem

[Viken Dadhaniya]

Load the firmware to QUP SE based on the 'firmware-name' property specified
in devicetree. Populate Serial engine and base address details in the probe
function of the protocol driver and pass to firmware load routine.

Skip the firmware loading if the firmware is already loaded in Serial
Engine's firmware memory area.

Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Signed-off-by: Viken Dadhaniya <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      | 423 +++++++++++++++++++++++++++
 include/linux/soc/qcom/geni-se.h     |  18 ++
 include/linux/soc/qcom/qup-fw-load.h | 179 ++++++++++++
 3 files changed, 620 insertions(+)
 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..345181ad6fcc 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -1,5 +1,6 @@
 // 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.
 
 /* Disable MMIO tracing to prevent excessive logging of unwanted MMIO traces */
 #define __DISABLE_TRACE_MMIO__
@@ -15,6 +16,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
@@ -110,6 +112,9 @@ struct geni_se_desc {
 static const char * const icc_path_names[] = {"qup-core", "qup-config",
 						"qup-memory"};
 
+static const char * const protocol_name[] = { "None", "SPI", "UART",
+					      "I2C", "I3C", "SPI SLAVE"};
+
 #define QUP_HW_VER_REG			0x4
 
 /* Common SE registers */
@@ -891,6 +896,424 @@ int geni_icc_disable(struct geni_se *se)
 }
 EXPORT_SYMBOL_GPL(geni_icc_disable);
 
+/**
+ * elf_phdr_valid() - Validate an ELF header.
+ * @phdr: Pointer to the ELF header.
+ *
+ * Validate the ELF header by comparing the fields stored in p_flags and the payload type.
+ *
+ * Return: true if the validation is successful, false otherwise.
+ */
+static bool elf_phdr_valid(const struct elf32_phdr *phdr)
+{
+	if (phdr->p_type != PT_LOAD || !phdr->p_memsz)
+		return false;
+
+	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)
+		return true;
+
+	return false;
+}
+
+/**
+ * valid_seg_size() - Validate the segment size.
+ * @pelfseg: Pointer to the ELF header.
+ * @p_filesz: Pointer to the file size.
+ *
+ * Validate the ELF segment size by comparing the file size.
+ *
+ * Return: true if the segment is valid, false if the segment is invalid.
+ */
+static bool valid_seg_size(struct elf_se_hdr *pelfseg, Elf32_Word p_filesz)
+{
+	if (p_filesz >= pelfseg->fw_offset + pelfseg->fw_size_in_items * sizeof(u32) &&
+	    p_filesz >= pelfseg->cfg_idx_offset + pelfseg->cfg_size_in_items * sizeof(u8) &&
+	    p_filesz >= pelfseg->cfg_val_offset + pelfseg->cfg_size_in_items * sizeof(u32))
+		return true;
+	return false;
+}
+
+/**
+ * read_elf() - Read an ELF file.
+ * @rsc: Pointer to the SE resources structure.
+ * @fw: Pointer to the firmware buffer.
+ * @pelfseg: Pointer to the SE-specific ELF header.
+ * @phdr: Pointer to one of the valid headers from the list in the firmware buffer.
+ *
+ * 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 read_elf(struct qup_se_rsc *rsc, const struct firmware *fw,
+		    struct elf_se_hdr **pelfseg, struct elf32_phdr **phdr)
+{
+	const struct elf32_hdr *ehdr = (const struct elf32_hdr *)fw->data;
+	struct elf32_phdr *phdrs = (struct elf32_phdr *)(ehdr + 1);
+	const u8 *addr;
+	int i;
+
+	ehdr = (struct elf32_hdr *)fw->data;
+
+	if (ehdr->e_phnum < 2)
+		return -EINVAL;
+
+	for (i = 0; i < ehdr->e_phnum; i++) {
+		*phdr = &phdrs[i];
+		if (!elf_phdr_valid(*phdr))
+			continue;
+
+		if ((*phdr)->p_filesz >= sizeof(struct elf_se_hdr)) {
+			addr =  fw->data + (*phdr)->p_offset;
+			*pelfseg = (struct elf_se_hdr *)addr;
+
+			if ((*pelfseg)->magic == MAGIC_NUM_SE &&
+			    (*pelfseg)->version == 1 &&
+			    valid_seg_size(*pelfseg, (*phdr)->p_filesz) &&
+			    (*pelfseg)->serial_protocol == rsc->protocol &&
+			    (*pelfseg)->serial_protocol != GENI_SE_NONE)
+				return 0;
+		}
+	}
+	return -EINVAL;
+}
+
+/**
+ * geni_config_common_control() - Configure common CGC and disable high priority interrupt.
+ * @rsc: Pointer to a structure representing SE-related resources.
+ *
+ * Configure the common CGC and disable high priority interrupts until the current low priority
+ * interrupts are handled.
+ *
+ * Return: None.
+ */
+static void geni_config_common_control(struct qup_se_rsc *rsc)
+{
+	/*
+	 * Disable high priority interrupt until current
+	 * low priority interrupts are handled.
+	 */
+	setbits32(rsc->se->wrapper->base + QUPV3_COMMON_CFG,
+		  FAST_SWITCH_TO_HIGH_DISABLE_BMASK);
+
+	/*
+	 * Set AHB_M_CLK_CGC_ON to indicate hardware controls
+	 * se-wrapper cgc clock.
+	 */
+	setbits32(rsc->se->wrapper->base + QUPV3_SE_AHB_M_CFG,
+		  AHB_M_CLK_CGC_ON_BMASK);
+
+	/* Let hardware to control common cgc. */
+	setbits32(rsc->se->wrapper->base + QUPV3_COMMON_CGC_CTRL,
+		  COMMON_CSR_SLV_CLK_CGC_ON_BMASK);
+}
+
+/**
+ * geni_configure_xfer_mode() - Set the transfer mode.
+ * @rsc: 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 qup_se_rsc *rsc)
+{
+	/* Configure SE FIFO, DMA or GSI mode. */
+	switch (rsc->mode) {
+	case GENI_GPI_DMA:
+		setbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
+			  GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
+		writel_relaxed(0x0, rsc->se->base + SE_IRQ_EN);
+		writel_relaxed(SE_GSI_EVENT_EN_BMSK, rsc->se->base + SE_GSI_EVENT_EN);
+		break;
+
+	case GENI_SE_FIFO:
+		clrbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
+			  GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
+		writel_relaxed(SE_IRQ_EN_RMSK, rsc->se->base + SE_IRQ_EN);
+		writel_relaxed(0x0, rsc->se->base + SE_GSI_EVENT_EN);
+		break;
+
+	case GENI_SE_DMA:
+		setbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
+			  GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
+		writel_relaxed(SE_IRQ_EN_RMSK, rsc->se->base + SE_IRQ_EN);
+		writel_relaxed(0x0, rsc->se->base + SE_GSI_EVENT_EN);
+		break;
+
+	default:
+		dev_err(rsc->se->dev, "invalid se mode: %d\n", rsc->mode);
+		return -EINVAL;
+	}
+	return 0;
+}
+
+/**
+ * geni_enable_interrupts() Enable interrupts.
+ * @rsc: Pointer to a structure representing SE-related resources.
+ *
+ * Enable the required interrupts during the firmware load process.
+ *
+ * Return: None.
+ */
+static void geni_enable_interrupts(struct qup_se_rsc *rsc)
+{
+	u32 reg_value;
+
+	/* Enable required interrupts. */
+	writel_relaxed(M_COMMON_GENI_M_IRQ_EN, rsc->se->base + GENI_M_IRQ_ENABLE);
+
+	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, rsc->se->base + GENI_S_IRQ_ENABLE);
+
+	/* DMA mode configuration. */
+	reg_value = DMA_TX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK |
+		    DMA_TX_IRQ_EN_SET_SBE_EN_SET_BMSK |
+		    DMA_TX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK;
+	writel_relaxed(reg_value, rsc->se->base + DMA_TX_IRQ_EN_SET);
+	reg_value = DMA_RX_IRQ_EN_SET_FLUSH_DONE_EN_SET_BMSK |
+		    DMA_RX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK |
+		    DMA_RX_IRQ_EN_SET_SBE_EN_SET_BMSK |
+		    DMA_RX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK;
+	writel_relaxed(reg_value, rsc->se->base + DMA_RX_IRQ_EN_SET);
+}
+
+/**
+ * geni_flash_fw_revision() - Flash the firmware revision.
+ * @rsc: Pointer to a structure representing SE-related resources.
+ * @hdr: Pointer to the ELF header of the Serial Engine.
+ *
+ * Flash the firmware revision and protocol into the respective register.
+ *
+ * Return: None.
+ */
+static void geni_flash_fw_revision(struct qup_se_rsc *rsc, struct elf_se_hdr *hdr)
+{
+	u32 reg_value;
+
+	/* Flash firmware revision register. */
+	reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
+		    (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
+	writel_relaxed(reg_value, rsc->se->base + SE_GENI_FW_REVISION);
+
+	reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
+		    (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
+
+	writel_relaxed(reg_value, rsc->se->base + SE_S_FW_REVISION);
+}
+
+/**
+ * geni_load_se_fw() - Load Serial Engine specific firmware.
+ * @rsc: 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 qup_se_rsc *rsc, const struct firmware *fw)
+{
+	const u32 *fw_val_arr, *cfg_val_arr;
+	const u8 *cfg_idx_arr;
+	u32 i, reg_value, mask, ramn_cnt;
+	int ret;
+	struct elf_se_hdr *hdr;
+	struct elf32_phdr *phdr;
+
+	ret = geni_icc_set_bw(rsc->se);
+	if (ret) {
+		dev_err(rsc->se->dev, "%s: Failed to set ICC BW %d\n", __func__, ret);
+		return ret;
+	}
+
+	ret = geni_icc_enable(rsc->se);
+	if (ret) {
+		dev_err(rsc->se->dev, "%s: Failed to enable ICC %d\n", __func__, ret);
+		return ret;
+	}
+
+	ret = geni_se_resources_on(rsc->se);
+	if (ret) {
+		dev_err(rsc->se->dev, "%s: Failed to enable common clocks %d\n", __func__, ret);
+		goto err;
+	}
+
+	ret = read_elf(rsc, fw, &hdr, &phdr);
+	if (ret) {
+		dev_err(rsc->se->dev, "%s: ELF parsing failed ret: %d\n", __func__, ret);
+		goto err;
+	}
+
+	fw_val_arr = (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);
+
+	geni_config_common_control(rsc);
+
+	/* Allows to drive corresponding data according to hardware value. */
+	writel_relaxed(0x0, rsc->se->base + GENI_OUTPUT_CTRL);
+
+	/* Set SCLK and HCLK to program RAM */
+	setbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
+			GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
+	writel_relaxed(0x0, rsc->se->base + SE_GENI_CLK_CTRL);
+	clrbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
+			GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
+
+	/* Enable required clocks for DMA CSR, TX and RX. */
+	reg_value = DMA_GENERAL_CFG_AHB_SEC_SLV_CLK_CGC_ON_BMSK |
+		DMA_GENERAL_CFG_DMA_AHB_SLV_CLK_CGC_ON_BMSK |
+		DMA_GENERAL_CFG_DMA_TX_CLK_CGC_ON_BMSK |
+		DMA_GENERAL_CFG_DMA_RX_CLK_CGC_ON_BMSK;
+
+	setbits32(rsc->se->base + DMA_GENERAL_CFG, reg_value);
+
+	/* Let hardware control CGC by default. */
+	writel_relaxed(DEFAULT_CGC_EN, rsc->se->base + GENI_CGC_CTRL);
+
+	/* Set version of the configuration register part of firmware. */
+	writel_relaxed(hdr->cfg_version, rsc->se->base + GENI_INIT_CFG_REVISION);
+	writel_relaxed(hdr->cfg_version, rsc->se->base + 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],
+			       rsc->se->base + GENI_CFG_REG0 + (cfg_idx_arr[i] * sizeof(u32)));
+
+	/* Configure condition for assertion of RX_RFR_WATERMARK condition. */
+	reg_value = readl_relaxed(rsc->se->base + QUPV3_SE_HW_PARAM_1);
+	mask = (reg_value >> RX_FIFO_WIDTH_BIT) & RX_FIFO_WIDTH_MASK;
+	writel_relaxed(mask - 2, rsc->se->base + GENI_RX_RFR_WATERMARK_REG);
+
+	/* Let hardware control CGC */
+	setbits32(rsc->se->base + GENI_OUTPUT_CTRL, DEFAULT_IO_OUTPUT_CTRL_MSK);
+
+	ret = geni_configure_xfer_mode(rsc);
+	if (ret)
+		goto err_resource;
+
+	geni_enable_interrupts(rsc);
+
+	geni_flash_fw_revision(rsc, hdr);
+
+	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((void *)(rsc->se->base + SE_GENI_CFG_RAMN), fw_val_arr, ramn_cnt * sizeof(u32));
+
+	/* Put default values on GENI's output pads. */
+	writel_relaxed(0x1, rsc->se->base + GENI_FORCE_DEFAULT_REG);
+
+	/* High to low SCLK and HCLK to finish RAM. */
+	setbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
+			GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
+	setbits32(rsc->se->base + SE_GENI_CLK_CTRL, GENI_CLK_CTRL_SER_CLK_SEL_BMSK);
+	clrbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
+			GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
+
+	/* Serial engine DMA interface is enabled. */
+	setbits32(rsc->se->base + SE_DMA_IF_EN, DMA_IF_EN_DMA_IF_EN_BMSK);
+
+	/* Enable or disable FIFO interface of the serial engine. */
+	if (rsc->mode == GENI_SE_FIFO)
+		clrbits32(rsc->se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
+	else
+		setbits32(rsc->se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
+
+err_resource:
+	geni_se_resources_off(rsc->se);
+err:
+	geni_icc_disable(rsc->se);
+	return ret;
+}
+
+/**
+ * qup_fw_load() - Initiate firmware load.
+ * @rsc: Pointer to a structure representing SE-related resources.
+ *
+ * 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 qup_se_rsc *rsc, const char *fw_name)
+{
+	int ret;
+	const struct firmware *fw;
+	struct device *dev = rsc->se->dev;
+
+	ret = request_firmware(&fw, fw_name, dev);
+	if (ret) {
+		dev_err(dev, "request_firmware failed for %d: %d\n", rsc->protocol, ret);
+		return ret;
+	}
+
+	ret = geni_load_se_fw(rsc, 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)
+{
+	struct qup_se_rsc rsc;
+	const char *fw_name;
+	int ret;
+
+	ret = device_property_read_string(se->wrapper->dev, "firmware-name", &fw_name);
+	if (ret)
+		return  -EINVAL;
+
+	rsc.se = se;
+	rsc.protocol = protocol;
+	/* Set default xfer mode to FIFO */
+	rsc.mode = GENI_SE_FIFO;
+
+	if (of_property_read_bool(se->dev->of_node, "qcom,gsi-dma-allowed"))
+		rsc.mode = GENI_GPI_DMA;
+
+	/* GSI mode is not supported by the UART driver; therefore, setting FIFO mode */
+	if (protocol == GENI_SE_UART)
+		rsc.mode = GENI_SE_FIFO;
+
+	ret = qup_fw_load(&rsc, fw_name);
+	if (ret)
+		return ret;
+
+	dev_dbg(se->dev, "Firmware load for %s protocol is successful for xfer mode %d\n",
+		protocol_name[rsc.protocol], rsc.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..fd8cf5c6791f 100644
--- a/include/linux/soc/qcom/geni-se.h
+++ b/include/linux/soc/qcom/geni-se.h
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /*
  * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2023-2025 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #ifndef _LINUX_QCOM_GENI_SE
@@ -36,6 +37,7 @@ enum geni_se_protocol_type {
 	GENI_SE_I2C,
 	GENI_SE_I3C,
 	GENI_SE_SPI_SLAVE,
+	GENI_SE_INVALID_PROTO = 255,
 };
 
 struct geni_wrapper;
@@ -72,6 +74,19 @@ struct geni_se {
 	struct geni_icc_path icc_paths[3];
 };
 
+/**
+ * struct qup_se_rsc - Structure containing se details protocol and xfer mode
+ *
+ * @mode: transfer mode se fifo, dma or gsi.
+ * @protocol: Protocol spi or i2c or serial.
+ * @se: Pointer to the concerned serial engine.
+ */
+struct qup_se_rsc {
+	struct geni_se *se;
+	enum geni_se_xfer_mode mode;
+	enum geni_se_protocol_type protocol;
+};
+
 /* Common SE registers */
 #define GENI_FORCE_DEFAULT_REG		0x20
 #define GENI_OUTPUT_CTRL		0x24
@@ -531,5 +546,8 @@ 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..f139720987f9
--- /dev/null
+++ b/include/linux/soc/qcom/qup-fw-load.h
@@ -0,0 +1,179 @@
+/* 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/device.h>
+#include <linux/elf.h>
+#include <linux/firmware.h>
+#include <linux/kernel.h>
+
+/*Magic numbers*/
+#define MAGIC_NUM_SE			0x57464553
+
+/* Common SE registers*/
+#define GENI_INIT_CFG_REVISION		0x0
+#define GENI_S_INIT_CFG_REVISION	0x4
+#define GENI_FORCE_DEFAULT_REG		0x20
+#define GENI_CGC_CTRL			0x28
+#define GENI_CFG_REG0			0x100
+
+#define QUPV3_SE_HW_PARAM_1		0xE28
+#define RX_FIFO_WIDTH_BIT		24
+#define RX_FIFO_WIDTH_MASK		0x3F
+
+/*Same registers as GENI_DMA_MODE_EN*/
+#define QUPV3_SE_GENI_DMA_MODE_EN	0x258
+#define GENI_M_IRQ_ENABLE		0x614
+#define GENI_S_IRQ_ENABLE		0x644
+#define GENI_RX_RFR_WATERMARK_REG	0x814
+#define DMA_TX_IRQ_EN_SET		0xC4C
+#define DMA_RX_IRQ_EN_SET		0xD4C
+#define 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
+
+#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_MASK	0x100000
+#define MI_PBT_FLAG_PAGE_MODE_SHIFT	0x14
+#define MI_PBT_FLAG_SEGMENT_TYPE_MASK	0x7000000
+#define MI_PBT_FLAG_SEGMENT_TYPE_SHIFT	0x18
+#define MI_PBT_FLAG_ACCESS_TYPE_MASK	0xE00000
+#define MI_PBT_FLAG_ACCESS_TYPE_SHIFT	0x15
+
+#define MI_PBT_PAGE_MODE_VALUE(x) \
+	(((x) & MI_PBT_FLAG_PAGE_MODE_MASK) >> \
+	  MI_PBT_FLAG_PAGE_MODE_SHIFT)
+
+#define MI_PBT_SEGMENT_TYPE_VALUE(x) \
+	(((x) & MI_PBT_FLAG_SEGMENT_TYPE_MASK) >> \
+		MI_PBT_FLAG_SEGMENT_TYPE_SHIFT)
+
+#define MI_PBT_ACCESS_TYPE_VALUE(x) \
+	(((x) & MI_PBT_FLAG_ACCESS_TYPE_MASK) >> \
+	  MI_PBT_FLAG_ACCESS_TYPE_SHIFT)
+
+/* GENI_FORCE_DEFAULT_REG fields */
+#define FORCE_DEFAULT			BIT(0)
+
+/* FW_REVISION_RO fields */
+#define FW_REV_PROTOCOL_SHFT		8
+#define FW_REV_VERSION_SHFT		0
+
+#define GENI_FW_REVISION_RO		0x68
+#define GENI_S_FW_REVISION_RO		0x6C
+
+/* SE_GENI_DMA_MODE_EN */
+#define GENI_DMA_MODE_EN		BIT(0)
+
+/* GENI_M_IRQ_EN fields */
+#define M_CMD_DONE_EN			BIT(0)
+#define M_IO_DATA_DEASSERT_EN		BIT(22)
+#define M_IO_DATA_ASSERT_EN		BIT(23)
+#define M_RX_FIFO_RD_ERR_EN		BIT(24)
+#define M_RX_FIFO_WR_ERR_EN		BIT(25)
+#define M_RX_FIFO_WATERMARK_EN		BIT(26)
+#define M_RX_FIFO_LAST_EN		BIT(27)
+#define M_TX_FIFO_RD_ERR_EN		BIT(28)
+#define M_TX_FIFO_WR_ERR_EN		BIT(29)
+#define M_TX_FIFO_WATERMARK_EN		BIT(30)
+#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)
+
+/* GENI_S_IRQ_EN fields */
+#define S_CMD_OVERRUN_EN		BIT(1)
+#define S_ILLEGAL_CMD_EN		BIT(2)
+#define S_CMD_CANCEL_EN			BIT(4)
+#define S_CMD_ABORT_EN			BIT(5)
+#define S_GP_IRQ_0_EN			BIT(9)
+#define S_GP_IRQ_1_EN			BIT(10)
+#define S_GP_IRQ_2_EN			BIT(11)
+#define S_GP_IRQ_3_EN			BIT(12)
+#define S_RX_FIFO_RD_ERR_EN		BIT(24)
+#define S_RX_FIFO_WR_ERR_EN		BIT(25)
+#define S_COMMON_GENI_S_IRQ_EN	(GENMASK(5, 1) | GENMASK(13, 9) | \
+				 S_RX_FIFO_RD_ERR_EN | S_RX_FIFO_WR_ERR_EN)
+
+#define GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK		0x00000200
+#define GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK		0x00000100
+
+#define GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK		0x00000001
+
+#define DMA_TX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK	0x00000008
+#define DMA_TX_IRQ_EN_SET_SBE_EN_SET_BMSK		0x00000004
+#define DMA_TX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK		0x00000001
+
+#define DMA_RX_IRQ_EN_SET_FLUSH_DONE_EN_SET_BMSK	0x00000010
+#define DMA_RX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK	0x00000008
+#define DMA_RX_IRQ_EN_SET_SBE_EN_SET_BMSK		0x00000004
+#define DMA_RX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK		0x00000001
+
+#define DMA_GENERAL_CFG_AHB_SEC_SLV_CLK_CGC_ON_BMSK	0x00000008
+#define DMA_GENERAL_CFG_DMA_AHB_SLV_CLK_CGC_ON_BMSK	0x00000004
+#define DMA_GENERAL_CFG_DMA_TX_CLK_CGC_ON_BMSK		0x00000002
+#define DMA_GENERAL_CFG_DMA_RX_CLK_CGC_ON_BMSK		0x00000001
+
+#define GENI_CLK_CTRL_SER_CLK_SEL_BMSK			0x00000001
+#define DMA_IF_EN_DMA_IF_EN_BMSK			0x00000001
+#define SE_GSI_EVENT_EN_BMSK				0x0000000f
+#define SE_IRQ_EN_RMSK					0x0000000f
+
+#define QUPV3_COMMON_CFG				0x0120
+#define FAST_SWITCH_TO_HIGH_DISABLE_BMASK		0x00000001
+
+#define QUPV3_SE_AHB_M_CFG				0x0118
+#define AHB_M_CLK_CGC_ON_BMASK				0x00000001
+
+#define QUPV3_COMMON_CGC_CTRL				0x021C
+#define COMMON_CSR_SLV_CLK_CGC_ON_BMASK			0x00000001
+
+/* access ports */
+#define setbits32(_addr, _v) out_be32((_addr), in_be32(_addr) |  (_v))
+#define clrbits32(_addr, _v) out_be32((_addr), in_be32(_addr) & ~(_v))
+
+#define out_be32(a, v) writel_relaxed(v, a)
+#define in_be32(a) readl_relaxed(a)
+
+/**
+ * struct elf_se_hdr - firmware configurations
+ *
+ * @magic: set to 'SEFW'
+ * @version: A 32-bit value indicating the structure’s version number
+ * @core_version: QUPV3_HW_VERSION
+ * @serial_protocol: Programmed into GENI_FW_REVISION
+ * @fw_version: Programmed into GENI_FW_REVISION
+ * @cfg_version: Programmed into GENI_INIT_CFG_REVISION
+ * @fw_size_in_items: Number of (uint32_t) GENI_FW_RAM words
+ * @fw_offset: Byte offset of GENI_FW_RAM array
+ * @cfg_size_in_items: Number of GENI_FW_CFG index/value pairs
+ * @cfg_idx_offset: Byte offset of GENI_FW_CFG index array
+ * @cfg_val_offset: Byte offset of GENI_FW_CFG values array
+ */
+struct elf_se_hdr {
+	u32 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

Re: [PATCH v3 6/9] 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 robh/for-next tty/tty-testing tty/tty-next tty/tty-linus linus/master v6.14-rc5 next-20250303]
[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-geni-se-Add-firmware-name-property-for-firmware-loading/20250303-204936
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20250303124349.3474185-7-quic_vdadhani%40quicinc.com
patch subject: [PATCH v3 6/9] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem
config: x86_64-buildonly-randconfig-002-20250304 (https://download.01.org/0day-ci/archive/20250304/202503041123.JpkvIp44-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250304/202503041123.JpkvIp44-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/202503041123.JpkvIp44-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/soc/qcom/qcom-geni-se.c:1056: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
    * geni_enable_interrupts() Enable interrupts.
>> drivers/soc/qcom/qcom-geni-se.c:1255: warning: Function parameter or struct member 'fw_name' not described in 'qup_fw_load'


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

  1054	
  1055	/**
> 1056	 * geni_enable_interrupts() Enable interrupts.
  1057	 * @rsc: Pointer to a structure representing SE-related resources.
  1058	 *
  1059	 * Enable the required interrupts during the firmware load process.
  1060	 *
  1061	 * Return: None.
  1062	 */
  1063	static void geni_enable_interrupts(struct qup_se_rsc *rsc)
  1064	{
  1065		u32 reg_value;
  1066	
  1067		/* Enable required interrupts. */
  1068		writel_relaxed(M_COMMON_GENI_M_IRQ_EN, rsc->se->base + GENI_M_IRQ_ENABLE);
  1069	
  1070		reg_value = S_CMD_OVERRUN_EN | S_ILLEGAL_CMD_EN |
  1071					S_CMD_CANCEL_EN | S_CMD_ABORT_EN |
  1072					S_GP_IRQ_0_EN | S_GP_IRQ_1_EN |
  1073					S_GP_IRQ_2_EN | S_GP_IRQ_3_EN |
  1074					S_RX_FIFO_WR_ERR_EN | S_RX_FIFO_RD_ERR_EN;
  1075		writel_relaxed(reg_value, rsc->se->base + GENI_S_IRQ_ENABLE);
  1076	
  1077		/* DMA mode configuration. */
  1078		reg_value = DMA_TX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK |
  1079			    DMA_TX_IRQ_EN_SET_SBE_EN_SET_BMSK |
  1080			    DMA_TX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK;
  1081		writel_relaxed(reg_value, rsc->se->base + DMA_TX_IRQ_EN_SET);
  1082		reg_value = DMA_RX_IRQ_EN_SET_FLUSH_DONE_EN_SET_BMSK |
  1083			    DMA_RX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK |
  1084			    DMA_RX_IRQ_EN_SET_SBE_EN_SET_BMSK |
  1085			    DMA_RX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK;
  1086		writel_relaxed(reg_value, rsc->se->base + DMA_RX_IRQ_EN_SET);
  1087	}
  1088	
  1089	/**
  1090	 * geni_flash_fw_revision() - Flash the firmware revision.
  1091	 * @rsc: Pointer to a structure representing SE-related resources.
  1092	 * @hdr: Pointer to the ELF header of the Serial Engine.
  1093	 *
  1094	 * Flash the firmware revision and protocol into the respective register.
  1095	 *
  1096	 * Return: None.
  1097	 */
  1098	static void geni_flash_fw_revision(struct qup_se_rsc *rsc, struct elf_se_hdr *hdr)
  1099	{
  1100		u32 reg_value;
  1101	
  1102		/* Flash firmware revision register. */
  1103		reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
  1104			    (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
  1105		writel_relaxed(reg_value, rsc->se->base + SE_GENI_FW_REVISION);
  1106	
  1107		reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
  1108			    (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
  1109	
  1110		writel_relaxed(reg_value, rsc->se->base + SE_S_FW_REVISION);
  1111	}
  1112	
  1113	/**
  1114	 * geni_load_se_fw() - Load Serial Engine specific firmware.
  1115	 * @rsc: Pointer to a structure representing SE-related resources.
  1116	 * @fw: Pointer to the firmware structure.
  1117	 *
  1118	 * Load the protocol firmware into the IRAM of the Serial Engine.
  1119	 *
  1120	 * Return: 0 if successful, otherwise return an error value.
  1121	 */
  1122	static int geni_load_se_fw(struct qup_se_rsc *rsc, const struct firmware *fw)
  1123	{
  1124		const u32 *fw_val_arr, *cfg_val_arr;
  1125		const u8 *cfg_idx_arr;
  1126		u32 i, reg_value, mask, ramn_cnt;
  1127		int ret;
  1128		struct elf_se_hdr *hdr;
  1129		struct elf32_phdr *phdr;
  1130	
  1131		ret = geni_icc_set_bw(rsc->se);
  1132		if (ret) {
  1133			dev_err(rsc->se->dev, "%s: Failed to set ICC BW %d\n", __func__, ret);
  1134			return ret;
  1135		}
  1136	
  1137		ret = geni_icc_enable(rsc->se);
  1138		if (ret) {
  1139			dev_err(rsc->se->dev, "%s: Failed to enable ICC %d\n", __func__, ret);
  1140			return ret;
  1141		}
  1142	
  1143		ret = geni_se_resources_on(rsc->se);
  1144		if (ret) {
  1145			dev_err(rsc->se->dev, "%s: Failed to enable common clocks %d\n", __func__, ret);
  1146			goto err;
  1147		}
  1148	
  1149		ret = read_elf(rsc, fw, &hdr, &phdr);
  1150		if (ret) {
  1151			dev_err(rsc->se->dev, "%s: ELF parsing failed ret: %d\n", __func__, ret);
  1152			goto err;
  1153		}
  1154	
  1155		fw_val_arr = (const u32 *)((u8 *)hdr + hdr->fw_offset);
  1156		cfg_idx_arr = (const u8 *)hdr + hdr->cfg_idx_offset;
  1157		cfg_val_arr = (const u32 *)((u8 *)hdr + hdr->cfg_val_offset);
  1158	
  1159		geni_config_common_control(rsc);
  1160	
  1161		/* Allows to drive corresponding data according to hardware value. */
  1162		writel_relaxed(0x0, rsc->se->base + GENI_OUTPUT_CTRL);
  1163	
  1164		/* Set SCLK and HCLK to program RAM */
  1165		setbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
  1166				GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
  1167		writel_relaxed(0x0, rsc->se->base + SE_GENI_CLK_CTRL);
  1168		clrbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
  1169				GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
  1170	
  1171		/* Enable required clocks for DMA CSR, TX and RX. */
  1172		reg_value = DMA_GENERAL_CFG_AHB_SEC_SLV_CLK_CGC_ON_BMSK |
  1173			DMA_GENERAL_CFG_DMA_AHB_SLV_CLK_CGC_ON_BMSK |
  1174			DMA_GENERAL_CFG_DMA_TX_CLK_CGC_ON_BMSK |
  1175			DMA_GENERAL_CFG_DMA_RX_CLK_CGC_ON_BMSK;
  1176	
  1177		setbits32(rsc->se->base + DMA_GENERAL_CFG, reg_value);
  1178	
  1179		/* Let hardware control CGC by default. */
  1180		writel_relaxed(DEFAULT_CGC_EN, rsc->se->base + GENI_CGC_CTRL);
  1181	
  1182		/* Set version of the configuration register part of firmware. */
  1183		writel_relaxed(hdr->cfg_version, rsc->se->base + GENI_INIT_CFG_REVISION);
  1184		writel_relaxed(hdr->cfg_version, rsc->se->base + GENI_S_INIT_CFG_REVISION);
  1185	
  1186		/* Configure GENI primitive table. */
  1187		for (i = 0; i < hdr->cfg_size_in_items; i++)
  1188			writel_relaxed(cfg_val_arr[i],
  1189				       rsc->se->base + GENI_CFG_REG0 + (cfg_idx_arr[i] * sizeof(u32)));
  1190	
  1191		/* Configure condition for assertion of RX_RFR_WATERMARK condition. */
  1192		reg_value = readl_relaxed(rsc->se->base + QUPV3_SE_HW_PARAM_1);
  1193		mask = (reg_value >> RX_FIFO_WIDTH_BIT) & RX_FIFO_WIDTH_MASK;
  1194		writel_relaxed(mask - 2, rsc->se->base + GENI_RX_RFR_WATERMARK_REG);
  1195	
  1196		/* Let hardware control CGC */
  1197		setbits32(rsc->se->base + GENI_OUTPUT_CTRL, DEFAULT_IO_OUTPUT_CTRL_MSK);
  1198	
  1199		ret = geni_configure_xfer_mode(rsc);
  1200		if (ret)
  1201			goto err_resource;
  1202	
  1203		geni_enable_interrupts(rsc);
  1204	
  1205		geni_flash_fw_revision(rsc, hdr);
  1206	
  1207		ramn_cnt = hdr->fw_size_in_items;
  1208		if (hdr->fw_size_in_items % 2 != 0)
  1209			ramn_cnt++;
  1210	
  1211		if (ramn_cnt >= MAX_GENI_CFG_RAMn_CNT)
  1212			goto err_resource;
  1213	
  1214		/* Program RAM address space. */
  1215		memcpy((void *)(rsc->se->base + SE_GENI_CFG_RAMN), fw_val_arr, ramn_cnt * sizeof(u32));
  1216	
  1217		/* Put default values on GENI's output pads. */
  1218		writel_relaxed(0x1, rsc->se->base + GENI_FORCE_DEFAULT_REG);
  1219	
  1220		/* High to low SCLK and HCLK to finish RAM. */
  1221		setbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
  1222				GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
  1223		setbits32(rsc->se->base + SE_GENI_CLK_CTRL, GENI_CLK_CTRL_SER_CLK_SEL_BMSK);
  1224		clrbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
  1225				GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
  1226	
  1227		/* Serial engine DMA interface is enabled. */
  1228		setbits32(rsc->se->base + SE_DMA_IF_EN, DMA_IF_EN_DMA_IF_EN_BMSK);
  1229	
  1230		/* Enable or disable FIFO interface of the serial engine. */
  1231		if (rsc->mode == GENI_SE_FIFO)
  1232			clrbits32(rsc->se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
  1233		else
  1234			setbits32(rsc->se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
  1235	
  1236	err_resource:
  1237		geni_se_resources_off(rsc->se);
  1238	err:
  1239		geni_icc_disable(rsc->se);
  1240		return ret;
  1241	}
  1242	
  1243	/**
  1244	 * qup_fw_load() - Initiate firmware load.
  1245	 * @rsc: Pointer to a structure representing SE-related resources.
  1246	 *
  1247	 * Load the firmware into a specific SE. Read the associated ELF file,
  1248	 * copy the data into a buffer in kernel space using the request_firmware API, write the
  1249	 * data into the SE's IRAM register, and then free the buffers. Handle firmware loading
  1250	 * and parsing for a specific protocol.
  1251	 *
  1252	 * Return: 0 if successful, otherwise return an error value.
  1253	 */
  1254	static int qup_fw_load(struct qup_se_rsc *rsc, const char *fw_name)
> 1255	{
  1256		int ret;
  1257		const struct firmware *fw;
  1258		struct device *dev = rsc->se->dev;
  1259	
  1260		ret = request_firmware(&fw, fw_name, dev);
  1261		if (ret) {
  1262			dev_err(dev, "request_firmware failed for %d: %d\n", rsc->protocol, ret);
  1263			return ret;
  1264		}
  1265	
  1266		ret = geni_load_se_fw(rsc, fw);
  1267	
  1268		release_firmware(fw);
  1269	
  1270		return ret;
  1271	}
  1272	

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

Re: [PATCH v3 6/9] 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 robh/for-next tty/tty-testing tty/tty-next tty/tty-linus linus/master v6.14-rc5 next-20250305]
[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-geni-se-Add-firmware-name-property-for-firmware-loading/20250303-204936
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20250303124349.3474185-7-quic_vdadhani%40quicinc.com
patch subject: [PATCH v3 6/9] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20250306/202503060042.qiMGnPlW-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250306/202503060042.qiMGnPlW-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/202503060042.qiMGnPlW-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/soc/qcom/qcom-geni-se.c:19:
>> include/linux/soc/qcom/qup-fw-load.h:148:9: warning: "out_be32" redefined
     148 | #define out_be32(a, v) writel_relaxed(v, a)
         |         ^~~~~~~~
   In file included from arch/m68k/include/asm/io_mm.h:25,
                    from arch/m68k/include/asm/io.h:8,
                    from include/linux/scatterlist.h:9,
                    from include/linux/dma-mapping.h:8,
                    from drivers/soc/qcom/qcom-geni-se.c:11:
   arch/m68k/include/asm/raw_io.h:32:9: note: this is the location of the previous definition
      32 | #define out_be32(addr,l) (void)((*(__force volatile u32 *) (unsigned long)(addr)) = (l))
         |         ^~~~~~~~
>> include/linux/soc/qcom/qup-fw-load.h:149:9: warning: "in_be32" redefined
     149 | #define in_be32(a) readl_relaxed(a)
         |         ^~~~~~~
   arch/m68k/include/asm/raw_io.h:23:9: note: this is the location of the previous definition
      23 | #define in_be32(addr) \
         |         ^~~~~~~


vim +/out_be32 +148 include/linux/soc/qcom/qup-fw-load.h

   147	
 > 148	#define out_be32(a, v) writel_relaxed(v, a)
 > 149	#define in_be32(a) readl_relaxed(a)
   150	

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

Re: [PATCH v3 6/9] 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 robh/for-next tty/tty-testing tty/tty-next tty/tty-linus linus/master v6.14-rc5 next-20250305]
[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-geni-se-Add-firmware-name-property-for-firmware-loading/20250303-204936
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20250303124349.3474185-7-quic_vdadhani%40quicinc.com
patch subject: [PATCH v3 6/9] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem
config: mips-randconfig-r131-20250305 (https://download.01.org/0day-ci/archive/20250306/202503060145.gERd9R0P-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project 14170b16028c087ca154878f5ed93d3089a965c6)
reproduce: (https://download.01.org/0day-ci/archive/20250306/202503060145.gERd9R0P-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/202503060145.gERd9R0P-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/soc/qcom/qcom-geni-se.c:1215:9: sparse: sparse: cast removes address space '__iomem' of expression
>> drivers/soc/qcom/qcom-geni-se.c:1215:9: sparse: sparse: cast removes address space '__iomem' of expression
>> drivers/soc/qcom/qcom-geni-se.c:1215:9: sparse: sparse: cast removes address space '__iomem' of expression

vim +/__iomem +1215 drivers/soc/qcom/qcom-geni-se.c

  1112	
  1113	/**
  1114	 * geni_load_se_fw() - Load Serial Engine specific firmware.
  1115	 * @rsc: Pointer to a structure representing SE-related resources.
  1116	 * @fw: Pointer to the firmware structure.
  1117	 *
  1118	 * Load the protocol firmware into the IRAM of the Serial Engine.
  1119	 *
  1120	 * Return: 0 if successful, otherwise return an error value.
  1121	 */
  1122	static int geni_load_se_fw(struct qup_se_rsc *rsc, const struct firmware *fw)
  1123	{
  1124		const u32 *fw_val_arr, *cfg_val_arr;
  1125		const u8 *cfg_idx_arr;
  1126		u32 i, reg_value, mask, ramn_cnt;
  1127		int ret;
  1128		struct elf_se_hdr *hdr;
  1129		struct elf32_phdr *phdr;
  1130	
  1131		ret = geni_icc_set_bw(rsc->se);
  1132		if (ret) {
  1133			dev_err(rsc->se->dev, "%s: Failed to set ICC BW %d\n", __func__, ret);
  1134			return ret;
  1135		}
  1136	
  1137		ret = geni_icc_enable(rsc->se);
  1138		if (ret) {
  1139			dev_err(rsc->se->dev, "%s: Failed to enable ICC %d\n", __func__, ret);
  1140			return ret;
  1141		}
  1142	
  1143		ret = geni_se_resources_on(rsc->se);
  1144		if (ret) {
  1145			dev_err(rsc->se->dev, "%s: Failed to enable common clocks %d\n", __func__, ret);
  1146			goto err;
  1147		}
  1148	
  1149		ret = read_elf(rsc, fw, &hdr, &phdr);
  1150		if (ret) {
  1151			dev_err(rsc->se->dev, "%s: ELF parsing failed ret: %d\n", __func__, ret);
  1152			goto err;
  1153		}
  1154	
  1155		fw_val_arr = (const u32 *)((u8 *)hdr + hdr->fw_offset);
  1156		cfg_idx_arr = (const u8 *)hdr + hdr->cfg_idx_offset;
  1157		cfg_val_arr = (const u32 *)((u8 *)hdr + hdr->cfg_val_offset);
  1158	
  1159		geni_config_common_control(rsc);
  1160	
  1161		/* Allows to drive corresponding data according to hardware value. */
  1162		writel_relaxed(0x0, rsc->se->base + GENI_OUTPUT_CTRL);
  1163	
  1164		/* Set SCLK and HCLK to program RAM */
  1165		setbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
  1166				GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
  1167		writel_relaxed(0x0, rsc->se->base + SE_GENI_CLK_CTRL);
  1168		clrbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
  1169				GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
  1170	
  1171		/* Enable required clocks for DMA CSR, TX and RX. */
  1172		reg_value = DMA_GENERAL_CFG_AHB_SEC_SLV_CLK_CGC_ON_BMSK |
  1173			DMA_GENERAL_CFG_DMA_AHB_SLV_CLK_CGC_ON_BMSK |
  1174			DMA_GENERAL_CFG_DMA_TX_CLK_CGC_ON_BMSK |
  1175			DMA_GENERAL_CFG_DMA_RX_CLK_CGC_ON_BMSK;
  1176	
  1177		setbits32(rsc->se->base + DMA_GENERAL_CFG, reg_value);
  1178	
  1179		/* Let hardware control CGC by default. */
  1180		writel_relaxed(DEFAULT_CGC_EN, rsc->se->base + GENI_CGC_CTRL);
  1181	
  1182		/* Set version of the configuration register part of firmware. */
  1183		writel_relaxed(hdr->cfg_version, rsc->se->base + GENI_INIT_CFG_REVISION);
  1184		writel_relaxed(hdr->cfg_version, rsc->se->base + GENI_S_INIT_CFG_REVISION);
  1185	
  1186		/* Configure GENI primitive table. */
  1187		for (i = 0; i < hdr->cfg_size_in_items; i++)
  1188			writel_relaxed(cfg_val_arr[i],
  1189				       rsc->se->base + GENI_CFG_REG0 + (cfg_idx_arr[i] * sizeof(u32)));
  1190	
  1191		/* Configure condition for assertion of RX_RFR_WATERMARK condition. */
  1192		reg_value = readl_relaxed(rsc->se->base + QUPV3_SE_HW_PARAM_1);
  1193		mask = (reg_value >> RX_FIFO_WIDTH_BIT) & RX_FIFO_WIDTH_MASK;
  1194		writel_relaxed(mask - 2, rsc->se->base + GENI_RX_RFR_WATERMARK_REG);
  1195	
  1196		/* Let hardware control CGC */
  1197		setbits32(rsc->se->base + GENI_OUTPUT_CTRL, DEFAULT_IO_OUTPUT_CTRL_MSK);
  1198	
  1199		ret = geni_configure_xfer_mode(rsc);
  1200		if (ret)
  1201			goto err_resource;
  1202	
  1203		geni_enable_interrupts(rsc);
  1204	
  1205		geni_flash_fw_revision(rsc, hdr);
  1206	
  1207		ramn_cnt = hdr->fw_size_in_items;
  1208		if (hdr->fw_size_in_items % 2 != 0)
  1209			ramn_cnt++;
  1210	
  1211		if (ramn_cnt >= MAX_GENI_CFG_RAMn_CNT)
  1212			goto err_resource;
  1213	
  1214		/* Program RAM address space. */
> 1215		memcpy((void *)(rsc->se->base + SE_GENI_CFG_RAMN), fw_val_arr, ramn_cnt * sizeof(u32));
  1216	
  1217		/* Put default values on GENI's output pads. */
  1218		writel_relaxed(0x1, rsc->se->base + GENI_FORCE_DEFAULT_REG);
  1219	
  1220		/* High to low SCLK and HCLK to finish RAM. */
  1221		setbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
  1222				GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
  1223		setbits32(rsc->se->base + SE_GENI_CLK_CTRL, GENI_CLK_CTRL_SER_CLK_SEL_BMSK);
  1224		clrbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
  1225				GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
  1226	
  1227		/* Serial engine DMA interface is enabled. */
  1228		setbits32(rsc->se->base + SE_DMA_IF_EN, DMA_IF_EN_DMA_IF_EN_BMSK);
  1229	
  1230		/* Enable or disable FIFO interface of the serial engine. */
  1231		if (rsc->mode == GENI_SE_FIFO)
  1232			clrbits32(rsc->se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
  1233		else
  1234			setbits32(rsc->se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
  1235	
  1236	err_resource:
  1237		geni_se_resources_off(rsc->se);
  1238	err:
  1239		geni_icc_disable(rsc->se);
  1240		return ret;
  1241	}
  1242	

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

Re: [PATCH v3 6/9] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem

[Bjorn Andersson]

On Mon, Mar 03, 2025 at 06:13:46PM +0530, Viken Dadhaniya wrote:
> Load the firmware to QUP SE based on the 'firmware-name' property specified

Please start your commit message with a description of "the problem"
you're trying to solve, explain that QUP firmware is typically loaded by
the "bootloader" and that for <reason> you're adding support for loading
firmware are load time. Please read and follow:
https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

> in devicetree. Populate Serial engine and base address details in the probe
> function of the protocol driver and pass to firmware load routine.

Don't describe the code flow, describe how this fits into the bigger
picture. E.g. that SE-functional drivers will load this at bootup.

> 
> Skip the firmware loading if the firmware is already loaded in Serial
> Engine's firmware memory area.
> 

Above description should be clear enough that this is obvious.

> Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> Signed-off-by: Viken Dadhaniya <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      | 423 +++++++++++++++++++++++++++
>  include/linux/soc/qcom/geni-se.h     |  18 ++
>  include/linux/soc/qcom/qup-fw-load.h | 179 ++++++++++++
>  3 files changed, 620 insertions(+)
>  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..345181ad6fcc 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -1,5 +1,6 @@
>  // 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.

Odd, this should be a multiline comment.

>  
>  /* Disable MMIO tracing to prevent excessive logging of unwanted MMIO traces */
>  #define __DISABLE_TRACE_MMIO__
> @@ -15,6 +16,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
> @@ -110,6 +112,9 @@ struct geni_se_desc {
>  static const char * const icc_path_names[] = {"qup-core", "qup-config",
>  						"qup-memory"};
>  
> +static const char * const protocol_name[] = { "None", "SPI", "UART",
> +					      "I2C", "I3C", "SPI SLAVE"};
> +
>  #define QUP_HW_VER_REG			0x4
>  
>  /* Common SE registers */
> @@ -891,6 +896,424 @@ int geni_icc_disable(struct geni_se *se)
>  }
>  EXPORT_SYMBOL_GPL(geni_icc_disable);
>  
> +/**
> + * elf_phdr_valid() - Validate an ELF header.
> + * @phdr: Pointer to the ELF header.
> + *
> + * Validate the ELF header by comparing the fields stored in p_flags and the payload type.

The interesting piece of information here would be what the definition
of "valid" is - because you're not checking that the program header
entry is valid, you're checking that it's a specific valid.

> + *
> + * Return: true if the validation is successful, false otherwise.
> + */

That said, I think you should just inline these checks in "read_elf()"
below, preferably with a comment about what you're looking for.

> +static bool elf_phdr_valid(const struct elf32_phdr *phdr)
> +{
> +	if (phdr->p_type != PT_LOAD || !phdr->p_memsz)
> +		return false;
> +
> +	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)
> +		return true;
> +
> +	return false;
> +}
> +
> +/**
> + * valid_seg_size() - Validate the segment size.
> + * @pelfseg: Pointer to the ELF header.
> + * @p_filesz: Pointer to the file size.
> + *
> + * Validate the ELF segment size by comparing the file size.
> + *
> + * Return: true if the segment is valid, false if the segment is invalid.
> + */
> +static bool valid_seg_size(struct elf_se_hdr *pelfseg, Elf32_Word p_filesz)
> +{
> +	if (p_filesz >= pelfseg->fw_offset + pelfseg->fw_size_in_items * sizeof(u32) &&
> +	    p_filesz >= pelfseg->cfg_idx_offset + pelfseg->cfg_size_in_items * sizeof(u8) &&
> +	    p_filesz >= pelfseg->cfg_val_offset + pelfseg->cfg_size_in_items * sizeof(u32))
> +		return true;
> +	return false;
> +}
> +
> +/**
> + * read_elf() - Read an ELF file.
> + * @rsc: Pointer to the SE resources structure.
> + * @fw: Pointer to the firmware buffer.
> + * @pelfseg: Pointer to the SE-specific ELF header.
> + * @phdr: Pointer to one of the valid headers from the list in the firmware buffer.
> + *
> + * 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 read_elf(struct qup_se_rsc *rsc, const struct firmware *fw,

I think this name is too generic, please prefix it with "geni_" to make
it easier to search for.

> +		    struct elf_se_hdr **pelfseg, struct elf32_phdr **phdr)
> +{
> +	const struct elf32_hdr *ehdr = (const struct elf32_hdr *)fw->data;
> +	struct elf32_phdr *phdrs = (struct elf32_phdr *)(ehdr + 1);
> +	const u8 *addr;
> +	int i;
> +
> +	ehdr = (struct elf32_hdr *)fw->data;

Please validate that fw->size is sufficient before accessing - here and
in the loop.

> +
> +	if (ehdr->e_phnum < 2)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ehdr->e_phnum; i++) {
> +		*phdr = &phdrs[i];
> +		if (!elf_phdr_valid(*phdr))
> +			continue;
> +
> +		if ((*phdr)->p_filesz >= sizeof(struct elf_se_hdr)) {
> +			addr =  fw->data + (*phdr)->p_offset;
> +			*pelfseg = (struct elf_se_hdr *)addr;

Don't assign *pelfseg until you've found a match.

> +
> +			if ((*pelfseg)->magic == MAGIC_NUM_SE &&
> +			    (*pelfseg)->version == 1 &&
> +			    valid_seg_size(*pelfseg, (*phdr)->p_filesz) &&
> +			    (*pelfseg)->serial_protocol == rsc->protocol &&
> +			    (*pelfseg)->serial_protocol != GENI_SE_NONE)

There's so much going on in this one conditional. Turn this the way
around:

if (phdr->p_filesz < sizeof(struct elf_se_hdr))
	continue;

if (se->magic != MAGIC_NUM_SE)
	continue;

...

Makes is super easy to read and each one encapsulates one disqualifying
factor.

> +				return 0;
> +		}
> +	}
> +	return -EINVAL;
> +}
> +
> +/**
> + * geni_config_common_control() - Configure common CGC and disable high priority interrupt.
> + * @rsc: Pointer to a structure representing SE-related resources.
> + *
> + * Configure the common CGC and disable high priority interrupts until the current low priority
> + * interrupts are handled.

What do you mean with "low priority interrupt", this is called from the
firmware loading operation - that doesn't sound like an interrupt at
all.

It leaves me with the question about why interrupts should be disabled
and why they should be disabled in a function called "config common
control".


Unless it makes the code easier to read, you should wrap it at 80
characters. I don't think this sentence is easier to read in it's
unwrapped form.

> + *
> + * Return: None.

Drop this for void functions.

> + */
> +static void geni_config_common_control(struct qup_se_rsc *rsc)
> +{
> +	/*
> +	 * Disable high priority interrupt until current
> +	 * low priority interrupts are handled.
> +	 */
> +	setbits32(rsc->se->wrapper->base + QUPV3_COMMON_CFG,

I find that the qup_se_rsc structure makes these functions overflow with
pointer dereferences, and it makes it impossible to know if a function
operates on 1, 2, or 3 of its members.

Please just pass the arguments around where necessary.

> +		  FAST_SWITCH_TO_HIGH_DISABLE_BMASK);
> +
> +	/*
> +	 * Set AHB_M_CLK_CGC_ON to indicate hardware controls
> +	 * se-wrapper cgc clock.
> +	 */
> +	setbits32(rsc->se->wrapper->base + QUPV3_SE_AHB_M_CFG,
> +		  AHB_M_CLK_CGC_ON_BMASK);
> +
> +	/* Let hardware to control common cgc. */
> +	setbits32(rsc->se->wrapper->base + QUPV3_COMMON_CGC_CTRL,
> +		  COMMON_CSR_SLV_CLK_CGC_ON_BMASK);
> +}
> +
> +/**
> + * geni_configure_xfer_mode() - Set the transfer mode.
> + * @rsc: 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 qup_se_rsc *rsc)
> +{
> +	/* Configure SE FIFO, DMA or GSI mode. */
> +	switch (rsc->mode) {
> +	case GENI_GPI_DMA:
> +		setbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
> +			  GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
> +		writel_relaxed(0x0, rsc->se->base + SE_IRQ_EN);
> +		writel_relaxed(SE_GSI_EVENT_EN_BMSK, rsc->se->base + SE_GSI_EVENT_EN);
> +		break;
> +
> +	case GENI_SE_FIFO:
> +		clrbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
> +			  GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
> +		writel_relaxed(SE_IRQ_EN_RMSK, rsc->se->base + SE_IRQ_EN);
> +		writel_relaxed(0x0, rsc->se->base + SE_GSI_EVENT_EN);
> +		break;
> +
> +	case GENI_SE_DMA:
> +		setbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
> +			  GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
> +		writel_relaxed(SE_IRQ_EN_RMSK, rsc->se->base + SE_IRQ_EN);
> +		writel_relaxed(0x0, rsc->se->base + SE_GSI_EVENT_EN);
> +		break;
> +
> +	default:
> +		dev_err(rsc->se->dev, "invalid se mode: %d\n", rsc->mode);

Please make this error more descriptive so that it's easier for someone
finding it in the kernel log to understand what the problem is.

> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * geni_enable_interrupts() Enable interrupts.
> + * @rsc: Pointer to a structure representing SE-related resources.
> + *
> + * Enable the required interrupts during the firmware load process.
> + *
> + * Return: None.
> + */
> +static void geni_enable_interrupts(struct qup_se_rsc *rsc)
> +{
> +	u32 reg_value;
> +
> +	/* Enable required interrupts. */
> +	writel_relaxed(M_COMMON_GENI_M_IRQ_EN, rsc->se->base + GENI_M_IRQ_ENABLE);
> +
> +	reg_value = S_CMD_OVERRUN_EN | S_ILLEGAL_CMD_EN |
> +				S_CMD_CANCEL_EN | S_CMD_ABORT_EN |

Please confirm that the indentation is correct here.

> +				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, rsc->se->base + GENI_S_IRQ_ENABLE);
> +
> +	/* DMA mode configuration. */
> +	reg_value = DMA_TX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK |
> +		    DMA_TX_IRQ_EN_SET_SBE_EN_SET_BMSK |
> +		    DMA_TX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK;
> +	writel_relaxed(reg_value, rsc->se->base + DMA_TX_IRQ_EN_SET);
> +	reg_value = DMA_RX_IRQ_EN_SET_FLUSH_DONE_EN_SET_BMSK |
> +		    DMA_RX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK |
> +		    DMA_RX_IRQ_EN_SET_SBE_EN_SET_BMSK |
> +		    DMA_RX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK;
> +	writel_relaxed(reg_value, rsc->se->base + DMA_RX_IRQ_EN_SET);
> +}
> +
> +/**
> + * geni_flash_fw_revision() - Flash the firmware revision.

It's not really "flashing", it's more just "writing"...right?

> + * @rsc: Pointer to a structure representing SE-related resources.
> + * @hdr: Pointer to the ELF header of the Serial Engine.

That's not the ELF header, it's the serial engine firmware header. That
said, you only consume the serial_protocol and fw_version, so a function
called "geni_write_fw_revision()" could perhaps just take those two
values as parameters?

> + *
> + * Flash the firmware revision and protocol into the respective register.

Again, no flashing, just writing.

> + *
> + * Return: None.
> + */
> +static void geni_flash_fw_revision(struct qup_se_rsc *rsc, struct elf_se_hdr *hdr)
> +{
> +	u32 reg_value;
> +
> +	/* Flash firmware revision register. */
> +	reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
> +		    (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
> +	writel_relaxed(reg_value, rsc->se->base + SE_GENI_FW_REVISION);
> +
> +	reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
> +		    (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
> +
> +	writel_relaxed(reg_value, rsc->se->base + SE_S_FW_REVISION);
> +}
> +
> +/**
> + * geni_load_se_fw() - Load Serial Engine specific firmware.
> + * @rsc: 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 qup_se_rsc *rsc, const struct firmware *fw)
> +{
> +	const u32 *fw_val_arr, *cfg_val_arr;
> +	const u8 *cfg_idx_arr;
> +	u32 i, reg_value, mask, ramn_cnt;
> +	int ret;
> +	struct elf_se_hdr *hdr;
> +	struct elf32_phdr *phdr;
> +
> +	ret = geni_icc_set_bw(rsc->se);
> +	if (ret) {
> +		dev_err(rsc->se->dev, "%s: Failed to set ICC BW %d\n", __func__, ret);

Drop the __func__ from all your printouts.

> +		return ret;
> +	}
> +
> +	ret = geni_icc_enable(rsc->se);
> +	if (ret) {
> +		dev_err(rsc->se->dev, "%s: Failed to enable ICC %d\n", __func__, ret);
> +		return ret;
> +	}
> +
> +	ret = geni_se_resources_on(rsc->se);

Why do you turn on the buses and clocks before you're parsing the
firmware file?

> +	if (ret) {
> +		dev_err(rsc->se->dev, "%s: Failed to enable common clocks %d\n", __func__, ret);
> +		goto err;
> +	}
> +
> +	ret = read_elf(rsc, fw, &hdr, &phdr);

Why is phdr returned here? Makes read_elf() harder to read...

> +	if (ret) {
> +		dev_err(rsc->se->dev, "%s: ELF parsing failed ret: %d\n", __func__, ret);
> +		goto err;
> +	}
> +
> +	fw_val_arr = (const u32 *)((u8 *)hdr + hdr->fw_offset);

Why is this a "firmware value array", isn't it the "firmware data"? Why
is it chunks of u32? Does it also need to be written in words?

> +	cfg_idx_arr = (const u8 *)hdr + hdr->cfg_idx_offset;
> +	cfg_val_arr = (const u32 *)((u8 *)hdr + hdr->cfg_val_offset);
> +
> +	geni_config_common_control(rsc);
> +
> +	/* Allows to drive corresponding data according to hardware value. */
> +	writel_relaxed(0x0, rsc->se->base + GENI_OUTPUT_CTRL);
> +
> +	/* Set SCLK and HCLK to program RAM */
> +	setbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
> +			GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
> +	writel_relaxed(0x0, rsc->se->base + SE_GENI_CLK_CTRL);
> +	clrbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
> +			GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
> +
> +	/* Enable required clocks for DMA CSR, TX and RX. */
> +	reg_value = DMA_GENERAL_CFG_AHB_SEC_SLV_CLK_CGC_ON_BMSK |
> +		DMA_GENERAL_CFG_DMA_AHB_SLV_CLK_CGC_ON_BMSK |
> +		DMA_GENERAL_CFG_DMA_TX_CLK_CGC_ON_BMSK |
> +		DMA_GENERAL_CFG_DMA_RX_CLK_CGC_ON_BMSK;
> +
> +	setbits32(rsc->se->base + DMA_GENERAL_CFG, reg_value);
> +
> +	/* Let hardware control CGC by default. */
> +	writel_relaxed(DEFAULT_CGC_EN, rsc->se->base + GENI_CGC_CTRL);
> +
> +	/* Set version of the configuration register part of firmware. */
> +	writel_relaxed(hdr->cfg_version, rsc->se->base + GENI_INIT_CFG_REVISION);
> +	writel_relaxed(hdr->cfg_version, rsc->se->base + 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],
> +			       rsc->se->base + GENI_CFG_REG0 + (cfg_idx_arr[i] * sizeof(u32)));

What goes into this configuration?

> +
> +	/* Configure condition for assertion of RX_RFR_WATERMARK condition. */
> +	reg_value = readl_relaxed(rsc->se->base + QUPV3_SE_HW_PARAM_1);
> +	mask = (reg_value >> RX_FIFO_WIDTH_BIT) & RX_FIFO_WIDTH_MASK;

Based on the WIDTH_MASK, those 6 bits doesn't sound like a "BIT"... And
is the RX_FIFO_WIDTH really a "mask"?

> +	writel_relaxed(mask - 2, rsc->se->base + GENI_RX_RFR_WATERMARK_REG);

mask - 2?

Use FIELD_GET() and name things with the intention of making it easy for
others to understand what the code does.

> +
> +	/* Let hardware control CGC */
> +	setbits32(rsc->se->base + GENI_OUTPUT_CTRL, DEFAULT_IO_OUTPUT_CTRL_MSK);
> +
> +	ret = geni_configure_xfer_mode(rsc);
> +	if (ret)
> +		goto err_resource;
> +
> +	geni_enable_interrupts(rsc);
> +
> +	geni_flash_fw_revision(rsc, hdr);
> +
> +	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;

As far as I can tell this error path is based entirely on the firmware
being read from the file system, so you could have checked it before you
turned on any resources - making a cleaner exit on failure.

> +
> +	/* Program RAM address space. */
> +	memcpy((void *)(rsc->se->base + SE_GENI_CFG_RAMN), fw_val_arr, ramn_cnt * sizeof(u32));

I presume this typecast to void * is to hide the fact that se->base is
flagged as __iomem? Use memcpy_toio() intead.

> +
> +	/* Put default values on GENI's output pads. */
> +	writel_relaxed(0x1, rsc->se->base + GENI_FORCE_DEFAULT_REG);
> +
> +	/* High to low SCLK and HCLK to finish RAM. */
> +	setbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
> +			GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
> +	setbits32(rsc->se->base + SE_GENI_CLK_CTRL, GENI_CLK_CTRL_SER_CLK_SEL_BMSK);
> +	clrbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
> +			GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
> +
> +	/* Serial engine DMA interface is enabled. */
> +	setbits32(rsc->se->base + SE_DMA_IF_EN, DMA_IF_EN_DMA_IF_EN_BMSK);
> +
> +	/* Enable or disable FIFO interface of the serial engine. */
> +	if (rsc->mode == GENI_SE_FIFO)
> +		clrbits32(rsc->se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
> +	else
> +		setbits32(rsc->se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
> +
> +err_resource:
> +	geni_se_resources_off(rsc->se);
> +err:
> +	geni_icc_disable(rsc->se);
> +	return ret;
> +}
> +
> +/**
> + * qup_fw_load() - Initiate firmware load.
> + * @rsc: Pointer to a structure representing SE-related resources.
> + *
> + * 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 qup_se_rsc *rsc, const char *fw_name)
> +{
> +	int ret;
> +	const struct firmware *fw;
> +	struct device *dev = rsc->se->dev;
> +
> +	ret = request_firmware(&fw, fw_name, dev);
> +	if (ret) {
> +		dev_err(dev, "request_firmware failed for %d: %d\n", rsc->protocol, ret);
> +		return ret;
> +	}
> +
> +	ret = geni_load_se_fw(rsc, 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)

This line is 82 characters long if you don't line break it, but it's
cleaner to read...so please unbreak it.

> +{
> +	struct qup_se_rsc rsc;
> +	const char *fw_name;
> +	int ret;
> +
> +	ret = device_property_read_string(se->wrapper->dev, "firmware-name", &fw_name);
> +	if (ret)
> +		return  -EINVAL;
> +
> +	rsc.se = se;
> +	rsc.protocol = protocol;
> +	/* Set default xfer mode to FIFO */

This isn't the "default" mode, this is the else statement in the
conditional below.

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

if (of_property_present())
	sc.mode = GENI_GPI_DMA;
else
	sc.mode = GENI_GPI_FIFO;

/* Comment about UART, because it's actually worth mentioning */
if (serial)


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

How do you know that rsc.protocol < ARRAY_SIZE(protocol_name)?

> +	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..fd8cf5c6791f 100644
> --- a/include/linux/soc/qcom/geni-se.h
> +++ b/include/linux/soc/qcom/geni-se.h
> @@ -1,6 +1,7 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  /*
>   * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023-2025 Qualcomm Innovation Center, Inc. All rights reserved.
>   */
>  
>  #ifndef _LINUX_QCOM_GENI_SE
> @@ -36,6 +37,7 @@ enum geni_se_protocol_type {
>  	GENI_SE_I2C,
>  	GENI_SE_I3C,
>  	GENI_SE_SPI_SLAVE,
> +	GENI_SE_INVALID_PROTO = 255,

This is unused.

>  };
>  
>  struct geni_wrapper;
> @@ -72,6 +74,19 @@ struct geni_se {
>  	struct geni_icc_path icc_paths[3];
>  };
>  
> +/**
> + * struct qup_se_rsc - Structure containing se details protocol and xfer mode
> + *
> + * @mode: transfer mode se fifo, dma or gsi.
> + * @protocol: Protocol spi or i2c or serial.
> + * @se: Pointer to the concerned serial engine.

I'd like you to drop this struct, so it doesn't really matter...but
order of parameters doesn't match the structure.

> + */
> +struct qup_se_rsc {
> +	struct geni_se *se;
> +	enum geni_se_xfer_mode mode;
> +	enum geni_se_protocol_type protocol;
> +};
> +
>  /* Common SE registers */
>  #define GENI_FORCE_DEFAULT_REG		0x20
>  #define GENI_OUTPUT_CTRL		0x24
> @@ -531,5 +546,8 @@ 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);

Unbreak the line.

>  #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..f139720987f9
> --- /dev/null
> +++ b/include/linux/soc/qcom/qup-fw-load.h
> @@ -0,0 +1,179 @@
> +/* 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/device.h>
> +#include <linux/elf.h>
> +#include <linux/firmware.h>

Why are device.h, elf.h and firmware.h included here, they are not used.

> +#include <linux/kernel.h>
> +
> +/*Magic numbers*/
> +#define MAGIC_NUM_SE			0x57464553

Prefix things.

> +
> +/* Common SE registers*/
> +#define GENI_INIT_CFG_REVISION		0x0
> +#define GENI_S_INIT_CFG_REVISION	0x4
> +#define GENI_FORCE_DEFAULT_REG		0x20

A bunch of these registers and bitmasks area already defined in
include/soc/qcom/geni-se.h why are they duplicated here?

> +#define GENI_CGC_CTRL			0x28
> +#define GENI_CFG_REG0			0x100
> +
> +#define QUPV3_SE_HW_PARAM_1		0xE28

Lowercase hex digits please.

> +#define RX_FIFO_WIDTH_BIT		24
> +#define RX_FIFO_WIDTH_MASK		0x3F
> +
> +/*Same registers as GENI_DMA_MODE_EN*/
> +#define QUPV3_SE_GENI_DMA_MODE_EN	0x258
> +#define GENI_M_IRQ_ENABLE		0x614
> +#define GENI_S_IRQ_ENABLE		0x644
> +#define GENI_RX_RFR_WATERMARK_REG	0x814
> +#define DMA_TX_IRQ_EN_SET		0xC4C
> +#define DMA_RX_IRQ_EN_SET		0xD4C
> +#define 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
> +
> +#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_MASK	0x100000
> +#define MI_PBT_FLAG_PAGE_MODE_SHIFT	0x14
> +#define MI_PBT_FLAG_SEGMENT_TYPE_MASK	0x7000000
> +#define MI_PBT_FLAG_SEGMENT_TYPE_SHIFT	0x18
> +#define MI_PBT_FLAG_ACCESS_TYPE_MASK	0xE00000
> +#define MI_PBT_FLAG_ACCESS_TYPE_SHIFT	0x15
> +
> +#define MI_PBT_PAGE_MODE_VALUE(x) \
> +	(((x) & MI_PBT_FLAG_PAGE_MODE_MASK) >> \
> +	  MI_PBT_FLAG_PAGE_MODE_SHIFT)

Please replace masking and shifting with FIELD_GET() from
linux/bitfield.h.

> +
> +#define MI_PBT_SEGMENT_TYPE_VALUE(x) \
> +	(((x) & MI_PBT_FLAG_SEGMENT_TYPE_MASK) >> \
> +		MI_PBT_FLAG_SEGMENT_TYPE_SHIFT)
> +
> +#define MI_PBT_ACCESS_TYPE_VALUE(x) \
> +	(((x) & MI_PBT_FLAG_ACCESS_TYPE_MASK) >> \
> +	  MI_PBT_FLAG_ACCESS_TYPE_SHIFT)
> +
> +/* GENI_FORCE_DEFAULT_REG fields */
> +#define FORCE_DEFAULT			BIT(0)
> +
> +/* FW_REVISION_RO fields */
> +#define FW_REV_PROTOCOL_SHFT		8
> +#define FW_REV_VERSION_SHFT		0
> +
> +#define GENI_FW_REVISION_RO		0x68
> +#define GENI_S_FW_REVISION_RO		0x6C
> +
> +/* SE_GENI_DMA_MODE_EN */
> +#define GENI_DMA_MODE_EN		BIT(0)
> +
> +/* GENI_M_IRQ_EN fields */
> +#define M_CMD_DONE_EN			BIT(0)
> +#define M_IO_DATA_DEASSERT_EN		BIT(22)
> +#define M_IO_DATA_ASSERT_EN		BIT(23)
> +#define M_RX_FIFO_RD_ERR_EN		BIT(24)
> +#define M_RX_FIFO_WR_ERR_EN		BIT(25)
> +#define M_RX_FIFO_WATERMARK_EN		BIT(26)
> +#define M_RX_FIFO_LAST_EN		BIT(27)
> +#define M_TX_FIFO_RD_ERR_EN		BIT(28)
> +#define M_TX_FIFO_WR_ERR_EN		BIT(29)
> +#define M_TX_FIFO_WATERMARK_EN		BIT(30)
> +#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)
> +
> +/* GENI_S_IRQ_EN fields */
> +#define S_CMD_OVERRUN_EN		BIT(1)
> +#define S_ILLEGAL_CMD_EN		BIT(2)
> +#define S_CMD_CANCEL_EN			BIT(4)
> +#define S_CMD_ABORT_EN			BIT(5)
> +#define S_GP_IRQ_0_EN			BIT(9)
> +#define S_GP_IRQ_1_EN			BIT(10)
> +#define S_GP_IRQ_2_EN			BIT(11)
> +#define S_GP_IRQ_3_EN			BIT(12)
> +#define S_RX_FIFO_RD_ERR_EN		BIT(24)
> +#define S_RX_FIFO_WR_ERR_EN		BIT(25)
> +#define S_COMMON_GENI_S_IRQ_EN	(GENMASK(5, 1) | GENMASK(13, 9) | \
> +				 S_RX_FIFO_RD_ERR_EN | S_RX_FIFO_WR_ERR_EN)
> +
> +#define GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK		0x00000200
> +#define GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK		0x00000100
> +
> +#define GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK		0x00000001
> +
> +#define DMA_TX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK	0x00000008
> +#define DMA_TX_IRQ_EN_SET_SBE_EN_SET_BMSK		0x00000004
> +#define DMA_TX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK		0x00000001
> +
> +#define DMA_RX_IRQ_EN_SET_FLUSH_DONE_EN_SET_BMSK	0x00000010
> +#define DMA_RX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK	0x00000008
> +#define DMA_RX_IRQ_EN_SET_SBE_EN_SET_BMSK		0x00000004
> +#define DMA_RX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK		0x00000001
> +
> +#define DMA_GENERAL_CFG_AHB_SEC_SLV_CLK_CGC_ON_BMSK	0x00000008
> +#define DMA_GENERAL_CFG_DMA_AHB_SLV_CLK_CGC_ON_BMSK	0x00000004
> +#define DMA_GENERAL_CFG_DMA_TX_CLK_CGC_ON_BMSK		0x00000002
> +#define DMA_GENERAL_CFG_DMA_RX_CLK_CGC_ON_BMSK		0x00000001
> +
> +#define GENI_CLK_CTRL_SER_CLK_SEL_BMSK			0x00000001
> +#define DMA_IF_EN_DMA_IF_EN_BMSK			0x00000001
> +#define SE_GSI_EVENT_EN_BMSK				0x0000000f
> +#define SE_IRQ_EN_RMSK					0x0000000f
> +
> +#define QUPV3_COMMON_CFG				0x0120
> +#define FAST_SWITCH_TO_HIGH_DISABLE_BMASK		0x00000001
> +
> +#define QUPV3_SE_AHB_M_CFG				0x0118
> +#define AHB_M_CLK_CGC_ON_BMASK				0x00000001
> +
> +#define QUPV3_COMMON_CGC_CTRL				0x021C
> +#define COMMON_CSR_SLV_CLK_CGC_ON_BMASK			0x00000001
> +
> +/* access ports */
> +#define setbits32(_addr, _v) out_be32((_addr), in_be32(_addr) |  (_v))
> +#define clrbits32(_addr, _v) out_be32((_addr), in_be32(_addr) & ~(_v))

These names are way too generic.

> +
> +#define out_be32(a, v) writel_relaxed(v, a)
> +#define in_be32(a) readl_relaxed(a)

Don't create aliases.

> +
> +/**
> + * struct elf_se_hdr - firmware configurations
> + *
> + * @magic: set to 'SEFW'
> + * @version: A 32-bit value indicating the structure’s version number
> + * @core_version: QUPV3_HW_VERSION
> + * @serial_protocol: Programmed into GENI_FW_REVISION
> + * @fw_version: Programmed into GENI_FW_REVISION
> + * @cfg_version: Programmed into GENI_INIT_CFG_REVISION
> + * @fw_size_in_items: Number of (uint32_t) GENI_FW_RAM words
> + * @fw_offset: Byte offset of GENI_FW_RAM array
> + * @cfg_size_in_items: Number of GENI_FW_CFG index/value pairs
> + * @cfg_idx_offset: Byte offset of GENI_FW_CFG index array
> + * @cfg_val_offset: Byte offset of GENI_FW_CFG values array
> + */
> +struct elf_se_hdr {

This structure doesn't seem to relate to ELF at all, it seems to be the
header for the serial engine configuration.

It happens to be packaged up in a ELF segment, but I don't see a reason
for defining the format of the outer container here.

> +	u32 magic;

What endian is that u32? Please be specific.

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 v3 6/9] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem

[Viken Dadhaniya]

On 3/6/2025 4:57 AM, Bjorn Andersson wrote:
> On Mon, Mar 03, 2025 at 06:13:46PM +0530, Viken Dadhaniya wrote:
>> Load the firmware to QUP SE based on the 'firmware-name' property specified
> 
> Please start your commit message with a description of "the problem"
> you're trying to solve, explain that QUP firmware is typically loaded by
> the "bootloader" and that for <reason> you're adding support for loading
> firmware are load time. Please read and follow:
> https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> 
>> in devicetree. Populate Serial engine and base address details in the probe
>> function of the protocol driver and pass to firmware load routine.
> 
> Don't describe the code flow, describe how this fits into the bigger
> picture. E.g. that SE-functional drivers will load this at bootup.
> 
>>
>> Skip the firmware loading if the firmware is already loaded in Serial
>> Engine's firmware memory area.
>>
> 
> Above description should be clear enough that this is obvious.
> 
>> Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> Signed-off-by: Viken Dadhaniya <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      | 423 +++++++++++++++++++++++++++
>>   include/linux/soc/qcom/geni-se.h     |  18 ++
>>   include/linux/soc/qcom/qup-fw-load.h | 179 ++++++++++++
>>   3 files changed, 620 insertions(+)
>>   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..345181ad6fcc 100644
>> --- a/drivers/soc/qcom/qcom-geni-se.c
>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>> @@ -1,5 +1,6 @@
>>   // 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.
> 
> Odd, this should be a multiline comment.
> 
>>   
>>   /* Disable MMIO tracing to prevent excessive logging of unwanted MMIO traces */
>>   #define __DISABLE_TRACE_MMIO__
>> @@ -15,6 +16,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
>> @@ -110,6 +112,9 @@ struct geni_se_desc {
>>   static const char * const icc_path_names[] = {"qup-core", "qup-config",
>>   						"qup-memory"};
>>   
>> +static const char * const protocol_name[] = { "None", "SPI", "UART",
>> +					      "I2C", "I3C", "SPI SLAVE"};
>> +
>>   #define QUP_HW_VER_REG			0x4
>>   
>>   /* Common SE registers */
>> @@ -891,6 +896,424 @@ int geni_icc_disable(struct geni_se *se)
>>   }
>>   EXPORT_SYMBOL_GPL(geni_icc_disable);
>>   
>> +/**
>> + * elf_phdr_valid() - Validate an ELF header.
>> + * @phdr: Pointer to the ELF header.
>> + *
>> + * Validate the ELF header by comparing the fields stored in p_flags and the payload type.
> 
> The interesting piece of information here would be what the definition
> of "valid" is - because you're not checking that the program header
> entry is valid, you're checking that it's a specific valid.
> 
>> + *
>> + * Return: true if the validation is successful, false otherwise.
>> + */
> 
> That said, I think you should just inline these checks in "read_elf()"
> below, preferably with a comment about what you're looking for.
> 
>> +static bool elf_phdr_valid(const struct elf32_phdr *phdr)
>> +{
>> +	if (phdr->p_type != PT_LOAD || !phdr->p_memsz)
>> +		return false;
>> +
>> +	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)
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +/**
>> + * valid_seg_size() - Validate the segment size.
>> + * @pelfseg: Pointer to the ELF header.
>> + * @p_filesz: Pointer to the file size.
>> + *
>> + * Validate the ELF segment size by comparing the file size.
>> + *
>> + * Return: true if the segment is valid, false if the segment is invalid.
>> + */
>> +static bool valid_seg_size(struct elf_se_hdr *pelfseg, Elf32_Word p_filesz)
>> +{
>> +	if (p_filesz >= pelfseg->fw_offset + pelfseg->fw_size_in_items * sizeof(u32) &&
>> +	    p_filesz >= pelfseg->cfg_idx_offset + pelfseg->cfg_size_in_items * sizeof(u8) &&
>> +	    p_filesz >= pelfseg->cfg_val_offset + pelfseg->cfg_size_in_items * sizeof(u32))
>> +		return true;
>> +	return false;
>> +}
>> +
>> +/**
>> + * read_elf() - Read an ELF file.
>> + * @rsc: Pointer to the SE resources structure.
>> + * @fw: Pointer to the firmware buffer.
>> + * @pelfseg: Pointer to the SE-specific ELF header.
>> + * @phdr: Pointer to one of the valid headers from the list in the firmware buffer.
>> + *
>> + * 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 read_elf(struct qup_se_rsc *rsc, const struct firmware *fw,
> 
> I think this name is too generic, please prefix it with "geni_" to make
> it easier to search for.
> 
>> +		    struct elf_se_hdr **pelfseg, struct elf32_phdr **phdr)
>> +{
>> +	const struct elf32_hdr *ehdr = (const struct elf32_hdr *)fw->data;
>> +	struct elf32_phdr *phdrs = (struct elf32_phdr *)(ehdr + 1);
>> +	const u8 *addr;
>> +	int i;
>> +
>> +	ehdr = (struct elf32_hdr *)fw->data;
> 
> Please validate that fw->size is sufficient before accessing - here and
> in the loop.
> 
>> +
>> +	if (ehdr->e_phnum < 2)
>> +		return -EINVAL;
>> +
>> +	for (i = 0; i < ehdr->e_phnum; i++) {
>> +		*phdr = &phdrs[i];
>> +		if (!elf_phdr_valid(*phdr))
>> +			continue;
>> +
>> +		if ((*phdr)->p_filesz >= sizeof(struct elf_se_hdr)) {
>> +			addr =  fw->data + (*phdr)->p_offset;
>> +			*pelfseg = (struct elf_se_hdr *)addr;
> 
> Don't assign *pelfseg until you've found a match.
> 
>> +
>> +			if ((*pelfseg)->magic == MAGIC_NUM_SE &&
>> +			    (*pelfseg)->version == 1 &&
>> +			    valid_seg_size(*pelfseg, (*phdr)->p_filesz) &&
>> +			    (*pelfseg)->serial_protocol == rsc->protocol &&
>> +			    (*pelfseg)->serial_protocol != GENI_SE_NONE)
> 
> There's so much going on in this one conditional. Turn this the way
> around:
> 
> if (phdr->p_filesz < sizeof(struct elf_se_hdr))
> 	continue;
> 
> if (se->magic != MAGIC_NUM_SE)
> 	continue;
> 
> ...
> 
> Makes is super easy to read and each one encapsulates one disqualifying
> factor.
> 
>> +				return 0;
>> +		}
>> +	}
>> +	return -EINVAL;
>> +}
>> +
>> +/**
>> + * geni_config_common_control() - Configure common CGC and disable high priority interrupt.
>> + * @rsc: Pointer to a structure representing SE-related resources.
>> + *
>> + * Configure the common CGC and disable high priority interrupts until the current low priority
>> + * interrupts are handled.
> 
> What do you mean with "low priority interrupt", this is called from the
> firmware loading operation - that doesn't sound like an interrupt at
> all.
> 
> It leaves me with the question about why interrupts should be disabled
> and why they should be disabled in a function called "config common
> control".
> 
> 
> Unless it makes the code easier to read, you should wrap it at 80
> characters. I don't think this sentence is easier to read in it's
> unwrapped form.
> 
>> + *
>> + * Return: None.
> 
> Drop this for void functions.
> 
>> + */
>> +static void geni_config_common_control(struct qup_se_rsc *rsc)
>> +{
>> +	/*
>> +	 * Disable high priority interrupt until current
>> +	 * low priority interrupts are handled.
>> +	 */
>> +	setbits32(rsc->se->wrapper->base + QUPV3_COMMON_CFG,
> 
> I find that the qup_se_rsc structure makes these functions overflow with
> pointer dereferences, and it makes it impossible to know if a function
> operates on 1, 2, or 3 of its members.
> 
> Please just pass the arguments around where necessary.
> 
>> +		  FAST_SWITCH_TO_HIGH_DISABLE_BMASK);
>> +
>> +	/*
>> +	 * Set AHB_M_CLK_CGC_ON to indicate hardware controls
>> +	 * se-wrapper cgc clock.
>> +	 */
>> +	setbits32(rsc->se->wrapper->base + QUPV3_SE_AHB_M_CFG,
>> +		  AHB_M_CLK_CGC_ON_BMASK);
>> +
>> +	/* Let hardware to control common cgc. */
>> +	setbits32(rsc->se->wrapper->base + QUPV3_COMMON_CGC_CTRL,
>> +		  COMMON_CSR_SLV_CLK_CGC_ON_BMASK);
>> +}
>> +
>> +/**
>> + * geni_configure_xfer_mode() - Set the transfer mode.
>> + * @rsc: 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 qup_se_rsc *rsc)
>> +{
>> +	/* Configure SE FIFO, DMA or GSI mode. */
>> +	switch (rsc->mode) {
>> +	case GENI_GPI_DMA:
>> +		setbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
>> +			  GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
>> +		writel_relaxed(0x0, rsc->se->base + SE_IRQ_EN);
>> +		writel_relaxed(SE_GSI_EVENT_EN_BMSK, rsc->se->base + SE_GSI_EVENT_EN);
>> +		break;
>> +
>> +	case GENI_SE_FIFO:
>> +		clrbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
>> +			  GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
>> +		writel_relaxed(SE_IRQ_EN_RMSK, rsc->se->base + SE_IRQ_EN);
>> +		writel_relaxed(0x0, rsc->se->base + SE_GSI_EVENT_EN);
>> +		break;
>> +
>> +	case GENI_SE_DMA:
>> +		setbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
>> +			  GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
>> +		writel_relaxed(SE_IRQ_EN_RMSK, rsc->se->base + SE_IRQ_EN);
>> +		writel_relaxed(0x0, rsc->se->base + SE_GSI_EVENT_EN);
>> +		break;
>> +
>> +	default:
>> +		dev_err(rsc->se->dev, "invalid se mode: %d\n", rsc->mode);
> 
> Please make this error more descriptive so that it's easier for someone
> finding it in the kernel log to understand what the problem is.
> 
>> +		return -EINVAL;
>> +	}
>> +	return 0;
>> +}
>> +
>> +/**
>> + * geni_enable_interrupts() Enable interrupts.
>> + * @rsc: Pointer to a structure representing SE-related resources.
>> + *
>> + * Enable the required interrupts during the firmware load process.
>> + *
>> + * Return: None.
>> + */
>> +static void geni_enable_interrupts(struct qup_se_rsc *rsc)
>> +{
>> +	u32 reg_value;
>> +
>> +	/* Enable required interrupts. */
>> +	writel_relaxed(M_COMMON_GENI_M_IRQ_EN, rsc->se->base + GENI_M_IRQ_ENABLE);
>> +
>> +	reg_value = S_CMD_OVERRUN_EN | S_ILLEGAL_CMD_EN |
>> +				S_CMD_CANCEL_EN | S_CMD_ABORT_EN |
> 
> Please confirm that the indentation is correct here.
> 
>> +				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, rsc->se->base + GENI_S_IRQ_ENABLE);
>> +
>> +	/* DMA mode configuration. */
>> +	reg_value = DMA_TX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK |
>> +		    DMA_TX_IRQ_EN_SET_SBE_EN_SET_BMSK |
>> +		    DMA_TX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK;
>> +	writel_relaxed(reg_value, rsc->se->base + DMA_TX_IRQ_EN_SET);
>> +	reg_value = DMA_RX_IRQ_EN_SET_FLUSH_DONE_EN_SET_BMSK |
>> +		    DMA_RX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK |
>> +		    DMA_RX_IRQ_EN_SET_SBE_EN_SET_BMSK |
>> +		    DMA_RX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK;
>> +	writel_relaxed(reg_value, rsc->se->base + DMA_RX_IRQ_EN_SET);
>> +}
>> +
>> +/**
>> + * geni_flash_fw_revision() - Flash the firmware revision.
> 
> It's not really "flashing", it's more just "writing"...right?
> 
>> + * @rsc: Pointer to a structure representing SE-related resources.
>> + * @hdr: Pointer to the ELF header of the Serial Engine.
> 
> That's not the ELF header, it's the serial engine firmware header. That
> said, you only consume the serial_protocol and fw_version, so a function
> called "geni_write_fw_revision()" could perhaps just take those two
> values as parameters?
> 
>> + *
>> + * Flash the firmware revision and protocol into the respective register.
> 
> Again, no flashing, just writing.
> 
>> + *
>> + * Return: None.
>> + */
>> +static void geni_flash_fw_revision(struct qup_se_rsc *rsc, struct elf_se_hdr *hdr)
>> +{
>> +	u32 reg_value;
>> +
>> +	/* Flash firmware revision register. */
>> +	reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
>> +		    (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
>> +	writel_relaxed(reg_value, rsc->se->base + SE_GENI_FW_REVISION);
>> +
>> +	reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
>> +		    (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
>> +
>> +	writel_relaxed(reg_value, rsc->se->base + SE_S_FW_REVISION);
>> +}
>> +
>> +/**
>> + * geni_load_se_fw() - Load Serial Engine specific firmware.
>> + * @rsc: 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 qup_se_rsc *rsc, const struct firmware *fw)
>> +{
>> +	const u32 *fw_val_arr, *cfg_val_arr;
>> +	const u8 *cfg_idx_arr;
>> +	u32 i, reg_value, mask, ramn_cnt;
>> +	int ret;
>> +	struct elf_se_hdr *hdr;
>> +	struct elf32_phdr *phdr;
>> +
>> +	ret = geni_icc_set_bw(rsc->se);
>> +	if (ret) {
>> +		dev_err(rsc->se->dev, "%s: Failed to set ICC BW %d\n", __func__, ret);
> 
> Drop the __func__ from all your printouts.
> 
>> +		return ret;
>> +	}
>> +
>> +	ret = geni_icc_enable(rsc->se);
>> +	if (ret) {
>> +		dev_err(rsc->se->dev, "%s: Failed to enable ICC %d\n", __func__, ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = geni_se_resources_on(rsc->se);
> 
> Why do you turn on the buses and clocks before you're parsing the
> firmware file?
> 
>> +	if (ret) {
>> +		dev_err(rsc->se->dev, "%s: Failed to enable common clocks %d\n", __func__, ret);
>> +		goto err;
>> +	}
>> +
>> +	ret = read_elf(rsc, fw, &hdr, &phdr);
> 
> Why is phdr returned here? Makes read_elf() harder to read...
> 
>> +	if (ret) {
>> +		dev_err(rsc->se->dev, "%s: ELF parsing failed ret: %d\n", __func__, ret);
>> +		goto err;
>> +	}
>> +
>> +	fw_val_arr = (const u32 *)((u8 *)hdr + hdr->fw_offset);
> 
> Why is this a "firmware value array", isn't it the "firmware data"? Why
> is it chunks of u32? Does it also need to be written in words?
> 
>> +	cfg_idx_arr = (const u8 *)hdr + hdr->cfg_idx_offset;
>> +	cfg_val_arr = (const u32 *)((u8 *)hdr + hdr->cfg_val_offset);
>> +
>> +	geni_config_common_control(rsc);
>> +
>> +	/* Allows to drive corresponding data according to hardware value. */
>> +	writel_relaxed(0x0, rsc->se->base + GENI_OUTPUT_CTRL);
>> +
>> +	/* Set SCLK and HCLK to program RAM */
>> +	setbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
>> +			GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
>> +	writel_relaxed(0x0, rsc->se->base + SE_GENI_CLK_CTRL);
>> +	clrbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
>> +			GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
>> +
>> +	/* Enable required clocks for DMA CSR, TX and RX. */
>> +	reg_value = DMA_GENERAL_CFG_AHB_SEC_SLV_CLK_CGC_ON_BMSK |
>> +		DMA_GENERAL_CFG_DMA_AHB_SLV_CLK_CGC_ON_BMSK |
>> +		DMA_GENERAL_CFG_DMA_TX_CLK_CGC_ON_BMSK |
>> +		DMA_GENERAL_CFG_DMA_RX_CLK_CGC_ON_BMSK;
>> +
>> +	setbits32(rsc->se->base + DMA_GENERAL_CFG, reg_value);
>> +
>> +	/* Let hardware control CGC by default. */
>> +	writel_relaxed(DEFAULT_CGC_EN, rsc->se->base + GENI_CGC_CTRL);
>> +
>> +	/* Set version of the configuration register part of firmware. */
>> +	writel_relaxed(hdr->cfg_version, rsc->se->base + GENI_INIT_CFG_REVISION);
>> +	writel_relaxed(hdr->cfg_version, rsc->se->base + 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],
>> +			       rsc->se->base + GENI_CFG_REG0 + (cfg_idx_arr[i] * sizeof(u32)));
> 
> What goes into this configuration?
> 
>> +
>> +	/* Configure condition for assertion of RX_RFR_WATERMARK condition. */
>> +	reg_value = readl_relaxed(rsc->se->base + QUPV3_SE_HW_PARAM_1);
>> +	mask = (reg_value >> RX_FIFO_WIDTH_BIT) & RX_FIFO_WIDTH_MASK;
> 
> Based on the WIDTH_MASK, those 6 bits doesn't sound like a "BIT"... And
> is the RX_FIFO_WIDTH really a "mask"?
> 
>> +	writel_relaxed(mask - 2, rsc->se->base + GENI_RX_RFR_WATERMARK_REG);
> 
> mask - 2?
> 
> Use FIELD_GET() and name things with the intention of making it easy for
> others to understand what the code does.
> 
>> +
>> +	/* Let hardware control CGC */
>> +	setbits32(rsc->se->base + GENI_OUTPUT_CTRL, DEFAULT_IO_OUTPUT_CTRL_MSK);
>> +
>> +	ret = geni_configure_xfer_mode(rsc);
>> +	if (ret)
>> +		goto err_resource;
>> +
>> +	geni_enable_interrupts(rsc);
>> +
>> +	geni_flash_fw_revision(rsc, hdr);
>> +
>> +	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;
> 
> As far as I can tell this error path is based entirely on the firmware
> being read from the file system, so you could have checked it before you
> turned on any resources - making a cleaner exit on failure.
> 
>> +
>> +	/* Program RAM address space. */
>> +	memcpy((void *)(rsc->se->base + SE_GENI_CFG_RAMN), fw_val_arr, ramn_cnt * sizeof(u32));
> 
> I presume this typecast to void * is to hide the fact that se->base is
> flagged as __iomem? Use memcpy_toio() intead.
> 
>> +
>> +	/* Put default values on GENI's output pads. */
>> +	writel_relaxed(0x1, rsc->se->base + GENI_FORCE_DEFAULT_REG);
>> +
>> +	/* High to low SCLK and HCLK to finish RAM. */
>> +	setbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
>> +			GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
>> +	setbits32(rsc->se->base + SE_GENI_CLK_CTRL, GENI_CLK_CTRL_SER_CLK_SEL_BMSK);
>> +	clrbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
>> +			GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
>> +
>> +	/* Serial engine DMA interface is enabled. */
>> +	setbits32(rsc->se->base + SE_DMA_IF_EN, DMA_IF_EN_DMA_IF_EN_BMSK);
>> +
>> +	/* Enable or disable FIFO interface of the serial engine. */
>> +	if (rsc->mode == GENI_SE_FIFO)
>> +		clrbits32(rsc->se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
>> +	else
>> +		setbits32(rsc->se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
>> +
>> +err_resource:
>> +	geni_se_resources_off(rsc->se);
>> +err:
>> +	geni_icc_disable(rsc->se);
>> +	return ret;
>> +}
>> +
>> +/**
>> + * qup_fw_load() - Initiate firmware load.
>> + * @rsc: Pointer to a structure representing SE-related resources.
>> + *
>> + * 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 qup_se_rsc *rsc, const char *fw_name)
>> +{
>> +	int ret;
>> +	const struct firmware *fw;
>> +	struct device *dev = rsc->se->dev;
>> +
>> +	ret = request_firmware(&fw, fw_name, dev);
>> +	if (ret) {
>> +		dev_err(dev, "request_firmware failed for %d: %d\n", rsc->protocol, ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = geni_load_se_fw(rsc, 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)
> 
> This line is 82 characters long if you don't line break it, but it's
> cleaner to read...so please unbreak it.
> 
>> +{
>> +	struct qup_se_rsc rsc;
>> +	const char *fw_name;
>> +	int ret;
>> +
>> +	ret = device_property_read_string(se->wrapper->dev, "firmware-name", &fw_name);
>> +	if (ret)
>> +		return  -EINVAL;
>> +
>> +	rsc.se = se;
>> +	rsc.protocol = protocol;
>> +	/* Set default xfer mode to FIFO */
> 
> This isn't the "default" mode, this is the else statement in the
> conditional below.
> 
>> +	rsc.mode = GENI_SE_FIFO;
>> +
>> +	if (of_property_read_bool(se->dev->of_node, "qcom,gsi-dma-allowed"))
>> +		rsc.mode = GENI_GPI_DMA;
> 
> if (of_property_present())
> 	sc.mode = GENI_GPI_DMA;
> else
> 	sc.mode = GENI_GPI_FIFO;
> 
> /* Comment about UART, because it's actually worth mentioning */
> if (serial)
> 
> 
>> +
>> +	/* GSI mode is not supported by the UART driver; therefore, setting FIFO mode */
>> +	if (protocol == GENI_SE_UART)
>> +		rsc.mode = GENI_SE_FIFO;
>> +
>> +	ret = qup_fw_load(&rsc, fw_name);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev_dbg(se->dev, "Firmware load for %s protocol is successful for xfer mode %d\n",
>> +		protocol_name[rsc.protocol], rsc.mode);
> 
> How do you know that rsc.protocol < ARRAY_SIZE(protocol_name)?
> 
>> +	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..fd8cf5c6791f 100644
>> --- a/include/linux/soc/qcom/geni-se.h
>> +++ b/include/linux/soc/qcom/geni-se.h
>> @@ -1,6 +1,7 @@
>>   /* SPDX-License-Identifier: GPL-2.0 */
>>   /*
>>    * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2023-2025 Qualcomm Innovation Center, Inc. All rights reserved.
>>    */
>>   
>>   #ifndef _LINUX_QCOM_GENI_SE
>> @@ -36,6 +37,7 @@ enum geni_se_protocol_type {
>>   	GENI_SE_I2C,
>>   	GENI_SE_I3C,
>>   	GENI_SE_SPI_SLAVE,
>> +	GENI_SE_INVALID_PROTO = 255,
> 
> This is unused.
> 
>>   };
>>   
>>   struct geni_wrapper;
>> @@ -72,6 +74,19 @@ struct geni_se {
>>   	struct geni_icc_path icc_paths[3];
>>   };
>>   
>> +/**
>> + * struct qup_se_rsc - Structure containing se details protocol and xfer mode
>> + *
>> + * @mode: transfer mode se fifo, dma or gsi.
>> + * @protocol: Protocol spi or i2c or serial.
>> + * @se: Pointer to the concerned serial engine.
> 
> I'd like you to drop this struct, so it doesn't really matter...but
> order of parameters doesn't match the structure.
> 
>> + */
>> +struct qup_se_rsc {
>> +	struct geni_se *se;
>> +	enum geni_se_xfer_mode mode;
>> +	enum geni_se_protocol_type protocol;
>> +};
>> +
>>   /* Common SE registers */
>>   #define GENI_FORCE_DEFAULT_REG		0x20
>>   #define GENI_OUTPUT_CTRL		0x24
>> @@ -531,5 +546,8 @@ 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);
> 
> Unbreak the line.
> 
>>   #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..f139720987f9
>> --- /dev/null
>> +++ b/include/linux/soc/qcom/qup-fw-load.h
>> @@ -0,0 +1,179 @@
>> +/* 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/device.h>
>> +#include <linux/elf.h>
>> +#include <linux/firmware.h>
> 
> Why are device.h, elf.h and firmware.h included here, they are not used.
> 
>> +#include <linux/kernel.h>
>> +
>> +/*Magic numbers*/
>> +#define MAGIC_NUM_SE			0x57464553
> 
> Prefix things.
> 
>> +
>> +/* Common SE registers*/
>> +#define GENI_INIT_CFG_REVISION		0x0
>> +#define GENI_S_INIT_CFG_REVISION	0x4
>> +#define GENI_FORCE_DEFAULT_REG		0x20
> 
> A bunch of these registers and bitmasks area already defined in
> include/soc/qcom/geni-se.h why are they duplicated here?
> 
>> +#define GENI_CGC_CTRL			0x28
>> +#define GENI_CFG_REG0			0x100
>> +
>> +#define QUPV3_SE_HW_PARAM_1		0xE28
> 
> Lowercase hex digits please.
> 
>> +#define RX_FIFO_WIDTH_BIT		24
>> +#define RX_FIFO_WIDTH_MASK		0x3F
>> +
>> +/*Same registers as GENI_DMA_MODE_EN*/
>> +#define QUPV3_SE_GENI_DMA_MODE_EN	0x258
>> +#define GENI_M_IRQ_ENABLE		0x614
>> +#define GENI_S_IRQ_ENABLE		0x644
>> +#define GENI_RX_RFR_WATERMARK_REG	0x814
>> +#define DMA_TX_IRQ_EN_SET		0xC4C
>> +#define DMA_RX_IRQ_EN_SET		0xD4C
>> +#define 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
>> +
>> +#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_MASK	0x100000
>> +#define MI_PBT_FLAG_PAGE_MODE_SHIFT	0x14
>> +#define MI_PBT_FLAG_SEGMENT_TYPE_MASK	0x7000000
>> +#define MI_PBT_FLAG_SEGMENT_TYPE_SHIFT	0x18
>> +#define MI_PBT_FLAG_ACCESS_TYPE_MASK	0xE00000
>> +#define MI_PBT_FLAG_ACCESS_TYPE_SHIFT	0x15
>> +
>> +#define MI_PBT_PAGE_MODE_VALUE(x) \
>> +	(((x) & MI_PBT_FLAG_PAGE_MODE_MASK) >> \
>> +	  MI_PBT_FLAG_PAGE_MODE_SHIFT)
> 
> Please replace masking and shifting with FIELD_GET() from
> linux/bitfield.h.
> 
>> +
>> +#define MI_PBT_SEGMENT_TYPE_VALUE(x) \
>> +	(((x) & MI_PBT_FLAG_SEGMENT_TYPE_MASK) >> \
>> +		MI_PBT_FLAG_SEGMENT_TYPE_SHIFT)
>> +
>> +#define MI_PBT_ACCESS_TYPE_VALUE(x) \
>> +	(((x) & MI_PBT_FLAG_ACCESS_TYPE_MASK) >> \
>> +	  MI_PBT_FLAG_ACCESS_TYPE_SHIFT)
>> +
>> +/* GENI_FORCE_DEFAULT_REG fields */
>> +#define FORCE_DEFAULT			BIT(0)
>> +
>> +/* FW_REVISION_RO fields */
>> +#define FW_REV_PROTOCOL_SHFT		8
>> +#define FW_REV_VERSION_SHFT		0
>> +
>> +#define GENI_FW_REVISION_RO		0x68
>> +#define GENI_S_FW_REVISION_RO		0x6C
>> +
>> +/* SE_GENI_DMA_MODE_EN */
>> +#define GENI_DMA_MODE_EN		BIT(0)
>> +
>> +/* GENI_M_IRQ_EN fields */
>> +#define M_CMD_DONE_EN			BIT(0)
>> +#define M_IO_DATA_DEASSERT_EN		BIT(22)
>> +#define M_IO_DATA_ASSERT_EN		BIT(23)
>> +#define M_RX_FIFO_RD_ERR_EN		BIT(24)
>> +#define M_RX_FIFO_WR_ERR_EN		BIT(25)
>> +#define M_RX_FIFO_WATERMARK_EN		BIT(26)
>> +#define M_RX_FIFO_LAST_EN		BIT(27)
>> +#define M_TX_FIFO_RD_ERR_EN		BIT(28)
>> +#define M_TX_FIFO_WR_ERR_EN		BIT(29)
>> +#define M_TX_FIFO_WATERMARK_EN		BIT(30)
>> +#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)
>> +
>> +/* GENI_S_IRQ_EN fields */
>> +#define S_CMD_OVERRUN_EN		BIT(1)
>> +#define S_ILLEGAL_CMD_EN		BIT(2)
>> +#define S_CMD_CANCEL_EN			BIT(4)
>> +#define S_CMD_ABORT_EN			BIT(5)
>> +#define S_GP_IRQ_0_EN			BIT(9)
>> +#define S_GP_IRQ_1_EN			BIT(10)
>> +#define S_GP_IRQ_2_EN			BIT(11)
>> +#define S_GP_IRQ_3_EN			BIT(12)
>> +#define S_RX_FIFO_RD_ERR_EN		BIT(24)
>> +#define S_RX_FIFO_WR_ERR_EN		BIT(25)
>> +#define S_COMMON_GENI_S_IRQ_EN	(GENMASK(5, 1) | GENMASK(13, 9) | \
>> +				 S_RX_FIFO_RD_ERR_EN | S_RX_FIFO_WR_ERR_EN)
>> +
>> +#define GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK		0x00000200
>> +#define GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK		0x00000100
>> +
>> +#define GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK		0x00000001
>> +
>> +#define DMA_TX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK	0x00000008
>> +#define DMA_TX_IRQ_EN_SET_SBE_EN_SET_BMSK		0x00000004
>> +#define DMA_TX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK		0x00000001
>> +
>> +#define DMA_RX_IRQ_EN_SET_FLUSH_DONE_EN_SET_BMSK	0x00000010
>> +#define DMA_RX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK	0x00000008
>> +#define DMA_RX_IRQ_EN_SET_SBE_EN_SET_BMSK		0x00000004
>> +#define DMA_RX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK		0x00000001
>> +
>> +#define DMA_GENERAL_CFG_AHB_SEC_SLV_CLK_CGC_ON_BMSK	0x00000008
>> +#define DMA_GENERAL_CFG_DMA_AHB_SLV_CLK_CGC_ON_BMSK	0x00000004
>> +#define DMA_GENERAL_CFG_DMA_TX_CLK_CGC_ON_BMSK		0x00000002
>> +#define DMA_GENERAL_CFG_DMA_RX_CLK_CGC_ON_BMSK		0x00000001
>> +
>> +#define GENI_CLK_CTRL_SER_CLK_SEL_BMSK			0x00000001
>> +#define DMA_IF_EN_DMA_IF_EN_BMSK			0x00000001
>> +#define SE_GSI_EVENT_EN_BMSK				0x0000000f
>> +#define SE_IRQ_EN_RMSK					0x0000000f
>> +
>> +#define QUPV3_COMMON_CFG				0x0120
>> +#define FAST_SWITCH_TO_HIGH_DISABLE_BMASK		0x00000001
>> +
>> +#define QUPV3_SE_AHB_M_CFG				0x0118
>> +#define AHB_M_CLK_CGC_ON_BMASK				0x00000001
>> +
>> +#define QUPV3_COMMON_CGC_CTRL				0x021C
>> +#define COMMON_CSR_SLV_CLK_CGC_ON_BMASK			0x00000001
>> +
>> +/* access ports */
>> +#define setbits32(_addr, _v) out_be32((_addr), in_be32(_addr) |  (_v))
>> +#define clrbits32(_addr, _v) out_be32((_addr), in_be32(_addr) & ~(_v))
> 
> These names are way too generic.
> 
>> +
>> +#define out_be32(a, v) writel_relaxed(v, a)
>> +#define in_be32(a) readl_relaxed(a)
> 
> Don't create aliases.
> 
>> +
>> +/**
>> + * struct elf_se_hdr - firmware configurations
>> + *
>> + * @magic: set to 'SEFW'
>> + * @version: A 32-bit value indicating the structure’s version number
>> + * @core_version: QUPV3_HW_VERSION
>> + * @serial_protocol: Programmed into GENI_FW_REVISION
>> + * @fw_version: Programmed into GENI_FW_REVISION
>> + * @cfg_version: Programmed into GENI_INIT_CFG_REVISION
>> + * @fw_size_in_items: Number of (uint32_t) GENI_FW_RAM words
>> + * @fw_offset: Byte offset of GENI_FW_RAM array
>> + * @cfg_size_in_items: Number of GENI_FW_CFG index/value pairs
>> + * @cfg_idx_offset: Byte offset of GENI_FW_CFG index array
>> + * @cfg_val_offset: Byte offset of GENI_FW_CFG values array
>> + */
>> +struct elf_se_hdr {
> 
> This structure doesn't seem to relate to ELF at all, it seems to be the
> header for the serial engine configuration.
> 
> It happens to be packaged up in a ELF segment, but I don't see a reason
> for defining the format of the outer container here.

This structure is related to firmware written in an ELF file. I didn't 
understand your concern. Could you please clarify what is required here?

> 
>> +	u32 magic;
> 
> What endian is that u32? Please be specific.
> 
> 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 v3 6/9] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem

[Viken Dadhaniya]

On 4/22/2025 12:02 PM, Viken Dadhaniya wrote:
> 
> 
> On 3/6/2025 4:57 AM, Bjorn Andersson wrote:
>> On Mon, Mar 03, 2025 at 06:13:46PM +0530, Viken Dadhaniya wrote:
>>> Load the firmware to QUP SE based on the 'firmware-name' property 
>>> specified
>>
>> Please start your commit message with a description of "the problem"
>> you're trying to solve, explain that QUP firmware is typically loaded by
>> the "bootloader" and that for <reason> you're adding support for loading
>> firmware are load time. Please read and follow:
>> https://docs.kernel.org/process/submitting-patches.html#describe-your- 
>> changes
>>
>>> in devicetree. Populate Serial engine and base address details in the 
>>> probe
>>> function of the protocol driver and pass to firmware load routine.
>>
>> Don't describe the code flow, describe how this fits into the bigger
>> picture. E.g. that SE-functional drivers will load this at bootup.
>>
>>>
>>> Skip the firmware loading if the firmware is already loaded in Serial
>>> Engine's firmware memory area.
>>>
>>
>> Above description should be clear enough that this is obvious.
>>
>>> Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>>> Signed-off-by: Viken Dadhaniya <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      | 423 +++++++++++++++++++++++++++
>>>   include/linux/soc/qcom/geni-se.h     |  18 ++
>>>   include/linux/soc/qcom/qup-fw-load.h | 179 ++++++++++++
>>>   3 files changed, 620 insertions(+)
>>>   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..345181ad6fcc 100644
>>> --- a/drivers/soc/qcom/qcom-geni-se.c
>>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>>> @@ -1,5 +1,6 @@
>>>   // 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.
>>
>> Odd, this should be a multiline comment.
>>
>>>   /* Disable MMIO tracing to prevent excessive logging of unwanted 
>>> MMIO traces */
>>>   #define __DISABLE_TRACE_MMIO__
>>> @@ -15,6 +16,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
>>> @@ -110,6 +112,9 @@ struct geni_se_desc {
>>>   static const char * const icc_path_names[] = {"qup-core", "qup- 
>>> config",
>>>                           "qup-memory"};
>>> +static const char * const protocol_name[] = { "None", "SPI", "UART",
>>> +                          "I2C", "I3C", "SPI SLAVE"};
>>> +
>>>   #define QUP_HW_VER_REG            0x4
>>>   /* Common SE registers */
>>> @@ -891,6 +896,424 @@ int geni_icc_disable(struct geni_se *se)
>>>   }
>>>   EXPORT_SYMBOL_GPL(geni_icc_disable);
>>> +/**
>>> + * elf_phdr_valid() - Validate an ELF header.
>>> + * @phdr: Pointer to the ELF header.
>>> + *
>>> + * Validate the ELF header by comparing the fields stored in p_flags 
>>> and the payload type.
>>
>> The interesting piece of information here would be what the definition
>> of "valid" is - because you're not checking that the program header
>> entry is valid, you're checking that it's a specific valid.
>>
>>> + *
>>> + * Return: true if the validation is successful, false otherwise.
>>> + */
>>
>> That said, I think you should just inline these checks in "read_elf()"
>> below, preferably with a comment about what you're looking for.
>>
>>> +static bool elf_phdr_valid(const struct elf32_phdr *phdr)
>>> +{
>>> +    if (phdr->p_type != PT_LOAD || !phdr->p_memsz)
>>> +        return false;
>>> +
>>> +    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)
>>> +        return true;
>>> +
>>> +    return false;
>>> +}
>>> +
>>> +/**
>>> + * valid_seg_size() - Validate the segment size.
>>> + * @pelfseg: Pointer to the ELF header.
>>> + * @p_filesz: Pointer to the file size.
>>> + *
>>> + * Validate the ELF segment size by comparing the file size.
>>> + *
>>> + * Return: true if the segment is valid, false if the segment is 
>>> invalid.
>>> + */
>>> +static bool valid_seg_size(struct elf_se_hdr *pelfseg, Elf32_Word 
>>> p_filesz)
>>> +{
>>> +    if (p_filesz >= pelfseg->fw_offset + pelfseg->fw_size_in_items * 
>>> sizeof(u32) &&
>>> +        p_filesz >= pelfseg->cfg_idx_offset + pelfseg- 
>>> >cfg_size_in_items * sizeof(u8) &&
>>> +        p_filesz >= pelfseg->cfg_val_offset + pelfseg- 
>>> >cfg_size_in_items * sizeof(u32))
>>> +        return true;
>>> +    return false;
>>> +}
>>> +
>>> +/**
>>> + * read_elf() - Read an ELF file.
>>> + * @rsc: Pointer to the SE resources structure.
>>> + * @fw: Pointer to the firmware buffer.
>>> + * @pelfseg: Pointer to the SE-specific ELF header.
>>> + * @phdr: Pointer to one of the valid headers from the list in the 
>>> firmware buffer.
>>> + *
>>> + * 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 read_elf(struct qup_se_rsc *rsc, const struct firmware *fw,
>>
>> I think this name is too generic, please prefix it with "geni_" to make
>> it easier to search for.
>>
>>> +            struct elf_se_hdr **pelfseg, struct elf32_phdr **phdr)
>>> +{
>>> +    const struct elf32_hdr *ehdr = (const struct elf32_hdr *)fw->data;
>>> +    struct elf32_phdr *phdrs = (struct elf32_phdr *)(ehdr + 1);
>>> +    const u8 *addr;
>>> +    int i;
>>> +
>>> +    ehdr = (struct elf32_hdr *)fw->data;
>>
>> Please validate that fw->size is sufficient before accessing - here and
>> in the loop.
>>
>>> +
>>> +    if (ehdr->e_phnum < 2)
>>> +        return -EINVAL;
>>> +
>>> +    for (i = 0; i < ehdr->e_phnum; i++) {
>>> +        *phdr = &phdrs[i];
>>> +        if (!elf_phdr_valid(*phdr))
>>> +            continue;
>>> +
>>> +        if ((*phdr)->p_filesz >= sizeof(struct elf_se_hdr)) {
>>> +            addr =  fw->data + (*phdr)->p_offset;
>>> +            *pelfseg = (struct elf_se_hdr *)addr;
>>
>> Don't assign *pelfseg until you've found a match.
>>
>>> +
>>> +            if ((*pelfseg)->magic == MAGIC_NUM_SE &&
>>> +                (*pelfseg)->version == 1 &&
>>> +                valid_seg_size(*pelfseg, (*phdr)->p_filesz) &&
>>> +                (*pelfseg)->serial_protocol == rsc->protocol &&
>>> +                (*pelfseg)->serial_protocol != GENI_SE_NONE)
>>
>> There's so much going on in this one conditional. Turn this the way
>> around:
>>
>> if (phdr->p_filesz < sizeof(struct elf_se_hdr))
>>     continue;
>>
>> if (se->magic != MAGIC_NUM_SE)
>>     continue;
>>
>> ...
>>
>> Makes is super easy to read and each one encapsulates one disqualifying
>> factor.
>>
>>> +                return 0;
>>> +        }
>>> +    }
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +/**
>>> + * geni_config_common_control() - Configure common CGC and disable 
>>> high priority interrupt.
>>> + * @rsc: Pointer to a structure representing SE-related resources.
>>> + *
>>> + * Configure the common CGC and disable high priority interrupts 
>>> until the current low priority
>>> + * interrupts are handled.
>>
>> What do you mean with "low priority interrupt", this is called from the
>> firmware loading operation - that doesn't sound like an interrupt at
>> all.
>>
>> It leaves me with the question about why interrupts should be disabled
>> and why they should be disabled in a function called "config common
>> control".
>>
>>
>> Unless it makes the code easier to read, you should wrap it at 80
>> characters. I don't think this sentence is easier to read in it's
>> unwrapped form.
>>
>>> + *
>>> + * Return: None.
>>
>> Drop this for void functions.
>>
>>> + */
>>> +static void geni_config_common_control(struct qup_se_rsc *rsc)
>>> +{
>>> +    /*
>>> +     * Disable high priority interrupt until current
>>> +     * low priority interrupts are handled.
>>> +     */
>>> +    setbits32(rsc->se->wrapper->base + QUPV3_COMMON_CFG,
>>
>> I find that the qup_se_rsc structure makes these functions overflow with
>> pointer dereferences, and it makes it impossible to know if a function
>> operates on 1, 2, or 3 of its members.
>>
>> Please just pass the arguments around where necessary.
>>
>>> +          FAST_SWITCH_TO_HIGH_DISABLE_BMASK);
>>> +
>>> +    /*
>>> +     * Set AHB_M_CLK_CGC_ON to indicate hardware controls
>>> +     * se-wrapper cgc clock.
>>> +     */
>>> +    setbits32(rsc->se->wrapper->base + QUPV3_SE_AHB_M_CFG,
>>> +          AHB_M_CLK_CGC_ON_BMASK);
>>> +
>>> +    /* Let hardware to control common cgc. */
>>> +    setbits32(rsc->se->wrapper->base + QUPV3_COMMON_CGC_CTRL,
>>> +          COMMON_CSR_SLV_CLK_CGC_ON_BMASK);
>>> +}
>>> +
>>> +/**
>>> + * geni_configure_xfer_mode() - Set the transfer mode.
>>> + * @rsc: 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 qup_se_rsc *rsc)
>>> +{
>>> +    /* Configure SE FIFO, DMA or GSI mode. */
>>> +    switch (rsc->mode) {
>>> +    case GENI_GPI_DMA:
>>> +        setbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
>>> +              GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
>>> +        writel_relaxed(0x0, rsc->se->base + SE_IRQ_EN);
>>> +        writel_relaxed(SE_GSI_EVENT_EN_BMSK, rsc->se->base + 
>>> SE_GSI_EVENT_EN);
>>> +        break;
>>> +
>>> +    case GENI_SE_FIFO:
>>> +        clrbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
>>> +              GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
>>> +        writel_relaxed(SE_IRQ_EN_RMSK, rsc->se->base + SE_IRQ_EN);
>>> +        writel_relaxed(0x0, rsc->se->base + SE_GSI_EVENT_EN);
>>> +        break;
>>> +
>>> +    case GENI_SE_DMA:
>>> +        setbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
>>> +              GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
>>> +        writel_relaxed(SE_IRQ_EN_RMSK, rsc->se->base + SE_IRQ_EN);
>>> +        writel_relaxed(0x0, rsc->se->base + SE_GSI_EVENT_EN);
>>> +        break;
>>> +
>>> +    default:
>>> +        dev_err(rsc->se->dev, "invalid se mode: %d\n", rsc->mode);
>>
>> Please make this error more descriptive so that it's easier for someone
>> finding it in the kernel log to understand what the problem is.
>>
>>> +        return -EINVAL;
>>> +    }
>>> +    return 0;
>>> +}
>>> +
>>> +/**
>>> + * geni_enable_interrupts() Enable interrupts.
>>> + * @rsc: Pointer to a structure representing SE-related resources.
>>> + *
>>> + * Enable the required interrupts during the firmware load process.
>>> + *
>>> + * Return: None.
>>> + */
>>> +static void geni_enable_interrupts(struct qup_se_rsc *rsc)
>>> +{
>>> +    u32 reg_value;
>>> +
>>> +    /* Enable required interrupts. */
>>> +    writel_relaxed(M_COMMON_GENI_M_IRQ_EN, rsc->se->base + 
>>> GENI_M_IRQ_ENABLE);
>>> +
>>> +    reg_value = S_CMD_OVERRUN_EN | S_ILLEGAL_CMD_EN |
>>> +                S_CMD_CANCEL_EN | S_CMD_ABORT_EN |
>>
>> Please confirm that the indentation is correct here.
>>
>>> +                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, rsc->se->base + GENI_S_IRQ_ENABLE);
>>> +
>>> +    /* DMA mode configuration. */
>>> +    reg_value = DMA_TX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK |
>>> +            DMA_TX_IRQ_EN_SET_SBE_EN_SET_BMSK |
>>> +            DMA_TX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK;
>>> +    writel_relaxed(reg_value, rsc->se->base + DMA_TX_IRQ_EN_SET);
>>> +    reg_value = DMA_RX_IRQ_EN_SET_FLUSH_DONE_EN_SET_BMSK |
>>> +            DMA_RX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK |
>>> +            DMA_RX_IRQ_EN_SET_SBE_EN_SET_BMSK |
>>> +            DMA_RX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK;
>>> +    writel_relaxed(reg_value, rsc->se->base + DMA_RX_IRQ_EN_SET);
>>> +}
>>> +
>>> +/**
>>> + * geni_flash_fw_revision() - Flash the firmware revision.
>>
>> It's not really "flashing", it's more just "writing"...right?
>>
>>> + * @rsc: Pointer to a structure representing SE-related resources.
>>> + * @hdr: Pointer to the ELF header of the Serial Engine.
>>
>> That's not the ELF header, it's the serial engine firmware header. That
>> said, you only consume the serial_protocol and fw_version, so a function
>> called "geni_write_fw_revision()" could perhaps just take those two
>> values as parameters?
>>
>>> + *
>>> + * Flash the firmware revision and protocol into the respective 
>>> register.
>>
>> Again, no flashing, just writing.
>>
>>> + *
>>> + * Return: None.
>>> + */
>>> +static void geni_flash_fw_revision(struct qup_se_rsc *rsc, struct 
>>> elf_se_hdr *hdr)
>>> +{
>>> +    u32 reg_value;
>>> +
>>> +    /* Flash firmware revision register. */
>>> +    reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
>>> +            (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
>>> +    writel_relaxed(reg_value, rsc->se->base + SE_GENI_FW_REVISION);
>>> +
>>> +    reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
>>> +            (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
>>> +
>>> +    writel_relaxed(reg_value, rsc->se->base + SE_S_FW_REVISION);
>>> +}
>>> +
>>> +/**
>>> + * geni_load_se_fw() - Load Serial Engine specific firmware.
>>> + * @rsc: 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 qup_se_rsc *rsc, const struct 
>>> firmware *fw)
>>> +{
>>> +    const u32 *fw_val_arr, *cfg_val_arr;
>>> +    const u8 *cfg_idx_arr;
>>> +    u32 i, reg_value, mask, ramn_cnt;
>>> +    int ret;
>>> +    struct elf_se_hdr *hdr;
>>> +    struct elf32_phdr *phdr;
>>> +
>>> +    ret = geni_icc_set_bw(rsc->se);
>>> +    if (ret) {
>>> +        dev_err(rsc->se->dev, "%s: Failed to set ICC BW %d\n", 
>>> __func__, ret);
>>
>> Drop the __func__ from all your printouts.
>>
>>> +        return ret;
>>> +    }
>>> +
>>> +    ret = geni_icc_enable(rsc->se);
>>> +    if (ret) {
>>> +        dev_err(rsc->se->dev, "%s: Failed to enable ICC %d\n", 
>>> __func__, ret);
>>> +        return ret;
>>> +    }
>>> +
>>> +    ret = geni_se_resources_on(rsc->se);
>>
>> Why do you turn on the buses and clocks before you're parsing the
>> firmware file?
>>
>>> +    if (ret) {
>>> +        dev_err(rsc->se->dev, "%s: Failed to enable common clocks 
>>> %d\n", __func__, ret);
>>> +        goto err;
>>> +    }
>>> +
>>> +    ret = read_elf(rsc, fw, &hdr, &phdr);
>>
>> Why is phdr returned here? Makes read_elf() harder to read...
>>
>>> +    if (ret) {
>>> +        dev_err(rsc->se->dev, "%s: ELF parsing failed ret: %d\n", 
>>> __func__, ret);
>>> +        goto err;
>>> +    }
>>> +
>>> +    fw_val_arr = (const u32 *)((u8 *)hdr + hdr->fw_offset);
>>
>> Why is this a "firmware value array", isn't it the "firmware data"? Why
>> is it chunks of u32? Does it also need to be written in words?
>>
>>> +    cfg_idx_arr = (const u8 *)hdr + hdr->cfg_idx_offset;
>>> +    cfg_val_arr = (const u32 *)((u8 *)hdr + hdr->cfg_val_offset);
>>> +
>>> +    geni_config_common_control(rsc);
>>> +
>>> +    /* Allows to drive corresponding data according to hardware 
>>> value. */
>>> +    writel_relaxed(0x0, rsc->se->base + GENI_OUTPUT_CTRL);
>>> +
>>> +    /* Set SCLK and HCLK to program RAM */
>>> +    setbits32(rsc->se->base + GENI_CGC_CTRL, 
>>> GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
>>> +            GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
>>> +    writel_relaxed(0x0, rsc->se->base + SE_GENI_CLK_CTRL);
>>> +    clrbits32(rsc->se->base + GENI_CGC_CTRL, 
>>> GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
>>> +            GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
>>> +
>>> +    /* Enable required clocks for DMA CSR, TX and RX. */
>>> +    reg_value = DMA_GENERAL_CFG_AHB_SEC_SLV_CLK_CGC_ON_BMSK |
>>> +        DMA_GENERAL_CFG_DMA_AHB_SLV_CLK_CGC_ON_BMSK |
>>> +        DMA_GENERAL_CFG_DMA_TX_CLK_CGC_ON_BMSK |
>>> +        DMA_GENERAL_CFG_DMA_RX_CLK_CGC_ON_BMSK;
>>> +
>>> +    setbits32(rsc->se->base + DMA_GENERAL_CFG, reg_value);
>>> +
>>> +    /* Let hardware control CGC by default. */
>>> +    writel_relaxed(DEFAULT_CGC_EN, rsc->se->base + GENI_CGC_CTRL);
>>> +
>>> +    /* Set version of the configuration register part of firmware. */
>>> +    writel_relaxed(hdr->cfg_version, rsc->se->base + 
>>> GENI_INIT_CFG_REVISION);
>>> +    writel_relaxed(hdr->cfg_version, rsc->se->base + 
>>> 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],
>>> +                   rsc->se->base + GENI_CFG_REG0 + (cfg_idx_arr[i] * 
>>> sizeof(u32)));
>>
>> What goes into this configuration?
>>
>>> +
>>> +    /* Configure condition for assertion of RX_RFR_WATERMARK 
>>> condition. */
>>> +    reg_value = readl_relaxed(rsc->se->base + QUPV3_SE_HW_PARAM_1);
>>> +    mask = (reg_value >> RX_FIFO_WIDTH_BIT) & RX_FIFO_WIDTH_MASK;
>>
>> Based on the WIDTH_MASK, those 6 bits doesn't sound like a "BIT"... And
>> is the RX_FIFO_WIDTH really a "mask"?
>>
>>> +    writel_relaxed(mask - 2, rsc->se->base + 
>>> GENI_RX_RFR_WATERMARK_REG);
>>
>> mask - 2?
>>
>> Use FIELD_GET() and name things with the intention of making it easy for
>> others to understand what the code does.
>>
>>> +
>>> +    /* Let hardware control CGC */
>>> +    setbits32(rsc->se->base + GENI_OUTPUT_CTRL, 
>>> DEFAULT_IO_OUTPUT_CTRL_MSK);
>>> +
>>> +    ret = geni_configure_xfer_mode(rsc);
>>> +    if (ret)
>>> +        goto err_resource;
>>> +
>>> +    geni_enable_interrupts(rsc);
>>> +
>>> +    geni_flash_fw_revision(rsc, hdr);
>>> +
>>> +    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;
>>
>> As far as I can tell this error path is based entirely on the firmware
>> being read from the file system, so you could have checked it before you
>> turned on any resources - making a cleaner exit on failure.
>>
>>> +
>>> +    /* Program RAM address space. */
>>> +    memcpy((void *)(rsc->se->base + SE_GENI_CFG_RAMN), fw_val_arr, 
>>> ramn_cnt * sizeof(u32));
>>
>> I presume this typecast to void * is to hide the fact that se->base is
>> flagged as __iomem? Use memcpy_toio() intead.
>>
>>> +
>>> +    /* Put default values on GENI's output pads. */
>>> +    writel_relaxed(0x1, rsc->se->base + GENI_FORCE_DEFAULT_REG);
>>> +
>>> +    /* High to low SCLK and HCLK to finish RAM. */
>>> +    setbits32(rsc->se->base + GENI_CGC_CTRL, 
>>> GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
>>> +            GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
>>> +    setbits32(rsc->se->base + SE_GENI_CLK_CTRL, 
>>> GENI_CLK_CTRL_SER_CLK_SEL_BMSK);
>>> +    clrbits32(rsc->se->base + GENI_CGC_CTRL, 
>>> GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
>>> +            GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
>>> +
>>> +    /* Serial engine DMA interface is enabled. */
>>> +    setbits32(rsc->se->base + SE_DMA_IF_EN, DMA_IF_EN_DMA_IF_EN_BMSK);
>>> +
>>> +    /* Enable or disable FIFO interface of the serial engine. */
>>> +    if (rsc->mode == GENI_SE_FIFO)
>>> +        clrbits32(rsc->se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
>>> +    else
>>> +        setbits32(rsc->se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
>>> +
>>> +err_resource:
>>> +    geni_se_resources_off(rsc->se);
>>> +err:
>>> +    geni_icc_disable(rsc->se);
>>> +    return ret;
>>> +}
>>> +
>>> +/**
>>> + * qup_fw_load() - Initiate firmware load.
>>> + * @rsc: Pointer to a structure representing SE-related resources.
>>> + *
>>> + * 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 qup_se_rsc *rsc, const char *fw_name)
>>> +{
>>> +    int ret;
>>> +    const struct firmware *fw;
>>> +    struct device *dev = rsc->se->dev;
>>> +
>>> +    ret = request_firmware(&fw, fw_name, dev);
>>> +    if (ret) {
>>> +        dev_err(dev, "request_firmware failed for %d: %d\n", rsc- 
>>> >protocol, ret);
>>> +        return ret;
>>> +    }
>>> +
>>> +    ret = geni_load_se_fw(rsc, 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)
>>
>> This line is 82 characters long if you don't line break it, but it's
>> cleaner to read...so please unbreak it.
>>
>>> +{
>>> +    struct qup_se_rsc rsc;
>>> +    const char *fw_name;
>>> +    int ret;
>>> +
>>> +    ret = device_property_read_string(se->wrapper->dev, "firmware- 
>>> name", &fw_name);
>>> +    if (ret)
>>> +        return  -EINVAL;
>>> +
>>> +    rsc.se = se;
>>> +    rsc.protocol = protocol;
>>> +    /* Set default xfer mode to FIFO */
>>
>> This isn't the "default" mode, this is the else statement in the
>> conditional below.
>>
>>> +    rsc.mode = GENI_SE_FIFO;
>>> +
>>> +    if (of_property_read_bool(se->dev->of_node, "qcom,gsi-dma- 
>>> allowed"))
>>> +        rsc.mode = GENI_GPI_DMA;
>>
>> if (of_property_present())
>>     sc.mode = GENI_GPI_DMA;
>> else
>>     sc.mode = GENI_GPI_FIFO;
>>
>> /* Comment about UART, because it's actually worth mentioning */
>> if (serial)
>>
>>
>>> +
>>> +    /* GSI mode is not supported by the UART driver; therefore, 
>>> setting FIFO mode */
>>> +    if (protocol == GENI_SE_UART)
>>> +        rsc.mode = GENI_SE_FIFO;
>>> +
>>> +    ret = qup_fw_load(&rsc, fw_name);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    dev_dbg(se->dev, "Firmware load for %s protocol is successful 
>>> for xfer mode %d\n",
>>> +        protocol_name[rsc.protocol], rsc.mode);
>>
>> How do you know that rsc.protocol < ARRAY_SIZE(protocol_name)?
>>
>>> +    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..fd8cf5c6791f 100644
>>> --- a/include/linux/soc/qcom/geni-se.h
>>> +++ b/include/linux/soc/qcom/geni-se.h
>>> @@ -1,6 +1,7 @@
>>>   /* SPDX-License-Identifier: GPL-2.0 */
>>>   /*
>>>    * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>>> + * Copyright (c) 2023-2025 Qualcomm Innovation Center, Inc. All 
>>> rights reserved.
>>>    */
>>>   #ifndef _LINUX_QCOM_GENI_SE
>>> @@ -36,6 +37,7 @@ enum geni_se_protocol_type {
>>>       GENI_SE_I2C,
>>>       GENI_SE_I3C,
>>>       GENI_SE_SPI_SLAVE,
>>> +    GENI_SE_INVALID_PROTO = 255,
>>
>> This is unused.
>>
>>>   };
>>>   struct geni_wrapper;
>>> @@ -72,6 +74,19 @@ struct geni_se {
>>>       struct geni_icc_path icc_paths[3];
>>>   };
>>> +/**
>>> + * struct qup_se_rsc - Structure containing se details protocol and 
>>> xfer mode
>>> + *
>>> + * @mode: transfer mode se fifo, dma or gsi.
>>> + * @protocol: Protocol spi or i2c or serial.
>>> + * @se: Pointer to the concerned serial engine.
>>
>> I'd like you to drop this struct, so it doesn't really matter...but
>> order of parameters doesn't match the structure.
>>
>>> + */
>>> +struct qup_se_rsc {
>>> +    struct geni_se *se;
>>> +    enum geni_se_xfer_mode mode;
>>> +    enum geni_se_protocol_type protocol;
>>> +};
>>> +
>>>   /* Common SE registers */
>>>   #define GENI_FORCE_DEFAULT_REG        0x20
>>>   #define GENI_OUTPUT_CTRL        0x24
>>> @@ -531,5 +546,8 @@ 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);
>>
>> Unbreak the line.
>>
>>>   #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..f139720987f9
>>> --- /dev/null
>>> +++ b/include/linux/soc/qcom/qup-fw-load.h
>>> @@ -0,0 +1,179 @@
>>> +/* 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/device.h>
>>> +#include <linux/elf.h>
>>> +#include <linux/firmware.h>
>>
>> Why are device.h, elf.h and firmware.h included here, they are not used.
>>
>>> +#include <linux/kernel.h>
>>> +
>>> +/*Magic numbers*/
>>> +#define MAGIC_NUM_SE            0x57464553
>>
>> Prefix things.
>>
>>> +
>>> +/* Common SE registers*/
>>> +#define GENI_INIT_CFG_REVISION        0x0
>>> +#define GENI_S_INIT_CFG_REVISION    0x4
>>> +#define GENI_FORCE_DEFAULT_REG        0x20
>>
>> A bunch of these registers and bitmasks area already defined in
>> include/soc/qcom/geni-se.h why are they duplicated here?
>>
>>> +#define GENI_CGC_CTRL            0x28
>>> +#define GENI_CFG_REG0            0x100
>>> +
>>> +#define QUPV3_SE_HW_PARAM_1        0xE28
>>
>> Lowercase hex digits please.
>>
>>> +#define RX_FIFO_WIDTH_BIT        24
>>> +#define RX_FIFO_WIDTH_MASK        0x3F
>>> +
>>> +/*Same registers as GENI_DMA_MODE_EN*/
>>> +#define QUPV3_SE_GENI_DMA_MODE_EN    0x258
>>> +#define GENI_M_IRQ_ENABLE        0x614
>>> +#define GENI_S_IRQ_ENABLE        0x644
>>> +#define GENI_RX_RFR_WATERMARK_REG    0x814
>>> +#define DMA_TX_IRQ_EN_SET        0xC4C
>>> +#define DMA_RX_IRQ_EN_SET        0xD4C
>>> +#define 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
>>> +
>>> +#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_MASK    0x100000
>>> +#define MI_PBT_FLAG_PAGE_MODE_SHIFT    0x14
>>> +#define MI_PBT_FLAG_SEGMENT_TYPE_MASK    0x7000000
>>> +#define MI_PBT_FLAG_SEGMENT_TYPE_SHIFT    0x18
>>> +#define MI_PBT_FLAG_ACCESS_TYPE_MASK    0xE00000
>>> +#define MI_PBT_FLAG_ACCESS_TYPE_SHIFT    0x15
>>> +
>>> +#define MI_PBT_PAGE_MODE_VALUE(x) \
>>> +    (((x) & MI_PBT_FLAG_PAGE_MODE_MASK) >> \
>>> +      MI_PBT_FLAG_PAGE_MODE_SHIFT)
>>
>> Please replace masking and shifting with FIELD_GET() from
>> linux/bitfield.h.
>>
>>> +
>>> +#define MI_PBT_SEGMENT_TYPE_VALUE(x) \
>>> +    (((x) & MI_PBT_FLAG_SEGMENT_TYPE_MASK) >> \
>>> +        MI_PBT_FLAG_SEGMENT_TYPE_SHIFT)
>>> +
>>> +#define MI_PBT_ACCESS_TYPE_VALUE(x) \
>>> +    (((x) & MI_PBT_FLAG_ACCESS_TYPE_MASK) >> \
>>> +      MI_PBT_FLAG_ACCESS_TYPE_SHIFT)
>>> +
>>> +/* GENI_FORCE_DEFAULT_REG fields */
>>> +#define FORCE_DEFAULT            BIT(0)
>>> +
>>> +/* FW_REVISION_RO fields */
>>> +#define FW_REV_PROTOCOL_SHFT        8
>>> +#define FW_REV_VERSION_SHFT        0
>>> +
>>> +#define GENI_FW_REVISION_RO        0x68
>>> +#define GENI_S_FW_REVISION_RO        0x6C
>>> +
>>> +/* SE_GENI_DMA_MODE_EN */
>>> +#define GENI_DMA_MODE_EN        BIT(0)
>>> +
>>> +/* GENI_M_IRQ_EN fields */
>>> +#define M_CMD_DONE_EN            BIT(0)
>>> +#define M_IO_DATA_DEASSERT_EN        BIT(22)
>>> +#define M_IO_DATA_ASSERT_EN        BIT(23)
>>> +#define M_RX_FIFO_RD_ERR_EN        BIT(24)
>>> +#define M_RX_FIFO_WR_ERR_EN        BIT(25)
>>> +#define M_RX_FIFO_WATERMARK_EN        BIT(26)
>>> +#define M_RX_FIFO_LAST_EN        BIT(27)
>>> +#define M_TX_FIFO_RD_ERR_EN        BIT(28)
>>> +#define M_TX_FIFO_WR_ERR_EN        BIT(29)
>>> +#define M_TX_FIFO_WATERMARK_EN        BIT(30)
>>> +#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)
>>> +
>>> +/* GENI_S_IRQ_EN fields */
>>> +#define S_CMD_OVERRUN_EN        BIT(1)
>>> +#define S_ILLEGAL_CMD_EN        BIT(2)
>>> +#define S_CMD_CANCEL_EN            BIT(4)
>>> +#define S_CMD_ABORT_EN            BIT(5)
>>> +#define S_GP_IRQ_0_EN            BIT(9)
>>> +#define S_GP_IRQ_1_EN            BIT(10)
>>> +#define S_GP_IRQ_2_EN            BIT(11)
>>> +#define S_GP_IRQ_3_EN            BIT(12)
>>> +#define S_RX_FIFO_RD_ERR_EN        BIT(24)
>>> +#define S_RX_FIFO_WR_ERR_EN        BIT(25)
>>> +#define S_COMMON_GENI_S_IRQ_EN    (GENMASK(5, 1) | GENMASK(13, 9) | \
>>> +                 S_RX_FIFO_RD_ERR_EN | S_RX_FIFO_WR_ERR_EN)
>>> +
>>> +#define GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK        0x00000200
>>> +#define GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK        0x00000100
>>> +
>>> +#define GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK        0x00000001
>>> +
>>> +#define DMA_TX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK    0x00000008
>>> +#define DMA_TX_IRQ_EN_SET_SBE_EN_SET_BMSK        0x00000004
>>> +#define DMA_TX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK        0x00000001
>>> +
>>> +#define DMA_RX_IRQ_EN_SET_FLUSH_DONE_EN_SET_BMSK    0x00000010
>>> +#define DMA_RX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK    0x00000008
>>> +#define DMA_RX_IRQ_EN_SET_SBE_EN_SET_BMSK        0x00000004
>>> +#define DMA_RX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK        0x00000001
>>> +
>>> +#define DMA_GENERAL_CFG_AHB_SEC_SLV_CLK_CGC_ON_BMSK    0x00000008
>>> +#define DMA_GENERAL_CFG_DMA_AHB_SLV_CLK_CGC_ON_BMSK    0x00000004
>>> +#define DMA_GENERAL_CFG_DMA_TX_CLK_CGC_ON_BMSK        0x00000002
>>> +#define DMA_GENERAL_CFG_DMA_RX_CLK_CGC_ON_BMSK        0x00000001
>>> +
>>> +#define GENI_CLK_CTRL_SER_CLK_SEL_BMSK            0x00000001
>>> +#define DMA_IF_EN_DMA_IF_EN_BMSK            0x00000001
>>> +#define SE_GSI_EVENT_EN_BMSK                0x0000000f
>>> +#define SE_IRQ_EN_RMSK                    0x0000000f
>>> +
>>> +#define QUPV3_COMMON_CFG                0x0120
>>> +#define FAST_SWITCH_TO_HIGH_DISABLE_BMASK        0x00000001
>>> +
>>> +#define QUPV3_SE_AHB_M_CFG                0x0118
>>> +#define AHB_M_CLK_CGC_ON_BMASK                0x00000001
>>> +
>>> +#define QUPV3_COMMON_CGC_CTRL                0x021C
>>> +#define COMMON_CSR_SLV_CLK_CGC_ON_BMASK            0x00000001
>>> +
>>> +/* access ports */
>>> +#define setbits32(_addr, _v) out_be32((_addr), in_be32(_addr) |  (_v))
>>> +#define clrbits32(_addr, _v) out_be32((_addr), in_be32(_addr) & ~(_v))
>>
>> These names are way too generic.
>>
>>> +
>>> +#define out_be32(a, v) writel_relaxed(v, a)
>>> +#define in_be32(a) readl_relaxed(a)
>>
>> Don't create aliases.
>>
>>> +
>>> +/**
>>> + * struct elf_se_hdr - firmware configurations
>>> + *
>>> + * @magic: set to 'SEFW'
>>> + * @version: A 32-bit value indicating the structure’s version number
>>> + * @core_version: QUPV3_HW_VERSION
>>> + * @serial_protocol: Programmed into GENI_FW_REVISION
>>> + * @fw_version: Programmed into GENI_FW_REVISION
>>> + * @cfg_version: Programmed into GENI_INIT_CFG_REVISION
>>> + * @fw_size_in_items: Number of (uint32_t) GENI_FW_RAM words
>>> + * @fw_offset: Byte offset of GENI_FW_RAM array
>>> + * @cfg_size_in_items: Number of GENI_FW_CFG index/value pairs
>>> + * @cfg_idx_offset: Byte offset of GENI_FW_CFG index array
>>> + * @cfg_val_offset: Byte offset of GENI_FW_CFG values array
>>> + */
>>> +struct elf_se_hdr {
>>
>> This structure doesn't seem to relate to ELF at all, it seems to be the
>> header for the serial engine configuration.
>>
>> It happens to be packaged up in a ELF segment, but I don't see a reason
>> for defining the format of the outer container here.
> 
> This structure is related to firmware written in an ELF file. I didn't 
> understand your concern. Could you please clarify what is required here?
> 

If I understand your comments correctly, the current struct name does 
not align with its functionality. shall i  update it to 
se_fw_config_hdr. Hope that matches the ELF as per functionality.

>>
>>> +    u32 magic;
>>
>> What endian is that u32? Please be specific.
>>
>> 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 v3 6/9] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem

[Viken Dadhaniya]

On 3/6/2025 4:57 AM, Bjorn Andersson wrote:
> On Mon, Mar 03, 2025 at 06:13:46PM +0530, Viken Dadhaniya wrote:
>> Load the firmware to QUP SE based on the 'firmware-name' property specified
> 
> Please start your commit message with a description of "the problem"
> you're trying to solve, explain that QUP firmware is typically loaded by
> the "bootloader" and that for <reason> you're adding support for loading
> firmware are load time. Please read and follow:
> https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> 
>> in devicetree. Populate Serial engine and base address details in the probe
>> function of the protocol driver and pass to firmware load routine.
> 
> Don't describe the code flow, describe how this fits into the bigger
> picture. E.g. that SE-functional drivers will load this at bootup.
> 

Sure, Updated in v4.

>>
>> Skip the firmware loading if the firmware is already loaded in Serial
>> Engine's firmware memory area.
>>
> 
> Above description should be clear enough that this is obvious.

Sure, Removed in v4.

> 
>> Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> Signed-off-by: Viken Dadhaniya <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      | 423 +++++++++++++++++++++++++++
>>   include/linux/soc/qcom/geni-se.h     |  18 ++
>>   include/linux/soc/qcom/qup-fw-load.h | 179 ++++++++++++
>>   3 files changed, 620 insertions(+)
>>   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..345181ad6fcc 100644
>> --- a/drivers/soc/qcom/qcom-geni-se.c
>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>> @@ -1,5 +1,6 @@
>>   // 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.
> 
> Odd, this should be a multiline comment.

Sure, Updated in v4.

> 
>>   
>>   /* Disable MMIO tracing to prevent excessive logging of unwanted MMIO traces */
>>   #define __DISABLE_TRACE_MMIO__
>> @@ -15,6 +16,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
>> @@ -110,6 +112,9 @@ struct geni_se_desc {
>>   static const char * const icc_path_names[] = {"qup-core", "qup-config",
>>   						"qup-memory"};
>>   
>> +static const char * const protocol_name[] = { "None", "SPI", "UART",
>> +					      "I2C", "I3C", "SPI SLAVE"};
>> +
>>   #define QUP_HW_VER_REG			0x4
>>   
>>   /* Common SE registers */
>> @@ -891,6 +896,424 @@ int geni_icc_disable(struct geni_se *se)
>>   }
>>   EXPORT_SYMBOL_GPL(geni_icc_disable);
>>   
>> +/**
>> + * elf_phdr_valid() - Validate an ELF header.
>> + * @phdr: Pointer to the ELF header.
>> + *
>> + * Validate the ELF header by comparing the fields stored in p_flags and the payload type.
> 
> The interesting piece of information here would be what the definition
> of "valid" is - because you're not checking that the program header
> entry is valid, you're checking that it's a specific valid.

Sure, Updated in v4.

> 
>> + *
>> + * Return: true if the validation is successful, false otherwise.
>> + */
> 
> That said, I think you should just inline these checks in "read_elf()"
> below, preferably with a comment about what you're looking for.

Sure, Updated in v4.

> 
>> +static bool elf_phdr_valid(const struct elf32_phdr *phdr)
>> +{
>> +	if (phdr->p_type != PT_LOAD || !phdr->p_memsz)
>> +		return false;
>> +
>> +	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)
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +/**
>> + * valid_seg_size() - Validate the segment size.
>> + * @pelfseg: Pointer to the ELF header.
>> + * @p_filesz: Pointer to the file size.
>> + *
>> + * Validate the ELF segment size by comparing the file size.
>> + *
>> + * Return: true if the segment is valid, false if the segment is invalid.
>> + */
>> +static bool valid_seg_size(struct elf_se_hdr *pelfseg, Elf32_Word p_filesz)
>> +{
>> +	if (p_filesz >= pelfseg->fw_offset + pelfseg->fw_size_in_items * sizeof(u32) &&
>> +	    p_filesz >= pelfseg->cfg_idx_offset + pelfseg->cfg_size_in_items * sizeof(u8) &&
>> +	    p_filesz >= pelfseg->cfg_val_offset + pelfseg->cfg_size_in_items * sizeof(u32))
>> +		return true;
>> +	return false;
>> +}
>> +
>> +/**
>> + * read_elf() - Read an ELF file.
>> + * @rsc: Pointer to the SE resources structure.
>> + * @fw: Pointer to the firmware buffer.
>> + * @pelfseg: Pointer to the SE-specific ELF header.
>> + * @phdr: Pointer to one of the valid headers from the list in the firmware buffer.
>> + *
>> + * 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 read_elf(struct qup_se_rsc *rsc, const struct firmware *fw,
> 
> I think this name is too generic, please prefix it with "geni_" to make
> it easier to search for.

Sure, Updated in v4.

> 
>> +		    struct elf_se_hdr **pelfseg, struct elf32_phdr **phdr)
>> +{
>> +	const struct elf32_hdr *ehdr = (const struct elf32_hdr *)fw->data;
>> +	struct elf32_phdr *phdrs = (struct elf32_phdr *)(ehdr + 1);
>> +	const u8 *addr;
>> +	int i;
>> +
>> +	ehdr = (struct elf32_hdr *)fw->data;
> 
> Please validate that fw->size is sufficient before accessing - here and
> in the loop.

Sure, Updated in v4.

> 
>> +
>> +	if (ehdr->e_phnum < 2)
>> +		return -EINVAL;
>> +
>> +	for (i = 0; i < ehdr->e_phnum; i++) {
>> +		*phdr = &phdrs[i];
>> +		if (!elf_phdr_valid(*phdr))
>> +			continue;
>> +
>> +		if ((*phdr)->p_filesz >= sizeof(struct elf_se_hdr)) {
>> +			addr =  fw->data + (*phdr)->p_offset;
>> +			*pelfseg = (struct elf_se_hdr *)addr;
> 
> Don't assign *pelfseg until you've found a match.

Sure, Updated in v4.

> 
>> +
>> +			if ((*pelfseg)->magic == MAGIC_NUM_SE &&
>> +			    (*pelfseg)->version == 1 &&
>> +			    valid_seg_size(*pelfseg, (*phdr)->p_filesz) &&
>> +			    (*pelfseg)->serial_protocol == rsc->protocol &&
>> +			    (*pelfseg)->serial_protocol != GENI_SE_NONE)
> 
> There's so much going on in this one conditional. Turn this the way
> around:
> 
> if (phdr->p_filesz < sizeof(struct elf_se_hdr))
> 	continue;
> 
> if (se->magic != MAGIC_NUM_SE)
> 	continue;
> 
> ...
> 
> Makes is super easy to read and each one encapsulates one disqualifying
> factor.
> 

Sure, Updated in v4.

>> +				return 0;
>> +		}
>> +	}
>> +	return -EINVAL;
>> +}
>> +
>> +/**
>> + * geni_config_common_control() - Configure common CGC and disable high priority interrupt.
>> + * @rsc: Pointer to a structure representing SE-related resources.
>> + *
>> + * Configure the common CGC and disable high priority interrupts until the current low priority
>> + * interrupts are handled.
> 
> What do you mean with "low priority interrupt", this is called from the
> firmware loading operation - that doesn't sound like an interrupt at
> all.
> 
> It leaves me with the question about why interrupts should be disabled
> and why they should be disabled in a function called "config common
> control".
> 
> 
> Unless it makes the code easier to read, you should wrap it at 80
> characters. I don't think this sentence is easier to read in it's
> unwrapped form.
> 

Sure, Updated in v4.

>> + *
>> + * Return: None.
> 
> Drop this for void functions.

Sure, Updated in v4.

> 
>> + */
>> +static void geni_config_common_control(struct qup_se_rsc *rsc)
>> +{
>> +	/*
>> +	 * Disable high priority interrupt until current
>> +	 * low priority interrupts are handled.
>> +	 */
>> +	setbits32(rsc->se->wrapper->base + QUPV3_COMMON_CFG,
> 
> I find that the qup_se_rsc structure makes these functions overflow with
> pointer dereferences, and it makes it impossible to know if a function
> operates on 1, 2, or 3 of its members.
> 
> Please just pass the arguments around where necessary.

Sure, Updated in v4.

> 
>> +		  FAST_SWITCH_TO_HIGH_DISABLE_BMASK);
>> +
>> +	/*
>> +	 * Set AHB_M_CLK_CGC_ON to indicate hardware controls
>> +	 * se-wrapper cgc clock.
>> +	 */
>> +	setbits32(rsc->se->wrapper->base + QUPV3_SE_AHB_M_CFG,
>> +		  AHB_M_CLK_CGC_ON_BMASK);
>> +
>> +	/* Let hardware to control common cgc. */
>> +	setbits32(rsc->se->wrapper->base + QUPV3_COMMON_CGC_CTRL,
>> +		  COMMON_CSR_SLV_CLK_CGC_ON_BMASK);
>> +}
>> +
>> +/**
>> + * geni_configure_xfer_mode() - Set the transfer mode.
>> + * @rsc: 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 qup_se_rsc *rsc)
>> +{
>> +	/* Configure SE FIFO, DMA or GSI mode. */
>> +	switch (rsc->mode) {
>> +	case GENI_GPI_DMA:
>> +		setbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
>> +			  GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
>> +		writel_relaxed(0x0, rsc->se->base + SE_IRQ_EN);
>> +		writel_relaxed(SE_GSI_EVENT_EN_BMSK, rsc->se->base + SE_GSI_EVENT_EN);
>> +		break;
>> +
>> +	case GENI_SE_FIFO:
>> +		clrbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
>> +			  GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
>> +		writel_relaxed(SE_IRQ_EN_RMSK, rsc->se->base + SE_IRQ_EN);
>> +		writel_relaxed(0x0, rsc->se->base + SE_GSI_EVENT_EN);
>> +		break;
>> +
>> +	case GENI_SE_DMA:
>> +		setbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
>> +			  GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
>> +		writel_relaxed(SE_IRQ_EN_RMSK, rsc->se->base + SE_IRQ_EN);
>> +		writel_relaxed(0x0, rsc->se->base + SE_GSI_EVENT_EN);
>> +		break;
>> +
>> +	default:
>> +		dev_err(rsc->se->dev, "invalid se mode: %d\n", rsc->mode);
> 
> Please make this error more descriptive so that it's easier for someone
> finding it in the kernel log to understand what the problem is.

Sure, Updated in v4.

> 
>> +		return -EINVAL;
>> +	}
>> +	return 0;
>> +}
>> +
>> +/**
>> + * geni_enable_interrupts() Enable interrupts.
>> + * @rsc: Pointer to a structure representing SE-related resources.
>> + *
>> + * Enable the required interrupts during the firmware load process.
>> + *
>> + * Return: None.
>> + */
>> +static void geni_enable_interrupts(struct qup_se_rsc *rsc)
>> +{
>> +	u32 reg_value;
>> +
>> +	/* Enable required interrupts. */
>> +	writel_relaxed(M_COMMON_GENI_M_IRQ_EN, rsc->se->base + GENI_M_IRQ_ENABLE);
>> +
>> +	reg_value = S_CMD_OVERRUN_EN | S_ILLEGAL_CMD_EN |
>> +				S_CMD_CANCEL_EN | S_CMD_ABORT_EN |
> 
> Please confirm that the indentation is correct here.

Sure, Updated in v4.

> 
>> +				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, rsc->se->base + GENI_S_IRQ_ENABLE);
>> +
>> +	/* DMA mode configuration. */
>> +	reg_value = DMA_TX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK |
>> +		    DMA_TX_IRQ_EN_SET_SBE_EN_SET_BMSK |
>> +		    DMA_TX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK;
>> +	writel_relaxed(reg_value, rsc->se->base + DMA_TX_IRQ_EN_SET);
>> +	reg_value = DMA_RX_IRQ_EN_SET_FLUSH_DONE_EN_SET_BMSK |
>> +		    DMA_RX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK |
>> +		    DMA_RX_IRQ_EN_SET_SBE_EN_SET_BMSK |
>> +		    DMA_RX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK;
>> +	writel_relaxed(reg_value, rsc->se->base + DMA_RX_IRQ_EN_SET);
>> +}
>> +
>> +/**
>> + * geni_flash_fw_revision() - Flash the firmware revision.
> 
> It's not really "flashing", it's more just "writing"...right?
> 
>> + * @rsc: Pointer to a structure representing SE-related resources.
>> + * @hdr: Pointer to the ELF header of the Serial Engine.
> 
> That's not the ELF header, it's the serial engine firmware header. That
> said, you only consume the serial_protocol and fw_version, so a function
> called "geni_write_fw_revision()" could perhaps just take those two
> values as parameters?

Sure, Updated in v4.

> 
>> + *
>> + * Flash the firmware revision and protocol into the respective register.
> 
> Again, no flashing, just writing.

Sure, Updated in v4.

> 
>> + *
>> + * Return: None.
>> + */
>> +static void geni_flash_fw_revision(struct qup_se_rsc *rsc, struct elf_se_hdr *hdr)
>> +{
>> +	u32 reg_value;
>> +
>> +	/* Flash firmware revision register. */
>> +	reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
>> +		    (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
>> +	writel_relaxed(reg_value, rsc->se->base + SE_GENI_FW_REVISION);
>> +
>> +	reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
>> +		    (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
>> +
>> +	writel_relaxed(reg_value, rsc->se->base + SE_S_FW_REVISION);
>> +}
>> +
>> +/**
>> + * geni_load_se_fw() - Load Serial Engine specific firmware.
>> + * @rsc: 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 qup_se_rsc *rsc, const struct firmware *fw)
>> +{
>> +	const u32 *fw_val_arr, *cfg_val_arr;
>> +	const u8 *cfg_idx_arr;
>> +	u32 i, reg_value, mask, ramn_cnt;
>> +	int ret;
>> +	struct elf_se_hdr *hdr;
>> +	struct elf32_phdr *phdr;
>> +
>> +	ret = geni_icc_set_bw(rsc->se);
>> +	if (ret) {
>> +		dev_err(rsc->se->dev, "%s: Failed to set ICC BW %d\n", __func__, ret);
> 
> Drop the __func__ from all your printouts.

Sure, Updated in v4.

> 
>> +		return ret;
>> +	}
>> +
>> +	ret = geni_icc_enable(rsc->se);
>> +	if (ret) {
>> +		dev_err(rsc->se->dev, "%s: Failed to enable ICC %d\n", __func__, ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = geni_se_resources_on(rsc->se);
> 
> Why do you turn on the buses and clocks before you're parsing the
> firmware file?

Not reqruied before parsing, Updated in v4.

> 
>> +	if (ret) {
>> +		dev_err(rsc->se->dev, "%s: Failed to enable common clocks %d\n", __func__, ret);
>> +		goto err;
>> +	}
>> +
>> +	ret = read_elf(rsc, fw, &hdr, &phdr);
> 
> Why is phdr returned here? Makes read_elf() harder to read...

Not requried, Updated in v4.

> 
>> +	if (ret) {
>> +		dev_err(rsc->se->dev, "%s: ELF parsing failed ret: %d\n", __func__, ret);
>> +		goto err;
>> +	}
>> +
>> +	fw_val_arr = (const u32 *)((u8 *)hdr + hdr->fw_offset);
> 
> Why is this a "firmware value array", isn't it the "firmware data"? Why
> is it chunks of u32? Does it also need to be written in words?

Sure, Updated in v4.
Yes, it is 32 bit register.

> 
>> +	cfg_idx_arr = (const u8 *)hdr + hdr->cfg_idx_offset;
>> +	cfg_val_arr = (const u32 *)((u8 *)hdr + hdr->cfg_val_offset);
>> +
>> +	geni_config_common_control(rsc);
>> +
>> +	/* Allows to drive corresponding data according to hardware value. */
>> +	writel_relaxed(0x0, rsc->se->base + GENI_OUTPUT_CTRL);
>> +
>> +	/* Set SCLK and HCLK to program RAM */
>> +	setbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
>> +			GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
>> +	writel_relaxed(0x0, rsc->se->base + SE_GENI_CLK_CTRL);
>> +	clrbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
>> +			GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
>> +
>> +	/* Enable required clocks for DMA CSR, TX and RX. */
>> +	reg_value = DMA_GENERAL_CFG_AHB_SEC_SLV_CLK_CGC_ON_BMSK |
>> +		DMA_GENERAL_CFG_DMA_AHB_SLV_CLK_CGC_ON_BMSK |
>> +		DMA_GENERAL_CFG_DMA_TX_CLK_CGC_ON_BMSK |
>> +		DMA_GENERAL_CFG_DMA_RX_CLK_CGC_ON_BMSK;
>> +
>> +	setbits32(rsc->se->base + DMA_GENERAL_CFG, reg_value);
>> +
>> +	/* Let hardware control CGC by default. */
>> +	writel_relaxed(DEFAULT_CGC_EN, rsc->se->base + GENI_CGC_CTRL);
>> +
>> +	/* Set version of the configuration register part of firmware. */
>> +	writel_relaxed(hdr->cfg_version, rsc->se->base + GENI_INIT_CFG_REVISION);
>> +	writel_relaxed(hdr->cfg_version, rsc->se->base + 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],
>> +			       rsc->se->base + GENI_CFG_REG0 + (cfg_idx_arr[i] * sizeof(u32)));
> 
> What goes into this configuration?

This is primitive data table for each Serial engine.
> 
>> +
>> +	/* Configure condition for assertion of RX_RFR_WATERMARK condition. */
>> +	reg_value = readl_relaxed(rsc->se->base + QUPV3_SE_HW_PARAM_1);
>> +	mask = (reg_value >> RX_FIFO_WIDTH_BIT) & RX_FIFO_WIDTH_MASK;
> 
> Based on the WIDTH_MASK, those 6 bits doesn't sound like a "BIT"... And
> is the RX_FIFO_WIDTH really a "mask"?
> 
>> +	writel_relaxed(mask - 2, rsc->se->base + GENI_RX_RFR_WATERMARK_REG);
> 
> mask - 2?
> 
> Use FIELD_GET() and name things with the intention of making it easy for
> others to understand what the code does.

Sure, use FIELD_GET in v4.
-2 as suggested in the hardware programming guide to avoid latencies in 
data transfer.

> 
>> +
>> +	/* Let hardware control CGC */
>> +	setbits32(rsc->se->base + GENI_OUTPUT_CTRL, DEFAULT_IO_OUTPUT_CTRL_MSK);
>> +
>> +	ret = geni_configure_xfer_mode(rsc);
>> +	if (ret)
>> +		goto err_resource;
>> +
>> +	geni_enable_interrupts(rsc);
>> +
>> +	geni_flash_fw_revision(rsc, hdr);
>> +
>> +	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;
> 
> As far as I can tell this error path is based entirely on the firmware
> being read from the file system, so you could have checked it before you
> turned on any resources - making a cleaner exit on failure.

Sure, updated in v4.
> 
>> +
>> +	/* Program RAM address space. */
>> +	memcpy((void *)(rsc->se->base + SE_GENI_CFG_RAMN), fw_val_arr, ramn_cnt * sizeof(u32));
> 
> I presume this typecast to void * is to hide the fact that se->base is
> flagged as __iomem? Use memcpy_toio() intead.

Sure, updated in v4.

> 
>> +
>> +	/* Put default values on GENI's output pads. */
>> +	writel_relaxed(0x1, rsc->se->base + GENI_FORCE_DEFAULT_REG);
>> +
>> +	/* High to low SCLK and HCLK to finish RAM. */
>> +	setbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
>> +			GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
>> +	setbits32(rsc->se->base + SE_GENI_CLK_CTRL, GENI_CLK_CTRL_SER_CLK_SEL_BMSK);
>> +	clrbits32(rsc->se->base + GENI_CGC_CTRL, GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK |
>> +			GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK);
>> +
>> +	/* Serial engine DMA interface is enabled. */
>> +	setbits32(rsc->se->base + SE_DMA_IF_EN, DMA_IF_EN_DMA_IF_EN_BMSK);
>> +
>> +	/* Enable or disable FIFO interface of the serial engine. */
>> +	if (rsc->mode == GENI_SE_FIFO)
>> +		clrbits32(rsc->se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
>> +	else
>> +		setbits32(rsc->se->base + SE_FIFO_IF_DISABLE, FIFO_IF_DISABLE);
>> +
>> +err_resource:
>> +	geni_se_resources_off(rsc->se);
>> +err:
>> +	geni_icc_disable(rsc->se);
>> +	return ret;
>> +}
>> +
>> +/**
>> + * qup_fw_load() - Initiate firmware load.
>> + * @rsc: Pointer to a structure representing SE-related resources.
>> + *
>> + * 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 qup_se_rsc *rsc, const char *fw_name)
>> +{
>> +	int ret;
>> +	const struct firmware *fw;
>> +	struct device *dev = rsc->se->dev;
>> +
>> +	ret = request_firmware(&fw, fw_name, dev);
>> +	if (ret) {
>> +		dev_err(dev, "request_firmware failed for %d: %d\n", rsc->protocol, ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = geni_load_se_fw(rsc, 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)
> 
> This line is 82 characters long if you don't line break it, but it's
> cleaner to read...so please unbreak it.
> 
Sure, updated in v4.

>> +{
>> +	struct qup_se_rsc rsc;
>> +	const char *fw_name;
>> +	int ret;
>> +
>> +	ret = device_property_read_string(se->wrapper->dev, "firmware-name", &fw_name);
>> +	if (ret)
>> +		return  -EINVAL;
>> +
>> +	rsc.se = se;
>> +	rsc.protocol = protocol;
>> +	/* Set default xfer mode to FIFO */
> 
> This isn't the "default" mode, this is the else statement in the
> conditional below.

Sure, updated in v4.

> 
>> +	rsc.mode = GENI_SE_FIFO;
>> +
>> +	if (of_property_read_bool(se->dev->of_node, "qcom,gsi-dma-allowed"))
>> +		rsc.mode = GENI_GPI_DMA;
> 
> if (of_property_present())
> 	sc.mode = GENI_GPI_DMA;
> else
> 	sc.mode = GENI_GPI_FIFO;
> 
> /* Comment about UART, because it's actually worth mentioning */
> if (serial)

Sure, updated in v4.
> 
> 
>> +
>> +	/* GSI mode is not supported by the UART driver; therefore, setting FIFO mode */
>> +	if (protocol == GENI_SE_UART)
>> +		rsc.mode = GENI_SE_FIFO;
>> +
>> +	ret = qup_fw_load(&rsc, fw_name);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev_dbg(se->dev, "Firmware load for %s protocol is successful for xfer mode %d\n",
>> +		protocol_name[rsc.protocol], rsc.mode);
> 
> How do you know that rsc.protocol < ARRAY_SIZE(protocol_name)?

Added check for same in v4.

> 
>> +	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..fd8cf5c6791f 100644
>> --- a/include/linux/soc/qcom/geni-se.h
>> +++ b/include/linux/soc/qcom/geni-se.h
>> @@ -1,6 +1,7 @@
>>   /* SPDX-License-Identifier: GPL-2.0 */
>>   /*
>>    * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2023-2025 Qualcomm Innovation Center, Inc. All rights reserved.
>>    */
>>   
>>   #ifndef _LINUX_QCOM_GENI_SE
>> @@ -36,6 +37,7 @@ enum geni_se_protocol_type {
>>   	GENI_SE_I2C,
>>   	GENI_SE_I3C,
>>   	GENI_SE_SPI_SLAVE,
>> +	GENI_SE_INVALID_PROTO = 255,
> 
> This is unused.

This is usded in protocol driver in next patchsets.

> 
>>   };
>>   
>>   struct geni_wrapper;
>> @@ -72,6 +74,19 @@ struct geni_se {
>>   	struct geni_icc_path icc_paths[3];
>>   };
>>   
>> +/**
>> + * struct qup_se_rsc - Structure containing se details protocol and xfer mode
>> + *
>> + * @mode: transfer mode se fifo, dma or gsi.
>> + * @protocol: Protocol spi or i2c or serial.
>> + * @se: Pointer to the concerned serial engine.
> 
> I'd like you to drop this struct, so it doesn't really matter...but
> order of parameters doesn't match the structure.

Sure, updated in v4.

> 
>> + */
>> +struct qup_se_rsc {
>> +	struct geni_se *se;
>> +	enum geni_se_xfer_mode mode;
>> +	enum geni_se_protocol_type protocol;
>> +};
>> +
>>   /* Common SE registers */
>>   #define GENI_FORCE_DEFAULT_REG		0x20
>>   #define GENI_OUTPUT_CTRL		0x24
>> @@ -531,5 +546,8 @@ 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);
> 
> Unbreak the line.

Sure, updated in v4.

> 
>>   #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..f139720987f9
>> --- /dev/null
>> +++ b/include/linux/soc/qcom/qup-fw-load.h
>> @@ -0,0 +1,179 @@
>> +/* 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/device.h>
>> +#include <linux/elf.h>
>> +#include <linux/firmware.h>
> 
> Why are device.h, elf.h and firmware.h included here, they are not used.

Sure, updated in v4.

> 
>> +#include <linux/kernel.h>
>> +
>> +/*Magic numbers*/
>> +#define MAGIC_NUM_SE			0x57464553
> 
> Prefix things.

This is the fix number to verify the correct firmware.

> 
>> +
>> +/* Common SE registers*/
>> +#define GENI_INIT_CFG_REVISION		0x0
>> +#define GENI_S_INIT_CFG_REVISION	0x4
>> +#define GENI_FORCE_DEFAULT_REG		0x20
> 
> A bunch of these registers and bitmasks area already defined in
> include/soc/qcom/geni-se.h why are they duplicated here?

Sure, Removed duplicated marcos in v4.

> 
>> +#define GENI_CGC_CTRL			0x28
>> +#define GENI_CFG_REG0			0x100
>> +
>> +#define QUPV3_SE_HW_PARAM_1		0xE28
> 
> Lowercase hex digits please.

Sure, updated in v4.

> 
>> +#define RX_FIFO_WIDTH_BIT		24
>> +#define RX_FIFO_WIDTH_MASK		0x3F
>> +
>> +/*Same registers as GENI_DMA_MODE_EN*/
>> +#define QUPV3_SE_GENI_DMA_MODE_EN	0x258
>> +#define GENI_M_IRQ_ENABLE		0x614
>> +#define GENI_S_IRQ_ENABLE		0x644
>> +#define GENI_RX_RFR_WATERMARK_REG	0x814
>> +#define DMA_TX_IRQ_EN_SET		0xC4C
>> +#define DMA_RX_IRQ_EN_SET		0xD4C
>> +#define 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
>> +
>> +#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_MASK	0x100000
>> +#define MI_PBT_FLAG_PAGE_MODE_SHIFT	0x14
>> +#define MI_PBT_FLAG_SEGMENT_TYPE_MASK	0x7000000
>> +#define MI_PBT_FLAG_SEGMENT_TYPE_SHIFT	0x18
>> +#define MI_PBT_FLAG_ACCESS_TYPE_MASK	0xE00000
>> +#define MI_PBT_FLAG_ACCESS_TYPE_SHIFT	0x15
>> +
>> +#define MI_PBT_PAGE_MODE_VALUE(x) \
>> +	(((x) & MI_PBT_FLAG_PAGE_MODE_MASK) >> \
>> +	  MI_PBT_FLAG_PAGE_MODE_SHIFT)
> 
> Please replace masking and shifting with FIELD_GET() from
> linux/bitfield.h.

Sure, updated in v4.

> 
>> +
>> +#define MI_PBT_SEGMENT_TYPE_VALUE(x) \
>> +	(((x) & MI_PBT_FLAG_SEGMENT_TYPE_MASK) >> \
>> +		MI_PBT_FLAG_SEGMENT_TYPE_SHIFT)
>> +
>> +#define MI_PBT_ACCESS_TYPE_VALUE(x) \
>> +	(((x) & MI_PBT_FLAG_ACCESS_TYPE_MASK) >> \
>> +	  MI_PBT_FLAG_ACCESS_TYPE_SHIFT)
>> +
>> +/* GENI_FORCE_DEFAULT_REG fields */
>> +#define FORCE_DEFAULT			BIT(0)
>> +
>> +/* FW_REVISION_RO fields */
>> +#define FW_REV_PROTOCOL_SHFT		8
>> +#define FW_REV_VERSION_SHFT		0
>> +
>> +#define GENI_FW_REVISION_RO		0x68
>> +#define GENI_S_FW_REVISION_RO		0x6C
>> +
>> +/* SE_GENI_DMA_MODE_EN */
>> +#define GENI_DMA_MODE_EN		BIT(0)
>> +
>> +/* GENI_M_IRQ_EN fields */
>> +#define M_CMD_DONE_EN			BIT(0)
>> +#define M_IO_DATA_DEASSERT_EN		BIT(22)
>> +#define M_IO_DATA_ASSERT_EN		BIT(23)
>> +#define M_RX_FIFO_RD_ERR_EN		BIT(24)
>> +#define M_RX_FIFO_WR_ERR_EN		BIT(25)
>> +#define M_RX_FIFO_WATERMARK_EN		BIT(26)
>> +#define M_RX_FIFO_LAST_EN		BIT(27)
>> +#define M_TX_FIFO_RD_ERR_EN		BIT(28)
>> +#define M_TX_FIFO_WR_ERR_EN		BIT(29)
>> +#define M_TX_FIFO_WATERMARK_EN		BIT(30)
>> +#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)
>> +
>> +/* GENI_S_IRQ_EN fields */
>> +#define S_CMD_OVERRUN_EN		BIT(1)
>> +#define S_ILLEGAL_CMD_EN		BIT(2)
>> +#define S_CMD_CANCEL_EN			BIT(4)
>> +#define S_CMD_ABORT_EN			BIT(5)
>> +#define S_GP_IRQ_0_EN			BIT(9)
>> +#define S_GP_IRQ_1_EN			BIT(10)
>> +#define S_GP_IRQ_2_EN			BIT(11)
>> +#define S_GP_IRQ_3_EN			BIT(12)
>> +#define S_RX_FIFO_RD_ERR_EN		BIT(24)
>> +#define S_RX_FIFO_WR_ERR_EN		BIT(25)
>> +#define S_COMMON_GENI_S_IRQ_EN	(GENMASK(5, 1) | GENMASK(13, 9) | \
>> +				 S_RX_FIFO_RD_ERR_EN | S_RX_FIFO_WR_ERR_EN)
>> +
>> +#define GENI_CGC_CTRL_PROG_RAM_SCLK_OFF_BMSK		0x00000200
>> +#define GENI_CGC_CTRL_PROG_RAM_HCLK_OFF_BMSK		0x00000100
>> +
>> +#define GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK		0x00000001
>> +
>> +#define DMA_TX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK	0x00000008
>> +#define DMA_TX_IRQ_EN_SET_SBE_EN_SET_BMSK		0x00000004
>> +#define DMA_TX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK		0x00000001
>> +
>> +#define DMA_RX_IRQ_EN_SET_FLUSH_DONE_EN_SET_BMSK	0x00000010
>> +#define DMA_RX_IRQ_EN_SET_RESET_DONE_EN_SET_BMSK	0x00000008
>> +#define DMA_RX_IRQ_EN_SET_SBE_EN_SET_BMSK		0x00000004
>> +#define DMA_RX_IRQ_EN_SET_DMA_DONE_EN_SET_BMSK		0x00000001
>> +
>> +#define DMA_GENERAL_CFG_AHB_SEC_SLV_CLK_CGC_ON_BMSK	0x00000008
>> +#define DMA_GENERAL_CFG_DMA_AHB_SLV_CLK_CGC_ON_BMSK	0x00000004
>> +#define DMA_GENERAL_CFG_DMA_TX_CLK_CGC_ON_BMSK		0x00000002
>> +#define DMA_GENERAL_CFG_DMA_RX_CLK_CGC_ON_BMSK		0x00000001
>> +
>> +#define GENI_CLK_CTRL_SER_CLK_SEL_BMSK			0x00000001
>> +#define DMA_IF_EN_DMA_IF_EN_BMSK			0x00000001
>> +#define SE_GSI_EVENT_EN_BMSK				0x0000000f
>> +#define SE_IRQ_EN_RMSK					0x0000000f
>> +
>> +#define QUPV3_COMMON_CFG				0x0120
>> +#define FAST_SWITCH_TO_HIGH_DISABLE_BMASK		0x00000001
>> +
>> +#define QUPV3_SE_AHB_M_CFG				0x0118
>> +#define AHB_M_CLK_CGC_ON_BMASK				0x00000001
>> +
>> +#define QUPV3_COMMON_CGC_CTRL				0x021C
>> +#define COMMON_CSR_SLV_CLK_CGC_ON_BMASK			0x00000001
>> +
>> +/* access ports */
>> +#define setbits32(_addr, _v) out_be32((_addr), in_be32(_addr) |  (_v))
>> +#define clrbits32(_addr, _v) out_be32((_addr), in_be32(_addr) & ~(_v))
> 
> These names are way too generic.

Sure, updated in v4.

> 
>> +
>> +#define out_be32(a, v) writel_relaxed(v, a)
>> +#define in_be32(a) readl_relaxed(a)
> 
> Don't create aliases.

Sure, updated in v4.

> 
>> +
>> +/**
>> + * struct elf_se_hdr - firmware configurations
>> + *
>> + * @magic: set to 'SEFW'
>> + * @version: A 32-bit value indicating the structure’s version number
>> + * @core_version: QUPV3_HW_VERSION
>> + * @serial_protocol: Programmed into GENI_FW_REVISION
>> + * @fw_version: Programmed into GENI_FW_REVISION
>> + * @cfg_version: Programmed into GENI_INIT_CFG_REVISION
>> + * @fw_size_in_items: Number of (uint32_t) GENI_FW_RAM words
>> + * @fw_offset: Byte offset of GENI_FW_RAM array
>> + * @cfg_size_in_items: Number of GENI_FW_CFG index/value pairs
>> + * @cfg_idx_offset: Byte offset of GENI_FW_CFG index array
>> + * @cfg_val_offset: Byte offset of GENI_FW_CFG values array
>> + */
>> +struct elf_se_hdr {
> 
> This structure doesn't seem to relate to ELF at all, it seems to be the
> header for the serial engine configuration.
> 
> It happens to be packaged up in a ELF segment, but I don't see a reason
> for defining the format of the outer container here.
> 
>> +	u32 magic;
> 
> What endian is that u32? Please be specific.

I didn't understand your comment. Could you please help me understand 
what is required here?

> 
> 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 v3 6/9] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem

[Konrad Dybcio]

On 5/3/25 1:11 PM, Viken Dadhaniya wrote:
> 
> 
> On 3/6/2025 4:57 AM, Bjorn Andersson wrote:
>> On Mon, Mar 03, 2025 at 06:13:46PM +0530, Viken Dadhaniya wrote:
>>> Load the firmware to QUP SE based on the 'firmware-name' property specified

[...]

>>> +    u32 magic;
>>
>> What endian is that u32? Please be specific.
> 
> I didn't understand your comment. Could you please help me understand what is required here?

External data can be of different endianness than the running OS.
This should either be __le32 or __be32.

Konrad

Re: [PATCH v3 6/9] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem

[Konrad Dybcio]

On 3.03.2025 1:43 PM, Viken Dadhaniya wrote:
> Load the firmware to QUP SE based on the 'firmware-name' property specified
> in devicetree. Populate Serial engine and base address details in the probe
> function of the protocol driver and pass to firmware load routine.
> 
> Skip the firmware loading if the firmware is already loaded in Serial
> Engine's firmware memory area.
> 
> Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
> ---

[...]

> +static bool elf_phdr_valid(const struct elf32_phdr *phdr)
> +{
> +	if (phdr->p_type != PT_LOAD || !phdr->p_memsz)
> +		return false;
> +
> +	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)
> +		return true;
> +
> +	return false;

return (contents of the if condition)

> +}
> +
> +/**
> + * valid_seg_size() - Validate the segment size.
> + * @pelfseg: Pointer to the ELF header.
> + * @p_filesz: Pointer to the file size.
> + *
> + * Validate the ELF segment size by comparing the file size.
> + *
> + * Return: true if the segment is valid, false if the segment is invalid.
> + */
> +static bool valid_seg_size(struct elf_se_hdr *pelfseg, Elf32_Word p_filesz)
> +{
> +	if (p_filesz >= pelfseg->fw_offset + pelfseg->fw_size_in_items * sizeof(u32) &&
> +	    p_filesz >= pelfseg->cfg_idx_offset + pelfseg->cfg_size_in_items * sizeof(u8) &&
> +	    p_filesz >= pelfseg->cfg_val_offset + pelfseg->cfg_size_in_items * sizeof(u32))
> +		return true;
> +	return false;
> +}

same here

[...]

> +static int geni_configure_xfer_mode(struct qup_se_rsc *rsc)
> +{
> +	/* Configure SE FIFO, DMA or GSI mode. */
> +	switch (rsc->mode) {
> +	case GENI_GPI_DMA:
> +		setbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
> +			  GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
> +		writel_relaxed(0x0, rsc->se->base + SE_IRQ_EN);
> +		writel_relaxed(SE_GSI_EVENT_EN_BMSK, rsc->se->base + SE_GSI_EVENT_EN);
> +		break;
> +
> +	case GENI_SE_FIFO:
> +		clrbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
> +			  GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
> +		writel_relaxed(SE_IRQ_EN_RMSK, rsc->se->base + SE_IRQ_EN);
> +		writel_relaxed(0x0, rsc->se->base + SE_GSI_EVENT_EN);
> +		break;
> +
> +	case GENI_SE_DMA:
> +		setbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
> +			  GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);

This write is common across all 3 modes

> +		writel_relaxed(SE_IRQ_EN_RMSK, rsc->se->base + SE_IRQ_EN);
> +		writel_relaxed(0x0, rsc->se->base + SE_GSI_EVENT_EN);

These two writes are common across !GPI_DMA

> +		break;
> +
> +	default:
> +		dev_err(rsc->se->dev, "invalid se mode: %d\n", rsc->mode);
> +		return -EINVAL;

I wouldn't expect this to ever fail..

> +	}
> +	return 0;
> +}
> +
> +/**
> + * geni_enable_interrupts() Enable interrupts.
> + * @rsc: Pointer to a structure representing SE-related resources.
> + *
> + * Enable the required interrupts during the firmware load process.
> + *
> + * Return: None.
> + */
> +static void geni_enable_interrupts(struct qup_se_rsc *rsc)
> +{
> +	u32 reg_value;
> +
> +	/* Enable required interrupts. */
> +	writel_relaxed(M_COMMON_GENI_M_IRQ_EN, rsc->se->base + GENI_M_IRQ_ENABLE);
> +
> +	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;

The S-es should be aligned, similarly for other additions in this patch

[...]

> +	/* Flash firmware revision register. */
> +	reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
> +		    (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);

Use FIELD_PREP and GENMASK to denote bitfields

> +	writel_relaxed(reg_value, rsc->se->base + SE_GENI_FW_REVISION);
> +
> +	reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
> +		    (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
> +
> +	writel_relaxed(reg_value, rsc->se->base + SE_S_FW_REVISION);
> +}

Konrad

Re: [PATCH v3 6/9] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem

[Viken Dadhaniya]

On 3/8/2025 11:36 PM, Konrad Dybcio wrote:
> On 3.03.2025 1:43 PM, Viken Dadhaniya wrote:
>> Load the firmware to QUP SE based on the 'firmware-name' property specified
>> in devicetree. Populate Serial engine and base address details in the probe
>> function of the protocol driver and pass to firmware load routine.
>>
>> Skip the firmware loading if the firmware is already loaded in Serial
>> Engine's firmware memory area.
>>
>> Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
>> ---
> 
> [...]
> 
>> +static bool elf_phdr_valid(const struct elf32_phdr *phdr)
>> +{
>> +	if (phdr->p_type != PT_LOAD || !phdr->p_memsz)
>> +		return false;
>> +
>> +	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)
>> +		return true;
>> +
>> +	return false;
> 
> return (contents of the if condition)
> 
>> +}
>> +
>> +/**
>> + * valid_seg_size() - Validate the segment size.
>> + * @pelfseg: Pointer to the ELF header.
>> + * @p_filesz: Pointer to the file size.
>> + *
>> + * Validate the ELF segment size by comparing the file size.
>> + *
>> + * Return: true if the segment is valid, false if the segment is invalid.
>> + */
>> +static bool valid_seg_size(struct elf_se_hdr *pelfseg, Elf32_Word p_filesz)
>> +{
>> +	if (p_filesz >= pelfseg->fw_offset + pelfseg->fw_size_in_items * sizeof(u32) &&
>> +	    p_filesz >= pelfseg->cfg_idx_offset + pelfseg->cfg_size_in_items * sizeof(u8) &&
>> +	    p_filesz >= pelfseg->cfg_val_offset + pelfseg->cfg_size_in_items * sizeof(u32))
>> +		return true;
>> +	return false;
>> +}
> 
> same here
> 
> [...]
> 
>> +static int geni_configure_xfer_mode(struct qup_se_rsc *rsc)
>> +{
>> +	/* Configure SE FIFO, DMA or GSI mode. */
>> +	switch (rsc->mode) {
>> +	case GENI_GPI_DMA:
>> +		setbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
>> +			  GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
>> +		writel_relaxed(0x0, rsc->se->base + SE_IRQ_EN);
>> +		writel_relaxed(SE_GSI_EVENT_EN_BMSK, rsc->se->base + SE_GSI_EVENT_EN);
>> +		break;
>> +
>> +	case GENI_SE_FIFO:
>> +		clrbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
>> +			  GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
>> +		writel_relaxed(SE_IRQ_EN_RMSK, rsc->se->base + SE_IRQ_EN);
>> +		writel_relaxed(0x0, rsc->se->base + SE_GSI_EVENT_EN);
>> +		break;
>> +
>> +	case GENI_SE_DMA:
>> +		setbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
>> +			  GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
> 
> This write is common across all 3 modes

In FIFO mode, the operation is to clear the bit, while in DMA mode, the 
operation is to set the bit.

> 
>> +		writel_relaxed(SE_IRQ_EN_RMSK, rsc->se->base + SE_IRQ_EN);
>> +		writel_relaxed(0x0, rsc->se->base + SE_GSI_EVENT_EN);
> 
> These two writes are common across !GPI_DMA

We have different operations in all three modes, so it's not possible to 
combine any of them.

> 
>> +		break;
>> +
>> +	default:
>> +		dev_err(rsc->se->dev, "invalid se mode: %d\n", rsc->mode);
>> +		return -EINVAL;
> 
> I wouldn't expect this to ever fail..

Yes, that's correct. But including a default case helps handle 
unexpected or invalid input gracefully.

Please let me know if you would like me to remove it.

> 
>> +	}
>> +	return 0;
>> +}
>> +
>> +/**
>> + * geni_enable_interrupts() Enable interrupts.
>> + * @rsc: Pointer to a structure representing SE-related resources.
>> + *
>> + * Enable the required interrupts during the firmware load process.
>> + *
>> + * Return: None.
>> + */
>> +static void geni_enable_interrupts(struct qup_se_rsc *rsc)
>> +{
>> +	u32 reg_value;
>> +
>> +	/* Enable required interrupts. */
>> +	writel_relaxed(M_COMMON_GENI_M_IRQ_EN, rsc->se->base + GENI_M_IRQ_ENABLE);
>> +
>> +	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;
> 
> The S-es should be aligned, similarly for other additions in this patch

Sure, updated in v4.

> 
> [...]
> 
>> +	/* Flash firmware revision register. */
>> +	reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
>> +		    (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
> 
> Use FIELD_PREP and GENMASK to denote bitfields

Sure, updated in v4.

> 
>> +	writel_relaxed(reg_value, rsc->se->base + SE_GENI_FW_REVISION);
>> +
>> +	reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
>> +		    (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
>> +
>> +	writel_relaxed(reg_value, rsc->se->base + SE_S_FW_REVISION);
>> +}
> 
> Konrad

Re: [PATCH v3 6/9] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem

[Dmitry Baryshkov]

On Sat, May 03, 2025 at 04:47:52PM +0530, Viken Dadhaniya wrote:
> 
> 
> On 3/8/2025 11:36 PM, Konrad Dybcio wrote:
> > On 3.03.2025 1:43 PM, Viken Dadhaniya wrote:
> > > Load the firmware to QUP SE based on the 'firmware-name' property specified
> > > in devicetree. Populate Serial engine and base address details in the probe
> > > function of the protocol driver and pass to firmware load routine.
> > > 
> > > Skip the firmware loading if the firmware is already loaded in Serial
> > > Engine's firmware memory area.
> > > 
> > > Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> > > Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> > > Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
> > > ---
> > 
> > [...]
> > 
> > > +		break;
> > > +
> > > +	default:
> > > +		dev_err(rsc->se->dev, "invalid se mode: %d\n", rsc->mode);
> > > +		return -EINVAL;
> > 
> > I wouldn't expect this to ever fail..
> 
> Yes, that's correct. But including a default case helps handle unexpected or
> invalid input gracefully.
> 
> Please let me know if you would like me to remove it.

If you are asking for additional comments, please refrain from sending
the next iteration until you actally resolve all the open questions.

-- 
With best wishes
Dmitry

Re: [PATCH v3 6/9] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem

[Viken Dadhaniya]

On 5/3/2025 10:51 PM, Dmitry Baryshkov wrote:
> On Sat, May 03, 2025 at 04:47:52PM +0530, Viken Dadhaniya wrote:
>>
>>
>> On 3/8/2025 11:36 PM, Konrad Dybcio wrote:
>>> On 3.03.2025 1:43 PM, Viken Dadhaniya wrote:
>>>> Load the firmware to QUP SE based on the 'firmware-name' property specified
>>>> in devicetree. Populate Serial engine and base address details in the probe
>>>> function of the protocol driver and pass to firmware load routine.
>>>>
>>>> Skip the firmware loading if the firmware is already loaded in Serial
>>>> Engine's firmware memory area.
>>>>
>>>> Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>>>> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
>>>> ---
>>>
>>> [...]
>>>
>>>> +		break;
>>>> +
>>>> +	default:
>>>> +		dev_err(rsc->se->dev, "invalid se mode: %d\n", rsc->mode);
>>>> +		return -EINVAL;
>>>
>>> I wouldn't expect this to ever fail..
>>
>> Yes, that's correct. But including a default case helps handle unexpected or
>> invalid input gracefully.
>>
>> Please let me know if you would like me to remove it.
> 
> If you are asking for additional comments, please refrain from sending
> the next iteration until you actally resolve all the open questions.
> 

Sure, I will take care of it in future patches.

[PATCH v3 7/9] 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 <quic_msavaliy@quicinc.com>
Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Signed-off-by: Viken Dadhaniya <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 7bbd478171e0..b6cf85fbe521 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -871,7 +871,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 v3 8/9] 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 <quic_msavaliy@quicinc.com>
Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Signed-off-by: Viken Dadhaniya <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 v3 9/9] 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 <quic_msavaliy@quicinc.com>
Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Signed-off-by: Viken Dadhaniya <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 a80ce7aaf309..ae66c5f673a6 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1145,7 +1145,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 v3 0/9] Add support to load QUP SE firmware from

[Krzysztof Kozlowski]

On Mon, Mar 03, 2025 at 06:13:40PM +0530, Viken Dadhaniya wrote:
> 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.


The most important part of cover letter (or patch changelog - not
present either) is to explain dependencies and merging stratgy. This
cannot be taken by respective maintainers directly and you *MUST*
clearly express it.

Best regards,
Krzysztof

Re: [PATCH v3 0/9] Add support to load QUP SE firmware from

[Krzysztof Kozlowski]

On 04/03/2025 09:11, Krzysztof Kozlowski wrote:
> On Mon, Mar 03, 2025 at 06:13:40PM +0530, Viken Dadhaniya wrote:
>> 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.
> 
> 
> The most important part of cover letter (or patch changelog - not
> present either) is to explain dependencies and merging stratgy. This


Actually you have changelog per patch but this information is missing
there as well.

> cannot be taken by respective maintainers directly and you *MUST*
> clearly express it.
> 
> Best regards,
> Krzysztof
> 


Best regards,
Krzysztof