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

[PATCH v5 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 V4 : https://lore.kernel.org/lkml/20241113161413.3821858-1-quic_msavaliy@quicinc.com/

Changes in V5:
 - Corrected name as qcom,shared-se instead of qcom,is-shared.
 - Added description for the SE acronyms into yaml file and commit log.
 - Renamed TRE_I2C_UNLOCK to TRE_UNLOCK being generic.
 - Log an error and return if non GPI mode goes into shared usecase.

---
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      |  8 ++++
 drivers/dma/qcom/gpi.c                        | 37 ++++++++++++++++++-
 drivers/i2c/busses/i2c-qcom-geni.c            | 22 +++++++++--
 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, 81 insertions(+), 8 deletions(-)

-- 
2.25.1

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

[Mukesh Kumar Savaliya]

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

SE(Serial Engine HW controller acting as protocol master controller) is an
I2C controller. Basically a programmable SERDES(serializer/deserializer)
coupled with data DMA entity, capable in handling a bus protocol, and data
moves to/from system memory.

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>
---
 .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml       | 8 ++++++++
 1 file changed, 8 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..88682a333399 100644
--- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
+++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
@@ -60,6 +60,14 @@ properties:
   power-domains:
     maxItems: 1
 
+  qcom,shared-se:
+    description: True if I2C controller is shared between two or more system processors.
+        SE(Serial Engine HW controller working as protocol master controller) is an
+        I2C controller. Basically, a programmable SERDES(serializer/deserializer)
+        coupled with data DMA entity, capable in handling a bus protocol, and data
+        moves to/from system memory.
+    type: boolean
+
   reg:
     maxItems: 1
 
-- 
2.25.1

[PATCH v5 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..c74417240012 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_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_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 v5 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..e4721bbd4b05 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 v5 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 | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 7a22e1f46e60..ccf9933e2dad 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,12 @@ static int geni_i2c_probe(struct platform_device *pdev)
 		dev_dbg(dev, "Using GPI DMA mode for I2C\n");
 	} else {
 		gi2c->gpi_mode = false;
+
+		if (gi2c->se.shared_geni_se) {
+			dev_err(dev, "I2C sharing is not supported in non GSI mode\n");
+			return -EINVAL;
+		}
+
 		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 +998,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 v5 1/4] dt-bindindgs: i2c: qcom,i2c-geni: Document shared flag

[Krzysztof Kozlowski]

On 29/11/2024 15:43, Mukesh Kumar Savaliya wrote:
> Adds qcom,shared-se flag usage. Use this flag when I2C serial controller
> needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment.
> 
> SE(Serial Engine HW controller acting as protocol master controller) is an
> I2C controller. Basically a programmable SERDES(serializer/deserializer)
> coupled with data DMA entity, capable in handling a bus protocol, and data
> moves to/from system memory.
> 
> 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>
> ---
>  .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml       | 8 ++++++++
>  1 file changed, 8 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..88682a333399 100644
> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> @@ -60,6 +60,14 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  qcom,shared-se:
> +    description: True if I2C controller is shared between two or more system processors.
> +        SE(Serial Engine HW controller working as protocol master controller) is an
> +        I2C controller. Basically, a programmable SERDES(serializer/deserializer)
> +        coupled with data DMA entity, capable in handling a bus protocol, and data
> +        moves to/from system memory.
I replied why I NAK it. You did not really address my concerns, but
replied with some generic statement. After that generic statement you
gave me exactly 0 seconds to react and you sent v5.

Really 0 seconds to respond to your comment, while you give yourself
days to respond to my comments.

This is not how it works.

NAK

Implement previous feedback. Don't send any new versions before you
understand what you have to do and get some agreement with reviewers.

Best regards,
Krzysztof

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

[Bjorn Andersson]

On Fri, Nov 29, 2024 at 08:13:54PM +0530, Mukesh Kumar Savaliya wrote:
> Adds qcom,shared-se flag usage. Use this flag when I2C serial controller
> needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment.
> 

Per https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
your commit message should start with a description of your problem.
"Add" isn't the right word to start a problem description with.

> SE(Serial Engine HW controller acting as protocol master controller) is an
> I2C controller. Basically a programmable SERDES(serializer/deserializer)

"Basically"?

> coupled with data DMA entity, capable in handling a bus protocol, and data
> moves to/from system memory.
> 
> 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.
> 

The DeviceTree binding describes properties used to describe the
hardware; your commit message describes what a SE is and that it can
exist can exist in a configuration with multiple client etc etc.

> Transfer gets serialized by Lock TRE + DMA xfer + Unlock TRE at HW level.
> 

This isn't what this patch implements. It defines a property which when
specified means to the OS that any DMA transfers should be performed
using TRE lock/unlock operations.

> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> ---
>  .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml       | 8 ++++++++
>  1 file changed, 8 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..88682a333399 100644
> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> @@ -60,6 +60,14 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  qcom,shared-se:
> +    description: True if I2C controller is shared between two or more system processors.

This attempts to describe the property.

> +        SE(Serial Engine HW controller working as protocol master controller) is an
> +        I2C controller. Basically, a programmable SERDES(serializer/deserializer)
> +        coupled with data DMA entity, capable in handling a bus protocol, and data
> +        moves to/from system memory.

But this is basically just 4 lines of text expanding the acronym "se",
but while it might give some insight into what this binding (the whole
binding) is about, I'm afraid it doesn't add value to the understanding
of the property...

Regards,
Bjorn

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

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

[Mukesh Kumar Savaliya]

Hi Krzysztof,

On 11/29/2024 8:44 PM, Krzysztof Kozlowski wrote:
> On 29/11/2024 15:43, Mukesh Kumar Savaliya wrote:
>> Adds qcom,shared-se flag usage. Use this flag when I2C serial controller
>> needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment.
>>
>> SE(Serial Engine HW controller acting as protocol master controller) is an
>> I2C controller. Basically a programmable SERDES(serializer/deserializer)
>> coupled with data DMA entity, capable in handling a bus protocol, and data
>> moves to/from system memory.
>>
>> 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>
>> ---
>>   .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml       | 8 ++++++++
>>   1 file changed, 8 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..88682a333399 100644
>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> @@ -60,6 +60,14 @@ properties:
>>     power-domains:
>>       maxItems: 1
>>   
>> +  qcom,shared-se:
>> +    description: True if I2C controller is shared between two or more system processors.
>> +        SE(Serial Engine HW controller working as protocol master controller) is an
>> +        I2C controller. Basically, a programmable SERDES(serializer/deserializer)
>> +        coupled with data DMA entity, capable in handling a bus protocol, and data
>> +        moves to/from system memory.
> I replied why I NAK it. You did not really address my concerns, but
> replied with some generic statement. After that generic statement you
> gave me exactly 0 seconds to react and you sent v5.
> 
Sorry for 0 seconds, i thought of addressing comment and uploading it 
new patch as i wanted to explain SE. whatever i have added for SE 
explanation is in qualcomm hardware programming guide document.
> Really 0 seconds to respond to your comment, while you give yourself
> days to respond to my comments.
> 
> This is not how it works.
> 
Sure, let me first conclude here what exactly should be done.
> NAK
> 
> Implement previous feedback. Don't send any new versions before you
> understand what you have to do and get some agreement with reviewers.
> 
Sure, this is definitely a good way. what did i do for previous comment ?
I have opened SE and expanded, explained.

which statement or explanation should i rephrase ? Is it description 
statement from this yaml file ? Could you please suggested better word 
instead of shared-se if this flag name is not suitable ?

I could not get this ask -
"There are few of such flags already and there are some patches adding 
it in different flavors."

> Best regards,
> Krzysztof

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

[Vinod Koul]

On 29-11-24, 20:13, 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.
> 

...

> @@ -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;

Looking at this why do you need this field? It can be internal to your
i2c driver... Why not just set an enum for lock and use the values as
lock/unlock/dont care and make the interface simpler. I see no reason to
use three variables to communicate the info which can be handled in
simpler way..?

> +	bool first_msg;
> +	bool last_msg;

-- 
~Vinod

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

[Krzysztof Kozlowski]

On 02/12/2024 05:00, Mukesh Kumar Savaliya wrote:
> Hi Krzysztof,
> 
> On 11/29/2024 8:44 PM, Krzysztof Kozlowski wrote:
>> On 29/11/2024 15:43, Mukesh Kumar Savaliya wrote:
>>> Adds qcom,shared-se flag usage. Use this flag when I2C serial controller
>>> needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment.
>>>
>>> SE(Serial Engine HW controller acting as protocol master controller) is an
>>> I2C controller. Basically a programmable SERDES(serializer/deserializer)
>>> coupled with data DMA entity, capable in handling a bus protocol, and data
>>> moves to/from system memory.
>>>
>>> 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>
>>> ---
>>>   .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml       | 8 ++++++++
>>>   1 file changed, 8 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..88682a333399 100644
>>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>> @@ -60,6 +60,14 @@ properties:
>>>     power-domains:
>>>       maxItems: 1
>>>   
>>> +  qcom,shared-se:
>>> +    description: True if I2C controller is shared between two or more system processors.
>>> +        SE(Serial Engine HW controller working as protocol master controller) is an
>>> +        I2C controller. Basically, a programmable SERDES(serializer/deserializer)
>>> +        coupled with data DMA entity, capable in handling a bus protocol, and data
>>> +        moves to/from system memory.
>> I replied why I NAK it. You did not really address my concerns, but
>> replied with some generic statement. After that generic statement you
>> gave me exactly 0 seconds to react and you sent v5.
>>
> Sorry for 0 seconds, i thought of addressing comment and uploading it 
> new patch as i wanted to explain SE. whatever i have added for SE 
> explanation is in qualcomm hardware programming guide document.
>> Really 0 seconds to respond to your comment, while you give yourself
>> days to respond to my comments.
>>
>> This is not how it works.
>>
> Sure, let me first conclude here what exactly should be done.
>> NAK
>>
>> Implement previous feedback. Don't send any new versions before you
>> understand what you have to do and get some agreement with reviewers.
>>
> Sure, this is definitely a good way. what did i do for previous comment ?
> I have opened SE and expanded, explained.
> 
> which statement or explanation should i rephrase ? Is it description 
> statement from this yaml file ? Could you please suggested better word 
> instead of shared-se if this flag name is not suitable ?
> 
> I could not get this ask -
> "There are few of such flags already and there are some patches adding 
> it in different flavors."

Come with one flag or enum, if needed, covering all your cases like this.

Best regards,
Krzysztof

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

[Mukesh Kumar Savaliya]

Hi Bjorn, Thanks for the review !

On 11/30/2024 10:15 AM, Bjorn Andersson wrote:
> On Fri, Nov 29, 2024 at 08:13:54PM +0530, Mukesh Kumar Savaliya wrote:
>> Adds qcom,shared-se flag usage. Use this flag when I2C serial controller
>> needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment.
>>
> 
> Per https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> your commit message should start with a description of your problem.
> "Add" isn't the right word to start a problem description with.
Problem statement i have explained in cover letter, let me add here too 
in V6. I thought same story gets repeated here. Will not start with Add, 
but problem statement and need of this flag.
>> SE(Serial Engine HW controller acting as protocol master controller) is an
>> I2C controller. Basically a programmable SERDES(serializer/deserializer)
> 
> "Basically"?
will remove this.
> 
>> coupled with data DMA entity, capable in handling a bus protocol, and data
>> moves to/from system memory.
>>
>> 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.
>>
> 
> The DeviceTree binding describes properties used to describe the
> hardware; your commit message describes what a SE is and that it can
> exist can exist in a configuration with multiple client etc etc.
> 
I have explained the use of flag, and background surrounding to the 
feature. See the V4 and V5 and earlier, where i was required to explain 
and open up about "what is SE" ?
Because of the SE word in flag name, i had to expand with explanation.
>> Transfer gets serialized by Lock TRE + DMA xfer + Unlock TRE at HW level.
>>
> 
> This isn't what this patch implements. It defines a property which when
> specified means to the OS that any DMA transfers should be performed
> using TRE lock/unlock operations.
I agree, it adds onto understanding about the flag feature. I can remove 
this statement in V6. Let me get complete agreement.
> 
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> ---
>>   .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml       | 8 ++++++++
>>   1 file changed, 8 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..88682a333399 100644
>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> @@ -60,6 +60,14 @@ properties:
>>     power-domains:
>>       maxItems: 1
>>   
>> +  qcom,shared-se:
>> +    description: True if I2C controller is shared between two or more system processors.
> 
> This attempts to describe the property.
I agree, thats why i limited but there was an ask to understand what is 
SE ? hence i added below.
> 
>> +        SE(Serial Engine HW controller working as protocol master controller) is an
>> +        I2C controller. Basically, a programmable SERDES(serializer/deserializer)
>> +        coupled with data DMA entity, capable in handling a bus protocol, and data
>> +        moves to/from system memory.
> 
> But this is basically just 4 lines of text expanding the acronym "se",
> but while it might give some insight into what this binding (the whole
> binding) is about, I'm afraid it doesn't add value to the understanding
> of the property...
> 
""se" is also not explained in the binding - please open it and look for
such explanation."

It was required to explain based on comment on V4, V5 hence i did. 
Please let me know if one line is enough to explain flag usage OR we 
need exact description from the hardware programming guide ?

I will need to get agreement on this patch first and then upload V6.

> Regards,
> Bjorn
> 
>> +    type: boolean
>> +
>>     reg:
>>       maxItems: 1
>>   
>> -- 
>> 2.25.1
>>

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

[Mukesh Kumar Savaliya]

Thanks Krzysztof for giving clarity on ask and reviewing this change.

On 12/2/2024 12:49 PM, Krzysztof Kozlowski wrote:
> On 02/12/2024 05:00, Mukesh Kumar Savaliya wrote:
>> Hi Krzysztof,
>>
>> On 11/29/2024 8:44 PM, Krzysztof Kozlowski wrote:
>>> On 29/11/2024 15:43, Mukesh Kumar Savaliya wrote:
>>>> Adds qcom,shared-se flag usage. Use this flag when I2C serial controller
>>>> needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment.
>>>>
>>>> SE(Serial Engine HW controller acting as protocol master controller) is an
>>>> I2C controller. Basically a programmable SERDES(serializer/deserializer)
>>>> coupled with data DMA entity, capable in handling a bus protocol, and data
>>>> moves to/from system memory.
>>>>
>>>> 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>
>>>> ---
>>>>    .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml       | 8 ++++++++
>>>>    1 file changed, 8 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..88682a333399 100644
>>>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>>> @@ -60,6 +60,14 @@ properties:
>>>>      power-domains:
>>>>        maxItems: 1
>>>>    
>>>> +  qcom,shared-se:
>>>> +    description: True if I2C controller is shared between two or more system processors.
>>>> +        SE(Serial Engine HW controller working as protocol master controller) is an
>>>> +        I2C controller. Basically, a programmable SERDES(serializer/deserializer)
>>>> +        coupled with data DMA entity, capable in handling a bus protocol, and data
>>>> +        moves to/from system memory.
>>> I replied why I NAK it. You did not really address my concerns, but
>>> replied with some generic statement. After that generic statement you
>>> gave me exactly 0 seconds to react and you sent v5.
>>>
>> Sorry for 0 seconds, i thought of addressing comment and uploading it
>> new patch as i wanted to explain SE. whatever i have added for SE
>> explanation is in qualcomm hardware programming guide document.
>>> Really 0 seconds to respond to your comment, while you give yourself
>>> days to respond to my comments.
>>>
>>> This is not how it works.
>>>
>> Sure, let me first conclude here what exactly should be done.
>>> NAK
>>>
>>> Implement previous feedback. Don't send any new versions before you
>>> understand what you have to do and get some agreement with reviewers.
>>>
>> Sure, this is definitely a good way. what did i do for previous comment ?
>> I have opened SE and expanded, explained.
>>
>> which statement or explanation should i rephrase ? Is it description
>> statement from this yaml file ? Could you please suggested better word
>> instead of shared-se if this flag name is not suitable ?
>>
>> I could not get this ask -
>> "There are few of such flags already and there are some patches adding
>> it in different flavors."
> 
> Come with one flag or enum, if needed, covering all your cases like this.
> 
Let me explain, this feature is one of the additional software case 
adding on base protocol support. if we dont have more than one usecase 
or repurposing this feature, why do we need to add enums ? I see one 
flag gpi_mode but it's internal to driver not exposed to user or expose 
any usecase/feature.

Below was our earlier context, just wanted to add for clarity.
--
 > Is sharing of IP blocks going to be also for other devices? If yes, then
 > this should be one property for all Qualcomm devices. If not, then be
 > sure that this is the case because I will bring it up if you come with
 > one more solution for something else.
 >
IP blocks like SE can be shared. Here we are talking about I2C sharing.
In future it can be SPI sharing. But design wise it fits better to add
flag per SE node. Same we shall be adding for SPI too in future.

Please let me know your further suggestions.
--

As we want to finalize agreement on this dt-bindings patch, will wait 
for agreement and confirmation before V6.

> Best regards,
> Krzysztof

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

[Mukesh Kumar Savaliya]

Thanks for the review comments Vinod !

On 12/2/2024 12:17 PM, Vinod Koul wrote:
> On 29-11-24, 20:13, 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.
>>
> 
> ...
> 
>> @@ -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;
> 
> Looking at this why do you need this field? It can be internal to your
> i2c driver... Why not just set an enum for lock and use the values as
> lock/unlock/dont care and make the interface simpler. I see no reason to
> use three variables to communicate the info which can be handled in
> simpler way..?
> 
Below was earlier reply to [PATCH V3, 2/4], please let me know if you 
have any additional comment and need further clarifications.
--
 > Looking at the usage in following patches, why cant this be handled
 > internally as part of prep call?
 >
As per design, i2c driver iterates over each message and submits to GPI 
where it creates TRE. Since it's per transfer, we need to create Lock 
and Unlock TRE based on first or last message.
--
>> +	bool first_msg;
>> +	bool last_msg;
> 

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

[Krzysztof Kozlowski]

On 02/12/2024 11:38, Mukesh Kumar Savaliya wrote:
>>
>> Come with one flag or enum, if needed, covering all your cases like this.
>>
> Let me explain, this feature is one of the additional software case 
> adding on base protocol support. if we dont have more than one usecase 
> or repurposing this feature, why do we need to add enums ? I see one 
> flag gpi_mode but it's internal to driver not exposed to user or expose 
> any usecase/feature.
> 
> Below was our earlier context, just wanted to add for clarity.
> --
>  > Is sharing of IP blocks going to be also for other devices? If yes, then
>  > this should be one property for all Qualcomm devices. If not, then be
>  > sure that this is the case because I will bring it up if you come with
>  > one more solution for something else.


You keep repeating the same. You won't receive any other answer.

>  >
> IP blocks like SE can be shared. Here we are talking about I2C sharing.
> In future it can be SPI sharing. But design wise it fits better to add
> flag per SE node. Same we shall be adding for SPI too in future.


How flag per SE node is relevant? I did not ask to move the property.

> 
> Please let me know your further suggestions.
We do not talk about I2C or SPI here only. We talk about entire SoC.
Since beginning. Find other patch proposals and align with rest of
Qualcomm developers so that you come with only one definition for this
feature/characteristic. Or do you want to say that I am free to NAK all
further properties duplicating this one?

Please confirm that you Qualcomm engineers understand the last statement
and that every block will use se-shared, even if we speak about UFS for
example.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 02/12/2024 12:04, Krzysztof Kozlowski wrote:
> On 02/12/2024 11:38, Mukesh Kumar Savaliya wrote:
>>>
>>> Come with one flag or enum, if needed, covering all your cases like this.
>>>
>> Let me explain, this feature is one of the additional software case 
>> adding on base protocol support. if we dont have more than one usecase 
>> or repurposing this feature, why do we need to add enums ? I see one 
>> flag gpi_mode but it's internal to driver not exposed to user or expose 
>> any usecase/feature.
>>
>> Below was our earlier context, just wanted to add for clarity.
>> --
>>  > Is sharing of IP blocks going to be also for other devices? If yes, then
>>  > this should be one property for all Qualcomm devices. If not, then be
>>  > sure that this is the case because I will bring it up if you come with
>>  > one more solution for something else.
> 
> 
> You keep repeating the same. You won't receive any other answer.
> 
>>  >
>> IP blocks like SE can be shared. Here we are talking about I2C sharing.
>> In future it can be SPI sharing. But design wise it fits better to add
>> flag per SE node. Same we shall be adding for SPI too in future.
> 
> 
> How flag per SE node is relevant? I did not ask to move the property.
> 
>>
>> Please let me know your further suggestions.
> We do not talk about I2C or SPI here only. We talk about entire SoC.
> Since beginning. Find other patch proposals and align with rest of
> Qualcomm developers so that you come with only one definition for this
> feature/characteristic. Or do you want to say that I am free to NAK all
> further properties duplicating this one?
> 
> Please confirm that you Qualcomm engineers understand the last statement
> and that every block will use se-shared, even if we speak about UFS for
> example.
> 

I think I was pretty clear also 2 months ago what do I expect from this:

https://lore.kernel.org/all/52f83419-cc5e-49f3-90a7-26a5b4ddd5a0@kernel.org/


I do not see this addressing qcom-wide way at all. Four new versions of
patch and you still did not address first fedback you got.


Best regards,
Krzysztof

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

[Mukesh Kumar Savaliya]

On 12/2/2024 4:34 PM, Krzysztof Kozlowski wrote:
> On 02/12/2024 11:38, Mukesh Kumar Savaliya wrote:
>>>
>>> Come with one flag or enum, if needed, covering all your cases like this.
>>>
>> Let me explain, this feature is one of the additional software case
>> adding on base protocol support. if we dont have more than one usecase
>> or repurposing this feature, why do we need to add enums ? I see one
>> flag gpi_mode but it's internal to driver not exposed to user or expose
>> any usecase/feature.
>>
>> Below was our earlier context, just wanted to add for clarity.
>> --
>>   > Is sharing of IP blocks going to be also for other devices? If yes, then
>>   > this should be one property for all Qualcomm devices. If not, then be
>>   > sure that this is the case because I will bring it up if you come with
>>   > one more solution for something else.
> 
> 
> You keep repeating the same. You won't receive any other answer.
> 
So far i was in context to SEs. I am not sure in qualcomm SOC all cores 
supporting this feature and if it at all it supports, it may have it's 
own mechanism then what is followed in SE IP. I was probably thinking on 
my owned IP core hence i was revolving around.

Hope this dt-binding i can conclude somewhere by seeking answer from 
other IP core owners within qualcomm.
>>   >
>> IP blocks like SE can be shared. Here we are talking about I2C sharing.
>> In future it can be SPI sharing. But design wise it fits better to add
>> flag per SE node. Same we shall be adding for SPI too in future.
> 
> 
> How flag per SE node is relevant? I did not ask to move the property.
> 
>>
>> Please let me know your further suggestions.
> We do not talk about I2C or SPI here only. We talk about entire SoC.
> Since beginning. Find other patch proposals and align with rest of
> Qualcomm developers so that you come with only one definition for this
> feature/characteristic. Or do you want to say that I am free to NAK all
> further properties duplicating this one?
> 
> Please confirm that you Qualcomm engineers understand the last statement
> and that every block will use se-shared, even if we speak about UFS for
> example.
This UFS word atleast makes me understand and gave me clarity that i 
need to talk to different IP owners within qualcomm and get an agreement 
for my i2c feature. I am not sure if there exist an usecase the way we 
are sharing for i2c. Also i don't know how we can make similar 
description if different cores and functionality are different.  If you 
have heard from any other IP core, please keep some usecases/IP names.

Since This demands internal discussion, so give me time to conclude how 
the IPs are shared and is it the similar to what i have developed here 
for I2C. (sorry that so far i was in context to my SE protocols/ IPs only).
> 
> Best regards,
> Krzysztof

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

[Konrad Dybcio]

On 2.12.2024 1:55 PM, Mukesh Kumar Savaliya wrote:
> 
> 
> On 12/2/2024 4:34 PM, Krzysztof Kozlowski wrote:
>> On 02/12/2024 11:38, Mukesh Kumar Savaliya wrote:
>>>>
>>>> Come with one flag or enum, if needed, covering all your cases like this.
>>>>
>>> Let me explain, this feature is one of the additional software case
>>> adding on base protocol support. if we dont have more than one usecase
>>> or repurposing this feature, why do we need to add enums ? I see one
>>> flag gpi_mode but it's internal to driver not exposed to user or expose
>>> any usecase/feature.
>>>
>>> Below was our earlier context, just wanted to add for clarity.
>>> -- 
>>>   > Is sharing of IP blocks going to be also for other devices? If yes, then
>>>   > this should be one property for all Qualcomm devices. If not, then be
>>>   > sure that this is the case because I will bring it up if you come with
>>>   > one more solution for something else.
>>
>>
>> You keep repeating the same. You won't receive any other answer.
>>
> So far i was in context to SEs. I am not sure in qualcomm SOC all cores supporting this feature and if it at all it supports, it may have it's own mechanism then what is followed in SE IP. I was probably thinking on my owned IP core hence i was revolving around.
> 
> Hope this dt-binding i can conclude somewhere by seeking answer from other IP core owners within qualcomm.
>>>   >
>>> IP blocks like SE can be shared. Here we are talking about I2C sharing.
>>> In future it can be SPI sharing. But design wise it fits better to add
>>> flag per SE node. Same we shall be adding for SPI too in future.
>>
>>
>> How flag per SE node is relevant? I did not ask to move the property.
>>
>>>
>>> Please let me know your further suggestions.
>> We do not talk about I2C or SPI here only. We talk about entire SoC.
>> Since beginning. Find other patch proposals and align with rest of
>> Qualcomm developers so that you come with only one definition for this
>> feature/characteristic. Or do you want to say that I am free to NAK all
>> further properties duplicating this one?

I'm not sure a single property name+description can fit all possible
cases here. The hardware being "shared" can mean a number of different
things, with some blocks having hardware provisions for that, while
others may have totally none and rely on external mechanisms (e.g.
a shared memory buffer) to indicate whether an external entity
manages power to them.

Even here, I'm not particularly sure whether dt is the right place.
Maybe we could think about checking for clock_is_enabled()? That's
just an idea, as it may fall apart if CCF gets a .sync_state impl.

Konrad

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

[Bjorn Andersson]

On Mon, Dec 02, 2024 at 04:08:32PM +0530, Mukesh Kumar Savaliya wrote:
> Hi Bjorn, Thanks for the review !
> 
> On 11/30/2024 10:15 AM, Bjorn Andersson wrote:
> > On Fri, Nov 29, 2024 at 08:13:54PM +0530, Mukesh Kumar Savaliya wrote:
> > > Adds qcom,shared-se flag usage. Use this flag when I2C serial controller
> > > needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment.
> > > 
> > 
> > Per https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> > your commit message should start with a description of your problem.
> > "Add" isn't the right word to start a problem description with.
> Problem statement i have explained in cover letter, let me add here too in
> V6. I thought same story gets repeated here. Will not start with Add, but
> problem statement and need of this flag.

The cover-letter is generally not included in the git history, so that
explanation would be lost on future readers.

> > > SE(Serial Engine HW controller acting as protocol master controller) is an
> > > I2C controller. Basically a programmable SERDES(serializer/deserializer)
> > 
> > "Basically"?
> will remove this.
> > 
> > > coupled with data DMA entity, capable in handling a bus protocol, and data
> > > moves to/from system memory.
> > > 
> > > 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.
> > > 
> > 
> > The DeviceTree binding describes properties used to describe the
> > hardware; your commit message describes what a SE is and that it can
> > exist can exist in a configuration with multiple client etc etc.
> > 
> I have explained the use of flag, and background surrounding to the feature.
> See the V4 and V5 and earlier, where i was required to explain and open up
> about "what is SE" ?
> Because of the SE word in flag name, i had to expand with explanation.
> > > Transfer gets serialized by Lock TRE + DMA xfer + Unlock TRE at HW level.
> > > 
> > 
> > This isn't what this patch implements. It defines a property which when
> > specified means to the OS that any DMA transfers should be performed
> > using TRE lock/unlock operations.
> I agree, it adds onto understanding about the flag feature. I can remove
> this statement in V6. Let me get complete agreement.

I think the understanding is necessary, but that the wording should be
different. Imaging you're implementing MukeshOS and reading this binding
document to understand what you're expected to do in your I2C driver
when you see this property. 


Similarly, the binding document should be sufficiently clear such that a
newly hired colleague of ours would understand if they should put this
property or not in the dts file they are writing.

> > 
> > > Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> > > ---
> > >   .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml       | 8 ++++++++
> > >   1 file changed, 8 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..88682a333399 100644
> > > --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> > > +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> > > @@ -60,6 +60,14 @@ properties:
> > >     power-domains:
> > >       maxItems: 1
> > > +  qcom,shared-se:
> > > +    description: True if I2C controller is shared between two or more system processors.
> > 
> > This attempts to describe the property.
> I agree, thats why i limited but there was an ask to understand what is SE ?
> hence i added below.
> > 
> > > +        SE(Serial Engine HW controller working as protocol master controller) is an
> > > +        I2C controller. Basically, a programmable SERDES(serializer/deserializer)
> > > +        coupled with data DMA entity, capable in handling a bus protocol, and data
> > > +        moves to/from system memory.
> > 
> > But this is basically just 4 lines of text expanding the acronym "se",
> > but while it might give some insight into what this binding (the whole
> > binding) is about, I'm afraid it doesn't add value to the understanding
> > of the property...
> > 
> ""se" is also not explained in the binding - please open it and look for
> such explanation."
> 
> It was required to explain based on comment on V4, V5 hence i did. Please
> let me know if one line is enough to explain flag usage OR we need exact
> description from the hardware programming guide ?
> 

What I'm saying is that this binding is for the serial engine, if you
need to describe what SE or a serial engine is you should do that in the
top-level description, not within one of the properties (or in a
possible future repeat that explanation in multiple different
properties).

> I will need to get agreement on this patch first and then upload V6.
> 

Sounds good.

Regards,
Bjorn

> > Regards,
> > Bjorn
> > 
> > > +    type: boolean
> > > +
> > >     reg:
> > >       maxItems: 1
> > > -- 
> > > 2.25.1
> > > 

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

[Vinod Koul]

On 02-12-24, 16:13, Mukesh Kumar Savaliya wrote:
> Thanks for the review comments Vinod !
> 
> On 12/2/2024 12:17 PM, Vinod Koul wrote:
> > On 29-11-24, 20:13, 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.
> > > 
> > 
> > ...
> > 
> > > @@ -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;
> > 
> > Looking at this why do you need this field? It can be internal to your
> > i2c driver... Why not just set an enum for lock and use the values as
> > lock/unlock/dont care and make the interface simpler. I see no reason to
> > use three variables to communicate the info which can be handled in
> > simpler way..?
> > 
> Below was earlier reply to [PATCH V3, 2/4], please let me know if you have
> any additional comment and need further clarifications.

Looks like you misunderstood, the question is why do you need three
variables to convey this info..? Use a single variable please

> --
> > Looking at the usage in following patches, why cant this be handled
> > internally as part of prep call?
> >
> As per design, i2c driver iterates over each message and submits to GPI
> where it creates TRE. Since it's per transfer, we need to create Lock and
> Unlock TRE based on first or last message.
> --
> > > +	bool first_msg;
> > > +	bool last_msg;
> > 

-- 
~Vinod

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

[Mukesh Kumar Savaliya]

On 12/2/2024 7:34 PM, Konrad Dybcio wrote:
> On 2.12.2024 1:55 PM, Mukesh Kumar Savaliya wrote:
>>
>>
>> On 12/2/2024 4:34 PM, Krzysztof Kozlowski wrote:
>>> On 02/12/2024 11:38, Mukesh Kumar Savaliya wrote:
>>>>>
>>>>> Come with one flag or enum, if needed, covering all your cases like this.
>>>>>
>>>> Let me explain, this feature is one of the additional software case
>>>> adding on base protocol support. if we dont have more than one usecase
>>>> or repurposing this feature, why do we need to add enums ? I see one
>>>> flag gpi_mode but it's internal to driver not exposed to user or expose
>>>> any usecase/feature.
>>>>
>>>> Below was our earlier context, just wanted to add for clarity.
>>>> -- 
>>>>    > Is sharing of IP blocks going to be also for other devices? If yes, then
>>>>    > this should be one property for all Qualcomm devices. If not, then be
>>>>    > sure that this is the case because I will bring it up if you come with
>>>>    > one more solution for something else.
>>>
>>>
>>> You keep repeating the same. You won't receive any other answer.
>>>
>> So far i was in context to SEs. I am not sure in qualcomm SOC all cores supporting this feature and if it at all it supports, it may have it's own mechanism then what is followed in SE IP. I was probably thinking on my owned IP core hence i was revolving around.
>>
>> Hope this dt-binding i can conclude somewhere by seeking answer from other IP core owners within qualcomm.
>>>>    >
>>>> IP blocks like SE can be shared. Here we are talking about I2C sharing.
>>>> In future it can be SPI sharing. But design wise it fits better to add
>>>> flag per SE node. Same we shall be adding for SPI too in future.
>>>
>>>
>>> How flag per SE node is relevant? I did not ask to move the property.
>>>
>>>>
>>>> Please let me know your further suggestions.
>>> We do not talk about I2C or SPI here only. We talk about entire SoC.
>>> Since beginning. Find other patch proposals and align with rest of
>>> Qualcomm developers so that you come with only one definition for this
>>> feature/characteristic. Or do you want to say that I am free to NAK all
>>> further properties duplicating this one?
> 
> I'm not sure a single property name+description can fit all possible
> cases here. The hardware being "shared" can mean a number of different
> things, with some blocks having hardware provisions for that, while
> others may have totally none and rely on external mechanisms (e.g.
> a shared memory buffer) to indicate whether an external entity
> manages power to them.
> 
I agree. Also i checked internally with UFS team and other peripheral 
core. Not heard of core being shared the way SE is being shared.
> Even here, I'm not particularly sure whether dt is the right place.
> Maybe we could think about checking for clock_is_enabled()? That's
> just an idea, as it may fall apart if CCF gets a .sync_state impl.
> 
I feel DT flag is the only way as one or other way this leads to some 
need of prior knowledge. in case of using clock_is_enabled() kind of 
API, we still need a flag to keep the clock enabled. By the way, we keep 
pinctrl only enabled for shared SE.

Please let me know if the given binding can be improved further OR this 
looks fine ?
> Konrad

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

[Mukesh Kumar Savaliya]

Hi Krzysztof ,

On 12/2/2024 4:43 PM, Krzysztof Kozlowski wrote:
> On 02/12/2024 12:04, Krzysztof Kozlowski wrote:
>> On 02/12/2024 11:38, Mukesh Kumar Savaliya wrote:
>>>>
>>>> Come with one flag or enum, if needed, covering all your cases like this.
Please review below comment which was about other cores. I think we can 
go with single flag naming qcom,shared-corename, for similar features 
for any core.
>>>>
>>> Let me explain, this feature is one of the additional software case
>>> adding on base protocol support. if we dont have more than one usecase
>>> or repurposing this feature, why do we need to add enums ? I see one
>>> flag gpi_mode but it's internal to driver not exposed to user or expose
>>> any usecase/feature.
>>>
>>> Below was our earlier context, just wanted to add for clarity.
>>> --
>>>   > Is sharing of IP blocks going to be also for other devices? If yes, then
>>>   > this should be one property for all Qualcomm devices. If not, then be
>>>   > sure that this is the case because I will bring it up if you come with
>>>   > one more solution for something else.
>>
>>
>> You keep repeating the same. You won't receive any other answer.
>>
>>>   >
>>> IP blocks like SE can be shared. Here we are talking about I2C sharing.
>>> In future it can be SPI sharing. But design wise it fits better to add
>>> flag per SE node. Same we shall be adding for SPI too in future.
>>
>>
>> How flag per SE node is relevant? I did not ask to move the property.
>>
>>>
>>> Please let me know your further suggestions.
>> We do not talk about I2C or SPI here only. We talk about entire SoC.
>> Since beginning. Find other patch proposals and align with rest of
>> Qualcomm developers so that you come with only one definition for this
>> feature/characteristic. Or do you want to say that I am free to NAK all
>> further properties duplicating this one?
>>
>> Please confirm that you Qualcomm engineers understand the last statement
>> and that every block will use se-shared, even if we speak about UFS for
>> example.
>>
> 
> I think I was pretty clear also 2 months ago what do I expect from this:
> 
> https://lore.kernel.org/all/52f83419-cc5e-49f3-90a7-26a5b4ddd5a0@kernel.org/
> 
> 
> I do not see this addressing qcom-wide way at all. Four new versions of
> patch and you still did not address first fedback you got.
> 

To answer the comment @ 
https://lore.kernel.org/all/52f83419-cc5e-49f3-90a7-26a5b4ddd5a0@kernel.org/ 


Sorry for not replying straight to this query. Let me, sort out here 
being in agreement with you. I queried internally, no USB OR UFS OR PCIe 
having this case. BAM (sps driver) has such usecase, and if it's comes 
up in future, we should give similar name like shared-xxx if it's 
similar in nature.

"qcom,shared-xxx" is what i think a better option. Can it be renamed per 
core and let individual HW core use it when required ? Just my thought, 
you may please suggest better and simplified way.

clock controllers, TLMMs, MMUs  etc should also should add similar name 
if it's best suiting to the needs and similar feature.

> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 02/12/2024 15:04, Konrad Dybcio wrote:
>>>>   >
>>>> IP blocks like SE can be shared. Here we are talking about I2C sharing.
>>>> In future it can be SPI sharing. But design wise it fits better to add
>>>> flag per SE node. Same we shall be adding for SPI too in future.
>>>
>>>
>>> How flag per SE node is relevant? I did not ask to move the property.
>>>
>>>>
>>>> Please let me know your further suggestions.
>>> We do not talk about I2C or SPI here only. We talk about entire SoC.
>>> Since beginning. Find other patch proposals and align with rest of
>>> Qualcomm developers so that you come with only one definition for this
>>> feature/characteristic. Or do you want to say that I am free to NAK all
>>> further properties duplicating this one?
> 
> I'm not sure a single property name+description can fit all possible
> cases here. The hardware being "shared" can mean a number of different

Existing property does not explain anything more, either. To recap -
this block is SE and property is named "se-shared", so basically it is
equal to just "shared". "shared" is indeed quite vague, so I was
expecting some wider work here.


> things, with some blocks having hardware provisions for that, while
> others may have totally none and rely on external mechanisms (e.g.
> a shared memory buffer) to indicate whether an external entity
> manages power to them.

We have properties for that too. Qualcomm SoCs need once per year for
such shared properties. BAM has two or three. IPA has two. There are
probably even more blocks which I don't remember now.

> 
> Even here, I'm not particularly sure whether dt is the right place.
> Maybe we could think about checking for clock_is_enabled()? That's
> just an idea, as it may fall apart if CCF gets a .sync_state impl.



Best regards,
Krzysztof

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

[Konrad Dybcio]

On 12/10/24 08:28, Krzysztof Kozlowski wrote:
> On 02/12/2024 15:04, Konrad Dybcio wrote:
>>>>>    >
>>>>> IP blocks like SE can be shared. Here we are talking about I2C sharing.
>>>>> In future it can be SPI sharing. But design wise it fits better to add
>>>>> flag per SE node. Same we shall be adding for SPI too in future.
>>>>
>>>>
>>>> How flag per SE node is relevant? I did not ask to move the property.
>>>>
>>>>>
>>>>> Please let me know your further suggestions.
>>>> We do not talk about I2C or SPI here only. We talk about entire SoC.
>>>> Since beginning. Find other patch proposals and align with rest of
>>>> Qualcomm developers so that you come with only one definition for this
>>>> feature/characteristic. Or do you want to say that I am free to NAK all
>>>> further properties duplicating this one?
>>
>> I'm not sure a single property name+description can fit all possible
>> cases here. The hardware being "shared" can mean a number of different
> 
> Existing property does not explain anything more, either. To recap -
> this block is SE and property is named "se-shared", so basically it is
> equal to just "shared". "shared" is indeed quite vague, so I was
> expecting some wider work here.
> 
> 
>> things, with some blocks having hardware provisions for that, while
>> others may have totally none and rely on external mechanisms (e.g.
>> a shared memory buffer) to indicate whether an external entity
>> manages power to them.
> 
> We have properties for that too. Qualcomm SoCs need once per year for
> such shared properties. BAM has two or three. IPA has two. There are
> probably even more blocks which I don't remember now.

So, the problem is "driver must not toggle GPIO states", because
"the bus controller must not be muxed away from the endpoint".
You can come up with a number of similar problems by swapping out
the quoted text.

We can either describe what the driver must do (A), or what the
reason for it is (B).


If we go with A, we could have a property like:

&i2c1 {
	externally-handled-resources = <(EHR_PINCTRL_STATE | EHR_CLOCK_RATE)>
};

which would be a generic list of things that the OS would have to
tiptoe around, fitting Linux's framework split quite well



or if we go with B, we could add a property like:

&i2c1 {
	qcom,shared-controller;
};

which would hide the implementation details into the driver

I could see both approaches having their place, but in this specific
instance I think A would be more fitting, as the problem is quite
simple.

Krzysztof, thoughts?

Konrad

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

[Krzysztof Kozlowski]

On 10/12/2024 10:09, Konrad Dybcio wrote:
> 
> 
> On 12/10/24 08:28, Krzysztof Kozlowski wrote:
>> On 02/12/2024 15:04, Konrad Dybcio wrote:
>>>>>>    >
>>>>>> IP blocks like SE can be shared. Here we are talking about I2C sharing.
>>>>>> In future it can be SPI sharing. But design wise it fits better to add
>>>>>> flag per SE node. Same we shall be adding for SPI too in future.
>>>>>
>>>>>
>>>>> How flag per SE node is relevant? I did not ask to move the property.
>>>>>
>>>>>>
>>>>>> Please let me know your further suggestions.
>>>>> We do not talk about I2C or SPI here only. We talk about entire SoC.
>>>>> Since beginning. Find other patch proposals and align with rest of
>>>>> Qualcomm developers so that you come with only one definition for this
>>>>> feature/characteristic. Or do you want to say that I am free to NAK all
>>>>> further properties duplicating this one?
>>>
>>> I'm not sure a single property name+description can fit all possible
>>> cases here. The hardware being "shared" can mean a number of different
>>
>> Existing property does not explain anything more, either. To recap -
>> this block is SE and property is named "se-shared", so basically it is
>> equal to just "shared". "shared" is indeed quite vague, so I was
>> expecting some wider work here.
>>
>>
>>> things, with some blocks having hardware provisions for that, while
>>> others may have totally none and rely on external mechanisms (e.g.
>>> a shared memory buffer) to indicate whether an external entity
>>> manages power to them.
>>
>> We have properties for that too. Qualcomm SoCs need once per year for
>> such shared properties. BAM has two or three. IPA has two. There are
>> probably even more blocks which I don't remember now.
> 
> So, the problem is "driver must not toggle GPIO states", because
> "the bus controller must not be muxed away from the endpoint".
> You can come up with a number of similar problems by swapping out
> the quoted text.
> 
> We can either describe what the driver must do (A), or what the
> reason for it is (B).
> 
> 
> If we go with A, we could have a property like:
> 
> &i2c1 {
> 	externally-handled-resources = <(EHR_PINCTRL_STATE | EHR_CLOCK_RATE)>
> };
> 
> which would be a generic list of things that the OS would have to
> tiptoe around, fitting Linux's framework split quite well
> 
> 
> 
> or if we go with B, we could add a property like:
> 
> &i2c1 {
> 	qcom,shared-controller;
> };
> 
> which would hide the implementation details into the driver
> 
> I could see both approaches having their place, but in this specific
> instance I think A would be more fitting, as the problem is quite
> simple.


The second is fine with me, maybe missing information about "whom" do
you share it with. Or maybe we get to the point that all this is
specific to SoC, thus implied by compatible and we do not need
downstream approach (another discussion in USB pushed by Qcom: I want
one compatible and 1000 properties).

I really wished Qualcomm start reworking their bindings before they are
being sent upstream to match standard DT guidelines, not downstream
approach. Somehow these hundreds reviews we give could result in new
patches doing things better, not just repeating the same issues.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 10/12/2024 12:53, Krzysztof Kozlowski wrote:
>>>> I'm not sure a single property name+description can fit all possible
>>>> cases here. The hardware being "shared" can mean a number of different
>>>
>>> Existing property does not explain anything more, either. To recap -
>>> this block is SE and property is named "se-shared", so basically it is
>>> equal to just "shared". "shared" is indeed quite vague, so I was
>>> expecting some wider work here.
>>>
>>>
>>>> things, with some blocks having hardware provisions for that, while
>>>> others may have totally none and rely on external mechanisms (e.g.
>>>> a shared memory buffer) to indicate whether an external entity
>>>> manages power to them.
>>>
>>> We have properties for that too. Qualcomm SoCs need once per year for
>>> such shared properties. BAM has two or three. IPA has two. There are
>>> probably even more blocks which I don't remember now.
>>
>> So, the problem is "driver must not toggle GPIO states", because
>> "the bus controller must not be muxed away from the endpoint".
>> You can come up with a number of similar problems by swapping out
>> the quoted text.
>>
>> We can either describe what the driver must do (A), or what the
>> reason for it is (B).
>>
>>
>> If we go with A, we could have a property like:
>>
>> &i2c1 {
>> 	externally-handled-resources = <(EHR_PINCTRL_STATE | EHR_CLOCK_RATE)>
>> };
>>
>> which would be a generic list of things that the OS would have to
>> tiptoe around, fitting Linux's framework split quite well
>>
>>
>>
>> or if we go with B, we could add a property like:
>>
>> &i2c1 {
>> 	qcom,shared-controller;
>> };
>>
>> which would hide the implementation details into the driver
>>
>> I could see both approaches having their place, but in this specific
>> instance I think A would be more fitting, as the problem is quite
>> simple.
> 
> 
> The second is fine with me, maybe missing information about "whom" do
> you share it with. Or maybe we get to the point that all this is
> specific to SoC, thus implied by compatible and we do not need
> downstream approach (another discussion in USB pushed by Qcom: I want
> one compatible and 1000 properties).
> 
> I really wished Qualcomm start reworking their bindings before they are
> being sent upstream to match standard DT guidelines, not downstream
> approach. Somehow these hundreds reviews we give could result in new
> patches doing things better, not just repeating the same issues.

This is BTW v5, with all the same concerns from v1 and still no answers
in commit msg about these concerns. Nothing explained in commit msg
which hardware needs it or why the same SoC have it once shared, once
not (exclusive). Basically there is nothing here corresponding to any
real product, so since five versions all this for me is just copy-paste
from downstream approach.


Best regards,
Krzysztof

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

[Konrad Dybcio]

On 12/10/24 13:05, Krzysztof Kozlowski wrote:
> On 10/12/2024 12:53, Krzysztof Kozlowski wrote:
>>>>> I'm not sure a single property name+description can fit all possible
>>>>> cases here. The hardware being "shared" can mean a number of different
>>>>
>>>> Existing property does not explain anything more, either. To recap -
>>>> this block is SE and property is named "se-shared", so basically it is
>>>> equal to just "shared". "shared" is indeed quite vague, so I was
>>>> expecting some wider work here.
>>>>
>>>>
>>>>> things, with some blocks having hardware provisions for that, while
>>>>> others may have totally none and rely on external mechanisms (e.g.
>>>>> a shared memory buffer) to indicate whether an external entity
>>>>> manages power to them.
>>>>
>>>> We have properties for that too. Qualcomm SoCs need once per year for
>>>> such shared properties. BAM has two or three. IPA has two. There are
>>>> probably even more blocks which I don't remember now.
>>>
>>> So, the problem is "driver must not toggle GPIO states", because
>>> "the bus controller must not be muxed away from the endpoint".
>>> You can come up with a number of similar problems by swapping out
>>> the quoted text.
>>>
>>> We can either describe what the driver must do (A), or what the
>>> reason for it is (B).
>>>
>>>
>>> If we go with A, we could have a property like:
>>>
>>> &i2c1 {
>>> 	externally-handled-resources = <(EHR_PINCTRL_STATE | EHR_CLOCK_RATE)>
>>> };
>>>
>>> which would be a generic list of things that the OS would have to
>>> tiptoe around, fitting Linux's framework split quite well
>>>
>>>
>>>
>>> or if we go with B, we could add a property like:
>>>
>>> &i2c1 {
>>> 	qcom,shared-controller;
>>> };
>>>
>>> which would hide the implementation details into the driver
>>>
>>> I could see both approaches having their place, but in this specific
>>> instance I think A would be more fitting, as the problem is quite
>>> simple.
>>
>>
>> The second is fine with me, maybe missing information about "whom" do
>> you share it with. Or maybe we get to the point that all this is
>> specific to SoC, thus implied by compatible and we do not need
>> downstream approach (another discussion in USB pushed by Qcom: I want
>> one compatible and 1000 properties).
>>
>> I really wished Qualcomm start reworking their bindings before they are
>> being sent upstream to match standard DT guidelines, not downstream
>> approach. Somehow these hundreds reviews we give could result in new
>> patches doing things better, not just repeating the same issues.
> 
> This is BTW v5, with all the same concerns from v1 and still no answers
> in commit msg about these concerns. Nothing explained in commit msg
> which hardware needs it or why the same SoC have it once shared, once
> not (exclusive). Basically there is nothing here corresponding to any
> real product, so since five versions all this for me is just copy-paste
> from downstream approach.

So since this is a software contract and not a hardware
feature, this is not bound to any specific SoC or "firmware",
but rather to what runs on other cores (e.g. DSPs, MCUs spread
across the SoC or in a different software world, like TZ).

Specifying the specific intended use would be helpful though,
indeed.

Let's see if we can somehow make this saner.


Mukesh, do we have any spare registers that we could use to
indicate that a given SE is shared? Preferably within the
SE's register space itself. The bootloader or another entity
(DSP or what have you) would then set that bit before Linux
runs and we could skip the bindings story altogether.

It would need to be reserved on all SoCs though (future and
past), to make sure the contract is always held up, but I
think finding a persistent bit that has never been used
shouldn't be impossible.

Konrad

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

[Mukesh Kumar Savaliya]

Thanks Konrad !

On 12/10/2024 6:08 PM, Konrad Dybcio wrote:
> 
> 
> On 12/10/24 13:05, Krzysztof Kozlowski wrote:
>> On 10/12/2024 12:53, Krzysztof Kozlowski wrote:
>>>>>> I'm not sure a single property name+description can fit all possible
>>>>>> cases here. The hardware being "shared" can mean a number of 
>>>>>> different
>>>>>
>>>>> Existing property does not explain anything more, either. To recap -
>>>>> this block is SE and property is named "se-shared", so basically it is
>>>>> equal to just "shared". "shared" is indeed quite vague, so I was
>>>>> expecting some wider work here.
>>>>>
>>>>>
>>>>>> things, with some blocks having hardware provisions for that, while
>>>>>> others may have totally none and rely on external mechanisms (e.g.
>>>>>> a shared memory buffer) to indicate whether an external entity
>>>>>> manages power to them.
>>>>>
>>>>> We have properties for that too. Qualcomm SoCs need once per year for
>>>>> such shared properties. BAM has two or three. IPA has two. There are
>>>>> probably even more blocks which I don't remember now.
>>>>
>>>> So, the problem is "driver must not toggle GPIO states", because
>>>> "the bus controller must not be muxed away from the endpoint".
>>>> You can come up with a number of similar problems by swapping out
>>>> the quoted text.
>>>>
>>>> We can either describe what the driver must do (A), or what the
>>>> reason for it is (B).
>>>>
>>>>
>>>> If we go with A, we could have a property like:
>>>>
>>>> &i2c1 {
>>>>     externally-handled-resources = <(EHR_PINCTRL_STATE | 
>>>> EHR_CLOCK_RATE)>
>>>> };
>>>>
>>>> which would be a generic list of things that the OS would have to
>>>> tiptoe around, fitting Linux's framework split quite well
>>>>
>>>>
>>>>
>>>> or if we go with B, we could add a property like:
>>>>
>>>> &i2c1 {
>>>>     qcom,shared-controller;
>>>> };
>>>>
>>>> which would hide the implementation details into the driver
>>>>
>>>> I could see both approaches having their place, but in this specific
>>>> instance I think A would be more fitting, as the problem is quite
>>>> simple.
>>>
>>>
>>> The second is fine with me, maybe missing information about "whom" do
>>> you share it with. Or maybe we get to the point that all this is
>>> specific to SoC, thus implied by compatible and we do not need
>>> downstream approach (another discussion in USB pushed by Qcom: I want
>>> one compatible and 1000 properties).
>>>
>>> I really wished Qualcomm start reworking their bindings before they are
>>> being sent upstream to match standard DT guidelines, not downstream
>>> approach. Somehow these hundreds reviews we give could result in new
>>> patches doing things better, not just repeating the same issues.
>>
>> This is BTW v5, with all the same concerns from v1 and still no answers
>> in commit msg about these concerns. Nothing explained in commit msg
>> which hardware needs it or why the same SoC have it once shared, once
>> not (exclusive). Basically there is nothing here corresponding to any
>> real product, so since five versions all this for me is just copy-paste
>> from downstream approach.
> 
> So since this is a software contract and not a hardware
> feature, this is not bound to any specific SoC or "firmware",
> but rather to what runs on other cores (e.g. DSPs, MCUs spread
> across the SoC or in a different software world, like TZ).
> 
> Specifying the specific intended use would be helpful though,
> indeed.
> 
> Let's see if we can somehow make this saner.
> 
> 
> Mukesh, do we have any spare registers that we could use to
> indicate that a given SE is shared? Preferably within the
> SE's register space itself. The bootloader or another entity
> (DSP or what have you) would then set that bit before Linux
> runs and we could skip the bindings story altogether.
> 
There would be spare register but i think it should be in sync with 
hardware team. let me check with them and update back if any bit can be 
repurposed for this feature. I agree, if any register is available, it 
can programmed prior to kernel.
> It would need to be reserved on all SoCs though (future and
> past), to make sure the contract is always held up, but I
> think finding a persistent bit that has never been used
> shouldn't be impossible.
> 
Yes, let me check it with hardware and firmware team and update back. 
Does this mean, there can't be a such software sharing mechanism (purely 
software decision) based on DTSI flag ?
> Konrad

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

