[PATCH v3 0/4] Enable shared SE support over I2C

[PATCH v3 0/4] Enable shared SE support over I2C

[Mukesh Kumar Savaliya]

This Series adds support to share QUP (Qualcomm Unified peripheral)
based I2C SE (Serial Engine) controller between two subsystems
(Apps/ADSP/TZ etc). Each SS having it's own dedicated GPII(General
Purpose Interface Instance) acting as pipe between SE and GSI(Generic SW-
Interface) DMA HW engine. Hence the sharing of SE is possible only with
I2C SE in GSI mode, not with FIFO/CPU DMA mode.

Subsystem must acquire Lock over the SE so that it gets uninterrupted
control till it unlocks the SE. It also makes sure the commonly shared
TLMM GPIOs are not touched which can impact other subsystem or cause
any interruption. Generally, GPIOs are being unconfigured during
suspend time.

GSI DMA engine is capable to perform requested transfer operations
from any of the I2C client in a seamless way and its transparent to
the subsystems. Make sure to enable “qcom,shared-se” flag only while
enabling this feature. I2C client should add in its respective parent
node. 

Example : 
Two clients from different SS can share an I2C SE for same slave device
OR their owned slave devices.
Assume I2C Slave EEPROM device connected with I2C controller.
Each client from ADSP SS and APPS Linux SS can perform i2c transactions.
This gets serialized by lock TRE + Transfers + Unlock TRE at HW level.


Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
---

Link to V2: https://lore.kernel.org/lkml/a88a16ff-3537-4396-b2ea-4ba02b4850e9@quicinc.com/T/

Changes in V3:
 - Added missing maintainers which i forgot to add.
 - Add cover letter with description of SS and EE for dt-bindings patch.
 - Added acronyms expansion to commit log.
 - [PATCH v2 3/4] : Removed exported symbol geni_se_clks_off(). 
   Instead added changes to bypass pinctrl sleep configuration from
   geni_se_resources_off() function.
 - Changed title name of [PATCH v2 3/4] to reflect the suggested changes.
 - [PATCH v2 4/4] kept geni_i2c_runtime_suspend() as is and removed 
   explicit call to geni_se_clks_off().
 - Removed is_shared variable from i2c driver and instead used common 
   shared_geni_se variable from qcom-geni-se.h so that other protocols
   can also extend for similar feature.
 - I2C driver log changed from dev_err() to dev_dbg() for timeout.
 - set gpi_mode = true if shared_geni_se is set for this usecase. Enhanced
   comments around code and commit log.
---

Link to V1: https://lore.kernel.org/lkml/cb7613d0-586e-4089-a1b6-2405f4dc4883@quicinc.com/T/

Changes in V2:
 - Enhanced commit log grammatically for PATCH v1 3/4 as suggested by Bryan.
 - Updated Cover letter along with acronyms expansion.
 - Added maintainers list from other subsystems for review, which was missing.
   Thanks to Krzysztof for pointing out.
 - Added cover letter with an example of Serial Engine sharing.
 - Addressed review comments for all the patches.

---

Mukesh Kumar Savaliya (4):
  dt-bindindgs: i2c: qcom,i2c-geni: Document shared flag
  dma: gpi: Add Lock and Unlock TRE support to access SE exclusively
  soc: qcom: geni-se: Do not keep GPIOs to sleep state for shared SE
    usecase
  i2c: i2c-qcom-geni: Enable i2c controller sharing between two
    subsystems

 .../bindings/i2c/qcom,i2c-geni-qcom.yaml      |  4 ++
 drivers/dma/qcom/gpi.c                        | 37 ++++++++++++++++++-
 drivers/i2c/busses/i2c-qcom-geni.c            | 24 ++++++++++--
 drivers/soc/qcom/qcom-geni-se.c               | 14 +++++--
 include/linux/dma/qcom-gpi-dma.h              |  6 +++
 include/linux/soc/qcom/geni-se.h              |  3 ++
 6 files changed, 79 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH v3 1/4] dt-bindindgs: i2c: qcom,i2c-geni: Document shared flag

[Mukesh Kumar Savaliya]

Adds qcom,shared-se flag usage. Use this when particular I2C serial
controller needs to be shared between two subsystems.

SE = Serial Engine, meant for I2C controller here.
TRE = Transfer Ring Element, refers to Queued Descriptor.
SS = Subsystems (APPS processor, Modem, TZ, ADSP etc).

Example :
Two clients from different SS can share an I2C SE for same slave device
OR their owned slave devices.
Assume I2C Slave EEPROM device connected with I2C controller.
Each client from ADSP SS and APPS Linux SS can perform i2c transactions.
This gets serialized by lock TRE + DMA Transfers + Unlock TRE at HW level.

Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
---
 Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 4 ++++
 1 file changed, 4 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..3b9b20a0edff 100644
--- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
+++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
@@ -60,6 +60,10 @@ properties:
   power-domains:
     maxItems: 1
 
+  qcom,shared-se:
+    description: True if I2C needs to be shared between two or more subsystems(SS).
+    type: boolean
+
   reg:
     maxItems: 1
 
-- 
2.25.1

Re: [PATCH v3 1/4] dt-bindindgs: i2c: qcom,i2c-geni: Document shared flag

[Krzysztof Kozlowski]

On Fri, Sep 27, 2024 at 12:01:05PM +0530, Mukesh Kumar Savaliya wrote:
> Adds qcom,shared-se flag usage. Use this when particular I2C serial
> controller needs to be shared between two subsystems.
> 
> SE = Serial Engine, meant for I2C controller here.
> TRE = Transfer Ring Element, refers to Queued Descriptor.
> SS = Subsystems (APPS processor, Modem, TZ, ADSP etc).
> 
> Example :
> Two clients from different SS can share an I2C SE for same slave device
> OR their owned slave devices.
> Assume I2C Slave EEPROM device connected with I2C controller.
> Each client from ADSP SS and APPS Linux SS can perform i2c transactions.
> This gets serialized by lock TRE + DMA Transfers + Unlock TRE at HW level.
> 
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> ---
>  Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 4 ++++
>  1 file changed, 4 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..3b9b20a0edff 100644
> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> @@ -60,6 +60,10 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  qcom,shared-se:
> +    description: True if I2C needs to be shared between two or more subsystems(SS).

The "SS" and subsystem should be explained in the binding. Please do not
use some qcom-specific abbreviations here, but explain exactly, e.g.
processors like application processor and DSP.

"se" is also not explained in the binding - please open it and look for
such explanation.

This all should be rephrased to make it clear... We talked about this
and I do not see much of improvements except commit msg, so we are
making circles. I don't know, get someone internally to help you in
upstreaming this.

Is sharing of IP blocks going to be also for other devices? If yes, then
this should be one property for all Qualcomm devices. If not, then be
sure that this is the case because I will bring it up if you come with
one more solution for something else.

Best regards,
Krzysztof

Re: [PATCH v3 1/4] dt-bindindgs: i2c: qcom,i2c-geni: Document shared flag

[Konrad Dybcio]

On 27.09.2024 11:24 AM, Krzysztof Kozlowski wrote:
> On Fri, Sep 27, 2024 at 12:01:05PM +0530, Mukesh Kumar Savaliya wrote:
>> Adds qcom,shared-se flag usage. Use this when particular I2C serial
>> controller needs to be shared between two subsystems.
>>
>> SE = Serial Engine, meant for I2C controller here.
>> TRE = Transfer Ring Element, refers to Queued Descriptor.
>> SS = Subsystems (APPS processor, Modem, TZ, ADSP etc).
>>
>> Example :
>> Two clients from different SS can share an I2C SE for same slave device
>> OR their owned slave devices.
>> Assume I2C Slave EEPROM device connected with I2C controller.
>> Each client from ADSP SS and APPS Linux SS can perform i2c transactions.
>> This gets serialized by lock TRE + DMA Transfers + Unlock TRE at HW level.
>>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> ---
>>  Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 4 ++++
>>  1 file changed, 4 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..3b9b20a0edff 100644
>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> @@ -60,6 +60,10 @@ properties:
>>    power-domains:
>>      maxItems: 1
>>  
>> +  qcom,shared-se:
>> +    description: True if I2C needs to be shared between two or more subsystems(SS).
> 
> The "SS" and subsystem should be explained in the binding. Please do not
> use some qcom-specific abbreviations here, but explain exactly, e.g.
> processors like application processor and DSP.
> 
> "se" is also not explained in the binding - please open it and look for
> such explanation.
> 
> This all should be rephrased to make it clear... We talked about this
> and I do not see much of improvements except commit msg, so we are
> making circles. I don't know, get someone internally to help you in
> upstreaming this.
> 
> Is sharing of IP blocks going to be also for other devices? If yes, then
> this should be one property for all Qualcomm devices. If not, then be
> sure that this is the case because I will bring it up if you come with
> one more solution for something else.

As far as I understand, everything that's not protocol-specific (in
this case it would be I2C tunables etc.) is common across all
protocols supported by the serial engine.

Konrad

Re: [PATCH v3 1/4] dt-bindindgs: i2c: qcom,i2c-geni: Document shared flag

[Krzysztof Kozlowski]

On 27/09/2024 13:20, Konrad Dybcio wrote:
> On 27.09.2024 11:24 AM, Krzysztof Kozlowski wrote:
>> On Fri, Sep 27, 2024 at 12:01:05PM +0530, Mukesh Kumar Savaliya wrote:
>>> Adds qcom,shared-se flag usage. Use this when particular I2C serial
>>> controller needs to be shared between two subsystems.
>>>
>>> SE = Serial Engine, meant for I2C controller here.
>>> TRE = Transfer Ring Element, refers to Queued Descriptor.
>>> SS = Subsystems (APPS processor, Modem, TZ, ADSP etc).
>>>
>>> Example :
>>> Two clients from different SS can share an I2C SE for same slave device
>>> OR their owned slave devices.
>>> Assume I2C Slave EEPROM device connected with I2C controller.
>>> Each client from ADSP SS and APPS Linux SS can perform i2c transactions.
>>> This gets serialized by lock TRE + DMA Transfers + Unlock TRE at HW level.
>>>
>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>>> ---
>>>  Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 4 ++++
>>>  1 file changed, 4 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..3b9b20a0edff 100644
>>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>> @@ -60,6 +60,10 @@ properties:
>>>    power-domains:
>>>      maxItems: 1
>>>  
>>> +  qcom,shared-se:
>>> +    description: True if I2C needs to be shared between two or more subsystems(SS).
>>
>> The "SS" and subsystem should be explained in the binding. Please do not
>> use some qcom-specific abbreviations here, but explain exactly, e.g.
>> processors like application processor and DSP.
>>
>> "se" is also not explained in the binding - please open it and look for
>> such explanation.
>>
>> This all should be rephrased to make it clear... We talked about this
>> and I do not see much of improvements except commit msg, so we are
>> making circles. I don't know, get someone internally to help you in
>> upstreaming this.
>>
>> Is sharing of IP blocks going to be also for other devices? If yes, then
>> this should be one property for all Qualcomm devices. If not, then be
>> sure that this is the case because I will bring it up if you come with
>> one more solution for something else.
> 
> As far as I understand, everything that's not protocol-specific (in
> this case it would be I2C tunables etc.) is common across all
> protocols supported by the serial engine.

