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

[PATCH v2 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.
Each subsystem should have its own dedicated GPII (General Purpose -
Interface Instance) acting as pipe between SE and GSI (Generic SW -
Interface) DMA HW engine.

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 + DMA Transfers + Unlock TRE at HW level.


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

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: Export function geni_se_clks_off()
  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            | 29 +++++++++++----
 drivers/soc/qcom/qcom-geni-se.c               |  4 +-
 include/linux/dma/qcom-gpi-dma.h              |  6 +++
 include/linux/soc/qcom/geni-se.h              |  3 ++
 6 files changed, 74 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH v2 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.

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..ae423127f736 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.
+    type: boolean
+
   reg:
     maxItems: 1
 
-- 
2.25.1

[PATCH v2 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 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 = Tranfser Ring Element, refers to the queued descriptor.

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 e6ebd688d746..ba11b2641ab6 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

[PATCH v2 3/4] soc: qcom: geni-se: Export function geni_se_clks_off()

[Mukesh Kumar Savaliya]

Currently the driver provides a function called geni_serial_resources_off()
to turn off resources like clocks and  pinctrl. We don't have a function to
control clocks separately hence, export the function geni_se_clks_off() to
turn off clocks separately without disturbing GPIO.

Client drivers like I2C require this function for use-cases where the I2C
SE is shared between two subsystems.

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

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 2e8f24d5da80..20166c8fc919 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__
@@ -482,13 +483,14 @@ void geni_se_config_packing(struct geni_se *se, int bpw, int pack_words,
 }
 EXPORT_SYMBOL_GPL(geni_se_config_packing);
 
-static void geni_se_clks_off(struct geni_se *se)
+void geni_se_clks_off(struct geni_se *se)
 {
 	struct geni_wrapper *wrapper = se->wrapper;
 
 	clk_disable_unprepare(se->clk);
 	clk_bulk_disable_unprepare(wrapper->num_clks, wrapper->clks);
 }
+EXPORT_SYMBOL_GPL(geni_se_clks_off);
 
 /**
  * geni_se_resources_off() - Turn off resources associated with the serial
diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
index 0f038a1a0330..caf2c0c4505b 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
@@ -494,6 +495,8 @@ int geni_se_resources_off(struct geni_se *se);
 
 int geni_se_resources_on(struct geni_se *se);
 
+void geni_se_clks_off(struct geni_se *se);
+
 int geni_se_clk_tbl_get(struct geni_se *se, unsigned long **tbl);
 
 int geni_se_clk_freq_match(struct geni_se *se, unsigned long req_freq,
-- 
2.25.1

[PATCH v2 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.

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 for the i2c bus between two SS, do not
touch GPIO configuration during runtime suspend and only turn off the
clocks. 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 | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index eebb0cbb6ca4..ee2e431601a6 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>
@@ -99,6 +100,7 @@ struct geni_i2c_dev {
 	struct dma_chan *rx_c;
 	bool gpi_mode;
 	bool abort_done;
+	bool is_shared;
 };
 
 struct geni_i2c_desc {
@@ -602,6 +604,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->is_shared;
 
 	for (i = 0; i < num; i++) {
 		gi2c->cur = &msgs[i];
@@ -612,6 +615,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 +636,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_err(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 +808,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->is_shared = true;
+		dev_dbg(&pdev->dev, "Shared SE Usecase\n");
+	}
+
 	if (has_acpi_companion(dev))
 		ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
 
@@ -962,14 +975,16 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
 	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
 
 	disable_irq(gi2c->irq);
-	ret = geni_se_resources_off(&gi2c->se);
-	if (ret) {
-		enable_irq(gi2c->irq);
-		return ret;
-
+	if (gi2c->is_shared) {
+		geni_se_clks_off(&gi2c->se);
 	} else {
-		gi2c->suspended = 1;
+		ret = geni_se_resources_off(&gi2c->se);
+		if (ret) {
+			enable_irq(gi2c->irq);
+			return ret;
+		}
 	}
+	gi2c->suspended = 1;
 
 	clk_disable_unprepare(gi2c->core_clk);
 
-- 
2.25.1

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

[Andi Shyti]

Hi Mukesh,

On Sat, Sep 07, 2024 at 12:44:38AM 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.
> 
> 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 for the i2c bus between two SS, do not
> touch GPIO configuration during runtime suspend and only turn off the
> clocks. This will allow other SS to continue to transfer the data
> without any disturbance over the IO lines.

if I remember correctly, someone already commented on your
patches to explain and expand the achronyms you are using. Please
improve the commit log so that people who don't know this device
can understand.

...

> @@ -631,8 +636,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_err(gi2c->se.dev, "I2C timeout gpi flags:%d addr:0x%x\n",
> +						gi2c->cur->flags, gi2c->cur->addr);

Please, don't print out here. The user doesn't really need to
know, let the upper levels decide what to do.

>  			gi2c->err = -ETIMEDOUT;
> +		}
>  
>  		if (gi2c->err) {
>  			ret = gi2c->err;
> @@ -800,6 +808,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->is_shared = true;
> +		dev_dbg(&pdev->dev, "Shared SE Usecase\n");

Please, improve this debug message.

The rest looks good to me.

Thanks,
Andi

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

[Krzysztof Kozlowski]

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

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC (and consider --no-git-fallback argument). It might
happen, that command when run on an older kernel, gives you outdated
entries. Therefore please be sure you base your patches on recent Linux
kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.
</form letter>

You already got this comment, so how many times it has to be repeated?
Your process is just wrong if you do not use the tools for this.
	

> 
> SE = Serial Engine, meant for I2C controller here.
> TRE = Transfer Ring Element, refers to Queued Descriptor.
> 
> Example :
> Two clients from different SS can share an I2C SE for same slave device

What is SS?

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

What is a subsystem? With commit msg I still do not understand this.
Maybe presence of hwlock defines it anyway, so this is redundant?

Best regards,
Krzysztof

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

[neil.armstrong]

Hi,

On 06/09/2024 21:14, 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.
> 
> 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 for the i2c bus between two SS, do not
> touch GPIO configuration during runtime suspend and only turn off the
> clocks. This will allow other SS to continue to transfer the data
> without any disturbance over the IO lines.

This doesn't answer my question about what would be the behavior if one
use uses, for example, GPI DMA, and the Linux kernel FIFO mode or SE DMA ?

Because it seems to "fix" only the GPI DMA shared case.

Neil

> 
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> ---
>   drivers/i2c/busses/i2c-qcom-geni.c | 29 ++++++++++++++++++++++-------
>   1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index eebb0cbb6ca4..ee2e431601a6 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>
> @@ -99,6 +100,7 @@ struct geni_i2c_dev {
>   	struct dma_chan *rx_c;
>   	bool gpi_mode;
>   	bool abort_done;
> +	bool is_shared;
>   };
>   
>   struct geni_i2c_desc {
> @@ -602,6 +604,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->is_shared;
>   
>   	for (i = 0; i < num; i++) {
>   		gi2c->cur = &msgs[i];
> @@ -612,6 +615,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 +636,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_err(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 +808,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->is_shared = true;
> +		dev_dbg(&pdev->dev, "Shared SE Usecase\n");
> +	}
> +
>   	if (has_acpi_companion(dev))
>   		ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
>   
> @@ -962,14 +975,16 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
>   	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>   
>   	disable_irq(gi2c->irq);
> -	ret = geni_se_resources_off(&gi2c->se);
> -	if (ret) {
> -		enable_irq(gi2c->irq);
> -		return ret;
> -
> +	if (gi2c->is_shared) {
> +		geni_se_clks_off(&gi2c->se);
>   	} else {
> -		gi2c->suspended = 1;
> +		ret = geni_se_resources_off(&gi2c->se);
> +		if (ret) {
> +			enable_irq(gi2c->irq);
> +			return ret;
> +		}
>   	}
> +	gi2c->suspended = 1;
>   
>   	clk_disable_unprepare(gi2c->core_clk);
>   

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

[Mukesh Kumar Savaliya]

Hi Neil,

On 9/9/2024 2:24 PM, neil.armstrong@linaro.org wrote:
> Hi,
> 
> On 06/09/2024 21:14, 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.
>>
>> 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 for the i2c bus between two SS, do not
>> touch GPIO configuration during runtime suspend and only turn off the
>> clocks. This will allow other SS to continue to transfer the data
>> without any disturbance over the IO lines.
> 
> This doesn't answer my question about what would be the behavior if one
> use uses, for example, GPI DMA, and the Linux kernel FIFO mode or SE DMA ?
> 
Shared usecase is not supported for non GSI mode (FIFO and DMA), it 
should be static usecase. Dynamic sharing from two clients of two 
subsystems is only for GSI mode. Hope this helps ?
> Because it seems to "fix" only the GPI DMA shared case.
> 
> Neil
> 
>>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> ---
>>   drivers/i2c/busses/i2c-qcom-geni.c | 29 ++++++++++++++++++++++-------
>>   1 file changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c 
>> b/drivers/i2c/busses/i2c-qcom-geni.c
>> index eebb0cbb6ca4..ee2e431601a6 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>
>> @@ -99,6 +100,7 @@ struct geni_i2c_dev {
>>       struct dma_chan *rx_c;
>>       bool gpi_mode;
>>       bool abort_done;
>> +    bool is_shared;
>>   };
>>   struct geni_i2c_desc {
>> @@ -602,6 +604,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->is_shared;
>>       for (i = 0; i < num; i++) {
>>           gi2c->cur = &msgs[i];
>> @@ -612,6 +615,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 +636,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_err(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 +808,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->is_shared = true;
>> +        dev_dbg(&pdev->dev, "Shared SE Usecase\n");
>> +    }
>> +
>>       if (has_acpi_companion(dev))
>>           ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
>> @@ -962,14 +975,16 @@ static int __maybe_unused 
>> geni_i2c_runtime_suspend(struct device *dev)
>>       struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>>       disable_irq(gi2c->irq);
>> -    ret = geni_se_resources_off(&gi2c->se);
>> -    if (ret) {
>> -        enable_irq(gi2c->irq);
>> -        return ret;
>> -
>> +    if (gi2c->is_shared) {
>> +        geni_se_clks_off(&gi2c->se);
>>       } else {
>> -        gi2c->suspended = 1;
>> +        ret = geni_se_resources_off(&gi2c->se);
>> +        if (ret) {
>> +            enable_irq(gi2c->irq);
>> +            return ret;
>> +        }
>>       }
>> +    gi2c->suspended = 1;
>>       clk_disable_unprepare(gi2c->core_clk);
> 
> 

Re: [PATCH v2 3/4] soc: qcom: geni-se: Export function geni_se_clks_off()

[Konrad Dybcio]

On 6.09.2024 9:14 PM, Mukesh Kumar Savaliya wrote:
> Currently the driver provides a function called geni_serial_resources_off()
> to turn off resources like clocks and  pinctrl. We don't have a function to
> control clocks separately hence, export the function geni_se_clks_off() to
> turn off clocks separately without disturbing GPIO.
> 
> Client drivers like I2C require this function for use-cases where the I2C
> SE is shared between two subsystems.
> 
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> ---

Well, i2c is probably not the only type of client you'll want
to share and the current approach requires changes in all protocol
drivers.

How about adding a parameter like `bool shared_se` to
geni_se_resources_off() and changing the pinctrl state conditionally?

Konrad

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

[Konrad Dybcio]

On 9.09.2024 11:18 AM, Mukesh Kumar Savaliya wrote:
> Hi Neil,
> 
> On 9/9/2024 2:24 PM, neil.armstrong@linaro.org wrote:
>> Hi,
>>
>> On 06/09/2024 21:14, 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.
>>>
>>> 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 for the i2c bus between two SS, do not
>>> touch GPIO configuration during runtime suspend and only turn off the
>>> clocks. This will allow other SS to continue to transfer the data
>>> without any disturbance over the IO lines.
>>
>> This doesn't answer my question about what would be the behavior if one
>> use uses, for example, GPI DMA, and the Linux kernel FIFO mode or SE DMA ?
>>
> Shared usecase is not supported for non GSI mode (FIFO and DMA), it should be static usecase. Dynamic sharing from two clients of two subsystems is only for GSI mode. Hope this helps ?

This should very much be explained in commit message and perhaps in code

And since it can't work with FIFO mode, there should be checks in code
to disallow such invalid configurations

Konrad

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

[Andi Shyti]

Thank you guys for your reviews,

On Mon, Sep 09, 2024 at 01:37:00PM GMT, Konrad Dybcio wrote:
> On 9.09.2024 11:18 AM, Mukesh Kumar Savaliya wrote:
> > Hi Neil,
> > 
> > On 9/9/2024 2:24 PM, neil.armstrong@linaro.org wrote:
> >> Hi,
> >>
> >> On 06/09/2024 21:14, 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.
> >>>
> >>> 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 for the i2c bus between two SS, do not
> >>> touch GPIO configuration during runtime suspend and only turn off the
> >>> clocks. This will allow other SS to continue to transfer the data
> >>> without any disturbance over the IO lines.
> >>
> >> This doesn't answer my question about what would be the behavior if one
> >> use uses, for example, GPI DMA, and the Linux kernel FIFO mode or SE DMA ?
> >>
> > Shared usecase is not supported for non GSI mode (FIFO and DMA), it should be static usecase. Dynamic sharing from two clients of two subsystems is only for GSI mode. Hope this helps ?
> 
> This should very much be explained in commit message and perhaps in code
> 
> And since it can't work with FIFO mode, there should be checks in code
> to disallow such invalid configurations

it would be nice if, along with all these open questions and
clarifications on the commit message, we could add some good
comments to the code as well.

Thanks,
Andi

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

[neil.armstrong]

On 09/09/2024 11:18, Mukesh Kumar Savaliya wrote:
> Hi Neil,
> 
> On 9/9/2024 2:24 PM, neil.armstrong@linaro.org wrote:
>> Hi,
>>
>> On 06/09/2024 21:14, 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.
>>>
>>> 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 for the i2c bus between two SS, do not
>>> touch GPIO configuration during runtime suspend and only turn off the
>>> clocks. This will allow other SS to continue to transfer the data
>>> without any disturbance over the IO lines.
>>
>> This doesn't answer my question about what would be the behavior if one
>> use uses, for example, GPI DMA, and the Linux kernel FIFO mode or SE DMA ?
>>
> Shared usecase is not supported for non GSI mode (FIFO and DMA), it should be static usecase. Dynamic sharing from two clients of two subsystems is only for GSI mode. Hope this helps ?

Sure, this is why I proposed on v1 cover letter reply to add:
==============><=====================================================================
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index ee2e431601a6..a15825ea56de 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -885,7 +885,7 @@ 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) {
+       if (gi2c->is_shared || fifo_disable) {
                  /* FIFO is disabled, so we can only use GPI DMA */
                  gi2c->gpi_mode = true;
                  ret = setup_gpi_dma(gi2c);
==============><=====================================================================

Thanks,
Neil

>> Because it seems to "fix" only the GPI DMA shared case.
>>
>> Neil
>>
>>>
>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>>> ---
>>>   drivers/i2c/busses/i2c-qcom-geni.c | 29 ++++++++++++++++++++++-------
>>>   1 file changed, 22 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>>> index eebb0cbb6ca4..ee2e431601a6 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>
>>> @@ -99,6 +100,7 @@ struct geni_i2c_dev {
>>>       struct dma_chan *rx_c;
>>>       bool gpi_mode;
>>>       bool abort_done;
>>> +    bool is_shared;
>>>   };
>>>   struct geni_i2c_desc {
>>> @@ -602,6 +604,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->is_shared;
>>>       for (i = 0; i < num; i++) {
>>>           gi2c->cur = &msgs[i];
>>> @@ -612,6 +615,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 +636,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_err(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 +808,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->is_shared = true;
>>> +        dev_dbg(&pdev->dev, "Shared SE Usecase\n");
>>> +    }
>>> +
>>>       if (has_acpi_companion(dev))
>>>           ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
>>> @@ -962,14 +975,16 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
>>>       struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>>>       disable_irq(gi2c->irq);
>>> -    ret = geni_se_resources_off(&gi2c->se);
>>> -    if (ret) {
>>> -        enable_irq(gi2c->irq);
>>> -        return ret;
>>> -
>>> +    if (gi2c->is_shared) {
>>> +        geni_se_clks_off(&gi2c->se);
>>>       } else {
>>> -        gi2c->suspended = 1;
>>> +        ret = geni_se_resources_off(&gi2c->se);
>>> +        if (ret) {
>>> +            enable_irq(gi2c->irq);
>>> +            return ret;
>>> +        }
>>>       }
>>> +    gi2c->suspended = 1;
>>>       clk_disable_unprepare(gi2c->core_clk);
>>
>>

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

[Mukesh Kumar Savaliya]

Thanks Krzysztof.

On 9/7/2024 2:34 PM, Krzysztof Kozlowski wrote:
> On 06/09/2024 21:14, Mukesh Kumar Savaliya wrote:
>> Adds qcom,shared-se flag usage. Use this when particular I2C serial
>> controller needs to be shared between two subsystems.
> 
> <form letter>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC (and consider --no-git-fallback argument). It might
> happen, that command when run on an older kernel, gives you outdated
> entries. Therefore please be sure you base your patches on recent Linux
> kernel.
> 
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline) or work on fork of kernel
> (don't, instead use mainline). Just use b4 and everything should be
> fine, although remember about `b4 prep --auto-to-cc` if you added new
> patches to the patchset.
> </form letter>
> 
> You already got this comment, so how many times it has to be repeated?
> Your process is just wrong if you do not use the tools for this.
> 
Sorry, I was already using scripts/get_maintainer.pl but i kept everyone 
into To list (That's my mistake here). I shall keep maintainers in TO 
list and rest in CC list.

Question: With <Form Letter> , are you asking to add letter in this 
first patch ? I have cover letter, but it will get removed when patch 
gets merged. Please help suggest and clarify.
> 
>>
>> SE = Serial Engine, meant for I2C controller here.
>> TRE = Transfer Ring Element, refers to Queued Descriptor.
>>
>> Example :
>> Two clients from different SS can share an I2C SE for same slave device
> 
> What is SS?
> 
SS = Subsystem (EE - Execution Environment, can be Apps 
processor/TZ/Modem/ADSP etc). Let me add this too in next patch.
>> 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..ae423127f736 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.
> 
> What is a subsystem? With commit msg I still do not understand this.
SS = Subsystem (EE - Execution Environment, can be Apps 
processor/TZ/Modem/ADSP etc). Let me add EE too with full form.
> Maybe presence of hwlock defines it anyway, so this is redundant?
No, this flag is required. As hwlock comes into picture if this flag is 
defined. So flag is acting as a condition to take hwlock TRE 
descriptor(transfer ring element). Hope i could answer your query.
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 3/4] soc: qcom: geni-se: Export function geni_se_clks_off()

[Mukesh Kumar Savaliya]

Hi Konrad, Thanks !

On 9/9/2024 5:05 PM, Konrad Dybcio wrote:
> On 6.09.2024 9:14 PM, Mukesh Kumar Savaliya wrote:
>> Currently the driver provides a function called geni_serial_resources_off()
>> to turn off resources like clocks and  pinctrl. We don't have a function to
>> control clocks separately hence, export the function geni_se_clks_off() to
>> turn off clocks separately without disturbing GPIO.
>>
>> Client drivers like I2C require this function for use-cases where the I2C
>> SE is shared between two subsystems.
>>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> ---
> 
> Well, i2c is probably not the only type of client you'll want
> to share and the current approach requires changes in all protocol
> drivers.
> 
That's true, it may require for other drivers too in future.
But logically seems more aligning to be handle at client driver side.
Meaning if it's required by SPI  client like touch, they can add this 
flag into touch client DT node.
> How about adding a parameter like `bool shared_se` to
> geni_se_resources_off() and changing the pinctrl state conditionally?
> 
Sure, Looks good design. This needs a change in common struct geni_se 
which can add this flag and set to true from i2c driver struct 
geni_i2c_dev->se->shared_se.
Then geni_se_resources_off() can bypass pinctrl state based on this flag.
if this is aligning, please confirm so can make the change in V3.
> Konrad

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

[Mukesh Kumar Savaliya]

Hi Neil,

On 9/9/2024 2:24 PM, neil.armstrong@linaro.org wrote:
> Hi,
> 
> On 06/09/2024 21:14, 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.
>>
>> 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 for the i2c bus between two SS, do not
>> touch GPIO configuration during runtime suspend and only turn off the
>> clocks. This will allow other SS to continue to transfer the data
>> without any disturbance over the IO lines.
> 
> This doesn't answer my question about what would be the behavior if one
> use uses, for example, GPI DMA, and the Linux kernel FIFO mode or SE DMA ?
> 
> Because it seems to "fix" only the GPI DMA shared case.
> 
This shared usecase is possible only GPI DMA mode, not for the FIFO OR 
SE DMA mode. Let me add conditional mask with GPI DMA mode only. Hope 
that would clarify ?
> Neil
> 
>>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> ---
>>   drivers/i2c/busses/i2c-qcom-geni.c | 29 ++++++++++++++++++++++-------
>>   1 file changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c 
>> b/drivers/i2c/busses/i2c-qcom-geni.c
>> index eebb0cbb6ca4..ee2e431601a6 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>
>> @@ -99,6 +100,7 @@ struct geni_i2c_dev {
>>       struct dma_chan *rx_c;
>>       bool gpi_mode;
>>       bool abort_done;
>> +    bool is_shared;
>>   };
>>   struct geni_i2c_desc {
>> @@ -602,6 +604,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->is_shared;
>>       for (i = 0; i < num; i++) {
>>           gi2c->cur = &msgs[i];
>> @@ -612,6 +615,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 +636,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_err(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 +808,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->is_shared = true;
>> +        dev_dbg(&pdev->dev, "Shared SE Usecase\n");
>> +    }
>> +
>>       if (has_acpi_companion(dev))
>>           ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
>> @@ -962,14 +975,16 @@ static int __maybe_unused 
>> geni_i2c_runtime_suspend(struct device *dev)
>>       struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>>       disable_irq(gi2c->irq);
>> -    ret = geni_se_resources_off(&gi2c->se);
>> -    if (ret) {
>> -        enable_irq(gi2c->irq);
>> -        return ret;
>> -
>> +    if (gi2c->is_shared) {
>> +        geni_se_clks_off(&gi2c->se);
>>       } else {
>> -        gi2c->suspended = 1;
>> +        ret = geni_se_resources_off(&gi2c->se);
>> +        if (ret) {
>> +            enable_irq(gi2c->irq);
>> +            return ret;
>> +        }
>>       }
>> +    gi2c->suspended = 1;
>>       clk_disable_unprepare(gi2c->core_clk);
> 
> 

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

[Mukesh Kumar Savaliya]

Thanks Konrad !

On 9/9/2024 5:07 PM, Konrad Dybcio wrote:
> On 9.09.2024 11:18 AM, Mukesh Kumar Savaliya wrote:
>> Hi Neil,
>>
>> On 9/9/2024 2:24 PM, neil.armstrong@linaro.org wrote:
>>> Hi,
>>>
>>> On 06/09/2024 21:14, 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.
>>>>
>>>> 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 for the i2c bus between two SS, do not
>>>> touch GPIO configuration during runtime suspend and only turn off the
>>>> clocks. This will allow other SS to continue to transfer the data
>>>> without any disturbance over the IO lines.
>>>
>>> This doesn't answer my question about what would be the behavior if one
>>> use uses, for example, GPI DMA, and the Linux kernel FIFO mode or SE DMA ?
>>>
>> Shared usecase is not supported for non GSI mode (FIFO and DMA), it should be static usecase. Dynamic sharing from two clients of two subsystems is only for GSI mode. Hope this helps ?
> 
> This should very much be explained in commit message and perhaps in code
> 
Noted,  will add in commit message and cover letter.
> And since it can't work with FIFO mode, there should be checks in code
> to disallow such invalid configurations
> 
Agree, will add in next patch.
> Konrad

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

[Mukesh Kumar Savaliya]

Hi Neil,

On 9/9/2024 6:34 PM, neil.armstrong@linaro.org wrote:
> On 09/09/2024 11:18, Mukesh Kumar Savaliya wrote:
>> Hi Neil,
>>
>> On 9/9/2024 2:24 PM, neil.armstrong@linaro.org wrote:
>>> Hi,
>>>
>>> On 06/09/2024 21:14, 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.
>>>>
>>>> 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 for the i2c bus between two SS, do not
>>>> touch GPIO configuration during runtime suspend and only turn off the
>>>> clocks. This will allow other SS to continue to transfer the data
>>>> without any disturbance over the IO lines.
>>>
>>> This doesn't answer my question about what would be the behavior if one
>>> use uses, for example, GPI DMA, and the Linux kernel FIFO mode or SE 
>>> DMA ?
>>>
>> Shared usecase is not supported for non GSI mode (FIFO and DMA), it 
>> should be static usecase. Dynamic sharing from two clients of two 
>> subsystems is only for GSI mode. Hope this helps ?
> 
> Sure, this is why I proposed on v1 cover letter reply to add:
Sure, i will add in cover letter and code check combining with 
fifo_disable check.
> ==============><=====================================================================
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c 
> b/drivers/i2c/busses/i2c-qcom-geni.c
> index ee2e431601a6..a15825ea56de 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -885,7 +885,7 @@ 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) {
> +       if (gi2c->is_shared || fifo_disable) {
  Should be ANDING logically, as we need to combine both check. Shared
  usecase possible only for fifo_disable.

  if(gi2c->is_shared && fifo_disable) {
>                   /* FIFO is disabled, so we can only use GPI DMA */
>                   gi2c->gpi_mode = true;
>                   ret = setup_gpi_dma(gi2c);
> ==============><=====================================================================
> 
> Thanks,
> Neil
> 
>>> Because it seems to "fix" only the GPI DMA shared case.
>>>
>>> Neil
>>>
>>>>
>>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>>>> ---
>>>>   drivers/i2c/busses/i2c-qcom-geni.c | 29 ++++++++++++++++++++++-------
>>>>   1 file changed, 22 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c 
>>>> b/drivers/i2c/busses/i2c-qcom-geni.c
>>>> index eebb0cbb6ca4..ee2e431601a6 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>
>>>> @@ -99,6 +100,7 @@ struct geni_i2c_dev {
>>>>       struct dma_chan *rx_c;
>>>>       bool gpi_mode;
>>>>       bool abort_done;
>>>> +    bool is_shared;
>>>>   };
>>>>   struct geni_i2c_desc {
>>>> @@ -602,6 +604,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->is_shared;
>>>>       for (i = 0; i < num; i++) {
>>>>           gi2c->cur = &msgs[i];
>>>> @@ -612,6 +615,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 +636,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_err(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 +808,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->is_shared = true;
>>>> +        dev_dbg(&pdev->dev, "Shared SE Usecase\n");
>>>> +    }
>>>> +
>>>>       if (has_acpi_companion(dev))
>>>>           ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
>>>> @@ -962,14 +975,16 @@ static int __maybe_unused 
>>>> geni_i2c_runtime_suspend(struct device *dev)
>>>>       struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>>>>       disable_irq(gi2c->irq);
>>>> -    ret = geni_se_resources_off(&gi2c->se);
>>>> -    if (ret) {
>>>> -        enable_irq(gi2c->irq);
>>>> -        return ret;
>>>> -
>>>> +    if (gi2c->is_shared) {
>>>> +        geni_se_clks_off(&gi2c->se);
>>>>       } else {
>>>> -        gi2c->suspended = 1;
>>>> +        ret = geni_se_resources_off(&gi2c->se);
>>>> +        if (ret) {
>>>> +            enable_irq(gi2c->irq);
>>>> +            return ret;
>>>> +        }
>>>>       }
>>>> +    gi2c->suspended = 1;
>>>>       clk_disable_unprepare(gi2c->core_clk);
>>>
>>>
> 
> 

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

[neil.armstrong]

On 10/09/2024 11:15, Mukesh Kumar Savaliya wrote:
> Hi Neil,
> 
> On 9/9/2024 6:34 PM, neil.armstrong@linaro.org wrote:
>> On 09/09/2024 11:18, Mukesh Kumar Savaliya wrote:
>>> Hi Neil,
>>>
>>> On 9/9/2024 2:24 PM, neil.armstrong@linaro.org wrote:
>>>> Hi,
>>>>
>>>> On 06/09/2024 21:14, 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.
>>>>>
>>>>> 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 for the i2c bus between two SS, do not
>>>>> touch GPIO configuration during runtime suspend and only turn off the
>>>>> clocks. This will allow other SS to continue to transfer the data
>>>>> without any disturbance over the IO lines.
>>>>
>>>> This doesn't answer my question about what would be the behavior if one
>>>> use uses, for example, GPI DMA, and the Linux kernel FIFO mode or SE DMA ?
>>>>
>>> Shared usecase is not supported for non GSI mode (FIFO and DMA), it should be static usecase. Dynamic sharing from two clients of two subsystems is only for GSI mode. Hope this helps ?
>>
>> Sure, this is why I proposed on v1 cover letter reply to add:
> Sure, i will add in cover letter and code check combining with fifo_disable check.
>> ==============><=====================================================================
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>> index ee2e431601a6..a15825ea56de 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -885,7 +885,7 @@ 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) {
>> +       if (gi2c->is_shared || fifo_disable) {
>   Should be ANDING logically, as we need to combine both check. Shared
>   usecase possible only for fifo_disable.

Could you elaborate on that ? GPI DMA is totally usable even if FIFO is enabled,
it's a decision took in the driver to _not_ use GPI when FIFO is enabled.

Neil

> 
>   if(gi2c->is_shared && fifo_disable) {
>>                   /* FIFO is disabled, so we can only use GPI DMA */
>>                   gi2c->gpi_mode = true;
>>                   ret = setup_gpi_dma(gi2c);
>> ==============><=====================================================================
>>
>> Thanks,
>> Neil
>>
>>>> Because it seems to "fix" only the GPI DMA shared case.
>>>>
>>>> Neil
>>>>
>>>>>
>>>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>>>>> ---
>>>>>   drivers/i2c/busses/i2c-qcom-geni.c | 29 ++++++++++++++++++++++-------
>>>>>   1 file changed, 22 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>>>>> index eebb0cbb6ca4..ee2e431601a6 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>
>>>>> @@ -99,6 +100,7 @@ struct geni_i2c_dev {
>>>>>       struct dma_chan *rx_c;
>>>>>       bool gpi_mode;
>>>>>       bool abort_done;
>>>>> +    bool is_shared;
>>>>>   };
>>>>>   struct geni_i2c_desc {
>>>>> @@ -602,6 +604,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->is_shared;
>>>>>       for (i = 0; i < num; i++) {
>>>>>           gi2c->cur = &msgs[i];
>>>>> @@ -612,6 +615,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 +636,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_err(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 +808,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->is_shared = true;
>>>>> +        dev_dbg(&pdev->dev, "Shared SE Usecase\n");
>>>>> +    }
>>>>> +
>>>>>       if (has_acpi_companion(dev))
>>>>>           ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
>>>>> @@ -962,14 +975,16 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
>>>>>       struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>>>>>       disable_irq(gi2c->irq);
>>>>> -    ret = geni_se_resources_off(&gi2c->se);
>>>>> -    if (ret) {
>>>>> -        enable_irq(gi2c->irq);
>>>>> -        return ret;
>>>>> -
>>>>> +    if (gi2c->is_shared) {
>>>>> +        geni_se_clks_off(&gi2c->se);
>>>>>       } else {
>>>>> -        gi2c->suspended = 1;
>>>>> +        ret = geni_se_resources_off(&gi2c->se);
>>>>> +        if (ret) {
>>>>> +            enable_irq(gi2c->irq);
>>>>> +            return ret;
>>>>> +        }
>>>>>       }
>>>>> +    gi2c->suspended = 1;
>>>>>       clk_disable_unprepare(gi2c->core_clk);
>>>>
>>>>
>>
>>

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

[Krzysztof Kozlowski]

On 10/09/2024 11:09, Mukesh Kumar Savaliya wrote:
> Thanks Krzysztof.
> 
> On 9/7/2024 2:34 PM, Krzysztof Kozlowski wrote:
>> On 06/09/2024 21:14, Mukesh Kumar Savaliya wrote:
>>> Adds qcom,shared-se flag usage. Use this when particular I2C serial
>>> controller needs to be shared between two subsystems.
>>
>> <form letter>
>> Please use scripts/get_maintainers.pl to get a list of necessary people
>> and lists to CC (and consider --no-git-fallback argument). It might
>> happen, that command when run on an older kernel, gives you outdated
>> entries. Therefore please be sure you base your patches on recent Linux
>> kernel.
>>
>> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
>> people, so fix your workflow. Tools might also fail if you work on some
>> ancient tree (don't, instead use mainline) or work on fork of kernel
>> (don't, instead use mainline). Just use b4 and everything should be
>> fine, although remember about `b4 prep --auto-to-cc` if you added new
>> patches to the patchset.
>> </form letter>
>>
>> You already got this comment, so how many times it has to be repeated?
>> Your process is just wrong if you do not use the tools for this.
>>
> Sorry, I was already using scripts/get_maintainer.pl but i kept everyone 
> into To list (That's my mistake here). I shall keep maintainers in TO 
> list and rest in CC list.

No, To or Cc does not matter. Your list is just incomplete.

> 
> Question: With <Form Letter> , are you asking to add letter in this 
> first patch ? I have cover letter, but it will get removed when patch 
> gets merged. Please help suggest and clarify.

No, it's just template. Form letter... I am just bored to repeat the
same comment.

>>
>>>
>>> SE = Serial Engine, meant for I2C controller here.
>>> TRE = Transfer Ring Element, refers to Queued Descriptor.
>>>
>>> Example :
>>> Two clients from different SS can share an I2C SE for same slave device
>>
>> What is SS?
>>
> SS = Subsystem (EE - Execution Environment, can be Apps 
> processor/TZ/Modem/ADSP etc). Let me add this too in next patch.

Yes, please explain in the binding itself.

>>> 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..ae423127f736 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.
>>
>> What is a subsystem? With commit msg I still do not understand this.
> SS = Subsystem (EE - Execution Environment, can be Apps 
> processor/TZ/Modem/ADSP etc). Let me add EE too with full form.
>> Maybe presence of hwlock defines it anyway, so this is redundant?
> No, this flag is required. As hwlock comes into picture if this flag is 

Flag is required? By what? Sorry, you push your downstream solution to us.

> defined. So flag is acting as a condition to take hwlock TRE 
> descriptor(transfer ring element). Hope i could answer your query.

Hm, not sure, maybe indeed hwlock would not be enough. However I think
existing binding misses hwlock property.

Best regards,
Krzysztof

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

[Mukesh Kumar Savaliya]

Hi Andi,

On 9/9/2024 6:23 PM, Andi Shyti wrote:
> Thank you guys for your reviews,
> 
> On Mon, Sep 09, 2024 at 01:37:00PM GMT, Konrad Dybcio wrote:
>> On 9.09.2024 11:18 AM, Mukesh Kumar Savaliya wrote:
>>> Hi Neil,
>>>
>>> On 9/9/2024 2:24 PM, neil.armstrong@linaro.org wrote:
>>>> Hi,
>>>>
>>>> On 06/09/2024 21:14, 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.
>>>>>
>>>>> 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 for the i2c bus between two SS, do not
>>>>> touch GPIO configuration during runtime suspend and only turn off the
>>>>> clocks. This will allow other SS to continue to transfer the data
>>>>> without any disturbance over the IO lines.
>>>>
>>>> This doesn't answer my question about what would be the behavior if one
>>>> use uses, for example, GPI DMA, and the Linux kernel FIFO mode or SE DMA ?
>>>>
>>> Shared usecase is not supported for non GSI mode (FIFO and DMA), it should be static usecase. Dynamic sharing from two clients of two subsystems is only for GSI mode. Hope this helps ?
>>
>> This should very much be explained in commit message and perhaps in code
>>
>> And since it can't work with FIFO mode, there should be checks in code
>> to disallow such invalid configurations
> 
> it would be nice if, along with all these open questions and
> clarifications on the commit message, we could add some good
> comments to the code as well.
> 
Agree, i realized it's good to add comment around code changes too.
In V3 i am incorporating all the suggestions and comments.
> Thanks,
> Andi

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

[Mukesh Kumar Savaliya]

Hi Andi, Sorry for late reply on this.

On 9/7/2024 1:26 PM, Andi Shyti wrote:
> Hi Mukesh,
> 
> On Sat, Sep 07, 2024 at 12:44:38AM 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.
>>
>> 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 for the i2c bus between two SS, do not
>> touch GPIO configuration during runtime suspend and only turn off the
>> clocks. This will allow other SS to continue to transfer the data
>> without any disturbance over the IO lines.
> 
> if I remember correctly, someone already commented on your
> patches to explain and expand the achronyms you are using. Please
> improve the commit log so that people who don't know this device
> can understand.
> 
Sure, Andi. Noted and addressed over comment. Will add full form for the 
acronyms.
> ...
> 
>> @@ -631,8 +636,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_err(gi2c->se.dev, "I2C timeout gpi flags:%d addr:0x%x\n",
>> +						gi2c->cur->flags, gi2c->cur->addr);
> 
> Please, don't print out here. The user doesn't really need to
> know, let the upper levels decide what to do.
Sure, can remove this completely and user can handle , print it.

> 
>>   			gi2c->err = -ETIMEDOUT;
>> +		}
>>   
>>   		if (gi2c->err) {
>>   			ret = gi2c->err;
>> @@ -800,6 +808,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->is_shared = true;
>> +		dev_dbg(&pdev->dev, "Shared SE Usecase\n");
> 
> Please, improve this debug message.
> 
"I2C is shared between subsystems" ?
> The rest looks good to me.
> 
> Thanks,
> Andi

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

[Mukesh Kumar Savaliya]

Thanks For your reviews,

On 9/10/2024 3:24 PM, Krzysztof Kozlowski wrote:
> On 10/09/2024 11:09, Mukesh Kumar Savaliya wrote:
>> Thanks Krzysztof.
>>
>> On 9/7/2024 2:34 PM, Krzysztof Kozlowski wrote:
>>> On 06/09/2024 21:14, Mukesh Kumar Savaliya wrote:
>>>> Adds qcom,shared-se flag usage. Use this when particular I2C serial
>>>> controller needs to be shared between two subsystems.
>>>
>>> <form letter>
>>> Please use scripts/get_maintainers.pl to get a list of necessary people
>>> and lists to CC (and consider --no-git-fallback argument). It might
>>> happen, that command when run on an older kernel, gives you outdated
>>> entries. Therefore please be sure you base your patches on recent Linux
>>> kernel.
>>>
>>> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
>>> people, so fix your workflow. Tools might also fail if you work on some
>>> ancient tree (don't, instead use mainline) or work on fork of kernel
>>> (don't, instead use mainline). Just use b4 and everything should be
>>> fine, although remember about `b4 prep --auto-to-cc` if you added new
>>> patches to the patchset.
>>> </form letter>
>>>
>>> You already got this comment, so how many times it has to be repeated?
>>> Your process is just wrong if you do not use the tools for this.
>>>
>> Sorry, I was already using scripts/get_maintainer.pl but i kept everyone
>> into To list (That's my mistake here). I shall keep maintainers in TO
>> list and rest in CC list.
> 
> No, To or Cc does not matter. Your list is just incomplete.
> 
Got it, sorry for the trouble. It seems i missed below 3 names adding 
into reviewers by copy paste mistake. I hope this makes it complete now 
and will add them in V3.

Bryan O'Donoghue" <bryan.odonoghue@linaro.org>
Krzysztof Kozlowski <krzk+dt@kernel.org>
Rob Herring <robh@kernel.org>
>>
>> Question: With <Form Letter> , are you asking to add letter in this
>> first patch ? I have cover letter, but it will get removed when patch
>> gets merged. Please help suggest and clarify.
> 
> No, it's just template. Form letter... I am just bored to repeat the
> same comment.
> 
Sorry for that. I hope i could catch now as per above missing list.
>>>
>>>>
>>>> SE = Serial Engine, meant for I2C controller here.
>>>> TRE = Transfer Ring Element, refers to Queued Descriptor.
>>>>
>>>> Example :
>>>> Two clients from different SS can share an I2C SE for same slave device
>>>
>>> What is SS?
>>>
>> SS = Subsystem (EE - Execution Environment, can be Apps
>> processor/TZ/Modem/ADSP etc). Let me add this too in next patch.
> 
> Yes, please explain in the binding itself.
> 
ok, Sure.
>>>> 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..ae423127f736 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.
>>>
>>> What is a subsystem? With commit msg I still do not understand this.
>> SS = Subsystem (EE - Execution Environment, can be Apps
>> processor/TZ/Modem/ADSP etc). Let me add EE too with full form.
>>> Maybe presence of hwlock defines it anyway, so this is redundant?
>> No, this flag is required. As hwlock comes into picture if this flag is
> 
> Flag is required? By what? Sorry, you push your downstream solution to us.
> 
Let me explain, Using this flag to take hwlock via TRE @ [PATCH v2 2/4]
We need this to lock SE protecting from other SS transfers until 
unlocked. Hence shared-se flag becomes a decision marker.
drivers/dma/qcom/gpi.c => gpi_create_i2c_tre()
+	/* create lock tre for first tranfser */
+	if (i2c->shared_se && i2c->first_msg) {

Question: what exactly you mean "Maybe presence of hwlock defines it 
anyway" ?
I am open to consider all upstream solutions, trying to understand your 
suggestions and comments.
>> defined. So flag is acting as a condition to take hwlock TRE
>> descriptor(transfer ring element). Hope i could answer your query.
> 
> Hm, not sure, maybe indeed hwlock would not be enough. However I think
> existing binding misses hwlock property.
> 
Let me clarify, you may help suggest further.
hwlock is a descriptor bit(TRE_I2C_LOCK).
"However I think  existing binding misses hwlock property"
Where shall i keep this hwlock property? what's the usage ?

> Best regards,
> Krzysztof
> 

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

[Mukesh Kumar Savaliya]

Thanks Neil for the review !

On 9/10/2024 3:22 PM, neil.armstrong@linaro.org wrote:
> On 10/09/2024 11:15, Mukesh Kumar Savaliya wrote:
>> Hi Neil,
>>
>> On 9/9/2024 6:34 PM, neil.armstrong@linaro.org wrote:
>>> On 09/09/2024 11:18, Mukesh Kumar Savaliya wrote:
>>>> Hi Neil,
>>>>
>>>> On 9/9/2024 2:24 PM, neil.armstrong@linaro.org wrote:
>>>>> Hi,
>>>>>
>>>>> On 06/09/2024 21:14, 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 ifthe
>>>>>> usecase requires i2c controller to be shared.
>>>>>>
>>>>>> 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 for the i2c bus between two SS, do 
>>>>>> not
>>>>>> touch GPIO configuration during runtime suspend and only turn off the
>>>>>> clocks. This will allow other SS to continue to transfer the data
>>>>>> without any disturbance over the IO lines.
>>>>>
>>>>> This doesn't answer my question about what would be the behavior if 
>>>>> one
>>>>> use uses, for example, GPI DMA, and the Linux kernel FIFO mode or 
>>>>> SE DMA ?
>>>>>
>>>> Shared usecase is not supported for non GSI mode (FIFO and DMA), it 
>>>> should be static usecase. Dynamic sharing from two clients of two 
>>>> subsystems is only for GSI mode. Hope this helps ?
>>>
>>> Sure, this is why I proposed on v1 cover letter reply to add:
>> Sure, i will add in cover letter and code check combining with 
>> fifo_disable check.
>>> ==============><=====================================================================
>>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c 
>>> b/drivers/i2c/busses/i2c-qcom-geni.c
>>> index ee2e431601a6..a15825ea56de 100644
>>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>>> @@ -885,7 +885,7 @@ 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) {
>>> +       if (gi2c->is_shared || fifo_disable) {
>>   Should be ANDING logically, as we need to combine both check. Shared
>>   usecase possible only for fifo_disable.
> 
> Could you elaborate on that ? GPI DMA is totally usable even if FIFO is 
> enabled,
> it's a decision took in the driver to _not_ use GPI when FIFO is enabled.
> 
Yes, Neil, you are right. Its actually reverse condition from HW 
configuration.

if fifo_disable = true, then FIFO registers will not be accessible, 
meaning its GPI mode only. And SW should decide use GPI DMA mode only.

if fifo_disable = false, it can still use GPI DMA/CPU_DMA. we need to 
restrict from SW side.

Provided above, i suggest to keep conditional check with ANDING.
We want only GSI mode to be supported with shared SE. Because GSI mode 
only has GPII channel allocated to each EE. if not, then it will be 
misused between EEs and no way to prevent concurrency at HW level.(E.g. 
we use lock/unlock in GSI mode)

If so, hope you agree with the conditional check of ANDing both flags?
> Neil
> 
>>
>>   if(gi2c->is_shared && fifo_disable) {
>>>                   /* FIFO is disabled, so we can only use GPI DMA */
>>>                   gi2c->gpi_mode = true;
>>>                   ret = setup_gpi_dma(gi2c);
>>> ==============><=====================================================================
>>>
>>> Thanks,
>>> Neil
>>>
>>>>> Because it seems to "fix" only the GPI DMA shared case.
>>>>>
>>>>> Neil
>>>>>
>>>>>>
>>>>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>>>>>> ---
>>>>>>   drivers/i2c/busses/i2c-qcom-geni.c | 29 
>>>>>> ++++++++++++++++++++++-------
>>>>>>   1 file changed, 22 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c 
>>>>>> b/drivers/i2c/busses/i2c-qcom-geni.c
>>>>>> index eebb0cbb6ca4..ee2e431601a6 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>
>>>>>> @@ -99,6 +100,7 @@ struct geni_i2c_dev {
>>>>>>       struct dma_chan *rx_c;
>>>>>>       bool gpi_mode;
>>>>>>       bool abort_done;
>>>>>> +    bool is_shared;
>>>>>>   };
>>>>>>   struct geni_i2c_desc {
>>>>>> @@ -602,6 +604,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->is_shared;
>>>>>>       for (i = 0; i < num; i++) {
>>>>>>           gi2c->cur =&msgs[i];
>>>>>> @@ -612,6 +615,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 +636,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_err(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 +808,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->is_shared = true;
>>>>>> +        dev_dbg(&pdev->dev, "Shared SE Usecase\n");
>>>>>> +    }
>>>>>> +
>>>>>>       if (has_acpi_companion(dev))
>>>>>>           ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
>>>>>> @@ -962,14 +975,16 @@ static int __maybe_unused 
>>>>>> geni_i2c_runtime_suspend(struct device *dev)
>>>>>>       struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>>>>>>       disable_irq(gi2c->irq);
>>>>>> -    ret = geni_se_resources_off(&gi2c->se);
>>>>>> -    if (ret) {
>>>>>> -        enable_irq(gi2c->irq);
>>>>>> -        return ret;
>>>>>> -
>>>>>> +    if (gi2c->is_shared) {
>>>>>> +        geni_se_clks_off(&gi2c->se);
>>>>>>       } else {
>>>>>> -        gi2c->suspended = 1;
>>>>>> +        ret = geni_se_resources_off(&gi2c->se);
>>>>>> +        if (ret) {
>>>>>> +            enable_irq(gi2c->irq);
>>>>>> +            return ret;
>>>>>> +        }
>>>>>>       }
>>>>>> +    gi2c->suspended = 1;
>>>>>>       clk_disable_unprepare(gi2c->core_clk);
>>>>>
>>>>>
>>>
>>>
>