Skip to contentSkip to conversation

i2c: designware: Avoid aborted transfers with fast reacting I2C slaves

PATCH Applied Jarkko Nikula 3 participants 3 messages Sep 29, 2016 – Oct 25, 2016

Subsystems (3)
I2C subsystem host drivers Intel LPSS (Low Power SubSystem) drivers Synopsys DesignWare I2C driver
lore
Share & tools
lore

Jarkko Nikula

author
#0 lore raw

PATCH 1/1 i2c: designware: Avoid aborted transfers with fast reacting I2C slaves

I2C DesignWare may abort transfer with arbitration lost if I2C slave pulls SDA down quickly after falling edge of SCL. Reason for this is unknown but after trial and error it was found this can be avoided by enabling non-zero SDA RX hold time for the receiver.

By the specification SDA RX hold time extends incoming SDA low to high transition by n * ic_clk cycles but only when SCL is high. However it seems to help avoid above faulty arbitration lost error.

Bits 23:16 in IC_SDA_HOLD register define the SDA RX hold time for the receiver. Be conservative and enable 1 ic_clk cycle long hold time in case boot firmware hasn't set it up.

Reported-by Jukka Laitinen <[email protected]>
Signed-off-by Jarkko Nikula <[email protected]>
---
 drivers/i2c/busses/i2c-designware-core.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)
drivers/i2c/busses/i2c-designware-core.c +14 -3
index 1fe93c43215c..11e866d05368 100644
@@ -95,6 +95,9 @@
 #define DW_IC_STATUS_TFE		BIT(2)
 #define DW_IC_STATUS_MST_ACTIVITY	BIT(5)
 
+#define DW_IC_SDA_HOLD_RX_SHIFT		16
+#define DW_IC_SDA_HOLD_RX_MASK		GENMASK(23, DW_IC_SDA_HOLD_RX_SHIFT)
+
 #define DW_IC_ERR_TX_ABRT	0x1
 
 #define DW_IC_TAR_10BITADDR_MASTER BIT(12)

@@ -420,12 +423,20 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	/* Configure SDA Hold Time if required */
 	reg = dw_readl(dev, DW_IC_COMP_VERSION);
 	if (reg >= DW_IC_SDA_HOLD_MIN_VERS) {
-		if (dev->sda_hold_time) {
-			dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
-		} else {
+		if (!dev->sda_hold_time) {
 			/* Keep previous hold time setting if no one set it */
 			dev->sda_hold_time = dw_readl(dev, DW_IC_SDA_HOLD);
 		}
+		/*
+		 * Workaround for avoiding TX arbitration lost in case I2C
+		 * slave pulls SDA down "too quickly" after falling egde of
+		 * SCL by enabling non-zero SDA RX hold. Specification says it
+		 * extends incoming SDA low to high transition while SCL is
+		 * high but it apprears to help also above issue.
+		 */
+		if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK))
+			dev->sda_hold_time |= 1 << DW_IC_SDA_HOLD_RX_SHIFT;
+		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
 	} else {
 		dev_warn(dev->dev,
 			"Hardware too old to adjust SDA hold time.\n");
Signature · 1 line
2.9.3
Tested-by on v1 1/1
#1 lore raw
Quoted 43 lines · Jarkko Nikula

By the specification SDA RX hold time extends incoming SDA low to high transition by n * ic_clk cycles but only when SCL is high. However it seems to help avoid above faulty arbitration lost error.

Bits 23:16 in IC_SDA_HOLD register define the SDA RX hold time for the receiver. Be conservative and enable 1 ic_clk cycle long hold time in case boot firmware hasn't set it up.

Reported-by Jukka Laitinen <[email protected]>
Signed-off-by Jarkko Nikula <[email protected]>
---
 drivers/i2c/busses/i2c-designware-core.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)
drivers/i2c/busses/i2c-designware-core.c +7 -3
index 1fe93c43215c..11e866d05368 100644
@@ -95,6 +95,9 @@
 #define DW_IC_STATUS_TFE		BIT(2)
 #define DW_IC_STATUS_MST_ACTIVITY	BIT(5)
 
+#define DW_IC_SDA_HOLD_RX_SHIFT		16
+#define DW_IC_SDA_HOLD_RX_MASK		GENMASK(23, DW_IC_SDA_HOLD_RX_SHIFT)
+
 #define DW_IC_ERR_TX_ABRT	0x1
 
 #define DW_IC_TAR_10BITADDR_MASTER BIT(12)

@@ -420,12 +423,20 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	/* Configure SDA Hold Time if required */
 	reg = dw_readl(dev, DW_IC_COMP_VERSION);
 	if (reg >= DW_IC_SDA_HOLD_MIN_VERS) {
-		if (dev->sda_hold_time) {
-			dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
-		} else {
+		if (!dev->sda_hold_time) {
 			/* Keep previous hold time setting if no one set it */
 			dev->sda_hold_time = dw_readl(dev, DW_IC_SDA_HOLD);
 		}
+		/*
+		 * Workaround for avoiding TX arbitration lost in case I2C
+		 * slave pulls SDA down "too quickly" after falling egde of

drivers/i2c/busses/i2c-designware-core.c int i2c_dw_init(struct dw_i2c_dev *dev) · line 442

Quoted 10 lines
drivers/i2c/busses/i2c-designware-core.c +7 -0
+		 * SCL by enabling non-zero SDA RX hold. Specification says it
+		 * extends incoming SDA low to high transition while SCL is
+		 * high but it apprears to help also above issue.
+		 */
+		if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK))
+			dev->sda_hold_time |= 1 << DW_IC_SDA_HOLD_RX_SHIFT;
+		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
 	} else {
 		dev_warn(dev->dev,
 			"Hardware too old to adjust SDA hold time.\n");
Tested-by Jukka Laitinen <[email protected]>

Tested with kabylake cpu and Ubuntu 16.04 + kernel 4.8-rc6 + ubuntu sauce from http://kernel.ubuntu.com/~kernel-ppa/mainline/v4.8-rc6/

Regards,
Jukka Laitinen

Applied
#2 lore raw
In reply to Jarkko Nikula
Quoted 9 lines · Jarkko Nikula

I2C DesignWare may abort transfer with arbitration lost if I2C slave pulls SDA down quickly after falling edge of SCL. Reason for this is unknown but after trial and error it was found this can be avoided by enabling non-zero SDA RX hold time for the receiver.

By the specification SDA RX hold time extends incoming SDA low to high transition by n * ic_clk cycles but only when SCL is high. However it seems to help avoid above faulty arbitration lost error.

Quoted 6 lines

Bits 23:16 in IC_SDA_HOLD register define the SDA RX hold time for the receiver. Be conservative and enable 1 ic_clk cycle long hold time in case boot firmware hasn't set it up.

Reported-by Jukka Laitinen <[email protected]>
Signed-off-by Jarkko Nikula <[email protected]>

Applied to for-current, thanks!