[PATCH v3 0/5] Support multiplexed main SMBus interface on SB800

[PATCH v3 0/5] Support multiplexed main SMBus interface on SB800

[Christian Fetzer]

This is an attempt to upstream the patches created by Thomas Brandon and
Eddi De Pieri to support the multiplexed main SMBus interface on the SB800
chipset. (https://www.mail-archive.com/linux-i2c@vger.kernel.org/msg06757.html)

I have mainly rebased the latest patch version and tested the driver on a
HP ProLiant MicroServer G7 N54L (where this patch allows to access sensor data
from a w83795adg).

The patched driver is running stable on the machine, given that ic2_piix4 is
loaded before jc42 and w83795. If jc42 is loaded before i2c_piix4 calling
sensors triggers some errors:
    ERROR: Can't get value of subfeature temp1_min_alarm: Can't read

While the kernel log shows:
    i2c i2c-1: Transaction (pre): CNT=0c, CMD=05, ADD=31, DAT0=03, DAT1=c0
    i2c i2c-1: Error: no response!
    i2c i2c-1: Transaction (post): CNT=0c, CMD=05, ADD=31, DAT0=ff, DAT1=ff
Unfortunately I don't know how to tackle this specific issue.

Please review and let me know required changes in order to get this upstream
finally.

Eddi, Thomas, it would be great if you could verify the changes on your
machines.

Regards,
Christian

v3:
- Incorporated changes requested by Mika and Andy
    - main adapter name set to 'main'
    - defined constant idx address
    - block comment style, joined string literals, reworked for loops
      into while loops

v2:
- Incorporated changes requested by Mika
    - remove adapter in reverse order
    - ERROR label
    - request base address index region only once

Christian Fetzer (5):
  i2c-piix4: Optionally release smba in piix4_adap_remove
  i2c-piix4: Convert piix4_main_adapter to array
  i2c-piix4: Request base address index region once for SB800
  i2c-piix4: Add support for multiplexed main adapter in SB800
  i2c-piix4: Add adapter port name support for SB800 chipset

 drivers/i2c/busses/i2c-piix4.c | 177 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 149 insertions(+), 28 deletions(-)

-- 
1.9.1

[PATCH v3 1/5] i2c-piix4: Optionally release smba in piix4_adap_remove

[Christian Fetzer]

This is in preparation to support the multiplexed SMBus main controller
in the SB800 chipset where the controller address is shared among
the four multiplexed ports. As such the address region should be
only freed for the first multiplexed adapter to avoid double free
warnings.

Signed-off-by: Christian Fetzer <fetzer.ch@gmail.com>
---
 drivers/i2c/busses/i2c-piix4.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 630bce6..e78b982 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -660,13 +660,14 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	return 0;
 }
 
-static void piix4_adap_remove(struct i2c_adapter *adap)
+static void piix4_adap_remove(struct i2c_adapter *adap, bool free_smba)
 {
 	struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap);
 
 	if (adapdata->smba) {
 		i2c_del_adapter(adap);
-		release_region(adapdata->smba, SMBIOSIZE);
+		if (free_smba)
+			release_region(adapdata->smba, SMBIOSIZE);
 		kfree(adapdata);
 		kfree(adap);
 	}
@@ -675,12 +676,12 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
 static void piix4_remove(struct pci_dev *dev)
 {
 	if (piix4_main_adapter) {
-		piix4_adap_remove(piix4_main_adapter);
+		piix4_adap_remove(piix4_main_adapter, true);
 		piix4_main_adapter = NULL;
 	}
 
 	if (piix4_aux_adapter) {
-		piix4_adap_remove(piix4_aux_adapter);
+		piix4_adap_remove(piix4_aux_adapter, true);
 		piix4_aux_adapter = NULL;
 	}
 }
-- 
1.9.1

Re: [PATCH v3 1/5] i2c-piix4: Optionally release smba in piix4_adap_remove

[Mika Westerberg]

