diff mbox series

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

Message ID 20250130143424.52389-5-kkartik@nvidia.com
State New
Headers show
Series Add I2C support for Tegra264 | expand

Commit Message

Kartik Rajput Jan. 30, 2025, 2:34 p.m. UTC 
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(-)

Comments

Krzysztof Kozlowski Jan. 30, 2025, 5:49 p.m. UTC | #1 
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
Kartik Rajput Jan. 31, 2025, 6:46 a.m. UTC | #2 
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
diff mbox series

Patch

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;