[PATCH v2 0/8] Add support to load QUP SE firmware from

[PATCH v2 0/8] 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.
---
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 (8):
  dt-bindings: qcom: geni-se: Add 'firmware-name' property for firmware
    loading
  dt-bindings: i2c: qcom,i2c-geni: Add support for selecting data
    transfer mode
  spi: dt-bindings: Add support for selecting data transfer mode
  dt-bindings: serial: Add support for selecting data transfer mode
  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      |   7 +
 .../serial/qcom,serial-geni-qcom.yaml         |   8 +
 .../bindings/soc/qcom/qcom,geni-se.yaml       |   5 +
 .../bindings/spi/qcom,spi-geni-qcom.yaml      |   7 +
 drivers/i2c/busses/i2c-qcom-geni.c            |   7 +-
 drivers/soc/qcom/qcom-geni-se.c               | 444 ++++++++++++++++++
 drivers/spi/spi-geni-qcom.c                   |   7 +-
 drivers/tty/serial/qcom_geni_serial.c         |   7 +-
 include/linux/soc/qcom/geni-se.h              |  17 +
 include/linux/soc/qcom/qup-fw-load.h          | 179 +++++++
 10 files changed, 682 insertions(+), 6 deletions(-)
 create mode 100644 include/linux/soc/qcom/qup-fw-load.h

-- 
2.34.1

[PATCH v2 1/8] 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>
---
 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..3e029d4d6f58 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:
+    $ref: /schemas/types.yaml#/definitions/string
+    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 v2 1/8] dt-bindings: qcom: geni-se: Add 'firmware-name' property for firmware loading

[Krzysztof Kozlowski]

On 24/01/2025 11:53, 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>
> ---
>  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..3e029d4d6f58 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:
> +    $ref: /schemas/types.yaml#/definitions/string


You work on outdated tree. Just look how other bindings are doing this.
maxItems instead.


Best regards,
Krzysztof

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

[Viken Dadhaniya]

On 1/24/2025 4:30 PM, Krzysztof Kozlowski wrote:
> On 24/01/2025 11:53, 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>
>> ---
>>   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..3e029d4d6f58 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:
>> +    $ref: /schemas/types.yaml#/definitions/string
> 
> 
> You work on outdated tree. Just look how other bindings are doing this.
> maxItems instead.
> 
> 
> Best regards,
> Krzysztof
> 

Sure, Will update in next patch.

Thanks
Viken

[PATCH v2 2/8] dt-bindings: i2c: qcom,i2c-geni: Add support for selecting data transfer mode

[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,xfer-mode' properties to select the data transfer mode,
either GPI DMA (Generic Packet Interface) or non-GPI mode (PIO/CPU DMA).

I2C controller can operate in one of two modes based on the
'qcom,xfer-mode' property, and the firmware is loaded accordingly.

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

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/
---
---
 .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml        | 7 +++++++
 1 file changed, 7 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..68e4bf0c84d1 100644
--- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
+++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
@@ -66,6 +66,12 @@ properties:
   required-opps:
     maxItems: 1
 
+  qcom,xfer-mode:
+    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) mode and 3 for GPI DMA mode.
+      The default mode is FIFO.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1, 3]
+
 required:
   - compatible
   - interrupts
@@ -142,5 +148,6 @@ examples:
         interconnect-names = "qup-core", "qup-config", "qup-memory";
         power-domains = <&rpmhpd SC7180_CX>;
         required-opps = <&rpmhpd_opp_low_svs>;
+        qcom,xfer-mode = <1>;
     };
 ...
-- 
2.34.1

Re: [PATCH v2 2/8] dt-bindings: i2c: qcom,i2c-geni: Add support for selecting data transfer mode

[Dmitry Baryshkov]

On Fri, Jan 24, 2025 at 04:23:03PM +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,xfer-mode' properties to select the data transfer mode,
> either GPI DMA (Generic Packet Interface) or non-GPI mode (PIO/CPU DMA).
> 
> I2C controller can operate in one of two modes based on the
> 'qcom,xfer-mode' property, and the firmware is loaded accordingly.

Is it possible to load the firmware after it being loaded by TZ? Is it
possible to change the mode at runtime too?

> 
> 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>
> ---
> 
> 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/
> ---
> ---
>  .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml        | 7 +++++++
>  1 file changed, 7 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..68e4bf0c84d1 100644
> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> @@ -66,6 +66,12 @@ properties:
>    required-opps:
>      maxItems: 1
>  
> +  qcom,xfer-mode:
> +    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) mode and 3 for GPI DMA mode.
> +      The default mode is FIFO.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 3]
> +
>  required:
>    - compatible
>    - interrupts
> @@ -142,5 +148,6 @@ examples:
>          interconnect-names = "qup-core", "qup-config", "qup-memory";
>          power-domains = <&rpmhpd SC7180_CX>;
>          required-opps = <&rpmhpd_opp_low_svs>;
> +        qcom,xfer-mode = <1>;

What does <1> mean? Please provide corresponding defines.

>      };
>  ...
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/8] dt-bindings: i2c: qcom,i2c-geni: Add support for selecting data transfer mode

[Viken Dadhaniya]

On 1/24/2025 4:48 PM, Dmitry Baryshkov wrote:
> On Fri, Jan 24, 2025 at 04:23:03PM +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,xfer-mode' properties to select the data transfer mode,
>> either GPI DMA (Generic Packet Interface) or non-GPI mode (PIO/CPU DMA).
>>
>> I2C controller can operate in one of two modes based on the
>> 'qcom,xfer-mode' property, and the firmware is loaded accordingly.
> 
> Is it possible to load the firmware after it being loaded by TZ? Is it
> possible to change the mode at runtime too?

No, firmware can be loaded either from the TZ side or APPS side.

In non-GPI mode, the transfer mode will change runtime between PIO and 
CPU DMA based on the data length.

We need to update the device tree property(qcom,xfer-mode) to change the 
mode between non-GPI and GPI.

> 
>>
>> 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>
>> ---
>>
>> 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/
>> ---
>> ---
>>   .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml        | 7 +++++++
>>   1 file changed, 7 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..68e4bf0c84d1 100644
>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> @@ -66,6 +66,12 @@ properties:
>>     required-opps:
>>       maxItems: 1
>>   
>> +  qcom,xfer-mode:
>> +    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) mode and 3 for GPI DMA mode.
>> +      The default mode is FIFO.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [1, 3]
>> +
>>   required:
>>     - compatible
>>     - interrupts
>> @@ -142,5 +148,6 @@ examples:
>>           interconnect-names = "qup-core", "qup-config", "qup-memory";
>>           power-domains = <&rpmhpd SC7180_CX>;
>>           required-opps = <&rpmhpd_opp_low_svs>;
>> +        qcom,xfer-mode = <1>;
> 
> What does <1> mean? Please provide corresponding defines.

Do we need to add a string instead of a number, like 
include/dt-bindings/dma/qcom-gpi.h?

> 
>>       };
>>   ...
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH v2 2/8] dt-bindings: i2c: qcom,i2c-geni: Add support for selecting data transfer mode

[Dmitry Baryshkov]

On Fri, Jan 24, 2025 at 05:52:24PM +0530, Viken Dadhaniya wrote:
> 
> 
> On 1/24/2025 4:48 PM, Dmitry Baryshkov wrote:
> > On Fri, Jan 24, 2025 at 04:23:03PM +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,xfer-mode' properties to select the data transfer mode,
> > > either GPI DMA (Generic Packet Interface) or non-GPI mode (PIO/CPU DMA).
> > > 
> > > I2C controller can operate in one of two modes based on the
> > > 'qcom,xfer-mode' property, and the firmware is loaded accordingly.
> > 
> > Is it possible to load the firmware after it being loaded by TZ? Is it
> > possible to change the mode at runtime too?
> 
> No, firmware can be loaded either from the TZ side or APPS side.

You answer actually reads as "No, yes" (excuse me, non-native here).
Most likely you mean that it can not be reloaded once either TZ or APPS
has loaded it.

> In non-GPI mode, the transfer mode will change runtime between PIO and CPU
> DMA based on the data length.
> 
> We need to update the device tree property(qcom,xfer-mode) to change the
> mode between non-GPI and GPI.

So, is it actually possible to change the mode? E.g. if the TZ has
loaded the firmware and configured SE for PIO/SE DMA, is it possible to
change it to GPI DMA?

> 
> > 
> > > 
> > > 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>
> > > ---
> > > 
> > > 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/
> > > ---
> > > ---
> > >   .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml        | 7 +++++++
> > >   1 file changed, 7 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..68e4bf0c84d1 100644
> > > --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> > > +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> > > @@ -66,6 +66,12 @@ properties:
> > >     required-opps:
> > >       maxItems: 1
> > > +  qcom,xfer-mode:
> > > +    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) mode and 3 for GPI DMA mode.
> > > +      The default mode is FIFO.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [1, 3]
> > > +
> > >   required:
> > >     - compatible
> > >     - interrupts
> > > @@ -142,5 +148,6 @@ examples:
> > >           interconnect-names = "qup-core", "qup-config", "qup-memory";
> > >           power-domains = <&rpmhpd SC7180_CX>;
> > >           required-opps = <&rpmhpd_opp_low_svs>;
> > > +        qcom,xfer-mode = <1>;
> > 
> > What does <1> mean? Please provide corresponding defines.
> 
> Do we need to add a string instead of a number, like
> include/dt-bindings/dma/qcom-gpi.h?