[Konrad Dybcio]

On 12/10/24 16:17, Mukesh Kumar Savaliya wrote:
> Thanks Konrad !
> 
> On 12/10/2024 6:08 PM, Konrad Dybcio wrote:
>>
>>
>> On 12/10/24 13:05, Krzysztof Kozlowski wrote:
>>> On 10/12/2024 12:53, Krzysztof Kozlowski wrote:
>>>>>>> I'm not sure a single property name+description can fit all possible
>>>>>>> cases here. The hardware being "shared" can mean a number of different
>>>>>>
>>>>>> Existing property does not explain anything more, either. To recap -
>>>>>> this block is SE and property is named "se-shared", so basically it is
>>>>>> equal to just "shared". "shared" is indeed quite vague, so I was
>>>>>> expecting some wider work here.
>>>>>>
>>>>>>
>>>>>>> things, with some blocks having hardware provisions for that, while
>>>>>>> others may have totally none and rely on external mechanisms (e.g.
>>>>>>> a shared memory buffer) to indicate whether an external entity
>>>>>>> manages power to them.
>>>>>>
>>>>>> We have properties for that too. Qualcomm SoCs need once per year for
>>>>>> such shared properties. BAM has two or three. IPA has two. There are
>>>>>> probably even more blocks which I don't remember now.
>>>>>
>>>>> So, the problem is "driver must not toggle GPIO states", because
>>>>> "the bus controller must not be muxed away from the endpoint".
>>>>> You can come up with a number of similar problems by swapping out
>>>>> the quoted text.
>>>>>
>>>>> We can either describe what the driver must do (A), or what the
>>>>> reason for it is (B).
>>>>>
>>>>>
>>>>> If we go with A, we could have a property like:
>>>>>
>>>>> &i2c1 {
>>>>>     externally-handled-resources = <(EHR_PINCTRL_STATE | EHR_CLOCK_RATE)>
>>>>> };
>>>>>
>>>>> which would be a generic list of things that the OS would have to
>>>>> tiptoe around, fitting Linux's framework split quite well
>>>>>
>>>>>
>>>>>
>>>>> or if we go with B, we could add a property like:
>>>>>
>>>>> &i2c1 {
>>>>>     qcom,shared-controller;
>>>>> };
>>>>>
>>>>> which would hide the implementation details into the driver
>>>>>
>>>>> I could see both approaches having their place, but in this specific
>>>>> instance I think A would be more fitting, as the problem is quite
>>>>> simple.
>>>>
>>>>
>>>> The second is fine with me, maybe missing information about "whom" do
>>>> you share it with. Or maybe we get to the point that all this is
>>>> specific to SoC, thus implied by compatible and we do not need
>>>> downstream approach (another discussion in USB pushed by Qcom: I want
>>>> one compatible and 1000 properties).
>>>>
>>>> I really wished Qualcomm start reworking their bindings before they are
>>>> being sent upstream to match standard DT guidelines, not downstream
>>>> approach. Somehow these hundreds reviews we give could result in new
>>>> patches doing things better, not just repeating the same issues.
>>>
>>> This is BTW v5, with all the same concerns from v1 and still no answers
>>> in commit msg about these concerns. Nothing explained in commit msg
>>> which hardware needs it or why the same SoC have it once shared, once
>>> not (exclusive). Basically there is nothing here corresponding to any
>>> real product, so since five versions all this for me is just copy-paste
>>> from downstream approach.
>>
>> So since this is a software contract and not a hardware
>> feature, this is not bound to any specific SoC or "firmware",
>> but rather to what runs on other cores (e.g. DSPs, MCUs spread
>> across the SoC or in a different software world, like TZ).
>>
>> Specifying the specific intended use would be helpful though,
>> indeed.
>>
>> Let's see if we can somehow make this saner.
>>
>>
>> Mukesh, do we have any spare registers that we could use to
>> indicate that a given SE is shared? Preferably within the
>> SE's register space itself. The bootloader or another entity
>> (DSP or what have you) would then set that bit before Linux
>> runs and we could skip the bindings story altogether.
>>
> There would be spare register but i think it should be in sync with hardware team. let me check with them and update back if any bit can be repurposed for this feature. I agree, if any register is available, it can programmed prior to kernel.
>> It would need to be reserved on all SoCs though (future and
>> past), to make sure the contract is always held up, but I
>> think finding a persistent bit that has never been used
>> shouldn't be impossible.
>>
> Yes, let me check it with hardware and firmware team and update back. Does this mean, there can't be a such software sharing mechanism (purely software decision) based on DTSI flag ?

