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

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

[Mukesh Kumar Savaliya]

QUP based I2C GENI controller driver doesn't support controller sharing
between two processors (E.g. APPS and DSP) running same or different OS.

For example, two I2C clients (one from ADSP and another from APPS) want to
communicate with EEPROM connected over I2C without any bus level disturbance
during transfer.

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

Each Processor subsystem must acquire Lock over the controller so that it
gets uninterrupted control till it unlocks the SE. Generally, GPIOs are
being unconfigured during It also makes sure the commonly shared TLMM GPIOs
are not touched which can impact other subsystem or cause any interruption. 
suspend time. Transfer from each processor gets serialized by lock TRE +
Transfers + Unlock TRE at HW level.

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,is-shared” flag only while enabling this feature.
I2C client should add in its respective parent node.

Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
---
Link to V3: https://lore.kernel.org/lkml/20240927063108.2773304-4-quic_msavaliy@quicinc.com/T/

Changes in V4:
 - Fixed Typo to dt-bindings in subject line of PATCH 1.
 - Replaced SS (subsystem) as multiprocessor as per Bryan's suggestions.
 - Replied to Krzysztof's comments and replaced SS with Multiprocessor system.
 - Removed Abbreviations and also bullet point list from  PATCH 1.
 - Changed feature flag name from qcom,shared-se to qcom,is-shared.
 - Removed bullet points from example of usecase and explained in paragraph.
 - Changed title suffix to dmaengine from dma for Patch 2.
 - Rename TRE_I2C_LOCK to TRE_LOCK in PATCH 2.
 - Enhanced comments about not modifying the pin states on shared SE for PATCH 3.
 - Enhanced shared_geni_se struct member explanation as per Bjorn's comment in PATCH 3.
 - Moved GPIO unconfiguration description from patch 4 to patch 3 as pointed by Bjorn.
 - Removed debug log which was unrelated to this feature change.
 - Added usecase exmaple of shared SE in commit log.

---
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
  dmaengine: gpi: Add Lock and Unlock TRE support to access I2C
    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            | 19 ++++++++--
 drivers/soc/qcom/qcom-geni-se.c               | 13 +++++--
 include/linux/dma/qcom-gpi-dma.h              |  6 +++
 include/linux/soc/qcom/geni-se.h              |  3 ++
 6 files changed, 74 insertions(+), 8 deletions(-)

-- 
2.25.1

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

[Mukesh Kumar Savaliya]

Adds qcom,is-shared flag usage. Use this flag when I2C serial controller
needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment.

Two clients from different processors can share an I2C controller 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.

Transfer gets serialized by Lock TRE + DMA xfer + 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..fe36938712f7 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 controller is shared between two or more system processors.
+    type: boolean
+
   reg:
     maxItems: 1
 
-- 
2.25.1

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

[Rob Herring]

On Wed, Nov 13, 2024 at 09:44:10PM +0530, Mukesh Kumar Savaliya wrote:
> Adds qcom,is-shared flag usage. Use this flag when I2C serial controller

Doesn't match the property name.

> needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment.
> 
> Two clients from different processors can share an I2C controller 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.
> 
> Transfer gets serialized by Lock TRE + DMA xfer + 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..fe36938712f7 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:

What is 'se'? Is that defined somewhere?

> +    description: True if I2C controller is shared between two or more system processors.
> +    type: boolean
> +
>    reg:
>      maxItems: 1
>  
> -- 
> 2.25.1
> 

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

[Mukesh Kumar Savaliya]

Thanks Rob for your review and comments !

On 11/15/2024 11:01 PM, Rob Herring wrote:
> On Wed, Nov 13, 2024 at 09:44:10PM +0530, Mukesh Kumar Savaliya wrote:
>> Adds qcom,is-shared flag usage. Use this flag when I2C serial controller
> 
> Doesn't match the property name.
Sure, i need to change the name here as qcom,shared-se, will upload a 
new patch.
> 
>> needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment.
>>
>> Two clients from different processors can share an I2C controller 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.
>>
>> Transfer gets serialized by Lock TRE + DMA xfer + 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..fe36938712f7 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:
> 
> What is 'se'? Is that defined somewhere?
> 
SE is Serial Engine acting as I2C controller. Let me add second line for 
SE here also.

