Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1487843 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] i2c: octeon,thunderx: Recovery fixes and improvements Jan Glauber <jglauber@cavium.com> - 2016-09-21 09:00 +0200
    [PATCH 1/5] i2c: octeon,thunderx: Fix set SCL recovery function Jan Glauber <jglauber@cavium.com> - 2016-09-21 09:00 +0200
      Re: [PATCH 1/5] i2c: octeon,thunderx: Fix set SCL recovery function Wolfram Sang <wsa@the-dreams.de> - 2016-09-21 23:10 +0200
    [PATCH 5/5] i2c: octeon,thunderx: Limit register access retries Jan Glauber <jglauber@cavium.com> - 2016-09-21 09:00 +0200
      Re: [PATCH 5/5] i2c: octeon,thunderx: Limit register access retries Wolfram Sang <wsa@the-dreams.de> - 2016-09-21 23:10 +0200
    [PATCH 4/5] i2c: octeon,thunderx: Check bus state before starting a transaction Jan Glauber <jglauber@cavium.com> - 2016-09-21 09:00 +0200
      Re: [PATCH 4/5] i2c: octeon,thunderx: Check bus state before  starting a transaction Wolfram Sang <wsa@the-dreams.de> - 2016-09-21 23:00 +0200
    [PATCH 3/5] i2c: octeon,thunderx: Fix high-level controller status check Jan Glauber <jglauber@cavium.com> - 2016-09-21 09:00 +0200
      Re: [PATCH 3/5] i2c: octeon,thunderx: Fix high-level controller  status check Wolfram Sang <wsa@the-dreams.de> - 2016-09-21 23:10 +0200
    [PATCH 2/5] i2c: octeon,thunderx: Avoid sending STOP during recovery Jan Glauber <jglauber@cavium.com> - 2016-09-21 09:00 +0200
      Re: [PATCH 2/5] i2c: octeon,thunderx: Avoid sending STOP during  recovery Wolfram Sang <wsa@the-dreams.de> - 2016-09-21 23:10 +0200

#1487843 — [PATCH 0/5] 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(-)

-- 
1.9.1

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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?

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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?

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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!

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web