Re: [PATCH 2/3] i2c: octeon: Add retry logic after receiving STAT_RXADDR_NAK

[Jan Glauber]

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

Re: [PATCH 2/3] i2c: octeon: Add retry logic after receiving STAT_RXADDR_NAK

[Jan Glauber]

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

Re: [PATCH 2/3] i2c: octeon: Add retry logic after receiving STAT_RXADDR_NAK

[Wolfram Sang]

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.



signature.asc
Description: PGP signature

Re: [PATCH 2/3] i2c: octeon: Add retry logic after receiving STAT_RXADDR_NAK

[Wolfram Sang]

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.



signature.asc
Description: PGP signature

[PATCH 2/3] i2c: octeon: Add retry logic after receiving STAT_RXADDR_NAK

[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 
---
 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,
  >len, pmsg->flags & 
I2C_M_RECV_LEN);
-- 
2.9.0.rc0.21.g322

[PATCH 2/3] i2c: octeon: Add retry logic after receiving STAT_RXADDR_NAK

[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 
---
 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,
  >len, pmsg->flags & 
I2C_M_RECV_LEN);
-- 
2.9.0.rc0.21.g322