You need to '#define FOO_BAR 1', then another one for 3. String is a
"string", it's not required here (in my opinion).

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/8] dt-bindings: i2c: qcom,i2c-geni: Add support for selecting data transfer mode

[Viken Dadhaniya]

On 1/24/2025 8:33 PM, Dmitry Baryshkov wrote:
> On Fri, Jan 24, 2025 at 05:52:24PM +0530, Viken Dadhaniya wrote:
>>
>>
>> On 1/24/2025 4:48 PM, Dmitry Baryshkov wrote:
>>> On Fri, Jan 24, 2025 at 04:23:03PM +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,xfer-mode' properties to select the data transfer mode,
>>>> either GPI DMA (Generic Packet Interface) or non-GPI mode (PIO/CPU DMA).
>>>>
>>>> I2C controller can operate in one of two modes based on the
>>>> 'qcom,xfer-mode' property, and the firmware is loaded accordingly.
>>>
>>> Is it possible to load the firmware after it being loaded by TZ? Is it
>>> possible to change the mode at runtime too?
>>
>> No, firmware can be loaded either from the TZ side or APPS side.
> 
> You answer actually reads as "No, yes" (excuse me, non-native here).
> Most likely you mean that it can not be reloaded once either TZ or APPS
> has loaded it.

Yes correct. it can not be reloaded once either TZ or APPS has loaded it.

> 
>> In non-GPI mode, the transfer mode will change runtime between PIO and CPU
>> DMA based on the data length.
>>
>> We need to update the device tree property(qcom,xfer-mode) to change the
>> mode between non-GPI and GPI.
> 
> So, is it actually possible to change the mode? E.g. if the TZ has
> loaded the firmware and configured SE for PIO/SE DMA, is it possible to
> change it to GPI DMA?

No, if the TZ has loaded the firmware, it is not possible to switch from 
non-GPI (PIO/SE DMA) to 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>
>>>> ---
>>>>
>>>> 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/
>>>> ---
>>>> ---
>>>>    .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml        | 7 +++++++
>>>>    1 file changed, 7 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..68e4bf0c84d1 100644
>>>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>>> @@ -66,6 +66,12 @@ properties:
>>>>      required-opps:
>>>>        maxItems: 1
>>>> +  qcom,xfer-mode:
>>>> +    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) mode and 3 for GPI DMA mode.
>>>> +      The default mode is FIFO.
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    enum: [1, 3]
>>>> +
>>>>    required:
>>>>      - compatible
>>>>      - interrupts
>>>> @@ -142,5 +148,6 @@ examples:
>>>>            interconnect-names = "qup-core", "qup-config", "qup-memory";
>>>>            power-domains = <&rpmhpd SC7180_CX>;
>>>>            required-opps = <&rpmhpd_opp_low_svs>;
>>>> +        qcom,xfer-mode = <1>;
>>>
>>> What does <1> mean? Please provide corresponding defines.
>>
>> Do we need to add a string instead of a number, like
>> include/dt-bindings/dma/qcom-gpi.h?
> 
> You need to '#define FOO_BAR 1', then another one for 3. String is a
> "string", it's not required here (in my opinion).
> 

Sure, I will update it in the next patch.

Re: [PATCH v2 2/8] dt-bindings: i2c: qcom,i2c-geni: Add support for selecting data transfer mode

[Krzysztof Kozlowski]

On 24/01/2025 16:16, Viken Dadhaniya wrote:
> 
> 
> On 1/24/2025 8:33 PM, Dmitry Baryshkov wrote:
>> On Fri, Jan 24, 2025 at 05:52:24PM +0530, Viken Dadhaniya wrote:
>>>
>>>
>>> On 1/24/2025 4:48 PM, Dmitry Baryshkov wrote:
>>>> On Fri, Jan 24, 2025 at 04:23:03PM +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,xfer-mode' properties to select the data transfer mode,
>>>>> either GPI DMA (Generic Packet Interface) or non-GPI mode (PIO/CPU DMA).
>>>>>
>>>>> I2C controller can operate in one of two modes based on the
>>>>> 'qcom,xfer-mode' property, and the firmware is loaded accordingly.
>>>>
>>>> Is it possible to load the firmware after it being loaded by TZ? Is it
>>>> possible to change the mode at runtime too?
>>>
>>> No, firmware can be loaded either from the TZ side or APPS side.
>>
>> You answer actually reads as "No, yes" (excuse me, non-native here).
>> Most likely you mean that it can not be reloaded once either TZ or APPS
>> has loaded it.
> 
> Yes correct. it can not be reloaded once either TZ or APPS has loaded it.
> 
>>
>>> In non-GPI mode, the transfer mode will change runtime between PIO and CPU
>>> DMA based on the data length.
>>>
>>> We need to update the device tree property(qcom,xfer-mode) to change the
>>> mode between non-GPI and GPI.
>>
>> So, is it actually possible to change the mode? E.g. if the TZ has
>> loaded the firmware and configured SE for PIO/SE DMA, is it possible to
>> change it to GPI DMA?
> 
> No, if the TZ has loaded the firmware, it is not possible to switch from 
> non-GPI (PIO/SE DMA) to 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>
>>>>> ---
>>>>>
>>>>> 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/
>>>>> ---
>>>>> ---
>>>>>    .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml        | 7 +++++++
>>>>>    1 file changed, 7 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..68e4bf0c84d1 100644
>>>>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>>>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>>>> @@ -66,6 +66,12 @@ properties:
>>>>>      required-opps:
>>>>>        maxItems: 1
>>>>> +  qcom,xfer-mode:
>>>>> +    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) mode and 3 for GPI DMA mode.
>>>>> +      The default mode is FIFO.
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +    enum: [1, 3]
>>>>> +
>>>>>    required:
>>>>>      - compatible
>>>>>      - interrupts
>>>>> @@ -142,5 +148,6 @@ examples:
>>>>>            interconnect-names = "qup-core", "qup-config", "qup-memory";
>>>>>            power-domains = <&rpmhpd SC7180_CX>;
>>>>>            required-opps = <&rpmhpd_opp_low_svs>;
>>>>> +        qcom,xfer-mode = <1>;
>>>>
>>>> What does <1> mean? Please provide corresponding defines.
>>>
>>> Do we need to add a string instead of a number, like
>>> include/dt-bindings/dma/qcom-gpi.h?
>>
>> You need to '#define FOO_BAR 1', then another one for 3. String is a
>> "string", it's not required here (in my opinion).
>>
> 
> Sure, I will update it in the next patch.

Well, no. Not 3 but 2, because why having gap there?

But anyway use string and explain what is GPI. I assume that DMA is
present in both cases, you just choose not to use it? If so, then why?

Best regards,
Krzysztof

[PATCH v2 3/8] spi: dt-bindings: Add support for selecting data transfer mode

[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,xfer-mode' properties to select the data transfer mode,
either GPI DMA (Generic Packet Interface) or non-GPI mode (PIO/CPU DMA).

SPI controller can operate in one of two modes based on the
'qcom,xfer-mode' property, and the firmware is loaded accordingly.

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

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/
---
---
 .../devicetree/bindings/spi/qcom,spi-geni-qcom.yaml        | 7 +++++++
 1 file changed, 7 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..d0dd960ee12f 100644
--- a/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml
+++ b/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml
@@ -66,6 +66,12 @@ properties:
   reg:
     maxItems: 1
 
+  qcom,xfer-mode:
+    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) mode and 3 for GPI DMA mode.
+      The default mode is FIFO.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1, 3]
+
 required:
   - compatible
   - clocks
@@ -97,6 +103,7 @@ examples:
         interconnects = <&qup_virt MASTER_QUP_CORE_0 0 &qup_virt SLAVE_QUP_CORE_0 0>,
                         <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_0 0>;
         interconnect-names = "qup-core", "qup-config";
+        qcom,xfer-mode = <1>;
     };
 
   - |
-- 
2.34.1

Re: [PATCH v2 3/8] spi: dt-bindings: Add support for selecting data transfer mode

[Krzysztof Kozlowski]

On 24/01/2025 11:53, Viken Dadhaniya wrote:
>  .../devicetree/bindings/spi/qcom,spi-geni-qcom.yaml        | 7 +++++++
>  1 file changed, 7 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..d0dd960ee12f 100644
> --- a/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml
> +++ b/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml
> @@ -66,6 +66,12 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  qcom,xfer-mode:
> +    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) mode and 3 for GPI DMA mode.
> +      The default mode is FIFO.
> +    $ref: /schemas/types.yaml#/definitions/uint32

