[PATCH 1/2] i2c: octeon: Fix register access

[PATCH 1/2] i2c: octeon: Fix register access

[Paul Burton]

Commit 70121f7f3725 ("i2c: octeon: thunderx: Limit register access
retries") attempted to replace potentially infinite loops with ones
which will time out using readq_poll_timeout, but in doing so it
inverted the condition for exiting this loop.

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

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: Jan Glauber <jglauber@cavium.com>
Cc: David Daney <david.daney@cavium.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: linux-i2c@vger.kernel.org

---

 drivers/i2c/busses/i2c-octeon-core.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.h b/drivers/i2c/busses/i2c-octeon-core.h
index 1db7c83..d980406 100644
--- a/drivers/i2c/busses/i2c-octeon-core.h
+++ b/drivers/i2c/busses/i2c-octeon-core.h
@@ -146,8 +146,9 @@ static inline void octeon_i2c_reg_write(struct octeon_i2c *i2c, u64 eop_reg, u8
 
 	__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);
+	readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
+			   !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
+			   i2c->adap.timeout);
 }
 
 #define octeon_i2c_ctl_write(i2c, val)					\
@@ -173,7 +174,7 @@ static inline int octeon_i2c_reg_read(struct octeon_i2c *i2c, u64 eop_reg,
 	__raw_writeq(SW_TWSI_V | eop_reg | SW_TWSI_R, i2c->twsi_base + SW_TWSI(i2c));
 
 	ret = readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
-				 tmp & SW_TWSI_V, I2C_OCTEON_EVENT_WAIT,
+				 !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
 				 i2c->adap.timeout);
 	if (error)
 		*error = ret;
-- 
2.10.2

[PATCH 2/2] i2c: octeon: Fix waiting for operation completion

[Paul Burton]

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 <paul.burton@imgtec.com>
Cc: David Daney <david.daney@cavium.com>
Cc: Jan Glauber <jglauber@cavium.com>
Cc: Peter Swain <pswain@cavium.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: linux-i2c@vger.kernel.org
---

 drivers/i2c/busses/i2c-octeon-core.c | 58 ++++++++++--------------------------
 1 file changed, 15 insertions(+), 43 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index 419b54b..2e270a1 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
@@ -80,8 +62,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 +86,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 +145,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 +162,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);
-- 
2.10.2

Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion

[Jan Glauber]

On Mon, Nov 07, 2016 at 08:09:21PM +0000, Paul Burton wrote:
> 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.

This patch breaks ipmi on ThunderX (CN88xx). We also have not seen the
problems you mention, although I must admit that I don't like the
complicated nested waits.

--Jan

> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Cc: David Daney <david.daney@cavium.com>
> Cc: Jan Glauber <jglauber@cavium.com>
> Cc: Peter Swain <pswain@cavium.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: linux-i2c@vger.kernel.org
> ---
> 
>  drivers/i2c/busses/i2c-octeon-core.c | 58 ++++++++++--------------------------
>  1 file changed, 15 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
> index 419b54b..2e270a1 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
> @@ -80,8 +62,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 +86,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 +145,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 +162,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);
> -- 
> 2.10.2

Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion

[Jan Glauber]

Hi Paul,

I think we should revert commit "70121f7 i2c: octeon: thunderx: Limit
register access retries". With debugging enabled I'm getting:

