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

Hi Andi,

Thanks for reviewing the patch!

On 05/09/25 04:58, Andi Shyti wrote:

External email: Use caution opening links or attachments


Hi Kartik,

On Thu, Aug 28, 2025 at 11:29:32AM +0530, Kartik Rajput wrote:

quoted

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

You could add a short description on how to use the mutex
register here.

Ack.

quoted

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: Kartik Rajput <redacted>
Signed-off-by: Akhil R <akhilrajeev@nvidia.com>

...

quoted

@@ -381,6 +391,73 @@ 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_mutex_acquired(struct tegra_i2c_dev *i2c_dev)

this is a bool function.

Ack.

quoted

+{
+     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_CCPLEX)
+             return 0;
+
+     return 1;

return id != I2C_SW_MUTEX_ID_CCPLEX;

Ack.

quoted

+}
+
+static int tegra_i2c_mutex_trylock(struct tegra_i2c_dev *i2c_dev)

I think this can be bool.

Ack.

quoted

+{
+     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_CCPLEX)
+             return 0;
+
+     val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID_CCPLEX);
+     writel(val, i2c_dev->base + reg);
+
+     return tegra_i2c_mutex_acquired(i2c_dev);
+}
+
+static int tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev)
+{
+     int locked;

I guess this can be bool.

Ack.

quoted

+     int ret;
+
+     if (i2c_dev->atomic_mode)
+             ret = read_poll_timeout_atomic(tegra_i2c_mutex_trylock, locked, locked,
+                                            USEC_PER_MSEC, I2C_SW_MUTEX_TIMEOUT_US,
+                                            false, i2c_dev);
+     else
+             ret = read_poll_timeout(tegra_i2c_mutex_trylock, locked, locked, USEC_PER_MSEC,
+                                     I2C_SW_MUTEX_TIMEOUT_US, false, i2c_dev);
+
+     if (!tegra_i2c_mutex_acquired(i2c_dev))
+             dev_warn(i2c_dev->dev, "failed to acquire mutex\n");

I would try a few times before giving up.

Yes, we are retrying with read_poll_timeout_* calls.

Besides, is there a chance where ret is '0' and the mutex is not

In case of failure ret should always be set to -ETIMEDOUT.

acquired? If so, we are not signalling error if the mutex is not
acquired, but I think we should.

I would do:

         if (...)
                 ret = ...
         else
                 ret = ...

         if (ret)
                 return ret;

         if (!tegra_i2c_mutex_acquired(i2c_dev)) {
                 dev_warn(i2c_dev->dev, "failed to acquire mutex\n");
                 return -ESOMETHING;
         }

         return 0;

Makes sense?

I agree, we can simplify this logic by just checking the value of ret instead
of calling tegra_i2c_mutex_acquired() again as tegra_i2c_mutex_trylock() already
checks if we have acquired the mutex or not.


	...

	if (ret)
		dev_warn(i2c_dev->dev, "failed to acquire mutex\n");

	return ret;
}

quoted

+
+     return ret;
+}
+
+static int 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 && id != I2C_SW_MUTEX_ID_CCPLEX) {
+             dev_warn(i2c_dev->dev, "unable to unlock mutex, mutex is owned by: %u\n", id);
+             return -EPERM;

I would try a few times before giving up.

Unlocking the mutex should fail if unlock is called and CCPLEX is not the owner.
In that case no need to retry.

quoted

+     }
+
+     writel(0, i2c_dev->base + reg);
+
+     return 0;
+}
+
  static void tegra_i2c_mask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask)
  {
       u32 int_mask;

@@ -1422,6 +1499,13 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
               return ret;
       }

+

no need for this extra blank line.

Ack.

quoted

+     if (i2c_dev->hw->has_mutex) {

I would put this check in tegra_i2c_mutex_lock() and _unlock() in
order to avoid two level indentation here.

Ack. It will look cleaner if the check is moved inside the lock and unlock functions.

quoted

+             ret = tegra_i2c_mutex_lock(i2c_dev);
+             if (ret)
+                     return ret;
+     }
+
       for (i = 0; i < num; i++) {
               enum msg_end_type end_type = MSG_END_STOP;

@@ -1451,6 +1535,12 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
                       break;
       }

+     if (i2c_dev->hw->has_mutex) {
+             ret = tegra_i2c_mutex_unlock(i2c_dev);
+             if (ret)
+                     return ret;

We are skipping pm_runtime_put(), though.

Thanks,
Andi

Yes, we skip calling pm_runtime_put() if unlocking fails. I will fix this in the next revision.

quoted

+     }
+
       pm_runtime_put(i2c_dev->dev);

       return ret ?: i;

Thanks & Regards,
Kartik