It's mentioned in source code in Patch 3 where it's used.
 >>> True if serial engine is shared between multiprocessors OR 
Execution Environment.

>> +    description: True if I2C controller is shared between two or more system processors.
>> +    type: boolean
>> +
>>     reg:
>>       maxItems: 1
>>   
>> -- 
>> 2.25.1
>>

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

[Krzysztof Kozlowski]

On 17/11/2024 18:45, Mukesh Kumar Savaliya wrote:
> Thanks Rob for your review and comments !
> 
> On 11/15/2024 11:01 PM, Rob Herring wrote:
>> On Wed, Nov 13, 2024 at 09:44:10PM +0530, Mukesh Kumar Savaliya wrote:
>>> Adds qcom,is-shared flag usage. Use this flag when I2C serial controller
>>
>> Doesn't match the property name.
> Sure, i need to change the name here as qcom,shared-se, will upload a 
> new patch.
>>
>>> needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment.
>>>
>>> Two clients from different processors can share an I2C controller 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.
>>>
>>> Transfer gets serialized by Lock TRE + DMA xfer + 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..fe36938712f7 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:
>>
>> What is 'se'? Is that defined somewhere?
>>
> SE is Serial Engine acting as I2C controller. Let me add second line for 
> SE here also.
> 
> It's mentioned in source code in Patch 3 where it's used.
>  >>> True if serial engine is shared between multiprocessors OR 
> Execution Environment.
You already got this comment:
https://lore.kernel.org/lkml/20240927063108.2773304-4-quic_msavaliy@quicinc.com/T/#m79efdd1172631aca99a838b4bfe57943755701e3

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

Further comments asked you to rephrase it. Did anything improve? No,
nothing.

You got comments, you ignore them and send the same.

But most important: I keep repeating this over and over - NAK for some
specific "shared-se" flag, different for each of your IP blocks. Come
with something generic for entire qualcomm. There are few of such flags
already and there are some patches adding it in different flavors.

Get this consistent.

NAK for this and v5 doing exactly theh same.

Best regards,
Krzysztof

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

[Mukesh Kumar Savaliya]

Thanks Rob,  Krzysztof !

On 11/25/2024 1:41 PM, Krzysztof Kozlowski wrote:
> On 17/11/2024 18:45, Mukesh Kumar Savaliya wrote:
>> Thanks Rob for your review and comments !
>>
>> On 11/15/2024 11:01 PM, Rob Herring wrote:
>>> On Wed, Nov 13, 2024 at 09:44:10PM +0530, Mukesh Kumar Savaliya wrote:
>>>> Adds qcom,is-shared flag usage. Use this flag when I2C serial controller
>>>
>>> Doesn't match the property name.
>> Sure, i need to change the name here as qcom,shared-se, will upload a
>> new patch.
>>>
>>>> needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment.
>>>>
>>>> Two clients from different processors can share an I2C controller 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.
>>>>
>>>> Transfer gets serialized by Lock TRE + DMA xfer + 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..fe36938712f7 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:
>>>
>>> What is 'se'? Is that defined somewhere?
>>>
>> SE is Serial Engine acting as I2C controller. Let me add second line for
>> SE here also.
>>
>> It's mentioned in source code in Patch 3 where it's used.
>>   >>> True if serial engine is shared between multiprocessors OR
>> Execution Environment.
> You already got this comment:
> https://lore.kernel.org/lkml/20240927063108.2773304-4-quic_msavaliy@quicinc.com/T/#m79efdd1172631aca99a838b4bfe57943755701e3
> 
> ""se" is also not explained in the binding - please open it and look for
> such explanation."
> 
> Further comments asked you to rephrase it. Did anything improve? No,
> nothing.
> 
> You got comments, you ignore them and send the same.
It's actually changed to is-shared flag and again renamed to shared-se 
based on the review comments. This went for correction for flag naming. 
Sorry for missing SE description into dt-bindings in the latest patch.
I am adding it with more description.
> 
> But most important: I keep repeating this over and over - NAK for some
> specific "shared-se" flag, different for each of your IP blocks. Come
> with something generic for entire qualcomm. There are few of such flags
> already and there are some patches adding it in different flavors.
> 
we do have SE (serial engine) which works for i2c, spi, uart, i3c. And 
SE is single HW entity as you are aware of. But I feel it makes sense to 
keep this flag name per SE and even for SPI OR I3C we should be using 
same flag name in DTSI.
> Get this consistent.
> 
> NAK for this and v5 doing exactly theh same.
> 
Hope i meet expectations considering all your suggestions and past 
learning and not missing anything out of my mind.

> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 29/11/2024 15:43, Mukesh Kumar Savaliya wrote:
>> But most important: I keep repeating this over and over - NAK for some
>> specific "shared-se" flag, different for each of your IP blocks. Come
>> with something generic for entire qualcomm. There are few of such flags
>> already and there are some patches adding it in different flavors.
>>
> we do have SE (serial engine) which works for i2c, spi, uart, i3c. And 
> SE is single HW entity as you are aware of. But I feel it makes sense to 
> keep this flag name per SE and even for SPI OR I3C we should be using 
> same flag name in DTSI.
>> Get this consistent.
>>
>> NAK for this and v5 doing exactly theh same.
>>
> Hope i meet expectations considering all your suggestions and past 
> learning and not missing anything out of my mind.
> 
Nothing from my comment above was resolved. I will NAK the next version
as well for the same reasons.

Best regards,
Krzysztof

[PATCH v4 2/4] dmaengine: gpi: Add Lock and Unlock TRE support to access I2C exclusively

[Mukesh Kumar Savaliya]

GSI DMA provides specific TREs(Transfer ring element) namely Lock and
Unlock TRE. It provides mutually exclusive access to I2C controller from
any of the processor(Apps,ADSP). Lock prevents other subsystems from
concurrently performing DMA transfers and avoids disturbance to data path.
Basically for shared I2C usecase, lock the SE(Serial Engine) for one of
the processor, 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.

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..c9e71c576680 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_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_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 v4 2/4] dmaengine: gpi: Add Lock and Unlock TRE support to access I2C exclusively

[Konrad Dybcio]

On 13.11.2024 5:14 PM, Mukesh Kumar Savaliya wrote:
> GSI DMA provides specific TREs(Transfer ring element) namely Lock and
> Unlock TRE. It provides mutually exclusive access to I2C controller from
> any of the processor(Apps,ADSP). Lock prevents other subsystems from
> concurrently performing DMA transfers and avoids disturbance to data path.
> Basically for shared I2C usecase, lock the SE(Serial Engine) for one of
> the processor, 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.
> 
> 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..c9e71c576680 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_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)

So the lock is generic.. I'd then expect the unlock to be generic, too?

