linux-kernel - [PATCH v4 2/2] i2c-mlxbf: Improve I2C bus timing configuration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250506193059.321345-2-cbabroski@nvidia.com>
Date: Tue, 6 May 2025 19:30:59 +0000
From: Chris Babroski <cbabroski@...dia.com>
To: <andi.shyti@...nel.org>, <kblaiech@...dia.com>, <asmaa@...dia.com>
CC: <davthompson@...dia.com>, <linux-i2c@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <cbabroski@...dia.com>
Subject: [PATCH v4 2/2] i2c-mlxbf: Improve I2C bus timing configuration

Update the I2C bus timing configuration on BlueField to match the
configuration recommended and verified by the HW team.

I2C block read failures were found on BlueField 3 during communication
with a device that requires the use of repeated start conditions.
Testing showed that these failures were caused by the I2C transaction
getting aborted early due to a short bus "timeout" configuration value.
This value determines how long the clock can be held low before the I2C
transaction is aborted.

Upon further inspection, it was also found that other I2C bus timing
configuration values used by the kernel driver do not match the
configuration that is recommended by the HW team and used in the
BlueField BSP I2C drivers.

Signed-off-by: Chris Babroski <cbabroski@...dia.com>
Reviewed-by: Asmaa Mnebhi <asmaa@...dia.com>
Reviewed-by: Khalil Blaiech <kblaiech@...dia.com>
---
 V3 -> V4: Split changes into two separate logical patches
 V2 -> V3: Cleaned up code and address review comments
 V1 -> V2: Removed default "Reviewed-by:" tags

 drivers/i2c/busses/i2c-mlxbf.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mlxbf.c b/drivers/i2c/busses/i2c-mlxbf.c
index 0f5b6a00c1b6..3f8b4574f735 100644
--- a/drivers/i2c/busses/i2c-mlxbf.c
+++ b/drivers/i2c/busses/i2c-mlxbf.c
@@ -196,6 +196,7 @@
 
 #define MLXBF_I2C_MASK_8    GENMASK(7, 0)
 #define MLXBF_I2C_MASK_16   GENMASK(15, 0)
+#define MLXBF_I2C_MASK_32   GENMASK(31, 0)
 
 #define MLXBF_I2C_MST_ADDR_OFFSET         0x200
 
@@ -1192,7 +1193,8 @@ static void mlxbf_i2c_set_timings(struct mlxbf_i2c_priv *priv,
 				     MLXBF_I2C_MASK_16, MLXBF_I2C_SHIFT_16);
 	writel(timer, priv->timer->io + MLXBF_I2C_SMBUS_THIGH_MAX_TBUF);
 
-	timer = timings->timeout;
+	timer = mlxbf_i2c_set_timer(priv, timings->timeout, false,
+				    MLXBF_I2C_MASK_32, MLXBF_I2C_SHIFT_0);
 	writel(timer, priv->timer->io + MLXBF_I2C_SMBUS_SCL_LOW_TIMEOUT);
 }
 
@@ -1202,11 +1204,7 @@ enum mlxbf_i2c_timings_config {
 	MLXBF_I2C_TIMING_CONFIG_1000KHZ,
 };
 
-/*
- * Note that the mlxbf_i2c_timings->timeout value is not related to the
- * bus frequency, it is impacted by the time it takes the driver to
- * complete data transmission before transaction abort.
- */
+/* Timing values are in nanoseconds */
 static const struct mlxbf_i2c_timings mlxbf_i2c_timings[] = {
 	[MLXBF_I2C_TIMING_CONFIG_100KHZ] = {
 		.scl_high = 4810,
@@ -1221,8 +1219,8 @@ static const struct mlxbf_i2c_timings mlxbf_i2c_timings[] = {
 		.scl_fall = 50,
 		.hold_data = 300,
 		.buf = 20000,
-		.thigh_max = 5000,
-		.timeout = 106500
+		.thigh_max = 50000,
+		.timeout = 35000000
 	},
 	[MLXBF_I2C_TIMING_CONFIG_400KHZ] = {
 		.scl_high = 1011,
@@ -1237,24 +1235,24 @@ static const struct mlxbf_i2c_timings mlxbf_i2c_timings[] = {
 		.scl_fall = 50,
 		.hold_data = 300,
 		.buf = 20000,
-		.thigh_max = 5000,
-		.timeout = 106500
+		.thigh_max = 50000,
+		.timeout = 35000000
 	},
 	[MLXBF_I2C_TIMING_CONFIG_1000KHZ] = {
-		.scl_high = 600,
-		.scl_low = 1300,
+		.scl_high = 383,
+		.scl_low = 460,
 		.hold_start = 600,
-		.setup_start = 600,
-		.setup_stop = 600,
-		.setup_data = 100,
+		.setup_start = 260,
+		.setup_stop = 260,
+		.setup_data = 50,
 		.sda_rise = 50,
 		.sda_fall = 50,
 		.scl_rise = 50,
 		.scl_fall = 50,
 		.hold_data = 300,
-		.buf = 20000,
-		.thigh_max = 5000,
-		.timeout = 106500
+		.buf = 500,
+		.thigh_max = 50000,
+		.timeout = 35000000
 	}
 };
 
-- 
2.43.2