I suppose that depends on our needs. If we can set that bit
before Linux starts (i.e. in UEFI), we can avoid touching
the pinctrl state regardless of whether the other entities
have started up yet to avoid overcomplicating it.

If we need Linux to set that bit, we would still need some
mechanism like a dt property. But I really think that the
bootloader should be burdened with this instead, given it
has a better understanding of the hardware due to it being
well, the bootloader).

Krzysztof, I'm assuming that sounds sane from your
perspective too?

Konrad

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

[Krzysztof Kozlowski]

>> There would be spare register but i think it should be in sync with hardware team. let me check with them and update back if any bit can be repurposed for this feature. I agree, if any register is available, it can programmed prior to kernel.
>>> It would need to be reserved on all SoCs though (future and
>>> past), to make sure the contract is always held up, but I
>>> think finding a persistent bit that has never been used
>>> shouldn't be impossible.
>>>
>> Yes, let me check it with hardware and firmware team and update back. Does this mean, there can't be a such software sharing mechanism (purely software decision) based on DTSI flag ?
> 
> I suppose that depends on our needs. If we can set that bit
> before Linux starts (i.e. in UEFI), we can avoid touching
> the pinctrl state regardless of whether the other entities
> have started up yet to avoid overcomplicating it.
> 
> If we need Linux to set that bit, we would still need some
> mechanism like a dt property. But I really think that the
> bootloader should be burdened with this instead, given it
> has a better understanding of the hardware due to it being
> well, the bootloader).
> 
> Krzysztof, I'm assuming that sounds sane from your
> perspective too?
Yes, sound okay.

