[PATCH v1 0/7] Add support to load QUP SE firmware from

[PATCH v1 0/7] 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.

Viken Dadhaniya (7):
  dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP
    firmware loading
  spi: dt-bindings: Document DT properties for QUP firmware loading
  dt-bindings: serial: Document DT properties for QUP firmware loading
  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      |  11 +
 .../serial/qcom,serial-geni-qcom.yaml         |  12 +
 .../bindings/spi/qcom,spi-geni-qcom.yaml      |  11 +
 drivers/i2c/busses/i2c-qcom-geni.c            |  11 +-
 drivers/soc/qcom/qcom-geni-se.c               | 445 ++++++++++++++++++
 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 +++++++
 9 files changed, 692 insertions(+), 8 deletions(-)
 create mode 100644 include/linux/soc/qcom/qup-fw-load.h

-- 
2.34.1

[PATCH v1 1/7] dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP firmware loading

[Viken Dadhaniya]

Document the 'qcom,load-firmware' and 'qcom,xfer-mode' properties to
support SE(Serial Engine) firmware loading from the protocol driver and 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>
---
 .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml   | 11 +++++++++++
 1 file changed, 11 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..a26f34fce1bb 100644
--- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
+++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
@@ -66,6 +66,15 @@ properties:
   required-opps:
     maxItems: 1
 
+  qcom,load-firmware:
+    type: boolean
+    description: Optional property to load SE (serial engine) Firmware from protocol driver.
+
+  qcom,xfer-mode:
+    description: Value 1,2 and 3 represents FIFO, CPU DMA and GSI DMA mode respectively.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1, 2, 3]
+
 required:
   - compatible
   - interrupts
@@ -142,5 +151,7 @@ examples:
         interconnect-names = "qup-core", "qup-config", "qup-memory";
         power-domains = <&rpmhpd SC7180_CX>;
         required-opps = <&rpmhpd_opp_low_svs>;
+        qcom,load-firmware;
+        qcom,xfer-mode = <1>;
     };
 ...
-- 
2.34.1

Re: [PATCH v1 1/7] dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP firmware loading

[Krzysztof Kozlowski]

On 04/12/2024 16:03, Viken Dadhaniya wrote:
> Document the 'qcom,load-firmware' and 'qcom,xfer-mode' properties to
> support SE(Serial Engine) firmware loading from the protocol driver and to
> select the data transfer mode, either GPI DMA (Generic Packet Interface)
> or non-GPI mode (PIO/CPU DMA).


You described the desired Linux feature or behavior, not the actual
hardware. The bindings are about the latter, so instead you need to
rephrase the property and its description to match actual hardware
capabilities/features/configuration etc.

I don't quite get why firmware-name is not suitable here, what is
"protocol driver" in this context and how firmware is loaded from it?

> 
> 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>
> ---
>  .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml   | 11 +++++++++++
>  1 file changed, 11 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..a26f34fce1bb 100644
> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> @@ -66,6 +66,15 @@ properties:
>    required-opps:
>      maxItems: 1
>  
> +  qcom,load-firmware:
> +    type: boolean
> +    description: Optional property to load SE (serial engine) Firmware from protocol driver.


Please wrap code according to coding style (checkpatch is not a coding
style description, but only a tool).


> +
> +  qcom,xfer-mode:
> +    description: Value 1,2 and 3 represents FIFO, CPU DMA and GSI DMA mode respectively.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 3]


Use string but anyway this would need some changes and explanation why
lack of DMA cannot be used to determine that. CPU DMA and GSI DMA also
need some background.



Best regards,
Krzysztof

Re: [PATCH v1 1/7] dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP firmware loading

[Viken Dadhaniya]

Thanks Krzysztof for the review and helpful comments.

On 12/4/2024 8:36 PM, Krzysztof Kozlowski wrote:
> On 04/12/2024 16:03, Viken Dadhaniya wrote:
>> Document the 'qcom,load-firmware' and 'qcom,xfer-mode' properties to
>> support SE(Serial Engine) firmware loading from the protocol driver and to
>> select the data transfer mode, either GPI DMA (Generic Packet Interface)
>> or non-GPI mode (PIO/CPU DMA).
> 
> 
> You described the desired Linux feature or behavior, not the actual
> hardware. The bindings are about the latter, so instead you need to
> rephrase the property and its description to match actual hardware
> capabilities/features/configuration etc.
Sure, IIUC, i should explain the need of FW loading. Agree that binding 
is for the hardware. This feature needs to have some intelligence to 
know that Software driver needs to load Firmware or not ?

Let me add description about the actual hardware capabilities, its 
features. Hope this can be better from my side on V2.
> 
> I don't quite get why firmware-name is not suitable here, what is
> "protocol driver" in this context and how firmware is loaded from it?
> 
yes, as per Dmitry's comment, i should replace with 
/soc/sc7180/firmware.  This would be become "firmware-name" property 
instead of qcom,load-firmware.

>>
>> 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>
>> ---
>>   .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml   | 11 +++++++++++
>>   1 file changed, 11 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..a26f34fce1bb 100644
>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> @@ -66,6 +66,15 @@ properties:
>>     required-opps:
>>       maxItems: 1
>>   
>> +  qcom,load-firmware:
>> +    type: boolean
>> +    description: Optional property to load SE (serial engine) Firmware from protocol driver.
> 
> 
> Please wrap code according to coding style (checkpatch is not a coding
> style description, but only a tool).
Actually i have ran dt-schema for yaml validation. I couldn't get if you 
have any comment for description statement OR it's related to code ? 
Could you please be more descriptive so i can adopt the suggestions.
> 
> 
>> +
>> +  qcom,xfer-mode:
>> +    description: Value 1,2 and 3 represents FIFO, CPU DMA and GSI DMA mode respectively.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [1, 2, 3]
> 
> 
> Use string but anyway this would need some changes and explanation why
> lack of DMA cannot be used to determine that. CPU DMA and GSI DMA also
> need some background.
Sure, i got it.
I need to add enum strings and shall provide explanation abut modes in use.
As per Doug's comment, we plan to keep GSI and non-GSI mode. It would be 
more clear in next patch.
> 
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v1 1/7] dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP firmware loading

[Krzysztof Kozlowski]

On 10/12/2024 05:43, Viken Dadhaniya wrote:
>>>       maxItems: 1
>>>   
>>> +  qcom,load-firmware:
>>> +    type: boolean
>>> +    description: Optional property to load SE (serial engine) Firmware from protocol driver.
>>
>>
>> Please wrap code according to coding style (checkpatch is not a coding
>> style description, but only a tool).
> Actually i have ran dt-schema for yaml validation. I couldn't get if you 
> have any comment for description statement OR it's related to code ? 
> Could you please be more descriptive so i can adopt the suggestions.

This code is not conforming to coding style in terms of wrapping, what
dtschema has to do with it? Please read coding style.



Best regards,
Krzysztof

Re: [PATCH v1 1/7] dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP firmware loading

[neil.armstrong]

On 04/12/2024 16:03, Viken Dadhaniya wrote:
> Document the 'qcom,load-firmware' and 'qcom,xfer-mode' properties to
> support SE(Serial Engine) firmware loading from the protocol driver and 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>
> ---
>   .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml   | 11 +++++++++++
>   1 file changed, 11 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..a26f34fce1bb 100644
> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> @@ -66,6 +66,15 @@ properties:
>     required-opps:
>       maxItems: 1
>   
> +  qcom,load-firmware:
> +    type: boolean
> +    description: Optional property to load SE (serial engine) Firmware from protocol driver.
> +
> +  qcom,xfer-mode:
> +    description: Value 1,2 and 3 represents FIFO, CPU DMA and GSI DMA mode respectively.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 3]

In the code, FIFO mode is default if not specified, please precise it in the
bindings aswell.

Thanks,
Neil

> +
>   required:
>     - compatible
>     - interrupts
> @@ -142,5 +151,7 @@ examples:
>           interconnect-names = "qup-core", "qup-config", "qup-memory";
>           power-domains = <&rpmhpd SC7180_CX>;
>           required-opps = <&rpmhpd_opp_low_svs>;
> +        qcom,load-firmware;
> +        qcom,xfer-mode = <1>;
>       };
>   ...

Re: [PATCH v1 1/7] dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP firmware loading

[Viken Dadhaniya]

On 12/4/2024 8:55 PM, neil.armstrong@linaro.org wrote:
> On 04/12/2024 16:03, Viken Dadhaniya wrote:
>> Document the 'qcom,load-firmware' and 'qcom,xfer-mode' properties to
>> support SE(Serial Engine) firmware loading from the protocol driver 
>> and 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>
>> ---
>>   .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml   | 11 +++++++++++
>>   1 file changed, 11 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..a26f34fce1bb 100644
>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> @@ -66,6 +66,15 @@ properties:
>>     required-opps:
>>       maxItems: 1
>> +  qcom,load-firmware:
>> +    type: boolean
>> +    description: Optional property to load SE (serial engine) 
>> Firmware from protocol driver.
>> +
>> +  qcom,xfer-mode:
>> +    description: Value 1,2 and 3 represents FIFO, CPU DMA and GSI DMA 
>> mode respectively.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [1, 2, 3]
> 
> In the code, FIFO mode is default if not specified, please precise it in 
> the
> bindings aswell.
> 
> Thanks,
> Neil

Sure will update in next patch.