Same concerns as for I2C, so discussion going there.

Best regards,
Krzysztof

[PATCH v2 4/8] dt-bindings: serial: Add support for selecting data transfer mode

[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,xfer-mode' properties to select the data transfer mode,
either GPI DMA (Generic Packet Interface) or non-GPI mode (PIO/CPU DMA).

UART controller can operate in one of two modes based on the
'qcom,xfer-mode' property, and the firmware is loaded accordingly.

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

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 | 8 ++++++++
 1 file changed, 8 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..383773b32e47 100644
--- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
+++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
@@ -56,6 +56,13 @@ properties:
   reg:
     maxItems: 1
 
+  qcom,xfer-mode:
+    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) mode and 3 for GPI DMA mode.
+      The default mode is FIFO.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1, 3]
+
+
 required:
   - compatible
   - clocks
@@ -82,5 +89,6 @@ examples:
         interconnects = <&qup_virt MASTER_QUP_CORE_0 0 &qup_virt SLAVE_QUP_CORE_0 0>,
                         <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_0 0>;
         interconnect-names = "qup-core", "qup-config";
+        qcom,xfer-mode = <1>;
     };
 ...
-- 
2.34.1

Re: [PATCH v2 4/8] dt-bindings: serial: Add support for selecting data transfer mode

[Krzysztof Kozlowski]

On 24/01/2025 11:53, 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,xfer-mode' properties to select the data transfer mode,
> either GPI DMA (Generic Packet Interface) or non-GPI mode (PIO/CPU DMA).
> 
> UART controller can operate in one of two modes based on the
> 'qcom,xfer-mode' property, and the firmware is loaded accordingly.
> 
> 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>
> ---
> 
> 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 | 8 ++++++++
>  1 file changed, 8 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..383773b32e47 100644
> --- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
> +++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
> @@ -56,6 +56,13 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  qcom,xfer-mode:
> +    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) mode and 3 for GPI DMA mode.
> +      The default mode is FIFO.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 3]
> +
> +

Just one blank line, but anyway, this property should not be in three
places. Do you really expect that each of serial engines within one
GeniQUP will be configured differently by TZ?

Best regards,
Krzysztof

Re: [PATCH v2 4/8] dt-bindings: serial: Add support for selecting data transfer mode

[Dmitry Baryshkov]

On Mon, Jan 27, 2025 at 08:02:12AM +0100, Krzysztof Kozlowski wrote:
> On 24/01/2025 11:53, 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,xfer-mode' properties to select the data transfer mode,
> > either GPI DMA (Generic Packet Interface) or non-GPI mode (PIO/CPU DMA).
> > 
> > UART controller can operate in one of two modes based on the
> > 'qcom,xfer-mode' property, and the firmware is loaded accordingly.
> > 
> > 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>
> > ---
> > 
> > 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 | 8 ++++++++
> >  1 file changed, 8 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..383773b32e47 100644
> > --- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
> > +++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
> > @@ -56,6 +56,13 @@ properties:
> >    reg:
> >      maxItems: 1
> >  
> > +  qcom,xfer-mode:
> > +    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) mode and 3 for GPI DMA mode.
> > +      The default mode is FIFO.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [1, 3]
> > +
> > +
> 
> Just one blank line, but anyway, this property should not be in three
> places. Do you really expect that each of serial engines within one
> GeniQUP will be configured differently by TZ?

Yes, each SE is configured separately and it's quite frequent when
different SEs have different DMA configuration.

-- 
With best wishes
Dmitry

Re: [PATCH v2 4/8] dt-bindings: serial: Add support for selecting data transfer mode

[Krzysztof Kozlowski]

On 27/01/2025 15:27, Dmitry Baryshkov wrote:
> On Mon, Jan 27, 2025 at 08:02:12AM +0100, Krzysztof Kozlowski wrote:
>> On 24/01/2025 11:53, 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,xfer-mode' properties to select the data transfer mode,
>>> either GPI DMA (Generic Packet Interface) or non-GPI mode (PIO/CPU DMA).
>>>
>>> UART controller can operate in one of two modes based on the
>>> 'qcom,xfer-mode' property, and the firmware is loaded accordingly.
>>>
>>> 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>
>>> ---
>>>
>>> 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 | 8 ++++++++
>>>  1 file changed, 8 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..383773b32e47 100644
>>> --- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
>>> +++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
>>> @@ -56,6 +56,13 @@ properties:
>>>    reg:
>>>      maxItems: 1
>>>  
>>> +  qcom,xfer-mode:
>>> +    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) mode and 3 for GPI DMA mode.
>>> +      The default mode is FIFO.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [1, 3]
>>> +
>>> +
>>
>> Just one blank line, but anyway, this property should not be in three
>> places. Do you really expect that each of serial engines within one
>> GeniQUP will be configured differently by TZ?
> 
> Yes, each SE is configured separately and it's quite frequent when
> different SEs have different DMA configuration.

Well, I checked at sm8550 and sm8650 and each pair of SE - which shares
resources - has the same DMAs, so I would not call it frequent. Care to
bring an example where same serial engines have different DMAs and
different TZ? We do not talk about single QUP.

Anyway, if you need property per node, this has to be shared schema.

Best regards,
Krzysztof

Re: [PATCH v2 4/8] dt-bindings: serial: Add support for selecting data transfer mode

[Konrad Dybcio]

On 27.01.2025 5:24 PM, Krzysztof Kozlowski wrote:
> On 27/01/2025 15:27, Dmitry Baryshkov wrote:
>> On Mon, Jan 27, 2025 at 08:02:12AM +0100, Krzysztof Kozlowski wrote:
>>> On 24/01/2025 11:53, 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,xfer-mode' properties to select the data transfer mode,
>>>> either GPI DMA (Generic Packet Interface) or non-GPI mode (PIO/CPU DMA).
>>>>
>>>> UART controller can operate in one of two modes based on the
>>>> 'qcom,xfer-mode' property, and the firmware is loaded accordingly.
>>>>
>>>> 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>
>>>> ---
>>>>
>>>> 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 | 8 ++++++++
>>>>  1 file changed, 8 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..383773b32e47 100644
>>>> --- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
>>>> +++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
>>>> @@ -56,6 +56,13 @@ properties:
>>>>    reg:
>>>>      maxItems: 1
>>>>  
>>>> +  qcom,xfer-mode:
>>>> +    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) mode and 3 for GPI DMA mode.
>>>> +      The default mode is FIFO.
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    enum: [1, 3]
>>>> +
>>>> +
>>>
>>> Just one blank line, but anyway, this property should not be in three
>>> places. Do you really expect that each of serial engines within one
>>> GeniQUP will be configured differently by TZ?
>>
>> Yes, each SE is configured separately and it's quite frequent when
>> different SEs have different DMA configuration.
> 
> Well, I checked at sm8550 and sm8650 and each pair of SE - which shares
> resources - has the same DMAs, so I would not call it frequent. Care to
> bring an example where same serial engines have different DMAs and
> different TZ? We do not talk about single QUP.
> 
> Anyway, if you need property per node, this has to be shared schema.

I'd rather ask a different question.. Is there *any* reason to not use
DMA for protocols that support it?

Konrad

Re: [PATCH v2 4/8] dt-bindings: serial: Add support for selecting data transfer mode

[Konrad Dybcio]

On 27.01.2025 6:50 PM, Konrad Dybcio wrote:
> On 27.01.2025 5:24 PM, Krzysztof Kozlowski wrote:
>> On 27/01/2025 15:27, Dmitry Baryshkov wrote:
>>> On Mon, Jan 27, 2025 at 08:02:12AM +0100, Krzysztof Kozlowski wrote:
>>>> On 24/01/2025 11:53, 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,xfer-mode' properties to select the data transfer mode,
>>>>> either GPI DMA (Generic Packet Interface) or non-GPI mode (PIO/CPU DMA).
>>>>>
>>>>> UART controller can operate in one of two modes based on the
>>>>> 'qcom,xfer-mode' property, and the firmware is loaded accordingly.
>>>>>
>>>>> 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>
>>>>> ---
>>>>>
>>>>> 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 | 8 ++++++++
>>>>>  1 file changed, 8 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..383773b32e47 100644
>>>>> --- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
>>>>> +++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
>>>>> @@ -56,6 +56,13 @@ properties:
>>>>>    reg:
>>>>>      maxItems: 1
>>>>>  
>>>>> +  qcom,xfer-mode:
>>>>> +    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) mode and 3 for GPI DMA mode.
>>>>> +      The default mode is FIFO.
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +    enum: [1, 3]
>>>>> +
>>>>> +
>>>>
>>>> Just one blank line, but anyway, this property should not be in three
>>>> places. Do you really expect that each of serial engines within one
>>>> GeniQUP will be configured differently by TZ?
>>>
>>> Yes, each SE is configured separately and it's quite frequent when
>>> different SEs have different DMA configuration.
>>
>> Well, I checked at sm8550 and sm8650 and each pair of SE - which shares
>> resources - has the same DMAs, so I would not call it frequent. Care to
>> bring an example where same serial engines have different DMAs and
>> different TZ? We do not talk about single QUP.
>>
>> Anyway, if you need property per node, this has to be shared schema.
> 
> I'd rather ask a different question.. Is there *any* reason to not use
> DMA for protocols that support it?

