[PATCH v1 0/3] i2c: mlxbf: Fix master GW corruption

[PATCH v1 0/3] i2c: mlxbf: Fix master GW corruption

[Chris Babroski]

This patch series fixes a bug that corrupts the slave address used in read
phases and cleans up the code to prevent similar corruption scenarios from
occurring in the future.

When the MLXBF_I2C_F_SMBUS_BLOCK and/or MLXBF_I2C_F_SMBUS_PEC flags are set, the
flag bitmask values are shifted and written to the master gateway control
register instead of the single-bit flag values. This caused slave address
corruption in read phases (unlike write phases which use the data_desc[0] slave
address value).

Patch 1 fixes this corruption issue by converting the flags to booleans before
setting their values in the control register.

Patch 2 converts register field accesses from explicit shifts to
BIT()/GENMASK()/FIELD_PREP().

Patch 3 removes an unnecessary register write (dead code).

Chris Babroski (3):
  i2c: mlxbf: Fix master GW corruption from unmasked SMBus flags
  i2c: mlxbf: Use GENMASK()/FIELD_PREP() for GW fields
  i2c: mlxbf: Remove unused slave GW PEC handling

 drivers/i2c/busses/i2c-mlxbf.c | 40 +++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 20 deletions(-)

-- 
2.34.1

[PATCH v1 1/3] i2c: mlxbf: Fix master GW corruption from unmasked SMBus flags

[Chris Babroski]

When the MLXBF_I2C_F_SMBUS_BLOCK and/or MLXBF_I2C_F_SMBUS_PEC flags are
set, the flag bitmask values are shifted and written to the master
gateway control register instead of the single-bit flag values. Shifting
and writing the bitmasks can corrupt adjacent bits in the master gateway
control register like the slave address used in read phases.

Convert the SMBus flags to boolean values before setting the master
gateway control register.

Signed-off-by: Chris Babroski <cbabroski@nvidia.com>
---
 drivers/i2c/busses/i2c-mlxbf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mlxbf.c b/drivers/i2c/busses/i2c-mlxbf.c