On Sat, Nov 07, 2015 at 12:35:22PM +0100, Christian Fetzer wrote:
> This is in preparation to support the multiplexed SMBus main controller
> in the SB800 chipset where the controller address is shared among
> the four multiplexed ports. As such the address region should be
> only freed for the first multiplexed adapter to avoid double free
> warnings.
> 
> Signed-off-by: Christian Fetzer <fetzer.ch@gmail.com>

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

[PATCH v3 2/5] i2c-piix4: Convert piix4_main_adapter to array

[Christian Fetzer]

The SB800 chipset supports a multiplexed main SMBus controller with
four ports. Therefore the static variable piix4_main_adapter is
converted into a piix4_main_adapters array that can hold one
i2c_adapter for each multiplexed port.

The auxiliary adapter remains unchanged since it represents the second
(not multiplexed) SMBus controller on the SB800 chipset.

Signed-off-by: Christian Fetzer <fetzer.ch@gmail.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/busses/i2c-piix4.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index e78b982..0e4ae60 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -75,6 +75,9 @@
 #define PIIX4_WORD_DATA		0x0C
 #define PIIX4_BLOCK_DATA	0x14
 
+/* Multi-port constants */
+#define PIIX4_MAX_ADAPTERS 4
+
 /* insmod parameters */
 
 /* If force is set to anything different from 0, we forcibly enable the
@@ -561,7 +564,7 @@ static const struct pci_device_id piix4_ids[] = {
 
 MODULE_DEVICE_TABLE (pci, piix4_ids);
 
-static struct i2c_adapter *piix4_main_adapter;
+static struct i2c_adapter *piix4_main_adapters[PIIX4_MAX_ADAPTERS];
 static struct i2c_adapter *piix4_aux_adapter;
 
 static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
@@ -629,7 +632,7 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		return retval;
 
 	/* Try to register main SMBus adapter, give up if we can't */
-	retval = piix4_add_adapter(dev, retval, &piix4_main_adapter);
+	retval = piix4_add_adapter(dev, retval, &piix4_main_adapters[0]);
 	if (retval < 0)
 		return retval;
 