If not, we can simplify this to:

xfer_mode = protocol == PROTOCOL_UART ? XFER_FIFO : XFER_DMA

Konrad

Re: [PATCH v2 4/8] dt-bindings: serial: Add support for selecting data transfer mode

[Dmitry Baryshkov]

On Mon, Jan 27, 2025 at 05:24:21PM +0100, Krzysztof Kozlowski wrote:
> On 27/01/2025 15:27, Dmitry Baryshkov wrote:
> > On Mon, Jan 27, 2025 at 08:02:12AM +0100, Krzysztof Kozlowski wrote:
> >> On 24/01/2025 11:53, 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,xfer-mode' properties to select the data transfer mode,
> >>> either GPI DMA (Generic Packet Interface) or non-GPI mode (PIO/CPU DMA).
> >>>
> >>> UART controller can operate in one of two modes based on the
> >>> 'qcom,xfer-mode' property, and the firmware is loaded accordingly.
> >>>
> >>> 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>
> >>> ---
> >>>
> >>> 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 | 8 ++++++++
> >>>  1 file changed, 8 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..383773b32e47 100644
> >>> --- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
> >>> +++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
> >>> @@ -56,6 +56,13 @@ properties:
> >>>    reg:
> >>>      maxItems: 1
> >>>  
> >>> +  qcom,xfer-mode:
> >>> +    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) mode and 3 for GPI DMA mode.
> >>> +      The default mode is FIFO.
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    enum: [1, 3]
> >>> +
> >>> +
> >>
> >> Just one blank line, but anyway, this property should not be in three
> >> places. Do you really expect that each of serial engines within one
> >> GeniQUP will be configured differently by TZ?
> > 
> > Yes, each SE is configured separately and it's quite frequent when
> > different SEs have different DMA configuration.
> 
> Well, I checked at sm8550 and sm8650 and each pair of SE - which shares
> resources - has the same DMAs, so I would not call it frequent. Care to
> bring an example where same serial engines have different DMAs and
> different TZ? We do not talk about single QUP.

Well, I don't have access to the latest sm8550 / sm8650 devcfg sources.
I checked the RB5 ones. As far as I understand out of 14 enabled SEs
only two are configured for the GSI DMA, others should use FIFO / SE
DMA. Same applies to the SM8250 MTP devices. Checking the RB1 / RB2
setup also shows 3 out of 6 SEs being set for GSI.

> 
> Anyway, if you need property per node, this has to be shared schema.
> 
> Best regards,
> Krzysztof

-- 
With best wishes
Dmitry

Re: [PATCH v2 4/8] dt-bindings: serial: Add support for selecting data transfer mode

[Krzysztof Kozlowski]

On 29/01/2025 03:21, Dmitry Baryshkov wrote:
> On Mon, Jan 27, 2025 at 05:24:21PM +0100, Krzysztof Kozlowski wrote:
>> On 27/01/2025 15:27, Dmitry Baryshkov wrote:
>>> On Mon, Jan 27, 2025 at 08:02:12AM +0100, Krzysztof Kozlowski wrote:
>>>> On 24/01/2025 11:53, 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,xfer-mode' properties to select the data transfer mode,
>>>>> either GPI DMA (Generic Packet Interface) or non-GPI mode (PIO/CPU DMA).
>>>>>
>>>>> UART controller can operate in one of two modes based on the
>>>>> 'qcom,xfer-mode' property, and the firmware is loaded accordingly.
>>>>>
>>>>> 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>
>>>>> ---
>>>>>
>>>>> 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 | 8 ++++++++
>>>>>  1 file changed, 8 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..383773b32e47 100644
>>>>> --- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
>>>>> +++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
>>>>> @@ -56,6 +56,13 @@ properties:
>>>>>    reg:
>>>>>      maxItems: 1
>>>>>  
>>>>> +  qcom,xfer-mode:
>>>>> +    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) mode and 3 for GPI DMA mode.
>>>>> +      The default mode is FIFO.
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +    enum: [1, 3]
>>>>> +
>>>>> +
>>>>
>>>> Just one blank line, but anyway, this property should not be in three
>>>> places. Do you really expect that each of serial engines within one
>>>> GeniQUP will be configured differently by TZ?
>>>
>>> Yes, each SE is configured separately and it's quite frequent when
>>> different SEs have different DMA configuration.
>>
>> Well, I checked at sm8550 and sm8650 and each pair of SE - which shares
>> resources - has the same DMAs, so I would not call it frequent. Care to
>> bring an example where same serial engines have different DMAs and
>> different TZ? We do not talk about single QUP.
> 
> Well, I don't have access to the latest sm8550 / sm8650 devcfg sources.
> I checked the RB5 ones. As far as I understand out of 14 enabled SEs
> only two are configured for the GSI DMA, others should use FIFO / SE
> DMA. Same applies to the SM8250 MTP devices. Checking the RB1 / RB2
> setup also shows 3 out of 6 SEs being set for GSI.

But they are the same serial engines. Again, we do not talk here about
QUP, but serial engines.

Best regards,
Krzysztof

Re: [PATCH v2 4/8] dt-bindings: serial: Add support for selecting data transfer mode

[neil.armstrong]

On 29/01/2025 03:21, Dmitry Baryshkov wrote:
> On Mon, Jan 27, 2025 at 05:24:21PM +0100, Krzysztof Kozlowski wrote:
>> On 27/01/2025 15:27, Dmitry Baryshkov wrote:
>>> On Mon, Jan 27, 2025 at 08:02:12AM +0100, Krzysztof Kozlowski wrote:
>>>> On 24/01/2025 11:53, 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,xfer-mode' properties to select the data transfer mode,
>>>>> either GPI DMA (Generic Packet Interface) or non-GPI mode (PIO/CPU DMA).
>>>>>
>>>>> UART controller can operate in one of two modes based on the
>>>>> 'qcom,xfer-mode' property, and the firmware is loaded accordingly.
>>>>>
>>>>> 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>
>>>>> ---
>>>>>
>>>>> 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 | 8 ++++++++
>>>>>   1 file changed, 8 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..383773b32e47 100644
>>>>> --- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
>>>>> +++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
>>>>> @@ -56,6 +56,13 @@ properties:
>>>>>     reg:
>>>>>       maxItems: 1
>>>>>   
>>>>> +  qcom,xfer-mode:
>>>>> +    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) mode and 3 for GPI DMA mode.
>>>>> +      The default mode is FIFO.
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +    enum: [1, 3]
>>>>> +
>>>>> +
>>>>
>>>> Just one blank line, but anyway, this property should not be in three
>>>> places. Do you really expect that each of serial engines within one
>>>> GeniQUP will be configured differently by TZ?
>>>
>>> Yes, each SE is configured separately and it's quite frequent when
>>> different SEs have different DMA configuration.
>>
>> Well, I checked at sm8550 and sm8650 and each pair of SE - which shares
>> resources - has the same DMAs, so I would not call it frequent. Care to
>> bring an example where same serial engines have different DMAs and
>> different TZ? We do not talk about single QUP.
> 
> Well, I don't have access to the latest sm8550 / sm8650 devcfg sources.
> I checked the RB5 ones. As far as I understand out of 14 enabled SEs
> only two are configured for the GSI DMA, others should use FIFO / SE
> DMA. Same applies to the SM8250 MTP devices. Checking the RB1 / RB2
> setup also shows 3 out of 6 SEs being set for GSI.

I think selecting GSI DMA is only for devices needs high speed streaming to the
device, like the touch screen, using GSI DMA for random small access is a non-sense.

But the thing is, in the TZ world the configuration was static so we had no choice
of using GSI DMA when configured, but now we have the choice so we could totally
reconfigure the SE with the transfer type (FIFO, SE DMA or GSI DMA) as runtime and
drop this attribute.

So instead of hardcoding this, add a way to dynamically select either of the 3
transfer types when firmware can be loaded from HLOS.

Neil

> 
>>
>> Anyway, if you need property per node, this has to be shared schema.
>>
>> Best regards,
>> Krzysztof
> 

Re: [PATCH v2 4/8] dt-bindings: serial: Add support for selecting data transfer mode

[Viken Dadhaniya]