> +
>  /* 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) {

Does the first/last logic handle errors well? i.e. what if we
have >= 3 transfers and:

1) the first transfer succeeds but the last doesn't
2) the first transfer succeeds, the second one doesn't and the lock
   is submitted again
3) the unlock never suceeds

Konrad

Re: [PATCH v4 2/4] dmaengine: gpi: Add Lock and Unlock TRE support to access I2C exclusively

[Mukesh Kumar Savaliya]

Thanks Konrad for the review !

On 11/16/2024 12:53 AM, Konrad Dybcio wrote:
> On 13.11.2024 5:14 PM, Mukesh Kumar Savaliya wrote:
>> GSI DMA provides specific TREs(Transfer ring element) namely Lock and
>> Unlock TRE. It provides mutually exclusive access to I2C controller from
>> any of the processor(Apps,ADSP). Lock prevents other subsystems from
>> concurrently performing DMA transfers and avoids disturbance to data path.
>> Basically for shared I2C usecase, lock the SE(Serial Engine) for one of
>> the processor, 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.
>>
>> 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..c9e71c576680 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_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)
> 
> So the lock is generic.. I'd then expect the unlock to be generic, too?
Absolutely, renamed it for generic as TRE_UNLOCK.
> 
>> +
>>   /* 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) {
> 
> Does the first/last logic handle errors well? i.e. what if we
> have >= 3 transfers and:
> 
> 1) the first transfer succeeds but the last doesn't
> 2) the first transfer succeeds, the second one doesn't and the lock
>     is submitted again
> 3) the unlock never suceeds
> 
geni_i2c_gpi_xfer() takes care of any of the error. Upon error, it does 
dma_engine_terminate_sync() which resets all the pipes. Internal 
downstream also has same implementation.
> Konrad

Re: [PATCH v4 2/4] dmaengine: gpi: Add Lock and Unlock TRE support to access I2C exclusively

[Konrad Dybcio]

On 18.11.2024 6:46 AM, Mukesh Kumar Savaliya wrote:
> Thanks Konrad for the review !
> 
> On 11/16/2024 12:53 AM, Konrad Dybcio wrote:
>> On 13.11.2024 5:14 PM, Mukesh Kumar Savaliya wrote:
>>> GSI DMA provides specific TREs(Transfer ring element) namely Lock and
>>> Unlock TRE. It provides mutually exclusive access to I2C controller from
>>> any of the processor(Apps,ADSP). Lock prevents other subsystems from
>>> concurrently performing DMA transfers and avoids disturbance to data path.
>>> Basically for shared I2C usecase, lock the SE(Serial Engine) for one of
>>> the processor, 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.
>>>
>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>>> ---

[...]

>>>   +    /* create lock tre for first tranfser */
>>> +    if (i2c->shared_se && i2c->first_msg) {
>>
>> Does the first/last logic handle errors well? i.e. what if we
>> have >= 3 transfers and:
>>
>> 1) the first transfer succeeds but the last doesn't
>> 2) the first transfer succeeds, the second one doesn't and the lock
>>     is submitted again
>> 3) the unlock never suceeds
>>
> geni_i2c_gpi_xfer() takes care of any of the error. Upon error, it does dma_engine_terminate_sync() which resets all the pipes. Internal downstream also has same implementation.

Okay, this sounds reassuring.

Since the TRE would be locked to APSS, I'm guessing we don't ever need
to worry about gpi_terminate_all() being executed halfway through a
non-APSS transaction?

Konrad

Re: [PATCH v4 2/4] dmaengine: gpi: Add Lock and Unlock TRE support to access I2C exclusively

[Mukesh Kumar Savaliya]

Hi Konrad, Thanks !

On 11/22/2024 7:10 PM, Konrad Dybcio wrote:
> On 18.11.2024 6:46 AM, Mukesh Kumar Savaliya wrote:
>> Thanks Konrad for the review !
>>
>> On 11/16/2024 12:53 AM, Konrad Dybcio wrote:
>>> On 13.11.2024 5:14 PM, Mukesh Kumar Savaliya wrote:
>>>> GSI DMA provides specific TREs(Transfer ring element) namely Lock and
>>>> Unlock TRE. It provides mutually exclusive access to I2C controller from
>>>> any of the processor(Apps,ADSP). Lock prevents other subsystems from
>>>> concurrently performing DMA transfers and avoids disturbance to data path.
>>>> Basically for shared I2C usecase, lock the SE(Serial Engine) for one of
>>>> the processor, 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.
>>>>
>>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>>>> ---
> 
> [...]
> 
>>>>    +    /* create lock tre for first tranfser */
>>>> +    if (i2c->shared_se && i2c->first_msg) {
>>>
>>> Does the first/last logic handle errors well? i.e. what if we
>>> have >= 3 transfers and:
>>>
>>> 1) the first transfer succeeds but the last doesn't
>>> 2) the first transfer succeeds, the second one doesn't and the lock
>>>      is submitted again
>>> 3) the unlock never suceeds
>>>
>> geni_i2c_gpi_xfer() takes care of any of the error. Upon error, it does dma_engine_terminate_sync() which resets all the pipes. Internal downstream also has same implementation.
> 
> Okay, this sounds reassuring.
> 
> Since the TRE would be locked to APSS, I'm guessing we don't ever need
> to worry about gpi_terminate_all() being executed halfway through a
> non-APSS transaction?
> 
Right.
> Konrad

