[PATCH v3 0/5] Add I2C support for Tegra264

[PATCH v3 0/5] Add I2C support for Tegra264

[Kartik Rajput]

Following series of patches add support for Tegra264 and High Speed (HS)
Mode in i2c-tegra.c driver.

Please note that this series depends on the following series posted by
Akhil for review:
https://lore.kernel.org/linux-tegra/9803c165-fa2f-44ba-a6fb-f11852c319e1@kernel.org/T/#t

Kartik Rajput (5):
  dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C
  i2c: tegra: Do not configure DMA if not supported
  i2c: tegra: Add HS mode support
  i2c: tegra: Add support for SW mutex register
  i2c: tegra: Add Tegra264 support

 .../bindings/i2c/nvidia,tegra20-i2c.yaml      |   7 +
 drivers/i2c/busses/i2c-tegra.c                | 196 ++++++++++++++++--
 2 files changed, 188 insertions(+), 15 deletions(-)

-- 
2.43.0

[PATCH v3 1/5] dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C

[Kartik Rajput]

iTegra264 has 17 generic I2C controllers, two of which are in always-on
partition of the SoC. In addition to the features supported by Tegra194
it also supports a SW mutex register to allow sharing the same I2C
instance across multiple firmware.

Document compatible string "nvidia,tegra264-i2c" for Tegra264 I2C.

Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
---
v2 -> v3:
	* Add constraints for "nvidia,tegra264-i2c".
v1 -> v2:
	* Fixed typos.
---
 .../devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml        | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
index 6b6f6762d122..f0693b872cb6 100644
--- a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
@@ -80,6 +80,12 @@ properties:
           support for 64 KiB transactions whereas earlier chips supported no
           more than 4 KiB per transactions.
         const: nvidia,tegra194-i2c
+      - description:
+          Tegra264 has 17 generic I2C controllers, two of which are in the AON
+          (always-on) partition of the SoC. In addition to the features from
+          Tegra194, a SW mutex register is added to support use of the same I2C
+          instance across multiple firmwares.
+        const: nvidia,tegra264-i2c
 
   reg:
     maxItems: 1
@@ -186,6 +192,7 @@ allOf:
             contains:
               enum:
                 - nvidia,tegra194-i2c
+                - nvidia,tegra264-i2c
     then:
       required:
         - resets
-- 
2.43.0

[PATCH v3 2/5] i2c: tegra: Do not configure DMA if not supported

[Kartik Rajput]

On Tegra264, not all I2C controllers have the necessary interface to
GPC DMA, this causes failures when function tegra_i2c_init_dma()
is called.

Ensure that "dmas" device-tree property is present before initializing
DMA in function tegra_i2c_init_dma().

Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
---
v1 -> v2:
	* Update commit message to clarify that some I2C controllers may
	  not have the necessary interface to GPC DMA.
---
 drivers/i2c/busses/i2c-tegra.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index ebd51165c46b..c7237d26b813 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -448,6 +448,9 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
 	if (IS_VI(i2c_dev))
 		return 0;
 