On 1/29/2025 1:48 PM, neil.armstrong@linaro.org wrote:
> On 29/01/2025 03:21, Dmitry Baryshkov wrote:
>> On Mon, Jan 27, 2025 at 05:24:21PM +0100, Krzysztof Kozlowski wrote:
>>> On 27/01/2025 15:27, Dmitry Baryshkov wrote:
>>>> On Mon, Jan 27, 2025 at 08:02:12AM +0100, Krzysztof Kozlowski wrote:
>>>>> On 24/01/2025 11:53, 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,xfer-mode' properties to select the data 
>>>>>> transfer mode,
>>>>>> either GPI DMA (Generic Packet Interface) or non-GPI mode (PIO/CPU 
>>>>>> DMA).
>>>>>>
>>>>>> UART controller can operate in one of two modes based on the
>>>>>> 'qcom,xfer-mode' property, and the firmware is loaded accordingly.
>>>>>>
>>>>>> 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>
>>>>>> ---
>>>>>>
>>>>>> 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 | 8 
>>>>>> ++++++++
>>>>>>   1 file changed, 8 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..383773b32e47 100644
>>>>>> --- 
>>>>>> a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
>>>>>> +++ 
>>>>>> b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
>>>>>> @@ -56,6 +56,13 @@ properties:
>>>>>>     reg:
>>>>>>       maxItems: 1
>>>>>> +  qcom,xfer-mode:
>>>>>> +    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) 
>>>>>> mode and 3 for GPI DMA mode.
>>>>>> +      The default mode is FIFO.
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +    enum: [1, 3]
>>>>>> +
>>>>>> +
>>>>>
>>>>> Just one blank line, but anyway, this property should not be in three
>>>>> places. Do you really expect that each of serial engines within one
>>>>> GeniQUP will be configured differently by TZ?
>>>>
>>>> Yes, each SE is configured separately and it's quite frequent when
>>>> different SEs have different DMA configuration.
>>>
>>> Well, I checked at sm8550 and sm8650 and each pair of SE - which shares
>>> resources - has the same DMAs, so I would not call it frequent. Care to
>>> bring an example where same serial engines have different DMAs and
>>> different TZ? We do not talk about single QUP.
>>
>> Well, I don't have access to the latest sm8550 / sm8650 devcfg sources.
>> I checked the RB5 ones. As far as I understand out of 14 enabled SEs
>> only two are configured for the GSI DMA, others should use FIFO / SE
>> DMA. Same applies to the SM8250 MTP devices. Checking the RB1 / RB2
>> setup also shows 3 out of 6 SEs being set for GSI.
> 
> I think selecting GSI DMA is only for devices needs high speed streaming 
> to the
> device, like the touch screen, using GSI DMA for random small access is 
> a non-sense.
> 
> But the thing is, in the TZ world the configuration was static so we had 
> no choice
> of using GSI DMA when configured, but now we have the choice so we could 
> totally
> reconfigure the SE with the transfer type (FIFO, SE DMA or GSI DMA) as 
> runtime and
> drop this attribute.
> 
> So instead of hardcoding this, add a way to dynamically select either of 
> the 3
> transfer types when firmware can be loaded from HLOS.
> 
> Neil
> 

Yes, GSI DMA mode is required for specific use cases only.

Dynamically switching from GSI mode to non-GSI mode is neither possible 
nor useful. For each SE, the use case is fixed, and based on the use 
case, the developer can choose the mode via the device tree property.

>>
>>>
>>> Anyway, if you need property per node, this has to be shared schema.
>>>
>>> Best regards,
>>> Krzysztof
>>
> 

Re: [PATCH v2 4/8] dt-bindings: serial: Add support for selecting data transfer mode

[Krzysztof Kozlowski]

On 09/02/2025 11:11, Viken Dadhaniya wrote:
>>>>>>
>>>>>> Just one blank line, but anyway, this property should not be in three
>>>>>> places. Do you really expect that each of serial engines within one
>>>>>> GeniQUP will be configured differently by TZ?
>>>>>
>>>>> Yes, each SE is configured separately and it's quite frequent when
>>>>> different SEs have different DMA configuration.
>>>>
>>>> Well, I checked at sm8550 and sm8650 and each pair of SE - which shares
>>>> resources - has the same DMAs, so I would not call it frequent. Care to
>>>> bring an example where same serial engines have different DMAs and
>>>> different TZ? We do not talk about single QUP.
>>>
>>> Well, I don't have access to the latest sm8550 / sm8650 devcfg sources.
>>> I checked the RB5 ones. As far as I understand out of 14 enabled SEs
>>> only two are configured for the GSI DMA, others should use FIFO / SE
>>> DMA. Same applies to the SM8250 MTP devices. Checking the RB1 / RB2
>>> setup also shows 3 out of 6 SEs being set for GSI.
>>
>> I think selecting GSI DMA is only for devices needs high speed streaming 
>> to the
>> device, like the touch screen, using GSI DMA for random small access is 
>> a non-sense.
>>
>> But the thing is, in the TZ world the configuration was static so we had 
>> no choice
>> of using GSI DMA when configured, but now we have the choice so we could 
>> totally
>> reconfigure the SE with the transfer type (FIFO, SE DMA or GSI DMA) as 
>> runtime and
>> drop this attribute.
>>
>> So instead of hardcoding this, add a way to dynamically select either of 
>> the 3
>> transfer types when firmware can be loaded from HLOS.
>>
>> Neil
>>
> 
> Yes, GSI DMA mode is required for specific use cases only.
> 
> Dynamically switching from GSI mode to non-GSI mode is neither possible 
> nor useful. For each SE, the use case is fixed, and based on the use 
> case, the developer can choose the mode via the device tree property.

No, it cannot. Do not describe downstream as something set in stone.

Best regards,
Krzysztof

Re: [PATCH v2 4/8] dt-bindings: serial: Add support for selecting data transfer mode

[Viken Dadhaniya]

On 2/9/2025 3:49 PM, Krzysztof Kozlowski wrote:
> On 09/02/2025 11:11, Viken Dadhaniya wrote:
>>>>>>>
>>>>>>> Just one blank line, but anyway, this property should not be in three
>>>>>>> places. Do you really expect that each of serial engines within one
>>>>>>> GeniQUP will be configured differently by TZ?
>>>>>>
>>>>>> Yes, each SE is configured separately and it's quite frequent when
>>>>>> different SEs have different DMA configuration.
>>>>>
>>>>> Well, I checked at sm8550 and sm8650 and each pair of SE - which shares
>>>>> resources - has the same DMAs, so I would not call it frequent. Care to
>>>>> bring an example where same serial engines have different DMAs and
>>>>> different TZ? We do not talk about single QUP.
>>>>
>>>> Well, I don't have access to the latest sm8550 / sm8650 devcfg sources.
>>>> I checked the RB5 ones. As far as I understand out of 14 enabled SEs
>>>> only two are configured for the GSI DMA, others should use FIFO / SE
>>>> DMA. Same applies to the SM8250 MTP devices. Checking the RB1 / RB2
>>>> setup also shows 3 out of 6 SEs being set for GSI.
>>>
>>> I think selecting GSI DMA is only for devices needs high speed streaming
>>> to the
>>> device, like the touch screen, using GSI DMA for random small access is
>>> a non-sense.
>>>
>>> But the thing is, in the TZ world the configuration was static so we had
>>> no choice
>>> of using GSI DMA when configured, but now we have the choice so we could
>>> totally
>>> reconfigure the SE with the transfer type (FIFO, SE DMA or GSI DMA) as
>>> runtime and
>>> drop this attribute.
>>>
>>> So instead of hardcoding this, add a way to dynamically select either of
>>> the 3
>>> transfer types when firmware can be loaded from HLOS.
>>>
>>> Neil
>>>
>>
To exactly summarize:
GSI DMA and CPU DMA are mostly same performance unless we have
multiprocessor systems queuing the transfers
together.

GSI DMA to be used when multiprocessor systems (Application 
processor/TZ/modem/ADSP subsystems) has use cases together.
If only single subsystem or processor is using CPU DMA mode should be used.

Hardware guidance and configuration suggest that CPU DMA and FIFO can be 
switched but GSI DMA.

CPI DMA : Doesn't work with multiple subsystems.
FIFO : same as CPU DMA but < 64 bytes (FIFO_SIZE)
GSI_DMA: Work with multiple subsystems

Overall, there will be GSI and non-GSI modes. Dynamic switching is only 
required in PIO mode (FIFO and CPU DMA).

>> Yes, GSI DMA mode is required for specific use cases only.
>>
>> Dynamically switching from GSI mode to non-GSI mode is neither possible
>> nor useful. For each SE, the use case is fixed, and based on the use
>> case, the developer can choose the mode via the device tree property.
> 
> No, it cannot. Do not describe downstream as something set in stone.

Sorry, I am not referring to downstream but rather to the general 
process of mode selection.

Please let us know if you need more clarity.

> 
> Best regards,
> Krzysztof

Re: [PATCH v2 4/8] dt-bindings: serial: Add support for selecting data transfer mode

[Viken Dadhaniya]