Best regards,
Krzysztof

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

[Bjorn Andersson]

On Tue, Dec 10, 2024 at 01:38:28PM +0100, Konrad Dybcio wrote:
> 
> 
> On 12/10/24 13:05, Krzysztof Kozlowski wrote:
> > On 10/12/2024 12:53, Krzysztof Kozlowski wrote:
> > > > > > I'm not sure a single property name+description can fit all possible
> > > > > > cases here. The hardware being "shared" can mean a number of different
> > > > > 
> > > > > Existing property does not explain anything more, either. To recap -
> > > > > this block is SE and property is named "se-shared", so basically it is
> > > > > equal to just "shared". "shared" is indeed quite vague, so I was
> > > > > expecting some wider work here.
> > > > > 
> > > > > 
> > > > > > things, with some blocks having hardware provisions for that, while
> > > > > > others may have totally none and rely on external mechanisms (e.g.
> > > > > > a shared memory buffer) to indicate whether an external entity
> > > > > > manages power to them.
> > > > > 
> > > > > We have properties for that too. Qualcomm SoCs need once per year for
> > > > > such shared properties. BAM has two or three. IPA has two. There are
> > > > > probably even more blocks which I don't remember now.
> > > > 
> > > > So, the problem is "driver must not toggle GPIO states", because
> > > > "the bus controller must not be muxed away from the endpoint".
> > > > You can come up with a number of similar problems by swapping out
> > > > the quoted text.
> > > > 
> > > > We can either describe what the driver must do (A), or what the
> > > > reason for it is (B).
> > > > 
> > > > 
> > > > If we go with A, we could have a property like:
> > > > 
> > > > &i2c1 {
> > > > 	externally-handled-resources = <(EHR_PINCTRL_STATE | EHR_CLOCK_RATE)>
> > > > };
> > > > 
> > > > which would be a generic list of things that the OS would have to
> > > > tiptoe around, fitting Linux's framework split quite well
> > > > 
> > > > 
> > > > 
> > > > or if we go with B, we could add a property like:
> > > > 
> > > > &i2c1 {
> > > > 	qcom,shared-controller;
> > > > };
> > > > 
> > > > which would hide the implementation details into the driver
> > > > 
> > > > I could see both approaches having their place, but in this specific
> > > > instance I think A would be more fitting, as the problem is quite
> > > > simple.
> > > 
> > > 
> > > The second is fine with me, maybe missing information about "whom" do
> > > you share it with. Or maybe we get to the point that all this is
> > > specific to SoC, thus implied by compatible and we do not need
> > > downstream approach (another discussion in USB pushed by Qcom: I want
> > > one compatible and 1000 properties).
> > > 
> > > I really wished Qualcomm start reworking their bindings before they are
> > > being sent upstream to match standard DT guidelines, not downstream
> > > approach. Somehow these hundreds reviews we give could result in new
> > > patches doing things better, not just repeating the same issues.
> > 
> > This is BTW v5, with all the same concerns from v1 and still no answers
> > in commit msg about these concerns. Nothing explained in commit msg
> > which hardware needs it or why the same SoC have it once shared, once
> > not (exclusive). Basically there is nothing here corresponding to any
> > real product, so since five versions all this for me is just copy-paste
> > from downstream approach.
> 
> So since this is a software contract and not a hardware
> feature, this is not bound to any specific SoC or "firmware",
> but rather to what runs on other cores (e.g. DSPs, MCUs spread
> across the SoC or in a different software world, like TZ).
> 