[PATCH v4 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.
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/soc/qcom/qcom-geni-se.c  | 13 +++++++++----
 include/linux/soc/qcom/geni-se.h |  3 +++
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 4cb959106efa..2116593c4d3b 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,14 @@ 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;
+	/* Don't alter pin states on shared SEs to avoid potentially
+	 * interrupting transfers started by other subsystems.
+	 */
+	if (!se->shared_geni_se) {
+		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 2996a3c28ef3..f330588873c1 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:	True if SE is shared between multiprocessors.
  */
 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

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

[Mukesh Kumar Savaliya]

Add support to share I2C controller in multiprocessor system 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 I2C SE(Serial engine) is possible only for GSI mode as client
from each processor 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.

For example, Assume an I2C EEPROM device connected with an I2C controller.
Each client from ADSP and APPS processor can perform i2c transactions
without any disturbance from each other.

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

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 7a22e1f46e60..4bc5a5ea47f7 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>
@@ -617,6 +618,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];
@@ -627,6 +629,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,
@@ -815,6 +819,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));
 
@@ -887,8 +896,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) {
@@ -900,6 +911,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 */
@@ -981,7 +995,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 v4 4/4] i2c: i2c-qcom-geni: Enable i2c controller sharing between two subsystems

[Konrad Dybcio]

On 13.11.2024 5:14 PM, Mukesh Kumar Savaliya wrote:
> Add support to share I2C controller in multiprocessor system 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.

Can we read back some value from the registers to know whether such sharing
takes place?

> Sharing of I2C SE(Serial engine) is possible only for GSI mode as client
> from each processor 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.

The DT is to be taken authoritatively

> 
> 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.
> 
> For example, Assume an I2C EEPROM device connected with an I2C controller.
> Each client from ADSP and APPS processor can perform i2c transactions
> without any disturbance from each other.
> 
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> ---

[...]

