Skip to contentSkip to conversation

i2c: octeon,thunderx: Recovery fixes and improvements

PATCH Applied Jan Glauber 3 participants 13 messages Sep 21, 2016 – Sep 22, 2016

Subsystems (2)
I2C subsystem host drivers Cavium I2C driver

v1 of 2 Applied Latest is v2 →

lore
Share & tools
lore

Jan Glauber

author
#0 lore raw

cover i2c: octeon,thunderx: Recovery fixes and improvements

Hi Wolfram,

the ThunderX i2c driver got more exposure with IPMI/BMC testing and we've found several issues that I'd like to address with this series.

The patches are all related to recovery or improve stability in the presence of hardware/firmware bugs.

Patches are against linux-next and were tested on ThunderX and Octeon (by Steven J. Hill).

thanks,
Jan

--------------

Dmitry Bazhenov (2):
i2c: octeon,thunderx: Fix set SCL recovery function
i2c: octeon,thunderx: Avoid sending STOP during recovery

Jan Glauber (3):
i2c: octeon,thunderx: Fix high-level controller status check
i2c: octeon,thunderx: Check bus state before starting a transaction
i2c: octeon,thunderx: Limit register access retries

 drivers/i2c/busses/i2c-octeon-core.c | 67 ++++++++++++++++++++++++++++--------
 drivers/i2c/busses/i2c-octeon-core.h | 21 ++++++++---
 2 files changed, 68 insertions(+), 20 deletions(-)
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH 1/5 i2c: octeon,thunderx: Fix set SCL recovery function

From: Dmitry Bazhenov <[email protected]>

The set SCL recovery function unconditionally pulls the SCL line low. Only pull SCL line low according to val parameter.

Signed-off-by Dmitry Bazhenov <[email protected]>
Signed-off-by Jan Glauber <[email protected]>
[Changed commit message]
---
 drivers/i2c/busses/i2c-octeon-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-octeon-core.c +1 -1
index f45ea5e..f322242 100644
@@ -767,7 +767,7 @@ static void octeon_i2c_set_scl(struct i2c_adapter *adap, int val)
 {
 	struct octeon_i2c *i2c = i2c_get_adapdata(adap);
 
-	octeon_i2c_write_int(i2c, TWSI_INT_SCL_OVR);
+	octeon_i2c_write_int(i2c, val ? 0 : TWSI_INT_SCL_OVR);
 }
 
 static int octeon_i2c_get_sda(struct i2c_adapter *adap)
Signature · 1 line
1.9.1
Applied
#2 lore raw

Re: [PATCH 1/5] i2c: octeon,thunderx: Fix set SCL recovery function

Quoted 8 lines · Jan Glauber

From: Dmitry Bazhenov <[email protected]>

The set SCL recovery function unconditionally pulls the SCL line low. Only pull SCL line low according to val parameter.

Signed-off-by Dmitry Bazhenov <[email protected]>
Signed-off-by Jan Glauber <[email protected]>
[Changed commit message]

Applied to for-next, thanks!

Minor nit: please only use "octeon:" if you change octeon-core.

author
#3 lore raw

PATCH 2/5 i2c: octeon,thunderx: Avoid sending STOP during recovery

In reply to Jan Glauber

From: Dmitry Bazhenov <[email protected]>

Due to a bug in the ThunderX I2C hardware sending STOP during a recovery attempt could lock up the hardware. To work around this problem do not send STOP at the beginning of the recovery but use the override registers to bring the TWSI including the high-level controller out of the bad state.

Signed-off-by Dmitry Bazhenov <[email protected]>
Signed-off-by Jan Glauber <[email protected]>
[Changed commit message]
---
 drivers/i2c/busses/i2c-octeon-core.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-octeon-core.c +14 -4
index f322242..7d4df83 100644
@@ -783,13 +783,14 @@ static void octeon_i2c_prepare_recovery(struct i2c_adapter *adap)
 {
 	struct octeon_i2c *i2c = i2c_get_adapdata(adap);
 
+	octeon_i2c_hlc_disable(i2c);
+
 	/*
-	 * The stop resets the state machine, does not _transmit_ STOP unless
-	 * engine was active.
+	 * Bring control register to a good state regardless
+	 * of HLC state.
 	 */
-	octeon_i2c_stop(i2c);
+	octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB);
 
-	octeon_i2c_hlc_disable(i2c);
 	octeon_i2c_write_int(i2c, 0);
 }
 