I don't think this is a reasonable distinction, the DeviceTree must
describe the interfaces/environment that the OS is to operate in.
Claiming that certain properties of that world directly or indirectly
comes from (static) "software choices" would make the whole concept of
DeviceTree useless.

The fact that a serial engine is shared, or not, is a static property of
the firmware for a given board, no different from "i2c1 being accessible
by this OS or not" or the fact that i2c1 is actually implement I2C and
not SPI (i.e. should this node be enabled in the DeviceTree passed to
the OS or not).


That said, the commit message still doesn't clearly describe the system
design or when this property should be set or not, which is what
Krzysztof has been asking for multiple times.

Let's circle back and help Mukesh rewrite the commit message such that
it clearly documents the problem being solved.

> Specifying the specific intended use would be helpful though,
> indeed.
> 
> Let's see if we can somehow make this saner.
> 
> 
> Mukesh, do we have any spare registers that we could use to
> indicate that a given SE is shared? Preferably within the
> SE's register space itself. The bootloader or another entity
> (DSP or what have you) would then set that bit before Linux
> runs and we could skip the bindings story altogether.
> 
> It would need to be reserved on all SoCs though (future and
> past), to make sure the contract is always held up, but I
> think finding a persistent bit that has never been used
> shouldn't be impossible.
> 

