linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] Bug fixes for octeon driver
@ 2016-06-08  6:51 Jan Glauber
  2016-06-08  6:51 ` [PATCH 1/3] i2c: octeon: Missing AAK flag in case of I2C_M_RECV_LEN Jan Glauber
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Jan Glauber @ 2016-06-08  6:51 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-kernel, linux-i2c, David Daney, Steven.Hill, Jan Glauber

Testing ipmi_ssif on ThunderX several bugs were found that also
apply to the Octeon i2c driver changes coming with 4.7.

I'll need to rebase the pending ThunderX driver series after this
fixes which I'll do shortly.

Please consider for 4.7.

thanks,
Jan

Jan Glauber (3):
  i2c: octeon: Missing AAK flag in case of I2C_M_RECV_LEN
  i2c: octeon: Add retry logic after receiving STAT_RXADDR_NAK
  i2c: octeon: Avoid printk after too long SMBUS message

 drivers/i2c/busses/i2c-octeon.c | 45 ++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 14 deletions(-)

-- 
2.9.0.rc0.21.g7777322

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/3] i2c: octeon: Missing AAK flag in case of I2C_M_RECV_LEN
  2016-06-08  6:51 [PATCH 0/3] Bug fixes for octeon driver Jan Glauber
@ 2016-06-08  6:51 ` Jan Glauber
  2016-06-09 20:09   ` Wolfram Sang
  2016-06-08  6:51 ` [PATCH 2/3] i2c: octeon: Add retry logic after receiving STAT_RXADDR_NAK Jan Glauber
  2016-06-08  6:51 ` [PATCH 3/3] i2c: octeon: Avoid printk after too long SMBUS message Jan Glauber
  2 siblings, 1 reply; 8+ messages in thread
From: Jan Glauber @ 2016-06-08  6:51 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-kernel, linux-i2c, David Daney, Steven.Hill, Jan Glauber

During receive the controller requires the AAK flag for all
bytes but the final one. This was wrong in case of I2C_M_RECV_LEN,
where the decision if the final byte is to be transmitted
happened before adding the additional received length byte.

Set the AAK flag if additional bytes are to be received.

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

diff --git a/drivers/i2c/busses/i2c-octeon.c b/drivers/i2c/busses/i2c-octeon.c
index aa5f01e..1922e4a 100644
--- a/drivers/i2c/busses/i2c-octeon.c
+++ b/drivers/i2c/busses/i2c-octeon.c
@@ -934,8 +934,15 @@ static int octeon_i2c_read(struct octeon_i2c *i2c, int target,
 		return result;
 
 	for (i = 0; i < length; i++) {
-		/* for the last byte TWSI_CTL_AAK must not be set */
-		if (i + 1 == length)
+		/*
+		 * For the last byte to receive TWSI_CTL_AAK must not be set.
+		 *
+		 * A special case is I2C_M_RECV_LEN where we don't know the
+		 * additional length yet. If recv_len is set we assume we're
+		 * not reading the final byte and therefore need to set
+		 * TWSI_CTL_AAK.
+		 */
+		if ((i + 1 == length) && !(recv_len && i == 0))
 			final_read = true;
 
 		/* clear iflg to allow next event */
-- 
2.9.0.rc0.21.g7777322

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/3] i2c: octeon: Add retry logic after receiving STAT_RXADDR_NAK
  2016-06-08  6:51 [PATCH 0/3] Bug fixes for octeon driver Jan Glauber
  2016-06-08  6:51 ` [PATCH 1/3] i2c: octeon: Missing AAK flag in case of I2C_M_RECV_LEN Jan Glauber