index 24ccc4546ab8..93fc9f0ba72c 100644
--- a/drivers/i2c/busses/i2c-mlxbf.c
+++ b/drivers/i2c/busses/i2c-mlxbf.c
@@ -740,8 +740,8 @@ mlxbf_i2c_smbus_start_transaction(struct mlxbf_i2c_priv *priv,
 		 * submitted by the first operation only.
 		 */
 		if (op_idx == 0 && flags & MLXBF_I2C_F_SMBUS_OPERATION) {
-			block_en = flags & MLXBF_I2C_F_SMBUS_BLOCK;
-			pec_en = flags & MLXBF_I2C_F_SMBUS_PEC;
+			block_en = !!(flags & MLXBF_I2C_F_SMBUS_BLOCK);
+			pec_en = !!(flags & MLXBF_I2C_F_SMBUS_PEC);
 		}
 
 		if (flags & MLXBF_I2C_F_WRITE) {
-- 
2.34.1

[PATCH v1 2/3] i2c: mlxbf: Use GENMASK()/FIELD_PREP() for GW fields

[Chris Babroski]

Replace the explicit shift amounts and rol32() calls used to build the
master and slave gateway control words with GENMASK()/FIELD_PREP().
This makes the field widths explicit and matches the style used
elsewhere in the kernel for register field access.

Also change the single-bit master GW PARSE_EXP and SEND_PEC control
bits to be set with a plain conditional OR instead of rol32(), since
moving a boolean into position via a mask is no longer needed.

Signed-off-by: Chris Babroski <cbabroski@nvidia.com>
---
 drivers/i2c/busses/i2c-mlxbf.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mlxbf.c b/drivers/i2c/busses/i2c-mlxbf.c
index 93fc9f0ba72c..2d2dc234db7a 100644
--- a/drivers/i2c/busses/i2c-mlxbf.c
+++ b/drivers/i2c/busses/i2c-mlxbf.c
@@ -219,7 +219,12 @@
 #define MLXBF_I2C_MASTER_BUSY_BIT         BIT(30) /* Busy bit. */
 #define MLXBF_I2C_MASTER_START_BIT        BIT(29) /* Control start. */
 #define MLXBF_I2C_MASTER_CTL_WRITE_BIT    BIT(28) /* Control write phase. */
+#define MLXBF_I2C_MASTER_WRITE_MASK       GENMASK(27, 21) /* Control write bytes */
+#define MLXBF_I2C_MASTER_SEND_PEC_BIT     BIT(20) /* Send PEC byte when set to 1 */
 #define MLXBF_I2C_MASTER_CTL_READ_BIT     BIT(19) /* Control read phase. */
+#define MLXBF_I2C_MASTER_SLV_ADDR_MASK    GENMASK(18, 12) /* Slave address */
+#define MLXBF_I2C_MASTER_PARSE_EXP_BIT    BIT(11) /* Control parse expected bytes */
+#define MLXBF_I2C_MASTER_READ_MASK        GENMASK(10, 4) /* Control read bytes */
 #define MLXBF_I2C_MASTER_STOP_BIT         BIT(3)  /* Control stop. */
 
 #define MLXBF_I2C_MASTER_ENABLE \
@@ -232,12 +237,6 @@
 #define MLXBF_I2C_MASTER_ENABLE_READ \
 	(MLXBF_I2C_MASTER_ENABLE | MLXBF_I2C_MASTER_CTL_READ_BIT)
 
-#define MLXBF_I2C_MASTER_WRITE_SHIFT      21 /* Control write bytes */
-#define MLXBF_I2C_MASTER_SEND_PEC_SHIFT   20 /* Send PEC byte when set to 1 */
-#define MLXBF_I2C_MASTER_PARSE_EXP_SHIFT  11 /* Control parse expected bytes */
-#define MLXBF_I2C_MASTER_SLV_ADDR_SHIFT   12 /* Slave address */
-#define MLXBF_I2C_MASTER_READ_SHIFT       4  /* Control read bytes */
-
 /* SMBus master GW Data descriptor. */
 #define MLXBF_I2C_MASTER_DATA_DESC_ADDR   0x80
 #define MLXBF_I2C_MASTER_DATA_DESC_SIZE   0x80 /* Size in bytes. */
@@ -288,7 +287,7 @@
 #define MLXBF_I2C_SLAVE_ENABLE \
 	(MLXBF_I2C_SLAVE_BUSY_BIT | MLXBF_I2C_SLAVE_WRITE_BIT)
 
-#define MLXBF_I2C_SLAVE_WRITE_BYTES_SHIFT 22 /* Number of bytes to write. */
+#define MLXBF_I2C_SLAVE_WRITE_BYTES_MASK  GENMASK(28, 22) /* Number of bytes to write. */
 #define MLXBF_I2C_SLAVE_SEND_PEC_SHIFT    21 /* Send PEC byte shift. */
 
 /* SMBus slave GW Data descriptor. */
@@ -643,14 +642,19 @@ static int mlxbf_i2c_smbus_enable(struct mlxbf_i2c_priv *priv, u8 slave,
 		command |= MLXBF_I2C_MASTER_STOP_BIT;
 	if (read) {
 		command |= MLXBF_I2C_MASTER_ENABLE_READ;
-		command |= rol32(len, MLXBF_I2C_MASTER_READ_SHIFT);
+		command |= FIELD_PREP(MLXBF_I2C_MASTER_READ_MASK, len);
 	} else {
 		command |= MLXBF_I2C_MASTER_ENABLE_WRITE;
-		command |= rol32(len, MLXBF_I2C_MASTER_WRITE_SHIFT);
+		command |= FIELD_PREP(MLXBF_I2C_MASTER_WRITE_MASK, len);
 	}
-	command |= rol32(slave, MLXBF_I2C_MASTER_SLV_ADDR_SHIFT);
-	command |= rol32(block_en, MLXBF_I2C_MASTER_PARSE_EXP_SHIFT);
-	command |= rol32(pec_en, MLXBF_I2C_MASTER_SEND_PEC_SHIFT);
+
+	if (block_en)
+		command |= MLXBF_I2C_MASTER_PARSE_EXP_BIT;
+
+	if (pec_en)
+		command |= MLXBF_I2C_MASTER_SEND_PEC_BIT;
+
+	command |= FIELD_PREP(MLXBF_I2C_MASTER_SLV_ADDR_MASK, slave);
 
 	/* Clear status bits. */
 	writel(0x0, priv->mst->io + MLXBF_I2C_SMBUS_MASTER_STATUS);
@@ -1888,7 +1892,7 @@ static int mlxbf_i2c_irq_send(struct mlxbf_i2c_priv *priv, u8 recv_bytes)
 
 	/* Prepare control word. */
 	control32 = MLXBF_I2C_SLAVE_ENABLE;
-	control32 |= rol32(write_size, MLXBF_I2C_SLAVE_WRITE_BYTES_SHIFT);
+	control32 |= FIELD_PREP(MLXBF_I2C_SLAVE_WRITE_BYTES_MASK, write_size);
 	control32 |= rol32(pec_en, MLXBF_I2C_SLAVE_SEND_PEC_SHIFT);
 
 	writel(control32, priv->slv->io + MLXBF_I2C_SMBUS_SLAVE_GW);
-- 
2.34.1

[PATCH v1 3/3] i2c: mlxbf: Remove unused slave GW PEC handling

[Chris Babroski]

The slave gateway control word write always cleared its SEND_PEC bit,
since PEC is not supported on the slave path. Remove the dead pec_en
local variable, the corresponding register write, and the now-unused
MLXBF_I2C_SLAVE_SEND_PEC_SHIFT definition.

Signed-off-by: Chris Babroski <cbabroski@nvidia.com>
---
 drivers/i2c/busses/i2c-mlxbf.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mlxbf.c b/drivers/i2c/busses/i2c-mlxbf.c
index 2d2dc234db7a..deaf22df68d0 100644
--- a/drivers/i2c/busses/i2c-mlxbf.c
+++ b/drivers/i2c/busses/i2c-mlxbf.c
@@ -288,7 +288,6 @@
 	(MLXBF_I2C_SLAVE_BUSY_BIT | MLXBF_I2C_SLAVE_WRITE_BIT)
 
 #define MLXBF_I2C_SLAVE_WRITE_BYTES_MASK  GENMASK(28, 22) /* Number of bytes to write. */
-#define MLXBF_I2C_SLAVE_SEND_PEC_SHIFT    21 /* Send PEC byte shift. */
 
 /* SMBus slave GW Data descriptor. */
 #define MLXBF_I2C_SLAVE_DATA_DESC_ADDR   0x80
@@ -1822,7 +1821,7 @@ static struct i2c_client *mlxbf_i2c_get_slave_from_addr(
 static int mlxbf_i2c_irq_send(struct mlxbf_i2c_priv *priv, u8 recv_bytes)
 {
 	u8 data_desc[MLXBF_I2C_SLAVE_DATA_DESC_SIZE] = { 0 };
-	u8 write_size, pec_en, addr, value, byte_cnt;
+	u8 write_size, addr, value, byte_cnt;
 	struct i2c_client *slave;
 	u32 control32, data32;
 	int ret = 0;
@@ -1888,12 +1887,9 @@ static int mlxbf_i2c_irq_send(struct mlxbf_i2c_priv *priv, u8 recv_bytes)
 	mlxbf_i2c_smbus_write_data(priv, data_desc, byte_cnt,
 				   MLXBF_I2C_SLAVE_DATA_DESC_ADDR, false);
 
-	pec_en = 0; /* Disable PEC since it is not supported. */
-
 	/* Prepare control word. */
 	control32 = MLXBF_I2C_SLAVE_ENABLE;
 	control32 |= FIELD_PREP(MLXBF_I2C_SLAVE_WRITE_BYTES_MASK, write_size);
-	control32 |= rol32(pec_en, MLXBF_I2C_SLAVE_SEND_PEC_SHIFT);
 
 	writel(control32, priv->slv->io + MLXBF_I2C_SMBUS_SLAVE_GW);
 
-- 
2.34.1