Let's not invent a custom one-off "hardware description" passing
interface.

Regards,
Bjorn

> Konrad

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

[Konrad Dybcio]

On 29.11.2024 3:43 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.
> 
> 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 | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 7a22e1f46e60..ccf9933e2dad 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.
> +		 **/

I don't think this change makes things clearer, I would drop it

>  		gi2c->gpi_mode = true;
>  		ret = setup_gpi_dma(gi2c);
>  		if (ret) {
> @@ -900,6 +911,12 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  		dev_dbg(dev, "Using GPI DMA mode for I2C\n");
>  	} else {
>  		gi2c->gpi_mode = false;
> +
> +		if (gi2c->se.shared_geni_se) {
> +			dev_err(dev, "I2C sharing is not supported in non GSI mode\n");
> +			return -EINVAL;

return dev_err_probe(dev, -EINVAL, "I2C...)

Konrad

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

[Mukesh Kumar Savaliya]

Hi Konrad,

On 12/13/2024 6:35 PM, Konrad Dybcio wrote:
> On 29.11.2024 3:43 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.
>>
>> 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 | 22 +++++++++++++++++++---
>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 7a22e1f46e60..ccf9933e2dad 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.
>> +		 **/
> 
> I don't think this change makes things clearer, I would drop it
Shall i revert back to previous change ? What's your suggestion ?
> 
>>   		gi2c->gpi_mode = true;
>>   		ret = setup_gpi_dma(gi2c);
>>   		if (ret) {
>> @@ -900,6 +911,12 @@ static int geni_i2c_probe(struct platform_device *pdev)
>>   		dev_dbg(dev, "Using GPI DMA mode for I2C\n");
>>   	} else {
>>   		gi2c->gpi_mode = false;
>> +
>> +		if (gi2c->se.shared_geni_se) {
>> +			dev_err(dev, "I2C sharing is not supported in non GSI mode\n");
>> +			return -EINVAL;
> 
> return dev_err_probe(dev, -EINVAL, "I2C...)
Yes, will incorporate this change in next patch. dt-binding change i am 
seeking something good writeup, then will upload V6. Thanks Konrad !
> 
> Konrad

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

[Konrad Dybcio]

On 15.12.2024 9:59 AM, Mukesh Kumar Savaliya wrote:
> Hi Konrad,
> 
> On 12/13/2024 6:35 PM, Konrad Dybcio wrote:
>> On 29.11.2024 3:43 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.
>>>
>>> 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 | 22 +++++++++++++++++++---
>>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>>> index 7a22e1f46e60..ccf9933e2dad 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.
>>> +         **/
>>
>> I don't think this change makes things clearer, I would drop it
> Shall i revert back to previous change ? What's your suggestion ?

Yes, drop changing this comment.

Konrad

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

[Mukesh Kumar Savaliya]

On 12/16/2024 5:40 PM, Konrad Dybcio wrote:
> On 15.12.2024 9:59 AM, Mukesh Kumar Savaliya wrote:
>> Hi Konrad,
>>
>> On 12/13/2024 6:35 PM, Konrad Dybcio wrote:
>>> On 29.11.2024 3:43 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.
>>>>
>>>> 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 | 22 +++++++++++++++++++---
>>>>    1 file changed, 19 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>>>> index 7a22e1f46e60..ccf9933e2dad 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.
>>>> +         **/
>>>
>>> I don't think this change makes things clearer, I would drop it
>> Shall i revert back to previous change ? What's your suggestion ?
> 
> Yes, drop changing this comment.
Sure, Thanks for confirming !
> 
> Konrad

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

[Mukesh Kumar Savaliya]

Hi Vinod, Thanks !  I just saw your comments now as somehow it was going 
in some other folder and didn't realize.

On 12/4/2024 5:51 PM, Vinod Koul wrote:
> On 02-12-24, 16:13, Mukesh Kumar Savaliya wrote:
>> Thanks for the review comments Vinod !
>>
>> On 12/2/2024 12:17 PM, Vinod Koul wrote:
>>> On 29-11-24, 20:13, 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.
>>>>
>>>
>>> ...
>>>
>>>> @@ -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;
>>>
>>> Looking at this why do you need this field? It can be internal to your
>>> i2c driver... Why not just set an enum for lock and use the values as
>>> lock/unlock/dont care and make the interface simpler. I see no reason to
>>> use three variables to communicate the info which can be handled in
>>> simpler way..?
>>>
>> Below was earlier reply to [PATCH V3, 2/4], please let me know if you have
>> any additional comment and need further clarifications.
> 
> Looks like you misunderstood, the question is why do you need three
> variables to convey this info..? Use a single variable please
Yes, I think so. Please let me clarify.
First variable is a feature flag and it's required to be explicitly 
mentioned by client (i2c/spi/etc) to GSI driver.

Second and third, can be optimized to boolean so either first or last 
can be passed.

Please correct me or add simple change where you would like to make, i 
can add that.
> 
>> --
>>> Looking at the usage in following patches, why cant this be handled
>>> internally as part of prep call?
>>>
>> As per design, i2c driver iterates over each message and submits to GPI
>> where it creates TRE. Since it's per transfer, we need to create Lock and
>> Unlock TRE based on first or last message.
>> --
>>>> +	bool first_msg;
>>>> +	bool last_msg;
>>>
> 

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

[Vinod Koul]

On 18-12-24, 18:04, Mukesh Kumar Savaliya wrote:
> Hi Vinod, Thanks !  I just saw your comments now as somehow it was going in
> some other folder and didn't realize.
> 
> On 12/4/2024 5:51 PM, Vinod Koul wrote:
> > On 02-12-24, 16:13, Mukesh Kumar Savaliya wrote:
> > > Thanks for the review comments Vinod !
> > > 
> > > On 12/2/2024 12:17 PM, Vinod Koul wrote:
> > > > On 29-11-24, 20:13, 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.
> > > > > 
> > > > 
> > > > ...
> > > > 
> > > > > @@ -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;
> > > > 
> > > > Looking at this why do you need this field? It can be internal to your
> > > > i2c driver... Why not just set an enum for lock and use the values as
> > > > lock/unlock/dont care and make the interface simpler. I see no reason to
> > > > use three variables to communicate the info which can be handled in
> > > > simpler way..?
> > > > 
> > > Below was earlier reply to [PATCH V3, 2/4], please let me know if you have
> > > any additional comment and need further clarifications.
> > 
> > Looks like you misunderstood, the question is why do you need three
> > variables to convey this info..? Use a single variable please
> Yes, I think so. Please let me clarify.
> First variable is a feature flag and it's required to be explicitly
> mentioned by client (i2c/spi/etc) to GSI driver.
> 
> Second and third, can be optimized to boolean so either first or last can be
> passed.
> 
> Please correct me or add simple change where you would like to make, i can
> add that.

I though we could do with a single and derive

Also, please see 20241212041639.4109039-3-quic_mdalam@quicinc.com, folks
from same company should talk together on same solutions, please
converge and come up with a single proposal which works for both drivers

-- 
~Vinod

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

[Mukesh Kumar Savaliya]

On 12/24/2024 3:28 PM, Vinod Koul wrote:
> On 18-12-24, 18:04, Mukesh Kumar Savaliya wrote:
>> Hi Vinod, Thanks !  I just saw your comments now as somehow it was going in
>> some other folder and didn't realize.
>>
>> On 12/4/2024 5:51 PM, Vinod Koul wrote:
>>> On 02-12-24, 16:13, Mukesh Kumar Savaliya wrote:
>>>> Thanks for the review comments Vinod !
>>>>
>>>> On 12/2/2024 12:17 PM, Vinod Koul wrote:
>>>>> On 29-11-24, 20:13, 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.
>>>>>>
>>>>>
>>>>> ...
>>>>>
>>>>>> @@ -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;
>>>>>
>>>>> Looking at this why do you need this field? It can be internal to your
>>>>> i2c driver... Why not just set an enum for lock and use the values as
>>>>> lock/unlock/dont care and make the interface simpler. I see no reason to
>>>>> use three variables to communicate the info which can be handled in
>>>>> simpler way..?
>>>>>
>>>> Below was earlier reply to [PATCH V3, 2/4], please let me know if you have
>>>> any additional comment and need further clarifications.
>>>
>>> Looks like you misunderstood, the question is why do you need three
>>> variables to convey this info..? Use a single variable please
>> Yes, I think so. Please let me clarify.
>> First variable is a feature flag and it's required to be explicitly
>> mentioned by client (i2c/spi/etc) to GSI driver.
>>
>> Second and third, can be optimized to boolean so either first or last can be
>> passed.
>>
>> Please correct me or add simple change where you would like to make, i can
>> add that.
> 
> I though we could do with a single and derive
> 
Sure, so as mentioned in the other crypto BAM patch probably dmaengine.h 
can hold flag and that can add support for lock/unlock similar to that 
patch.
I just realized it from your shared patch. let me work internally with 
Md sadre and review. Thanks for the comment.
> Also, please see 20241212041639.4109039-3-quic_mdalam@quicinc.com, folks
> from same company should talk together on same solutions, please
> converge and come up with a single proposal which works for both drivers
> 
Sure

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

[Mukesh Kumar Savaliya]

Hi Vinod,

On 12/26/2024 5:52 PM, Mukesh Kumar Savaliya wrote:
> 
> 
> On 12/24/2024 3:28 PM, Vinod Koul wrote:
>> On 18-12-24, 18:04, Mukesh Kumar Savaliya wrote:
>>> Hi Vinod, Thanks !  I just saw your comments now as somehow it was 
>>> going in
>>> some other folder and didn't realize.
>>>
>>> On 12/4/2024 5:51 PM, Vinod Koul wrote:
>>>> On 02-12-24, 16:13, Mukesh Kumar Savaliya wrote:
>>>>> Thanks for the review comments Vinod !
>>>>>
>>>>> On 12/2/2024 12:17 PM, Vinod Koul wrote:
>>>>>> On 29-11-24, 20:13, 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.
>>>>>>>
>>>>>>
>>>>>> ...
>>>>>>
>>>>>>> @@ -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;
>>>>>>
>>>>>> Looking at this why do you need this field? It can be internal to 
>>>>>> your
>>>>>> i2c driver... Why not just set an enum for lock and use the values as
>>>>>> lock/unlock/dont care and make the interface simpler. I see no 
>>>>>> reason to
>>>>>> use three variables to communicate the info which can be handled in
>>>>>> simpler way..?
>>>>>>
>>>>> Below was earlier reply to [PATCH V3, 2/4], please let me know if 
>>>>> you have
>>>>> any additional comment and need further clarifications.
>>>>
>>>> Looks like you misunderstood, the question is why do you need three
>>>> variables to convey this info..? Use a single variable please
>>> Yes, I think so. Please let me clarify.
>>> First variable is a feature flag and it's required to be explicitly
>>> mentioned by client (i2c/spi/etc) to GSI driver.
>>>
>>> Second and third, can be optimized to boolean so either first or last 
>>> can be
>>> passed.
>>>
>>> Please correct me or add simple change where you would like to make, 
>>> i can
>>> add that.
>>
>> I though we could do with a single and derive
>>
> Sure, so as mentioned in the other crypto BAM patch probably dmaengine.h 
> can hold flag and that can add support for lock/unlock similar to that 
> patch.
> I just realized it from your shared patch. let me work internally with 
> Md sadre and review. Thanks for the comment.
>> Also, please see 20241212041639.4109039-3-quic_mdalam@quicinc.com, folks
>> from same company should talk together on same solutions, please
>> converge and come up with a single proposal which works for both drivers
>>
I have discussed with Md Sadre and tried to understand and utilize the 
enum of lock and unlock in my changes. Below is the summary.

I can't use those lock and unlock enums here because it's required for 
first and last message respectively. intermediate transfers will not use 
anything. So we need to define one more enum like dma_ctrl_none.

if i create another internal parent structure having required 3 members, 
then also it will need 3 child members. So i think current one looks 
good to me.

Please help review and suggest if anything can be better here.

> Sure
> 
>