>  	} else {
>  		gi2c->gpi_mode = false;
> +
> +		/* Force disable shared SE case for non GSI mode */
> +		gi2c->se.shared_geni_se = false;

Doing this silently sounds rather odd..

Konrad

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

[Mukesh Kumar Savaliya]

Thanks for the review konrad !

On 11/16/2024 12:58 AM, Konrad Dybcio wrote:
> On 13.11.2024 5:14 PM, Mukesh Kumar Savaliya wrote:
>> Add support to share I2C controller in multiprocessor system 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.
> 
> Can we read back some value from the registers to know whether such sharing
> takes place?
Actually, HW register doesn't provide such mechanism, it's add on 
feature if SE is programmed for GSI mode.
> 
>> Sharing of I2C SE(Serial engine) is possible only for GSI mode as client
>> from each processor 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.
> 
> The DT is to be taken authoritatively
> 
To clarify - Does it mean i should not have SW disable this feature OR 
override  ? And let it continue in non GSI mode even it's not going to 
work ?
>>
>> 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.
>>
>> For example, Assume an I2C EEPROM device connected with an I2C controller.
>> Each client from ADSP and APPS processor can perform i2c transactions
>> without any disturbance from each other.
>>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> ---
> 
> [...]
> 
>>   	} else {
>>   		gi2c->gpi_mode = false;
>> +
>> +		/* Force disable shared SE case for non GSI mode */
>> +		gi2c->se.shared_geni_se = false;
> 
> Doing this silently sounds rather odd..
we can have Some SW logging added ?
> 
> Konrad

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

[Konrad Dybcio]

On 18.11.2024 6:45 AM, Mukesh Kumar Savaliya wrote:
> Thanks for the review konrad !
> 
> On 11/16/2024 12:58 AM, Konrad Dybcio wrote:
>> On 13.11.2024 5:14 PM, Mukesh Kumar Savaliya wrote:
>>> Add support to share I2C controller in multiprocessor system 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.
>>
>> Can we read back some value from the registers to know whether such sharing
>> takes place?
> Actually, HW register doesn't provide such mechanism, it's add on feature if SE is programmed for GSI mode.

So it's more of an unwritten contract between subsystems.. okay

>>
>>> Sharing of I2C SE(Serial engine) is possible only for GSI mode as client
>>> from each processor 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.
>>
>> The DT is to be taken authoritatively
>>
> To clarify - Does it mean i should not have SW disable this feature OR override  ? And let it continue in non GSI mode even it's not going to work ?

If a configuration is invalid, you should return -EINVAL from probe,
with an appropriate error message.

>>>
>>> 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.
>>>
>>> For example, Assume an I2C EEPROM device connected with an I2C controller.
>>> Each client from ADSP and APPS processor can perform i2c transactions
>>> without any disturbance from each other.
>>>
>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>>> ---
>>
>> [...]
>>
>>>       } else {
>>>           gi2c->gpi_mode = false;
>>> +
>>> +        /* Force disable shared SE case for non GSI mode */
>>> +        gi2c->se.shared_geni_se = false;
>>
>> Doing this silently sounds rather odd..
> we can have Some SW logging added ?

Normally such overrides mandate a warning/notice, but as I said above,
we should disallow such combinations altogether for sanity

Konrad

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

[Mukesh Kumar Savaliya]

Thanks Konrad !

On 11/22/2024 7:12 PM, Konrad Dybcio wrote:
> On 18.11.2024 6:45 AM, Mukesh Kumar Savaliya wrote:
>> Thanks for the review konrad !
>>
>> On 11/16/2024 12:58 AM, Konrad Dybcio wrote:
>>> On 13.11.2024 5:14 PM, Mukesh Kumar Savaliya wrote:
>>>> Add support to share I2C controller in multiprocessor system 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.
>>>
>>> Can we read back some value from the registers to know whether such sharing
>>> takes place?
>> Actually, HW register doesn't provide such mechanism, it's add on feature if SE is programmed for GSI mode.
> 
> So it's more of an unwritten contract between subsystems.. okay
> 
yes, purely SW flag based decision.
>>>
>>>> Sharing of I2C SE(Serial engine) is possible only for GSI mode as client
>>>> from each processor 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.
>>>
>>> The DT is to be taken authoritatively
>>>
>> To clarify - Does it mean i should not have SW disable this feature OR override  ? And let it continue in non GSI mode even it's not going to work ?
> 
> If a configuration is invalid, you should return -EINVAL from probe,
> with an appropriate error message.
> 
Sure, i agree. I will make a change here and will bail out with an error 
and print logs  if (gi2c->se.shared_geni_se == true). Thanks for this 
suggestion.

>>>>
>>>> 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.
>>>>
>>>> For example, Assume an I2C EEPROM device connected with an I2C controller.
>>>> Each client from ADSP and APPS processor can perform i2c transactions
>>>> without any disturbance from each other.
>>>>
>>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>>>> ---
>>>
>>> [...]
>>>
>>>>        } else {
>>>>            gi2c->gpi_mode = false;
>>>> +
>>>> +        /* Force disable shared SE case for non GSI mode */
>>>> +        gi2c->se.shared_geni_se = false;
>>>
>>> Doing this silently sounds rather odd..
>> we can have Some SW logging added ?
> 
> Normally such overrides mandate a warning/notice, but as I said above,
> we should disallow such combinations altogether for sanity
> 
Sure, taken cared with above suggestion.
> Konrad