[PATCH 0/5] i2c: octeon,thunderx: Recovery fixes and improvements

[PATCH 0/5] i2c: octeon,thunderx: Recovery fixes and improvements

[Jan Glauber]

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(-)

-- 
1.9.1

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

[Jan Glauber]

From: Dmitry Bazhenov <dmitry.bazhenov@auriga.com>

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 <dmitry.bazhenov@auriga.com>
Signed-off-by: Jan Glauber <jglauber@cavium.com>
[Changed commit message]
---
 drivers/i2c/busses/i2c-octeon-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index f45ea5e..f322242 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -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)
-- 
1.9.1

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

[Jan Glauber]

From: Dmitry Bazhenov <dmitry.bazhenov@auriga.com>

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 <dmitry.bazhenov@auriga.com>
Signed-off-by: Jan Glauber <jglauber@cavium.com>
[Changed commit message]
---
 drivers/i2c/busses/i2c-octeon-core.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index f322242..7d4df83 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -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);
 }
 
-- 
1.9.1

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

[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 <jglauber@cavium.com>
---
 drivers/i2c/busses/i2c-octeon-core.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index 7d4df83..5e63b17 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -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;
-- 
1.9.1

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

[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 <jglauber@cavium.com>
---
 drivers/i2c/busses/i2c-octeon-core.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index 5e63b17..b21aa3e 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -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)
-- 
1.9.1

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

[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 <jglauber@cavium.com>
---
 drivers/i2c/busses/i2c-octeon-core.c |  4 +++-
 drivers/i2c/busses/i2c-octeon-core.h | 21 ++++++++++++++++-----
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index b21aa3e..6ba0146 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -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;
diff --git a/drivers/i2c/busses/i2c-octeon-core.h b/drivers/i2c/busses/i2c-octeon-core.h
index 87151ea..177d4cc 100644
--- a/drivers/i2c/busses/i2c-octeon-core.h
+++ b/drivers/i2c/busses/i2c-octeon-core.h
@@ -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
-- 
1.9.1

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

[Wolfram Sang]

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

On Wed, Sep 21, 2016 at 08:51:05AM +0200, Jan Glauber wrote:
> Add an additional status check before starting a transaction and,
> if required, trigger the recovery if the check fails.
> 
> Signed-off-by: Jan Glauber <jglauber@cavium.com>

Won't this break multi-master setups?


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

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

[Wolfram Sang]

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

On Wed, Sep 21, 2016 at 08:51:02AM +0200, Jan Glauber wrote:
> From: Dmitry Bazhenov <dmitry.bazhenov@auriga.com>
> 
> 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 <dmitry.bazhenov@auriga.com>
> Signed-off-by: Jan Glauber <jglauber@cavium.com>
> [Changed commit message]

Applied to for-next, thanks!

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


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

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

[Wolfram Sang]

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

On Wed, Sep 21, 2016 at 08:51:03AM +0200, Jan Glauber wrote:
> From: Dmitry Bazhenov <dmitry.bazhenov@auriga.com>
> 
> 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 <dmitry.bazhenov@auriga.com>
> Signed-off-by: Jan Glauber <jglauber@cavium.com>
> [Changed commit message]

Applied to for-next, thanks!


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

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

[Wolfram Sang]

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

On Wed, Sep 21, 2016 at 08:51:04AM +0200, Jan Glauber wrote:
> 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 <jglauber@cavium.com>

Applied to for-next, thanks!


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

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

[Wolfram Sang]

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

On Wed, Sep 21, 2016 at 08:51:06AM +0200, Jan Glauber wrote:
> 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 <jglauber@cavium.com>

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


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

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

[Jan Glauber]

On Wed, Sep 21, 2016 at 10:55:41PM +0200, Wolfram Sang wrote:
> On Wed, Sep 21, 2016 at 08:51:05AM +0200, Jan Glauber wrote:
> > Add an additional status check before starting a transaction and,
> > if required, trigger the recovery if the check fails.
> > 
> > Signed-off-by: Jan Glauber <jglauber@cavium.com>
> 
> 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

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

[Jan Glauber]

On Wed, Sep 21, 2016 at 10:55:41PM +0200, Wolfram Sang wrote:
> On Wed, Sep 21, 2016 at 08:51:05AM +0200, Jan Glauber wrote:
> > Add an additional status check before starting a transaction and,
> > if required, trigger the recovery if the check fails.
> > 
> > Signed-off-by: Jan Glauber <jglauber@cavium.com>
> 
> 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

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

[Jan Glauber]

On Wed, Sep 21, 2016 at 11:03:35PM +0200, Wolfram Sang wrote:
> On Wed, Sep 21, 2016 at 08:51:06AM +0200, Jan Glauber wrote:
> > 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 <jglauber@cavium.com>
> 
> 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

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

[Jan Glauber]

On Wed, Sep 21, 2016 at 11:03:35PM +0200, Wolfram Sang wrote:
> On Wed, Sep 21, 2016 at 08:51:06AM +0200, Jan Glauber wrote:
> > 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 <jglauber@cavium.com>
> 
> 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