> 
>> +
>>   required:
>>     - compatible
>>     - interrupts
>> @@ -142,5 +151,7 @@ examples:
>>           interconnect-names = "qup-core", "qup-config", "qup-memory";
>>           power-domains = <&rpmhpd SC7180_CX>;
>>           required-opps = <&rpmhpd_opp_low_svs>;
>> +        qcom,load-firmware;
>> +        qcom,xfer-mode = <1>;
>>       };
>>   ...
> 

Re: [PATCH v1 1/7] dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP firmware loading

[Doug Anderson]

Hi,

On Wed, Dec 4, 2024 at 7:03 AM Viken Dadhaniya
<quic_vdadhani@quicinc.com> wrote:
>
> Document the 'qcom,load-firmware' and 'qcom,xfer-mode' properties to
> support SE(Serial Engine) firmware loading from the protocol driver and 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>
> ---
>  .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml   | 11 +++++++++++
>  1 file changed, 11 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..a26f34fce1bb 100644
> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> @@ -66,6 +66,15 @@ properties:
>    required-opps:
>      maxItems: 1
>
> +  qcom,load-firmware:
> +    type: boolean
> +    description: Optional property to load SE (serial engine) Firmware from protocol driver.
> +
> +  qcom,xfer-mode:
> +    description: Value 1,2 and 3 represents FIFO, CPU DMA and GSI DMA mode respectively.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 3]

I'm a little confused about this. I'll admit I haven't fully analyzed
your patch with actual code in it, but in the past "CPU DMA" mode and
"FIFO" mode were compatible with each other and then it was up to the
driver to decide which of the two modes made sense in any given
situation. For instance, last I looked at the i2c driver it tried to
use DMA for large transfers and FIFO for small transfers. The SPI
driver also has some cases where it will use DMA mode and then
fallback to FIFO mode.

...so what exactly is the point of differentiating between "FIFO" and
"CPU DMA" mode here?

Then when it comes to "GSI DMA" mode, my understanding is that the
firmware for "GSI DMA" mode is always loaded by Trustzone because the
whole point is that the GSI mode arbitrates between multiple clients.
Presumably if the firmware already loaded the GSI firmware then the
code would just detect that case. ...so there shouldn't need to be any
reason to specify GSI mode here either, right?

-Doug

Re: [PATCH v1 1/7] dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP firmware loading

[Viken Dadhaniya]

On 12/4/2024 10:55 PM, Doug Anderson wrote:
> Hi,
> 
> On Wed, Dec 4, 2024 at 7:03 AM Viken Dadhaniya
> <quic_vdadhani@quicinc.com> wrote:
>>
>> Document the 'qcom,load-firmware' and 'qcom,xfer-mode' properties to
>> support SE(Serial Engine) firmware loading from the protocol driver and 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>
>> ---
>>   .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml   | 11 +++++++++++
>>   1 file changed, 11 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..a26f34fce1bb 100644
>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> @@ -66,6 +66,15 @@ properties:
>>     required-opps:
>>       maxItems: 1
>>
>> +  qcom,load-firmware:
>> +    type: boolean
>> +    description: Optional property to load SE (serial engine) Firmware from protocol driver.
>> +
>> +  qcom,xfer-mode:
>> +    description: Value 1,2 and 3 represents FIFO, CPU DMA and GSI DMA mode respectively.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [1, 2, 3]
> 
> I'm a little confused about this. I'll admit I haven't fully analyzed
> your patch with actual code in it, but in the past "CPU DMA" mode and
> "FIFO" mode were compatible with each other and then it was up to the
> driver to decide which of the two modes made sense in any given
> situation. For instance, last I looked at the i2c driver it tried to
> use DMA for large transfers and FIFO for small transfers. The SPI
> driver also has some cases where it will use DMA mode and then
> fallback to FIFO mode.
> 
> ...so what exactly is the point of differentiating between "FIFO" and
> "CPU DMA" mode here?

Yes, correct, Will update in V2.
I plan to add 2 modes, GSI and non-GSI(PIO or DMA based on length).

> 
> Then when it comes to "GSI DMA" mode, my understanding is that the
> firmware for "GSI DMA" mode is always loaded by Trustzone because the
> whole point is that the GSI mode arbitrates between multiple clients.
> Presumably if the firmware already loaded the GSI firmware then the
> code would just detect that case. ...so there shouldn't need to be any
> reason to specify GSI mode here either, right?
> 
> -Doug

GSI firmware is loaded from TZ per QUP, but to use GSI mode,
we need to configure the SE to use GSI mode by writing into SE register 
QUPV3_SE_GENI_DMA_MODE_EN and SE_GSI_EVENT_EN. This register is
used to configure data transfer mode for Serial Engine.

Re: [PATCH v1 1/7] dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP firmware loading

[Doug Anderson]

Hi,

On Mon, Dec 9, 2024 at 9:28 PM Viken Dadhaniya
<quic_vdadhani@quicinc.com> wrote:
>
> On 12/4/2024 10:55 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Dec 4, 2024 at 7:03 AM Viken Dadhaniya
> > <quic_vdadhani@quicinc.com> wrote:
> >>
> >> Document the 'qcom,load-firmware' and 'qcom,xfer-mode' properties to
> >> support SE(Serial Engine) firmware loading from the protocol driver and 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>
> >> ---
> >>   .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml   | 11 +++++++++++
> >>   1 file changed, 11 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..a26f34fce1bb 100644
> >> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> >> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> >> @@ -66,6 +66,15 @@ properties:
> >>     required-opps:
> >>       maxItems: 1
> >>
> >> +  qcom,load-firmware:
> >> +    type: boolean
> >> +    description: Optional property to load SE (serial engine) Firmware from protocol driver.
> >> +
> >> +  qcom,xfer-mode:
> >> +    description: Value 1,2 and 3 represents FIFO, CPU DMA and GSI DMA mode respectively.
> >> +    $ref: /schemas/types.yaml#/definitions/uint32
> >> +    enum: [1, 2, 3]
> >
> > I'm a little confused about this. I'll admit I haven't fully analyzed
> > your patch with actual code in it, but in the past "CPU DMA" mode and
> > "FIFO" mode were compatible with each other and then it was up to the
> > driver to decide which of the two modes made sense in any given
> > situation. For instance, last I looked at the i2c driver it tried to
> > use DMA for large transfers and FIFO for small transfers. The SPI
> > driver also has some cases where it will use DMA mode and then
> > fallback to FIFO mode.
> >
> > ...so what exactly is the point of differentiating between "FIFO" and
> > "CPU DMA" mode here?
>
> Yes, correct, Will update in V2.
> I plan to add 2 modes, GSI and non-GSI(PIO or DMA based on length).
>
> >
> > Then when it comes to "GSI DMA" mode, my understanding is that the
> > firmware for "GSI DMA" mode is always loaded by Trustzone because the
> > whole point is that the GSI mode arbitrates between multiple clients.
> > Presumably if the firmware already loaded the GSI firmware then the
> > code would just detect that case. ...so there shouldn't need to be any
> > reason to specify GSI mode here either, right?
> >
> > -Doug
>
> GSI firmware is loaded from TZ per QUP, but to use GSI mode,
> we need to configure the SE to use GSI mode by writing into SE register
> QUPV3_SE_GENI_DMA_MODE_EN and SE_GSI_EVENT_EN. This register is
> used to configure data transfer mode for Serial Engine.

Can't you detect it's in GSI mode without any device tree property
like the code does today?

-Doug

Re: [PATCH v1 1/7] dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP firmware loading

[Viken Dadhaniya]

On 12/10/2024 11:12 PM, Doug Anderson wrote:
> Hi,
> 
> On Mon, Dec 9, 2024 at 9:28 PM Viken Dadhaniya
> <quic_vdadhani@quicinc.com> wrote:
>>
>> On 12/4/2024 10:55 PM, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Wed, Dec 4, 2024 at 7:03 AM Viken Dadhaniya
>>> <quic_vdadhani@quicinc.com> wrote:
>>>>
>>>> Document the 'qcom,load-firmware' and 'qcom,xfer-mode' properties to
>>>> support SE(Serial Engine) firmware loading from the protocol driver and 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>
>>>> ---
>>>>    .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml   | 11 +++++++++++
>>>>    1 file changed, 11 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..a26f34fce1bb 100644
>>>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>>> @@ -66,6 +66,15 @@ properties:
>>>>      required-opps:
>>>>        maxItems: 1
>>>>
>>>> +  qcom,load-firmware:
>>>> +    type: boolean
>>>> +    description: Optional property to load SE (serial engine) Firmware from protocol driver.
>>>> +
>>>> +  qcom,xfer-mode:
>>>> +    description: Value 1,2 and 3 represents FIFO, CPU DMA and GSI DMA mode respectively.
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    enum: [1, 2, 3]
>>>
>>> I'm a little confused about this. I'll admit I haven't fully analyzed
>>> your patch with actual code in it, but in the past "CPU DMA" mode and
>>> "FIFO" mode were compatible with each other and then it was up to the
>>> driver to decide which of the two modes made sense in any given
>>> situation. For instance, last I looked at the i2c driver it tried to
>>> use DMA for large transfers and FIFO for small transfers. The SPI
>>> driver also has some cases where it will use DMA mode and then
>>> fallback to FIFO mode.
>>>
>>> ...so what exactly is the point of differentiating between "FIFO" and
>>> "CPU DMA" mode here?
>>
>> Yes, correct, Will update in V2.
>> I plan to add 2 modes, GSI and non-GSI(PIO or DMA based on length).
>>
>>>
>>> Then when it comes to "GSI DMA" mode, my understanding is that the
>>> firmware for "GSI DMA" mode is always loaded by Trustzone because the
>>> whole point is that the GSI mode arbitrates between multiple clients.
>>> Presumably if the firmware already loaded the GSI firmware then the
>>> code would just detect that case. ...so there shouldn't need to be any
>>> reason to specify GSI mode here either, right?
>>>
>>> -Doug
>>
>> GSI firmware is loaded from TZ per QUP, but to use GSI mode,
>> we need to configure the SE to use GSI mode by writing into SE register
>> QUPV3_SE_GENI_DMA_MODE_EN and SE_GSI_EVENT_EN. This register is
>> used to configure data transfer mode for Serial Engine.
> 
> Can't you detect it's in GSI mode without any device tree property
> like the code does today?
> 
> -Doug

