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

Kartik Rajput
2025-06-16T10:25:22Z

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