@ 2016-06-08  6:51 ` Jan Glauber
  2016-06-09 20:11   ` Wolfram Sang
  2016-06-08  6:51 ` [PATCH 3/3] i2c: octeon: Avoid printk after too long SMBUS message Jan Glauber
  2 siblings, 1 reply; 8+ messages in thread
From: Jan Glauber @ 2016-06-08  6:51 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-kernel, linux-i2c, David Daney, Steven.Hill, Jan Glauber

The controller specification states that when receiving STAT_RXADDR_NAK
the START should be sent again. Retry several times before finally
failing with -ENXIO.

Without this change the IPMI SSIF driver fails executing several commands
like 'ipmitool fru' on ThunderX.

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

diff --git a/drivers/i2c/busses/i2c-octeon.c b/drivers/i2c/busses/i2c-octeon.c
index 1922e4a..8ade7fb 100644
--- a/drivers/i2c/busses/i2c-octeon.c
+++ b/drivers/i2c/busses/i2c-octeon.c
@@ -880,6 +880,10 @@ static int octeon_i2c_write(struct octeon_i2c *i2c, int target,
 {
 	int i, result;
 
+	result = octeon_i2c_start(i2c);
+	if (result)
+		return result;
+
 	octeon_i2c_data_write(i2c, target << 1);
 	octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB);
 
@@ -918,9 +922,14 @@ static int octeon_i2c_write(struct octeon_i2c *i2c, int target,
 static int octeon_i2c_read(struct octeon_i2c *i2c, int target,
 			   u8 *data, u16 *rlength, bool recv_len)
 {
-	int i, result, length = *rlength;
+	int i, result, length = *rlength, retries = 10;
 	bool final_read = false;
 
+restart:
+	result = octeon_i2c_start(i2c);
+	if (result)
+		return result;
+
 	octeon_i2c_data_write(i2c, (target << 1) | 1);
 	octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB);
 
@@ -930,8 +939,17 @@ static int octeon_i2c_read(struct octeon_i2c *i2c, int target,
 
 	/* address OK ? */
 	result = octeon_i2c_check_status(i2c, false);
-	if (result)
-		return result;
+	if (result) {
+		/*
+		 * According to controller specification on STAT_RXADDR_NAK
+		 * the START should be repeated so retry several times before
+		 * giving up with -ENXIO.
+		 */
+		if (result == -ENXIO && --retries > 0)
+			goto restart;
+		else
+			return result;
+	}
 
 	for (i = 0; i < length; i++) {
 		/*
@@ -1019,10 +1037,6 @@ static int octeon_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 			break;
 		}
 
-		ret = octeon_i2c_start(i2c);
-		if (ret)
-			return ret;
-
 		if (pmsg->flags & I2C_M_RD)
 			ret = octeon_i2c_read(i2c, pmsg->addr, pmsg->buf,
 					      &pmsg->len, pmsg->flags & I2C_M_RECV_LEN);
-- 
2.9.0.rc0.21.g7777322

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/3] i2c: octeon: Avoid printk after too long SMBUS message
  2016-06-08  6:51 [PATCH 0/3] Bug fixes for octeon driver Jan Glauber
  2016-06-08  6:51 ` [PATCH 1/3] i2c: octeon: Missing AAK flag in case of I2C_M_RECV_LEN Jan Glauber
  2016-06-08  6:51 ` [PATCH 2/3] i2c: octeon: Add retry logic after receiving STAT_RXADDR_NAK Jan Glauber
@ 2016-06-08  6:51 ` Jan Glauber
  2016-06-09 20:09   ` Wolfram Sang
  2 siblings, 1 reply; 8+ messages in thread
From: Jan Glauber @ 2016-06-08  6:51 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-kernel, linux-i2c, David Daney, Steven.Hill, Jan Glauber

Remove the warning about a too long SMBUS message because
the ipmi_ssif driver triggers this warning too frequently so it
spams the message log.

Signed-off-by: Jan Glauber <jglauber@cavium.com>
---
 drivers/i2c/busses/i2c-octeon.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon.c b/drivers/i2c/busses/i2c-octeon.c
index 8ade7fb..83fd6d8 100644
--- a/drivers/i2c/busses/i2c-octeon.c
+++ b/drivers/i2c/busses/i2c-octeon.c
@@ -975,12 +975,8 @@ restart:
 
 		data[i] = octeon_i2c_data_read(i2c);
 		if (recv_len && i == 0) {
-			if (data[i] > I2C_SMBUS_BLOCK_MAX + 1) {
-				dev_err(i2c->dev,
-					"%s: read len > I2C_SMBUS_BLOCK_MAX %d\n",
-					__func__, data[i]);
+			if (data[i] > I2C_SMBUS_BLOCK_MAX + 1)
 				return -EPROTO;
-			}
 			length += data[i];
 		}
 
-- 
2.9.0.rc0.21.g7777322

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] i2c: octeon: Missing AAK flag in case of I2C_M_RECV_LEN
  2016-06-08  6:51 ` [PATCH 1/3] i2c: octeon: Missing AAK flag in case of I2C_M_RECV_LEN Jan Glauber
