[PATCH v2 0/3] i2c: octeon: thunder: Fix i2c not working on Octeon

Hi Wolfram,

Since time is running out for 4.9 (or might have already if you're not
going to send another pull request) I'm going for the safe option
to fix the Octeon i2c problems, which is:

1. Reverting the readq_poll_timeout patch since it is broken
2. Apply Patch #2 from Paul
3. Add a small fix for the recovery that makes Paul's patch
   work on ThunderX

I'll try to come up with a better solution for 4.10. My plan is to get rid
of the polling-around-interrupt thing completely, but for that we need more
time to make it work on Octeon.

Please consider for 4.9.

thanks,
Jan

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

Jan Glauber (2):
  Revert "i2c: octeon: thunderx: Limit register access retries"
  i2c: octeon: thunderx: TWSI software reset in recovery

Paul Burton (1):
  i2c: octeon: Fix waiting for operation completion

 drivers/i2c/busses/i2c-octeon-core.c | 66 +++++++++++-------------------------
 drivers/i2c/busses/i2c-octeon-core.h | 27 ++++++---------
 2 files changed, 30 insertions(+), 63 deletions(-)

-- 
1.9.1

This reverts commit 70121f7f3725 ("i2c: octeon: thunderx: Limit register access retries").
Using readq_poll_timeout instead of __raw_readq triggers the following
debug warning:

[   78.871568] ipmi_ssif: Trying hotmod-specified SSIF interface at i2c address 0x12, adapter Cavium ThunderX i2c adapter at 0000:01:09.4, slave address 0x0
[   78.886107] do not call blocking ops when !TASK_RUNNING; state=2 set at [<fffffc00080e0088>] prepare_to_wait_event+0x58/0x10c
[   78.897436] ------------[ cut here ]------------
[   78.902050] WARNING: CPU: 6 PID: 2235 at kernel/sched/core.c:7718 __might_sleep+0x80/0x88

[...]

[   79.133553] [<fffffc00080c3aac>] __might_sleep+0x80/0x88
[   79.138862] [<fffffc0000e30138>] octeon_i2c_test_iflg+0x4c/0xbc [i2c_thunderx]
[   79.146077] [<fffffc0000e30958>] octeon_i2c_test_ready+0x18/0x70 [i2c_thunderx]
[   79.153379] [<fffffc0000e30b04>] octeon_i2c_wait+0x154/0x1a4 [i2c_thunderx]
[   79.160334] [<fffffc0000e310bc>] octeon_i2c_xfer+0xf4/0xf60 [i2c_thunderx]

Signed-off-by: Jan Glauber <redacted>
---
 drivers/i2c/busses/i2c-octeon-core.c |  4 +---
 drivers/i2c/busses/i2c-octeon-core.h | 27 +++++++++++----------------
 2 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index 419b54b..5e63b17 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c

@@ -381,9 +381,7 @@ static int octeon_i2c_read(struct octeon_i2c *i2c, int target,
 		if (result)
 			return result;
 
-		data[i] = octeon_i2c_data_read(i2c, &result);
-		if (result)
-			return result;
+		data[i] = octeon_i2c_data_read(i2c);
 		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 1db7c83..87151ea 100644
--- a/drivers/i2c/busses/i2c-octeon-core.h
+++ b/drivers/i2c/busses/i2c-octeon-core.h

@@ -5,7 +5,6 @@
 #include <linux/i2c.h>
 #include <linux/i2c-smbus.h>
 #include <linux/io.h>
-#include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/pci.h>
 

@@ -145,9 +144,9 @@ static inline void octeon_i2c_reg_write(struct octeon_i2c *i2c, u64 eop_reg, u8
 	u64 tmp;
 
 	__raw_writeq(SW_TWSI_V | eop_reg | data, i2c->twsi_base + SW_TWSI(i2c));
-
-	readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp, tmp & SW_TWSI_V,
-			   I2C_OCTEON_EVENT_WAIT, i2c->adap.timeout);
+	do {
+		tmp = __raw_readq(i2c->twsi_base + SW_TWSI(i2c));
+	} while ((tmp & SW_TWSI_V) != 0);
 }
 
 #define octeon_i2c_ctl_write(i2c, val)					\