@@ -675,9 +678,14 @@ static void piix4_adap_remove(struct i2c_adapter *adap, bool free_smba)
 
 static void piix4_remove(struct pci_dev *dev)
 {
-	if (piix4_main_adapter) {
-		piix4_adap_remove(piix4_main_adapter, true);
-		piix4_main_adapter = NULL;
+	int port = PIIX4_MAX_ADAPTERS;
+
+	while (--port >= 0) {
+		if (piix4_main_adapters[port]) {
+			piix4_adap_remove(piix4_main_adapters[port],
+					  port == 0);
+			piix4_main_adapters[port] = NULL;
+		}
 	}
 
 	if (piix4_aux_adapter) {
-- 
1.9.1

[PATCH v3 3/5] i2c-piix4: Request base address index region once for SB800

[Christian Fetzer]

Request the SMBus base address index region once in piix4_probe. This
is particularly useful when using the multiplexed adapter in SB800 as
it avoids requesting and releasing the region on every transfer.

Signed-off-by: Christian Fetzer <fetzer.ch@gmail.com>
---
 drivers/i2c/busses/i2c-piix4.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 0e4ae60..67ada1e 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -78,6 +78,9 @@
 /* Multi-port constants */
 #define PIIX4_MAX_ADAPTERS 4
 
+/* SB800 constants */
+#define SB800_PIIX4_SMB_IDX 0xCD6
+
 /* insmod parameters */
 
 /* If force is set to anything different from 0, we forcibly enable the
@@ -125,6 +128,9 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
 	{ },
 };
 
+/* SB800 globals */
+static bool piix4_smb_idx_sb800;
+
 struct i2c_piix4_adapdata {
 	unsigned short smba;
 };
@@ -232,7 +238,6 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 			     const struct pci_device_id *id, u8 aux)
 {
 	unsigned short piix4_smba;
-	unsigned short smba_idx = 0xcd6;
 	u8 smba_en_lo, smba_en_hi, smb_en, smb_en_status;
 	u8 i2ccfg, i2ccfg_offset = 0x10;
 
@@ -254,16 +259,10 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	else
 		smb_en = (aux) ? 0x28 : 0x2c;
 
-	if (!request_region(smba_idx, 2, "smba_idx")) {
-		dev_err(&PIIX4_dev->dev, "SMBus base address index region "
-			"0x%x already in use!\n", smba_idx);
-		return -EBUSY;
-	}
-	outb_p(smb_en, smba_idx);
-	smba_en_lo = inb_p(smba_idx + 1);
-	outb_p(smb_en + 1, smba_idx);
-	smba_en_hi = inb_p(smba_idx + 1);
-	release_region(smba_idx, 2);
+	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
+	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
+	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
+	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
 
 	if (!smb_en) {
 		smb_en_status = smba_en_lo & 0x10;
@@ -621,11 +620,20 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	if ((dev->vendor == PCI_VENDOR_ID_ATI &&
 	     dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
 	     dev->revision >= 0x40) ||
-	    dev->vendor == PCI_VENDOR_ID_AMD)
+	    dev->vendor == PCI_VENDOR_ID_AMD) {
+		if (!request_region(SB800_PIIX4_SMB_IDX, 2, "smba_idx")) {
+			dev_err(&dev->dev,
+			"SMBus base address index region 0x%x already in use!\n",
+			SB800_PIIX4_SMB_IDX);
+			return -EBUSY;
+		}
+		piix4_smb_idx_sb800 = true;
+
 		/* base address location etc changed in SB800 */
 		retval = piix4_setup_sb800(dev, id, 0);
-	else
+	} else {
 		retval = piix4_setup(dev, id);
+	}
 
 	/* If no main SMBus found, give up */
 	if (retval < 0)
@@ -692,6 +700,9 @@ static void piix4_remove(struct pci_dev *dev)
 		piix4_adap_remove(piix4_aux_adapter, true);
 		piix4_aux_adapter = NULL;
 	}
+
+	if (piix4_smb_idx_sb800)
+		release_region(SB800_PIIX4_SMB_IDX, 2);
 }
 
 static struct pci_driver piix4_driver = {
-- 
1.9.1

Re: [PATCH v3 3/5] i2c-piix4: Request base address index region once for SB800

[Andy Shevchenko]

On Sat, 2015-11-07 at 12:35 +0100, Christian Fetzer wrote:
> Request the SMBus base address index region once in piix4_probe. This
> is particularly useful when using the multiplexed adapter in SB800 as
> it avoids requesting and releasing the region on every transfer.
> 
> Signed-off-by: Christian Fetzer <fetzer.ch@gmail.com>
> ---
>  drivers/i2c/busses/i2c-piix4.c | 37 ++++++++++++++++++++++++------
> -------
>  1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-
> piix4.c
> index 0e4ae60..67ada1e 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -78,6 +78,9 @@
>  /* Multi-port constants */
>  #define PIIX4_MAX_ADAPTERS 4
>  
> +/* SB800 constants */
> +#define SB800_PIIX4_SMB_IDX 0xCD6

Small letters for value?

> +
>  /* insmod parameters */
>  
>  /* If force is set to anything different from 0, we forcibly enable
> the
> @@ -125,6 +128,9 @@ static const struct dmi_system_id piix4_dmi_ibm[]
> = {
>  	{ },
>  };
>  
> +/* SB800 globals */
> +static bool piix4_smb_idx_sb800;
> +

By the way, could it be part of driver data, i.e. member of struct
i2c_piix4_adapdata?

> 
>  struct i2c_piix4_adapdata {
>  	unsigned short smba;
>  };
> @@ -232,7 +238,6 @@ static int piix4_setup_sb800(struct pci_dev
> *PIIX4_dev,
>  			     const struct pci_device_id *id, u8 aux)
>  {
>  	unsigned short piix4_smba;
> -	unsigned short smba_idx = 0xcd6;
>  	u8 smba_en_lo, smba_en_hi, smb_en, smb_en_status;
>  	u8 i2ccfg, i2ccfg_offset = 0x10;
>  
> @@ -254,16 +259,10 @@ static int piix4_setup_sb800(struct pci_dev
> *PIIX4_dev,
>  	else
>  		smb_en = (aux) ? 0x28 : 0x2c;
>  
> -	if (!request_region(smba_idx, 2, "smba_idx")) {
> -		dev_err(&PIIX4_dev->dev, "SMBus base address index
> region "
> -			"0x%x already in use!\n", smba_idx);
> -		return -EBUSY;
> -	}
> -	outb_p(smb_en, smba_idx);
> -	smba_en_lo = inb_p(smba_idx + 1);
> -	outb_p(smb_en + 1, smba_idx);
> -	smba_en_hi = inb_p(smba_idx + 1);
> -	release_region(smba_idx, 2);
> +	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
> +	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
> +	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
> +	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
>  
>  	if (!smb_en) {
>  		smb_en_status = smba_en_lo & 0x10;
> @@ -621,11 +620,20 @@ static int piix4_probe(struct pci_dev *dev,
> const struct pci_device_id *id)
>  	if ((dev->vendor == PCI_VENDOR_ID_ATI &&
>  	     dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
>  	     dev->revision >= 0x40) ||
> -	    dev->vendor == PCI_VENDOR_ID_AMD)
> +	    dev->vendor == PCI_VENDOR_ID_AMD) {
> +		if (!request_region(SB800_PIIX4_SMB_IDX, 2,
> "smba_idx")) {
> +			dev_err(&dev->dev,
> +			"SMBus base address index region 0x%x
> already in use!\n",
> +			SB800_PIIX4_SMB_IDX);
> +			return -EBUSY;
> +		}
> +		piix4_smb_idx_sb800 = true;
> +
>  		/* base address location etc changed in SB800 */
>  		retval = piix4_setup_sb800(dev, id, 0);
> -	else
> +	} else {
>  		retval = piix4_setup(dev, id);
> +	}
>  
>  	/* If no main SMBus found, give up */
>  	if (retval < 0)
> @@ -692,6 +700,9 @@ static void piix4_remove(struct pci_dev *dev)
>  		piix4_adap_remove(piix4_aux_adapter, true);
>  		piix4_aux_adapter = NULL;
>  	}
> +
> +	if (piix4_smb_idx_sb800)
> +		release_region(SB800_PIIX4_SMB_IDX, 2);
>  }
>  
>  static struct pci_driver piix4_driver = {

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v3 4/5] i2c-piix4: Add support for multiplexed main adapter in SB800

[Christian Fetzer]

The SB800 chipset supports a multiplexed main SMBus controller with
four ports. The multiplexed ports share the same SMBus address and
register set. The port is selected by bits 2:1 of the smb_en register
(0x2C).

Only one port can be active at any point in time therefore a mutex is
needed in order to synchronize access.

Tested on HP ProLiant MicroServer G7 N54L (where this patch adds
support to access sensor data from the w83795adg).

Cc: Thomas Brandon <tbrandonau@gmail.com>
Cc: Eddi De Pieri <eddi@depieri.net>
Signed-off-by: Christian Fetzer <fetzer.ch@gmail.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/busses/i2c-piix4.c | 105 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 100 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 67ada1e..4beaa3d 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -23,6 +23,9 @@
 
    Note: we assume there can only be one device, with one or more
    SMBus interfaces.
+   The device can register multiple i2c_adapters (up to PIIX4_MAX_ADAPTERS).
+   For devices supporting multiple ports the i2c_adapter should provide
+   an i2c_algorithm to access them.
 */
 
 #include <linux/module.h>
@@ -37,6 +40,7 @@
 #include <linux/dmi.h>
 #include <linux/acpi.h>
 #include <linux/io.h>
+#include <linux/mutex.h>
 
 
 /* PIIX4 SMBus address offsets */
@@ -129,10 +133,12 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
 };
 
 /* SB800 globals */
+DEFINE_MUTEX(piix4_mutex_sb800);
 static bool piix4_smb_idx_sb800;
 
 struct i2c_piix4_adapdata {
 	unsigned short smba;
+	unsigned short port;
 };
 
 static int piix4_setup(struct pci_dev *PIIX4_dev,
@@ -312,6 +318,8 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	else
 		dev_dbg(&PIIX4_dev->dev, "Using SMI# for SMBus\n");
 
+	mutex_init(&piix4_mutex_sb800);
+
 	dev_info(&PIIX4_dev->dev,
 		 "SMBus Host Controller at 0x%x, revision %d\n",
 		 piix4_smba, i2ccfg >> 4);
@@ -526,6 +534,43 @@ static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
 	return 0;
 }
 
+/*
+ * Handles access to multiple SMBus ports on the SB800.
+ * The port is selected by bits 2:1 of the smb_en register (0x2C).
+ * Returns negative errno on error.
+ *
+ * Note: The selected port must be returned to the initial selection to avoid
+ * problems on certain systems.
+ */
+static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
+		 unsigned short flags, char read_write,
+		 u8 command, int size, union i2c_smbus_data *data)
+{
+	struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap);
+	u8 smba_en_lo, smb_en = 0x2c;
+	u8 port;
+	int retval;
+
+	mutex_lock(&piix4_mutex_sb800);
+
+	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
+	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
+
+	port = adapdata->port;
+	if ((smba_en_lo & 6) != (port << 1))
+		outb_p((smba_en_lo & ~6) | (port << 1),
+		       SB800_PIIX4_SMB_IDX + 1);
+
+	retval = piix4_access(adap, addr, flags, read_write,
+			      command, size, data);
+
+	outb_p(smba_en_lo, SB800_PIIX4_SMB_IDX + 1);
+
+	mutex_unlock(&piix4_mutex_sb800);
+
+	return retval;
+}
+
 static u32 piix4_func(struct i2c_adapter *adapter)
 {
 	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
@@ -538,6 +583,11 @@ static const struct i2c_algorithm smbus_algorithm = {
 	.functionality	= piix4_func,
 };
 
+static const struct i2c_algorithm piix4_smbus_algorithm_sb800 = {
+	.smbus_xfer	= piix4_access_sb800,
+	.functionality	= piix4_func,
+};
+
 static const struct pci_device_id piix4_ids[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82443MX_3) },
