[PATCH 0/4] i2c octeon & thunderx bug fixes

[PATCH 0/4] i2c octeon & thunderx bug fixes

[Jan Glauber]

Hi Wolfram,

Patches #1 & #2 contain the fixes that didn't make 4.9.
We've double-checked that they are working on Octeon MIPS cn71xx,
so I hope there are no surprises this time.

Patch #3 is my original attempt on limiting the number of
retries for the i2c device register access. As I found out we
need to keep this simple, because these functions are called
very early in the i2c driver and also from all types of context.

Patch #4 addresses a bug report I got. I haven't seen this myself,
but apparently depending on probing method and/or hardware type
ipmi_ssif can fail to detect the IPMI device. Setting the class
in the adapter solves the problem and seems harmless.

Tested on MIPS Octeon CN71xx and ARM64 ThunderX on 4.9-rc8.

thanks,
Jan

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

Jan Glauber (4):
  i2c: octeon: thunderx: TWSI software reset in recovery
  i2c: octeon: thunderx: Remove double-check after interrupt
  i2c: octeon: thunderx: Limit register access retries
  i2c: octeon: thunderx: Add I2C_CLASS_HWMON

 drivers/i2c/busses/i2c-octeon-core.c     | 50 +++++---------------------------
 drivers/i2c/busses/i2c-octeon-core.h     | 21 ++++++++++----
 drivers/i2c/busses/i2c-octeon-platdrv.c  |  1 +
 drivers/i2c/busses/i2c-thunderx-pcidrv.c |  1 +
 4 files changed, 26 insertions(+), 47 deletions(-)

-- 
2.9.0.rc0.21.g7777322

[PATCH 1/4] i2c: octeon: thunderx: TWSI software reset in recovery

[Jan Glauber]

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 5e63b17..2b8a7bf 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -789,6 +789,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
-- 
2.9.0.rc0.21.g7777322

Re: [PATCH 1/4] i2c: octeon: thunderx: TWSI software reset in recovery

[Wolfram Sang]

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

On Fri, Dec 09, 2016 at 10:31:55AM +0100, Jan Glauber wrote:
> 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>

Applied to for-next, thanks!


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

[PATCH 2/4] i2c: octeon: thunderx: Remove double-check after interrupt

[Jan Glauber]

Commit 1bb1ff3e7c74 ("i2c: octeon: Improve performance if interrupt is
early") added a double-check around the wait_event_timeout() condition.
The performance problem that this commit tried to work-around
could not be reproduced. It also makes the wait condition more
complicated then it should be. Therefore remove the double-check.

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 2b8a7bf..3d10f1a 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)
-- 
2.9.0.rc0.21.g7777322

Re: [PATCH 2/4] i2c: octeon: thunderx: Remove double-check after interrupt

[Wolfram Sang]

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