@@ -164,28 +163,24 @@ 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 int octeon_i2c_reg_read(struct octeon_i2c *i2c, u64 eop_reg,
-				      int *error)
+static inline u8 octeon_i2c_reg_read(struct octeon_i2c *i2c, u64 eop_reg)
 {
 	u64 tmp;
-	int ret;
 
 	__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));
+	} while ((tmp & SW_TWSI_V) != 0);
 
-	ret = readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
-				 tmp & SW_TWSI_V, I2C_OCTEON_EVENT_WAIT,
-				 i2c->adap.timeout);
-	if (error)
-		*error = ret;
 	return tmp & 0xFF;
 }
 
 #define octeon_i2c_ctl_read(i2c)					\
-	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)
+	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)
 #define octeon_i2c_stat_read(i2c)					\
-	octeon_i2c_reg_read(i2c, SW_TWSI_EOP_TWSI_STAT, NULL)
+	octeon_i2c_reg_read(i2c, SW_TWSI_EOP_TWSI_STAT)
 
 /**
  * octeon_i2c_read_int - read the TWSI_INT register

-- 
1.9.1

From: Paul Burton <redacted>

Commit 1bb1ff3e7c74 ("i2c: octeon: Improve performance if interrupt is
early") modified octeon_i2c_wait() & octeon_i2c_hlc_wait() to attempt to
check for a valid bit being clear & if not to sleep for a while then try
again before waiting on a waitqueue which may time out. However it does
so by sleeping within a function called as the condition provided to
wait_event_timeout() which seems to cause strange behaviour, with the
system hanging during boot with the condition being checked constantly &
the timeout not seeming to have any effect.

Fix this by instead checking for the valid bit being clear in the
octeon_i2c(_hlc)_wait() functions & sleeping there if that condition is
not met, then calling the wait_event_timeout with a condition that does
not sleep.

Tested on a Rhino Labs UTM-8 with Octeon CN7130.

Signed-off-by: Paul Burton <redacted>
Signed-off-by: Jan Glauber <redacted>
Cc: David Daney <redacted>
Cc: Jan Glauber <redacted>
[jglauber@cavium.com: removed unused variable]
Cc: Peter Swain <redacted>
Cc: Wolfram Sang <redacted>
Cc: linux-i2c@vger.kernel.org
---
 drivers/i2c/busses/i2c-octeon-core.c | 59 +++++++++---------------------------
 1 file changed, 15 insertions(+), 44 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index 5e63b17..54a9c14 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c

@@ -36,24 +36,6 @@ static bool octeon_i2c_test_iflg(struct octeon_i2c *i2c)
 	return (octeon_i2c_ctl_read(i2c) & TWSI_CTL_IFLG);
 }
 
-static bool octeon_i2c_test_ready(struct octeon_i2c *i2c, bool *first)
-{
-	if (octeon_i2c_test_iflg(i2c))
-		return true;
-
-	if (*first) {
-		*first = false;
-		return false;
-	}
-
-	/*
-	 * IRQ has signaled an event but IFLG hasn't changed.
-	 * Sleep and retry once.
-	 */
-	usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT);
-	return octeon_i2c_test_iflg(i2c);
-}
-
 /**
  * octeon_i2c_wait - wait for the IFLG to be set
  * @i2c: The struct octeon_i2c

@@ -63,7 +45,6 @@ static bool octeon_i2c_test_ready(struct octeon_i2c *i2c, bool *first)
 static int octeon_i2c_wait(struct octeon_i2c *i2c)
 {
 	long time_left;
-	bool first = true;
 
 	/*
 	 * Some chip revisions don't assert the irq in the interrupt

@@ -80,8 +61,13 @@ static int octeon_i2c_wait(struct octeon_i2c *i2c)
 	}
 
 	i2c->int_enable(i2c);
-	time_left = wait_event_timeout(i2c->queue, octeon_i2c_test_ready(i2c, &first),
-				       i2c->adap.timeout);
+	time_left = i2c->adap.timeout;
+	if (!octeon_i2c_test_iflg(i2c)) {
+		usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT);
+		time_left = wait_event_timeout(i2c->queue,
+					       octeon_i2c_test_iflg(i2c),
+					       time_left);
+	}
 	i2c->int_disable(i2c);
 
 	if (i2c->broken_irq_check && !time_left &&

@@ -99,26 +85,8 @@ static int octeon_i2c_wait(struct octeon_i2c *i2c)
 
 static bool octeon_i2c_hlc_test_valid(struct octeon_i2c *i2c)
 {
-	return (__raw_readq(i2c->twsi_base + SW_TWSI(i2c)) & SW_TWSI_V) == 0;
-}
-
-static bool octeon_i2c_hlc_test_ready(struct octeon_i2c *i2c, bool *first)
-{
 	/* check if valid bit is cleared */
