[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

[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

[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

[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 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 --]

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 --]

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 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 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 --]

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

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?
> 

Hi Wolfram,

I've looked at this again because apparently auto-detection of BMCs
(ipmi_ssif driver) is not working anymore.

Debugging it shows that ssif_probe is not called when ipmi_ssif is
trying to automatically detect BMCs. Adding the i2c device manually
works fine (ssif_probe called).

Setting the class value in the ThunderX i2c driver to I2C_CLASS_HWMON
makes the autodection of ipmi_ssif work.

Looking for clues how to solve this (because you mentioned this might be
the wrong solution) I've stumbled over this:

Documentation/i2c/writing-clients:
  You simply have to define a detect callback which will attempt to
  identify supported devices (returning 0 for supported ones and -ENODEV
  for unsupported ones), a list of addresses to probe, and a device type
  (or class) so that only I2C buses which may have that type of device
  connected (and not otherwise enumerated) will be probed.  For example,
  a driver for a hardware monitoring chip for which auto-detection is
  needed would set its class to I2C_CLASS_HWMON, and only I2C adapters
  with a class including I2C_CLASS_HWMON would be probed by this driver.
  Note that the absence of matching classes does not prevent the use of
  a device of that type on the given I2C adapter.  All it prevents is
  auto-detection; explicit instantiation of devices is still possible.

We do want auto-detection, so either this text is wrong or setting
I2C_CLASS_HWMON in our i2c adapter is correct. Am I missing
something?

thanks,
Jan

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

[Wolfram Sang]

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

Hi Jan,

> We do want auto-detection, so either this text is wrong or setting
> I2C_CLASS_HWMON in our i2c adapter is correct. Am I missing
> something?

I was assuming the pci-driver could ACPI for device instantiation, and
the platdrv could use DT. Then, the adapter class usage raised my
eyebrow.

I just saw that the pci-driver does not have ACPI support, yet. So, the
class could be used. Not sure about the platdrv case? But even then, the
adapter will probe EVERY hwmon device as soon as its i2c client driver
loads. Do you really want that? This may cost boot time, has unneeded
traffic on the bus, etc.

Regards,

   Wolfram


[-- 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 Thu, Apr 20, 2017 at 05:55:29PM +0200, Wolfram Sang wrote:
> Hi Jan,
> 
> > We do want auto-detection, so either this text is wrong or setting
> > I2C_CLASS_HWMON in our i2c adapter is correct. Am I missing
> > something?
> 
> I was assuming the pci-driver could ACPI for device instantiation, and
> the platdrv could use DT. Then, the adapter class usage raised my
> eyebrow.
> 
> I just saw that the pci-driver does not have ACPI support, yet. So, the
> class could be used. Not sure about the platdrv case? But even then, the
> adapter will probe EVERY hwmon device as soon as its i2c client driver
> loads. Do you really want that? This may cost boot time, has unneeded
> traffic on the bus, etc.

What is missing in the pci-driver for ACPI support? We already use ACPI
to detect the sclk setting.

The automatic probing is requested by distributions. As we have to support
both ACPI and DT there I thought of letting ipmi-ssif use the smbios/dmi
information that is already there and usable regardless of ACPI / DT.

For servers I don't think the probing overhead is an issue, the firmware
only exposes the BMC device there. On the "embedded" systems I've not
seen a BMC yet and anyone attempting do minimize boot time can disable
the automatic probing.

So, if you're ok with this I'll re-phrase the commit message and
re-submit the patch.

Gruß,
Jan

> Regards,
> 
>    Wolfram
> 

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

[Wolfram Sang]

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

Hi Jan,

> What is missing in the pci-driver for ACPI support? We already use ACPI
> to detect the sclk setting.

I noticed this in the driver:

134 static int thunder_i2c_smbus_setup(struct octeon_i2c *i2c,
135                                    struct device_node *node)
136 {
137         /* TODO: ACPI support */
138         if (!acpi_disabled)
139                 return -EOPNOTSUPP;
140 
141         return thunder_i2c_smbus_setup_of(i2c, node);
142 }

And from a glimpse, I assumed this is the place to detect and
instantiate client devices.

> So, if you're ok with this I'll re-phrase the commit message and
> re-submit the patch.

I'm OK. It is basically your call. I just wanted to make sure you know
what it means adding the class to a master driver.

Regards,

   Wolfram


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

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

[Jan Glauber]

On Fri, Apr 21, 2017 at 08:29:40AM +0200, Wolfram Sang wrote:
> Hi Jan,
> 
> > What is missing in the pci-driver for ACPI support? We already use ACPI
> > to detect the sclk setting.
> 
> I noticed this in the driver:
> 
> 134 static int thunder_i2c_smbus_setup(struct octeon_i2c *i2c,
> 135                                    struct device_node *node)
> 136 {
> 137         /* TODO: ACPI support */
> 138         if (!acpi_disabled)
> 139                 return -EOPNOTSUPP;
> 140 
> 141         return thunder_i2c_smbus_setup_of(i2c, node);
> 142 }
> 
> And from a glimpse, I assumed this is the place to detect and
> instantiate client devices.

I see. This function is only setting up the optional smbus alert irq
stuff.

> > So, if you're ok with this I'll re-phrase the commit message and
> > re-submit the patch.
> 
> I'm OK. It is basically your call. I just wanted to make sure you know
> what it means adding the class to a master driver.

Understood. I think we should look at fully describing the BMC in ACPI
and DT, if that is possible we could get rid of the additional probing.

As we are not there yet I'll post the patch adding the class.

thanks,
Jan

> Regards,
> 
>    Wolfram
>