+	if (!device_property_present(i2c_dev->dev, "dmas"))
+		return 0;
+
 	if (i2c_dev->hw->has_apb_dma) {
 		if (!IS_ENABLED(CONFIG_TEGRA20_APB_DMA)) {
 			dev_dbg(i2c_dev->dev, "APB DMA support not enabled\n");
-- 
2.43.0

[PATCH v3 3/5] i2c: tegra: Add HS mode support

[Kartik Rajput]

Add support for HS (High Speed) mode transfers, which is supported by
Tegra194 onwards.

Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
---
v2 -> v3:
	* Document tlow_hs_mode and thigh_hs_mode.
v1 -> v2:
	* Document has_hs_mode_support.
	* Add a check to set the frequency to fastmode+ if the device
	  does not support HS mode but the requested frequency is more
	  than fastmode+.
---
 drivers/i2c/busses/i2c-tegra.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index c7237d26b813..d0b6aa013c96 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -91,6 +91,7 @@
 #define I2C_HEADER_IE_ENABLE			BIT(17)
 #define I2C_HEADER_REPEAT_START			BIT(16)
 #define I2C_HEADER_CONTINUE_XFER		BIT(15)
+#define I2C_HEADER_HS_MODE			BIT(22)
 #define I2C_HEADER_SLAVE_ADDR_SHIFT		1
 
 #define I2C_BUS_CLEAR_CNFG			0x084
@@ -198,6 +199,8 @@ enum msg_end_type {
  * @thigh_std_mode: High period of the clock in standard mode.
  * @tlow_fast_fastplus_mode: Low period of the clock in fast/fast-plus modes.
  * @thigh_fast_fastplus_mode: High period of the clock in fast/fast-plus modes.
+ * @tlow_hs_mode: Low period of the clock in HS mode.
+ * @thigh_hs_mode: High period of the clock in HS mode.
  * @setup_hold_time_std_mode: Setup and hold time for start and stop conditions
  *		in standard mode.
  * @setup_hold_time_fast_fast_plus_mode: Setup and hold time for start and stop
@@ -206,6 +209,7 @@ enum msg_end_type {
  *		in HS mode.
  * @has_interface_timing_reg: Has interface timing register to program the tuned
  *		timing settings.
+ * @has_hs_mode_support: Has support for high speed (HS) mode transfers.
  */
 struct tegra_i2c_hw_feature {
 	bool has_continue_xfer_support;
@@ -226,10 +230,13 @@ struct tegra_i2c_hw_feature {
 	u32 thigh_std_mode;
 	u32 tlow_fast_fastplus_mode;
 	u32 thigh_fast_fastplus_mode;
+	u32 tlow_hs_mode;
+	u32 thigh_hs_mode;
 	u32 setup_hold_time_std_mode;
 	u32 setup_hold_time_fast_fast_plus_mode;
 	u32 setup_hold_time_hs_mode;
 	bool has_interface_timing_reg;
+	bool has_hs_mode_support;
 };
 
 /**
@@ -706,6 +713,20 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 	if (i2c_dev->hw->has_interface_timing_reg && tsu_thd)
 		i2c_writel(i2c_dev, tsu_thd, I2C_INTERFACE_TIMING_1);
 
+	/* Write HS mode registers. These will get used only for HS mode*/
+	if (i2c_dev->hw->has_hs_mode_support) {
+		tlow = i2c_dev->hw->tlow_hs_mode;
+		thigh = i2c_dev->hw->thigh_hs_mode;
+		tsu_thd = i2c_dev->hw->setup_hold_time_hs_mode;
+
+		val = FIELD_PREP(I2C_HS_INTERFACE_TIMING_THIGH, thigh) |
+			FIELD_PREP(I2C_HS_INTERFACE_TIMING_TLOW, tlow);
+		i2c_writel(i2c_dev, val, I2C_HS_INTERFACE_TIMING_0);
+		i2c_writel(i2c_dev, tsu_thd, I2C_HS_INTERFACE_TIMING_1);
+	} else if (t->bus_freq_hz > I2C_MAX_FAST_MODE_PLUS_FREQ) {
+		t->bus_freq_hz = I2C_MAX_FAST_MODE_PLUS_FREQ;
+	}
+
 	clk_multiplier = (tlow + thigh + 2) * (non_hs_mode + 1);
 
 	err = clk_set_rate(i2c_dev->div_clk,
@@ -1203,6 +1224,9 @@ static void tegra_i2c_push_packet_header(struct tegra_i2c_dev *i2c_dev,
 	if (msg->flags & I2C_M_RD)
 		packet_header |= I2C_HEADER_READ;
 
+	if (i2c_dev->timings.bus_freq_hz > I2C_MAX_FAST_MODE_PLUS_FREQ)
+		packet_header |= I2C_HEADER_HS_MODE;
+
 	if (i2c_dev->dma_mode && !i2c_dev->msg_read)
 		*dma_buf++ = packet_header;
 	else
@@ -1637,10 +1661,13 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
 	.thigh_std_mode = 0x7,
 	.tlow_fast_fastplus_mode = 0x2,
 	.thigh_fast_fastplus_mode = 0x2,
+	.tlow_hs_mode = 0x8,
+	.thigh_hs_mode = 0x3,
 	.setup_hold_time_std_mode = 0x08080808,
 	.setup_hold_time_fast_fast_plus_mode = 0x02020202,
 	.setup_hold_time_hs_mode = 0x090909,
 	.has_interface_timing_reg = true,
+	.has_hs_mode_support = true,
 };
 
 static const struct of_device_id tegra_i2c_of_match[] = {
-- 
2.43.0

[PATCH v3 4/5] i2c: tegra: Add support for SW mutex register

[Kartik Rajput]

Add support for SW mutex register introduced in Tegra264 to provide
an option to share the interface between multiple firmwares and/or
VMs.

However, the hardware does not ensure any protection based on the
values. The driver/firmware should honor the peer who already holds
the mutex.

Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
---
v2 -> v3:
	* Update tegra_i2c_mutex_trylock and tegra_i2c_mutex_unlock to
	  use readl and writel APIs instead of i2c_readl and i2c_writel
	  which use relaxed APIs.
	* Use dev_warn instead of WARN_ON if mutex lock/unlock fails.
v1 -> v2:
	* Fixed typos.
	* Fix tegra_i2c_mutex_lock() logic.
	* Add a timeout in tegra_i2c_mutex_lock() instead of polling for
	  mutex indefinitely.
---
 drivers/i2c/busses/i2c-tegra.c | 137 +++++++++++++++++++++++++++++----
 1 file changed, 122 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index d0b6aa013c96..dae59e9e993b 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -137,6 +137,14 @@
 
 #define I2C_MASTER_RESET_CNTRL			0x0a8
 
+#define I2C_SW_MUTEX				0x0ec
+#define I2C_SW_MUTEX_REQUEST			GENMASK(3, 0)
+#define I2C_SW_MUTEX_GRANT			GENMASK(7, 4)
+#define I2C_SW_MUTEX_ID				9
+
+/* SW mutex acquire timeout value in milliseconds. */
+#define I2C_SW_MUTEX_TIMEOUT			25
+
 /* configuration load timeout in microseconds */
 #define I2C_CONFIG_LOAD_TIMEOUT			1000000
 
@@ -210,6 +218,7 @@ enum msg_end_type {
  * @has_interface_timing_reg: Has interface timing register to program the tuned
  *		timing settings.
  * @has_hs_mode_support: Has support for high speed (HS) mode transfers.
+ * @has_mutex: Has mutex register for mutual exclusion with other firmwares or VMs.
  */
 struct tegra_i2c_hw_feature {
 	bool has_continue_xfer_support;
@@ -237,6 +246,7 @@ struct tegra_i2c_hw_feature {
 	u32 setup_hold_time_hs_mode;
 	bool has_interface_timing_reg;
 	bool has_hs_mode_support;
+	bool has_mutex;
 };
 
 /**
@@ -380,6 +390,108 @@ static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
 	readsl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
 }
 
+static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
+				   u32 reg, u32 mask, u32 delay_us,
+				   u32 timeout_us)
+{
+	void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
+	u32 val;
+
+	if (!i2c_dev->atomic_mode)
+		return readl_relaxed_poll_timeout(addr, val, !(val & mask),
+						  delay_us, timeout_us);
+
+	return readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask),
+						 delay_us, timeout_us);
+}
+
+static int tegra_i2c_mutex_trylock(struct tegra_i2c_dev *i2c_dev)
+{
+	unsigned int reg = tegra_i2c_reg_addr(i2c_dev, I2C_SW_MUTEX);
+	u32 val, id;
+
+	val = readl(i2c_dev->base + reg);
+	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
+	if (id != 0 && id != I2C_SW_MUTEX_ID)
+		return 0;
+
+	val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID);
+	writel(val, i2c_dev->base + reg);
+
+	val = readl(i2c_dev->base + reg);
+	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
+
+	if (id != I2C_SW_MUTEX_ID)
+		return 0;
+
+	return 1;
+}
+
+static void tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev)
+{
+	unsigned int num_retries = I2C_SW_MUTEX_TIMEOUT;
+
+	/* Poll until mutex is acquired or timeout. */
+	while (--num_retries && !tegra_i2c_mutex_trylock(i2c_dev))
+		usleep_range(1000, 2000);
+
+	if (!num_retries)
+		dev_warn(i2c_dev->dev, "timeout while acquiring mutex, proceeding anyway\n");
+}
+
+static void tegra_i2c_mutex_unlock(struct tegra_i2c_dev *i2c_dev)
+{
+	unsigned int reg = tegra_i2c_reg_addr(i2c_dev, I2C_SW_MUTEX);
+	u32 val, id;
+
+	val = readl(i2c_dev->base + reg);
+	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
+
+	if (id != I2C_SW_MUTEX_ID) {
+		dev_warn(i2c_dev->dev, "unable to unlock mutex, mutex is owned by: %u\n", id);
+		return;
+	}
+
+	writel(0, i2c_dev->base + reg);
+}
+
+static void tegra_i2c_bus_lock(struct i2c_adapter *adapter,
+			       unsigned int flags)
+{
+	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter);
+
+	rt_mutex_lock_nested(&adapter->bus_lock, i2c_adapter_depth(adapter));
+	tegra_i2c_mutex_lock(i2c_dev);
+}
+
+static int tegra_i2c_bus_trylock(struct i2c_adapter *adapter,
+				  unsigned int flags)
+{
+	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter);
+	int ret;
+
+	ret = rt_mutex_trylock(&adapter->bus_lock);
+	if (ret)
+		ret = tegra_i2c_mutex_trylock(i2c_dev);
+
+	return ret;
+}
+
+static void tegra_i2c_bus_unlock(struct i2c_adapter *adapter,
+				 unsigned int flags)
+{
+	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter);
+
+	tegra_i2c_mutex_unlock(i2c_dev);
+	rt_mutex_unlock(&adapter->bus_lock);
+}
+
+static const struct i2c_lock_operations tegra_i2c_lock_ops = {
+	.lock_bus = tegra_i2c_bus_lock,
+	.trylock_bus = tegra_i2c_bus_trylock,
+	.unlock_bus = tegra_i2c_bus_unlock,
+};
+
 static void tegra_i2c_mask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask)
 {
 	u32 int_mask;
@@ -558,21 +670,6 @@ static void tegra_i2c_vi_init(struct tegra_i2c_dev *i2c_dev)
 	i2c_writel(i2c_dev, 0x0, I2C_TLOW_SEXT);
 }
 
-static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
-				   u32 reg, u32 mask, u32 delay_us,
-				   u32 timeout_us)
-{
-	void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
-	u32 val;
-
-	if (!i2c_dev->atomic_mode)
-		return readl_relaxed_poll_timeout(addr, val, !(val & mask),
-						  delay_us, timeout_us);
-
-	return readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask),
-						 delay_us, timeout_us);
-}
-
 static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
 {
 	u32 mask, val, offset;
@@ -1515,6 +1612,7 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
 	.setup_hold_time_fast_fast_plus_mode = 0x0,
 	.setup_hold_time_hs_mode = 0x0,
 	.has_interface_timing_reg = false,
+	.has_mutex = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
@@ -1540,6 +1638,7 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
 	.setup_hold_time_fast_fast_plus_mode = 0x0,
 	.setup_hold_time_hs_mode = 0x0,
 	.has_interface_timing_reg = false,
+	.has_mutex = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
@@ -1565,6 +1664,7 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
 	.setup_hold_time_fast_fast_plus_mode = 0x0,
 	.setup_hold_time_hs_mode = 0x0,
 	.has_interface_timing_reg = false,
+	.has_mutex = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
@@ -1590,6 +1690,7 @@ static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
 	.setup_hold_time_fast_fast_plus_mode = 0x0,
 	.setup_hold_time_hs_mode = 0x0,
 	.has_interface_timing_reg = true,
+	.has_mutex = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
@@ -1615,6 +1716,7 @@ static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
 	.setup_hold_time_fast_fast_plus_mode = 0,
 	.setup_hold_time_hs_mode = 0,
 	.has_interface_timing_reg = true,
+	.has_mutex = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
@@ -1640,6 +1742,7 @@ static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
 	.setup_hold_time_fast_fast_plus_mode = 0,
 	.setup_hold_time_hs_mode = 0,
 	.has_interface_timing_reg = true,
+	.has_mutex = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
@@ -1668,6 +1771,7 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
 	.setup_hold_time_hs_mode = 0x090909,
 	.has_interface_timing_reg = true,
 	.has_hs_mode_support = true,
+	.has_mutex = false,
 };
 
 static const struct of_device_id tegra_i2c_of_match[] = {
@@ -1871,6 +1975,9 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	i2c_dev->adapter.nr = pdev->id;
 	ACPI_COMPANION_SET(&i2c_dev->adapter.dev, ACPI_COMPANION(&pdev->dev));
 
+	if (i2c_dev->hw->has_mutex)
+		i2c_dev->adapter.lock_ops = &tegra_i2c_lock_ops;
+
 	if (i2c_dev->hw->supports_bus_clear)
 		i2c_dev->adapter.bus_recovery_info = &tegra_i2c_recovery_info;
 
-- 
2.43.0

[PATCH v3 5/5] i2c: tegra: Add Tegra264 support

[Kartik Rajput]

Add support for Tegra264 SoC which supports 17 generic I2C controllers,
two of which are in the AON (always-on) partition of the SoC. Tegra264
I2C supports all the features supported by Tegra194 I2C controllers.

Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
---
 drivers/i2c/busses/i2c-tegra.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index dae59e9e993b..6cdf44e7d3ca 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -1774,7 +1774,36 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
 	.has_mutex = false,
 };
 
+static const struct tegra_i2c_hw_feature tegra264_i2c_hw = {
+	.has_continue_xfer_support = true,
+	.has_per_pkt_xfer_complete_irq = true,
+	.clk_divisor_hs_mode = 1,
+	.clk_divisor_std_mode = 0x1d,
+	.clk_divisor_fast_mode = 0x15,
+	.clk_divisor_fast_plus_mode = 0x8,
+	.has_config_load_reg = true,
+	.has_multi_master_mode = true,
+	.has_slcg_override_reg = true,
+	.has_mst_fifo = true,
+	.quirks = &tegra194_i2c_quirks,
+	.supports_bus_clear = true,
+	.has_apb_dma = false,
+	.tlow_std_mode = 0x8,
+	.thigh_std_mode = 0x7,
+	.tlow_fast_fastplus_mode = 0x2,
+	.thigh_fast_fastplus_mode = 0x2,
+	.tlow_hs_mode = 0x4,
+	.thigh_hs_mode = 0x2,
+	.setup_hold_time_std_mode = 0x08080808,
+	.setup_hold_time_fast_fast_plus_mode = 0x02020202,
+	.setup_hold_time_hs_mode = 0x090909,
+	.has_interface_timing_reg = true,
+	.has_hs_mode_support = true,
+	.has_mutex = true,
+};
+
 static const struct of_device_id tegra_i2c_of_match[] = {
+	{ .compatible = "nvidia,tegra264-i2c", .data = &tegra264_i2c_hw, },
 	{ .compatible = "nvidia,tegra194-i2c", .data = &tegra194_i2c_hw, },
 	{ .compatible = "nvidia,tegra186-i2c", .data = &tegra186_i2c_hw, },
 #if IS_ENABLED(CONFIG_ARCH_TEGRA_210_SOC)
-- 
2.43.0

Re: [PATCH v3 1/5] dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C

[Kartik Rajput]

On Mon, 9 Jun 2025 15:04:16 +0530, Kartik Rajput wrote:
> iTegra264 has 17 generic I2C controllers, two of which are in always-on

There is a typo here, it should be Tegra264 not iTegra264.

> partition of the SoC. In addition to the features supported by Tegra194
> it also supports a SW mutex register to allow sharing the same I2C
> instance across multiple firmware.
> 
> Document compatible string "nvidia,tegra264-i2c" for Tegra264 I2C.
> 
> Signed-off-by: Kartik Rajput <kkartik@nvidia.com>

Thanks,
Kartik

Re: [PATCH v3 1/5] dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1900 bytes --]

On Mon, Jun 09, 2025 at 03:04:16PM +0530, Kartik Rajput wrote:
> iTegra264 has 17 generic I2C controllers, two of which are in always-on
> partition of the SoC. In addition to the features supported by Tegra194
> it also supports a SW mutex register to allow sharing the same I2C
> instance across multiple firmware.
> 
> Document compatible string "nvidia,tegra264-i2c" for Tegra264 I2C.
> 
> Signed-off-by: Kartik Rajput <kkartik@nvidia.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

> ---
> v2 -> v3:
> 	* Add constraints for "nvidia,tegra264-i2c".
> v1 -> v2:
> 	* Fixed typos.
> ---
>  .../devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml        | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
> index 6b6f6762d122..f0693b872cb6 100644
> --- a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
> @@ -80,6 +80,12 @@ properties:
>            support for 64 KiB transactions whereas earlier chips supported no
>            more than 4 KiB per transactions.
>          const: nvidia,tegra194-i2c
> +      - description:
> +          Tegra264 has 17 generic I2C controllers, two of which are in the AON
> +          (always-on) partition of the SoC. In addition to the features from
> +          Tegra194, a SW mutex register is added to support use of the same I2C
> +          instance across multiple firmwares.
> +        const: nvidia,tegra264-i2c
>  
>    reg:
>      maxItems: 1
> @@ -186,6 +192,7 @@ allOf:
>              contains:
>                enum:
>                  - nvidia,tegra194-i2c
> +                - nvidia,tegra264-i2c
>      then:
>        required:
>          - resets
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 4/5] i2c: tegra: Add support for SW mutex register

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5752 bytes --]