@@ -613,6 +663,42 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 	return 0;
 }
 
+static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba)
+{
+	unsigned short port;
+	int retval;
+	struct i2c_piix4_adapdata *adapdata;
+
+	for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
+		retval = piix4_add_adapter(dev, smba,
+					   &piix4_main_adapters[port]);
+		if (retval < 0)
+			goto error;
+
+		piix4_main_adapters[port]->algo = &piix4_smbus_algorithm_sb800;
+
+		adapdata = i2c_get_adapdata(piix4_main_adapters[port]);
+		adapdata->port = port;
+	}
+
+	return retval;
+
+error:
+	dev_err(&dev->dev,
+		"Error setting up SB800 adapters. Unregistering!\n");
+	while (--port >= 0) {
+		adapdata = i2c_get_adapdata(piix4_main_adapters[port]);
+		if (adapdata->smba) {
+			i2c_del_adapter(piix4_main_adapters[port]);
+			kfree(adapdata);
+			kfree(piix4_main_adapters[port]);
+			piix4_main_adapters[port] = NULL;
+		}
+	}
+
+	return retval;
+}
+
 static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int retval;
@@ -631,19 +717,28 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 		/* base address location etc changed in SB800 */
 		retval = piix4_setup_sb800(dev, id, 0);
+		if (retval < 0)
+			return retval;
+
+		/*
+		 * Try to register multiplexed main SMBus adapter,
+		 * give up if we can't
+		 */
+		retval = piix4_add_adapters_sb800(dev, retval);
 	} else {
 		retval = piix4_setup(dev, id);
+		if (retval < 0)
+			return retval;
+
+		/* Try to register main SMBus adapter, give up if we can't */
+		retval = piix4_add_adapter(dev, retval,
+					   &piix4_main_adapters[0]);
 	}
 
 	/* If no main SMBus found, give up */
 	if (retval < 0)
 		return retval;
 