No, we can't detect GSI mode in the current design. The GSI firmware is 
loaded from the TZ side, while mode selection occurs on the APPS side 
based on the Device Tree property.

Re: [PATCH v1 1/7] dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP firmware loading

[Doug Anderson]

Hi,

On Tue, Dec 10, 2024 at 9:27 PM Viken Dadhaniya
<quic_vdadhani@quicinc.com> wrote:
>
> On 12/10/2024 11:12 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Dec 9, 2024 at 9:28 PM Viken Dadhaniya
> > <quic_vdadhani@quicinc.com> wrote:
> >>
> >> On 12/4/2024 10:55 PM, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Wed, Dec 4, 2024 at 7:03 AM Viken Dadhaniya
> >>> <quic_vdadhani@quicinc.com> wrote:
> >>>>
> >>>> Document the 'qcom,load-firmware' and 'qcom,xfer-mode' properties to
> >>>> support SE(Serial Engine) firmware loading from the protocol driver and 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>
> >>>> ---
> >>>>    .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml   | 11 +++++++++++
> >>>>    1 file changed, 11 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..a26f34fce1bb 100644
> >>>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> >>>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> >>>> @@ -66,6 +66,15 @@ properties:
> >>>>      required-opps:
> >>>>        maxItems: 1
> >>>>
> >>>> +  qcom,load-firmware:
> >>>> +    type: boolean
> >>>> +    description: Optional property to load SE (serial engine) Firmware from protocol driver.
> >>>> +
> >>>> +  qcom,xfer-mode:
> >>>> +    description: Value 1,2 and 3 represents FIFO, CPU DMA and GSI DMA mode respectively.
> >>>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>>> +    enum: [1, 2, 3]
> >>>
> >>> I'm a little confused about this. I'll admit I haven't fully analyzed
> >>> your patch with actual code in it, but in the past "CPU DMA" mode and
> >>> "FIFO" mode were compatible with each other and then it was up to the
> >>> driver to decide which of the two modes made sense in any given
> >>> situation. For instance, last I looked at the i2c driver it tried to
> >>> use DMA for large transfers and FIFO for small transfers. The SPI
> >>> driver also has some cases where it will use DMA mode and then
> >>> fallback to FIFO mode.
> >>>
> >>> ...so what exactly is the point of differentiating between "FIFO" and
> >>> "CPU DMA" mode here?
> >>
> >> Yes, correct, Will update in V2.
> >> I plan to add 2 modes, GSI and non-GSI(PIO or DMA based on length).
> >>
> >>>
> >>> Then when it comes to "GSI DMA" mode, my understanding is that the
> >>> firmware for "GSI DMA" mode is always loaded by Trustzone because the
> >>> whole point is that the GSI mode arbitrates between multiple clients.
> >>> Presumably if the firmware already loaded the GSI firmware then the
> >>> code would just detect that case. ...so there shouldn't need to be any
> >>> reason to specify GSI mode here either, right?
> >>>
> >>> -Doug
> >>
> >> GSI firmware is loaded from TZ per QUP, but to use GSI mode,
> >> we need to configure the SE to use GSI mode by writing into SE register
> >> QUPV3_SE_GENI_DMA_MODE_EN and SE_GSI_EVENT_EN. This register is
> >> used to configure data transfer mode for Serial Engine.
> >
> > Can't you detect it's in GSI mode without any device tree property
> > like the code does today?
> >
> > -Doug
>
> No, we can't detect GSI mode in the current design. The GSI firmware is
> loaded from the TZ side, while mode selection occurs on the APPS side
> based on the Device Tree property.

Presumably you can check to see if the geni firmware has already been
loaded before the kernel started, right? In the case that it's already
loaded, can't you fall back to the way that existing code detects GSI
mode? From reading `drivers/spi/spi-geni-qcom.c` I see that if the
FIFO is disabled then it assumes we must be in GSI mode...
Specifically, it checks:

readl(se->base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;

The i2c code today (`drivers/i2c/busses/i2c-qcom-geni.c`) does the same.

So, essentially:

* If geni firmware has already been loaded, then check
FIFO_IF_DISABLE. If the FIFO is disabled it's GSI. If not then both
"CPU DMA" and "FIFO" are allowed.

* If geni firmware hasn't already been loaded then it's impossible to
be in GSI mode since GSI only makes sense if the geni firmware was
loaded before the kernel started. Thus we're in "CPU DMA" / "FIFO"
mode.

In both cases you don't need an attribute telling you whether to use
GSI mode or not, right?

-Doug

Re: [PATCH v1 1/7] dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP firmware loading

[Dmitry Baryshkov]

On Wed, Dec 04, 2024 at 08:33:20PM +0530, Viken Dadhaniya wrote:
> Document the 'qcom,load-firmware' and 'qcom,xfer-mode' properties to
> support SE(Serial Engine) firmware loading from the protocol driver and 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>
> ---
>  .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml   | 11 +++++++++++
>  1 file changed, 11 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..a26f34fce1bb 100644
> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> @@ -66,6 +66,15 @@ properties:
>    required-opps:
>      maxItems: 1
>  
> +  qcom,load-firmware:
> +    type: boolean
> +    description: Optional property to load SE (serial engine) Firmware from protocol driver.
> +
> +  qcom,xfer-mode:
> +    description: Value 1,2 and 3 represents FIFO, CPU DMA and GSI DMA mode respectively.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 3]
> +
>  required:
>    - compatible
>    - interrupts
> @@ -142,5 +151,7 @@ examples:
>          interconnect-names = "qup-core", "qup-config", "qup-memory";
>          power-domains = <&rpmhpd SC7180_CX>;
>          required-opps = <&rpmhpd_opp_low_svs>;
> +        qcom,load-firmware;

please use instead:
firmware-name = "qcom/sc7180/qupv3.elf"

> +        qcom,xfer-mode = <1>;
>      };
>  ...
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 1/7] dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP firmware loading

[Viken Dadhaniya]

On 12/5/2024 4:06 AM, Dmitry Baryshkov wrote:
> On Wed, Dec 04, 2024 at 08:33:20PM +0530, Viken Dadhaniya wrote:
>> Document the 'qcom,load-firmware' and 'qcom,xfer-mode' properties to
>> support SE(Serial Engine) firmware loading from the protocol driver and 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>
>> ---
>>   .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml   | 11 +++++++++++
>>   1 file changed, 11 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..a26f34fce1bb 100644
>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> @@ -66,6 +66,15 @@ properties:
>>     required-opps:
>>       maxItems: 1
>>   
>> +  qcom,load-firmware:
>> +    type: boolean
>> +    description: Optional property to load SE (serial engine) Firmware from protocol driver.
>> +
>> +  qcom,xfer-mode:
>> +    description: Value 1,2 and 3 represents FIFO, CPU DMA and GSI DMA mode respectively.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [1, 2, 3]
>> +
>>   required:
>>     - compatible
>>     - interrupts
>> @@ -142,5 +151,7 @@ examples:
>>           interconnect-names = "qup-core", "qup-config", "qup-memory";
>>           power-domains = <&rpmhpd SC7180_CX>;
>>           required-opps = <&rpmhpd_opp_low_svs>;
>> +        qcom,load-firmware;
> 
> please use instead:
> firmware-name = "qcom/sc7180/qupv3.elf"

Agreed, will be adopted in next patch.

> 
>> +        qcom,xfer-mode = <1>;
>>       };
>>   ...
>> -- 
>> 2.34.1
>>
> 

[PATCH v1 2/7] spi: dt-bindings: Document DT properties for QUP firmware loading

[Viken Dadhaniya]

Document the 'qcom,load-firmware' and 'qcom,xfer-mode' properties to
support SE(Serial Engine) firmware loading from the protocol driver and
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>
---
 .../devicetree/bindings/spi/qcom,spi-geni-qcom.yaml   | 11 +++++++++++
 1 file changed, 11 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..7038981f54ff 100644
--- a/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml
+++ b/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml
@@ -66,6 +66,15 @@ properties:
   reg:
     maxItems: 1
 
+  qcom,load-firmware:
+    type: boolean
+    description: Optional property to load SE (serial engine) Firmware from protocol driver.
+
+  qcom,xfer-mode:
+    description: Value 1,2 and 3 represents FIFO, CPU DMA and GSI DMA mode respectively.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1, 2, 3]
+
 required:
   - compatible
   - clocks
@@ -97,6 +106,8 @@ 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,load-firmware;
+        qcom,xfer-mode = <1>;
     };
 
   - |
-- 
2.34.1

Re: [PATCH v1 2/7] spi: dt-bindings: Document DT properties for QUP firmware loading

[Krzysztof Kozlowski]

On 04/12/2024 16:03, Viken Dadhaniya wrote:
> Document the 'qcom,load-firmware' and 'qcom,xfer-mode' properties to
> support SE(Serial Engine) firmware loading from the protocol driver and
> 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>
> ---