Yeah, but I also think about other things like clock controllers, TLMMs,
MMUs and so on. Each of them will get a new "qcom,shared-xxx" property?

I expect Mukesh to solve it in qcom-wide way, not only his one problem.

Best regards,
Krzysztof

Re: [PATCH v3 1/4] dt-bindindgs: i2c: qcom,i2c-geni: Document shared flag

[Mukesh Kumar Savaliya]

On 9/27/2024 2:54 PM, Krzysztof Kozlowski wrote:
> On Fri, Sep 27, 2024 at 12:01:05PM +0530, Mukesh Kumar Savaliya wrote:
>> Adds qcom,shared-se flag usage. Use this when particular I2C serial
>> controller needs to be shared between two subsystems.
>>
>> SE = Serial Engine, meant for I2C controller here.
>> TRE = Transfer Ring Element, refers to Queued Descriptor.
>> SS = Subsystems (APPS processor, Modem, TZ, ADSP etc).
>>
>> Example :
>> Two clients from different SS can share an I2C SE for same slave device
>> OR their owned slave devices.
>> Assume I2C Slave EEPROM device connected with I2C controller.
>> Each client from ADSP SS and APPS Linux SS can perform i2c transactions.
>> This gets serialized by lock TRE + DMA Transfers + Unlock TRE at HW level.
>>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> ---
>>   Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 4 ++++
>>   1 file changed, 4 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..3b9b20a0edff 100644
>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> @@ -60,6 +60,10 @@ properties:
>>     power-domains:
>>       maxItems: 1
>>   
>> +  qcom,shared-se:
>> +    description: True if I2C needs to be shared between two or more subsystems(SS).
> 
> The "SS" and subsystem should be explained in the binding. Please do not
> use some qcom-specific abbreviations here, but explain exactly, e.g.
> processors like application processor and DSP.
> 
> "se" is also not explained in the binding - please open it and look for
> such explanation.
Sure, i thought cover letter explanation is good enough. I will add it 
per patch as cover letter will not be visible and go away after merge.
> 
> This all should be rephrased to make it clear... We talked about this
> and I do not see much of improvements except commit msg, so we are
> making circles. I don't know, get someone internally to help you in
> upstreaming this.
Let me retry to make it better.
Will make SS (subsystem) to system processor (can be APPS or DSP OR any 
other).
> 
> Is sharing of IP blocks going to be also for other devices? If yes, then
> this should be one property for all Qualcomm devices. If not, then be
> sure that this is the case because I will bring it up if you come with
> one more solution for something else.
> 
IP blocks like SE can be shared. Here we are talking about I2C sharing.
In future it can be SPI sharing. But design wise it fits better to add 
flag per SE node. Same we shall be adding for SPI too in future.

Please let me know your further suggestions.
> Best regards,
> Krzysztof
> 

Re: [PATCH v3 1/4] dt-bindindgs: i2c: qcom,i2c-geni: Document shared flag

[Krzysztof Kozlowski]

On 13/11/2024 17:08, Mukesh Kumar Savaliya wrote:
> 
> 
> On 9/27/2024 2:54 PM, Krzysztof Kozlowski wrote:
>> On Fri, Sep 27, 2024 at 12:01:05PM +0530, Mukesh Kumar Savaliya wrote:
>>> Adds qcom,shared-se flag usage. Use this when particular I2C serial
>>> controller needs to be shared between two subsystems.
>>>
>>> SE = Serial Engine, meant for I2C controller here.
>>> TRE = Transfer Ring Element, refers to Queued Descriptor.
>>> SS = Subsystems (APPS processor, Modem, TZ, ADSP etc).
>>>
>>> Example :
>>> Two clients from different SS can share an I2C SE for same slave device
>>> OR their owned slave devices.
>>> Assume I2C Slave EEPROM device connected with I2C controller.
>>> Each client from ADSP SS and APPS Linux SS can perform i2c transactions.
>>> This gets serialized by lock TRE + DMA Transfers + Unlock TRE at HW level.
>>>
>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>>> ---
>>>   Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 4 ++++
>>>   1 file changed, 4 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..3b9b20a0edff 100644
>>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>> @@ -60,6 +60,10 @@ properties:
>>>     power-domains:
>>>       maxItems: 1
>>>   
>>> +  qcom,shared-se:
>>> +    description: True if I2C needs to be shared between two or more subsystems(SS).
>>
>> The "SS" and subsystem should be explained in the binding. Please do not
>> use some qcom-specific abbreviations here, but explain exactly, e.g.
>> processors like application processor and DSP.
>>
>> "se" is also not explained in the binding - please open it and look for
>> such explanation.
> Sure, i thought cover letter explanation is good enough. I will add it 
> per patch as cover letter will not be visible and go away after merge.
>>
>> This all should be rephrased to make it clear... We talked about this
>> and I do not see much of improvements except commit msg, so we are
>> making circles. I don't know, get someone internally to help you in
>> upstreaming this.
> Let me retry to make it better.
> Will make SS (subsystem) to system processor (can be APPS or DSP OR any 
> other).
>>
>> Is sharing of IP blocks going to be also for other devices? If yes, then
>> this should be one property for all Qualcomm devices. If not, then be
>> sure that this is the case because I will bring it up if you come with
>> one more solution for something else.
>>
> IP blocks like SE can be shared. Here we are talking about I2C sharing.
> In future it can be SPI sharing. But design wise it fits better to add 
> flag per SE node. Same we shall be adding for SPI too in future.
> 
> Please let me know your further suggestions.

You responded 1.5 months after my message.

I will provide you suggestions also 1.5 months, when I dig the context.
Oh wait, all previous emails are long gone from my inbox...

Anyway, my above comment stands for all Qualcomm reviews: stop coming up
every month with twenty different "shared IP" properties.

Best regards,
Krzysztof

Re: [PATCH v3 1/4] dt-bindindgs: i2c: qcom,i2c-geni: Document shared flag

[Krzysztof Kozlowski]

On 27/09/2024 08:31, Mukesh Kumar Savaliya wrote:
> Adds qcom,shared-se flag usage. Use this when particular I2C serial
> controller needs to be shared between two subsystems.
> 

Also, fix the typo in subject prefix. It is dt-bindings.

Best regards,
Krzysztof

Re: [PATCH v3 1/4] dt-bindindgs: i2c: qcom,i2c-geni: Document shared flag

[Mukesh Kumar Savaliya]

Thanks Krzysztof!

On 9/27/2024 3:31 PM, Krzysztof Kozlowski wrote:
> On 27/09/2024 08:31, Mukesh Kumar Savaliya wrote:
>> Adds qcom,shared-se flag usage. Use this when particular I2C serial
>> controller needs to be shared between two subsystems.
>>
> 
> Also, fix the typo in subject prefix. It is dt-bindings.
Sure, Done.
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v3 1/4] dt-bindindgs: i2c: qcom,i2c-geni: Document shared flag

[Bjorn Andersson]

On Fri, Sep 27, 2024 at 12:01:05PM GMT, Mukesh Kumar Savaliya wrote:
> Adds qcom,shared-se flag usage. Use this when particular I2C serial
> controller needs to be shared between two subsystems.
> 

https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

> SE = Serial Engine, meant for I2C controller here.
> TRE = Transfer Ring Element, refers to Queued Descriptor.
> SS = Subsystems (APPS processor, Modem, TZ, ADSP etc).

Expressing yourself in terms of abbreviations just makes the text harder
to read. The dictionary is nice, but I don't see that it adds value to
introduce these abbreviations with the reader.

> 
> Example :
> Two clients from different SS can share an I2C SE for same slave device
> OR their owned slave devices.
> Assume I2C Slave EEPROM device connected with I2C controller.
> Each client from ADSP SS and APPS Linux SS can perform i2c transactions.
> This gets serialized by lock TRE + DMA Transfers + Unlock TRE at HW level.
> 

Don't describe your problem using a bullet-point list. You should be
able to express it in a flowing English sentence/paragraph.

> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> ---
>  Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 4 ++++
>  1 file changed, 4 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..3b9b20a0edff 100644
> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> @@ -60,6 +60,10 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  qcom,shared-se:
> +    description: True if I2C needs to be shared between two or more subsystems(SS).

I see no value in establishing the "SS" abbreviation here, what would be
useful is to write this sentence such that it establishes the "SE"
abbreviation (to avoid having to expand the property).

On the other hand, it's a boolean property in a serial-engine node, so
I don't know if it's worth repeating "se" here. "qcom,is-shared" sounds
like a better boolean in a se-node.

Regards,
Bjorn

> +    type: boolean
> +
>    reg:
>      maxItems: 1
>  
> -- 
> 2.25.1
> 

Re: [PATCH v3 1/4] dt-bindindgs: i2c: qcom,i2c-geni: Document shared flag

[Mukesh Kumar Savaliya]

Thanks Bjorn !

