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

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

Akhil R (3):
  i2c: tegra: Add HS mode support
  i2c: tegra: Add support for SW mutex register
  i2c: tegra: Add Tegra264 support

Kartik Rajput (2):
  dt-bindings: i2c: nvidia,tegra20-i2c: Document Tegra264 I2C
  i2c: tegra: Do not configure DMA if not supported

 .../bindings/i2c/nvidia,tegra20-i2c.yaml      |   6 +
 drivers/i2c/busses/i2c-tegra.c                | 189 ++++++++++++++++--
 2 files changed, 180 insertions(+), 15 deletions(-)

---
v1 -> v2:
	* Rearranged the series.
---

-- 
2.43.0

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

[Kartik Rajput]

Tegra264 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>
---
v1 -> v2:
	* Fixed typos.
---
 .../devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml         | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
index b57ae6963e62..89138384517e 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
-- 
2.43.0

[PATCH v2 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 87976e99e6d0..b0dd129714a2 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -442,6 +442,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 v2 3/5] i2c: tegra: Add HS mode support

[Kartik Rajput]

From: Akhil R <akhilrajeev@nvidia.com>

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>
---
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 | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index b0dd129714a2..7c8b76406e2e 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
@@ -201,6 +202,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;
@@ -220,10 +222,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;
 };
 
 /**
@@ -684,6 +689,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,
@@ -1181,6 +1200,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
@@ -1621,10 +1643,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 v2 4/5] i2c: tegra: Add support for SW mutex register

[Kartik Rajput]

From: Akhil R <akhilrajeev@nvidia.com>

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>
---
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 | 132 +++++++++++++++++++++++++++++----
 1 file changed, 117 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 7c8b76406e2e..aa92faa6f5cb 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -135,6 +135,14 @@
 #define I2C_MST_FIFO_STATUS_TX			GENMASK(23, 16)
 #define I2C_MST_FIFO_STATUS_RX			GENMASK(7, 0)
 
+#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
 
@@ -203,6 +211,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 VM.
  */
 struct tegra_i2c_hw_feature {
 	bool has_continue_xfer_support;
@@ -229,6 +238,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;
 };
 
 /**
@@ -372,6 +382,103 @@ 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)
+{
+	u32 val, id;
+
+	val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
+	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);
+	i2c_writel(i2c_dev, val, I2C_SW_MUTEX);
+
+	val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
+	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);
+
+	WARN_ON(!num_retries);
+}
+
+static void tegra_i2c_mutex_unlock(struct tegra_i2c_dev *i2c_dev)
+{
+	u32 val, id;
+
+	val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
+	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
+
+	if (WARN_ON(id != I2C_SW_MUTEX_ID))
+		return;
+
+	i2c_writel(i2c_dev, 0, I2C_SW_MUTEX);
+}
+
+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);
+
+	rt_mutex_unlock(&adapter->bus_lock);
+	tegra_i2c_mutex_unlock(i2c_dev);
+}
+
+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;
@@ -550,21 +657,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;
@@ -1503,6 +1595,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 = {
@@ -1527,6 +1620,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 = {
@@ -1551,6 +1645,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 = {
@@ -1575,6 +1670,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 = {
@@ -1599,6 +1695,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 = {
@@ -1623,6 +1720,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 = {
@@ -1650,6 +1748,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[] = {
@@ -1853,6 +1952,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 v2 5/5] i2c: tegra: Add Tegra264 support

[Kartik Rajput]

From: Akhil R <akhilrajeev@nvidia.com>

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 aa92faa6f5cb..415337e069f5 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -1751,7 +1751,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 v2 4/5] i2c: tegra: Add support for SW mutex register

[Krzysztof Kozlowski]

On 30/01/2025 15:34, Kartik Rajput wrote:
> From: Akhil R <akhilrajeev@nvidia.com>
> 
> 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>
> ---
> 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 | 132 +++++++++++++++++++++++++++++----
>  1 file changed, 117 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 7c8b76406e2e..aa92faa6f5cb 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -135,6 +135,14 @@
>  #define I2C_MST_FIFO_STATUS_TX			GENMASK(23, 16)
>  #define I2C_MST_FIFO_STATUS_RX			GENMASK(7, 0)
>  
> +#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
>  
> @@ -203,6 +211,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 VM.
>   */
>  struct tegra_i2c_hw_feature {
>  	bool has_continue_xfer_support;
> @@ -229,6 +238,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;
>  };
>  
>  /**
> @@ -372,6 +382,103 @@ 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)
> +{
> +	u32 val, id;
> +
> +	val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
> +	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);
> +	i2c_writel(i2c_dev, val, I2C_SW_MUTEX);

And how do you exactly prevent concurrent, overwriting write? This looks
like pure race.

> +
> +	val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
> +	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);
> +
> +	WARN_ON(!num_retries);


Blocked thread is not a reason to reboot entire system (see panic on
warn). Drop or change to some dev_warn.


> +}
> +
> +static void tegra_i2c_mutex_unlock(struct tegra_i2c_dev *i2c_dev)
> +{
> +	u32 val, id;
> +
> +	val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
> +	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> +
> +	if (WARN_ON(id != I2C_SW_MUTEX_ID))

Same problem here.



Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 30/01/2025 15:34, Kartik Rajput wrote:
> Tegra264 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>

You did not update the constraints.

Best regards,
Krzysztof

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

[Kartik Rajput]

Thanks for reviewing the patch Krzysztof!

On Thu, 2025-01-30 at 17:12 +0100, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 30/01/2025 15:34, Kartik Rajput wrote:
> > From: Akhil R <akhilrajeev@nvidia.com>
> > 
> > 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>
> > ---
> > 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 | 132
> > +++++++++++++++++++++++++++++----
> >  1 file changed, 117 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-tegra.c
> > b/drivers/i2c/busses/i2c-tegra.c
> > index 7c8b76406e2e..aa92faa6f5cb 100644
> > --- a/drivers/i2c/busses/i2c-tegra.c
> > +++ b/drivers/i2c/busses/i2c-tegra.c
> > @@ -135,6 +135,14 @@
> >  #define I2C_MST_FIFO_STATUS_TX                       GENMASK(23,
> > 16)
> >  #define I2C_MST_FIFO_STATUS_RX                       GENMASK(7, 0)
> > 
> > +#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
> > 
> > @@ -203,6 +211,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 VM.
> >   */
> >  struct tegra_i2c_hw_feature {
> >       bool has_continue_xfer_support;
> > @@ -229,6 +238,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;
> >  };
> > 
> >  /**
> > @@ -372,6 +382,103 @@ 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)
> > +{
> > +     u32 val, id;
> > +
> > +     val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
> > +     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);
> > +     i2c_writel(i2c_dev, val, I2C_SW_MUTEX);
> 
> And how do you exactly prevent concurrent, overwriting write? This
> looks
> like pure race.
> 

The I2C_SW_MUTEX_GRANT field reflects the id of the current mutex
owner. The I2C_SW_MUTEX_GRANT field does not change with overwrites to
the I2C_SW_MUTEX_REQUEST field, unless I2C_SW_MUTEX_REQUEST field is
cleared.

> > +
> > +     val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
> > +     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);
> > +
> > +     WARN_ON(!num_retries);
> 
> 
> Blocked thread is not a reason to reboot entire system (see panic on
> warn). Drop or change to some dev_warn.
> 
> 

Ack. Will change this to dev_warn in v3.

> > +}
> > +
> > +static void tegra_i2c_mutex_unlock(struct tegra_i2c_dev *i2c_dev)
> > +{
> > +     u32 val, id;
> > +
> > +     val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
> > +     id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> > +
> > +     if (WARN_ON(id != I2C_SW_MUTEX_ID))
> 
> Same problem here.
> 

Ack.

> 
> 
> Best regards,
> Krzysztof

Thanks & Regards,
Kartik

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

[Krzysztof Kozlowski]

On 30/01/2025 17:35, Kartik Rajput wrote:
>>>  /**
>>> @@ -372,6 +382,103 @@ 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)
>>> +{
>>> +     u32 val, id;
>>> +
>>> +     val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
>>> +     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);
>>> +     i2c_writel(i2c_dev, val, I2C_SW_MUTEX);
>>
>> And how do you exactly prevent concurrent, overwriting write? This
>> looks
>> like pure race.
>>
> 
> The I2C_SW_MUTEX_GRANT field reflects the id of the current mutex
> owner. The I2C_SW_MUTEX_GRANT field does not change with overwrites to
> the I2C_SW_MUTEX_REQUEST field, unless I2C_SW_MUTEX_REQUEST field is
> cleared.


So second concurrent write to I2C_SW_MUTEX_REQUEST will fail silently,
and you rely on below check which ID succeeded to write?

If that is how it works, then should succeed... except the trouble is
that you use here i2c_readl/writel wrappers (which was already a poor
idea, because it hides the implementation for no real gain) and it turns
out they happen to be relaxed making all your assumptions about ordering
inaccurate. You need to switch to non-relaxed API.


Best regards,
Krzysztof

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

[Kartik Rajput]

On Thu, 2025-01-30 at 18:49 +0100, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 30/01/2025 17:35, Kartik Rajput wrote:
> > > >  /**
> > > > @@ -372,6 +382,103 @@ 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)
> > > > +{
> > > > +     u32 val, id;
> > > > +
> > > > +     val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
> > > > +     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);
> > > > +     i2c_writel(i2c_dev, val, I2C_SW_MUTEX);
> > > 
> > > And how do you exactly prevent concurrent, overwriting write?
> > > This
> > > looks
> > > like pure race.
> > > 
> > 
> > The I2C_SW_MUTEX_GRANT field reflects the id of the current mutex
> > owner. The I2C_SW_MUTEX_GRANT field does not change with overwrites
> > to
> > the I2C_SW_MUTEX_REQUEST field, unless I2C_SW_MUTEX_REQUEST field
> > is
> > cleared.
> 
> 
> So second concurrent write to I2C_SW_MUTEX_REQUEST will fail
> silently,
> and you rely on below check which ID succeeded to write?
> 

Correct.

> If that is how it works, then should succeed... except the trouble is
> that you use here i2c_readl/writel wrappers (which was already a poor
> idea, because it hides the implementation for no real gain) and it
> turns
> out they happen to be relaxed making all your assumptions about
> ordering
> inaccurate. You need to switch to non-relaxed API.
> 

Ack. I will update the implementation to use non-relaxed APIs instead.

> 
> Best regards,
> Krzysztof

Thanks & Regards,
Kartik