My comments from I2C patch apply here, but let's keep discussion there.
Responding here just for formality.

Best regards,
Krzysztof

[PATCH v1 3/7] dt-bindings: serial: Document DT properties for QUP firmware loading

[Viken Dadhaniya]

Document the 'qcom,load-firmware' and 'qcom,xfer-mode' properties to
support SE(Serial Engine) firmware loading from the protocol driver and 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>
---
 .../bindings/serial/qcom,serial-geni-qcom.yaml       | 12 ++++++++++++
 1 file changed, 12 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..21a5b0dafbe0 100644
--- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
+++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml
@@ -56,6 +56,16 @@ properties:
   reg:
     maxItems: 1
 
+  qcom,load-firmware:
+    type: boolean
+    description: Optional property to load SE (serial engine) Firmware from protocol driver.
+
+  qcom,xfer-mode:
+    description: Value 1,2 and 3 represents FIFO, CPU DMA and GSI DMA mode respectively.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1, 2, 3]
+
+
 required:
   - compatible
   - clocks
@@ -82,5 +92,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,load-firmware;
+        qcom,xfer-mode = <1>;
     };
 ...
-- 
2.34.1

Re: [PATCH v1 3/7] dt-bindings: serial: Document DT properties for QUP firmware loading

[Krzysztof Kozlowski]

On 04/12/2024 16:03, Viken Dadhaniya wrote:
> Document the 'qcom,load-firmware' and 'qcom,xfer-mode' properties to
> support SE(Serial Engine) firmware loading from the protocol driver and 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>
> ---

My comments from I2C patch apply here, but let's keep discussion there.
Responding here just for formality.

Best regards,
Krzysztof

[PATCH v1 4/7] 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 "qcom,load-firmware" 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>
---
 drivers/soc/qcom/qcom-geni-se.c      | 445 +++++++++++++++++++++++++++
 include/linux/soc/qcom/geni-se.h     |  17 +
 include/linux/soc/qcom/qup-fw-load.h | 179 +++++++++++
 3 files changed, 641 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..423102fac3fc 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) 2024 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
@@ -97,6 +99,9 @@ struct geni_wrapper {
 	unsigned int num_clks;
 };
 
+/* elf file should be at /lib/firmware/ */
+#define QUP_FW_ELF_FILE	"qupv3fw.elf"
+
 /**
  * struct geni_se_desc - Data structure to represent the QUP Wrapper resources
  * @clks:		Name of the primary & optional secondary AHB clocks
@@ -110,6 +115,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 +899,443 @@ 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)
+{
+	const 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.
+ */
+int qup_fw_load(struct qup_se_rsc *rsc)
+{
+	int ret;
+	const struct firmware *fw;
+	struct device *dev = rsc->se->dev;
+
+	ret = request_firmware(&fw, QUP_FW_ELF_FILE, 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;
+	int ret;
+
+	if (device_property_read_bool(se->dev, "qcom,load-firmware")) {
+		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);
+		if (ret) {
+			dev_err(se->dev,  "Firmware Loading failed for proto: %s Error: %d\n",
+				protocol_name[rsc.protocol], ret);
+			return ret;
+		}
+
+		dev_info(se->dev, "Firmware load for %s protocol is Success for xfer mode %d\n",
+			 protocol_name[rsc.protocol], rsc.mode);
+		return ret;
+	}
+	return -EINVAL;
+}
+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-2024 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) 2024 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 v1 4/7] soc: qcom: geni-se:: Add support to load QUP SE Firmware via Linux subsystem

[neil.armstrong]

Hi,

On 04/12/2024 16:03, Viken Dadhaniya wrote:
> Load the firmware to QUP SE based on the "qcom,load-firmware" 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>
> ---
>   drivers/soc/qcom/qcom-geni-se.c      | 445 +++++++++++++++++++++++++++
>   include/linux/soc/qcom/geni-se.h     |  17 +
>   include/linux/soc/qcom/qup-fw-load.h | 179 +++++++++++
>   3 files changed, 641 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..423102fac3fc 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) 2024 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
> @@ -97,6 +99,9 @@ struct geni_wrapper {
>   	unsigned int num_clks;
>   };
>   
> +/* elf file should be at /lib/firmware/ */
> +#define QUP_FW_ELF_FILE	"qupv3fw.elf"

I supposed the qupv3fw.elf is SoC specific, so it should use /lib/firmware/qcom
base path and also a SoC/platform specific path that should be specified
with firmware-name in DT.

With this property, "qcom,load-firmware" could be dropped.

> +
>   /**
>    * struct geni_se_desc - Data structure to represent the QUP Wrapper resources
>    * @clks:		Name of the primary & optional secondary AHB clocks
> @@ -110,6 +115,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
>   
<snip>

Re: [PATCH v1 4/7] soc: qcom: geni-se:: Add support to load QUP SE Firmware via Linux subsystem

[Viken Dadhaniya]

On 12/4/2024 8:54 PM, neil.armstrong@linaro.org wrote:
> Hi,
> 
> On 04/12/2024 16:03, Viken Dadhaniya wrote:
>> Load the firmware to QUP SE based on the "qcom,load-firmware" 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>
>> ---
>>   drivers/soc/qcom/qcom-geni-se.c      | 445 +++++++++++++++++++++++++++
>>   include/linux/soc/qcom/geni-se.h     |  17 +
>>   include/linux/soc/qcom/qup-fw-load.h | 179 +++++++++++
>>   3 files changed, 641 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..423102fac3fc 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) 2024 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
>> @@ -97,6 +99,9 @@ struct geni_wrapper {
>>       unsigned int num_clks;
>>   };
>> +/* elf file should be at /lib/firmware/ */
>> +#define QUP_FW_ELF_FILE    "qupv3fw.elf"
> 
> I supposed the qupv3fw.elf is SoC specific, so it should use 
> /lib/firmware/qcom
> base path and also a SoC/platform specific path that should be specified
> with firmware-name in DT.
> 
> With this property, "qcom,load-firmware" could be dropped.
> 

Agree, will update in next patch.

>> +
>>   /**
>>    * struct geni_se_desc - Data structure to represent the QUP Wrapper 
>> resources
>>    * @clks:        Name of the primary & optional secondary AHB clocks
>> @@ -110,6 +115,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
> <snip>
> 

Re: [PATCH v1 4/7] soc: qcom: geni-se:: Add support to load QUP SE Firmware via Linux subsystem

[kernel test robot]

Hi Viken,

kernel test robot noticed the following build warnings:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Viken-Dadhaniya/dt-bindings-i2c-qcom-i2c-geni-Document-DT-properties-for-QUP-firmware-loading/20241204-230736
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20241204150326.1470749-5-quic_vdadhani%40quicinc.com
patch subject: [PATCH v1 4/7] soc: qcom: geni-se:: Add support to load QUP SE Firmware via Linux subsystem
config: arm-randconfig-002 (https://download.01.org/0day-ci/archive/20241205/202412050429.SJvNsU2f-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241205/202412050429.SJvNsU2f-lkp@intel.com/reproduce)

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

All warnings (new ones prefixed by >>):

   drivers/soc/qcom/qcom-geni-se.c: In function 'read_elf':
>> drivers/soc/qcom/qcom-geni-se.c:975:23: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     975 |                 *phdr = &phdrs[i];
         |                       ^
   drivers/soc/qcom/qcom-geni-se.c: At top level:
   drivers/soc/qcom/qcom-geni-se.c:1268:5: warning: no previous prototype for 'qup_fw_load' [-Wmissing-prototypes]
    1268 | int qup_fw_load(struct qup_se_rsc *rsc)
         |     ^~~~~~~~~~~


vim +/const +975 drivers/soc/qcom/qcom-geni-se.c

   946	
   947	/**
   948	 * read_elf: Function to read elf file.
   949	 * @rsc: A pointer to SE resources structure.
   950	 * @fw: A pointer to the fw buffer.
   951	 * @pelfseg: A pointer to SE specific elf header.
   952	 * @phdr: pointer to one of the valid headers from list from fw buffer.
   953	 *
   954	 * This function reads the ELF file and outputs the pointer to header
   955	 * data which contains the FW data and any other details.
   956	 *
   957	 * return: Return 0 if no error, else return error value.
   958	 */
   959	static int read_elf(struct qup_se_rsc *rsc, const struct firmware *fw,
   960			    struct elf_se_hdr **pelfseg, struct elf32_phdr **phdr)
   961	{
   962		const struct elf32_phdr *phdrs;
   963		const struct elf32_hdr *ehdr;
   964		const u8 *addr;
   965		int i;
   966	
   967		ehdr = (struct elf32_hdr *)fw->data;
   968	
   969		if (ehdr->e_phnum < 2)
   970			return -EINVAL;
   971	
   972		phdrs = (struct elf32_phdr *)(ehdr + 1);
   973	
   974		for (i = 0; i < ehdr->e_phnum; i++) {
 > 975			*phdr = &phdrs[i];
   976			if (!elf_phdr_valid(*phdr))
   977				continue;
   978	
   979			if ((*phdr)->p_filesz >= sizeof(struct elf_se_hdr)) {
   980				addr =  fw->data + (*phdr)->p_offset;
   981				*pelfseg = (struct elf_se_hdr *)addr;
   982	
   983				if ((*pelfseg)->magic == MAGIC_NUM_SE &&
   984				    (*pelfseg)->version == 1 &&
   985				    valid_seg_size(*pelfseg, (*phdr)->p_filesz))
   986					if ((*pelfseg)->serial_protocol == rsc->protocol &&
   987					    (*pelfseg)->serial_protocol != GENI_SE_NONE)
   988						return 0;
   989			}
   990		}
   991		return -EINVAL;
   992	}
   993	

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