@@ -797,6 +798,15 @@ static void octeon_i2c_unprepare_recovery(struct i2c_adapter *adap)
 {
 	struct octeon_i2c *i2c = i2c_get_adapdata(adap);
 
+	/*
+	 * Generate STOP to finish the unfinished transaction.
+	 * Can't generate STOP via the TWSI CTL register
+	 * since it could bring the TWSI controller into an inoperable state.
+	 */
+	octeon_i2c_write_int(i2c, TWSI_INT_SDA_OVR | TWSI_INT_SCL_OVR);
+	udelay(5);
+	octeon_i2c_write_int(i2c, TWSI_INT_SDA_OVR);
+	udelay(5);
 	octeon_i2c_write_int(i2c, 0);
 }
Signature · 1 line
1.9.1
Applied
#4 lore raw

Re: [PATCH 2/5] i2c: octeon,thunderx: Avoid sending STOP during recovery

Quoted 11 lines · Jan Glauber

From: Dmitry Bazhenov <[email protected]>

Due to a bug in the ThunderX I2C hardware sending STOP during a recovery attempt could lock up the hardware. To work around this problem do not send STOP at the beginning of the recovery but use the override registers to bring the TWSI including the high-level controller out of the bad state.

Signed-off-by Dmitry Bazhenov <[email protected]>
Signed-off-by Jan Glauber <[email protected]>
[Changed commit message]

Applied to for-next, thanks!

author
#5 lore raw

PATCH 3/5 i2c: octeon,thunderx: Fix high-level controller status check

In reply to Jan Glauber

In case the high-level controller (HLC) is used the status code is reported at a different location. Check that location after HLC write operations if the ready bit is not set and return an appropriate error code instead of always returning -EAGAIN.

Signed-off-by Jan Glauber <[email protected]>
---
 drivers/i2c/busses/i2c-octeon-core.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)
