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