Re: [PATCH v1 4/7] soc: qcom: geni-se:: Add support to load QUP SE Firmware via Linux subsystem

[Dmitry Baryshkov]

On Thu, Dec 05, 2024 at 04:19:25AM +0800, kernel test robot wrote:
> Hi Viken,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on andi-shyti/i2c/i2c-host]
> [also build test WARNING on tty/tty-testing tty/tty-next tty/tty-linus broonie-spi/for-next linus/master v6.13-rc1 next-20241204]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Viken-Dadhaniya/dt-bindings-i2c-qcom-i2c-geni-Document-DT-properties-for-QUP-firmware-loading/20241204-230736
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
> patch link:    https://lore.kernel.org/r/20241204150326.1470749-5-quic_vdadhani%40quicinc.com
> patch subject: [PATCH v1 4/7] soc: qcom: geni-se:: Add support to load QUP SE Firmware via Linux subsystem
> config: arm-randconfig-002 (https://download.01.org/0day-ci/archive/20241205/202412050429.SJvNsU2f-lkp@intel.com/config)
> compiler: arm-linux-gnueabi-gcc (GCC) 14.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241205/202412050429.SJvNsU2f-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202412050429.SJvNsU2f-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
>    drivers/soc/qcom/qcom-geni-se.c: In function 'read_elf':
> >> drivers/soc/qcom/qcom-geni-se.c:975:23: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>      975 |                 *phdr = &phdrs[i];
>          |                       ^
>    drivers/soc/qcom/qcom-geni-se.c: At top level:
>    drivers/soc/qcom/qcom-geni-se.c:1268:5: warning: no previous prototype for 'qup_fw_load' [-Wmissing-prototypes]
>     1268 | int qup_fw_load(struct qup_se_rsc *rsc)
>          |     ^~~~~~~~~~~

This doesn't looks like it was properly compile-tested. Please always
make sure that the build cleanly passes "make W=1" for the changed
paths.


-- 
With best wishes
Dmitry

Re: [PATCH v1 4/7] soc: qcom: geni-se:: Add support to load QUP SE Firmware via Linux subsystem

[Viken Dadhaniya]

On 12/5/2024 4:07 AM, Dmitry Baryshkov wrote:
> On Thu, Dec 05, 2024 at 04:19:25AM +0800, kernel test robot wrote:
>> Hi Viken,
>>
>> kernel test robot noticed the following build warnings:
>>
>> [auto build test WARNING on andi-shyti/i2c/i2c-host]
>> [also build test WARNING on tty/tty-testing tty/tty-next tty/tty-linus broonie-spi/for-next linus/master v6.13-rc1 next-20241204]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>
>> url:    https://github.com/intel-lab-lkp/linux/commits/Viken-Dadhaniya/dt-bindings-i2c-qcom-i2c-geni-Document-DT-properties-for-QUP-firmware-loading/20241204-230736
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
>> patch link:    https://lore.kernel.org/r/20241204150326.1470749-5-quic_vdadhani%40quicinc.com
>> patch subject: [PATCH v1 4/7] soc: qcom: geni-se:: Add support to load QUP SE Firmware via Linux subsystem
>> config: arm-randconfig-002 (https://download.01.org/0day-ci/archive/20241205/202412050429.SJvNsU2f-lkp@intel.com/config)
>> compiler: arm-linux-gnueabi-gcc (GCC) 14.2.0
>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241205/202412050429.SJvNsU2f-lkp@intel.com/reproduce)
>>
>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>> the same patch/commit), kindly add following tags
>> | Reported-by: kernel test robot <lkp@intel.com>
>> | Closes: https://lore.kernel.org/oe-kbuild-all/202412050429.SJvNsU2f-lkp@intel.com/
>>
>> All warnings (new ones prefixed by >>):
>>
>>     drivers/soc/qcom/qcom-geni-se.c: In function 'read_elf':
>>>> drivers/soc/qcom/qcom-geni-se.c:975:23: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>>       975 |                 *phdr = &phdrs[i];
>>           |                       ^
>>     drivers/soc/qcom/qcom-geni-se.c: At top level:
>>     drivers/soc/qcom/qcom-geni-se.c:1268:5: warning: no previous prototype for 'qup_fw_load' [-Wmissing-prototypes]
>>      1268 | int qup_fw_load(struct qup_se_rsc *rsc)
>>           |     ^~~~~~~~~~~
> 
> This doesn't looks like it was properly compile-tested. Please always
> make sure that the build cleanly passes "make W=1" for the changed
> paths.
> 
> 

we have compiled with -Werror but there was something missing.
Tried with with W=1 and now i see this error coming locally. Will 
incorporate and take care in future.

Re: [PATCH v1 4/7] soc: qcom: geni-se:: Add support to load QUP SE Firmware via Linux subsystem

[kernel test robot]

Hi Viken,

kernel test robot noticed the following build errors:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Viken-Dadhaniya/dt-bindings-i2c-qcom-i2c-geni-Document-DT-properties-for-QUP-firmware-loading/20241204-230736
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20241204150326.1470749-5-quic_vdadhani%40quicinc.com
patch subject: [PATCH v1 4/7] soc: qcom: geni-se:: Add support to load QUP SE Firmware via Linux subsystem
config: x86_64-buildonly-randconfig-003-20241205 (https://download.01.org/0day-ci/archive/20241205/202412050408.rTlLBe7e-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241205/202412050408.rTlLBe7e-lkp@intel.com/reproduce)

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

All error/warnings (new ones prefixed by >>):

   In file included from drivers/soc/qcom/qcom-geni-se.c:11:
   In file included from include/linux/dma-mapping.h:8:
   In file included from include/linux/scatterlist.h:8:
   In file included from include/linux/mm.h:2223:
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> drivers/soc/qcom/qcom-geni-se.c:975:9: error: assigning to 'struct elf32_phdr *' from 'const struct elf32_phdr *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
     975 |                 *phdr = &phdrs[i];
         |                       ^ ~~~~~~~~~
>> drivers/soc/qcom/qcom-geni-se.c:1182:2: warning: variable 'reg_value' is uninitialized when used here [-Wuninitialized]
    1182 |         reg_value |= DMA_GENERAL_CFG_AHB_SEC_SLV_CLK_CGC_ON_BMSK |
         |         ^~~~~~~~~
   drivers/soc/qcom/qcom-geni-se.c:1136:18: note: initialize the variable 'reg_value' to silence this warning
    1136 |         u32 i, reg_value, mask, ramn_cnt;
         |                         ^
         |                          = 0
   drivers/soc/qcom/qcom-geni-se.c:1268:5: warning: no previous prototype for function 'qup_fw_load' [-Wmissing-prototypes]
    1268 | int qup_fw_load(struct qup_se_rsc *rsc)
         |     ^
   drivers/soc/qcom/qcom-geni-se.c:1268:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
    1268 | int qup_fw_load(struct qup_se_rsc *rsc)
         | ^
         | static 
   3 warnings and 1 error generated.


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

   946	
   947	/**
   948	 * read_elf: Function to read elf file.
   949	 * @rsc: A pointer to SE resources structure.
   950	 * @fw: A pointer to the fw buffer.
   951	 * @pelfseg: A pointer to SE specific elf header.
   952	 * @phdr: pointer to one of the valid headers from list from fw buffer.
   953	 *
   954	 * This function reads the ELF file and outputs the pointer to header
   955	 * data which contains the FW data and any other details.
   956	 *
   957	 * return: Return 0 if no error, else return error value.
   958	 */
   959	static int read_elf(struct qup_se_rsc *rsc, const struct firmware *fw,
   960			    struct elf_se_hdr **pelfseg, struct elf32_phdr **phdr)
   961	{
   962		const struct elf32_phdr *phdrs;
   963		const struct elf32_hdr *ehdr;
   964		const u8 *addr;
   965		int i;
   966	
   967		ehdr = (struct elf32_hdr *)fw->data;
   968	
   969		if (ehdr->e_phnum < 2)
   970			return -EINVAL;
   971	
   972		phdrs = (struct elf32_phdr *)(ehdr + 1);
   973	
   974		for (i = 0; i < ehdr->e_phnum; i++) {
 > 975			*phdr = &phdrs[i];
   976			if (!elf_phdr_valid(*phdr))
   977				continue;
   978	
   979			if ((*phdr)->p_filesz >= sizeof(struct elf_se_hdr)) {
   980				addr =  fw->data + (*phdr)->p_offset;
   981				*pelfseg = (struct elf_se_hdr *)addr;
   982	
   983				if ((*pelfseg)->magic == MAGIC_NUM_SE &&
   984				    (*pelfseg)->version == 1 &&
   985				    valid_seg_size(*pelfseg, (*phdr)->p_filesz))
   986					if ((*pelfseg)->serial_protocol == rsc->protocol &&
   987					    (*pelfseg)->serial_protocol != GENI_SE_NONE)
   988						return 0;
   989			}
   990		}
   991		return -EINVAL;
   992	}
   993	

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

Re: [PATCH v1 4/7] soc: qcom: geni-se:: Add support to load QUP SE Firmware via Linux subsystem

[kernel test robot]

Hi Viken,

kernel test robot noticed the following build warnings:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Viken-Dadhaniya/dt-bindings-i2c-qcom-i2c-geni-Document-DT-properties-for-QUP-firmware-loading/20241204-230736
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20241204150326.1470749-5-quic_vdadhani%40quicinc.com
patch subject: [PATCH v1 4/7] soc: qcom: geni-se:: Add support to load QUP SE Firmware via Linux subsystem
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20241205/202412050806.0jxlVq68-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241205/202412050806.0jxlVq68-lkp@intel.com/reproduce)

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