-	/* Try to register main SMBus adapter, give up if we can't */
-	retval = piix4_add_adapter(dev, retval, &piix4_main_adapters[0]);
-	if (retval < 0)
-		return retval;
-
 	/* Check for auxiliary SMBus on some AMD chipsets */
 	retval = -ENODEV;
 
-- 
1.9.1

Re: [PATCH v3 4/5] i2c-piix4: Add support for multiplexed main adapter in SB800

[Andy Shevchenko]

On Sat, 2015-11-07 at 12:35 +0100, Christian Fetzer wrote:
> The SB800 chipset supports a multiplexed main SMBus controller with
> four ports. The multiplexed ports share the same SMBus address and
> register set. The port is selected by bits 2:1 of the smb_en register
> (0x2C).
> 
> Only one port can be active at any point in time therefore a mutex is
> needed in order to synchronize access.
> 
> Tested on HP ProLiant MicroServer G7 N54L (where this patch adds
> support to access sensor data from the w83795adg).
> 
> Cc: Thomas Brandon <tbrandonau@gmail.com>
> Cc: Eddi De Pieri <eddi@depieri.net>
> Signed-off-by: Christian Fetzer <fetzer.ch@gmail.com>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/i2c/busses/i2c-piix4.c | 105
> +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 100 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-
> piix4.c
> index 67ada1e..4beaa3d 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -23,6 +23,9 @@
>  
>     Note: we assume there can only be one device, with one or more
>     SMBus interfaces.
> +   The device can register multiple i2c_adapters (up to
> PIIX4_MAX_ADAPTERS).
> +   For devices supporting multiple ports the i2c_adapter should
> provide
> +   an i2c_algorithm to access them.
>  */
>  
>  #include <linux/module.h>
> @@ -37,6 +40,7 @@
>  #include <linux/dmi.h>
>  #include <linux/acpi.h>
>  #include <linux/io.h>
> +#include <linux/mutex.h>
>  
>  
>  /* PIIX4 SMBus address offsets */
> @@ -129,10 +133,12 @@ static const struct dmi_system_id
> piix4_dmi_ibm[] = {
>  };
>  
>  /* SB800 globals */
> +DEFINE_MUTEX(piix4_mutex_sb800);