On 1/27/2025 9:54 PM, Krzysztof Kozlowski wrote:
> On 27/01/2025 15:27, Dmitry Baryshkov wrote:
>> On Mon, Jan 27, 2025 at 08:02:12AM +0100, Krzysztof Kozlowski wrote:
>>> On 24/01/2025 11:53, 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,xfer-mode' properties to select the data transfer mode,
>>>> either GPI DMA (Generic Packet Interface) or non-GPI mode (PIO/CPU DMA).
>>>>
>>>> UART controller can operate in one of two modes based on the
>>>> 'qcom,xfer-mode' property, and the firmware is loaded accordingly.
>>>>
>>>> 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>
>>>> ---
>>>>
>>>> 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 | 8 ++++++++
>>>>   1 file changed, 8 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..383773b32e47 100644
>>>> --- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
>>>> +++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
>>>> @@ -56,6 +56,13 @@ properties:
>>>>     reg:
>>>>       maxItems: 1
>>>>   
>>>> +  qcom,xfer-mode:
>>>> +    description: Set the value to 1 for non-GPI (FIFO/CPU DMA) mode and 3 for GPI DMA mode.
>>>> +      The default mode is FIFO.
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    enum: [1, 3]
>>>> +
>>>> +
>>>
>>> Just one blank line, but anyway, this property should not be in three
>>> places. Do you really expect that each of serial engines within one
>>> GeniQUP will be configured differently by TZ?
>>
>> Yes, each SE is configured separately and it's quite frequent when
>> different SEs have different DMA configuration.
> 
> Well, I checked at sm8550 and sm8650 and each pair of SE - which shares
> resources - has the same DMAs, so I would not call it frequent. Care to
> bring an example where same serial engines have different DMAs and
> different TZ? We do not talk about single QUP.
> 
> Anyway, if you need property per node, this has to be shared schema.

Yes, this property is required for each SE node.
Can we use qcom,geni-se.yaml as a common YAML for the shared schema? 
Please suggest.

> 
> Best regards,
> Krzysztof

[PATCH v2 5/8] 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>
---

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      | 444 +++++++++++++++++++++++++++
 include/linux/soc/qcom/geni-se.h     |  17 +
 include/linux/soc/qcom/qup-fw-load.h | 179 +++++++++++
 3 files changed, 640 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..069b3f50de75 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,445 @@ int geni_icc_disable(struct geni_se *se)
 }
 EXPORT_SYMBOL_GPL(geni_icc_disable);
 