[   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
[   78.910218] Modules linked in: ipmi_ssif i2c_thunderx i2c_smbus nicvf nicpf thunder_bgx thunder_xcv mdio_thunder

[   78.921916] CPU: 6 PID: 2235 Comm: bash Tainted: G        W       4.9.0-rc3-jang+ #17
[   78.929737] Hardware name: www.cavium.com ThunderX CRB-1S/ThunderX CRB-1S, BIOS 0.3 Aug 24 2016
[   78.938426] task: fffffe1fdd554500 task.stack: fffffe1fe384c000
[   78.944338] PC is at __might_sleep+0x80/0x88
[   78.948601] LR is at __might_sleep+0x80/0x88
[   78.952863] pc : [<fffffc00080c3aac>] lr : [<fffffc00080c3aac>] pstate: 80000145
[   78.960250] sp : fffffe1fe384f600
[   78.963557] x29: fffffe1fe384f600 x28: fffffe1fe384f860
[   78.968875] x27: fffffe1fd07fa018 x26: fffffe1fe384f968
[   78.974193] x25: fffffc0009a2b000 x24: 00000000ffff26d6
[   78.979510] x23: fffffe1fe384f860 x22: fffffe1fe384f860
[   78.984827] x21: 0000000000000000 x20: 00000000000000b1
[   78.990144] x19: fffffc0000e330b8 x18: 0000000000005bb0
[   78.995461] x17: fffffc0009669ca8 x16: 0000000000000000
[   79.000779] x15: 0000000000000539 x14: 66663c5b20746120
[   79.006097] x13: 74657320323d6574 x12: 617473203b474e49
[   79.011415] x11: 4e4e55525f4b5341 x10: 5421206e65687720
[   79.016732] x9 : 73706f20676e696b x8 : 0000000000000000
[   79.022049] x7 : fffffc00080f5740 x6 : fffffc00080f5740
[   79.027367] x5 : ffffffffffffff80 x4 : 0000000000000060
[   79.032684] x3 : 0000000000000000 x2 : 0000000000000001
[   79.038001] x1 : 0000000000000000 x0 : 0000000000000071

[   79.044803] ---[ end trace d8af6005f683d444 ]---
[   79.049413] Call trace:
[   79.051853] Exception stack(0xfffffe1fe384f420 to 0xfffffe1fe384f550)
[   79.058287] f420: fffffc0000e330b8 0000040000000000 fffffe1fe384f600 fffffc00080c3aac
[   79.066109] f440: 0000000080000145 000000000000003d 0000000000000000 fffffc0008853920
[   79.073931] f460: 0000040000000000 0000000100000001 fffffe1fe384f520 fffffc00080f60d8
[   79.081752] f480: fffffc0000e330b8 00000000000000b1 0000000000000000 fffffe1fe384f860
[   79.089574] f4a0: fffffe1fe384f860 00000000ffff26d6 fffffc0009a2b000 fffffe1fe384f968
[   79.097396] f4c0: fffffe1fd07fa018 fffffe1fe384f860 0000000000000071 0000000000000000
[   79.105218] f4e0: 0000000000000001 0000000000000000 0000000000000060 ffffffffffffff80
[   79.113040] f500: fffffc00080f5740 fffffc00080f5740 0000000000000000 73706f20676e696b
[   79.120861] f520: 5421206e65687720 4e4e55525f4b5341 617473203b474e49 74657320323d6574
[   79.128683] f540: 66663c5b20746120 0000000000000539
[   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]

This is not caused by the usleep inside the wait_event but by readq_poll_timeout().
Could you try if it works for you if you only revert this patch?

Thanks,
Jan

Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion

[Paul Burton]

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

On Wednesday, 9 November 2016 14:41:03 GMT Jan Glauber wrote:
> Hi Paul,
> 
> I think we should revert commit "70121f7 i2c: octeon: thunderx: Limit
> register access retries". With debugging enabled I'm getting:
> 
> <snip>
> 
> This is not caused by the usleep inside the wait_event but by
> readq_poll_timeout(). Could you try if it works for you if you only revert
> this patch?
> 
> Thanks,
> Jan

Hi Jan,

If I drop both my patches & just revert 70121f7f3725 ("i2c: octeon: thunderx: 
Limit register access retries") sadly it doesn't fix my system. A boot of a 
cavium_octeon_defconfig kernel with initcall_debug ends with:

  calling  octeon_mgmt_mod_init+0x0/0x28 @ 1
  initcall octeon_mgmt_mod_init+0x0/0x28 returned 0 after 67 usecs
  calling  ds1307_driver_init+0x0/0x10 @ 1
  initcall ds1307_driver_init+0x0/0x10 returned 0 after 19 usecs
  calling  octeon_i2c_driver_init+0x0/0x10 @ 1
  ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
  ata1.00: ATA-9: SanDisk SDSSDA240G, Z22000RL, max UDMA/133
  ata1.00: 468862128 sectors, multi 1: LBA48 NCQ (depth 31/32)
  ata1.00: configured for UDMA/133
  scsi 0:0:0:0: Direct-Access     ATA      SanDisk SDSSDA24 00RL PQ: 0 ANSI: 5
  sd 0:0:0:0: [sda] 468862128 512-byte logical blocks: (240 GB/224 GiB)
  sd 0:0:0:0: [sda] Write Protect is off
  sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
  sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support 
DPO or FUA
   sda: sda1 sda2 sda3 sda4
  sd 0:0:0:0: [sda] Attached SCSI disk
  ata2: SATA link down (SStatus 0 SControl 300)
  random: crng init done

As you can see octeon_i2c_driver_init never returns. Are you able to test on 
one of your MIPS-based systems?

Thanks,
    Paul

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion

[Jan Glauber]

On Wed, Nov 09, 2016 at 02:07:58PM +0000, Paul Burton wrote:
> On Wednesday, 9 November 2016 14:41:03 GMT Jan Glauber wrote:
> > Hi Paul,
> > 
> > I think we should revert commit "70121f7 i2c: octeon: thunderx: Limit
> > register access retries". With debugging enabled I'm getting:
> > 
> > <snip>
> > 
> > This is not caused by the usleep inside the wait_event but by
> > readq_poll_timeout(). Could you try if it works for you if you only revert
> > this patch?
> > 
> > Thanks,
> > Jan
> 
> Hi Jan,
> 
> If I drop both my patches & just revert 70121f7f3725 ("i2c: octeon: thunderx: 
> Limit register access retries") sadly it doesn't fix my system. A boot of a 
> cavium_octeon_defconfig kernel with initcall_debug ends with:
> 
>   calling  octeon_mgmt_mod_init+0x0/0x28 @ 1
>   initcall octeon_mgmt_mod_init+0x0/0x28 returned 0 after 67 usecs
>   calling  ds1307_driver_init+0x0/0x10 @ 1
>   initcall ds1307_driver_init+0x0/0x10 returned 0 after 19 usecs
>   calling  octeon_i2c_driver_init+0x0/0x10 @ 1
>   ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>   ata1.00: ATA-9: SanDisk SDSSDA240G, Z22000RL, max UDMA/133
>   ata1.00: 468862128 sectors, multi 1: LBA48 NCQ (depth 31/32)
>   ata1.00: configured for UDMA/133
>   scsi 0:0:0:0: Direct-Access     ATA      SanDisk SDSSDA24 00RL PQ: 0 ANSI: 5
>   sd 0:0:0:0: [sda] 468862128 512-byte logical blocks: (240 GB/224 GiB)
>   sd 0:0:0:0: [sda] Write Protect is off
>   sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
>   sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support 
> DPO or FUA
>    sda: sda1 sda2 sda3 sda4
>   sd 0:0:0:0: [sda] Attached SCSI disk
>   ata2: SATA link down (SStatus 0 SControl 300)
>   random: crng init done
> 
> As you can see octeon_i2c_driver_init never returns. Are you able to test on 
> one of your MIPS-based systems?

CC'ing Steven who might be able to test on MIPS.

> Thanks,
>     Paul

Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion

[Jan Glauber]

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

> As you can see octeon_i2c_driver_init never returns. Are you able to test on 
> one of your MIPS-based systems?
> 
> Thanks,
>     Paul

Hi Paul,

we can reproduce the problem on our side, but I don't have direct access
to a MIPS system so I'm still just guessing what happens. If you want
you can try the attached patches.

I'm trying to get rid of the polling around the interrupt altogether.
It works fine on Thunderx, sometimes I run into an interrupt timeout 
after a lost-arbitration but recovery/retry is able to clean that up.

Please also revert 70121f7 as before. The last patch adds some debugging
output to see if we run into timeouts or recovery.

thanks,
Jan

[-- Attachment #2: 0001-i2c-octeon-thunderx-TWSI-software-reset-in-recovery.patch --]
[-- Type: text/x-diff, Size: 1304 bytes --]

>From 21a158efd0b6c5a08af8f1c1b078bc4b40291bbf Mon Sep 17 00:00:00 2001
From: Jan Glauber <jglauber@cavium.com>
Date: Fri, 11 Nov 2016 09:17:19 +0100
Subject: [PATCH 1/3] i2c: octeon: thunderx: TWSI software reset in recovery

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 <jglauber@cavium.com>
---
 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 419b54b..0b02070 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -791,6 +791,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


[-- Attachment #3: 0002-i2c-octeon-thunderx-Remove-polling-after-interrupt.patch --]
[-- Type: text/x-diff, Size: 3279 bytes --]

>From d904526a298edcf1bfba173b1b71a677eb413677 Mon Sep 17 00:00:00 2001
From: Jan Glauber <jglauber@cavium.com>
Date: Fri, 11 Nov 2016 09:34:34 +0100
Subject: [PATCH 2/3] i2c: octeon: thunderx: Remove polling after interrupt

Remove the polling after the interrupt. In case the IFLG
is not set although the interrupt occured we will run into
a timeout and retry the operation. This should happen very
seldom.

Note: the default timeout (1s) can be changed via an ioclt
per device.

Signed-off-by: Jan Glauber <jglauber@cavium.com>
---
 drivers/i2c/busses/i2c-octeon-core.c | 43 ++----------------------------------
 1 file changed, 2 insertions(+), 41 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index 0b02070..1d8775799 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,7 +61,7 @@ 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),
+	time_left = wait_event_timeout(i2c->queue, octeon_i2c_test_iflg(i2c),
 				       i2c->adap.timeout);
 	i2c->int_disable(i2c);
 
@@ -102,25 +83,6 @@ 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);
-}
-
 static void octeon_i2c_hlc_int_clear(struct octeon_i2c *i2c)
 {
 	/* clear ST/TS events, listen for neither */
@@ -176,7 +138,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;
 
 	/*
@@ -195,7 +156,7 @@ 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),
+				       octeon_i2c_hlc_test_valid(i2c),
 				       i2c->adap.timeout);
 	i2c->hlc_int_disable(i2c);
 	if (!time_left)
-- 
1.9.1


[-- Attachment #4: 0003-i2c-octeon-thunderx-Debug-prints-for-timeout-and-rec.patch --]
[-- Type: text/x-diff, Size: 1291 bytes --]

>From 45eae05b8d793f5652f77ab1d5faa62c30927a10 Mon Sep 17 00:00:00 2001
From: Jan Glauber <jglauber@cavium.com>
Date: Fri, 11 Nov 2016 09:40:15 +0100
Subject: [PATCH 3/3] i2c: octeon: thunderx: Debug prints for timeout and
 recovery

Signed-off-by: Jan Glauber <jglauber@cavium.com>
---
 drivers/i2c/busses/i2c-octeon-core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c
index 1d8775799..a2a92b6 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -72,9 +72,10 @@ static int octeon_i2c_wait(struct octeon_i2c *i2c)
 		return 0;
 	}
 
-	if (!time_left)
+	if (!time_left) {
+		pr_err("%s: timed out\n", __func__);
 		return -ETIMEDOUT;
-
+	}
 	return 0;
 }
 
@@ -169,8 +170,10 @@ static int octeon_i2c_hlc_wait(struct octeon_i2c *i2c)
 		return 0;
 	}
 
-	if (!time_left)
+	if (!time_left) {
+		pr_err("%s: timed out\n", __func__);
 		return -ETIMEDOUT;
+	}
 	return 0;
 }
 
@@ -280,6 +283,7 @@ static int octeon_i2c_start(struct octeon_i2c *i2c)
 
 error:
 	/* START failed, try to recover */
+	pr_err("%s: try to recover from status: %d\n", __func__, stat);
 	ret = octeon_i2c_recovery(i2c);
 	return (ret) ? ret : -EAGAIN;
 }
-- 
1.9.1

Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion

[Steven J. Hill]

On 11/11/2016 02:57 AM, Jan Glauber wrote:
> 
> we can reproduce the problem on our side, but I don't have direct access
> to a MIPS system so I'm still just guessing what happens. If you want
> you can try the attached patches.
> 
> I'm trying to get rid of the polling around the interrupt altogether.
> It works fine on Thunderx, sometimes I run into an interrupt timeout 
> after a lost-arbitration but recovery/retry is able to clean that up.
> 
> Please also revert 70121f7 as before. The last patch adds some debugging
> output to see if we run into timeouts or recovery.
> 
Using your three patches without reverting 70121f7 fails on both 71xx
and 78xx boards. Paul's "[PATCH 1/2] i2c: octeon: Fix register access"
eliminates the i2c probe hang and both boards boot properly. Tested
on top of Linus' v4.9-rc4 tag.

Steve

Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion

[David Daney]

On 11/11/2016 12:51 PM, Steven J. Hill wrote:
> On 11/11/2016 02:57 AM, Jan Glauber wrote:
>>
>> we can reproduce the problem on our side, but I don't have direct access
>> to a MIPS system so I'm still just guessing what happens. If you want
>> you can try the attached patches.
>>
>> I'm trying to get rid of the polling around the interrupt altogether.
>> It works fine on Thunderx, sometimes I run into an interrupt timeout
>> after a lost-arbitration but recovery/retry is able to clean that up.
>>
>> Please also revert 70121f7 as before. The last patch adds some debugging
>> output to see if we run into timeouts or recovery.
>>
> Using your three patches without reverting 70121f7 fails on both 71xx
> and 78xx boards. Paul's "[PATCH 1/2] i2c: octeon: Fix register access"
> eliminates the i2c probe hang and both boards boot properly. Tested
> on top of Linus' v4.9-rc4 tag.
>

We need a definitive set of patches to apply.  If Paul's patches are not 
sufficient, work with Jan to create and test a set (perhaps including 
reversions) that will make it work on both OCTEON and Thunder, noting 
any dependencies on Paul's patches.  Then get that posted ASAP so that 
I2C can work again

David.

Re: [PATCH 1/2] i2c: octeon: Fix register access

[Jan Glauber]

On Mon, Nov 07, 2016 at 08:09:20PM +0000, Paul Burton wrote:
> Commit 70121f7f3725 ("i2c: octeon: thunderx: Limit register access
> retries") attempted to replace potentially infinite loops with ones
> which will time out using readq_poll_timeout, but in doing so it
> inverted the condition for exiting this loop.
> 
> Tested on a Rhino Labs UTM-8 with Octeon CN7130.
> 
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Cc: Jan Glauber <jglauber@cavium.com>
> Cc: David Daney <david.daney@cavium.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: linux-i2c@vger.kernel.org
> 
> ---

Acked-by: Jan Glauber <jglauber@cavium.com>

Thanks for spotting this. I think this should go into stable too for
4.8, so adding Cc: stable@vger.kernel.org.

>  drivers/i2c/busses/i2c-octeon-core.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-octeon-core.h b/drivers/i2c/busses/i2c-octeon-core.h
> index 1db7c83..d980406 100644
> --- a/drivers/i2c/busses/i2c-octeon-core.h
> +++ b/drivers/i2c/busses/i2c-octeon-core.h
> @@ -146,8 +146,9 @@ static inline void octeon_i2c_reg_write(struct octeon_i2c *i2c, u64 eop_reg, u8
>  
>  	__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);
> +	readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
> +			   !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
> +			   i2c->adap.timeout);
>  }
>  
>  #define octeon_i2c_ctl_write(i2c, val)					\
> @@ -173,7 +174,7 @@ static inline int octeon_i2c_reg_read(struct octeon_i2c *i2c, u64 eop_reg,
>  	__raw_writeq(SW_TWSI_V | eop_reg | SW_TWSI_R, i2c->twsi_base + SW_TWSI(i2c));
>  
>  	ret = readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
> -				 tmp & SW_TWSI_V, I2C_OCTEON_EVENT_WAIT,
> +				 !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
>  				 i2c->adap.timeout);
>  	if (error)
>  		*error = ret;
> -- 
> 2.10.2

Re: [PATCH 1/2] i2c: octeon: Fix register access

[Paul Burton]

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

On Tuesday, 8 November 2016 08:13:37 GMT Jan Glauber wrote:
> On Mon, Nov 07, 2016 at 08:09:20PM +0000, Paul Burton wrote:
> > Commit 70121f7f3725 ("i2c: octeon: thunderx: Limit register access
> > retries") attempted to replace potentially infinite loops with ones
> > which will time out using readq_poll_timeout, but in doing so it
> > inverted the condition for exiting this loop.
> > 
> > Tested on a Rhino Labs UTM-8 with Octeon CN7130.
> > 
> > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > Cc: Jan Glauber <jglauber@cavium.com>
> > Cc: David Daney <david.daney@cavium.com>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> > Cc: linux-i2c@vger.kernel.org
> > 
> > ---
> 
> Acked-by: Jan Glauber <jglauber@cavium.com>
> 
> Thanks for spotting this. I think this should go into stable too for
> 4.8, so adding Cc: stable@vger.kernel.org.

Hi Jan,

...but the bad patch was only merged for v4.9-rc1?

Thanks,
    Paul

> 
> >  drivers/i2c/busses/i2c-octeon-core.h | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-octeon-core.h
> > b/drivers/i2c/busses/i2c-octeon-core.h index 1db7c83..d980406 100644
> > --- a/drivers/i2c/busses/i2c-octeon-core.h
> > +++ b/drivers/i2c/busses/i2c-octeon-core.h
> > @@ -146,8 +146,9 @@ static inline void octeon_i2c_reg_write(struct
> > octeon_i2c *i2c, u64 eop_reg, u8> 
> >  	__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);
> > +	readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
> > +			   !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
> > +			   i2c->adap.timeout);
> > 
> >  }
> >  
> >  #define octeon_i2c_ctl_write(i2c, val)					\
> > 
> > @@ -173,7 +174,7 @@ static inline int octeon_i2c_reg_read(struct
> > octeon_i2c *i2c, u64 eop_reg,> 
> >  	__raw_writeq(SW_TWSI_V | eop_reg | SW_TWSI_R, i2c->twsi_base +
> >  	SW_TWSI(i2c));
> >  	
> >  	ret = readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
> > 
> > -				 tmp & SW_TWSI_V, I2C_OCTEON_EVENT_WAIT,
> > +				 !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
> > 
> >  				 i2c->adap.timeout);
> >  	
> >  	if (error)
> >  	
> >  		*error = ret;


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 1/2] i2c: octeon: Fix register access