Same question as for patch 3.

>  static bool piix4_smb_idx_sb800;
>  
>  struct i2c_piix4_adapdata {
>  	unsigned short smba;
> +	unsigned short port;
>  };
>  
>  static int piix4_setup(struct pci_dev *PIIX4_dev,
> @@ -312,6 +318,8 @@ static int piix4_setup_sb800(struct pci_dev
> *PIIX4_dev,
>  	else
>  		dev_dbg(&PIIX4_dev->dev, "Using SMI# for SMBus\n");
>  
> +	mutex_init(&piix4_mutex_sb800);
> +
>  	dev_info(&PIIX4_dev->dev,
>  		 "SMBus Host Controller at 0x%x, revision %d\n",
>  		 piix4_smba, i2ccfg >> 4);
> @@ -526,6 +534,43 @@ static s32 piix4_access(struct i2c_adapter *
> adap, u16 addr,
>  	return 0;
>  }
>  
> +/*
> + * Handles access to multiple SMBus ports on the SB800.
> + * The port is selected by bits 2:1 of the smb_en register (0x2C).
> + * Returns negative errno on error.
> + *
> + * Note: The selected port must be returned to the initial selection
> to avoid
> + * problems on certain systems.
> + */
> +static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
> +		 unsigned short flags, char read_write,
> +		 u8 command, int size, union i2c_smbus_data *data)
> +{
> +	struct i2c_piix4_adapdata *adapdata =
> i2c_get_adapdata(adap);
> +	u8 smba_en_lo, smb_en = 0x2c;
> +	u8 port;
> +	int retval;
> +
> +	mutex_lock(&piix4_mutex_sb800);
> +
> +	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
> +	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
> +
> +	port = adapdata->port;
> +	if ((smba_en_lo & 6) != (port << 1))
> +		outb_p((smba_en_lo & ~6) | (port << 1),
> +		       SB800_PIIX4_SMB_IDX + 1);
> +
> +	retval = piix4_access(adap, addr, flags, read_write,
> +			      command, size, data);
> +
> +	outb_p(smba_en_lo, SB800_PIIX4_SMB_IDX + 1);
> +
> +	mutex_unlock(&piix4_mutex_sb800);
> +
> +	return retval;
> +}
> +
>  static u32 piix4_func(struct i2c_adapter *adapter)
>  {
>  	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
> @@ -538,6 +583,11 @@ static const struct i2c_algorithm
> smbus_algorithm = {
>  	.functionality	= piix4_func,
>  };
>  
> +static const struct i2c_algorithm piix4_smbus_algorithm_sb800 = {
> +	.smbus_xfer	= piix4_access_sb800,
> +	.functionality	= piix4_func,
> +};
> +
>  static const struct pci_device_id piix4_ids[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_82371AB_3) },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_82443MX_3) },
> @@ -613,6 +663,42 @@ static int piix4_add_adapter(struct pci_dev
> *dev, unsigned short smba,
>  	return 0;
>  }
>  
> +static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned
> short smba)
> +{
> +	unsigned short port;

This one has to be at least signed and int to be aligned with patch 2.

> +	int retval;
> +	struct i2c_piix4_adapdata *adapdata;

These lines better to exchange.

> +
> +	for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
> +		retval = piix4_add_adapter(dev, smba,
> +					   &piix4_main_adapters[port
> ]);
> +		if (retval < 0)
> +			goto error;
> +
> +		piix4_main_adapters[port]->algo =
> &piix4_smbus_algorithm_sb800;
> +
> +		adapdata =
> i2c_get_adapdata(piix4_main_adapters[port]);
> +		adapdata->port = port;
> +	}
> +
> +	return retval;
> +
> +error:
> +	dev_err(&dev->dev,
> +		"Error setting up SB800 adapters.
> Unregistering!\n");
> +	while (--port >= 0) {
> +		adapdata =
> i2c_get_adapdata(piix4_main_adapters[port]);
> +		if (adapdata->smba) {
> +			i2c_del_adapter(piix4_main_adapters[port]);
> +			kfree(adapdata);
> +			kfree(piix4_main_adapters[port]);
> +			piix4_main_adapters[port] = NULL;
> +		}
> +	}
> +
> +	return retval;
> +}
> +
>  static int piix4_probe(struct pci_dev *dev, const struct
> pci_device_id *id)
>  {
>  	int retval;
> @@ -631,19 +717,28 @@ static int piix4_probe(struct pci_dev *dev,
> const struct pci_device_id *id)
>  
>  		/* base address location etc changed in SB800 */
>  		retval = piix4_setup_sb800(dev, id, 0);
> +		if (retval < 0)
> +			return retval;
> +
> +		/*
> +		 * Try to register multiplexed main SMBus adapter,
> +		 * give up if we can't
> +		 */
> +		retval = piix4_add_adapters_sb800(dev, retval);
>  	} else {
>  		retval = piix4_setup(dev, id);
> +		if (retval < 0)
> +			return retval;
> +
> +		/* Try to register main SMBus adapter, give up if we
> can't */
> +		retval = piix4_add_adapter(dev, retval,
> +					   &piix4_main_adapters[0]);
>  	}
>  
>  	/* If no main SMBus found, give up */
>  	if (retval < 0)
>  		return retval;
>  
> -	/* Try to register main SMBus adapter, give up if we can't
> */
> -	retval = piix4_add_adapter(dev, retval,
> &piix4_main_adapters[0]);
> -	if (retval < 0)
> -		return retval;
> -
>  	/* Check for auxiliary SMBus on some AMD chipsets */
>  	retval = -ENODEV;
>  

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 4/5] i2c-piix4: Add support for multiplexed main adapter in SB800