+/**
+ * elf_phdr_valid: Function to validate elf header.
+ * @phdr: A pointer to a elf header.
+ *
+ * This function validates elf header by comparing fields
+ * stored in p_flags and payload type.
+ *
+ * return: true for success and false for failure/error case.
+ */
+static bool elf_phdr_valid(const struct elf32_phdr *phdr)
+{
+	if (phdr->p_type != PT_LOAD || !phdr->p_memsz)
+		return false;
+
+	if (phdr->p_type == PT_LOAD &&
+	    (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: Function to validate segment size.
+ * @pelfseg: A pointer to a elf header.
+ * @p_filesz: A pointer to file size.
+ *
+ * This function validates elf segment size by comparing file size
+ *
+ * return: Return true if segment is valid and false if 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: Function to read elf file.
+ * @rsc: A pointer to SE resources structure.
+ * @fw: A pointer to the fw buffer.
+ * @pelfseg: A pointer to SE specific elf header.
+ * @phdr: pointer to one of the valid headers from list from fw buffer.
+ *
+ * This function reads the ELF file and outputs the pointer to header
+ * data which contains the FW data and any other details.
+ *
+ * return: Return 0 if no error, else return error value.
+ */
+static int read_elf(struct qup_se_rsc *rsc, const struct firmware *fw,
+		    struct elf_se_hdr **pelfseg, struct elf32_phdr **phdr)
+{
+	struct elf32_phdr *phdrs;
+	const struct elf32_hdr *ehdr;
+	const u8 *addr;
+	int i;
+
+	ehdr = (struct elf32_hdr *)fw->data;
+
+	if (ehdr->e_phnum < 2)
+		return -EINVAL;
+
+	phdrs = (struct elf32_phdr *)(ehdr + 1);
+
+	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))
+				if ((*pelfseg)->serial_protocol == rsc->protocol &&
+				    (*pelfseg)->serial_protocol != GENI_SE_NONE)
+					return 0;
+		}
+	}
+	return -EINVAL;
+}
+
+/**
+ * geni_config_common_control: Function to configure common cgc
+ * and disable high priority interrupt.
+ * @rsc: A pointer to a structure representing SE related resources.
+ *
+ * This function configures cgc and disables high priority interrupt
+ * until 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: Function to set transfer mode.
+ * @rsc: A pointer to a structure representing SE related resources.
+ *
+ * This function sets transfer mode FIFO or DMA according to mode
+ * specified by protocol driver..
+ *
+ * return: Return 0 if no error, else return 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: Function to enable interrupts
+ * @rsc: A pointer to a structure representing SE related resources.
+ *
+ * This function enables required interrupt during 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: Function to flash revision
+ * @rsc: A pointer to a structure representing SE related resources.
+ * @hdr: A pointer to ELF header of Serial Engine.
+ * This function flash firmware revision and protocol in 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: Function to load serial engine specific firmware
+ * @rsc: A pointer to a structure representing SE related resources.
+ * @fw: A pointer to Firmware structure.
+ *
+ * This function loads the protocol FW at the IRAM of the SE.
+ *
+ * return: Return 0 if no error, else return 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 = 0;
+	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 to 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 to 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((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: Function to initiate firmware load
+ * @rsc: A pointer to a structure representing SE related resources.
+ *
+ * This function is called for loading the firmware into a particular
+ * SE. This is achieved by reading the associated ELF file, copying
+ * the data in the ELF file into buffer in kernel space using
+ * request_firmware API's. The data is then written in the SE's
+ * IRAM register and the buffers are freed after.  Overall, this
+ * function handles firmware loading and parsing for a specific protocol.
+ *
+ * return: Return 0 if no error, else return 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 = (rsc->protocol != GENI_SE_NONE) ? geni_load_se_fw(rsc, fw) : -EINVAL;
+
+	release_firmware(fw);
+
+	return ret;
+}
+
+/**
+ * geni_load_se_firmware: Function to initiate firmware loading.
+ * @se: Serial engine details.
+ * @protocol: protocol from spi, i2c or uart for which firmware to
+ * be loaded
+ *
+ * This function is called from the probe function of protocol driver.
+ * if dtsi properties are configured to load QUP firmware and firmware
+ * is already not loaded, it will start firmware loading. if dtsi
+ * properties are not defined,it will skip loading firmware assuming
+ * it is already loaded by TZ.
+ *
+ * return: Return 0 if no error, else return 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;
+	of_property_read_u32(se->dev->of_node, "qcom,xfer-mode", &rsc.mode);
+	switch (rsc.mode) {
+	case GENI_SE_FIFO:
+	case GENI_SE_DMA:
+	case GENI_GPI_DMA:
+		break;
+	default:
+		dev_err(se->dev, "Invalid xfer mode specified: %d\n", rsc.mode);
+		return -EINVAL;
+	}
+
+	ret = qup_fw_load(&rsc, fw_name);
+	if (ret) {
+		dev_err(se->dev,  "Firmware Loading failed for proto: %s Error: %d\n",
+			protocol_name[rsc.protocol], ret);
+		return ret;
+	}
+
+	dev_dbg(se->dev, "Firmware load for %s protocol is Success for xfer mode %d\n",
+		protocol_name[rsc.protocol], rsc.mode);
+	return ret;
+}
+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..289fa6675d2b 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
@@ -72,6 +73,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 +545,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..b9b58e81f5cb
--- /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 v2 5/8] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem

[Krzysztof Kozlowski]

On 24/01/2025 11:53, Viken Dadhaniya wrote:
>  /* Common SE registers */
> @@ -891,6 +896,445 @@ int geni_icc_disable(struct geni_se *se)
>  }
>  EXPORT_SYMBOL_GPL(geni_icc_disable);
>  
> +/**
> + * elf_phdr_valid: Function to validate elf header.
> + * @phdr: A pointer to a elf header.
> + *
> + * This function validates elf header by comparing fields

Drop "This function" and use imperative. It's redundant and you keep
using it everywherre here


...

> +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 = (rsc->protocol != GENI_SE_NONE) ? geni_load_se_fw(rsc, fw) : -EINVAL;

Drop ternary operator. Not easy to read.

> +
> +	release_firmware(fw);
> +
> +	return ret;
> +}

> +
> +/**
> + * geni_load_se_firmware: Function to initiate firmware loading.
> + * @se: Serial engine details.
> + * @protocol: protocol from spi, i2c or uart for which firmware to
> + * be loaded
> + *
> + * This function is called from the probe function of protocol driver.
> + * if dtsi properties are configured to load QUP firmware and firmware
> + * is already not loaded, it will start firmware loading. if dtsi
> + * properties are not defined,it will skip loading firmware assuming
> + * it is already loaded by TZ.
> + *
> + * return: Return 0 if no error, else return 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;
> +	of_property_read_u32(se->dev->of_node, "qcom,xfer-mode", &rsc.mode);
> +	switch (rsc.mode) {
> +	case GENI_SE_FIFO:
> +	case GENI_SE_DMA:

How value of 2 is acceptable? Your bindings said it is not.


> +	case GENI_GPI_DMA:
> +		break;
> +	default:
> +		dev_err(se->dev, "Invalid xfer mode specified: %d\n", rsc.mode);
> +		return -EINVAL;
> +	}
> +
> +	ret = qup_fw_load(&rsc, fw_name);
> +	if (ret) {
> +		dev_err(se->dev,  "Firmware Loading failed for proto: %s Error: %d\n",
> +			protocol_name[rsc.protocol], ret);

Aren't you printing same error multiple times?

> +		return ret;
> +	}
> +
> +	dev_dbg(se->dev, "Firmware load for %s protocol is Success for xfer mode %d\n",
> +		protocol_name[rsc.protocol], rsc.mode);
> +	return ret;
> +}
> +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..289fa6675d2b 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
> @@ -72,6 +73,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 +545,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..b9b58e81f5cb
> --- /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

Drop indentation after 'define'


Best regards,
Krzysztof

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

[Viken Dadhaniya]

On 1/27/2025 12:36 PM, Krzysztof Kozlowski wrote:
> On 24/01/2025 11:53, Viken Dadhaniya wrote:
>>   /* Common SE registers */
>> @@ -891,6 +896,445 @@ int geni_icc_disable(struct geni_se *se)
>>   }
>>   EXPORT_SYMBOL_GPL(geni_icc_disable);
>>   
>> +/**
>> + * elf_phdr_valid: Function to validate elf header.
>> + * @phdr: A pointer to a elf header.
>> + *
>> + * This function validates elf header by comparing fields
> 
> Drop "This function" and use imperative. It's redundant and you keep
> using it everywherre here
>  

Updated in V3.

> ...
> 
>> +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 = (rsc->protocol != GENI_SE_NONE) ? geni_load_se_fw(rsc, fw) : -EINVAL;
> 
> Drop ternary operator. Not easy to read
Updated in V3.

> 
>> +
>> +	release_firmware(fw);
>> +
>> +	return ret;
>> +}
> 
>> +
>> +/**
>> + * geni_load_se_firmware: Function to initiate firmware loading.
>> + * @se: Serial engine details.
>> + * @protocol: protocol from spi, i2c or uart for which firmware to
>> + * be loaded
>> + *
>> + * This function is called from the probe function of protocol driver.
>> + * if dtsi properties are configured to load QUP firmware and firmware
>> + * is already not loaded, it will start firmware loading. if dtsi
>> + * properties are not defined,it will skip loading firmware assuming
>> + * it is already loaded by TZ.
>> + *
>> + * return: Return 0 if no error, else return 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;
>> +	of_property_read_u32(se->dev->of_node, "qcom,xfer-mode", &rsc.mode);
>> +	switch (rsc.mode) {
>> +	case GENI_SE_FIFO:
>> +	case GENI_SE_DMA:
> 
> How value of 2 is acceptable? Your bindings said it is not.

Corrected in V3.

> 
> 
>> +	case GENI_GPI_DMA:
>> +		break;
>> +	default:
>> +		dev_err(se->dev, "Invalid xfer mode specified: %d\n", rsc.mode);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = qup_fw_load(&rsc, fw_name);
>> +	if (ret) {
>> +		dev_err(se->dev,  "Firmware Loading failed for proto: %s Error: %d\n",
>> +			protocol_name[rsc.protocol], ret);
> 
> Aren't you printing same error multiple times?

Removed in V3.

> 
>> +		return ret;
>> +	}
>> +
>> +	dev_dbg(se->dev, "Firmware load for %s protocol is Success for xfer mode %d\n",
>> +		protocol_name[rsc.protocol], rsc.mode);
>> +	return ret;
>> +}
>> +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..289fa6675d2b 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
>> @@ -72,6 +73,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 +545,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..b9b58e81f5cb
>> --- /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
> 
> Drop indentation after 'define'

Updated in V3.

> 
> 
> Best regards,
> Krzysztof

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

[Mukesh Kumar Savaliya]

On 1/24/2025 4:23 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>
> ---
> 
> 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      | 444 +++++++++++++++++++++++++++
>   include/linux/soc/qcom/geni-se.h     |  17 +
>   include/linux/soc/qcom/qup-fw-load.h | 179 +++++++++++
>   3 files changed, 640 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..069b3f50de75 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,445 @@ int geni_icc_disable(struct geni_se *se)
>   }
>   EXPORT_SYMBOL_GPL(geni_icc_disable);
>   
> +/**
> + * elf_phdr_valid: Function to validate elf header.
> + * @phdr: A pointer to a elf header.
> + *
> + * This function validates elf header by comparing fields
> + * stored in p_flags and payload type.
> + *
> + * return: true for success and false for failure/error case.
> + */
> +static bool elf_phdr_valid(const struct elf32_phdr *phdr)
> +{
> +	if (phdr->p_type != PT_LOAD || !phdr->p_memsz)
> +		return false;
> +
> +	if (phdr->p_type == PT_LOAD &&
> +	    (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: Function to validate segment size.
> + * @pelfseg: A pointer to a elf header.
> + * @p_filesz: A pointer to file size.
> + *
> + * This function validates elf segment size by comparing file size
> + *
> + * return: Return true if segment is valid and false if 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: Function to read elf file.
> + * @rsc: A pointer to SE resources structure.
> + * @fw: A pointer to the fw buffer.
> + * @pelfseg: A pointer to SE specific elf header.
> + * @phdr: pointer to one of the valid headers from list from fw buffer.
> + *
> + * This function reads the ELF file and outputs the pointer to header
> + * data which contains the FW data and any other details.
> + *
> + * return: Return 0 if no error, else return error value.
> + */
> +static int read_elf(struct qup_se_rsc *rsc, const struct firmware *fw,
> +		    struct elf_se_hdr **pelfseg, struct elf32_phdr **phdr)
> +{
> +	struct elf32_phdr *phdrs;
> +	const struct elf32_hdr *ehdr;
> +	const u8 *addr;
> +	int i;
> +
> +	ehdr = (struct elf32_hdr *)fw->data;
> +
> +	if (ehdr->e_phnum < 2)
> +		return -EINVAL;
> +
> +	phdrs = (struct elf32_phdr *)(ehdr + 1);
> +
> +	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))
> +				if ((*pelfseg)->serial_protocol == rsc->protocol &&
> +				    (*pelfseg)->serial_protocol != GENI_SE_NONE)
> +					return 0;
> +		}
> +	}
> +	return -EINVAL;
> +}
> +
> +/**
> + * geni_config_common_control: Function to configure common cgc
> + * and disable high priority interrupt.
> + * @rsc: A pointer to a structure representing SE related resources.
> + *
> + * This function configures cgc and disables high priority interrupt
> + * until 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: Function to set transfer mode.
> + * @rsc: A pointer to a structure representing SE related resources.
> + *
> + * This function sets transfer mode FIFO or DMA according to mode
> + * specified by protocol driver..
> + *
> + * return: Return 0 if no error, else return 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: Function to enable interrupts
> + * @rsc: A pointer to a structure representing SE related resources.
> + *
> + * This function enables required interrupt during 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: Function to flash revision
> + * @rsc: A pointer to a structure representing SE related resources.
> + * @hdr: A pointer to ELF header of Serial Engine.
> + * This function flash firmware revision and protocol in 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: Function to load serial engine specific firmware
> + * @rsc: A pointer to a structure representing SE related resources.
> + * @fw: A pointer to Firmware structure.
> + *
> + * This function loads the protocol FW at the IRAM of the SE.
> + *
> + * return: Return 0 if no error, else return 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 = 0;
> +	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 to 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 to 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((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: Function to initiate firmware load
> + * @rsc: A pointer to a structure representing SE related resources.
> + *
> + * This function is called for loading the firmware into a particular
> + * SE. This is achieved by reading the associated ELF file, copying
> + * the data in the ELF file into buffer in kernel space using
> + * request_firmware API's. The data is then written in the SE's
> + * IRAM register and the buffers are freed after.  Overall, this
> + * function handles firmware loading and parsing for a specific protocol.
> + *
> + * return: Return 0 if no error, else return 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 = (rsc->protocol != GENI_SE_NONE) ? geni_load_se_fw(rsc, fw) : -EINVAL;
> +
> +	release_firmware(fw);
> +
> +	return ret;
> +}
> +
> +/**
> + * geni_load_se_firmware: Function to initiate firmware loading.
> + * @se: Serial engine details.
> + * @protocol: protocol from spi, i2c or uart for which firmware to
> + * be loaded
> + *
> + * This function is called from the probe function of protocol driver.
> + * if dtsi properties are configured to load QUP firmware and firmware
> + * is already not loaded, it will start firmware loading. if dtsi
> + * properties are not defined,it will skip loading firmware assuming
> + * it is already loaded by TZ.
> + *
> + * return: Return 0 if no error, else return 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;
This needs to be moved after reading  "qcom,xfer-mode", there you can 
override.
Also please mention UART doesn't support GPI_DMA mode hence can override 
with SE_FIFO.
> +	of_property_read_u32(se->dev->of_node, "qcom,xfer-mode", &rsc.mode);
> +	switch (rsc.mode) {
> +	case GENI_SE_FIFO:
> +	case GENI_SE_DMA:
> +	case GENI_GPI_DMA:
> +		break;
> +	default:
> +		dev_err(se->dev, "Invalid xfer mode specified: %d\n", rsc.mode);
> +		return -EINVAL;
> +	}
> +
> +	ret = qup_fw_load(&rsc, fw_name);
> +	if (ret) {
> +		dev_err(se->dev,  "Firmware Loading failed for proto: %s Error: %d\n",
> +			protocol_name[rsc.protocol], ret);
> +		return ret;
> +	}
> +
> +	dev_dbg(se->dev, "Firmware load for %s protocol is Success for xfer mode %d\n",
> +		protocol_name[rsc.protocol], rsc.mode);
> +	return ret;
> +}
> +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..289fa6675d2b 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
> @@ -72,6 +73,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 +545,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..b9b58e81f5cb
> --- /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 */