[Jan Glauber]

On Wed, Nov 09, 2016 at 02:09:38PM +0000, Paul Burton wrote:
> On Tuesday, 8 November 2016 08:13:37 GMT Jan Glauber wrote:
> > On Mon, Nov 07, 2016 at 08:09:20PM +0000, Paul Burton wrote:
> > > Commit 70121f7f3725 ("i2c: octeon: thunderx: Limit register access
> > > retries") attempted to replace potentially infinite loops with ones
> > > which will time out using readq_poll_timeout, but in doing so it
> > > inverted the condition for exiting this loop.
> > > 
> > > Tested on a Rhino Labs UTM-8 with Octeon CN7130.
> > > 
> > > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > > Cc: Jan Glauber <jglauber@cavium.com>
> > > Cc: David Daney <david.daney@cavium.com>
> > > Cc: Wolfram Sang <wsa@the-dreams.de>
> > > Cc: linux-i2c@vger.kernel.org
> > > 
> > > ---
> > 
> > Acked-by: Jan Glauber <jglauber@cavium.com>
> > 
> > Thanks for spotting this. I think this should go into stable too for
> > 4.8, so adding Cc: stable@vger.kernel.org.
> 
> Hi Jan,
> 
> ...but the bad patch was only merged for v4.9-rc1?

true, I've misread it.

> Thanks,
>     Paul
> 
> > 
> > >  drivers/i2c/busses/i2c-octeon-core.h | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-octeon-core.h
> > > b/drivers/i2c/busses/i2c-octeon-core.h index 1db7c83..d980406 100644
> > > --- a/drivers/i2c/busses/i2c-octeon-core.h
> > > +++ b/drivers/i2c/busses/i2c-octeon-core.h
> > > @@ -146,8 +146,9 @@ static inline void octeon_i2c_reg_write(struct
> > > octeon_i2c *i2c, u64 eop_reg, u8> 
> > >  	__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);
> > > +	readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
> > > +			   !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
> > > +			   i2c->adap.timeout);
> > > 
> > >  }
> > >  
> > >  #define octeon_i2c_ctl_write(i2c, val)					\
> > > 
> > > @@ -173,7 +174,7 @@ static inline int octeon_i2c_reg_read(struct
> > > octeon_i2c *i2c, u64 eop_reg,> 
> > >  	__raw_writeq(SW_TWSI_V | eop_reg | SW_TWSI_R, i2c->twsi_base +
> > >  	SW_TWSI(i2c));
> > >  	
> > >  	ret = readq_poll_timeout(i2c->twsi_base + SW_TWSI(i2c), tmp,
> > > 
> > > -				 tmp & SW_TWSI_V, I2C_OCTEON_EVENT_WAIT,
> > > +				 !(tmp & SW_TWSI_V), I2C_OCTEON_EVENT_WAIT,
> > > 
> > >  				 i2c->adap.timeout);
> > >  	
> > >  	if (error)
> > >  	
> > >  		*error = ret;
> 

Re: [PATCH 1/2] i2c: octeon: Fix register access

[Wolfram Sang]

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

On Tue, Nov 08, 2016 at 08:13:37AM +0100, Jan Glauber wrote:
> On Mon, Nov 07, 2016 at 08:09:20PM +0000, Paul Burton wrote:
> > Commit 70121f7f3725 ("i2c: octeon: thunderx: Limit register access
> > retries") attempted to replace potentially infinite loops with ones
> > which will time out using readq_poll_timeout, but in doing so it
> > inverted the condition for exiting this loop.
> > 
> > Tested on a Rhino Labs UTM-8 with Octeon CN7130.
> > 
> > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > Cc: Jan Glauber <jglauber@cavium.com>
> > Cc: David Daney <david.daney@cavium.com>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> > Cc: linux-i2c@vger.kernel.org
> > 
> > ---
> 
> Acked-by: Jan Glauber <jglauber@cavium.com>
> 
> Thanks for spotting this. I think this should go into stable too for
> 4.8, so adding Cc: stable@vger.kernel.org.

Shall I still apply this one? Seems to me that 70121f7f3725 is still
under discussion of being reverted?


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

Re: [PATCH 1/2] i2c: octeon: Fix register access

[Jan Glauber]

On Thu, Nov 10, 2016 at 09:17:31PM +0100, Wolfram Sang wrote:
> On Tue, Nov 08, 2016 at 08:13:37AM +0100, Jan Glauber wrote:
> > On Mon, Nov 07, 2016 at 08:09:20PM +0000, Paul Burton wrote:
> > > Commit 70121f7f3725 ("i2c: octeon: thunderx: Limit register access
> > > retries") attempted to replace potentially infinite loops with ones
> > > which will time out using readq_poll_timeout, but in doing so it
> > > inverted the condition for exiting this loop.
> > > 
> > > Tested on a Rhino Labs UTM-8 with Octeon CN7130.
> > > 
> > > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > > Cc: Jan Glauber <jglauber@cavium.com>
> > > Cc: David Daney <david.daney@cavium.com>
> > > Cc: Wolfram Sang <wsa@the-dreams.de>
> > > Cc: linux-i2c@vger.kernel.org
> > > 
> > > ---
> > 
> > Acked-by: Jan Glauber <jglauber@cavium.com>
> > 
> > Thanks for spotting this. I think this should go into stable too for
> > 4.8, so adding Cc: stable@vger.kernel.org.
> 
> Shall I still apply this one? Seems to me that 70121f7f3725 is still
> under discussion of being reverted?
> 

70121f7 needs to be reverted, but it looks like i2c devices are still
not working on Octeon after that. We are still debugging the issue,
so I would suggest to wait for some more days.

--Jan