Thanks for reviewing the patch Krzysztof!
On Thu, 2025-01-30 at 17:12 +0100, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
>
>
> On 30/01/2025 15:34, Kartik Rajput wrote:
> > From: Akhil R <akhilrajeev@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Kartik Rajput <kkartik@xxxxxxxxxx>
> > ---
> > 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.
>
The I2C_SW_MUTEX_GRANT field reflects the id of the current mutex
owner. The I2C_SW_MUTEX_GRANT field does not change with overwrites to
the I2C_SW_MUTEX_REQUEST field, unless I2C_SW_MUTEX_REQUEST field is
cleared.
> > +
> > + 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.
>
>
Ack. Will change this to dev_warn in v3.
> > +}
> > +
> > +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.
>
Ack.
>
>
> Best regards,
> Krzysztof
Thanks & Regards,
Kartik
