[PATCH 0/3] Improvements to the i2c-piix4 driver

[PATCH 0/3] Improvements to the i2c-piix4 driver

[Jean Delvare]

Hi all,

Here are 3 proposed improvements to the i2c-piix4 driver, on top of the
3 patches I sent recently. Christian, I would appreciate if you can
test them and confirm that they do not cause any regression for you.

[PATCH 1/3] i2c-piix4: Support alternative port selection register
[PATCH 2/3] i2c-piix4: Always use the same type for port
[PATCH 3/3] i2c-piix4: Pre-shift the port number

-- 
Jean Delvare
SUSE L3 Support

[PATCH 1/3] i2c-piix4: Support alternative port selection register

[Jean Delvare]

The SB800 register reference guide says that the SMBus port selection
bits may not always be in register Smbus0En (0x2c) but could
alternatively be found in register Smbus0Sel (0x2e) depending on the
settings in register Smbus0SelEn (0x2f.) Add support for this
configuration.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Christian Fetzer <fetzer.ch@gmail.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-piix4.c |   30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

--- linux-4.5-rc0.orig/drivers/i2c/busses/i2c-piix4.c	2016-01-26 18:25:38.487310421 +0100
+++ linux-4.5-rc0/drivers/i2c/busses/i2c-piix4.c	2016-01-29 07:54:35.135765313 +0100
@@ -85,8 +85,13 @@
 /* SB800 constants */
 #define SB800_PIIX4_SMB_IDX		0xcd6
 
-/* SB800 port is selected by bits 2:1 of the smb_en register (0x2c) */
+/*
+ * SB800 port is selected by bits 2:1 of the smb_en register (0x2c)
+ * or the smb_sel register (0x2e), depending on bit 0 of register 0x2f.
+ */
 #define SB800_PIIX4_PORT_IDX		0x2c
+#define SB800_PIIX4_PORT_IDX_ALT	0x2e
+#define SB800_PIIX4_PORT_IDX_SEL	0x2f
 #define SB800_PIIX4_PORT_IDX_MASK	0x06
 
 /* insmod parameters */
@@ -136,8 +141,13 @@ static const struct dmi_system_id piix4_
 	{ },
 };
 
-/* SB800 globals */
+/*
+ * SB800 globals
+ * piix4_mutex_sb800 protects piix4_port_sel_sb800 and the pair
+ * of I/O ports at SB800_PIIX4_SMB_IDX.
+ */
 static DEFINE_MUTEX(piix4_mutex_sb800);
+static u8 piix4_port_sel_sb800;
 static const char *piix4_main_port_names_sb800[PIIX4_MAX_ADAPTERS] = {
 	" port 0", " port 2", " port 3", " port 4"
 };
@@ -254,7 +264,7 @@ static int piix4_setup_sb800(struct pci_
 			     const struct pci_device_id *id, u8 aux)
 {
 	unsigned short piix4_smba;
-	u8 smba_en_lo, smba_en_hi, smb_en, smb_en_status;
+	u8 smba_en_lo, smba_en_hi, smb_en, smb_en_status, port_sel;
 	u8 i2ccfg, i2ccfg_offset = 0x10;
 
 	/* SB800 and later SMBus does not support forcing address */
@@ -334,6 +344,18 @@ static int piix4_setup_sb800(struct pci_
 		 "SMBus Host Controller at 0x%x, revision %d\n",
 		 piix4_smba, i2ccfg >> 4);
 
+	/* Find which register is used for port selection */
+	mutex_lock(&piix4_mutex_sb800);
+	outb_p(SB800_PIIX4_PORT_IDX_SEL, SB800_PIIX4_SMB_IDX);
+	port_sel = inb_p(SB800_PIIX4_SMB_IDX + 1);
+	piix4_port_sel_sb800 = (port_sel & 0x01) ? SB800_PIIX4_PORT_IDX_ALT
+						 : SB800_PIIX4_PORT_IDX;
+	mutex_unlock(&piix4_mutex_sb800);
+
+	dev_info(&PIIX4_dev->dev,
+		 "Using register 0x%02x for SMBus port selection\n",
+		 (unsigned int)piix4_port_sel_sb800);
+
 	return piix4_smba;
 }
 
