| Message ID | 20241204150326.1470749-2-quic_vdadhani@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support to load QUP SE firmware from | expand |
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
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>; > }; > ...
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
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 >
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
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>; >> }; >> ... >
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 >> >
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.
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
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
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.
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
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>; }; ...