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:

quoted

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 <redacted>
---
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.

quoted

+
+     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.

quoted

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