[Jean Delvare]

Sorry for the late reaction, but...

On Mon, 09 Nov 2015 12:45:42 +0200, Andy Shevchenko wrote:
> On Sat, 2015-11-07 at 12:35 +0100, Christian Fetzer wrote:
> (...)
> > @@ -129,10 +133,12 @@ static const struct dmi_system_id
> > piix4_dmi_ibm[] = {
> >  };
> >  
> >  /* SB800 globals */
> > +DEFINE_MUTEX(piix4_mutex_sb800);
> 
> Same question as for patch 3.

Noooooo!

The mutex is needed to ensure that only one of the 4 multiplexed SMBus
ports is used at any given time. i2c_piix4_adapdata is per i2c_adapter,
so if you move the mutex there you get one mutex per SMBus port, which
completely voids the point of having a mutex. A per-port mutex doesn't
protect anything, all it does is serialize the access to THAT port, but
i2c-core already takes care of this (thankfully.)

So moving this mutex to i2c_piix4_adapdata introduced a serious bug in
the driver, which needs to be fixed ASAP.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v3 4/5] i2c-piix4: Add support for multiplexed main adapter in SB800

[Andy Shevchenko]

On Fri, 2016-01-22 at 13:39 +0100, Jean Delvare wrote:
> Sorry for the late reaction, but...
> 
> On Mon, 09 Nov 2015 12:45:42 +0200, Andy Shevchenko wrote:
> > On Sat, 2015-11-07 at 12:35 +0100, Christian Fetzer wrote:
> > (...)
> > > @@ -129,10 +133,12 @@ static const struct dmi_system_id
> > > piix4_dmi_ibm[] = {
> > >  };
> > >  
> > >  /* SB800 globals */
> > > +DEFINE_MUTEX(piix4_mutex_sb800);
> > 
> > Same question as for patch 3.
> 
> Noooooo!