On Fri, Dec 09, 2016 at 10:31:56AM +0100, Jan Glauber wrote:
> Commit 1bb1ff3e7c74 ("i2c: octeon: Improve performance if interrupt is
> early") added a double-check around the wait_event_timeout() condition.
> The performance problem that this commit tried to work-around
> could not be reproduced. It also makes the wait condition more
> complicated then it should be. Therefore remove the double-check.
> 
> Signed-off-by: Jan Glauber <jglauber@cavium.com>

Applied to for-next, thanks!


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

[PATCH 3/4] i2c: octeon: thunderx: Limit register access retries

[Jan Glauber]

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 3d10f1a..1d8775799 100644
--- a/drivers/i2c/busses/i2c-octeon-core.c
+++ b/drivers/i2c/busses/i2c-octeon-core.c
@@ -342,7 +342,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..e160f83 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 = 1000;
 	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 = 1000;
 	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
-- 
2.9.0.rc0.21.g7777322

Re: [PATCH 3/4] i2c: octeon: thunderx: Limit register access retries

[Wolfram Sang]

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

On Fri, Dec 09, 2016 at 10:31:57AM +0100, 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 can't apply this one?


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

Re: [PATCH 3/4] i2c: octeon: thunderx: Limit register access retries

[Jan Glauber]

On Sun, Dec 11, 2016 at 11:01:48PM +0100, Wolfram Sang wrote:
> On Fri, Dec 09, 2016 at 10:31:57AM +0100, 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 can't apply this one?
> 

Strange. Applies for me on top of 4.9 and also next-20161212.
I can also apply if back from the mail. Is the mail messed up on your
side?

Re: [PATCH 3/4] i2c: octeon: thunderx: Limit register access retries

[Jan Glauber]

On Sun, Dec 11, 2016 at 11:01:48PM +0100, Wolfram Sang wrote:
> On Fri, Dec 09, 2016 at 10:31:57AM +0100, 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 can't apply this one?
> 

Strange. Applies for me on top of 4.9 and also next-20161212.
I can also apply if back from the mail. Is the mail messed up on your
side?

Re: [PATCH 3/4] i2c: octeon: thunderx: Limit register access retries

[Wolfram Sang]

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

> > 
> > I can't apply this one?
> > 
> 
> Strange. Applies for me on top of 4.9 and also next-20161212.
> I can also apply if back from the mail. Is the mail messed up on your
> side?

My fault, sorry for the noise. My branch missed the latest revert for
4.9. I'll apply this patch for a second pull request during the merge
window.


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

Re: [PATCH 3/4] i2c: octeon: thunderx: Limit register access retries

[Wolfram Sang]

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

On Fri, Dec 09, 2016 at 10:31:57AM +0100, 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>

Applied to for-current, thanks!


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

[PATCH 4/4] i2c: octeon: thunderx: Add I2C_CLASS_HWMON

[Jan Glauber]

It was reported that ipmi_ssif fails to create the
ipmi device on some systems if the adapter class is not containing
I2C_CLASS_HWMON. Fix it by setting the class.

Reported-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
Signed-off-by: Jan Glauber <jglauber@cavium.com>
---
 drivers/i2c/busses/i2c-octeon-platdrv.c  | 1 +
 drivers/i2c/busses/i2c-thunderx-pcidrv.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/i2c/busses/i2c-octeon-platdrv.c b/drivers/i2c/busses/i2c-octeon-platdrv.c
index 917524c..809b868 100644
--- a/drivers/i2c/busses/i2c-octeon-platdrv.c
+++ b/drivers/i2c/busses/i2c-octeon-platdrv.c
@@ -239,6 +239,7 @@ static int octeon_i2c_probe(struct platform_device *pdev)
 	i2c->adap = octeon_i2c_ops;
 	i2c->adap.timeout = msecs_to_jiffies(2);
 	i2c->adap.retries = 5;
+	i2c->adap.class = I2C_CLASS_HWMON;
 	i2c->adap.bus_recovery_info = &octeon_i2c_recovery_info;
 	i2c->adap.dev.parent = &pdev->dev;
 	i2c->adap.dev.of_node = node;
diff --git a/drivers/i2c/busses/i2c-thunderx-pcidrv.c b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
index bba5b42..9e3365f 100644
--- a/drivers/i2c/busses/i2c-thunderx-pcidrv.c
+++ b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
@@ -205,6 +205,7 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev,
 
 	i2c->adap = thunderx_i2c_ops;
 	i2c->adap.retries = 5;
+	i2c->adap.class = I2C_CLASS_HWMON;
 	i2c->adap.bus_recovery_info = &octeon_i2c_recovery_info;
 	i2c->adap.dev.parent = dev;
 	i2c->adap.dev.of_node = pdev->dev.of_node;
-- 
2.9.0.rc0.21.g7777322

Re: [PATCH 4/4] i2c: octeon: thunderx: Add I2C_CLASS_HWMON

[Wolfram Sang]

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

On Fri, Dec 09, 2016 at 10:31:58AM +0100, Jan Glauber wrote:
> It was reported that ipmi_ssif fails to create the
> ipmi device on some systems if the adapter class is not containing
> I2C_CLASS_HWMON. Fix it by setting the class.
> 
> Reported-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> Signed-off-by: Jan Glauber <jglauber@cavium.com>

The intention of adapter classes is to *limit* probing to a certain
class of devices. If a class is needed to *enable* probing, then
something there looks wrong. From the details given, this must be solved
elsewhere I'd say.


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

Re: [PATCH 4/4] i2c: octeon: thunderx: Add I2C_CLASS_HWMON

[Wolfram Sang]

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

On Sun, Dec 11, 2016 at 11:04:35PM +0100, Wolfram Sang wrote:
> On Fri, Dec 09, 2016 at 10:31:58AM +0100, Jan Glauber wrote:
> > It was reported that ipmi_ssif fails to create the
> > ipmi device on some systems if the adapter class is not containing
> > I2C_CLASS_HWMON. Fix it by setting the class.
> > 
> > Reported-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> > Signed-off-by: Jan Glauber <jglauber@cavium.com>
> 
> The intention of adapter classes is to *limit* probing to a certain
> class of devices. If a class is needed to *enable* probing, then
> something there looks wrong. From the details given, this must be solved
> elsewhere I'd say.

Makes sense?


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

Re: [PATCH 4/4] i2c: octeon: thunderx: Add I2C_CLASS_HWMON

[Jan Glauber]

On Wed, Jan 25, 2017 at 09:49:23PM +0100, Wolfram Sang wrote:
> On Sun, Dec 11, 2016 at 11:04:35PM +0100, Wolfram Sang wrote:
> > On Fri, Dec 09, 2016 at 10:31:58AM +0100, Jan Glauber wrote:
> > > It was reported that ipmi_ssif fails to create the
> > > ipmi device on some systems if the adapter class is not containing
> > > I2C_CLASS_HWMON. Fix it by setting the class.
> > > 
> > > Reported-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> > > Signed-off-by: Jan Glauber <jglauber@cavium.com>
> > 
> > The intention of adapter classes is to *limit* probing to a certain
> > class of devices. If a class is needed to *enable* probing, then
> > something there looks wrong. From the details given, this must be solved
> > elsewhere I'd say.
> 
> Makes sense?
> 

Yes, perfectly, the patch can be dropped.

thanks,
Jan

Re: [PATCH 4/4] i2c: octeon: thunderx: Add I2C_CLASS_HWMON

[Jan Glauber]

On Wed, Jan 25, 2017 at 09:49:23PM +0100, Wolfram Sang wrote:
> On Sun, Dec 11, 2016 at 11:04:35PM +0100, Wolfram Sang wrote:
> > On Fri, Dec 09, 2016 at 10:31:58AM +0100, Jan Glauber wrote:
> > > It was reported that ipmi_ssif fails to create the
> > > ipmi device on some systems if the adapter class is not containing
> > > I2C_CLASS_HWMON. Fix it by setting the class.
> > > 
> > > Reported-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> > > Signed-off-by: Jan Glauber <jglauber@cavium.com>
> > 
> > The intention of adapter classes is to *limit* probing to a certain
> > class of devices. If a class is needed to *enable* probing, then
> > something there looks wrong. From the details given, this must be solved
> > elsewhere I'd say.
> 
> Makes sense?
> 

Yes, perfectly, the patch can be dropped.

thanks,
Jan