On Mon, Jun 09, 2025 at 03:04:19PM +0530, Kartik Rajput wrote:
> Add support for SW mutex register introduced in Tegra264 to provide
> an option to share the interface between multiple firmwares and/or
> VMs.
> 
> However, the hardware does not ensure any protection based on the
> values. The driver/firmware should honor the peer who already holds
> the mutex.
> 
> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> ---
> v2 -> v3:
> 	* Update tegra_i2c_mutex_trylock and tegra_i2c_mutex_unlock to
> 	  use readl and writel APIs instead of i2c_readl and i2c_writel
> 	  which use relaxed APIs.
> 	* Use dev_warn instead of WARN_ON if mutex lock/unlock fails.
> v1 -> v2:
> 	* Fixed typos.
> 	* Fix tegra_i2c_mutex_lock() logic.
> 	* Add a timeout in tegra_i2c_mutex_lock() instead of polling for
> 	  mutex indefinitely.
> ---
>  drivers/i2c/busses/i2c-tegra.c | 137 +++++++++++++++++++++++++++++----
>  1 file changed, 122 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index d0b6aa013c96..dae59e9e993b 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -137,6 +137,14 @@
>  
>  #define I2C_MASTER_RESET_CNTRL			0x0a8
>  
> +#define I2C_SW_MUTEX				0x0ec
> +#define I2C_SW_MUTEX_REQUEST			GENMASK(3, 0)
> +#define I2C_SW_MUTEX_GRANT			GENMASK(7, 4)
> +#define I2C_SW_MUTEX_ID				9