drivers/i2c/busses/i2c-octeon-core.c +14 -9
index 7d4df83..5e63b17 100644
@@ -215,7 +215,16 @@ static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c)
 
 static int octeon_i2c_check_status(struct octeon_i2c *i2c, int final_read)
 {
-	u8 stat = octeon_i2c_stat_read(i2c);
+	u8 stat;
+
+	/*
+	 * This is ugly... in HLC mode the status is not in the status register
+	 * but in the lower 8 bits of SW_TWSI.
+	 */
+	if (i2c->hlc_enabled)
+		stat = __raw_readq(i2c->twsi_base + SW_TWSI(i2c));
+	else
+		stat = octeon_i2c_stat_read(i2c);
 
 	switch (stat) {
 	/* Everything is fine */

@@ -453,7 +462,7 @@ static int octeon_i2c_hlc_read(struct octeon_i2c *i2c, struct i2c_msg *msgs)
 
 	cmd = __raw_readq(i2c->twsi_base + SW_TWSI(i2c));
 	if ((cmd & SW_TWSI_R) == 0)
-		return -EAGAIN;
+		return octeon_i2c_check_status(i2c, false);
 
 	for (i = 0, j = msgs[0].len - 1; i  < msgs[0].len && i < 4; i++, j--)
 		msgs[0].buf[j] = (cmd >> (8 * i)) & 0xff;

@@ -506,9 +515,7 @@ static int octeon_i2c_hlc_write(struct octeon_i2c *i2c, struct i2c_msg *msgs)
 
 	cmd = __raw_readq(i2c->twsi_base + SW_TWSI(i2c));
 	if ((cmd & SW_TWSI_R) == 0)
-		return -EAGAIN;
-
-	ret = octeon_i2c_check_status(i2c, false);
+		return octeon_i2c_check_status(i2c, false);
 
 err:
 	return ret;

@@ -553,7 +560,7 @@ static int octeon_i2c_hlc_comp_read(struct octeon_i2c *i2c, struct i2c_msg *msgs
 
 	cmd = __raw_readq(i2c->twsi_base + SW_TWSI(i2c));
 	if ((cmd & SW_TWSI_R) == 0)
-		return -EAGAIN;
+		return octeon_i2c_check_status(i2c, false);
 
 	for (i = 0, j = msgs[1].len - 1; i  < msgs[1].len && i < 4; i++, j--)
 		msgs[1].buf[j] = (cmd >> (8 * i)) & 0xff;

@@ -617,9 +624,7 @@ static int octeon_i2c_hlc_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msg
 
 	cmd = __raw_readq(i2c->twsi_base + SW_TWSI(i2c));
 	if ((cmd & SW_TWSI_R) == 0)
-		return -EAGAIN;
-
-	ret = octeon_i2c_check_status(i2c, false);
+		return octeon_i2c_check_status(i2c, false);
 
 err:
 	return ret;
Signature · 1 line
1.9.1
Applied
#6 lore raw

Re: [PATCH 3/5] i2c: octeon,thunderx: Fix high-level controller status check

Quoted 6 lines · Jan Glauber

In case the high-level controller (HLC) is used the status code is reported at a different location. Check that location after HLC write operations if the ready bit is not set and return an appropriate error code instead of always returning -EAGAIN.

Signed-off-by Jan Glauber <[email protected]>

Applied to for-next, thanks!

author
#7 lore raw

PATCH 4/5 i2c: octeon,thunderx: Check bus state before starting a transaction

In reply to Jan Glauber

Add an additional status check before starting a transaction and, if required, trigger the recovery if the check fails.

Signed-off-by Jan Glauber <[email protected]>
---
 drivers/i2c/busses/i2c-octeon-core.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
drivers/i2c/busses/i2c-octeon-core.c +20 -0
index 5e63b17..b21aa3e 100644
@@ -630,6 +630,22 @@ static int octeon_i2c_hlc_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msg
 	return ret;
 }
 
+static int octeon_i2c_check_bus(struct octeon_i2c *i2c)
+{
+	int stat, lines;
+
+	stat = octeon_i2c_stat_read(i2c);
+
+	/* get I2C line state */
+	lines = octeon_i2c_read_int(i2c) & (TWSI_INT_SCL | TWSI_INT_SDA);
+
+	if (stat == STAT_IDLE && lines == (TWSI_INT_SCL | TWSI_INT_SDA))
+		return 0;
+
+	/* bus check failed, try to recover */
+	return octeon_i2c_recovery(i2c);
+}
+
 /**
  * octeon_i2c_xfer - The driver's master_xfer function
  * @adap: Pointer to the i2c_adapter structure

@@ -643,6 +659,10 @@ int octeon_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	struct octeon_i2c *i2c = i2c_get_adapdata(adap);
 	int i, ret = 0;
 
+	ret = octeon_i2c_check_bus(i2c);
+	if (ret)
+		goto out;
+
 	if (num == 1) {
 		if (msgs[0].len > 0 && msgs[0].len <= 8) {
 			if (msgs[0].flags & I2C_M_RD)
Signature · 1 line
1.9.1
#8 lore raw

Re: [PATCH 4/5] i2c: octeon,thunderx: Check bus state before starting a transaction

Quoted 4 lines · Jan Glauber

Add an additional status check before starting a transaction and, if required, trigger the recovery if the check fails.

Signed-off-by Jan Glauber <[email protected]>

Won't this break multi-master setups?

#9 lore raw

Re: [PATCH 4/5] i2c: octeon,thunderx: Check bus state before starting a transaction

Quoted 8 lines · Wolfram Sang
Quoted 4 lines · Jan Glauber

Add an additional status check before starting a transaction and, if required, trigger the recovery if the check fails.

Signed-off-by Jan Glauber <[email protected]>

Won't this break multi-master setups?

I'm afraid yes. I don't have a multi-master setup, but agree that we should not break it.

How about re-checking if the bus is idle until a timeout (i2c->adap.timeout ?) happens and only then recover?

Additionaly we can check for arbitration loss as Dmitry did in his original patch.

--Jan

author
#10 lore raw

PATCH 5/5 i2c: octeon,thunderx: Limit register access retries

In reply to Jan Glauber

Do not infinitely retry register readq and writeq operations in order to not lock up the CPU in case the TWSI gets stuck.

Return -EIO in case of a failed data read. For all other cases just return so subsequent operations will fail and trigger the recovery.

Signed-off-by Jan Glauber <[email protected]>
---
 drivers/i2c/busses/i2c-octeon-core.c |  4 +++-
 drivers/i2c/busses/i2c-octeon-core.h | 21 ++++++++++++++++-----
 2 files changed, 19 insertions(+), 6 deletions(-)
drivers/i2c/busses/i2c-octeon-core.c +3 -1
index b21aa3e..6ba0146 100644
@@ -381,7 +381,9 @@ static int octeon_i2c_read(struct octeon_i2c *i2c, int target,
 		if (result)
 			return result;
 
-		data[i] = octeon_i2c_data_read(i2c);
+		data[i] = octeon_i2c_data_read(i2c, &result);
+		if (result)
+			return result;
 		if (recv_len && i == 0) {
 			if (data[i] > I2C_SMBUS_BLOCK_MAX + 1)
 				return -EPROTO;
drivers/i2c/busses/i2c-octeon-core.h +16 -5
index 87151ea..177d4cc 100644
@@ -141,11 +141,14 @@ static inline void octeon_i2c_writeq_flush(u64 val, void __iomem *addr)
  */
 static inline void octeon_i2c_reg_write(struct octeon_i2c *i2c, u64 eop_reg, u8 data)
 {
+	int tries = 100;
 	u64 tmp;
 
 	__raw_writeq(SW_TWSI_V | eop_reg | data, i2c->twsi_base + SW_TWSI(i2c));
 	do {
 		tmp = __raw_readq(i2c->twsi_base + SW_TWSI(i2c));
+		if (--tries < 0)
+			return;
 	} while ((tmp & SW_TWSI_V) != 0);
 }
 

@@ -163,24 +166,32 @@ static inline void octeon_i2c_reg_write(struct octeon_i2c *i2c, u64 eop_reg, u8
  *
  * The I2C core registers are accessed indirectly via the SW_TWSI CSR.
  */
-static inline u8 octeon_i2c_reg_read(struct octeon_i2c *i2c, u64 eop_reg)
+static inline int octeon_i2c_reg_read(struct octeon_i2c *i2c, u64 eop_reg,
+				      int *error)
 {
+	int tries = 100;
 	u64 tmp;
 
 	__raw_writeq(SW_TWSI_V | eop_reg | SW_TWSI_R, i2c->twsi_base + SW_TWSI(i2c));
 	do {
 		tmp = __raw_readq(i2c->twsi_base + SW_TWSI(i2c));
+		if (--tries < 0) {
+			/* signal that the returned data is invalid */
+			if (error)
+				*error = -EIO;
+			return 0;
+		}
 	} while ((tmp & SW_TWSI_V) != 0);
 
 	return tmp & 0xFF;
 }
 
 #define octeon_i2c_ctl_read(i2c)					\
-	octeon_i2c_reg_read(i2c, SW_TWSI_EOP_TWSI_CTL)
-#define octeon_i2c_data_read(i2c)					\
-	octeon_i2c_reg_read(i2c, SW_TWSI_EOP_TWSI_DATA)
+	octeon_i2c_reg_read(i2c, SW_TWSI_EOP_TWSI_CTL, NULL)
+#define octeon_i2c_data_read(i2c, error)				\
+	octeon_i2c_reg_read(i2c, SW_TWSI_EOP_TWSI_DATA, error)
 #define octeon_i2c_stat_read(i2c)					\
-	octeon_i2c_reg_read(i2c, SW_TWSI_EOP_TWSI_STAT)
+	octeon_i2c_reg_read(i2c, SW_TWSI_EOP_TWSI_STAT, NULL)
 
 /**
  * octeon_i2c_read_int - read the TWSI_INT register
Signature · 1 line
1.9.1
#11 lore raw

Re: [PATCH 5/5] i2c: octeon,thunderx: Limit register access retries

Quoted 8 lines · Jan Glauber

Do not infinitely retry register readq and writeq operations in order to not lock up the CPU in case the TWSI gets stuck.

Return -EIO in case of a failed data read. For all other cases just return so subsequent operations will fail and trigger the recovery.

Signed-off-by Jan Glauber <[email protected]>

I didn't really check, but have you considered using readq_poll_timeout() from iopoll.h?

#12 lore raw

Re: [PATCH 5/5] i2c: octeon,thunderx: Limit register access retries

Quoted 4 lines · Wolfram Sang
Quoted 3 lines · Jan Glauber

Do not infinitely retry register readq and writeq operations in order to not lock up the CPU in case the TWSI gets stuck.

Quoted 9 lines
Quoted 5 lines

Return -EIO in case of a failed data read. For all other cases just return so subsequent operations will fail and trigger the recovery.

Signed-off-by Jan Glauber <[email protected]>

I didn't really check, but have you considered using readq_poll_timeout() from iopoll.h?

Indeed, readq_poll_timeout() fits quite well here. It will lose some cycles on mips but I'm not convinced that matters with i2c.

That would be the first user of readq_poll_timeout() in the kernel :)

--Jan