All warnings (new ones prefixed by >>):

   In file included from drivers/soc/qcom/qcom-geni-se.c:19:
>> include/linux/soc/qcom/qup-fw-load.h:148:9: warning: "out_be32" redefined
     148 | #define out_be32(a, v) writel_relaxed(v, a)
         |         ^~~~~~~~
   In file included from arch/m68k/include/asm/io_mm.h:25,
                    from arch/m68k/include/asm/io.h:8,
                    from include/linux/scatterlist.h:9,
                    from include/linux/dma-mapping.h:8,
                    from drivers/soc/qcom/qcom-geni-se.c:11:
   arch/m68k/include/asm/raw_io.h:32:9: note: this is the location of the previous definition
      32 | #define out_be32(addr,l) (void)((*(__force volatile u32 *) (unsigned long)(addr)) = (l))
         |         ^~~~~~~~
>> include/linux/soc/qcom/qup-fw-load.h:149:9: warning: "in_be32" redefined
     149 | #define in_be32(a) readl_relaxed(a)
         |         ^~~~~~~
   arch/m68k/include/asm/raw_io.h:23:9: note: this is the location of the previous definition
      23 | #define in_be32(addr) \
         |         ^~~~~~~
   drivers/soc/qcom/qcom-geni-se.c: In function 'read_elf':
   drivers/soc/qcom/qcom-geni-se.c:975:23: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     975 |                 *phdr = &phdrs[i];
         |                       ^
   drivers/soc/qcom/qcom-geni-se.c: At top level:
   drivers/soc/qcom/qcom-geni-se.c:1268:5: warning: no previous prototype for 'qup_fw_load' [-Wmissing-prototypes]
    1268 | int qup_fw_load(struct qup_se_rsc *rsc)
         |     ^~~~~~~~~~~


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

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

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

[PATCH v1 5/7] 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 | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 7a22e1f46e60..98878cf7c044 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -876,10 +876,13 @@ static int geni_i2c_probe(struct platform_device *pdev)
 	}
 	proto = geni_se_read_proto(&gi2c->se);
 	if (proto != GENI_SE_I2C) {
-		dev_err(dev, "Invalid proto %d\n", proto);
-		geni_se_resources_off(&gi2c->se);
-		clk_disable_unprepare(gi2c->core_clk);
-		return -ENXIO;
+		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);
+			geni_se_resources_off(&gi2c->se);
+			clk_disable_unprepare(gi2c->core_clk);
+			return ret;
+		}
 	}
 
 	if (desc && desc->no_dma_support)
-- 
2.34.1

[PATCH v1 6/7] 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 v1 7/7] 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

Re: [PATCH v1 0/7] Add support to load QUP SE firmware from

[Konrad Dybcio]

On 4.12.2024 4:03 PM, Viken Dadhaniya wrote:
> In Qualcomm SoCs, firmware loading for Serial Engines (SE) in the QUP
> hardware has traditionally been managed by TrustZone (TZ). This setup
> handled Serial Engines(SE) assignments and access control permissions,
> ensuring a high level of security but limiting flexibility and
> accessibility.
>  
> 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.

I think this is a great opportunity to rethink the SE node in general.

Currently, for each supported protocol, we create a new node that
differs in (possibly) interconnects and pinctrl states. These are really
defined per-SE however and we can programmatically determine which ones
are relevant.

With the growing number of protocols supported, we would have to add
20+ nodes in some cases for each one of them. I think a good one would
look like:

geni_se10: serial-engine@abcdef {
	compatible = "qcom,geni-se";

	reg
	clocks
	power-domains
	interconnects
	...

	status

	geni_se10_i2c: i2c {
		// i2c-controller.yaml
	};

	geni_se10_spi: spi {
		// spi-controller.yaml
	};

	...
}

Or maybe even get rid of the subnodes and restrict that to a single
se-protocol = <SE_PROTOCOL_xyz> property, if the bindings folks agree.

We could extend the DMA APIs to dynamically determine the protocol
ID and get rid of hardcoding it.

And then we could spawn an instance of the spi, i2c, etc. driver from
the GENI SE driver.

Konrad

Re: [PATCH v1 0/7] Add support to load QUP SE firmware from

[neil.armstrong]

On 05/12/2024 16:59, Konrad Dybcio wrote:
> On 4.12.2024 4:03 PM, Viken Dadhaniya wrote:
>> In Qualcomm SoCs, firmware loading for Serial Engines (SE) in the QUP
>> hardware has traditionally been managed by TrustZone (TZ). This setup
>> handled Serial Engines(SE) assignments and access control permissions,
>> ensuring a high level of security but limiting flexibility and
>> accessibility.
>>   
>> 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.
> 
> I think this is a great opportunity to rethink the SE node in general.
> 
> Currently, for each supported protocol, we create a new node that
> differs in (possibly) interconnects and pinctrl states. These are really
> defined per-SE however and we can programmatically determine which ones
> are relevant.
> 
> With the growing number of protocols supported, we would have to add
> 20+ nodes in some cases for each one of them. I think a good one would
> look like:
> 
> geni_se10: serial-engine@abcdef {
> 	compatible = "qcom,geni-se";
> 
> 	reg
> 	clocks
> 	power-domains
> 	interconnects
> 	...
> 
> 	status
> 
> 	geni_se10_i2c: i2c {
> 		// i2c-controller.yaml
> 	};
> 
> 	geni_se10_spi: spi {
> 		// spi-controller.yaml
> 	};
> 
> 	...
> }
> 
> Or maybe even get rid of the subnodes and restrict that to a single
> se-protocol = <SE_PROTOCOL_xyz> property, if the bindings folks agree.
> 
> We could extend the DMA APIs to dynamically determine the protocol
> ID and get rid of hardcoding it.
> 
> And then we could spawn an instance of the spi, i2c, etc. driver from
> the GENI SE driver.

How/where would you add the peripheral subnodes ? A Serial Engine can only be a
single type on a board, but I agree we could have a "generic" serial engine node
that would be differenciated in the board DT with the protocol, and use the bindings
yaml checked to properly check the subnodes/properties depending on the protocol
property.

But we would still need all the serial nodes in the SoC DT.

This may make the software support harder, meaning we would either need to
have the same compatible probed in sequence from the i2c/spi/uart driver until
one matches the protocol, or have the qup driver spawn an auxiliary device.

Honestly, not sure it would be much simpler...

Neil

> 
> Konrad
> 

Re: [PATCH v1 0/7] Add support to load QUP SE firmware from

[Viken Dadhaniya]

On 12/9/2024 8:15 PM, neil.armstrong@linaro.org wrote:
> On 05/12/2024 16:59, Konrad Dybcio wrote:
>> On 4.12.2024 4:03 PM, Viken Dadhaniya wrote:
>>> In Qualcomm SoCs, firmware loading for Serial Engines (SE) in the QUP
>>> hardware has traditionally been managed by TrustZone (TZ). This setup
>>> handled Serial Engines(SE) assignments and access control permissions,
>>> ensuring a high level of security but limiting flexibility and
>>> accessibility.
>>> 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.
>>
>> I think this is a great opportunity to rethink the SE node in general.
>>
>> Currently, for each supported protocol, we create a new node that
>> differs in (possibly) interconnects and pinctrl states. These are really
>> defined per-SE however and we can programmatically determine which ones
>> are relevant.
>>
>> With the growing number of protocols supported, we would have to add
>> 20+ nodes in some cases for each one of them. I think a good one would
>> look like:
>>
>> geni_se10: serial-engine@abcdef {
>>     compatible = "qcom,geni-se";
>>
>>     reg
>>     clocks
>>     power-domains
>>     interconnects
>>     ...
>>
>>     status
>>
>>     geni_se10_i2c: i2c {
>>         // i2c-controller.yaml
>>     };
>>
>>     geni_se10_spi: spi {
>>         // spi-controller.yaml
>>     };
>>
>>     ...
>> }
>>
>> Or maybe even get rid of the subnodes and restrict that to a single
>> se-protocol = <SE_PROTOCOL_xyz> property, if the bindings folks agree.
>>
>> We could extend the DMA APIs to dynamically determine the protocol
>> ID and get rid of hardcoding it.
>>
>> And then we could spawn an instance of the spi, i2c, etc. driver from
>> the GENI SE driver.
> 
> How/where would you add the peripheral subnodes ? A Serial Engine can 
> only be a
> single type on a board, but I agree we could have a "generic" serial 
> engine node
> that would be differenciated in the board DT with the protocol, and use 
> the bindings
> yaml checked to properly check the subnodes/properties depending on the 
> protocol
> property.
> 
> But we would still need all the serial nodes in the SoC DT.
> 
> This may make the software support harder, meaning we would either need to
> have the same compatible probed in sequence from the i2c/spi/uart driver 
> until
> one matches the protocol, or have the qup driver spawn an auxiliary device.
> 
> Honestly, not sure it would be much simpler...
> 

Agree Neil, it has it's own challenges in terms actual code changes per 
driver and common driver redesign when we move the SE nodes and make it 
common. We may come up with some solution to make one SE DTSI node for 
all protocols having different (pinctrl configuration, DMA 
configuration) but it's also going to add some level of code 
complexities and yaml changes.
Can we exclude this design change for this firmware loading and later 
align to this new design change ?

> Neil
> 
>>
>> Konrad
>>
> 

