diff mbox series

[v3,5/9] i2c-mlxbf.c: support lock mechanism

Message ID 20220908173544.32615-6-asmaa@nvidia.com
State New
Headers show
Series i2c-mlxbf.c: bug fixes and new feature support | expand

Commit Message

Asmaa Mnebhi Sept. 8, 2022, 5:35 p.m. UTC 
Linux is not the only entity using the BlueField I2C busses so
support a lock mechanism provided by hardware to avoid issues
when multiple entities are trying to access the same bus.

The lock is acquired whenever written explicitely or the lock
register is read. So make sure it is always released at the end
of a successful or failed transaction.

Fixes: b5b5b32081cd206b (i2c: mlxbf: I2C SMBus driver for Mellanox BlueField SoC)
Reviewed-by: Khalil Blaiech <kblaiech@nvidia.com>
Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
---
 drivers/i2c/busses/i2c-mlxbf.c | 42 ++++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 4 deletions(-)

Comments

Wolfram Sang Sept. 17, 2022, 6:25 p.m. UTC | #1 
> +	/* Try to acquire the smbus gw lock before any reads of the GW register since
> +	 * a read sets the lock.
> +	 */

Please use kernel multiline comment style.

> @@ -792,6 +824,8 @@ mlxbf_i2c_smbus_start_transaction(struct mlxbf_i2c_priv *priv,
>  			priv->smbus->io + MLXBF_I2C_SMBUS_MASTER_FSM);
>  	}

Dunno if this is taste, but I think it is easier to follow the locking
when we have central acquire and release points for it. So, I'd suggest
to add something like

out_unlock:

here and then goto to this place from the other locations.

What do you think?

>  
> +	mlxbf_i2c_smbus_master_unlock(priv);
> +
>  	return ret;
Asmaa Mnebhi Sept. 19, 2022, 12:50 p.m. UTC | #2 
> +	/* Try to acquire the smbus gw lock before any reads of the GW register since
> +	 * a read sets the lock.
> +	 */

Please use kernel multiline comment style.

Done.
> @@ -792,6 +824,8 @@ mlxbf_i2c_smbus_start_transaction(struct mlxbf_i2c_priv *priv,
>  			priv->smbus->io + MLXBF_I2C_SMBUS_MASTER_FSM);
>  	}

Dunno if this is taste, but I think it is easier to follow the locking when we have central acquire and release points for it. So, I'd suggest to add something like

out_unlock:

here and then goto to this place from the other locations.

What do you think?

Sure!
>  
> +	mlxbf_i2c_smbus_master_unlock(priv);
> +
>  	return ret;
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-mlxbf.c b/drivers/i2c/busses/i2c-mlxbf.c
index a8c5fef3f1cd..b54019b4b01b 100644
--- a/drivers/i2c/busses/i2c-mlxbf.c
+++ b/drivers/i2c/busses/i2c-mlxbf.c
@@ -306,6 +306,7 @@  static u64 mlxbf_i2c_corepll_frequency;
  * exact.
  */
 #define MLXBF_I2C_SMBUS_TIMEOUT   (300 * 1000) /* 300ms */
+#define MLXBF_I2C_SMBUS_LOCK_POLL_TIMEOUT (300 * 1000) /* 300ms */
 
 /* Encapsulates timing parameters. */
 struct mlxbf_i2c_timings {
@@ -514,6 +515,25 @@  static bool mlxbf_smbus_master_wait_for_idle(struct mlxbf_i2c_priv *priv)
 	return false;
 }
 
+/*
+ * wait for the lock to be released before acquiring it.
+ */
+static bool mlxbf_i2c_smbus_master_lock(struct mlxbf_i2c_priv *priv)
+{
+	if (mlxbf_smbus_poll(priv->smbus->io, MLXBF_I2C_SMBUS_MASTER_GW,
+			   MLXBF_I2C_MASTER_LOCK_BIT, true,
+			   MLXBF_I2C_SMBUS_LOCK_POLL_TIMEOUT))
+		return true;
+
+	return false;
+}
+
+static void mlxbf_i2c_smbus_master_unlock(struct mlxbf_i2c_priv *priv)
+{
+	/* Clear the gw to clear the lock */
+	writel(0, priv->smbus->io + MLXBF_I2C_SMBUS_MASTER_GW);
+}
+
 static bool mlxbf_i2c_smbus_transaction_success(u32 master_status,
 						u32 cause_status)
 {
@@ -705,10 +725,18 @@  mlxbf_i2c_smbus_start_transaction(struct mlxbf_i2c_priv *priv,
 	slave = request->slave & GENMASK(6, 0);
 	addr = slave << 1;
 
-	/* First of all, check whether the HW is idle. */
-	if (WARN_ON(!mlxbf_smbus_master_wait_for_idle(priv)))
+	/* Try to acquire the smbus gw lock before any reads of the GW register since
+	 * a read sets the lock.
+	 */
+	if (WARN_ON(!mlxbf_i2c_smbus_master_lock(priv)))
 		return -EBUSY;
 
+	/* Check whether the HW is idle */
+	if (WARN_ON(!mlxbf_smbus_master_wait_for_idle(priv))) {
+		mlxbf_i2c_smbus_master_unlock(priv);
+		return -EBUSY;
+	}
+
 	/* Set first byte. */
 	data_desc[data_idx++] = addr;
 
@@ -732,8 +760,10 @@  mlxbf_i2c_smbus_start_transaction(struct mlxbf_i2c_priv *priv,
 			write_en = 1;
 			write_len += operation->length;
 			if (data_idx + operation->length >
-					MLXBF_I2C_MASTER_DATA_DESC_SIZE)
+					MLXBF_I2C_MASTER_DATA_DESC_SIZE) {
+				mlxbf_i2c_smbus_master_unlock(priv);
 				return -ENOBUFS;
+			}
 			memcpy(data_desc + data_idx,
 			       operation->buffer, operation->length);
 			data_idx += operation->length;
@@ -764,8 +794,10 @@  mlxbf_i2c_smbus_start_transaction(struct mlxbf_i2c_priv *priv,
 	if (write_en) {
 		ret = mlxbf_i2c_smbus_enable(priv, slave, write_len, block_en,
 					 pec_en, 0);
-		if (ret)
+		if (ret) {
+			mlxbf_i2c_smbus_master_unlock(priv);
 			return ret;
+		}
 	}
 
 	if (read_en) {
@@ -792,6 +824,8 @@  mlxbf_i2c_smbus_start_transaction(struct mlxbf_i2c_priv *priv,
 			priv->smbus->io + MLXBF_I2C_SMBUS_MASTER_FSM);
 	}
 
+	mlxbf_i2c_smbus_master_unlock(priv);
+
 	return ret;
 }