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

[Kartik Rajput <kkartik@nvidia.com>]

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:
>> 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.

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

>> +{
>> +     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.

>> +}
>> +
>> +static int tegra_i2c_mutex_trylock(struct tegra_i2c_dev *i2c_dev)
> 
> I think this can be bool.
> 

Ack.

>> +{
>> +     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.

>> +     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;
}

> 
>> +
>> +     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.

>> +     }
>> +
>> +     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.

>> +     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.

>> +             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.

>> +     }
>> +
>>        pm_runtime_put(i2c_dev->dev);
>>
>>        return ret ?: i;

Thanks & Regards,
Kartik