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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 30/01/2025 15:34, Kartik Rajput wrote:
> 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(-)
> 
> 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);

And how do you exactly prevent concurrent, overwriting write? This looks
like pure race.

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


Blocked thread is not a reason to reboot entire system (see panic on
warn). Drop or change to some dev_warn.


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

Same problem here.



Best regards,
Krzysztof