On 9/30/2024 8:50 AM, Bjorn Andersson wrote:
> On Fri, Sep 27, 2024 at 12:01:05PM GMT, Mukesh Kumar Savaliya wrote:
>> Adds qcom,shared-se flag usage. Use this when particular I2C serial
>> controller needs to be shared between two subsystems.
>>
> 
> https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> 
>> SE = Serial Engine, meant for I2C controller here.
>> TRE = Transfer Ring Element, refers to Queued Descriptor.
>> SS = Subsystems (APPS processor, Modem, TZ, ADSP etc).
> 
> Expressing yourself in terms of abbreviations just makes the text harder
> to read. The dictionary is nice, but I don't see that it adds value to
> introduce these abbreviations with the reader.
Sure, thought it will ease the explanations of abbreviations. I learnt 
that not to use it and directly use complete name in opensource way.
> 
>>
>> Example :
>> Two clients from different SS can share an I2C SE for same slave device
>> OR their owned slave devices.
>> Assume I2C Slave EEPROM device connected with I2C controller.
>> Each client from ADSP SS and APPS Linux SS can perform i2c transactions.
>> This gets serialized by lock TRE + DMA Transfers + Unlock TRE at HW level.
>>
> 
> Don't describe your problem using a bullet-point list. You should be
> able to express it in a flowing English sentence/paragraph.
Sure Bjorn.
> 
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> ---
>>   Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 4 ++++
>>   1 file changed, 4 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..3b9b20a0edff 100644
>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> @@ -60,6 +60,10 @@ properties:
>>     power-domains:
>>       maxItems: 1
>>   
>> +  qcom,shared-se:
>> +    description: True if I2C needs to be shared between two or more subsystems(SS).
> 
> I see no value in establishing the "SS" abbreviation here, what would be
> useful is to write this sentence such that it establishes the "SE"
> abbreviation (to avoid having to expand the property).
> 
> On the other hand, it's a boolean property in a serial-engine node, so
> I don't know if it's worth repeating "se" here. "qcom,is-shared" sounds
> like a better boolean in a se-node.
> 
Sure, Done. I thought shared-se will make it understand that SE is 
shared. I will modify to qcom,is-shared as recommended.
Also i am removing SS and make it multiprocessor as asked by Bryan.
> Regards,
> Bjorn
> 
>> +    type: boolean
>> +
>>     reg:
>>       maxItems: 1
>>   
>> -- 
>> 2.25.1
>>

[PATCH v3 2/4] dma: gpi: Add Lock and Unlock TRE support to access SE exclusively

[Mukesh Kumar Savaliya]

GSI DMA provides specific TREs namely Lock and Unlock TRE, which
provides mutual exclusive access to SE from any of the subsystem
(E.g. Apps, TZ, ADSP etc). Lock prevents other subsystems from
concurrently performing DMA transfers and avoids disturbance to
data path. Basically for shared SE usecase, lock the SE for
particular subsystem, complete the transfer, unlock the SE.

Apply Lock TRE for the first transfer of shared SE and Apply Unlock
TRE for the last transfer.

Also change MAX_TRE macro to 5 from 3 because of the two additional TREs.

TRE = Transfer Ring Element, refers to the queued descriptor.
SE = Serial Engine
SS = Subsystems (Apps processor, TZ, ADSP, Modem)

Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
---
 drivers/dma/qcom/gpi.c           | 37 +++++++++++++++++++++++++++++++-
 include/linux/dma/qcom-gpi-dma.h |  6 ++++++
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
index 52a7c8f2498f..120d91234442 100644
--- a/drivers/dma/qcom/gpi.c
+++ b/drivers/dma/qcom/gpi.c
@@ -2,6 +2,7 @@
 /*
  * Copyright (c) 2017-2020, The Linux Foundation. All rights reserved.
  * Copyright (c) 2020, Linaro Limited
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #include <dt-bindings/dma/qcom-gpi.h>
@@ -65,6 +66,14 @@
 /* DMA TRE */
 #define TRE_DMA_LEN		GENMASK(23, 0)
 
+/* Lock TRE */
+#define TRE_I2C_LOCK		BIT(0)
+#define TRE_MINOR_TYPE		GENMASK(19, 16)
+#define TRE_MAJOR_TYPE		GENMASK(23, 20)
+
+/* Unlock TRE */
+#define TRE_I2C_UNLOCK		BIT(8)
+
 /* Register offsets from gpi-top */
 #define GPII_n_CH_k_CNTXT_0_OFFS(n, k)	(0x20000 + (0x4000 * (n)) + (0x80 * (k)))
 #define GPII_n_CH_k_CNTXT_0_EL_SIZE	GENMASK(31, 24)
@@ -516,7 +525,7 @@ struct gpii {
 	bool ieob_set;
 };
 
-#define MAX_TRE 3
+#define MAX_TRE 5
 
 struct gpi_desc {
 	struct virt_dma_desc vd;
@@ -1637,6 +1646,19 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
 	struct gpi_tre *tre;
 	unsigned int i;
 
+	/* create lock tre for first tranfser */
+	if (i2c->shared_se && i2c->first_msg) {
+		tre = &desc->tre[tre_idx];
+		tre_idx++;
+
+		tre->dword[0] = 0;
+		tre->dword[1] = 0;
+		tre->dword[2] = 0;
+		tre->dword[3] = u32_encode_bits(1, TRE_I2C_LOCK);
+		tre->dword[3] |= u32_encode_bits(0, TRE_MINOR_TYPE);
+		tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE);
+	}
+
 	/* first create config tre if applicable */
 	if (i2c->set_config) {
 		tre = &desc->tre[tre_idx];
@@ -1695,6 +1717,19 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
 		tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_IEOT);
 	}
 
+	/* Unlock tre for last transfer */
+	if (i2c->shared_se && i2c->last_msg && i2c->op != I2C_READ) {
+		tre = &desc->tre[tre_idx];
+		tre_idx++;
+
+		tre->dword[0] = 0;
+		tre->dword[1] = 0;
+		tre->dword[2] = 0;
+		tre->dword[3] = u32_encode_bits(1, TRE_I2C_UNLOCK);
+		tre->dword[3] |= u32_encode_bits(1, TRE_MINOR_TYPE);
+		tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE);
+	}
+
 	for (i = 0; i < tre_idx; i++)
 		dev_dbg(dev, "TRE:%d %x:%x:%x:%x\n", i, desc->tre[i].dword[0],
 			desc->tre[i].dword[1], desc->tre[i].dword[2], desc->tre[i].dword[3]);