Re: [PATCH v1 0/7] Add support to load QUP SE firmware from

[Konrad Dybcio]

On 9.12.2024 3:45 PM, neil.armstrong@linaro.org wrote:
> On 05/12/2024 16:59, Konrad Dybcio wrote:
>> On 4.12.2024 4:03 PM, Viken Dadhaniya wrote:
>>> In Qualcomm SoCs, firmware loading for Serial Engines (SE) in the QUP
>>> hardware has traditionally been managed by TrustZone (TZ). This setup
>>> handled Serial Engines(SE) assignments and access control permissions,
>>> ensuring a high level of security but limiting flexibility and
>>> accessibility.
>>>   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.
>>
>> I think this is a great opportunity to rethink the SE node in general.
>>
>> Currently, for each supported protocol, we create a new node that
>> differs in (possibly) interconnects and pinctrl states. These are really
>> defined per-SE however and we can programmatically determine which ones
>> are relevant.
>>
>> With the growing number of protocols supported, we would have to add
>> 20+ nodes in some cases for each one of them. I think a good one would
>> look like:
>>
>> geni_se10: serial-engine@abcdef {
>>     compatible = "qcom,geni-se";
>>
>>     reg
>>     clocks
>>     power-domains
>>     interconnects
>>     ...
>>
>>     status
>>
>>     geni_se10_i2c: i2c {
>>         // i2c-controller.yaml
>>     };
>>
>>     geni_se10_spi: spi {
>>         // spi-controller.yaml
>>     };
>>
>>     ...
>> }
>>
>> Or maybe even get rid of the subnodes and restrict that to a single
>> se-protocol = <SE_PROTOCOL_xyz> property, if the bindings folks agree.
>>
>> We could extend the DMA APIs to dynamically determine the protocol
>> ID and get rid of hardcoding it.
>>
>> And then we could spawn an instance of the spi, i2c, etc. driver from
>> the GENI SE driver.
> 
> How/where would you add the peripheral subnodes ? A Serial Engine can only be a
> single type on a board, but I agree we could have a "generic" serial engine node
> that would be differenciated in the board DT with the protocol, and use the bindings
> yaml checked to properly check the subnodes/properties depending on the protocol
> property.
> 
> But we would still need all the serial nodes in the SoC DT.

Correct, but NUM_PROTOCOLS times less. NUM_PROTOCOLS is 3 upstream as
of right now, but it's much higher in general (which will trickle
upstream one day or another).

> 
> This may make the software support harder, meaning we would either need to
> have the same compatible probed in sequence from the i2c/spi/uart driver until
> one matches the protocol, or have the qup driver spawn an auxiliary device.

No, just read back the protocol id from hardware (if the SE is running), or
from some DT property (if we need to load the FW ourselves).

Then, based on that, we can call

platform_device_register_data(dev, "geni_i2c", ...) 

(or similar)

> Honestly, not sure it would be much simpler...

Not sure if I'm happy to maintain NUM_QUPs * NUM_SEs * NUM_PROTOCOLS DT nodes,
per each platform separately..

Konrad

Re: [PATCH v1 0/7] Add support to load QUP SE firmware from

[Viken Dadhaniya]

On 12/5/2024 9:29 PM, Konrad Dybcio wrote:
> On 4.12.2024 4:03 PM, Viken Dadhaniya wrote:
>> In Qualcomm SoCs, firmware loading for Serial Engines (SE) in the QUP
>> hardware has traditionally been managed by TrustZone (TZ). This setup
>> handled Serial Engines(SE) assignments and access control permissions,
>> ensuring a high level of security but limiting flexibility and
>> accessibility.
>>   
>> 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.
> 
> I think this is a great opportunity to rethink the SE node in general.
> 
> Currently, for each supported protocol, we create a new node that
> differs in (possibly) interconnects and pinctrl states. These are really
> defined per-SE however and we can programmatically determine which ones
> are relevant.
> 
> With the growing number of protocols supported, we would have to add
> 20+ nodes in some cases for each one of them. I think a good one would
> look like:
> 
> geni_se10: serial-engine@abcdef {
> 	compatible = "qcom,geni-se";
> 
> 	reg
> 	clocks
> 	power-domains
> 	interconnects
> 	...
> 
> 	status
> 
> 	geni_se10_i2c: i2c {
> 		// i2c-controller.yaml
> 	};
> 
> 	geni_se10_spi: spi {
> 		// spi-controller.yaml
> 	};
> 
> 	...
> }
> 
> Or maybe even get rid of the subnodes and restrict that to a single
> se-protocol = <SE_PROTOCOL_xyz> property, if the bindings folks agree.
> 
> We could extend the DMA APIs to dynamically determine the protocol
> ID and get rid of hardcoding it.
> 
> And then we could spawn an instance of the spi, i2c, etc. driver from
> the GENI SE driver.
> 
> Konrad

Thanks for the advice.
The above design suggested by you may add more code change into protocol 
driver as well as common driver.
I am really interested to discuss more options and come to better 
design. let me discuss with you on this.
Also do you think we can push the re-design of DTSI nodes as separate 
change instead of clubbing with this FW load change ?

Re: [PATCH v1 0/7] Add support to load QUP SE firmware from

[Konrad Dybcio]

On 10.12.2024 6:06 AM, Viken Dadhaniya wrote:
> 
> 
> On 12/5/2024 9:29 PM, Konrad Dybcio wrote:
>> On 4.12.2024 4:03 PM, Viken Dadhaniya wrote:
>>> In Qualcomm SoCs, firmware loading for Serial Engines (SE) in the QUP
>>> hardware has traditionally been managed by TrustZone (TZ). This setup
>>> handled Serial Engines(SE) assignments and access control permissions,
>>> ensuring a high level of security but limiting flexibility and
>>> accessibility.
>>>   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.
>>
>> I think this is a great opportunity to rethink the SE node in general.
>>
>> Currently, for each supported protocol, we create a new node that
>> differs in (possibly) interconnects and pinctrl states. These are really
>> defined per-SE however and we can programmatically determine which ones
>> are relevant.
>>
>> With the growing number of protocols supported, we would have to add
>> 20+ nodes in some cases for each one of them. I think a good one would
>> look like:
>>
>> geni_se10: serial-engine@abcdef {
>>     compatible = "qcom,geni-se";
>>
>>     reg
>>     clocks
>>     power-domains
>>     interconnects
>>     ...
>>
>>     status
>>
>>     geni_se10_i2c: i2c {
>>         // i2c-controller.yaml
>>     };
>>
>>     geni_se10_spi: spi {
>>         // spi-controller.yaml
>>     };
>>
>>     ...
>> }
>>
>> Or maybe even get rid of the subnodes and restrict that to a single
>> se-protocol = <SE_PROTOCOL_xyz> property, if the bindings folks agree.
>>
>> We could extend the DMA APIs to dynamically determine the protocol
>> ID and get rid of hardcoding it.
>>
>> And then we could spawn an instance of the spi, i2c, etc. driver from
>> the GENI SE driver.
>>
>> Konrad
> 
> Thanks for the advice.
> The above design suggested by you may add more code change into protocol driver as well as common driver.
> I am really interested to discuss more options and come to better design. let me discuss with you on this.
> Also do you think we can push the re-design of DTSI nodes as separate change instead of clubbing with this FW load change ?

Sure, the firmware loading support itself is a separate change.
I simply used this thread as an opportunity to talk about a future
cleanup.

Konrad

Re: [PATCH v1 0/7] Add support to load QUP SE firmware from

[Caleb Connolly]

Hi Viken,

On 04/12/2024 16:03, Viken Dadhaniya wrote:
> In Qualcomm SoCs, firmware loading for Serial Engines (SE) in the QUP
> hardware has traditionally been managed by TrustZone (TZ). This setup
> handled Serial Engines(SE) assignments and access control permissions,
> ensuring a high level of security but limiting flexibility and
> accessibility.
>  
> 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.

I gave this series a spin on the RB3 Gen 2 with U-Boot.

After fixing the compilation errors, it seems like there is a consistent
hard crash (the board freezes and resets) at some point during i2c
controller init with this series.

I noticed a similar issue with this same logic implemented in U-Boot.

Could you clarify which xfer mode is appropriate for the i2c controllers
on the RB3 Gen 2 and maybe give this a try yourself, or let me know what
other info you'd need to debug this.

Thanks and kind regards,
> 
> Viken Dadhaniya (7):
>   dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP
>     firmware loading
>   spi: dt-bindings: Document DT properties for QUP firmware loading
>   dt-bindings: serial: Document DT properties for QUP firmware loading
>   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      |  11 +
>  .../serial/qcom,serial-geni-qcom.yaml         |  12 +
>  .../bindings/spi/qcom,spi-geni-qcom.yaml      |  11 +
>  drivers/i2c/busses/i2c-qcom-geni.c            |  11 +-
>  drivers/soc/qcom/qcom-geni-se.c               | 445 ++++++++++++++++++
>  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 +++++++
>  9 files changed, 692 insertions(+), 8 deletions(-)
>  create mode 100644 include/linux/soc/qcom/qup-fw-load.h
> 

-- 
// Caleb (they/them)

Re: [PATCH v1 0/7] Add support to load QUP SE firmware from

[Mukesh Kumar Savaliya]

Thanks Caleb for your testing and sharing results. Since Viken is on 
leave, i am following on this.

