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

Kartik Rajput
2025-01-15T11:39:21Z

Thanks for reviewing the patch Thierry!

On Wed, 2025-01-08 at 18:01 +0100, Thierry Reding wrote:
> On Wed, Jan 08, 2025 at 04:36:19PM +0530, Kartik Rajput wrote:
> > From: Akhil R <akhilrajeev@nvidia.com>
> >
> > Add support for SW Mutex register introduced in Tegra264 to provide
>
> The spelling is a bit inconsistent. Earlier you referred to this as
> SW
> MUTEX register, which makes sense if that's what it's called. But
> then
> you call it "SW Mutex" register here. If you don't want to refer to
> it
> by the documented name, it should probably be "SW mutex" instead.
>
> > an option to share the interface between multiple firmware and/or
>
> "firmwares"
>
> > Virtual Machines.
>
> "virtual machines" or "VMs"
>

ACK. I will fix this in the next patchset.

> > 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>
> > ---
> >  drivers/i2c/busses/i2c-tegra.c | 126
> > +++++++++++++++++++++++++++++----
> >  1 file changed, 111 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-tegra.c
> > b/drivers/i2c/busses/i2c-tegra.c
> > index cf05937cb826..a5974af5b1af 100644
> > --- a/drivers/i2c/busses/i2c-tegra.c
> > +++ b/drivers/i2c/busses/i2c-tegra.c
> > @@ -135,6 +135,11 @@
> >  #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
> > +
> >  /* configuration load timeout in microseconds */
> >  #define I2C_CONFIG_LOAD_TIMEOUT                        1000000
> >
> > @@ -202,6 +207,7 @@ enum msg_end_type {
> >   *             in HS mode.
> >   * @has_interface_timing_reg: Has interface timing register to
> > program the tuned
> >   *             timing settings.
> > + * @has_mutex: Has Mutex register for mutual exclusion with other
> > firmwares or VM.
>
> "mutex"
>
> >   */
> >  struct tegra_i2c_hw_feature {
> >         bool has_continue_xfer_support;
> > @@ -228,6 +234,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;
> >  };
> >
> >  /**
> > @@ -371,6 +378,99 @@ 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)
> > +               return 0;
> > +
> > +       val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID);
> > +       i2c_writel(i2c_dev, val, I2C_SW_MUTEX);
> > +
> > +       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)
> > +{
> > +       /* Poll until mutex is acquired. */
> > +       while (tegra_i2c_mutex_trylock(i2c_dev))
> > +               cpu_relax();
> > +}
>
> Don't we want to use a timeout here? Otherwise we risk blocking the
> thread that this runs on if some firmware decides not to release the
> mutex.
>

We can continue to access the controller with a warning in case
the request times out.

Something like this?

static void tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev)
{
unsigned int num_retries = 25; // Move this to a macro.

/* Poll until mutex is acquired or timeout. */
while ( --num_retries && !tegra_i2c_mutex_trylock(i2c_dev))
msleep(1);

WARN_ON(!num_retries);
}

> Also, is the logic not the wrong way around? I.e. trylock returns
> true
> if the hardware mutex was successfully locked, in which case it
> doesn't
> make sense to keep spinning, right? Or do I misunderstand how this
> works?
>
> Thierry
>

The logic is indeed wrong here, apologies for the oversight. I will fix
this in the next patchset.

