Message-ID: <c4e51677c889e74dc3d138b436af640c126ca613.camel@nvidia.com>
Date: Mon, 16 Jun 2025 10:25:18 +0000
From: Kartik Rajput <kkartik@...dia.com>
To: "thierry.reding@...il.com" <thierry.reding@...il.com>
CC: Laxman Dewangan <ldewangan@...dia.com>, Jon Hunter <jonathanh@...dia.com>,
	Akhil R <akhilrajeev@...dia.com>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "robh@...nel.org" <robh@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "andi.shyti@...nel.org"
	<andi.shyti@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>, "digetx@...il.com"
	<digetx@...il.com>, "linux-tegra@...r.kernel.org"
	<linux-tegra@...r.kernel.org>
Subject: Re: [PATCH v3 4/5] i2c: tegra: Add support for SW mutex register

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@...dia.com>
> > Signed-off-by: Kartik Rajput <kkartik@...dia.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

Powered by blists - more mailing lists