diff --git a/include/linux/dma/qcom-gpi-dma.h b/include/linux/dma/qcom-gpi-dma.h
index 6680dd1a43c6..8589c711afae 100644
--- a/include/linux/dma/qcom-gpi-dma.h
+++ b/include/linux/dma/qcom-gpi-dma.h
@@ -65,6 +65,9 @@ enum i2c_op {
  * @rx_len: receive length for buffer
  * @op: i2c cmd
  * @muli-msg: is part of multi i2c r-w msgs
+ * @shared_se: bus is shared between subsystems
+ * @bool first_msg: use it for tracking multimessage xfer
+ * @bool last_msg: use it for tracking multimessage xfer
  */
 struct gpi_i2c_config {
 	u8 set_config;
@@ -78,6 +81,9 @@ struct gpi_i2c_config {
 	u32 rx_len;
 	enum i2c_op op;
 	bool multi_msg;
+	bool shared_se;
+	bool first_msg;
+	bool last_msg;
 };
 
 #endif /* QCOM_GPI_DMA_H */
-- 
2.25.1

Re: [PATCH v3 2/4] dma: gpi: Add Lock and Unlock TRE support to access SE exclusively

[Vinod Koul]

On 27-09-24, 12:01, Mukesh Kumar Savaliya wrote:
> GSI DMA provides specific TREs namely Lock and Unlock TRE, which
> provides mutual exclusive access to SE from any of the subsystem
> (E.g. Apps, TZ, ADSP etc). Lock prevents other subsystems from
> concurrently performing DMA transfers and avoids disturbance to
> data path. Basically for shared SE usecase, lock the SE for
> particular subsystem, complete the transfer, unlock the SE.

it is dmaengine: xxx so please update that

> 
> Apply Lock TRE for the first transfer of shared SE and Apply Unlock
> TRE for the last transfer.
> 
> Also change MAX_TRE macro to 5 from 3 because of the two additional TREs.
> 
> TRE = Transfer Ring Element, refers to the queued descriptor.
> SE = Serial Engine
> SS = Subsystems (Apps processor, TZ, ADSP, Modem)
> 
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> ---
>  drivers/dma/qcom/gpi.c           | 37 +++++++++++++++++++++++++++++++-
>  include/linux/dma/qcom-gpi-dma.h |  6 ++++++
>  2 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index 52a7c8f2498f..120d91234442 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
> @@ -2,6 +2,7 @@
>  /*
>   * Copyright (c) 2017-2020, The Linux Foundation. All rights reserved.
>   * Copyright (c) 2020, Linaro Limited
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>   */
>  
>  #include <dt-bindings/dma/qcom-gpi.h>
> @@ -65,6 +66,14 @@
>  /* DMA TRE */
>  #define TRE_DMA_LEN		GENMASK(23, 0)
>  
> +/* Lock TRE */
> +#define TRE_I2C_LOCK		BIT(0)
> +#define TRE_MINOR_TYPE		GENMASK(19, 16)
> +#define TRE_MAJOR_TYPE		GENMASK(23, 20)
> +
> +/* Unlock TRE */
> +#define TRE_I2C_UNLOCK		BIT(8)
> +
>  /* Register offsets from gpi-top */
>  #define GPII_n_CH_k_CNTXT_0_OFFS(n, k)	(0x20000 + (0x4000 * (n)) + (0x80 * (k)))
>  #define GPII_n_CH_k_CNTXT_0_EL_SIZE	GENMASK(31, 24)
> @@ -516,7 +525,7 @@ struct gpii {
>  	bool ieob_set;
>  };
>  
> -#define MAX_TRE 3
> +#define MAX_TRE 5
>  
>  struct gpi_desc {
>  	struct virt_dma_desc vd;
> @@ -1637,6 +1646,19 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
>  	struct gpi_tre *tre;
>  	unsigned int i;
>  
> +	/* create lock tre for first tranfser */
> +	if (i2c->shared_se && i2c->first_msg) {
> +		tre = &desc->tre[tre_idx];
> +		tre_idx++;
> +
> +		tre->dword[0] = 0;
> +		tre->dword[1] = 0;
> +		tre->dword[2] = 0;
> +		tre->dword[3] = u32_encode_bits(1, TRE_I2C_LOCK);
> +		tre->dword[3] |= u32_encode_bits(0, TRE_MINOR_TYPE);
> +		tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE);
> +	}
> +
>  	/* first create config tre if applicable */
>  	if (i2c->set_config) {
>  		tre = &desc->tre[tre_idx];
> @@ -1695,6 +1717,19 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
>  		tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_IEOT);
>  	}
>  
> +	/* Unlock tre for last transfer */
> +	if (i2c->shared_se && i2c->last_msg && i2c->op != I2C_READ) {
> +		tre = &desc->tre[tre_idx];
> +		tre_idx++;
> +
> +		tre->dword[0] = 0;
> +		tre->dword[1] = 0;
> +		tre->dword[2] = 0;
> +		tre->dword[3] = u32_encode_bits(1, TRE_I2C_UNLOCK);
> +		tre->dword[3] |= u32_encode_bits(1, TRE_MINOR_TYPE);
> +		tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE);
> +	}
> +
>  	for (i = 0; i < tre_idx; i++)
>  		dev_dbg(dev, "TRE:%d %x:%x:%x:%x\n", i, desc->tre[i].dword[0],
>  			desc->tre[i].dword[1], desc->tre[i].dword[2], desc->tre[i].dword[3]);
> diff --git a/include/linux/dma/qcom-gpi-dma.h b/include/linux/dma/qcom-gpi-dma.h
> index 6680dd1a43c6..8589c711afae 100644
> --- a/include/linux/dma/qcom-gpi-dma.h
> +++ b/include/linux/dma/qcom-gpi-dma.h
> @@ -65,6 +65,9 @@ enum i2c_op {
>   * @rx_len: receive length for buffer
>   * @op: i2c cmd
>   * @muli-msg: is part of multi i2c r-w msgs
> + * @shared_se: bus is shared between subsystems
> + * @bool first_msg: use it for tracking multimessage xfer
> + * @bool last_msg: use it for tracking multimessage xfer
>   */
>  struct gpi_i2c_config {
>  	u8 set_config;
> @@ -78,6 +81,9 @@ struct gpi_i2c_config {
>  	u32 rx_len;
>  	enum i2c_op op;
>  	bool multi_msg;
> +	bool shared_se;
> +	bool first_msg;
> +	bool last_msg;

Looking at the usage in following patches, why cant this be handled
internally as part of prep call?


>  };
>  
>  #endif /* QCOM_GPI_DMA_H */
> -- 
> 2.25.1

-- 
~Vinod

Re: [PATCH v3 2/4] dma: gpi: Add Lock and Unlock TRE support to access SE exclusively

[Mukesh Kumar Savaliya]

Thanks Vinod for your review comments !

On 10/15/2024 12:01 AM, Vinod Koul wrote:
> On 27-09-24, 12:01, Mukesh Kumar Savaliya wrote:
>> GSI DMA provides specific TREs namely Lock and Unlock TRE, which
>> provides mutual exclusive access to SE from any of the subsystem
>> (E.g. Apps, TZ, ADSP etc). Lock prevents other subsystems from
>> concurrently performing DMA transfers and avoids disturbance to
>> data path. Basically for shared SE usecase, lock the SE for
>> particular subsystem, complete the transfer, unlock the SE.
> 
> it is dmaengine: xxx so please update that
> 
Done
>>
>> Apply Lock TRE for the first transfer of shared SE and Apply Unlock
>> TRE for the last transfer.
>>
>> Also change MAX_TRE macro to 5 from 3 because of the two additional TREs.
>>
>> TRE = Transfer Ring Element, refers to the queued descriptor.
>> SE = Serial Engine
>> SS = Subsystems (Apps processor, TZ, ADSP, Modem)
>>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> ---
>>   drivers/dma/qcom/gpi.c           | 37 +++++++++++++++++++++++++++++++-
>>   include/linux/dma/qcom-gpi-dma.h |  6 ++++++
>>   2 files changed, 42 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
>> index 52a7c8f2498f..120d91234442 100644
>> --- a/drivers/dma/qcom/gpi.c
>> +++ b/drivers/dma/qcom/gpi.c
>> @@ -2,6 +2,7 @@
>>   /*
>>    * Copyright (c) 2017-2020, The Linux Foundation. All rights reserved.
>>    * Copyright (c) 2020, Linaro Limited
>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>    */
>>   
>>   #include <dt-bindings/dma/qcom-gpi.h>
>> @@ -65,6 +66,14 @@
>>   /* DMA TRE */
>>   #define TRE_DMA_LEN		GENMASK(23, 0)
>>   
>> +/* Lock TRE */
>> +#define TRE_I2C_LOCK		BIT(0)
>> +#define TRE_MINOR_TYPE		GENMASK(19, 16)
>> +#define TRE_MAJOR_TYPE		GENMASK(23, 20)
>> +
>> +/* Unlock TRE */
>> +#define TRE_I2C_UNLOCK		BIT(8)
>> +
>>   /* Register offsets from gpi-top */
>>   #define GPII_n_CH_k_CNTXT_0_OFFS(n, k)	(0x20000 + (0x4000 * (n)) + (0x80 * (k)))
>>   #define GPII_n_CH_k_CNTXT_0_EL_SIZE	GENMASK(31, 24)
>> @@ -516,7 +525,7 @@ struct gpii {
>>   	bool ieob_set;
>>   };
>>   
>> -#define MAX_TRE 3
>> +#define MAX_TRE 5
>>   
>>   struct gpi_desc {
>>   	struct virt_dma_desc vd;
>> @@ -1637,6 +1646,19 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
>>   	struct gpi_tre *tre;
>>   	unsigned int i;
>>   
>> +	/* create lock tre for first tranfser */
>> +	if (i2c->shared_se && i2c->first_msg) {
>> +		tre = &desc->tre[tre_idx];
>> +		tre_idx++;
>> +
>> +		tre->dword[0] = 0;
>> +		tre->dword[1] = 0;
>> +		tre->dword[2] = 0;
>> +		tre->dword[3] = u32_encode_bits(1, TRE_I2C_LOCK);
>> +		tre->dword[3] |= u32_encode_bits(0, TRE_MINOR_TYPE);
>> +		tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE);
>> +	}
>> +
>>   	/* first create config tre if applicable */
>>   	if (i2c->set_config) {
>>   		tre = &desc->tre[tre_idx];
>> @@ -1695,6 +1717,19 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
>>   		tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_IEOT);
>>   	}
>>   
>> +	/* Unlock tre for last transfer */
>> +	if (i2c->shared_se && i2c->last_msg && i2c->op != I2C_READ) {
>> +		tre = &desc->tre[tre_idx];
>> +		tre_idx++;
>> +
>> +		tre->dword[0] = 0;
>> +		tre->dword[1] = 0;
>> +		tre->dword[2] = 0;
>> +		tre->dword[3] = u32_encode_bits(1, TRE_I2C_UNLOCK);
>> +		tre->dword[3] |= u32_encode_bits(1, TRE_MINOR_TYPE);
>> +		tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE);
>> +	}
>> +
>>   	for (i = 0; i < tre_idx; i++)
>>   		dev_dbg(dev, "TRE:%d %x:%x:%x:%x\n", i, desc->tre[i].dword[0],
>>   			desc->tre[i].dword[1], desc->tre[i].dword[2], desc->tre[i].dword[3]);
>> diff --git a/include/linux/dma/qcom-gpi-dma.h b/include/linux/dma/qcom-gpi-dma.h
>> index 6680dd1a43c6..8589c711afae 100644
>> --- a/include/linux/dma/qcom-gpi-dma.h
>> +++ b/include/linux/dma/qcom-gpi-dma.h
>> @@ -65,6 +65,9 @@ enum i2c_op {
>>    * @rx_len: receive length for buffer
>>    * @op: i2c cmd
>>    * @muli-msg: is part of multi i2c r-w msgs
>> + * @shared_se: bus is shared between subsystems
>> + * @bool first_msg: use it for tracking multimessage xfer
>> + * @bool last_msg: use it for tracking multimessage xfer
>>    */
>>   struct gpi_i2c_config {
>>   	u8 set_config;
>> @@ -78,6 +81,9 @@ struct gpi_i2c_config {
>>   	u32 rx_len;
>>   	enum i2c_op op;
>>   	bool multi_msg;
>> +	bool shared_se;
>> +	bool first_msg;
>> +	bool last_msg;
> 
> Looking at the usage in following patches, why cant this be handled
> internally as part of prep call?
> 
As per design, i2c driver iterates over each message and submits to GPI 
where it creates TRE. Since it's per transfer, we need to create Lock 
and Unlock TRE based on first or last message.
> 
>>   };
>>   
>>   #endif /* QCOM_GPI_DMA_H */
>> -- 
>> 2.25.1
> 

Re: [PATCH v3 2/4] dma: gpi: Add Lock and Unlock TRE support to access SE exclusively

[Konrad Dybcio]

On 27.09.2024 8:31 AM, Mukesh Kumar Savaliya wrote:
> GSI DMA provides specific TREs namely Lock and Unlock TRE, which
> provides mutual exclusive access to SE from any of the subsystem
> (E.g. Apps, TZ, ADSP etc). Lock prevents other subsystems from
> concurrently performing DMA transfers and avoids disturbance to
> data path. Basically for shared SE usecase, lock the SE for
> particular subsystem, complete the transfer, unlock the SE.
> 
> Apply Lock TRE for the first transfer of shared SE and Apply Unlock
> TRE for the last transfer.
> 
> Also change MAX_TRE macro to 5 from 3 because of the two additional TREs.
> 
> TRE = Transfer Ring Element, refers to the queued descriptor.
> SE = Serial Engine
> SS = Subsystems (Apps processor, TZ, ADSP, Modem)
> 
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> ---
>  drivers/dma/qcom/gpi.c           | 37 +++++++++++++++++++++++++++++++-
>  include/linux/dma/qcom-gpi-dma.h |  6 ++++++
>  2 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index 52a7c8f2498f..120d91234442 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
> @@ -2,6 +2,7 @@
>  /*
>   * Copyright (c) 2017-2020, The Linux Foundation. All rights reserved.
>   * Copyright (c) 2020, Linaro Limited
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>   */
>  
>  #include <dt-bindings/dma/qcom-gpi.h>
> @@ -65,6 +66,14 @@
>  /* DMA TRE */
>  #define TRE_DMA_LEN		GENMASK(23, 0)
>  
> +/* Lock TRE */
> +#define TRE_I2C_LOCK		BIT(0)
> +#define TRE_MINOR_TYPE		GENMASK(19, 16)
> +#define TRE_MAJOR_TYPE		GENMASK(23, 20)

Is this solely I2C specific?

Konrad

Re: [PATCH v3 2/4] dma: gpi: Add Lock and Unlock TRE support to access SE exclusively

[Mukesh Kumar Savaliya]

Thanks Konrad for the review !

On 10/26/2024 12:18 AM, Konrad Dybcio wrote:
> On 27.09.2024 8:31 AM, Mukesh Kumar Savaliya wrote:
>> GSI DMA provides specific TREs namely Lock and Unlock TRE, which
>> provides mutual exclusive access to SE from any of the subsystem
>> (E.g. Apps, TZ, ADSP etc). Lock prevents other subsystems from
>> concurrently performing DMA transfers and avoids disturbance to
>> data path. Basically for shared SE usecase, lock the SE for
>> particular subsystem, complete the transfer, unlock the SE.
>>
>> Apply Lock TRE for the first transfer of shared SE and Apply Unlock
>> TRE for the last transfer.
>>
>> Also change MAX_TRE macro to 5 from 3 because of the two additional TREs.
>>
>> TRE = Transfer Ring Element, refers to the queued descriptor.
>> SE = Serial Engine
>> SS = Subsystems (Apps processor, TZ, ADSP, Modem)
>>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> ---
>>   drivers/dma/qcom/gpi.c           | 37 +++++++++++++++++++++++++++++++-
>>   include/linux/dma/qcom-gpi-dma.h |  6 ++++++
>>   2 files changed, 42 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
>> index 52a7c8f2498f..120d91234442 100644
>> --- a/drivers/dma/qcom/gpi.c
>> +++ b/drivers/dma/qcom/gpi.c
>> @@ -2,6 +2,7 @@
>>   /*
>>    * Copyright (c) 2017-2020, The Linux Foundation. All rights reserved.
>>    * Copyright (c) 2020, Linaro Limited
>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>    */
>>   
>>   #include <dt-bindings/dma/qcom-gpi.h>
>> @@ -65,6 +66,14 @@
>>   /* DMA TRE */
>>   #define TRE_DMA_LEN		GENMASK(23, 0)
>>   
>> +/* Lock TRE */
>> +#define TRE_I2C_LOCK		BIT(0)
>> +#define TRE_MINOR_TYPE		GENMASK(19, 16)
>> +#define TRE_MAJOR_TYPE		GENMASK(23, 20)
> 
> Is this solely I2C specific?
> 
No, its generic. So i guess i should rename to TRE_LOCK only, we can use 
for other protocols in future. Thanks!
> Konrad

[PATCH v3 3/4] soc: qcom: geni-se: Do not keep GPIOs to sleep state for shared SE usecase

[Mukesh Kumar Savaliya]

Currently the driver provides a function called geni_serial_resources_off()
to turn off resources like clocks and  pinctrl.

For shared SE between two SS, we don't need to keep pinctrl to sleep state
as other SS may be actively transferring data over SE. Hence,bypass keeping
pinctrl to sleep state conditionally using shared_geni_se flag.

Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
---
 drivers/soc/qcom/qcom-geni-se.c  | 14 ++++++++++----
 include/linux/soc/qcom/geni-se.h |  3 +++
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 2e8f24d5da80..89cf18699336 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__
@@ -503,10 +504,15 @@ int geni_se_resources_off(struct geni_se *se)
 
 	if (has_acpi_companion(se->dev))
 		return 0;
-
-	ret = pinctrl_pm_select_sleep_state(se->dev);
-	if (ret)
-		return ret;
+	/* Keep pinctrl to sleep state only for regular usecase.
+	 * Do not sleep pinctrl for shared SE because other SS(subsystems)
+	 * may continueto perform transfer.
+	 */
+	if (se->shared_geni_se == false) {
+		ret = pinctrl_pm_select_sleep_state(se->dev);
+		if (ret)
+			return ret;
+	}
 
 	geni_se_clks_off(se);
 	return 0;
diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
index c3bca9c0bf2c..359041c64ad8 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) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #ifndef _LINUX_QCOM_GENI_SE
@@ -61,6 +62,7 @@ struct geni_icc_path {
  * @num_clk_levels:	Number of valid clock levels in clk_perf_tbl
  * @clk_perf_tbl:	Table of clock frequency input to serial engine clock
  * @icc_paths:		Array of ICC paths for SE
+ * @shared_geni_se:	Tells if SE is used by two SS in shared environment.
  */
 struct geni_se {
 	void __iomem *base;
@@ -70,6 +72,7 @@ struct geni_se {
 	unsigned int num_clk_levels;
 	unsigned long *clk_perf_tbl;
 	struct geni_icc_path icc_paths[3];
+	bool shared_geni_se;
 };
 
 /* Common SE registers */
-- 
2.25.1

Re: [PATCH v3 3/4] soc: qcom: geni-se: Do not keep GPIOs to sleep state for shared SE usecase

[Bjorn Andersson]

On Fri, Sep 27, 2024 at 12:01:07PM GMT, Mukesh Kumar Savaliya wrote:
> Currently the driver provides a function called geni_serial_resources_off()
> to turn off resources like clocks and  pinctrl.
> 
> For shared SE between two SS, we don't need to keep pinctrl to sleep state
> as other SS may be actively transferring data over SE.

"don't need to" sounds like an optimization. Is this really the case?
The comment in the code below seems to indicate no.

As with the other commit message, expand your abbreviations.

> Hence,bypass keeping
> pinctrl to sleep state conditionally using shared_geni_se flag.
> 
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> ---
>  drivers/soc/qcom/qcom-geni-se.c  | 14 ++++++++++----
>  include/linux/soc/qcom/geni-se.h |  3 +++
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 2e8f24d5da80..89cf18699336 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__
> @@ -503,10 +504,15 @@ int geni_se_resources_off(struct geni_se *se)
>  
>  	if (has_acpi_companion(se->dev))
>  		return 0;
> -
> -	ret = pinctrl_pm_select_sleep_state(se->dev);
> -	if (ret)
> -		return ret;
> +	/* Keep pinctrl to sleep state only for regular usecase.
> +	 * Do not sleep pinctrl for shared SE because other SS(subsystems)
> +	 * may continueto perform transfer.
> +	 */
> +	if (se->shared_geni_se == false) {
> +		ret = pinctrl_pm_select_sleep_state(se->dev);

I'm a bit rusty on the pinctrl API, but wouldn't you achieve the same
result by just not specifying a "sleep" pinctrl state?

> +		if (ret)
> +			return ret;
> +	}
>  
>  	geni_se_clks_off(se);
>  	return 0;
> diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
> index c3bca9c0bf2c..359041c64ad8 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) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>   */
>  
>  #ifndef _LINUX_QCOM_GENI_SE
> @@ -61,6 +62,7 @@ struct geni_icc_path {
>   * @num_clk_levels:	Number of valid clock levels in clk_perf_tbl
>   * @clk_perf_tbl:	Table of clock frequency input to serial engine clock
>   * @icc_paths:		Array of ICC paths for SE
> + * @shared_geni_se:	Tells if SE is used by two SS in shared environment.

Please avoid the abbreviations. Be succinct, e.g. does it matter that
it's two SS - what if it's 3?
"Tell" is not the correct verb here, struct members don't speak.

Regards,
Bjorn

>   */
>  struct geni_se {
>  	void __iomem *base;
> @@ -70,6 +72,7 @@ struct geni_se {
>  	unsigned int num_clk_levels;
>  	unsigned long *clk_perf_tbl;
>  	struct geni_icc_path icc_paths[3];
> +	bool shared_geni_se;
>  };
>  
>  /* Common SE registers */
> -- 
> 2.25.1
> 

Re: [PATCH v3 3/4] soc: qcom: geni-se: Do not keep GPIOs to sleep state for shared SE usecase

[Mukesh Kumar Savaliya]

Hi Bjorn,

On 9/30/2024 8:57 AM, Bjorn Andersson wrote:
> On Fri, Sep 27, 2024 at 12:01:07PM GMT, Mukesh Kumar Savaliya wrote:
>> Currently the driver provides a function called geni_serial_resources_off()
>> to turn off resources like clocks and  pinctrl.
>>
>> For shared SE between two SS, we don't need to keep pinctrl to sleep state
>> as other SS may be actively transferring data over SE.
> 
> "don't need to" sounds like an optimization. Is this really the case?
> The comment in the code below seems to indicate no.
It's not an optimization but real need for shared SE.
> 
> As with the other commit message, expand your abbreviations.
> 
Sure.
>> Hence,bypass keeping
>> pinctrl to sleep state conditionally using shared_geni_se flag.
>>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> ---
>>   drivers/soc/qcom/qcom-geni-se.c  | 14 ++++++++++----
>>   include/linux/soc/qcom/geni-se.h |  3 +++
>>   2 files changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
>> index 2e8f24d5da80..89cf18699336 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__
>> @@ -503,10 +504,15 @@ int geni_se_resources_off(struct geni_se *se)
>>   
>>   	if (has_acpi_companion(se->dev))
>>   		return 0;
>> -
>> -	ret = pinctrl_pm_select_sleep_state(se->dev);
>> -	if (ret)
>> -		return ret;
>> +	/* Keep pinctrl to sleep state only for regular usecase.
>> +	 * Do not sleep pinctrl for shared SE because other SS(subsystems)
>> +	 * may continueto perform transfer.
>> +	 */
>> +	if (se->shared_geni_se == false) {
>> +		ret = pinctrl_pm_select_sleep_state(se->dev);
> 
> I'm a bit rusty on the pinctrl API, but wouldn't you achieve the same
> result by just not specifying a "sleep" pinctrl state?
I find it more generic if code takes required action based on the flag. 
As such this flag is required to perform other actions too. Rather 
guiding with documentations, it would be good to have feature flag and 
manage.
> 
>> +		if (ret)
>> +			return ret;
>> +	}
>>   
>>   	geni_se_clks_off(se);
>>   	return 0;
>> diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
>> index c3bca9c0bf2c..359041c64ad8 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) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>    */
>>   
>>   #ifndef _LINUX_QCOM_GENI_SE
>> @@ -61,6 +62,7 @@ struct geni_icc_path {
>>    * @num_clk_levels:	Number of valid clock levels in clk_perf_tbl
>>    * @clk_perf_tbl:	Table of clock frequency input to serial engine clock
>>    * @icc_paths:		Array of ICC paths for SE
>> + * @shared_geni_se:	Tells if SE is used by two SS in shared environment.
> 
> Please avoid the abbreviations. Be succinct, e.g. does it matter that
> it's two SS - what if it's 3?
Sure Bjorn, I shall enhance like multiprocessors running different OS.
> "Tell" is not the correct verb here, struct members don't speak.
Agree, corrected.
> 
> Regards,
> Bjorn
> 
>>    */
>>   struct geni_se {
>>   	void __iomem *base;
>> @@ -70,6 +72,7 @@ struct geni_se {
>>   	unsigned int num_clk_levels;
>>   	unsigned long *clk_perf_tbl;
>>   	struct geni_icc_path icc_paths[3];
>> +	bool shared_geni_se;
>>   };
>>   
>>   /* Common SE registers */
>> -- 
>> 2.25.1
>>

Re: [PATCH v3 3/4] soc: qcom: geni-se: Do not keep GPIOs to sleep state for shared SE usecase

[Konrad Dybcio]

On 27.09.2024 8:31 AM, Mukesh Kumar Savaliya wrote:
> Currently the driver provides a function called geni_serial_resources_off()
> to turn off resources like clocks and  pinctrl.
> 
> For shared SE between two SS, we don't need to keep pinctrl to sleep state
> as other SS may be actively transferring data over SE. Hence,bypass keeping
> pinctrl to sleep state conditionally using shared_geni_se flag.
> 
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> ---
>  drivers/soc/qcom/qcom-geni-se.c  | 14 ++++++++++----
>  include/linux/soc/qcom/geni-se.h |  3 +++
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 2e8f24d5da80..89cf18699336 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__
> @@ -503,10 +504,15 @@ int geni_se_resources_off(struct geni_se *se)
>  
>  	if (has_acpi_companion(se->dev))
>  		return 0;
> -
> -	ret = pinctrl_pm_select_sleep_state(se->dev);
> -	if (ret)
> -		return ret;
> +	/* Keep pinctrl to sleep state only for regular usecase.
> +	 * Do not sleep pinctrl for shared SE because other SS(subsystems)
> +	 * may continueto perform transfer.
> +	 */

/*
 * Don't alter pin states on shared SEs to avoid potentially
 * interrupting transfers started by other subsystems
 */


> +	if (se->shared_geni_se == false) {

if (!se->shared_geni_se)

> +		ret = pinctrl_pm_select_sleep_state(se->dev);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	geni_se_clks_off(se);

Should the clocks be turned off?

Konrad

Re: [PATCH v3 3/4] soc: qcom: geni-se: Do not keep GPIOs to sleep state for shared SE usecase

[Mukesh Kumar Savaliya]

Thanks Konrad for the review.

On 10/26/2024 12:23 AM, Konrad Dybcio wrote:
> On 27.09.2024 8:31 AM, Mukesh Kumar Savaliya wrote:
>> Currently the driver provides a function called geni_serial_resources_off()
>> to turn off resources like clocks and  pinctrl.
>>
>> For shared SE between two SS, we don't need to keep pinctrl to sleep state
>> as other SS may be actively transferring data over SE. Hence,bypass keeping
>> pinctrl to sleep state conditionally using shared_geni_se flag.
>>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> ---
>>   drivers/soc/qcom/qcom-geni-se.c  | 14 ++++++++++----
>>   include/linux/soc/qcom/geni-se.h |  3 +++
>>   2 files changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
>> index 2e8f24d5da80..89cf18699336 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__
>> @@ -503,10 +504,15 @@ int geni_se_resources_off(struct geni_se *se)
>>   
>>   	if (has_acpi_companion(se->dev))
>>   		return 0;
>> -
>> -	ret = pinctrl_pm_select_sleep_state(se->dev);
>> -	if (ret)
>> -		return ret;
>> +	/* Keep pinctrl to sleep state only for regular usecase.
>> +	 * Do not sleep pinctrl for shared SE because other SS(subsystems)
>> +	 * may continueto perform transfer.
>> +	 */
> 
> /*
>   * Don't alter pin states on shared SEs to avoid potentially
>   * interrupting transfers started by other subsystems
>   */
> 
Done
> 
>> +	if (se->shared_geni_se == false) {
> 
> if (!se->shared_geni_se)
Done
> 
>> +		ret = pinctrl_pm_select_sleep_state(se->dev);
>> +		if (ret)
>> +			return ret;
>> +	}
>>   
>>   	geni_se_clks_off(se);
> 
> Should the clocks be turned off?
> 
Yes, it's required to be turned off.
> Konrad

[PATCH v3 4/4] i2c: i2c-qcom-geni: Enable i2c controller sharing between two subsystems

[Mukesh Kumar Savaliya]

Add support to share I2C SE by two Subsystems in a mutually exclusive way.
Use "qcom,shared-se" flag in a particular i2c instance node if the usecase
requires i2c controller to be shared.

Sharing of SE(Serial engine) is possible only for GSI mode as each
subsystem(SS) can queue transfers over its own GPII Channel. For non GSI
mode, we should force disable this feature even if set by user from DT by
mistake.

I2C driver just need to mark first_msg and last_msg flag to help indicate
GPI driver to take lock and unlock TRE there by protecting from concurrent
access from other EE or Subsystem.

gpi_create_i2c_tre() function at gpi.c will take care of adding Lock and
Unlock TRE for the respective transfer operations.

Since the GPIOs are also shared between two SS, do not unconfigure them
during runtime suspend. This will allow other SS to continue to transfer
the data without any disturbance over the IO lines.

Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 212336f724a6..479fa8e1c33f 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.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.
 
 #include <linux/acpi.h>
 #include <linux/clk.h>
@@ -602,6 +603,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
 	peripheral.clk_div = itr->clk_div;
 	peripheral.set_config = 1;
 	peripheral.multi_msg = false;
+	peripheral.shared_se = gi2c->se.shared_geni_se;
 
 	for (i = 0; i < num; i++) {
 		gi2c->cur = &msgs[i];
@@ -612,6 +614,8 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
 		if (i < num - 1)
 			peripheral.stretch = 1;
 
+		peripheral.first_msg = (i == 0);
+		peripheral.last_msg = (i == num - 1);
 		peripheral.addr = msgs[i].addr;
 
 		ret =  geni_i2c_gpi(gi2c, &msgs[i], &config,
@@ -631,8 +635,11 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
 		dma_async_issue_pending(gi2c->tx_c);
 
 		time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
-		if (!time_left)
+		if (!time_left) {
+			dev_dbg(gi2c->se.dev, "I2C timeout gpi flags:%d addr:0x%x\n",
+						gi2c->cur->flags, gi2c->cur->addr);
 			gi2c->err = -ETIMEDOUT;
+		}
 
 		if (gi2c->err) {
 			ret = gi2c->err;
@@ -800,6 +807,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
 		gi2c->clk_freq_out = KHZ(100);
 	}
 
+	if (of_property_read_bool(pdev->dev.of_node, "qcom,shared-se")) {
+		gi2c->se.shared_geni_se = true;
+		dev_dbg(&pdev->dev, "I2C is shared between subsystems\n");
+	}
+
 	if (has_acpi_companion(dev))
 		ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
 
@@ -870,8 +882,10 @@ static int geni_i2c_probe(struct platform_device *pdev)
 	else
 		fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
 
-	if (fifo_disable) {
-		/* FIFO is disabled, so we can only use GPI DMA */
+	if (fifo_disable || gi2c->se.shared_geni_se) {
+		/* FIFO is disabled, so we can only use GPI DMA.
+		 * SE can be shared in GSI mode between subsystems, each SS owns a GPII.
+		 **/
 		gi2c->gpi_mode = true;
 		ret = setup_gpi_dma(gi2c);
 		if (ret) {
@@ -883,6 +897,9 @@ static int geni_i2c_probe(struct platform_device *pdev)
 		dev_dbg(dev, "Using GPI DMA mode for I2C\n");
 	} else {
 		gi2c->gpi_mode = false;
+
+		/* Force disable shared SE case for non GSI mode */
+		gi2c->se.shared_geni_se = false;
 		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
 
 		/* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
@@ -964,7 +981,6 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
 	if (ret) {
 		enable_irq(gi2c->irq);
 		return ret;
-
 	} else {
 		gi2c->suspended = 1;
 	}
-- 
2.25.1

Re: [PATCH v3 4/4] i2c: i2c-qcom-geni: Enable i2c controller sharing between two subsystems

[Bjorn Andersson]

On Fri, Sep 27, 2024 at 12:01:08PM GMT, Mukesh Kumar Savaliya wrote:
> Add support to share I2C SE by two Subsystems in a mutually exclusive way.
> Use "qcom,shared-se" flag in a particular i2c instance node if the usecase
> requires i2c controller to be shared.
> 

Please start your commit message by describing the problem your patch
is solving.

> Sharing of SE(Serial engine) is possible only for GSI mode as each
> subsystem(SS) can queue transfers over its own GPII Channel. For non GSI
> mode, we should force disable this feature even if set by user from DT by
> mistake.
> 
> I2C driver just need to mark first_msg and last_msg flag to help indicate
> GPI driver to take lock and unlock TRE there by protecting from concurrent
> access from other EE or Subsystem.
> 
> gpi_create_i2c_tre() function at gpi.c will take care of adding Lock and
> Unlock TRE for the respective transfer operations.
> 
> Since the GPIOs are also shared between two SS, do not unconfigure them
> during runtime suspend. This will allow other SS to continue to transfer
> the data without any disturbance over the IO lines.
> 

This last paragraph describes patch 3, right?

> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> ---
>  drivers/i2c/busses/i2c-qcom-geni.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 212336f724a6..479fa8e1c33f 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.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.
>  
>  #include <linux/acpi.h>
>  #include <linux/clk.h>
> @@ -602,6 +603,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>  	peripheral.clk_div = itr->clk_div;
>  	peripheral.set_config = 1;
>  	peripheral.multi_msg = false;
> +	peripheral.shared_se = gi2c->se.shared_geni_se;
>  
>  	for (i = 0; i < num; i++) {
>  		gi2c->cur = &msgs[i];
> @@ -612,6 +614,8 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>  		if (i < num - 1)
>  			peripheral.stretch = 1;
>  
> +		peripheral.first_msg = (i == 0);
> +		peripheral.last_msg = (i == num - 1);

There are multiple error paths in this loop, which would result in us
never issuing the unlock TRE - effectively blocking other subsystems
from accessing the serial engine until we perform our next access
(assuming that APSS issuing a lock TRE when APSS already has the channel
locked isn't a problem?)

>  		peripheral.addr = msgs[i].addr;
>  
>  		ret =  geni_i2c_gpi(gi2c, &msgs[i], &config,
> @@ -631,8 +635,11 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>  		dma_async_issue_pending(gi2c->tx_c);
>  
>  		time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
> -		if (!time_left)
> +		if (!time_left) {
> +			dev_dbg(gi2c->se.dev, "I2C timeout gpi flags:%d addr:0x%x\n",
> +						gi2c->cur->flags, gi2c->cur->addr);

This looks useful, but unrelated to this patch.

>  			gi2c->err = -ETIMEDOUT;
> +		}
>  
>  		if (gi2c->err) {
>  			ret = gi2c->err;
> @@ -800,6 +807,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  		gi2c->clk_freq_out = KHZ(100);
>  	}
>  
> +	if (of_property_read_bool(pdev->dev.of_node, "qcom,shared-se")) {
> +		gi2c->se.shared_geni_se = true;

	gi2c->se.shared_geni_se = of_property_read_bool(dev->of_node, "qcom,shared-se");

> +		dev_dbg(&pdev->dev, "I2C is shared between subsystems\n");
> +	}
> +
>  	if (has_acpi_companion(dev))
>  		ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
>  
> @@ -870,8 +882,10 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  	else
>  		fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
>  
> -	if (fifo_disable) {
> -		/* FIFO is disabled, so we can only use GPI DMA */
> +	if (fifo_disable || gi2c->se.shared_geni_se) {
> +		/* FIFO is disabled, so we can only use GPI DMA.
> +		 * SE can be shared in GSI mode between subsystems, each SS owns a GPII.
> +		 **/

I think you're trying to document why we're entering the "GPI-only"
branch. The addition you made was that if the user has requested
"shared-se", then it's GPI-only.

But I'm not able to wrap my head around your addition here. Why does it
matter that each subsystem own a GPII? Is that a reason for choosing
GPI-only mode?

>  		gi2c->gpi_mode = true;
>  		ret = setup_gpi_dma(gi2c);
>  		if (ret) {
> @@ -883,6 +897,9 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  		dev_dbg(dev, "Using GPI DMA mode for I2C\n");
>  	} else {
>  		gi2c->gpi_mode = false;
> +
> +		/* Force disable shared SE case for non GSI mode */

GSI or GPI mode?

> +		gi2c->se.shared_geni_se = false;

If shared_geni_se was true prior to this assignment, wouldn't we have
entered the if (fifo_disable ...) branch?

>  		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
>  
>  		/* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
> @@ -964,7 +981,6 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
>  	if (ret) {
>  		enable_irq(gi2c->irq);
>  		return ret;
> -

Please avoid such unrelated cleanups.

Regards,
Bjorn

>  	} else {
>  		gi2c->suspended = 1;
>  	}
> -- 
> 2.25.1
> 

Re: [PATCH v3 4/4] i2c: i2c-qcom-geni: Enable i2c controller sharing between two subsystems

[Dan Carpenter]

On Sun, Sep 29, 2024 at 10:46:37PM -0500, Bjorn Andersson wrote:
> > @@ -602,6 +603,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
> >  	peripheral.clk_div = itr->clk_div;
> >  	peripheral.set_config = 1;
> >  	peripheral.multi_msg = false;
> > +	peripheral.shared_se = gi2c->se.shared_geni_se;
> >  
> >  	for (i = 0; i < num; i++) {
> >  		gi2c->cur = &msgs[i];
> > @@ -612,6 +614,8 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
> >  		if (i < num - 1)
> >  			peripheral.stretch = 1;
> >  
> > +		peripheral.first_msg = (i == 0);
> > +		peripheral.last_msg = (i == num - 1);
> 
> There are multiple error paths in this loop, which would result in us
> never issuing the unlock TRE - effectively blocking other subsystems
> from accessing the serial engine until we perform our next access
> (assuming that APSS issuing a lock TRE when APSS already has the channel
> locked isn't a problem?)
> 

Hi Bjorn,

I saw the words "error paths" and "unlock" and I thought there was maybe
something we could do here with static analysis.  But I don't know what TRE
or APSS mean.

The one thing I do see is that this uses "one err" style error handling where
there is one err label and it calls dmaengine_terminate_sync(gi2c->rx_c)
regardless of whether or not geni_i2c_gpi() was called or failed/succeeded.

regards,
dan carpenter

Re: [PATCH v3 4/4] i2c: i2c-qcom-geni: Enable i2c controller sharing between two subsystems

[Bjorn Andersson]

On Mon, Sep 30, 2024 at 11:21:23AM GMT, Dan Carpenter wrote:
> On Sun, Sep 29, 2024 at 10:46:37PM -0500, Bjorn Andersson wrote:
> > > @@ -602,6 +603,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
> > >  	peripheral.clk_div = itr->clk_div;
> > >  	peripheral.set_config = 1;
> > >  	peripheral.multi_msg = false;
> > > +	peripheral.shared_se = gi2c->se.shared_geni_se;
> > >  
> > >  	for (i = 0; i < num; i++) {
> > >  		gi2c->cur = &msgs[i];
> > > @@ -612,6 +614,8 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
> > >  		if (i < num - 1)
> > >  			peripheral.stretch = 1;
> > >  
> > > +		peripheral.first_msg = (i == 0);
> > > +		peripheral.last_msg = (i == num - 1);
> > 
> > There are multiple error paths in this loop, which would result in us
> > never issuing the unlock TRE - effectively blocking other subsystems
> > from accessing the serial engine until we perform our next access
> > (assuming that APSS issuing a lock TRE when APSS already has the channel
> > locked isn't a problem?)
> > 
> 
> Hi Bjorn,
> 
> I saw the words "error paths" and "unlock" and I thought there was maybe
> something we could do here with static analysis.

Appreciate you picking up on those topics :)

> But I don't know what TRE or APSS mean.
> 

The "APSS" is "the application processor sub system", which is where
we typically find Linux running. TRE is, if I understand correctly, a
request on the DMA engine queue.

> The one thing I do see is that this uses "one err" style error handling where
> there is one err label and it calls dmaengine_terminate_sync(gi2c->rx_c)
> regardless of whether or not geni_i2c_gpi() was called or failed/succeeded.
> 

The scheme presented in this series injects a "LOCK" request before the
DMA request marked first_msg, and an "UNLOCK" after the ones marked
last_msg. This is needed because the controller is also concurrently by
some DSP (or similar), so the LOCK/UNLOCK scheme forms mutual exclusion
of the operations between the Linux and DSP systems.

I'm not sure if it's possible to tie the unlock operation to
dmaengine_terminate_sync() in some way.

Giving this some more thought, it feels like the current scheme serves
the purpose of providing mutual exclusion both for the controller and
to some degree for the device. But I'd like to understand why we can't
inject the lock/unlock implicitly for each transfer...

Regards,
Bjorn

> regards,
> dan carpenter
> 

Re: [PATCH v3 4/4] i2c: i2c-qcom-geni: Enable i2c controller sharing between two subsystems

[Mukesh Kumar Savaliya]

Hi Bjorn,

On 10/1/2024 8:09 AM, Bjorn Andersson wrote:
> On Mon, Sep 30, 2024 at 11:21:23AM GMT, Dan Carpenter wrote:
>> On Sun, Sep 29, 2024 at 10:46:37PM -0500, Bjorn Andersson wrote:
>>>> @@ -602,6 +603,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>>>>   	peripheral.clk_div = itr->clk_div;
>>>>   	peripheral.set_config = 1;
>>>>   	peripheral.multi_msg = false;
>>>> +	peripheral.shared_se = gi2c->se.shared_geni_se;
>>>>   
>>>>   	for (i = 0; i < num; i++) {
>>>>   		gi2c->cur = &msgs[i];
>>>> @@ -612,6 +614,8 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>>>>   		if (i < num - 1)
>>>>   			peripheral.stretch = 1;
>>>>   
>>>> +		peripheral.first_msg = (i == 0);
>>>> +		peripheral.last_msg = (i == num - 1);
>>>
>>> There are multiple error paths in this loop, which would result in us
>>> never issuing the unlock TRE - effectively blocking other subsystems
>>> from accessing the serial engine until we perform our next access
>>> (assuming that APSS issuing a lock TRE when APSS already has the channel
>>> locked isn't a problem?)
>>>
>>
>> Hi Bjorn,
>>
>> I saw the words "error paths" and "unlock" and I thought there was maybe
>> something we could do here with static analysis.
> 
> Appreciate you picking up on those topics :)
> 
>> But I don't know what TRE or APSS mean.
>>
> 
> The "APSS" is "the application processor sub system", which is where
> we typically find Linux running. TRE is, if I understand correctly, a
> request on the DMA engine queue.
Yes, Thats right. Transfer ring element, it's like a command to the 
engine. Can be configuration TRE, DMA xfer request TRE etc.
> 
>> The one thing I do see is that this uses "one err" style error handling where
>> there is one err label and it calls dmaengine_terminate_sync(gi2c->rx_c)
>> regardless of whether or not geni_i2c_gpi() was called or failed/succeeded.
>>
> 
> The scheme presented in this series injects a "LOCK" request before the
> DMA request marked first_msg, and an "UNLOCK" after the ones marked
> last_msg. This is needed because the controller is also concurrently by
> some DSP (or similar), so the LOCK/UNLOCK scheme forms mutual exclusion
> of the operations between the Linux and DSP systems.
> 
> I'm not sure if it's possible to tie the unlock operation to
> dmaengine_terminate_sync() in some way.
> 
I think terminate_sync() should clean up all xfers and will continue for 
the next request as a fresh start.
> Giving this some more thought, it feels like the current scheme serves
> the purpose of providing mutual exclusion both for the controller and
> to some degree for the device. But I'd like to understand why we can't
> inject the lock/unlock implicitly for each transfer...
> 
Explicitly adding lock/unlock per transfer adds execution load. Lock is 
meant for taking an ownership of the bus which is better to manage per 
session.
> Regards,
> Bjorn
> 
>> regards,
>> dan carpenter
>>

Re: [PATCH v3 4/4] i2c: i2c-qcom-geni: Enable i2c controller sharing between two subsystems

[Mukesh Kumar Savaliya]

Thanks for the review Bjorn !

On 9/30/2024 9:16 AM, Bjorn Andersson wrote:
> On Fri, Sep 27, 2024 at 12:01:08PM GMT, Mukesh Kumar Savaliya wrote:
>> Add support to share I2C SE by two Subsystems in a mutually exclusive way.
>> Use "qcom,shared-se" flag in a particular i2c instance node if the usecase
>> requires i2c controller to be shared.
>>
> 
> Please start your commit message by describing the problem your patch
> is solving.
> 
Done
>> Sharing of SE(Serial engine) is possible only for GSI mode as each
>> subsystem(SS) can queue transfers over its own GPII Channel. For non GSI
>> mode, we should force disable this feature even if set by user from DT by
>> mistake.
>>
>> I2C driver just need to mark first_msg and last_msg flag to help indicate
>> GPI driver to take lock and unlock TRE there by protecting from concurrent
>> access from other EE or Subsystem.
>>
>> gpi_create_i2c_tre() function at gpi.c will take care of adding Lock and
>> Unlock TRE for the respective transfer operations.
>>
>> Since the GPIOs are also shared between two SS, do not unconfigure them
>> during runtime suspend. This will allow other SS to continue to transfer
>> the data without any disturbance over the IO lines.
>>
> 
> This last paragraph describes patch 3, right?
Yes,  i think i should i keep it in patch 3.
> 
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> ---
>>   drivers/i2c/busses/i2c-qcom-geni.c | 24 ++++++++++++++++++++----
>>   1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 212336f724a6..479fa8e1c33f 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.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.
>>   
>>   #include <linux/acpi.h>
>>   #include <linux/clk.h>
>> @@ -602,6 +603,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>>   	peripheral.clk_div = itr->clk_div;
>>   	peripheral.set_config = 1;
>>   	peripheral.multi_msg = false;
>> +	peripheral.shared_se = gi2c->se.shared_geni_se;
>>   
>>   	for (i = 0; i < num; i++) {
>>   		gi2c->cur = &msgs[i];
>> @@ -612,6 +614,8 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>>   		if (i < num - 1)
>>   			peripheral.stretch = 1;
>>   
>> +		peripheral.first_msg = (i == 0);
>> +		peripheral.last_msg = (i == num - 1);
> 
> There are multiple error paths in this loop, which would result in us
> never issuing the unlock TRE - effectively blocking other subsystems
> from accessing the serial engine until we perform our next access
> (assuming that APSS issuing a lock TRE when APSS already has the channel
> locked isn't a problem?)
> 
>>   		peripheral.addr = msgs[i].addr;
>>   
>>   		ret =  geni_i2c_gpi(gi2c, &msgs[i], &config,
>> @@ -631,8 +635,11 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>>   		dma_async_issue_pending(gi2c->tx_c);
>>   
>>   		time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
>> -		if (!time_left)
>> +		if (!time_left) {
>> +			dev_dbg(gi2c->se.dev, "I2C timeout gpi flags:%d addr:0x%x\n",
>> +						gi2c->cur->flags, gi2c->cur->addr);
> 
> This looks useful, but unrelated to this patch.
Sure, Removed it.
> 
>>   			gi2c->err = -ETIMEDOUT;
>> +		}
>>   
>>   		if (gi2c->err) {
>>   			ret = gi2c->err;
>> @@ -800,6 +807,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
>>   		gi2c->clk_freq_out = KHZ(100);
>>   	}
>>   
>> +	if (of_property_read_bool(pdev->dev.of_node, "qcom,shared-se")) {
>> +		gi2c->se.shared_geni_se = true;
> 
> 	gi2c->se.shared_geni_se = of_property_read_bool(dev->of_node, "qcom,shared-se");
> 
>> +		dev_dbg(&pdev->dev, "I2C is shared between subsystems\n");
>> +	}
>> +
>>   	if (has_acpi_companion(dev))
>>   		ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
>>   
>> @@ -870,8 +882,10 @@ static int geni_i2c_probe(struct platform_device *pdev)
>>   	else
>>   		fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
>>   
>> -	if (fifo_disable) {
>> -		/* FIFO is disabled, so we can only use GPI DMA */
>> +	if (fifo_disable || gi2c->se.shared_geni_se) {
>> +		/* FIFO is disabled, so we can only use GPI DMA.
>> +		 * SE can be shared in GSI mode between subsystems, each SS owns a GPII.
>> +		 **/
> 
> I think you're trying to document why we're entering the "GPI-only"
> branch. The addition you made was that if the user has requested
> "shared-se", then it's GPI-only.
> 
yes, that's right.
> But I'm not able to wrap my head around your addition here. Why does it
> matter that each subsystem own a GPII? Is that a reason for choosing
> GPI-only mode?
Not sure i got your question here.
The feature flag true means it should be in GPI mode.
> 
>>   		gi2c->gpi_mode = true;
>>   		ret = setup_gpi_dma(gi2c);
>>   		if (ret) {
>> @@ -883,6 +897,9 @@ static int geni_i2c_probe(struct platform_device *pdev)
>>   		dev_dbg(dev, "Using GPI DMA mode for I2C\n");
>>   	} else {
>>   		gi2c->gpi_mode = false;
>> +
>> +		/* Force disable shared SE case for non GSI mode */
> 
> GSI or GPI mode?
Changed to GPI.
> 
>> +		gi2c->se.shared_geni_se = false;
> 
> If shared_geni_se was true prior to this assignment, wouldn't we have
> entered the if (fifo_disable ...) branch?
In that case also we should enter the condition of executing in GPI mode.
> 
>>   		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
>>   
>>   		/* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
>> @@ -964,7 +981,6 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
>>   	if (ret) {
>>   		enable_irq(gi2c->irq);
>>   		return ret;
>> -
> 
> Please avoid such unrelated cleanups.
> 
Sure
> Regards,
> Bjorn
> 
>>   	} else {
>>   		gi2c->suspended = 1;
>>   	}
>> -- 
>> 2.25.1
>>

Re: [PATCH v3 4/4] i2c: i2c-qcom-geni: Enable i2c controller sharing between two subsystems

[Bryan O'Donoghue]

On 27/09/2024 07:31, Mukesh Kumar Savaliya wrote:
> Add support to share I2C SE by two Subsystems in a mutually exclusive way.

As I read this the question jumps out "what is a subsystem" - in Linux 
speak subsystem is say a bus or a memory management method but, here 
what you really mean if I've understood the intent of this series is to 
share the serial engine between two different bus-masters or perhaps a 
better description is "system agent".

Please make that delination clear - its not two Linux subsystems but two 
different Qcom SoC bus masters right ?

For example the APSS - Application Specific Sub Subsystem - where Linux 
runs and say cDSP - the compute DSP on qcom SoCs.

I'd rename this patch to make that clear - because "between two 
subsystems" if you aren't intimately versed in qcom's architecture 
suggests that a Linux i2c and spi driver are somehow muxing pins ..

Really this is a type of AMP - asymmetric multi processing.

"i2c: i2c-qcom-geni: Enable i2c controller sharing between two different 
bus masters"

And I'd mention in the commit log specific examples - APSS yes we get 
but what is the other system agent in your use-case ?

A DSP ? Some other processor in the SoC ?

Anyway highlight one use-case for this AMP case, please.

---
bod

Re: [PATCH v3 4/4] i2c: i2c-qcom-geni: Enable i2c controller sharing between two subsystems

[Mukesh Kumar Savaliya]

Thanks Bryan for sharing your comments.

On 10/15/2024 3:06 AM, Bryan O'Donoghue wrote:
> On 27/09/2024 07:31, Mukesh Kumar Savaliya wrote:
>> Add support to share I2C SE by two Subsystems in a mutually exclusive 
>> way.
> 
> As I read this the question jumps out "what is a subsystem" - in Linux 
> speak subsystem is say a bus or a memory management method but, here 
> what you really mean if I've understood the intent of this series is to 
> share the serial engine between two different bus-masters or perhaps a 
> better description is "system agent".
> 
> Please make that delination clear - its not two Linux subsystems but two 
> different Qcom SoC bus masters right ?
> 
It's like between two processor systems.
> For example the APSS - Application Specific Sub Subsystem - where Linux 
> runs and say cDSP - the compute DSP on qcom SoCs.
> 
Yes
> I'd rename this patch to make that clear - because "between two 
> subsystems" if you aren't intimately versed in qcom's architecture 
> suggests that a Linux i2c and spi driver are somehow muxing pins ..
> 
> Really this is a type of AMP - asymmetric multi processing.
> 
> "i2c: i2c-qcom-geni: Enable i2c controller sharing between two different 
> bus masters"
> 
I think bus masters can be within same APPS system too. hence
> And I'd mention in the commit log specific examples - APSS yes we get 
> but what is the other system agent in your use-case ?
> 
> A DSP ? Some other processor in the SoC ?
yes, It's DSP processor here. But can be Low power DSP OR Modem DSP.
> 
> Anyway highlight one use-case for this AMP case, please.
I have added below in cover letter. I should add example in this patch also.
Example :
Two clients from different SS can share an I2C SE for same slave device
OR their owned slave devices.
Assume I2C Slave EEPROM device connected with I2C controller.
Each client from ADSP SS and APPS Linux SS can perform i2c transactions.
This gets serialized by lock TRE + Transfers + Unlock TRE at HW level.
> 
> ---
> bod
> 
> 
> 
>