> > +
> > +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))
> > +               return;
> > +
> > +       i2c_writel(i2c_dev, 0, I2C_SW_MUTEX);
> > +}
> > +
> > +static void tegra_i2c_bus_lock(struct i2c_adapter *adapter,
> > +                              unsigned int flags)
> > +{
> > +       struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter);
> > +
> > +       rt_mutex_lock_nested(&adapter->bus_lock,
> > i2c_adapter_depth(adapter));
> > +       tegra_i2c_mutex_lock(i2c_dev);
> > +}
> > +
> > +static int tegra_i2c_bus_trylock(struct i2c_adapter *adapter,
> > +                                 unsigned int flags)
> > +{
> > +       struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter);
> > +       int ret;
> > +
> > +       ret = rt_mutex_trylock(&adapter->bus_lock);
> > +       if (ret)
> > +               ret = tegra_i2c_mutex_trylock(i2c_dev);
> > +
> > +       return ret;
> > +}
> > +
> > +static void tegra_i2c_bus_unlock(struct i2c_adapter *adapter,
> > +                                unsigned int flags)
> > +{
> > +       struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter);
> > +
> > +       rt_mutex_unlock(&adapter->bus_lock);
> > +       tegra_i2c_mutex_unlock(i2c_dev);
> > +}
> > +
> > +static const struct i2c_lock_operations tegra_i2c_lock_ops = {
> > +       .lock_bus = tegra_i2c_bus_lock,
> > +       .trylock_bus = tegra_i2c_bus_trylock,
> > +       .unlock_bus = tegra_i2c_bus_unlock,
> > +};
> > +
> >  static void tegra_i2c_mask_irq(struct tegra_i2c_dev *i2c_dev, u32
> > mask)
> >  {
> >         u32 int_mask;
> > @@ -546,21 +646,6 @@ static void tegra_i2c_vi_init(struct
> > tegra_i2c_dev *i2c_dev)
> >         i2c_writel(i2c_dev, 0x0, I2C_TLOW_SEXT);
> >  }
> >
> > -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_flush_fifos(struct tegra_i2c_dev *i2c_dev)
> >  {
> >         u32 mask, val, offset;
> > @@ -1497,6 +1582,7 @@ static const struct tegra_i2c_hw_feature
> > tegra20_i2c_hw = {
> >         .setup_hold_time_fast_fast_plus_mode = 0x0,
> >         .setup_hold_time_hs_mode = 0x0,
> >         .has_interface_timing_reg = false,
> > +       .has_mutex = false,
> >  };
> >
> >  static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
> > @@ -1521,6 +1607,7 @@ static const struct tegra_i2c_hw_feature
> > tegra30_i2c_hw = {
> >         .setup_hold_time_fast_fast_plus_mode = 0x0,
> >         .setup_hold_time_hs_mode = 0x0,
> >         .has_interface_timing_reg = false,
> > +       .has_mutex = false,
> >  };
> >
> >  static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
> > @@ -1545,6 +1632,7 @@ static const struct tegra_i2c_hw_feature
> > tegra114_i2c_hw = {
> >         .setup_hold_time_fast_fast_plus_mode = 0x0,
> >         .setup_hold_time_hs_mode = 0x0,
> >         .has_interface_timing_reg = false,
> > +       .has_mutex = false,
> >  };
> >
> >  static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
> > @@ -1569,6 +1657,7 @@ static const struct tegra_i2c_hw_feature
> > tegra124_i2c_hw = {
> >         .setup_hold_time_fast_fast_plus_mode = 0x0,
> >         .setup_hold_time_hs_mode = 0x0,
> >         .has_interface_timing_reg = true,
> > +       .has_mutex = false,
> >  };
> >
> >  static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
> > @@ -1593,6 +1682,7 @@ static const struct tegra_i2c_hw_feature
> > tegra210_i2c_hw = {
> >         .setup_hold_time_fast_fast_plus_mode = 0,
> >         .setup_hold_time_hs_mode = 0,
> >         .has_interface_timing_reg = true,
> > +       .has_mutex = false,
> >  };
> >
> >  static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
> > @@ -1617,6 +1707,7 @@ static const struct tegra_i2c_hw_feature
> > tegra186_i2c_hw = {
> >         .setup_hold_time_fast_fast_plus_mode = 0,
> >         .setup_hold_time_hs_mode = 0,
> >         .has_interface_timing_reg = true,
> > +       .has_mutex = false,
> >  };
> >
> >  static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
> > @@ -1644,6 +1735,7 @@ static const struct tegra_i2c_hw_feature
> > tegra194_i2c_hw = {
> >         .setup_hold_time_hs_mode = 0x090909,
> >         .has_interface_timing_reg = true,
> >         .has_hs_mode_support = true,
> > +       .has_mutex = false,
> >  };
> >
> >  static const struct tegra_i2c_hw_feature tegra264_i2c_hw = {
> > @@ -1671,6 +1763,7 @@ static const struct tegra_i2c_hw_feature
> > tegra264_i2c_hw = {
> >         .setup_hold_time_hs_mode = 0x090909,
> >         .has_interface_timing_reg = true,
> >         .has_hs_mode_support = true,
> > +       .has_mutex = true,
> >  };
> >
> >  static const struct of_device_id tegra_i2c_of_match[] = {
> > @@ -1875,6 +1968,9 @@ static int tegra_i2c_probe(struct
> > platform_device *pdev)
> >         i2c_dev->adapter.nr = pdev->id;
> >         ACPI_COMPANION_SET(&i2c_dev->adapter.dev,
> > ACPI_COMPANION(&pdev->dev));
> >
> > +       if (i2c_dev->hw->has_mutex)
> > +               i2c_dev->adapter.lock_ops = &tegra_i2c_lock_ops;
> > +
> >         if (i2c_dev->hw->supports_bus_clear)
> >                 i2c_dev->adapter.bus_recovery_info =
> > &tegra_i2c_recovery_info;
> >
> > --
> > 2.43.0
> >