Maybe this should contain some sort of suffix to denote which ID this
is? Maybe I2C_SW_MUTEX_ID_CCPLEX?

> +
> +/* SW mutex acquire timeout value in milliseconds. */
> +#define I2C_SW_MUTEX_TIMEOUT			25
> +
>  /* configuration load timeout in microseconds */
>  #define I2C_CONFIG_LOAD_TIMEOUT			1000000
>  
> @@ -210,6 +218,7 @@ enum msg_end_type {
>   * @has_interface_timing_reg: Has interface timing register to program the tuned
>   *		timing settings.
>   * @has_hs_mode_support: Has support for high speed (HS) mode transfers.
> + * @has_mutex: Has mutex register for mutual exclusion with other firmwares or VMs.
>   */
>  struct tegra_i2c_hw_feature {
>  	bool has_continue_xfer_support;
> @@ -237,6 +246,7 @@ struct tegra_i2c_hw_feature {
>  	u32 setup_hold_time_hs_mode;
>  	bool has_interface_timing_reg;
>  	bool has_hs_mode_support;
> +	bool has_mutex;
>  };
>  
>  /**
> @@ -380,6 +390,108 @@ static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
>  	readsl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
>  }
>  
> +static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
> +				   u32 reg, u32 mask, u32 delay_us,
> +				   u32 timeout_us)
> +{
> +	void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
> +	u32 val;
> +
> +	if (!i2c_dev->atomic_mode)
> +		return readl_relaxed_poll_timeout(addr, val, !(val & mask),
> +						  delay_us, timeout_us);
> +
> +	return readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask),
> +						 delay_us, timeout_us);
> +}

The move of this function seems unnecessary.

> +
> +static int tegra_i2c_mutex_trylock(struct tegra_i2c_dev *i2c_dev)
> +{
> +	unsigned int reg = tegra_i2c_reg_addr(i2c_dev, I2C_SW_MUTEX);
> +	u32 val, id;
> +
> +	val = readl(i2c_dev->base + reg);
> +	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> +	if (id != 0 && id != I2C_SW_MUTEX_ID)
> +		return 0;
> +
> +	val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID);
> +	writel(val, i2c_dev->base + reg);
> +
> +	val = readl(i2c_dev->base + reg);
> +	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> +
> +	if (id != I2C_SW_MUTEX_ID)
> +		return 0;
> +
> +	return 1;
> +}

Do we need some sort of locking around these? Or is this always
guaranteed to be called from a locked region already?

> +
> +static void tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev)
> +{
> +	unsigned int num_retries = I2C_SW_MUTEX_TIMEOUT;
> +
> +	/* Poll until mutex is acquired or timeout. */
> +	while (--num_retries && !tegra_i2c_mutex_trylock(i2c_dev))
> +		usleep_range(1000, 2000);

Maybe this can be rewritten to be easier on the eye? Something like:

	while (--num_retries) {
		if (tegra_i2c_mutex_trylock(i2c_dev))
			break;

		usleep_range(1000, 2000);
	}

looks a bit easier to follow. It may also be better to change this to a
properly timed loop. As it is, the usleep_range() can take anywhere from
1 to 2 ms, so effectively I2C_SW_MUTEX_TIMEOUT 25 can be 50 ms long. I'm
sure that doesn't matter all that much, but it's a bit of an ambiguous
specification. So I think we should either be precise with a timed loop
if we specify the timeout in milliseconds or we should not pretend that
we care about the specific time and rename the variable to something
like I2C_SW_MUTEX_RETRIES instead. I prefer the timed loop variant, and
I think there's ways you can use helpers from linux/iopoll.h to achieve
this (i.e. use the generic read_poll_timeout() with
tegra_i2c_mutex_trylock as op parameter and passing i2c_dev as args).

> +
> +	if (!num_retries)
> +		dev_warn(i2c_dev->dev, "timeout while acquiring mutex, proceeding anyway\n");
> +}

I take it there's no way to refuse operations since there's no return
value for this function? I wonder if it's the right interface for this,
then. If there's no mechanism to enforce the lock in hardware, then we
must somehow respect it in software. Proceeding even after failing to
acquire the lock seems like a recipe for breaking things.

Also, this looks slightly wrong. What if the trylock succeeds on the
last retry? num_retries will have been decremented to 0 at that point,
so we'll see the warning even if it did succeed.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 5/5] i2c: tegra: Add Tegra264 support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 492 bytes --]

On Mon, Jun 09, 2025 at 03:04:20PM +0530, Kartik Rajput wrote:
> Add support for Tegra264 SoC which supports 17 generic I2C controllers,
> two of which are in the AON (always-on) partition of the SoC. Tegra264
> I2C supports all the features supported by Tegra194 I2C controllers.

Maybe mention here as well that there's an additional SW mutex feature?
It's not that big a deal, but since you already mention that it's
similar to Tegra194, might as well be as accurate as possible.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/5] i2c: tegra: Do not configure DMA if not supported

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2125 bytes --]