@ 2016-06-09 20:09   ` Wolfram Sang
  0 siblings, 0 replies; 8+ messages in thread
From: Wolfram Sang @ 2016-06-09 20:09 UTC (permalink / raw)
  To: Jan Glauber; +Cc: linux-kernel, linux-i2c, David Daney, Steven.Hill

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

On Wed, Jun 08, 2016 at 08:51:17AM +0200, Jan Glauber wrote:
> During receive the controller requires the AAK flag for all
> bytes but the final one. This was wrong in case of I2C_M_RECV_LEN,
> where the decision if the final byte is to be transmitted
> happened before adding the additional received length byte.
> 
> Set the AAK flag if additional bytes are to be received.
> 
> Signed-off-by: Jan Glauber <jglauber@cavium.com>

Applied to for-current, thanks!


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] i2c: octeon: Avoid printk after too long SMBUS message
  2016-06-08  6:51 ` [PATCH 3/3] i2c: octeon: Avoid printk after too long SMBUS message Jan Glauber
@ 2016-06-09 20:09   ` Wolfram Sang
  0 siblings, 0 replies; 8+ messages in thread
From: Wolfram Sang @ 2016-06-09 20:09 UTC (permalink / raw)
  To: Jan Glauber; +Cc: linux-kernel, linux-i2c, David Daney, Steven.Hill

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

On Wed, Jun 08, 2016 at 08:51:19AM +0200, Jan Glauber wrote:
> Remove the warning about a too long SMBUS message because
> the ipmi_ssif driver triggers this warning too frequently so it
> spams the message log.
> 
> Signed-off-by: Jan Glauber <jglauber@cavium.com>

Applied to for-current, thanks!


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] i2c: octeon: Add retry logic after receiving STAT_RXADDR_NAK
  2016-06-08  6:51 ` [PATCH 2/3] i2c: octeon: Add retry logic after receiving STAT_RXADDR_NAK Jan Glauber
@ 2016-06-09 20:11   ` Wolfram Sang
  2016-06-14  8:03     ` Jan Glauber
  0 siblings, 1 reply; 8+ messages in thread
From: Wolfram Sang @ 2016-06-09 20:11 UTC (permalink / raw)
  To: Jan Glauber; +Cc: linux-kernel, linux-i2c, David Daney, Steven.Hill

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

On Wed, Jun 08, 2016 at 08:51:18AM +0200, Jan Glauber wrote:
> The controller specification states that when receiving STAT_RXADDR_NAK
> the START should be sent again. Retry several times before finally
> failing with -ENXIO.
> 
> Without this change the IPMI SSIF driver fails executing several commands
> like 'ipmitool fru' on ThunderX.

Huh? Looks wrong to me. I'd say the client driver needs to retry. Only
that one knows if retrying is appropriate or a waste of time.


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] i2c: octeon: Add retry logic after receiving STAT_RXADDR_NAK
  2016-06-09 20:11   ` Wolfram Sang
@ 2016-06-14  8:03     ` Jan Glauber
  0 siblings, 0 replies; 8+ messages in thread
From: Jan Glauber @ 2016-06-14  8:03 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: linux-kernel, linux-i2c, David Daney, Steven.Hill

On Thu, Jun 09, 2016 at 10:11:51PM +0200, Wolfram Sang wrote:
> On Wed, Jun 08, 2016 at 08:51:18AM +0200, Jan Glauber wrote:
> > The controller specification states that when receiving STAT_RXADDR_NAK
> > the START should be sent again. Retry several times before finally
> > failing with -ENXIO.
> > 
> > Without this change the IPMI SSIF driver fails executing several commands
> > like 'ipmitool fru' on ThunderX.
> 
> Huh? Looks wrong to me. I'd say the client driver needs to retry. Only
> that one knows if retrying is appropriate or a waste of time.
> 

I've been debugging this and it turned out that there was an related issue with
the clock setting. With that corrected this patch is not needed anymore,
so you can drop it.

I still see a huge number of RXADDR_NAK's after START but the ipmi_ssif
driver retry logic seems to deal with that.

thanks,
Jan

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-06-14  8:19 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-08  6:51 [PATCH 0/3] Bug fixes for octeon driver Jan Glauber
2016-06-08  6:51 ` [PATCH 1/3] i2c: octeon: Missing AAK flag in case of I2C_M_RECV_LEN Jan Glauber
2016-06-09 20:09   ` Wolfram Sang
2016-06-08  6:51 ` [PATCH 2/3] i2c: octeon: Add retry logic after receiving STAT_RXADDR_NAK Jan Glauber
2016-06-09 20:11   ` Wolfram Sang
2016-06-14  8:03     ` Jan Glauber
2016-06-08  6:51 ` [PATCH 3/3] i2c: octeon: Avoid printk after too long SMBUS message Jan Glauber
2016-06-09 20:09   ` Wolfram Sang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).