[PATCH 0/1] Increase I2C bus arbitration timeout in the PCA9541 driver

[PATCH 0/1] Increase I2C bus arbitration timeout in the PCA9541 driver

[Manikanta Guntupalli]

This patch increases the I2C bus arbitration timeout in the PCA9541 driver
to improve robustness during multi-master scenarios.

If it makes more sense to make this arbitration timeout configurable via
a module parameter instead, we would be happy to revise the patch
accordingly. Please advise.

Looking forward to your feedback.

Jonathan Stroud (1):
  PCA9541: Increase I2C bus arbitration timeout

 drivers/i2c/muxes/i2c-mux-pca9541.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/1] PCA9541: Increase I2C bus arbitration timeout

[Manikanta Guntupalli]

From: Jonathan Stroud <jonathan.stroud@amd.com>

When u-boot has the bus acquired on the other host, it can hold the bus
for much longer than 125ms.  Forcefully stealing the bus will result in
u-boot hanging.  Increase arbitration timeout to 1 second to avoid this
condition.

Arbitration timeout and forcefully taking the bus should only be
happening in a failure condition when the other host is dead.  Therefor
increasing this timeout to a large value should not impact normal
operation.

Signed-off-by: Jonathan Stroud <jonathan.stroud@amd.com>
Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
---
 drivers/i2c/muxes/i2c-mux-pca9541.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
index 8663c8a7c269..e602c4a294d3 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -64,8 +64,8 @@
 #define busoff(x)	(!((x) & BUSON) || ((x) & BUSON) == BUSON)
 
 /* arbitration timeouts, in jiffies */
-#define ARB_TIMEOUT	(HZ / 8)	/* 125 ms until forcing bus ownership */
-#define ARB2_TIMEOUT	(HZ / 4)	/* 250 ms until acquisition failure */
+#define ARB_TIMEOUT	(HZ)		/* 1 s until forcing bus ownership */
+#define ARB2_TIMEOUT	(2 * HZ)	/* 2 s until acquisition failure */
 
 /* arbitration retry delays, in us */
 #define SELECT_DELAY_SHORT	50
@@ -229,6 +229,7 @@ static int pca9541_arbitrate(struct i2c_client *client)
 		 */
 		data->select_timeout = SELECT_DELAY_LONG;
 		if (time_is_before_eq_jiffies(data->arb_timeout)) {
+			dev_info(&client->dev, "I2C Bus Arbiter timeout, forcing take bus\n");
 			/* Time is up, take the bus and reset it. */
 			pca9541_reg_write(client,
 					  PCA9541_CONTROL,
@@ -267,6 +268,7 @@ static int pca9541_select_chan(struct i2c_mux_core *muxc, u32 chan)
 		else
 			msleep(data->select_timeout / 1000);
 	} while (time_is_after_eq_jiffies(timeout));
+	dev_info(&client->dev, "Timeout acquiring I2C bus\n");
 
 	return -ETIMEDOUT;
 }
-- 
2.34.1

Re: [PATCH 1/1] PCA9541: Increase I2C bus arbitration timeout

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 917 bytes --]

Hi,

thanks for your patch.

>  /* arbitration timeouts, in jiffies */
> -#define ARB_TIMEOUT	(HZ / 8)	/* 125 ms until forcing bus ownership */
> -#define ARB2_TIMEOUT	(HZ / 4)	/* 250 ms until acquisition failure */
> +#define ARB_TIMEOUT	(HZ)		/* 1 s until forcing bus ownership */
> +#define ARB2_TIMEOUT	(2 * HZ)	/* 2 s until acquisition failure */

Can't we use the timeout value of the parent struct i2c_adapter? This is
by default HZ and can be set by userspace via IOCTL depending on the
actual use case. So, we would use (pseudo-code, probably):

254         unsigned long timeout = jiffies + 2 * client->adapter->timeout;
255                 /* give up after this time */
256 
257         data->arb_timeout = jiffies + client->adapter->timeout;

?


> +			dev_info(&client->dev, "I2C Bus Arbiter timeout, forcing take bus\n");

'dev_warn' for both?

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]