On Mon, Jun 09, 2025 at 03:04:17PM +0530, Kartik Rajput wrote:
> On Tegra264, not all I2C controllers have the necessary interface to
> GPC DMA, this causes failures when function tegra_i2c_init_dma()
> is called.
> 
> Ensure that "dmas" device-tree property is present before initializing
> DMA in function tegra_i2c_init_dma().
> 
> Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> ---
> v1 -> v2:
> 	* Update commit message to clarify that some I2C controllers may
> 	  not have the necessary interface to GPC DMA.
> ---
>  drivers/i2c/busses/i2c-tegra.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index ebd51165c46b..c7237d26b813 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -448,6 +448,9 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
>  	if (IS_VI(i2c_dev))
>  		return 0;
>  
> +	if (!device_property_present(i2c_dev->dev, "dmas"))
> +		return 0;

I know that you use the OF-independent variant here, but has this been
tested on ACPI?

Originally the intention behind this code was to get some sort of
validation of the DT (i.e. dmas property is desired, so we want to flag
if it isn't provided) with the fallback existing mostly just so things
can operate in the absence (or if APB/GPC DMA isn't available for some
reason).

If we now solely make this depend on the availability of the DT (or
ACPI) property, then we loose all of that validation. I suppose we have
DT schema to check for these kinds of things now, but since we're not
marking these properties as required, there's really no validation at
all anymore.

My concern is that if somebody's left out the dmas/dma-names properties
by accident, they may not get what they were asking for and we have no
hints to provide whatsoever. Maybe that's okay if we provide the base
DT, which has been unmodified for a while.

If that's what we want to do, it no longer makes sense to keep the
IS_VI() check above, though, because that's just redundant now.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/5] i2c: tegra: Do not configure DMA if not supported

[Kartik Rajput]

Thanks for reviewing the patch Thierry!

On Tue, 2025-06-10 at 10:28 +0200, Thierry Reding wrote:
> On Mon, Jun 09, 2025 at 03:04:17PM +0530, Kartik Rajput wrote:
> > On Tegra264, not all I2C controllers have the necessary interface
> > to
> > GPC DMA, this causes failures when function tegra_i2c_init_dma()
> > is called.
> > 
> > Ensure that "dmas" device-tree property is present before
> > initializing
> > DMA in function tegra_i2c_init_dma().
> > 
> > Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> > ---
> > v1 -> v2:
> > 	* Update commit message to clarify that some I2C
> > controllers may
> > 	  not have the necessary interface to GPC DMA.
> > ---
> >  drivers/i2c/busses/i2c-tegra.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-tegra.c
> > b/drivers/i2c/busses/i2c-tegra.c
> > index ebd51165c46b..c7237d26b813 100644
> > --- a/drivers/i2c/busses/i2c-tegra.c
> > +++ b/drivers/i2c/busses/i2c-tegra.c
> > @@ -448,6 +448,9 @@ static int tegra_i2c_init_dma(struct
> > tegra_i2c_dev *i2c_dev)
> >  	if (IS_VI(i2c_dev))
> >  		return 0;
> >  
> > +	if (!device_property_present(i2c_dev->dev, "dmas"))
> > +		return 0;
> 
> I know that you use the OF-independent variant here, but has this
> been
> tested on ACPI?

No, Tegra I2C driver does not support DMA with ACPI boot.

> 
> Originally the intention behind this code was to get some sort of
> validation of the DT (i.e. dmas property is desired, so we want to
> flag
> if it isn't provided) with the fallback existing mostly just so
> things
> can operate in the absence (or if APB/GPC DMA isn't available for
> some
> reason).
> 
> If we now solely make this depend on the availability of the DT (or
> ACPI) property, then we loose all of that validation. I suppose we
> have
> DT schema to check for these kinds of things now, but since we're not
> marking these properties as required, there's really no validation at
> all anymore.
> 
> My concern is that if somebody's left out the dmas/dma-names
> properties
> by accident, they may not get what they were asking for and we have
> no
> hints to provide whatsoever. Maybe that's okay if we provide the base
> DT, which has been unmodified for a while.

Should I add an info print here, to indicate that we are missing the
"dmas" property?

> 
> If that's what we want to do, it no longer makes sense to keep the
> IS_VI() check above, though, because that's just redundant now.

Ack, I will move this check above.

> 
> Thierry

Thanks & Regards,
Kartik

Re: [PATCH v3 4/5] i2c: tegra: Add support for SW mutex register

[Kartik Rajput]

On Tue, 2025-06-10 at 09:49 +0200, Thierry Reding wrote:
> On Mon, Jun 09, 2025 at 03:04:19PM +0530, Kartik Rajput wrote:
> > Add support for SW mutex register introduced in Tegra264 to provide
> > an option to share the interface between multiple firmwares and/or
> > VMs.
> > 
> > However, the hardware does not ensure any protection based on the
> > values. The driver/firmware should honor the peer who already holds
> > the mutex.
> > 
> > Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> > Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> > ---
> > v2 -> v3:
> > 	* Update tegra_i2c_mutex_trylock and
> > tegra_i2c_mutex_unlock to
> > 	  use readl and writel APIs instead of i2c_readl and
> > i2c_writel
> > 	  which use relaxed APIs.
> > 	* Use dev_warn instead of WARN_ON if mutex lock/unlock
> > fails.
> > v1 -> v2:
> > 	* Fixed typos.
> > 	* Fix tegra_i2c_mutex_lock() logic.
> > 	* Add a timeout in tegra_i2c_mutex_lock() instead of
> > polling for
> > 	  mutex indefinitely.
> > ---
> >  drivers/i2c/busses/i2c-tegra.c | 137
> > +++++++++++++++++++++++++++++----
> >  1 file changed, 122 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-tegra.c
> > b/drivers/i2c/busses/i2c-tegra.c
> > index d0b6aa013c96..dae59e9e993b 100644
> > --- a/drivers/i2c/busses/i2c-tegra.c
> > +++ b/drivers/i2c/busses/i2c-tegra.c
> > @@ -137,6 +137,14 @@
> >  
> >  #define I2C_MASTER_RESET_CNTRL			0x0a8
> >  
> > +#define I2C_SW_MUTEX				0x0ec
> > +#define I2C_SW_MUTEX_REQUEST			GENMASK(3, 0)
> > +#define I2C_SW_MUTEX_GRANT			GENMASK(7, 4)
> > +#define I2C_SW_MUTEX_ID				9
> 
> Maybe this should contain some sort of suffix to denote which ID this
> is? Maybe I2C_SW_MUTEX_ID_CCPLEX?

Ack, I2C_SW_MUTEX_ID_CCPLEX sounds good. I will update this in next
revision.

> 
> > +
> > +/* SW mutex acquire timeout value in milliseconds. */
> > +#define I2C_SW_MUTEX_TIMEOUT			25
> > +
> >  /* configuration load timeout in microseconds */
> >  #define I2C_CONFIG_LOAD_TIMEOUT			1000000
> >  
> > @@ -210,6 +218,7 @@ enum msg_end_type {
> >   * @has_interface_timing_reg: Has interface timing register to
> > program the tuned
> >   *		timing settings.
> >   * @has_hs_mode_support: Has support for high speed (HS) mode
> > transfers.
> > + * @has_mutex: Has mutex register for mutual exclusion with other
> > firmwares or VMs.
> >   */
> >  struct tegra_i2c_hw_feature {
> >  	bool has_continue_xfer_support;
> > @@ -237,6 +246,7 @@ struct tegra_i2c_hw_feature {
> >  	u32 setup_hold_time_hs_mode;
> >  	bool has_interface_timing_reg;
> >  	bool has_hs_mode_support;
> > +	bool has_mutex;
> >  };
> >  
> >  /**
> > @@ -380,6 +390,108 @@ static void i2c_readsl(struct tegra_i2c_dev
> > *i2c_dev, void *data,
> >  	readsl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg),
> > data, len);
> >  }
> >  
> > +static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
> > +				   u32 reg, u32 mask, u32
> > delay_us,
> > +				   u32 timeout_us)
> > +{
> > +	void __iomem *addr = i2c_dev->base +
> > tegra_i2c_reg_addr(i2c_dev, reg);
> > +	u32 val;
> > +
> > +	if (!i2c_dev->atomic_mode)
> > +		return readl_relaxed_poll_timeout(addr, val, !(val
> > & mask),
> > +						  delay_us,
> > timeout_us);
> > +
> > +	return readl_relaxed_poll_timeout_atomic(addr, val, !(val
> > & mask),
> > +						 delay_us,
> > timeout_us);
> > +}
> 
> The move of this function seems unnecessary.

Ack, I will fix this in the next revision.

> 
> > +
> > +static int tegra_i2c_mutex_trylock(struct tegra_i2c_dev *i2c_dev)
> > +{
> > +	unsigned int reg = tegra_i2c_reg_addr(i2c_dev,
> > I2C_SW_MUTEX);
> > +	u32 val, id;
> > +
> > +	val = readl(i2c_dev->base + reg);
> > +	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> > +	if (id != 0 && id != I2C_SW_MUTEX_ID)
> > +		return 0;
> > +
> > +	val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID);
> > +	writel(val, i2c_dev->base + reg);
> > +
> > +	val = readl(i2c_dev->base + reg);
> > +	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> > +
> > +	if (id != I2C_SW_MUTEX_ID)
> > +		return 0;
> > +
> > +	return 1;
> > +}
> 
> Do we need some sort of locking around these? Or is this always
> guaranteed to be called from a locked region already?

This is currently called from locked region. But, since we plan to move
these APIs out of bus lock/unlock operations we should add a lock
around these.

> 
> > +
> > +static void tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev)
> > +{
> > +	unsigned int num_retries = I2C_SW_MUTEX_TIMEOUT;
> > +
> > +	/* Poll until mutex is acquired or timeout. */
> > +	while (--num_retries && !tegra_i2c_mutex_trylock(i2c_dev))
> > +		usleep_range(1000, 2000);
> 
> Maybe this can be rewritten to be easier on the eye? Something like:
> 
> 	while (--num_retries) {
> 		if (tegra_i2c_mutex_trylock(i2c_dev))
> 			break;
> 
> 		usleep_range(1000, 2000);
> 	}
> 
> looks a bit easier to follow. It may also be better to change this to
> a
> properly timed loop. As it is, the usleep_range() can take anywhere
> from
> 1 to 2 ms, so effectively I2C_SW_MUTEX_TIMEOUT 25 can be 50 ms long.
> I'm
> sure that doesn't matter all that much, but it's a bit of an
> ambiguous
> specification. So I think we should either be precise with a timed
> loop
> if we specify the timeout in milliseconds or we should not pretend
> that
> we care about the specific time and rename the variable to something
> like I2C_SW_MUTEX_RETRIES instead. I prefer the timed loop variant,
> and
> I think there's ways you can use helpers from linux/iopoll.h to
> achieve
> this (i.e. use the generic read_poll_timeout() with
> tegra_i2c_mutex_trylock as op parameter and passing i2c_dev as args).

I will update the implementation to use read_poll_timeout() to achieve
an accurate timeout here

> 
> > +
> > +	if (!num_retries)
> > +		dev_warn(i2c_dev->dev, "timeout while acquiring
> > mutex, proceeding anyway\n");
> > +}
> 
> I take it there's no way to refuse operations since there's no return
> value for this function? I wonder if it's the right interface for
> this,
> then. If there's no mechanism to enforce the lock in hardware, then
> we
> must somehow respect it in software. Proceeding even after failing to
> acquire the lock seems like a recipe for breaking things.

Should I move the lock/unlock operations to
tegra_i2c_runtime_resume/suspend functions?

That way we can propagate the error correctly and fail in case we do
not acquire the mutex.

> 
> Also, this looks slightly wrong. What if the trylock succeeds on the
> last retry? num_retries will have been decremented to 0 at that
> point,
> so we'll see the warning even if it did succeed.

You are correct, ideally we should check I2C_SW_MUTEX register to know
if the mutex has been acquired or not.
I will fix this in the next revision.

> 
> Thierry

Thanks & Regards,
Kartik

Re: [PATCH v3 5/5] i2c: tegra: Add Tegra264 support

[Kartik Rajput]

On Tue, 2025-06-10 at 09:53 +0200, Thierry Reding wrote:
> On Mon, Jun 09, 2025 at 03:04:20PM +0530, Kartik Rajput wrote:
> > Add support for Tegra264 SoC which supports 17 generic I2C
> > controllers,
> > two of which are in the AON (always-on) partition of the SoC.
> > Tegra264
> > I2C supports all the features supported by Tegra194 I2C
> > controllers.
> 
> Maybe mention here as well that there's an additional SW mutex
> feature?
> It's not that big a deal, but since you already mention that it's
> similar to Tegra194, might as well be as accurate as possible.

Ack, I will update the commit description in the next revision.

> 
> Thierry

Thanks & Regards,
Kartik

Re: [PATCH v3 2/5] i2c: tegra: Do not configure DMA if not supported

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3424 bytes --]

On Mon, Jun 16, 2025 at 10:01:39AM +0000, Kartik Rajput wrote:
> Thanks for reviewing the patch Thierry!
> 
> On Tue, 2025-06-10 at 10:28 +0200, Thierry Reding wrote:
> > On Mon, Jun 09, 2025 at 03:04:17PM +0530, Kartik Rajput wrote:
> > > On Tegra264, not all I2C controllers have the necessary interface
> > > to
> > > GPC DMA, this causes failures when function tegra_i2c_init_dma()
> > > is called.
> > > 
> > > Ensure that "dmas" device-tree property is present before
> > > initializing
> > > DMA in function tegra_i2c_init_dma().
> > > 
> > > Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> > > ---
> > > v1 -> v2:
> > > 	* Update commit message to clarify that some I2C
> > > controllers may
> > > 	  not have the necessary interface to GPC DMA.
> > > ---
> > >  drivers/i2c/busses/i2c-tegra.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-tegra.c
> > > b/drivers/i2c/busses/i2c-tegra.c
> > > index ebd51165c46b..c7237d26b813 100644
> > > --- a/drivers/i2c/busses/i2c-tegra.c
> > > +++ b/drivers/i2c/busses/i2c-tegra.c
> > > @@ -448,6 +448,9 @@ static int tegra_i2c_init_dma(struct
> > > tegra_i2c_dev *i2c_dev)
> > >  	if (IS_VI(i2c_dev))
> > >  		return 0;
> > >  
> > > +	if (!device_property_present(i2c_dev->dev, "dmas"))
> > > +		return 0;
> > 
> > I know that you use the OF-independent variant here, but has this
> > been
> > tested on ACPI?
> 
> No, Tegra I2C driver does not support DMA with ACPI boot.

Maybe in that case we should just use the of_property_present() here?
It's not a big deal, but it could help point out that this is only meant
to work with DT.

> > Originally the intention behind this code was to get some sort of
> > validation of the DT (i.e. dmas property is desired, so we want to
> > flag
> > if it isn't provided) with the fallback existing mostly just so
> > things
> > can operate in the absence (or if APB/GPC DMA isn't available for
> > some
> > reason).
> > 
> > If we now solely make this depend on the availability of the DT (or
> > ACPI) property, then we loose all of that validation. I suppose we
> > have
> > DT schema to check for these kinds of things now, but since we're not
> > marking these properties as required, there's really no validation at
> > all anymore.
> > 
> > My concern is that if somebody's left out the dmas/dma-names
> > properties
> > by accident, they may not get what they were asking for and we have
> > no
> > hints to provide whatsoever. Maybe that's okay if we provide the base
> > DT, which has been unmodified for a while.
> 
> Should I add an info print here, to indicate that we are missing the
> "dmas" property?

That's not ideal either, because it would cause a print about missing
DMA for ACPI where DMA is expected not to work. And it will cause the
messages to be printed for I2C controllers that don't have the GPC DMA
interface.

Properly dealing with this would mean annotating each instance so we
know when this should be an error/warning. I don't think it's worth it
at this point. People will probably rely on the upstream DTSI file for
their needs anyway and we've got the dmas properties there. I think it
can be left as-is.

Or maybe we can add a dev_dbg() message to point this out, that's better
than nothing at all and shouldn't confuse people unnecessarily.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 4/5] i2c: tegra: Add support for SW mutex register

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]

On Mon, Jun 16, 2025 at 10:25:18AM +0000, Kartik Rajput wrote:
[...]
> > > diff --git a/drivers/i2c/busses/i2c-tegra.c
[...]
> > > +	if (!num_retries)
> > > +		dev_warn(i2c_dev->dev, "timeout while acquiring
> > > mutex, proceeding anyway\n");
> > > +}
> > 
> > I take it there's no way to refuse operations since there's no return
> > value for this function? I wonder if it's the right interface for
> > this,
> > then. If there's no mechanism to enforce the lock in hardware, then
> > we
> > must somehow respect it in software. Proceeding even after failing to
> > acquire the lock seems like a recipe for breaking things.
> 
> Should I move the lock/unlock operations to
> tegra_i2c_runtime_resume/suspend functions?
> 
> That way we can propagate the error correctly and fail in case we do
> not acquire the mutex.

suspend/resume isn't the proper place for this. On one hand, returning
an error from suspend/resume typically indicates that something went
wrong with the power management bits, and the SW mutex is not that.

Also, runtime PM support can be disabled, so those callbacks are not
guaranteed to be called in the proper sequence.

I think we want to add calls for these to tegra_i2c_xfer(), after the
pm_runtime_get_sync() and before pm_runtime_put(), respectively.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]