Calm down, it was only a question. I don't know why the author chose
the way that is in upstream.

Good you have explained this here.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v3 5/5] i2c-piix4: Add adapter port name support for SB800 chipset

[Christian Fetzer]

This patch adds support for port names for the SB800 chipset.
Since the chipset supports a multiplexed main SMBus controller, adding
the channel name to the adapter name is necessary to differentiate the
ports better (for example in sensors output).

Signed-off-by: Christian Fetzer <fetzer.ch@gmail.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/busses/i2c-piix4.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 4beaa3d..47a221d 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -135,6 +135,10 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
 /* SB800 globals */
 DEFINE_MUTEX(piix4_mutex_sb800);
 static bool piix4_smb_idx_sb800;
+static const char *piix4_main_port_names_sb800[PIIX4_MAX_ADAPTERS] = {
+	"SDA0", "SDA2", "SDA3", "SDA4"
+};
+static const char *piix4_aux_port_name_sb800 = "SDA1";
 
 struct i2c_piix4_adapdata {
 	unsigned short smba;
@@ -617,7 +621,7 @@ static struct i2c_adapter *piix4_main_adapters[PIIX4_MAX_ADAPTERS];
 static struct i2c_adapter *piix4_aux_adapter;
 
 static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
-			     struct i2c_adapter **padap)
+			     const char *name, struct i2c_adapter **padap)
 {
 	struct i2c_adapter *adap;
 	struct i2c_piix4_adapdata *adapdata;
@@ -646,7 +650,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 	adap->dev.parent = &dev->dev;
 
 	snprintf(adap->name, sizeof(adap->name),
-		"SMBus PIIX4 adapter at %04x", smba);
+		"SMBus PIIX4 adapter %s at %04x", name, smba);
 
 	i2c_set_adapdata(adap, adapdata);
 
@@ -671,6 +675,7 @@ static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba)
 
 	for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
 		retval = piix4_add_adapter(dev, smba,
+					   piix4_main_port_names_sb800[port],
 					   &piix4_main_adapters[port]);
 		if (retval < 0)
 			goto error;
@@ -731,7 +736,7 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 			return retval;
 
 		/* Try to register main SMBus adapter, give up if we can't */
-		retval = piix4_add_adapter(dev, retval,
+		retval = piix4_add_adapter(dev, retval, "main",
 					   &piix4_main_adapters[0]);
 	}
 
@@ -760,7 +765,8 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	if (retval > 0) {
 		/* Try to add the aux adapter if it exists,
 		 * piix4_add_adapter will clean up if this fails */
-		piix4_add_adapter(dev, retval, &piix4_aux_adapter);
+		piix4_add_adapter(dev, retval, piix4_aux_port_name_sb800,
+				  &piix4_aux_adapter);
 	}
 
 	return 0;
-- 
1.9.1