[PATCH v2 6/8] 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>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 7bbd478171e0..9ad3b8c9a224 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -872,8 +872,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
 	}
 	proto = geni_se_read_proto(&gi2c->se);
 	if (proto != GENI_SE_I2C) {
-		ret = dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto);
-		goto err_resources;
+		ret = geni_load_se_firmware(&gi2c->se, GENI_SE_I2C);
+		if (ret) {
+			dev_err(gi2c->se.dev, "i2c firmware load failed ret: %d\n", ret);
+			goto err_resources;
+		}
 	}
 
 	if (desc && desc->no_dma_support)
-- 
2.34.1

Re: [PATCH v2 6/8] i2c: qcom-geni: Load i2c qup Firmware from linux side

[Dmitry Baryshkov]

On Fri, Jan 24, 2025 at 04:23:07PM +0530, Viken Dadhaniya wrote:
> 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>
> ---
>  drivers/i2c/busses/i2c-qcom-geni.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 7bbd478171e0..9ad3b8c9a224 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -872,8 +872,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  	}
>  	proto = geni_se_read_proto(&gi2c->se);
>  	if (proto != GENI_SE_I2C) {
> -		ret = dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto);
> -		goto err_resources;
> +		ret = geni_load_se_firmware(&gi2c->se, GENI_SE_I2C);

Hmm, so if the SE has been configured to e.g. SPI by the TZ, can we
switch it to the I2C?

> +		if (ret) {
> +			dev_err(gi2c->se.dev, "i2c firmware load failed ret: %d\n", ret);
> +			goto err_resources;
> +		}
>  	}
>  
>  	if (desc && desc->no_dma_support)
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 6/8] i2c: qcom-geni: Load i2c qup Firmware from linux side

[Viken Dadhaniya]

On 1/24/2025 8:34 PM, Dmitry Baryshkov wrote:
> On Fri, Jan 24, 2025 at 04:23:07PM +0530, Viken Dadhaniya wrote:
>> 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>
>> ---
>>   drivers/i2c/busses/i2c-qcom-geni.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 7bbd478171e0..9ad3b8c9a224 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -872,8 +872,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
>>   	}
>>   	proto = geni_se_read_proto(&gi2c->se);
>>   	if (proto != GENI_SE_I2C) {
>> -		ret = dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto);
>> -		goto err_resources;
>> +		ret = geni_load_se_firmware(&gi2c->se, GENI_SE_I2C);
> 
> Hmm, so if the SE has been configured to e.g. SPI by the TZ, can we
> switch it to the I2C?

No, in the current design, TZ will not load the SE firmware.

> 
>> +		if (ret) {
>> +			dev_err(gi2c->se.dev, "i2c firmware load failed ret: %d\n", ret);
>> +			goto err_resources;
>> +		}
>>   	}
>>   
>>   	if (desc && desc->no_dma_support)
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH v2 6/8] i2c: qcom-geni: Load i2c qup Firmware from linux side

[Dmitry Baryshkov]

On Fri, 24 Jan 2025 at 17:24, Viken Dadhaniya <quic_vdadhani@quicinc.com> wrote:
>
>
>
> On 1/24/2025 8:34 PM, Dmitry Baryshkov wrote:
> > On Fri, Jan 24, 2025 at 04:23:07PM +0530, Viken Dadhaniya wrote:
> >> 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>
> >> ---
> >>   drivers/i2c/busses/i2c-qcom-geni.c | 7 +++++--
> >>   1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> >> index 7bbd478171e0..9ad3b8c9a224 100644
> >> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> >> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> >> @@ -872,8 +872,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
> >>      }
> >>      proto = geni_se_read_proto(&gi2c->se);
> >>      if (proto != GENI_SE_I2C) {
> >> -            ret = dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto);
> >> -            goto err_resources;
> >> +            ret = geni_load_se_firmware(&gi2c->se, GENI_SE_I2C);
> >
> > Hmm, so if the SE has been configured to e.g. SPI by the TZ, can we
> > switch it to the I2C?
>
> No, in the current design, TZ will not load the SE firmware.

But that's what your patch is doing: if the protocol is not I2C, try
switching to I2C.

Instead it should be 'if unconfigured, try loading I2C'.

>
> >
> >> +            if (ret) {
> >> +                    dev_err(gi2c->se.dev, "i2c firmware load failed ret: %d\n", ret);
> >> +                    goto err_resources;
> >> +            }
> >>      }
> >>
> >>      if (desc && desc->no_dma_support)
> >> --
> >> 2.34.1
> >>
> >



-- 
With best wishes
Dmitry

Re: [PATCH v2 6/8] i2c: qcom-geni: Load i2c qup Firmware from linux side

[Viken Dadhaniya]

On 1/24/2025 9:25 PM, Dmitry Baryshkov wrote:
> On Fri, 24 Jan 2025 at 17:24, Viken Dadhaniya <quic_vdadhani@quicinc.com> wrote:
>>
>>
>>
>> On 1/24/2025 8:34 PM, Dmitry Baryshkov wrote:
>>> On Fri, Jan 24, 2025 at 04:23:07PM +0530, Viken Dadhaniya wrote:
>>>> 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>
>>>> ---
>>>>    drivers/i2c/busses/i2c-qcom-geni.c | 7 +++++--
>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>>>> index 7bbd478171e0..9ad3b8c9a224 100644
>>>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>>>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>>>> @@ -872,8 +872,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
>>>>       }
>>>>       proto = geni_se_read_proto(&gi2c->se);
>>>>       if (proto != GENI_SE_I2C) {
>>>> -            ret = dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto);
>>>> -            goto err_resources;
>>>> +            ret = geni_load_se_firmware(&gi2c->se, GENI_SE_I2C);
>>>
>>> Hmm, so if the SE has been configured to e.g. SPI by the TZ, can we
>>> switch it to the I2C?
>>
>> No, in the current design, TZ will not load the SE firmware.
> 
> But that's what your patch is doing: if the protocol is not I2C, try
> switching to I2C.
> 
> Instead it should be 'if unconfigured, try loading I2C'.

Sure, I will update it in the next patch.

> 
>>
>>>
>>>> +            if (ret) {
>>>> +                    dev_err(gi2c->se.dev, "i2c firmware load failed ret: %d\n", ret);
>>>> +                    goto err_resources;
>>>> +            }
>>>>       }
>>>>
>>>>       if (desc && desc->no_dma_support)
>>>> --
>>>> 2.34.1
>>>>
>>>
> 
> 
> 

[PATCH v2 7/8] 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>
---
 drivers/spi/spi-geni-qcom.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 768d7482102a..f4f6d8210245 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -672,8 +672,11 @@ static int spi_geni_init(struct spi_geni_master *mas)
 		}
 		spi_slv_setup(mas);
 	} else if (proto != GENI_SE_SPI) {
-		dev_err(mas->dev, "Invalid proto %d\n", proto);
-		goto out_pm;
+		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;
+		}
 	}
 	mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se);
 
-- 
2.34.1

[PATCH v2 8/8] 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>
---
 drivers/tty/serial/qcom_geni_serial.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index a80ce7aaf309..e3b0fc65f3bb 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1146,8 +1146,11 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
 
 	proto = geni_se_read_proto(&port->se);
 	if (proto != GENI_SE_UART) {
-		dev_err(uport->dev, "Invalid FW loaded, proto: %d\n", proto);
-		return -ENXIO;
+		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;
+		}
 	}
 
 	qcom_geni_serial_stop_rx(uport);
-- 
2.34.1