@@ -563,7 +585,7 @@ static s32 piix4_access_sb800(struct i2c
 
 	mutex_lock(&piix4_mutex_sb800);
 
-	outb_p(SB800_PIIX4_PORT_IDX, SB800_PIIX4_SMB_IDX);
+	outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX);
 	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
 
 	port = adapdata->port;

-- 
Jean Delvare
SUSE L3 Support

[PATCH 2/3] i2c-piix4: Always use the same type for port

[Jean Delvare]

Sometimes u8 is used to store the port number, sometimes unsigned
short is used. Consistently stick to a single type, for consistency
and to avoid implicit casts.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Christian Fetzer <fetzer.ch@gmail.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-piix4.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- linux-4.5-rc0.orig/drivers/i2c/busses/i2c-piix4.c	2016-01-29 07:54:35.135765313 +0100
+++ linux-4.5-rc0/drivers/i2c/busses/i2c-piix4.c	2016-01-29 07:57:13.706365999 +0100
@@ -158,7 +158,7 @@ struct i2c_piix4_adapdata {
 
 	/* SB800 */
 	bool sb800_main;
-	unsigned short port;
+	u8 port;
 };
 
 static int piix4_setup(struct pci_dev *PIIX4_dev,
@@ -649,7 +649,7 @@ static struct i2c_adapter *piix4_main_ad
 static struct i2c_adapter *piix4_aux_adapter;
 
 static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
-			     bool sb800_main, unsigned short port,
+			     bool sb800_main, u8 port,
 			     const char *name, struct i2c_adapter **padap)
 {
 	struct i2c_adapter *adap;

-- 
Jean Delvare
SUSE L3 Support

[PATCH 3/3] i2c-piix4: Pre-shift the port number

[Jean Delvare]

Shift the port number at initialization time, so that it is ready to
use at run time. That way we don't have to do it again for every SMBus
transaction.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Christian Fetzer <fetzer.ch@gmail.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-piix4.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

--- linux-4.5-rc0.orig/drivers/i2c/busses/i2c-piix4.c	2016-01-29 07:57:13.706365999 +0100
+++ linux-4.5-rc0/drivers/i2c/busses/i2c-piix4.c	2016-01-29 10:38:34.720453729 +0100
@@ -158,7 +158,7 @@ struct i2c_piix4_adapdata {
 
 	/* SB800 */
 	bool sb800_main;
-	u8 port;
+	u8 port;		/* Port number, shifted */
 };
 
 static int piix4_setup(struct pci_dev *PIIX4_dev,
@@ -589,8 +589,8 @@ static s32 piix4_access_sb800(struct i2c
 	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
 
 	port = adapdata->port;
-	if ((smba_en_lo & SB800_PIIX4_PORT_IDX_MASK) != (port << 1))
-		outb_p((smba_en_lo & ~SB800_PIIX4_PORT_IDX_MASK) | (port << 1),
+	if ((smba_en_lo & SB800_PIIX4_PORT_IDX_MASK) != port)
+		outb_p((smba_en_lo & ~SB800_PIIX4_PORT_IDX_MASK) | port,
 		       SB800_PIIX4_SMB_IDX + 1);
 
 	retval = piix4_access(adap, addr, flags, read_write,
@@ -676,7 +676,7 @@ static int piix4_add_adapter(struct pci_
 
 	adapdata->smba = smba;
 	adapdata->sb800_main = sb800_main;
-	adapdata->port = port;
+	adapdata->port = port << 1;
 
 	/* set up the sysfs linkage to our parent device */
 	adap->dev.parent = &dev->dev;
@@ -812,7 +812,7 @@ static void piix4_adap_remove(struct i2c
 
 	if (adapdata->smba) {
 		i2c_del_adapter(adap);
-		if (adapdata->port == 0) {
+		if (adapdata->port == (0 << 1)) {
 			release_region(adapdata->smba, SMBIOSIZE);
 			if (adapdata->sb800_main)
 				release_region(SB800_PIIX4_SMB_IDX, 2);

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/3] i2c-piix4: Support alternative port selection register

[Mika Westerberg]

On Fri, Jan 29, 2016 at 10:44:52AM +0100, Jean Delvare wrote:
> The SB800 register reference guide says that the SMBus port selection
> bits may not always be in register Smbus0En (0x2c) but could
> alternatively be found in register Smbus0Sel (0x2e) depending on the
> settings in register Smbus0SelEn (0x2f.) Add support for this
> configuration.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

One small comment, see below.

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

> Cc: Christian Fetzer <fetzer.ch@gmail.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> ---
>  drivers/i2c/busses/i2c-piix4.c |   30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> --- linux-4.5-rc0.orig/drivers/i2c/busses/i2c-piix4.c	2016-01-26 18:25:38.487310421 +0100
> +++ linux-4.5-rc0/drivers/i2c/busses/i2c-piix4.c	2016-01-29 07:54:35.135765313 +0100
> @@ -85,8 +85,13 @@
>  /* SB800 constants */
>  #define SB800_PIIX4_SMB_IDX		0xcd6
>  
> -/* SB800 port is selected by bits 2:1 of the smb_en register (0x2c) */
> +/*
> + * SB800 port is selected by bits 2:1 of the smb_en register (0x2c)
> + * or the smb_sel register (0x2e), depending on bit 0 of register 0x2f.
> + */
>  #define SB800_PIIX4_PORT_IDX		0x2c
> +#define SB800_PIIX4_PORT_IDX_ALT	0x2e
> +#define SB800_PIIX4_PORT_IDX_SEL	0x2f
>  #define SB800_PIIX4_PORT_IDX_MASK	0x06
>  
>  /* insmod parameters */
> @@ -136,8 +141,13 @@ static const struct dmi_system_id piix4_
>  	{ },
>  };
>  
> -/* SB800 globals */
> +/*
> + * SB800 globals
> + * piix4_mutex_sb800 protects piix4_port_sel_sb800 and the pair
> + * of I/O ports at SB800_PIIX4_SMB_IDX.
> + */
>  static DEFINE_MUTEX(piix4_mutex_sb800);
> +static u8 piix4_port_sel_sb800;
>  static const char *piix4_main_port_names_sb800[PIIX4_MAX_ADAPTERS] = {
>  	" port 0", " port 2", " port 3", " port 4"
>  };
> @@ -254,7 +264,7 @@ static int piix4_setup_sb800(struct pci_
>  			     const struct pci_device_id *id, u8 aux)
>  {
>  	unsigned short piix4_smba;
> -	u8 smba_en_lo, smba_en_hi, smb_en, smb_en_status;
> +	u8 smba_en_lo, smba_en_hi, smb_en, smb_en_status, port_sel;
>  	u8 i2ccfg, i2ccfg_offset = 0x10;
>  
>  	/* SB800 and later SMBus does not support forcing address */
> @@ -334,6 +344,18 @@ static int piix4_setup_sb800(struct pci_
>  		 "SMBus Host Controller at 0x%x, revision %d\n",
>  		 piix4_smba, i2ccfg >> 4);
>  
> +	/* Find which register is used for port selection */
> +	mutex_lock(&piix4_mutex_sb800);
> +	outb_p(SB800_PIIX4_PORT_IDX_SEL, SB800_PIIX4_SMB_IDX);
> +	port_sel = inb_p(SB800_PIIX4_SMB_IDX + 1);
> +	piix4_port_sel_sb800 = (port_sel & 0x01) ? SB800_PIIX4_PORT_IDX_ALT
> +						 : SB800_PIIX4_PORT_IDX;
> +	mutex_unlock(&piix4_mutex_sb800);
> +
> +	dev_info(&PIIX4_dev->dev,
> +		 "Using register 0x%02x for SMBus port selection\n",
> +		 (unsigned int)piix4_port_sel_sb800);

Would dev_dbg() be better here? Not sure how useful this information is
for normal users.

> +
>  	return piix4_smba;
>  }
>  
> @@ -563,7 +585,7 @@ static s32 piix4_access_sb800(struct i2c
>  
>  	mutex_lock(&piix4_mutex_sb800);
>  
> -	outb_p(SB800_PIIX4_PORT_IDX, SB800_PIIX4_SMB_IDX);
> +	outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX);
>  	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
>  
>  	port = adapdata->port;
> 
> -- 
> Jean Delvare
> SUSE L3 Support

Re: [PATCH 2/3] i2c-piix4: Always use the same type for port

[Mika Westerberg]

On Fri, Jan 29, 2016 at 10:45:30AM +0100, Jean Delvare wrote:
> Sometimes u8 is used to store the port number, sometimes unsigned
> short is used. Consistently stick to a single type, for consistency
> and to avoid implicit casts.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 3/3] i2c-piix4: Pre-shift the port number

[Mika Westerberg]

On Fri, Jan 29, 2016 at 10:46:37AM +0100, Jean Delvare wrote:
> Shift the port number at initialization time, so that it is ready to
> use at run time. That way we don't have to do it again for every SMBus
> transaction.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Christian Fetzer <fetzer.ch@gmail.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> ---
>  drivers/i2c/busses/i2c-piix4.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> --- linux-4.5-rc0.orig/drivers/i2c/busses/i2c-piix4.c	2016-01-29 07:57:13.706365999 +0100
> +++ linux-4.5-rc0/drivers/i2c/busses/i2c-piix4.c	2016-01-29 10:38:34.720453729 +0100
> @@ -158,7 +158,7 @@ struct i2c_piix4_adapdata {
>  
>  	/* SB800 */
>  	bool sb800_main;
> -	u8 port;
> +	u8 port;		/* Port number, shifted */
>  };
>  
>  static int piix4_setup(struct pci_dev *PIIX4_dev,
> @@ -589,8 +589,8 @@ static s32 piix4_access_sb800(struct i2c
>  	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
>  
>  	port = adapdata->port;
> -	if ((smba_en_lo & SB800_PIIX4_PORT_IDX_MASK) != (port << 1))
> -		outb_p((smba_en_lo & ~SB800_PIIX4_PORT_IDX_MASK) | (port << 1),
> +	if ((smba_en_lo & SB800_PIIX4_PORT_IDX_MASK) != port)
> +		outb_p((smba_en_lo & ~SB800_PIIX4_PORT_IDX_MASK) | port,
>  		       SB800_PIIX4_SMB_IDX + 1);
>  
>  	retval = piix4_access(adap, addr, flags, read_write,
> @@ -676,7 +676,7 @@ static int piix4_add_adapter(struct pci_
>  
>  	adapdata->smba = smba;
>  	adapdata->sb800_main = sb800_main;
> -	adapdata->port = port;
> +	adapdata->port = port << 1;
>  
>  	/* set up the sysfs linkage to our parent device */
>  	adap->dev.parent = &dev->dev;
> @@ -812,7 +812,7 @@ static void piix4_adap_remove(struct i2c
>  
>  	if (adapdata->smba) {
>  		i2c_del_adapter(adap);
> -		if (adapdata->port == 0) {
> +		if (adapdata->port == (0 << 1)) {

I suppose this is only for documentation purposes, right?

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

>  			release_region(adapdata->smba, SMBIOSIZE);
>  			if (adapdata->sb800_main)
>  				release_region(SB800_PIIX4_SMB_IDX, 2);
> 
> -- 
> Jean Delvare
> SUSE L3 Support

Re: [PATCH 3/3] i2c-piix4: Pre-shift the port number

[Jean Delvare]

Le Friday 29 January 2016 à 13:02 +0200, Mika Westerberg a écrit :
> On Fri, Jan 29, 2016 at 10:46:37AM +0100, Jean Delvare wrote:
> > Shift the port number at initialization time, so that it is ready to
> > use at run time. That way we don't have to do it again for every SMBus
> > transaction.
> > 
> > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Christian Fetzer <fetzer.ch@gmail.com>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> > ---
> >  drivers/i2c/busses/i2c-piix4.c |   10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > --- linux-4.5-rc0.orig/drivers/i2c/busses/i2c-piix4.c	2016-01-29 07:57:13.706365999 +0100
> > +++ linux-4.5-rc0/drivers/i2c/busses/i2c-piix4.c	2016-01-29 10:38:34.720453729 +0100
> > @@ -158,7 +158,7 @@ struct i2c_piix4_adapdata {
> >  
> >  	/* SB800 */
> >  	bool sb800_main;
> > -	u8 port;
> > +	u8 port;		/* Port number, shifted */
> >  };
> >  
> >  static int piix4_setup(struct pci_dev *PIIX4_dev,
> > @@ -589,8 +589,8 @@ static s32 piix4_access_sb800(struct i2c
> >  	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
> >  
> >  	port = adapdata->port;
> > -	if ((smba_en_lo & SB800_PIIX4_PORT_IDX_MASK) != (port << 1))
> > -		outb_p((smba_en_lo & ~SB800_PIIX4_PORT_IDX_MASK) | (port << 1),
> > +	if ((smba_en_lo & SB800_PIIX4_PORT_IDX_MASK) != port)
> > +		outb_p((smba_en_lo & ~SB800_PIIX4_PORT_IDX_MASK) | port,
> >  		       SB800_PIIX4_SMB_IDX + 1);
> >  
> >  	retval = piix4_access(adap, addr, flags, read_write,
> > @@ -676,7 +676,7 @@ static int piix4_add_adapter(struct pci_
> >  
> >  	adapdata->smba = smba;
> >  	adapdata->sb800_main = sb800_main;
> > -	adapdata->port = port;
> > +	adapdata->port = port << 1;
> >  
> >  	/* set up the sysfs linkage to our parent device */
> >  	adap->dev.parent = &dev->dev;
> > @@ -812,7 +812,7 @@ static void piix4_adap_remove(struct i2c
> >  
> >  	if (adapdata->smba) {
> >  		i2c_del_adapter(adap);
> > -		if (adapdata->port == 0) {
> > +		if (adapdata->port == (0 << 1)) {
> 
> I suppose this is only for documentation purposes, right?

Yes. This change wasn't originally part of the patch, as I know it is a
no-op, but I thought it was more correct to include it.

> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> >  			release_region(adapdata->smba, SMBIOSIZE);
> >  			if (adapdata->sb800_main)
> >  				release_region(SB800_PIIX4_SMB_IDX, 2);
> > 

Thanks for the reviews!

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/3] i2c-piix4: Support alternative port selection register

[Jean Delvare]

On Fri, 29 Jan 2016 12:59:23 +0200, Mika Westerberg wrote:
> > @@ -334,6 +344,18 @@ static int piix4_setup_sb800(struct pci_
> >  		 "SMBus Host Controller at 0x%x, revision %d\n",
> >  		 piix4_smba, i2ccfg >> 4);
> >  
> > +	/* Find which register is used for port selection */
> > +	mutex_lock(&piix4_mutex_sb800);
> > +	outb_p(SB800_PIIX4_PORT_IDX_SEL, SB800_PIIX4_SMB_IDX);
> > +	port_sel = inb_p(SB800_PIIX4_SMB_IDX + 1);
> > +	piix4_port_sel_sb800 = (port_sel & 0x01) ? SB800_PIIX4_PORT_IDX_ALT
> > +						 : SB800_PIIX4_PORT_IDX;
> > +	mutex_unlock(&piix4_mutex_sb800);
> > +
> > +	dev_info(&PIIX4_dev->dev,
> > +		 "Using register 0x%02x for SMBus port selection\n",
> > +		 (unsigned int)piix4_port_sel_sb800);
> 
> Would dev_dbg() be better here? Not sure how useful this information is
> for normal users.

Not sure. I agree that the register address isn't that helpful, but the
fact that port selection is needed is good to know. Sure I could use
dev_info() to mention that multiplexing is taking place, and then
dev_dbg() to give the port address, but that seems needlessly complex.
And it's not like the kernel log isn't full of I/O addresses already...

So I'd leave it as is, unless Wolfram objects.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/3] i2c-piix4: Support alternative port selection register

[Wolfram Sang]

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

On Fri, Jan 29, 2016 at 10:44:52AM +0100, Jean Delvare wrote:
> The SB800 register reference guide says that the SMBus port selection
> bits may not always be in register Smbus0En (0x2c) but could
> alternatively be found in register Smbus0Sel (0x2e) depending on the
> settings in register Smbus0SelEn (0x2f.) Add support for this
> configuration.

Were you able to test both cases?


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

Re: [PATCH 1/3] i2c-piix4: Support alternative port selection register

[Jean Delvare]

Hi Wolfram,

On Fri, 12 Feb 2016 20:27:20 +0100, Wolfram Sang wrote:
> On Fri, Jan 29, 2016 at 10:44:52AM +0100, Jean Delvare wrote:
> > The SB800 register reference guide says that the SMBus port selection
> > bits may not always be in register Smbus0En (0x2c) but could
> > alternatively be found in register Smbus0Sel (0x2e) depending on the
> > settings in register Smbus0SelEn (0x2f.) Add support for this
> > configuration.
> 
> Were you able to test both cases?

I don't have the hardware myself so I was not able to test any case.
I was hoping Christian would test. This is the reason why I am logging
which register is used, so that we know if the alternative setting is
ever used. I found the potential problem by looking at the datasheet,
it's not something that has been reported (yet.)

Meanwhile I have found a datasheet for device 780Bh (named Bolton FCH
on AMD's web site, but Hudson2 in our driver) which suggest that the
"alternative" setting is the only possible one on this chipset. The
register used to figure out the setting is marked as reserved. If the
register exists still and the relevant bit is set, then my patch should
work. If not then a better patch will be needed.

I'll try to gain access to a system with a Bolton FCH and experiment
with it.

Then there's the most recent device, codenamed "CZ", for which I have
no information at all.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/3] i2c-piix4: Support alternative port selection register

[fetzerch]

Hi Jean,

On 13.02.2016 22:51, Jean Delvare wrote:
> Hi Wolfram,
> 
> On Fri, 12 Feb 2016 20:27:20 +0100, Wolfram Sang wrote:
>> On Fri, Jan 29, 2016 at 10:44:52AM +0100, Jean Delvare wrote:
>>> The SB800 register reference guide says that the SMBus port selection
>>> bits may not always be in register Smbus0En (0x2c) but could
>>> alternatively be found in register Smbus0Sel (0x2e) depending on the
>>> settings in register Smbus0SelEn (0x2f.) Add support for this
>>> configuration.
>>
>> Were you able to test both cases?
> 
> I don't have the hardware myself so I was not able to test any case.
> I was hoping Christian would test. This is the reason why I am logging
> which register is used, so that we know if the alternative setting is
> ever used. I found the potential problem by looking at the datasheet,
> it's not something that has been reported (yet.)
> 
> Meanwhile I have found a datasheet for device 780Bh (named Bolton FCH
> on AMD's web site, but Hudson2 in our driver) which suggest that the
> "alternative" setting is the only possible one on this chipset. The
> register used to figure out the setting is marked as reserved. If the
> register exists still and the relevant bit is set, then my patch should
> work. If not then a better patch will be needed.
> 
> I'll try to gain access to a system with a Bolton FCH and experiment
> with it.
> 
> Then there's the most recent device, codenamed "CZ", for which I have
> no information at all.
> 

sorry for the late response. This time it slipped through on my side.

The sensors command still works fine without any noticable change.
Here is the output:

[1894808.057493] piix4_smbus 0000:00:14.0: SMBus Host Controller at
0xb00, revision 0
[1894808.057504] piix4_smbus 0000:00:14.0: Using register 0x2c for SMBus
port selection
[1894808.394960] i2c i2c-1: Found w83795adg rev. B at 0x2f
[1894808.666968] piix4_smbus 0000:00:14.0: Auxiliary SMBus Host
Controller at 0xb20

Thanks,
Christian

Re: [PATCH 1/3] i2c-piix4: Support alternative port selection register

[Jean Delvare]

On Sun, 14 Feb 2016 09:39:39 +0100, fetzerch wrote:
> Hi Jean,
> 
> On 13.02.2016 22:51, Jean Delvare wrote:
> > Hi Wolfram,
> > 
> > On Fri, 12 Feb 2016 20:27:20 +0100, Wolfram Sang wrote:
> >> On Fri, Jan 29, 2016 at 10:44:52AM +0100, Jean Delvare wrote:
> >>> The SB800 register reference guide says that the SMBus port selection
> >>> bits may not always be in register Smbus0En (0x2c) but could
> >>> alternatively be found in register Smbus0Sel (0x2e) depending on the
> >>> settings in register Smbus0SelEn (0x2f.) Add support for this
> >>> configuration.
> >>
> >> Were you able to test both cases?
> > 
> > I don't have the hardware myself so I was not able to test any case.
> > I was hoping Christian would test. This is the reason why I am logging
> > which register is used, so that we know if the alternative setting is
> > ever used. I found the potential problem by looking at the datasheet,
> > it's not something that has been reported (yet.)
> > 
> > Meanwhile I have found a datasheet for device 780Bh (named Bolton FCH
> > on AMD's web site, but Hudson2 in our driver) which suggest that the
> > "alternative" setting is the only possible one on this chipset. The
> > register used to figure out the setting is marked as reserved. If the
> > register exists still and the relevant bit is set, then my patch should
> > work. If not then a better patch will be needed.
> > 
> > I'll try to gain access to a system with a Bolton FCH and experiment
> > with it.
> > 
> > Then there's the most recent device, codenamed "CZ", for which I have
> > no information at all.
> > 
> 
> sorry for the late response. This time it slipped through on my side.
> 
> The sensors command still works fine without any noticable change.
> Here is the output:
> 
> [1894808.057493] piix4_smbus 0000:00:14.0: SMBus Host Controller at
> 0xb00, revision 0
> [1894808.057504] piix4_smbus 0000:00:14.0: Using register 0x2c for SMBus
> port selection
> [1894808.394960] i2c i2c-1: Found w83795adg rev. B at 0x2f
> [1894808.666968] piix4_smbus 0000:00:14.0: Auxiliary SMBus Host
> Controller at 0xb20

Thanks for testing, Christian, this is very helpful.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/3] i2c-piix4: Support alternative port selection register

[Jean Delvare]

On Sat, 13 Feb 2016 22:51:47 +0100, Jean Delvare wrote:
> Hi Wolfram,
> 
> On Fri, 12 Feb 2016 20:27:20 +0100, Wolfram Sang wrote:
> > On Fri, Jan 29, 2016 at 10:44:52AM +0100, Jean Delvare wrote:
> > > The SB800 register reference guide says that the SMBus port selection
> > > bits may not always be in register Smbus0En (0x2c) but could
> > > alternatively be found in register Smbus0Sel (0x2e) depending on the
> > > settings in register Smbus0SelEn (0x2f.) Add support for this
> > > configuration.
> > 
> > Were you able to test both cases?
> 
> I don't have the hardware myself so I was not able to test any case.
> I was hoping Christian would test. This is the reason why I am logging
> which register is used, so that we know if the alternative setting is
> ever used. I found the potential problem by looking at the datasheet,
> it's not something that has been reported (yet.)
> 
> Meanwhile I have found a datasheet for device 780Bh (named Bolton FCH
> on AMD's web site, but Hudson2 in our driver) which suggest that the
> "alternative" setting is the only possible one on this chipset. The
> register used to figure out the setting is marked as reserved. If the
> register exists still and the relevant bit is set, then my patch should
> work. If not then a better patch will be needed.
> 
> I'll try to gain access to a system with a Bolton FCH and experiment
> with it.

I have access to a Bolton FCH system for one week. I tested the port
selection and as the datasheet suggested, the "alternative" register is
always used on this chipset. Register Smbus0SelEn (0x2f) doesn't exist
so the code I submitted doesn't work for this chipset. After
unconditionally using 0x2e for port selection, the driver seems to work
fine, except that sensors-detect just hung the machine when probing
SMBus port 2 address 0x2f (and it's 1200 km away from me and I can't
reboot it, yay.) That could be a driver issue or (more likely) just
another case of I2C/SMBus device that hates being probed and hangs the
system for reprisal.

> Then there's the most recent device, codenamed "CZ", for which I have
> no information at all.

Still looking into that as well, trying to get my hands on both the
documentation and the hardware. I would bet that it works the same as
the Hudson2/Bolton, but if I can verify that it would be safer.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/3] i2c-piix4: Support alternative port selection register

[Wolfram Sang]

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


> so the code I submitted doesn't work for this chipset. After

So I wait for V2 in any case?


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

Re: [PATCH 1/3] i2c-piix4: Support alternative port selection register

[Jean Delvare]

On Mon, 15 Feb 2016 18:37:52 +0100, Wolfram Sang wrote:
> 
> > so the code I submitted doesn't work for this chipset. After
> 
> So I wait for V2 in any case?

Yes please.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/3] i2c-piix4: Always use the same type for port

[Wolfram Sang]

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

On Fri, Jan 29, 2016 at 10:45:30AM +0100, Jean Delvare wrote:
> Sometimes u8 is used to store the port number, sometimes unsigned
> short is used. Consistently stick to a single type, for consistency
> and to avoid implicit casts.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Christian Fetzer <fetzer.ch@gmail.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>

Applied to for-next, thanks!


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

Re: [PATCH 3/3] i2c-piix4: Pre-shift the port number

[Wolfram Sang]

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

On Fri, Jan 29, 2016 at 10:46:37AM +0100, Jean Delvare wrote:
> Shift the port number at initialization time, so that it is ready to
> use at run time. That way we don't have to do it again for every SMBus
> transaction.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Christian Fetzer <fetzer.ch@gmail.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>

Applied to for-next, thanks!


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