-	if (octeon_i2c_hlc_test_valid(i2c))
-		return true;
-
-	if (*first) {
-		*first = false;
-		return false;
-	}
-
-	/*
-	 * IRQ has signaled an event but valid bit isn't cleared.
-	 * Sleep and retry once.
-	 */
-	usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT);
-	return octeon_i2c_hlc_test_valid(i2c);
+	return (__raw_readq(i2c->twsi_base + SW_TWSI(i2c)) & SW_TWSI_V) == 0;
 }
 
 static void octeon_i2c_hlc_int_clear(struct octeon_i2c *i2c)

@@ -176,7 +144,6 @@ static void octeon_i2c_hlc_disable(struct octeon_i2c *i2c)
  */
 static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c)
 {
-	bool first = true;
 	int time_left;
 
 	/*

@@ -194,9 +161,13 @@ static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c)
 	}
 
 	i2c->hlc_int_enable(i2c);
-	time_left = wait_event_timeout(i2c->queue,
-				       octeon_i2c_hlc_test_ready(i2c, &first),
-				       i2c->adap.timeout);
+	time_left = i2c->adap.timeout;
+	if (!octeon_i2c_hlc_test_valid(i2c)) {
+		usleep_range(I2C_OCTEON_EVENT_WAIT, 2 * I2C_OCTEON_EVENT_WAIT);
+		time_left = wait_event_timeout(i2c->queue,
+					       octeon_i2c_hlc_test_valid(i2c),
+					       time_left);
+	}
 	i2c->hlc_int_disable(i2c);
 	if (!time_left)
 		octeon_i2c_hlc_int_clear(i2c);

-- 
1.9.1

I've seen i2c recovery reporting long loops of:

[ 1035.887818] i2c i2c-4: SCL is stuck low, exit recovery
[ 1037.999748] i2c i2c-4: SCL is stuck low, exit recovery
[ 1040.111694] i2c i2c-4: SCL is stuck low, exit recovery
...

Add a TWSI software reset which clears the status and
STA,STP,IFLG in SW_TWSI_EOP_TWSI_CTL.

With this the recovery works fine and above message is not seen.

Signed-off-by: Jan Glauber <redacted>
---
 drivers/i2c/busses/i2c-octeon-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index 54a9c14..c3b63c1 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c

@@ -760,6 +760,9 @@ static void octeon_i2c_prepare_recovery(struct i2c_adapter *adap)
 	struct octeon_i2c *i2c = i2c_get_adapdata(adap);
 
 	octeon_i2c_hlc_disable(i2c);
+	octeon_i2c_reg_write(i2c, SW_TWSI_EOP_TWSI_RST, 0);
+	/* wait for software reset to settle */
+	udelay(5);
 
 	/*
 	 * Bring control register to a good state regardless

-- 
1.9.1

On 11/14/2016 12:50 PM, Jan Glauber wrote:

Since time is running out for 4.9 (or might have already if you're not
going to send another pull request) I'm going for the safe option
to fix the Octeon i2c problems, which is:

1. Reverting the readq_poll_timeout patch since it is broken
2. Apply Patch #2 from Paul
3. Add a small fix for the recovery that makes Paul's patch
   work on ThunderX

I'll try to come up with a better solution for 4.10. My plan is to get rid
of the polling-around-interrupt thing completely, but for that we need more
time to make it work on Octeon.

Please consider for 4.9.

Hey Jan.

This does not work on Octeon 71xx platforms. I will look at it more
closely tomorrow.

Steve

On Mon, Nov 14, 2016 at 01:53:40PM -0600, Steven J. Hill wrote:

On 11/14/2016 12:50 PM, Jan Glauber wrote:

quoted

Since time is running out for 4.9 (or might have already if you're not
going to send another pull request) I'm going for the safe option
to fix the Octeon i2c problems, which is:

1. Reverting the readq_poll_timeout patch since it is broken
2. Apply Patch #2 from Paul
3. Add a small fix for the recovery that makes Paul's patch
   work on ThunderX

I'll try to come up with a better solution for 4.10. My plan is to get rid
of the polling-around-interrupt thing completely, but for that we need more
time to make it work on Octeon.

Please consider for 4.9.

Hey Jan.

This does not work on Octeon 71xx platforms. I will look at it more
closely tomorrow.

Paul, can you confirm this? It doesn't make sense for me, since patches #1
and #3 are unlikely to break anything... And patch #2 worked for you.

--Jan

Steve

Hi Jan,

On Tuesday, 15 November 2016 14:03:15 GMT Jan Glauber wrote:

On Mon, Nov 14, 2016 at 01:53:40PM -0600, Steven J. Hill wrote:

quoted

On 11/14/2016 12:50 PM, Jan Glauber wrote:

quoted

Since time is running out for 4.9 (or might have already if you're not
going to send another pull request) I'm going for the safe option
to fix the Octeon i2c problems, which is:

1. Reverting the readq_poll_timeout patch since it is broken
2. Apply Patch #2 from Paul
3. Add a small fix for the recovery that makes Paul's patch

   work on ThunderX

I'll try to come up with a better solution for 4.10. My plan is to get
rid
of the polling-around-interrupt thing completely, but for that we need
more
time to make it work on Octeon.

Please consider for 4.9.

Hey Jan.

This does not work on Octeon 71xx platforms. I will look at it more
closely tomorrow.

Paul, can you confirm this? It doesn't make sense for me, since patches #1
and #3 are unlikely to break anything... And patch #2 worked for you.

For me v4.9-rc5 plus these 3 patches boots fine on a Rhino Labs UTM8 system 
which previously hung whilst probing the I2C driver & devices. Feel free to 
add:

    Tested-by: Paul Burton [off-list ref]

Thanks,
    Paul

On Mon, Nov 14, 2016 at 01:53:40PM -0600, Steven J. Hill wrote:

On 11/14/2016 12:50 PM, Jan Glauber wrote:

quoted

Since time is running out for 4.9 (or might have already if you're not
going to send another pull request) I'm going for the safe option
to fix the Octeon i2c problems, which is:

1. Reverting the readq_poll_timeout patch since it is broken
2. Apply Patch #2 from Paul
3. Add a small fix for the recovery that makes Paul's patch
   work on ThunderX

I'll try to come up with a better solution for 4.10. My plan is to get rid
of the polling-around-interrupt thing completely, but for that we need more
time to make it work on Octeon.

Please consider for 4.9.

Hey Jan.

This does not work on Octeon 71xx platforms. I will look at it more
closely tomorrow.

What's the outcome here? It seems we want a bugfix for 4.9 but this
report keeps me reluctant to apply the series.

On Tue, Nov 22, 2016 at 01:01:06PM +0100, Wolfram Sang wrote:

On Mon, Nov 14, 2016 at 01:53:40PM -0600, Steven J. Hill wrote:

quoted

On 11/14/2016 12:50 PM, Jan Glauber wrote:

quoted

Since time is running out for 4.9 (or might have already if you're not
going to send another pull request) I'm going for the safe option
to fix the Octeon i2c problems, which is:

1. Reverting the readq_poll_timeout patch since it is broken
2. Apply Patch #2 from Paul
3. Add a small fix for the recovery that makes Paul's patch
   work on ThunderX

I'll try to come up with a better solution for 4.10. My plan is to get rid
of the polling-around-interrupt thing completely, but for that we need more
time to make it work on Octeon.

Please consider for 4.9.

Hey Jan.

This does not work on Octeon 71xx platforms. I will look at it more
closely tomorrow.

What's the outcome here? It seems we want a bugfix for 4.9 but this
report keeps me reluctant to apply the series.

Steven, did you have a chance to check which of the patches makes
Octeon 71xx fail?

--Jan

quoted

quoted

This does not work on Octeon 71xx platforms. I will look at it more
closely tomorrow.

What's the outcome here? It seems we want a bugfix for 4.9 but this
report keeps me reluctant to apply the series.

Steven, did you have a chance to check which of the patches makes
Octeon 71xx fail?

How do we proceed with this one? Is somebody at Cavium able to contact
Steven internally? I mentioned this on-going regression to Linus and
said an rc8 would help us, but reading LWN it seems we shouldn't count
on it...

On 11/28/2016 08:22 AM, Wolfram Sang wrote:

quoted

quoted

quoted

This does not work on Octeon 71xx platforms. I will look at it more
closely tomorrow.

What's the outcome here? It seems we want a bugfix for 4.9 but this
report keeps me reluctant to apply the series.

Steven, did you have a chance to check which of the patches makes
Octeon 71xx fail?

How do we proceed with this one? Is somebody at Cavium able to contact
Steven internally? I mentioned this on-going regression to Linus and
said an rc8 would help us, but reading LWN it seems we shouldn't count
on it...

I will finish the bisecting today and will let you know as soon as
the bug is found.

Steve

On Mon, Nov 28, 2016 at 03:22:08PM +0100, Wolfram Sang wrote:

quoted

quoted

quoted

This does not work on Octeon 71xx platforms. I will look at it more
closely tomorrow.

What's the outcome here? It seems we want a bugfix for 4.9 but this
report keeps me reluctant to apply the series.

Steven, did you have a chance to check which of the patches makes
Octeon 71xx fail?

How do we proceed with this one? Is somebody at Cavium able to contact
Steven internally? I mentioned this on-going regression to Linus and
said an rc8 would help us, but reading LWN it seems we shouldn't count
on it...

Hi Wolfram,

if possible we should at least revert commit 70121f7f3725. I should get
access to an Octeon 71xx board tomorrow, but I'm afraid we'll miss the
deadline for a well tested fix that works across all machines.

--Jan

On 11/29/2016 03:19 AM, Jan Glauber wrote:

if possible we should at least revert commit 70121f7f3725. I should get
access to an Octeon 71xx board tomorrow, but I'm afraid we'll miss the
deadline for a well tested fix that works across all machines.

I second Jan's advice. Please revert the commit and we'll have the
fix for the next release.

On Tue, Nov 29, 2016 at 12:22:34PM -0600, Steven J. Hill wrote:

On 11/29/2016 03:19 AM, Jan Glauber wrote:

quoted

if possible we should at least revert commit 70121f7f3725. I should get
access to an Octeon 71xx board tomorrow, but I'm afraid we'll miss the
deadline for a well tested fix that works across all machines.

I second Jan's advice. Please revert the commit and we'll have the
fix for the next release.

Okay, I will pick up patch 1/3 from Jan (the revert) and read your
comment above as an ack. Paul, is your tested-by still valid?

Thanks,

   Wolfram

On 11/29/2016 12:37 PM, Wolfram Sang wrote:

Okay, I will pick up patch 1/3 from Jan (the revert) and read your
comment above as an ack. Paul, is your tested-by still valid?

Just to be explicitly clear, the patch revert allows I2C on OCTEON
to work again. I apologize if that was not clear.

Steve

On Tue, Nov 29, 2016 at 01:10:30PM -0600, Steven J. Hill wrote:

On 11/29/2016 12:37 PM, Wolfram Sang wrote:

quoted

Okay, I will pick up patch 1/3 from Jan (the revert) and read your
comment above as an ack. Paul, is your tested-by still valid?

Just to be explicitly clear, the patch revert allows I2C on OCTEON
to work again. I apologize if that was not clear.

Partly. It was not clear to me if only patch 1 was enough to get Paul's
system to boot again or if it needed all 3 patches to do that.

Anyway, the revert will be in v4.9 for sure.