On 1/7/2025 4:55 PM, Caleb Connolly wrote:
> Hi Viken,
> 
> On 04/12/2024 16:03, Viken Dadhaniya wrote:
>> In Qualcomm SoCs, firmware loading for Serial Engines (SE) in the QUP
>> hardware has traditionally been managed by TrustZone (TZ). This setup
>> handled Serial Engines(SE) assignments and access control permissions,
>> ensuring a high level of security but limiting flexibility and
>> accessibility.
>>   
>> 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.
> 
> I gave this series a spin on the RB3 Gen 2 with U-Boot.
> 
Is it possible to try on RB8 board ? Because that's where this support 
is enabled. It also needs respective TZ configuration to allow FW 
loading from Linux.


> After fixing the compilation errors, it seems like there is a consistent
> hard crash (the board freezes and resets) at some point during i2c
> controller init with this series.
> 
Can you please share exact repro steps ? We can try locally and check 
what's wrong and also review in future how we make it working for U-boot 
combination.
> I noticed a similar issue with this same logic implemented in U-Boot.
> 
> Could you clarify which xfer mode is appropriate for the i2c controllers
> on the RB3 Gen 2 and maybe give this a try yourself, or let me know what
> other info you'd need to debug this.
> 
Yes, please share the procedure , we will try internally.
is there any DTSI change done as part of your testing ?
> Thanks and kind regards,
>>
>> Viken Dadhaniya (7):
>>    dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP
>>      firmware loading
>>    spi: dt-bindings: Document DT properties for QUP firmware loading
>>    dt-bindings: serial: Document DT properties for QUP firmware loading
>>    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      |  11 +
>>   .../serial/qcom,serial-geni-qcom.yaml         |  12 +
>>   .../bindings/spi/qcom,spi-geni-qcom.yaml      |  11 +
>>   drivers/i2c/busses/i2c-qcom-geni.c            |  11 +-
>>   drivers/soc/qcom/qcom-geni-se.c               | 445 ++++++++++++++++++
>>   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 +++++++
>>   9 files changed, 692 insertions(+), 8 deletions(-)
>>   create mode 100644 include/linux/soc/qcom/qup-fw-load.h
>>
> 

Re: [PATCH v1 0/7] Add support to load QUP SE firmware from

[Caleb Connolly]

Hi Mukesh,

On 10/01/2025 07:56, Mukesh Kumar Savaliya wrote:
> Thanks Caleb for your testing and sharing results. Since Viken is on
> leave, i am following on this.
> 
> On 1/7/2025 4:55 PM, Caleb Connolly wrote:
>> Hi Viken,
>>
>> On 04/12/2024 16:03, Viken Dadhaniya wrote:
>>> In Qualcomm SoCs, firmware loading for Serial Engines (SE) in the QUP
>>> hardware has traditionally been managed by TrustZone (TZ). This setup
>>> handled Serial Engines(SE) assignments and access control permissions,
>>> ensuring a high level of security but limiting flexibility and
>>> accessibility.
>>>   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.
>>
>> I gave this series a spin on the RB3 Gen 2 with U-Boot.
>>
> Is it possible to try on RB8 board ? Because that's where this support
> is enabled. It also needs respective TZ configuration to allow FW
> loading from Linux.

I don't have access to RB8, so no... This support will also be useful on
RB3 Gen 2 for U-Boot support, can the next tz release for this platform
include the necessary changes?
> 
> 
>> After fixing the compilation errors, it seems like there is a consistent
>> hard crash (the board freezes and resets) at some point during i2c
>> controller init with this series.
>>
> Can you please share exact repro steps ? We can try locally and check
> what's wrong and also review in future how we make it working for U-boot
> combination.

If it's true that tz changes are needed that would certainly explain the
crash.

Unfortunately it isn't currently possible to boot QC Linux via U-Boot
since the ESP uses a 512 byte sector size on 4k block size UFS which is
not supported.

If you build an image with a correct ESP (mkfs.vfat -S 4096) then you
can boot U-Boot from upstream by following the RB3 Gen 2 documentation

https://docs.u-boot.org/en/latest/board/qualcomm/rb3gen2.html

You'll also need to apply this patch to fix a boot regression
https://lore.kernel.org/u-boot/20250122-qcom-parse-memory-updates-v2-0-98dfcac821d7@samcday.com/

On the Linux/DTS side, apply this series, add the appropriate properties
to enable fw loading as-per the dt-bindings added by this series and
place the qupv3fw.elf file.

Kind regards,
>> I noticed a similar issue with this same logic implemented in U-Boot.
>>
>> Could you clarify which xfer mode is appropriate for the i2c controllers
>> on the RB3 Gen 2 and maybe give this a try yourself, or let me know what
>> other info you'd need to debug this.
>>
> Yes, please share the procedure , we will try internally.
> is there any DTSI change done as part of your testing ?
>> Thanks and kind regards,
>>>
>>> Viken Dadhaniya (7):
>>>    dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP
>>>      firmware loading
>>>    spi: dt-bindings: Document DT properties for QUP firmware loading
>>>    dt-bindings: serial: Document DT properties for QUP firmware loading
>>>    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      |  11 +
>>>   .../serial/qcom,serial-geni-qcom.yaml         |  12 +
>>>   .../bindings/spi/qcom,spi-geni-qcom.yaml      |  11 +
>>>   drivers/i2c/busses/i2c-qcom-geni.c            |  11 +-
>>>   drivers/soc/qcom/qcom-geni-se.c               | 445 ++++++++++++++++++
>>>   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 +++++++
>>>   9 files changed, 692 insertions(+), 8 deletions(-)
>>>   create mode 100644 include/linux/soc/qcom/qup-fw-load.h
>>>
>>
> 

-- 
// Caleb (they/them)

Re: [PATCH v1 0/7] Add support to load QUP SE firmware from

[Mukesh Kumar Savaliya]

Hi Caleb, we shall update for latest ask and will check how to enable 
validation on required board.

On 1/22/2025 8:53 PM, Caleb Connolly wrote:
> Hi Mukesh,
> 
> On 10/01/2025 07:56, Mukesh Kumar Savaliya wrote:
>> Thanks Caleb for your testing and sharing results. Since Viken is on
>> leave, i am following on this.
>>
>> On 1/7/2025 4:55 PM, Caleb Connolly wrote:
>>> Hi Viken,
>>>
>>> On 04/12/2024 16:03, Viken Dadhaniya wrote:
>>>> In Qualcomm SoCs, firmware loading for Serial Engines (SE) in the QUP
>>>> hardware has traditionally been managed by TrustZone (TZ). This setup
>>>> handled Serial Engines(SE) assignments and access control permissions,
>>>> ensuring a high level of security but limiting flexibility and
>>>> accessibility.
>>>>    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.
>>>
>>> I gave this series a spin on the RB3 Gen 2 with U-Boot.
>>>
>> Is it possible to try on RB8 board ? Because that's where this support
>> is enabled. It also needs respective TZ configuration to allow FW
>> loading from Linux.
> 
> I don't have access to RB8, so no... This support will also be useful on
> RB3 Gen 2 for U-Boot support, can the next tz release for this platform
> include the necessary changes?
Let us evaluate and review TZ configs and changes. This was meant for 
only development boards. I will check internally and review for RB3 Gen2 
and update.
>>
>>
>>> After fixing the compilation errors, it seems like there is a consistent
>>> hard crash (the board freezes and resets) at some point during i2c
>>> controller init with this series.
>>>
>> Can you please share exact repro steps ? We can try locally and check
>> what's wrong and also review in future how we make it working for U-boot
>> combination.
> 
> If it's true that tz changes are needed that would certainly explain the
> crash.
> 
yes
> Unfortunately it isn't currently possible to boot QC Linux via U-Boot
> since the ESP uses a 512 byte sector size on 4k block size UFS which is
> not supported.
> 
> If you build an image with a correct ESP (mkfs.vfat -S 4096) then you
> can boot U-Boot from upstream by following the RB3 Gen 2 documentation
> 
We need to check this and update back.
> https://docs.u-boot.org/en/latest/board/qualcomm/rb3gen2.html
> 
> You'll also need to apply this patch to fix a boot regression
> https://lore.kernel.org/u-boot/20250122-qcom-parse-memory-updates-v2-0-98dfcac821d7@samcday.com/
> 
> On the Linux/DTS side, apply this series, add the appropriate properties
> to enable fw loading as-per the dt-bindings added by this series and
> place the qupv3fw.elf file.
> 
Sure, got it.
> Kind regards,
>>> I noticed a similar issue with this same logic implemented in U-Boot.
>>>
>>> Could you clarify which xfer mode is appropriate for the i2c controllers
>>> on the RB3 Gen 2 and maybe give this a try yourself, or let me know what
>>> other info you'd need to debug this.
>>>
>> Yes, please share the procedure , we will try internally.
>> is there any DTSI change done as part of your testing ?
>>> Thanks and kind regards,
>>>>
>>>> Viken Dadhaniya (7):
>>>>     dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP
>>>>       firmware loading
>>>>     spi: dt-bindings: Document DT properties for QUP firmware loading
>>>>     dt-bindings: serial: Document DT properties for QUP firmware loading
>>>>     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      |  11 +
>>>>    .../serial/qcom,serial-geni-qcom.yaml         |  12 +
>>>>    .../bindings/spi/qcom,spi-geni-qcom.yaml      |  11 +
>>>>    drivers/i2c/busses/i2c-qcom-geni.c            |  11 +-
>>>>    drivers/soc/qcom/qcom-geni-se.c               | 445 ++++++++++++++++++
>>>>    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 +++++++
>>>>    9 files changed, 692 insertions(+), 8 deletions(-)
>>>>    create mode 100644 include/linux/soc/qcom/qup-fw-load.h
>>>>
>>>
>>
>