Skip to contentSkip to conversation

Support multiplexed main SMBus interface on SB800

PATCH Applied Christian Fetzer 4 participants 12 messages Nov 19, 2015 – Jan 25, 2016

Subsystems (2)
I2C subsystem host drivers I2C/smbus controller drivers for pc
lore
Share & tools
lore

Christian Fetzer

author
#0 lore raw

v5 cover Support multiplexed main SMBus interface on SB800

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/[email protected]/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

v5:
- Incorporated changes requested by Andy
- Defines for magic numbers

v4:
- Incorporated changes requested by Andy
- added mutex to struct i2c_piix4_adapdata
- added flag for releasing SMBus index region to struct i2c_piix4_adapdata
- this flag now indicates that the adapter is a sb800 main adapter
- together with the port number it simplifies the adapter releasing and the first patch in v3 is no more needed
- unfortunately patch 3 and 4 in v3 had to be combined as only the patch introducing multiplexing adds a separate add_adapters_sb800 method that can be used to set the flag.
- fixed releasing the SMBus index region in case setting up the adapter fails

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 (3):
i2c-piix4: Convert piix4_main_adapter to array
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 | 198 +++++++++++++++++++++++++++++++++++------
 1 file changed, 169 insertions(+), 29 deletions(-)
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH v5 1/3 i2c-piix4: Convert piix4_main_adapter to array

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 <[email protected]>
Reviewed-by Mika Westerberg <[email protected]>
Reviewed-by Andy Shevchenko <[email protected]>
---
 drivers/i2c/busses/i2c-piix4.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)
drivers/i2c/busses/i2c-piix4.c +12 -5
index 630bce6..9c32eb1 100644
@@ -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;
 

@@ -674,9 +677,13 @@ 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_main_adapter = NULL;
+	int port = PIIX4_MAX_ADAPTERS;
+
+	while (--port >= 0) {
+		if (piix4_main_adapters[port]) {
+			piix4_adap_remove(piix4_main_adapters[port]);
+			piix4_main_adapters[port] = NULL;
+		}
 	}
 
 	if (piix4_aux_adapter) {
Signature · 1 line
1.9.1
author
#2 lore raw

PATCH v5 2/3 i2c-piix4: Add support for multiplexed main adapter in SB800

In reply to 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.

Additionally, the commit avoids requesting and releasing the SMBus base address index region on every multiplexed transfer by moving the request_region call into piix4_probe.

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

Cc Thomas Brandon <[email protected]>
Cc Eddi De Pieri <[email protected]>
Signed-off-by Christian Fetzer <[email protected]>
Reviewed-by Mika Westerberg <[email protected]>
Reviewed-by Andy Shevchenko <[email protected]>
---
 drivers/i2c/busses/i2c-piix4.c | 169 +++++++++++++++++++++++++++++++++++------
 1 file changed, 147 insertions(+), 22 deletions(-)
drivers/i2c/busses/i2c-piix4.c +147 -22
index 9c32eb1..6b4231d 100644
@@ -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 */

@@ -78,6 +82,13 @@
 /* Multi-port constants */
 #define PIIX4_MAX_ADAPTERS 4
 
+/* SB800 constants */
+#define SB800_PIIX4_SMB_IDX		0xcd6
+
+/* SB800 port is selected by bits 2:1 of the smb_en register (0x2c) */
+#define SB800_PIIX4_PORT_IDX		0x2c
+#define SB800_PIIX4_PORT_IDX_MASK	0x06
+
 /* insmod parameters */
 
 /* If force is set to anything different from 0, we forcibly enable the

@@ -127,6 +138,11 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
 
 struct i2c_piix4_adapdata {
 	unsigned short smba;
+
+	/* SB800 */
+	bool sb800_main;
+	unsigned short port;
+	struct mutex *mutex;
 };
 
 static int piix4_setup(struct pci_dev *PIIX4_dev,

@@ -232,7 +248,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 +269,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;

@@ -527,6 +536,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;
+	u8 port;
+	int retval;
+
+	mutex_lock(adapdata->mutex);
+
+	outb_p(SB800_PIIX4_PORT_IDX, SB800_PIIX4_SMB_IDX);
+	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),
+		       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(adapdata->mutex);
+
+	return retval;
+}
+
 static u32 piix4_func(struct i2c_adapter *adapter)
 {
 	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |

@@ -539,6 +585,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) },

@@ -614,6 +665,53 @@ 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)
+{
+	struct mutex *mutex;
+	struct i2c_piix4_adapdata *adapdata;
+	int port;
+	int retval;
+
+	mutex = kzalloc(sizeof(*mutex), GFP_KERNEL);
+	if (mutex == NULL)
+		return -ENOMEM;
+
+	mutex_init(mutex);
+
+	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->sb800_main = true;
+		adapdata->port = port;
+		adapdata->mutex = mutex;
+	}
+
+	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;
+		}
+	}
+
+	kfree(mutex);
+
+	return retval;
+}
+
 static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int retval;

@@ -621,20 +719,41 @@ 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;
+		}
+
 		/* base address location etc changed in SB800 */
 		retval = piix4_setup_sb800(dev, id, 0);
-	else
-		retval = piix4_setup(dev, id);
+		if (retval < 0) {
+			release_region(SB800_PIIX4_SMB_IDX, 2);
+			return retval;
+		}
 
-	/* If no main SMBus found, give up */
-	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);
+		if (retval < 0) {
+			release_region(SB800_PIIX4_SMB_IDX, 2);
+			return 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 (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;

@@ -669,7 +788,13 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
 
 	if (adapdata->smba) {
 		i2c_del_adapter(adap);
-		release_region(adapdata->smba, SMBIOSIZE);
+		if (adapdata->port == 0) {
+			release_region(adapdata->smba, SMBIOSIZE);
+			if (adapdata->sb800_main) {
+				kfree(adapdata->mutex);
+				release_region(SB800_PIIX4_SMB_IDX, 2);
+			}
+		}
 		kfree(adapdata);
 		kfree(adap);
 	}
Signature · 1 line
1.9.1
author
#3 lore raw

PATCH v5 3/3 i2c-piix4: Add adapter port name support for SB800 chipset

In reply to 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 <[email protected]>
Reviewed-by Mika Westerberg <[email protected]>
Reviewed-by Andy Shevchenko <[email protected]>
---
 drivers/i2c/busses/i2c-piix4.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-piix4.c +12 -4
index 6b4231d..af4e606 100644
@@ -136,6 +136,12 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
 	{ },
 };
 
+/* SB800 globals */
+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;
 

@@ -619,7 +625,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;

@@ -648,7 +654,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);
 

@@ -680,6 +686,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;

@@ -749,7 +756,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]);
 		if (retval < 0)
 			return retval;

@@ -776,7 +783,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;
Signature · 1 line
1.9.1
#4 lore raw

Re: [PATCH v5 3/3] i2c-piix4: Add adapter port name support for SB800 chipset

Quoted 5 lines · 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).
(...)

Note that I'm not too happy with this change. I understand the need for unique I2C bus names, however changing names for legacy devices which have a single bus is bad. The name can be used in sensors.conf or in scripts to uniquely reference a specific I2C bus, and changing it will break that.

So I'd rather only change the names for the mux'd SB800 ports where this is needed, and leave the rest untouched.

Also I don't much like "SDA0"... in bus names. SDA0 standard for "serial data 0" is the name of one of the pins for SMBus channel 0. The other one is SCL0 ("serial clock 0".) There's no reason to include a pin name in the I2C adapter name string, just the channel number matters.

I'll submit a patch later today that implements things the way I would like, and you can comment on it.

Signature · 2 lines
Jean Delvare
SUSE L3 Support
Applied
#5 lore raw
In reply to Christian Fetzer
Quoted 13 lines · 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/[email protected]/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

Quoted 8 lines

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.

Applied to for-next, thanks! And thanks to Mika and Andy for the review!

#6 lore raw
In reply to Christian Fetzer

Hi Christian,

Quoted 13 lines · 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/[email protected]/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

Quoted 5 lines

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.

I think I can explain it. In piix4_setup_sb800() you touch the SB800_PIIX4_SMB_IDX port without first taking the mutex that protects it. You only take the mutex on transactions (in piix4_access_sb800) not during initialization. If self-probing I2C device drivers such as jc42 are already loaded before you load i2c-piix4, then as soon as the first SMBus port is registered, i2c-core will try to attach I2C devices to it, while at the same time i2c-piix4 is registering the second SMBus port. So SB800_PIIX4_SMB_IDX is changed while piix4_access_sb800 accesses it and chaos happens.

I think if we had proper locking in piix4_setup_sb800() then you should be able to load jc42 first and then i2c-piix4 and it should work fine.

Signature · 2 lines
Jean Delvare
SUSE L3 Support
author
#7 lore raw

Hi Jean,

Quoted 32 lines · Jean Delvare

Hi Christian,

Quoted 13 lines · 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/[email protected]/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

Quoted 5 lines

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.

I think I can explain it. In piix4_setup_sb800() you touch the SB800_PIIX4_SMB_IDX port without first taking the mutex that protects it. You only take the mutex on transactions (in piix4_access_sb800) not during initialization. If self-probing I2C device drivers such as jc42 are already loaded before you load i2c-piix4, then as soon as the first SMBus port is registered, i2c-core will try to attach I2C devices to it, while at the same time i2c-piix4 is registering the second SMBus port. So SB800_PIIX4_SMB_IDX is changed while piix4_access_sb800 accesses it and chaos happens.

Quoted 3 lines

I think if we had proper locking in piix4_setup_sb800() then you should be able to load jc42 first and then i2c-piix4 and it should work fine.

Since the problem remains even after your patch, I'll try to provide more information about the issue.

If i2c_piix4 is loaded before jc42 and w83795 things work as they should and sensors prints values from all modules.

If w83795 is loaded before i2c_piix4, the w83795adg is not detected. I'm not sure though if this is because of i2c_piix4 or just a shortcoming in the w83795 module.

If jc42 is loaded before i2c_piix4 then the problem described above occurs. It seems that jc42 tries to read from all multiplexed ports. This is the full sensors output:

jc42-i2c-0-18
Adapter: SMBus PIIX4 adapter SDA0 at 0b00
RAM1 Temp: +26.8°C (low = +0.0°C)

                       (high = +60.0°C, hyst = +54.0°C)
                       (crit = +70.0°C, hyst = +64.0°C)

jc42-i2c-0-19
Adapter: SMBus PIIX4 adapter SDA0 at 0b00
RAM2 Temp: +26.5°C (low = +0.0°C)

                       (high = +60.0°C, hyst = +54.0°C)
                       (crit = +70.0°C, hyst = +64.0°C)

k10temp-pci-00c3
Adapter: PCI adapter
CPU Temp: +30.6°C (high = +70.0°C)

                       (crit = +100.0°C, hyst = +95.0°C)

jc42-i2c-1-18
Adapter: SMBus PIIX4 adapter SDA2 at 0b00
ERROR: Can't get value of subfeature temp1_min_alarm: Can't read
ERROR: Can't get value of subfeature temp1_max_alarm: Can't read
ERROR: Can't get value of subfeature temp1_crit_alarm: Can't read
ERROR: Can't get value of subfeature temp1_min: Can't read
ERROR: Can't get value of subfeature temp1_max: Can't read
ERROR: Can't get value of subfeature temp1_max_hyst: Can't read
ERROR: Can't get value of subfeature temp1_crit: Can't read
ERROR: Can't get value of subfeature temp1_crit_hyst: Can't read
temp1: N/A (low = +0.0°C)

                       (high =  +0.0°C, hyst =  +0.0°C)
                       (crit =  +0.0°C, hyst =  +0.0°C)

jc42-i2c-1-19
Adapter: SMBus PIIX4 adapter SDA2 at 0b00
ERROR: Can't get value of subfeature temp1_min_alarm: Can't read
ERROR: Can't get value of subfeature temp1_max_alarm: Can't read
ERROR: Can't get value of subfeature temp1_crit_alarm: Can't read
ERROR: Can't get value of subfeature temp1_min: Can't read
ERROR: Can't get value of subfeature temp1_max: Can't read
ERROR: Can't get value of subfeature temp1_max_hyst: Can't read
ERROR: Can't get value of subfeature temp1_crit: Can't read
ERROR: Can't get value of subfeature temp1_crit_hyst: Can't read
temp1: N/A (low = +0.0°C)

                       (high =  +0.0°C, hyst =  +0.0°C)
                       (crit =  +0.0°C, hyst =  +0.0°C)

jc42-i2c-2-18
Adapter: SMBus PIIX4 adapter SDA3 at 0b00
ERROR: Can't get value of subfeature temp1_min_alarm: Can't read
ERROR: Can't get value of subfeature temp1_max_alarm: Can't read
ERROR: Can't get value of subfeature temp1_crit_alarm: Can't read
ERROR: Can't get value of subfeature temp1_min: Can't read
ERROR: Can't get value of subfeature temp1_max: Can't read
ERROR: Can't get value of subfeature temp1_max_hyst: Can't read
ERROR: Can't get value of subfeature temp1_crit: Can't read
ERROR: Can't get value of subfeature temp1_crit_hyst: Can't read
temp1: N/A (low = +0.0°C)

                       (high =  +0.0°C, hyst =  +0.0°C)
                       (crit =  +0.0°C, hyst =  +0.0°C)

jc42-i2c-2-19
Adapter: SMBus PIIX4 adapter SDA3 at 0b00
ERROR: Can't get value of subfeature temp1_min_alarm: Can't read
ERROR: Can't get value of subfeature temp1_max_alarm: Can't read
ERROR: Can't get value of subfeature temp1_crit_alarm: Can't read
ERROR: Can't get value of subfeature temp1_min: Can't read
ERROR: Can't get value of subfeature temp1_max: Can't read
ERROR: Can't get value of subfeature temp1_max_hyst: Can't read
ERROR: Can't get value of subfeature temp1_crit: Can't read
ERROR: Can't get value of subfeature temp1_crit_hyst: Can't read
temp1: N/A (low = +0.0°C)

                       (high =  +0.0°C, hyst =  +0.0°C)
                       (crit =  +0.0°C, hyst =  +0.0°C)

jc42-i2c-3-18
Adapter: SMBus PIIX4 adapter SDA4 at 0b00
ERROR: Can't get value of subfeature temp1_min_alarm: Can't read
ERROR: Can't get value of subfeature temp1_max_alarm: Can't read
ERROR: Can't get value of subfeature temp1_crit_alarm: Can't read
ERROR: Can't get value of subfeature temp1_min: Can't read
ERROR: Can't get value of subfeature temp1_max: Can't read
ERROR: Can't get value of subfeature temp1_max_hyst: Can't read
ERROR: Can't get value of subfeature temp1_crit: Can't read
ERROR: Can't get value of subfeature temp1_crit_hyst: Can't read
temp1: N/A (low = +0.0°C)

                       (high =  +0.0°C, hyst =  +0.0°C)
                       (crit =  +0.0°C, hyst =  +0.0°C)

jc42-i2c-3-19
Adapter: SMBus PIIX4 adapter SDA4 at 0b00
ERROR: Can't get value of subfeature temp1_min_alarm: Can't read
ERROR: Can't get value of subfeature temp1_max_alarm: Can't read
ERROR: Can't get value of subfeature temp1_crit_alarm: Can't read
ERROR: Can't get value of subfeature temp1_min: Can't read
ERROR: Can't get value of subfeature temp1_max: Can't read
ERROR: Can't get value of subfeature temp1_max_hyst: Can't read
ERROR: Can't get value of subfeature temp1_crit: Can't read
ERROR: Can't get value of subfeature temp1_crit_hyst: Can't read
temp1: N/A (low = +0.0°C)

                       (high =  +0.0°C, hyst =  +0.0°C)
                       (crit =  +0.0°C, hyst =  +0.0°C)
#8 lore raw

Hi Christian,

Quoted 29 lines · fetzerch
Quoted 22 lines · Jean Delvare
Quoted 10 lines · Christian Fetzer

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.

I think I can explain it. In piix4_setup_sb800() you touch the SB800_PIIX4_SMB_IDX port without first taking the mutex that protects it. You only take the mutex on transactions (in piix4_access_sb800) not during initialization. If self-probing I2C device drivers such as jc42 are already loaded before you load i2c-piix4, then as soon as the first SMBus port is registered, i2c-core will try to attach I2C devices to it, while at the same time i2c-piix4 is registering the second SMBus port. So SB800_PIIX4_SMB_IDX is changed while piix4_access_sb800 accesses it and chaos happens.

Quoted 2 lines

I think if we had proper locking in piix4_setup_sb800() then you should be able to load jc42 first and then i2c-piix4 and it should work fine.

Since the problem remains even after your patch, I'll try to provide more information about the issue.

Quoted 6 lines

If i2c_piix4 is loaded before jc42 and w83795 things work as they should and sensors prints values from all modules.

If w83795 is loaded before i2c_piix4, the w83795adg is not detected. I'm not sure though if this is because of i2c_piix4 or just a shortcoming in the w83795 module.

The w83795 driver works fine for many users including myself for years. So definitely this is related to the i2c-piix4 driver.

Quoted 102 lines

If jc42 is loaded before i2c_piix4 then the problem described above occurs. It seems that jc42 tries to read from all multiplexed ports. This is the full sensors output:

jc42-i2c-0-18
Adapter: SMBus PIIX4 adapter SDA0 at 0b00
RAM1 Temp: +26.8°C (low = +0.0°C)

                       (high = +60.0°C, hyst = +54.0°C)
                       (crit = +70.0°C, hyst = +64.0°C)

jc42-i2c-0-19
Adapter: SMBus PIIX4 adapter SDA0 at 0b00
RAM2 Temp: +26.5°C (low = +0.0°C)

                       (high = +60.0°C, hyst = +54.0°C)
                       (crit = +70.0°C, hyst = +64.0°C)

k10temp-pci-00c3
Adapter: PCI adapter
CPU Temp: +30.6°C (high = +70.0°C)

                       (crit = +100.0°C, hyst = +95.0°C)

jc42-i2c-1-18
Adapter: SMBus PIIX4 adapter SDA2 at 0b00
ERROR: Can't get value of subfeature temp1_min_alarm: Can't read
ERROR: Can't get value of subfeature temp1_max_alarm: Can't read
ERROR: Can't get value of subfeature temp1_crit_alarm: Can't read
ERROR: Can't get value of subfeature temp1_min: Can't read
ERROR: Can't get value of subfeature temp1_max: Can't read
ERROR: Can't get value of subfeature temp1_max_hyst: Can't read
ERROR: Can't get value of subfeature temp1_crit: Can't read
ERROR: Can't get value of subfeature temp1_crit_hyst: Can't read
temp1: N/A (low = +0.0°C)

                       (high =  +0.0°C, hyst =  +0.0°C)
                       (crit =  +0.0°C, hyst =  +0.0°C)

jc42-i2c-1-19
Adapter: SMBus PIIX4 adapter SDA2 at 0b00
ERROR: Can't get value of subfeature temp1_min_alarm: Can't read
ERROR: Can't get value of subfeature temp1_max_alarm: Can't read
ERROR: Can't get value of subfeature temp1_crit_alarm: Can't read
ERROR: Can't get value of subfeature temp1_min: Can't read
ERROR: Can't get value of subfeature temp1_max: Can't read
ERROR: Can't get value of subfeature temp1_max_hyst: Can't read
ERROR: Can't get value of subfeature temp1_crit: Can't read
ERROR: Can't get value of subfeature temp1_crit_hyst: Can't read
temp1: N/A (low = +0.0°C)

                       (high =  +0.0°C, hyst =  +0.0°C)
                       (crit =  +0.0°C, hyst =  +0.0°C)

jc42-i2c-2-18
Adapter: SMBus PIIX4 adapter SDA3 at 0b00
ERROR: Can't get value of subfeature temp1_min_alarm: Can't read
ERROR: Can't get value of subfeature temp1_max_alarm: Can't read
ERROR: Can't get value of subfeature temp1_crit_alarm: Can't read
ERROR: Can't get value of subfeature temp1_min: Can't read
ERROR: Can't get value of subfeature temp1_max: Can't read
ERROR: Can't get value of subfeature temp1_max_hyst: Can't read
ERROR: Can't get value of subfeature temp1_crit: Can't read
ERROR: Can't get value of subfeature temp1_crit_hyst: Can't read
temp1: N/A (low = +0.0°C)

                       (high =  +0.0°C, hyst =  +0.0°C)
                       (crit =  +0.0°C, hyst =  +0.0°C)

jc42-i2c-2-19
Adapter: SMBus PIIX4 adapter SDA3 at 0b00
ERROR: Can't get value of subfeature temp1_min_alarm: Can't read
ERROR: Can't get value of subfeature temp1_max_alarm: Can't read
ERROR: Can't get value of subfeature temp1_crit_alarm: Can't read
ERROR: Can't get value of subfeature temp1_min: Can't read
ERROR: Can't get value of subfeature temp1_max: Can't read
ERROR: Can't get value of subfeature temp1_max_hyst: Can't read
ERROR: Can't get value of subfeature temp1_crit: Can't read
ERROR: Can't get value of subfeature temp1_crit_hyst: Can't read
temp1: N/A (low = +0.0°C)

                       (high =  +0.0°C, hyst =  +0.0°C)
                       (crit =  +0.0°C, hyst =  +0.0°C)

jc42-i2c-3-18
Adapter: SMBus PIIX4 adapter SDA4 at 0b00
ERROR: Can't get value of subfeature temp1_min_alarm: Can't read
ERROR: Can't get value of subfeature temp1_max_alarm: Can't read
ERROR: Can't get value of subfeature temp1_crit_alarm: Can't read
ERROR: Can't get value of subfeature temp1_min: Can't read
ERROR: Can't get value of subfeature temp1_max: Can't read
ERROR: Can't get value of subfeature temp1_max_hyst: Can't read
ERROR: Can't get value of subfeature temp1_crit: Can't read
ERROR: Can't get value of subfeature temp1_crit_hyst: Can't read
temp1: N/A (low = +0.0°C)

                       (high =  +0.0°C, hyst =  +0.0°C)
                       (crit =  +0.0°C, hyst =  +0.0°C)

jc42-i2c-3-19
Adapter: SMBus PIIX4 adapter SDA4 at 0b00
ERROR: Can't get value of subfeature temp1_min_alarm: Can't read
ERROR: Can't get value of subfeature temp1_max_alarm: Can't read
ERROR: Can't get value of subfeature temp1_crit_alarm: Can't read
ERROR: Can't get value of subfeature temp1_min: Can't read
ERROR: Can't get value of subfeature temp1_max: Can't read
ERROR: Can't get value of subfeature temp1_max_hyst: Can't read
ERROR: Can't get value of subfeature temp1_crit: Can't read
ERROR: Can't get value of subfeature temp1_crit_hyst: Can't read
temp1: N/A (low = +0.0°C)

Quoted 2 lines
                       (high =  +0.0°C, hyst =  +0.0°C)
                       (crit =  +0.0°C, hyst =  +0.0°C)

It means that the autodetection function of the jc42 driver returned success, allowing the driver to instantiate jc42 devices on each of them. This means that SMBus port 0 was selected at that time even though the kernel believed port 2 then 3 then 4 was selected. Later on (when you run "sensors") port selection works properly so the jc42 driver returns error as intended for non-existent devices.

Where is the W83795ADG device when it is properly detected?

I looked at the code again but can't see any obvious problem left after applying my patch.

One thing that bothers me is that pins for SMBus ports may be shared with other functions. The "AMD SB810/850 Southbridge Databook" document shows that all SMBus ports are shared with at least GPIOs and port 4 also with PS/2 function. Unfortunately I can't find any document explaining how to check which function is assigned to each pin.

Signature · 2 lines
Jean Delvare
SUSE L3 Support
#9 lore raw

Hi Jean,

Quoted 5 lines

One thing that bothers me is that pins for SMBus ports may be shared with other functions. The "AMD SB810/850 Southbridge Databook" document shows that all SMBus ports are shared with at least GPIOs and port 4 also with PS/2 function. Unfortunately I can't find any document explaining how to check which function is assigned to each pin.

Well it is quite easy, you need to check IOMUX register for specific pin.

The base address "AcpiMMioAddr" is defined in PM_reg x24, with the default base address at "FED8_0000"

See 2.3.7 IoMux Registers

PS2_CLK/SCL4/ GPIO188

The function 0 is PS2_CLIK function1 is SCL4 and GPIO188 is fun2, so all you need is to check it at "AcpiMMioAddr" + 0xd00 + 188 (8 bit register) bits 1:0 encode it. 00 - function 0 etc...

Btw similar functionality is also on later SBs of AMD, I have an hudson system, so we can add the support for it later, when SB800 is done. I think it follows same patern, maybe the GPIOs will be different.

Thanks
Rudolf

#10 lore raw
In reply to fetzerch

Hi Christian,

Quoted 23 lines · fetzerch
Quoted 22 lines · Jean Delvare
Quoted 10 lines · Christian Fetzer

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.

I think I can explain it. In piix4_setup_sb800() you touch the SB800_PIIX4_SMB_IDX port without first taking the mutex that protects it. You only take the mutex on transactions (in piix4_access_sb800) not during initialization. If self-probing I2C device drivers such as jc42 are already loaded before you load i2c-piix4, then as soon as the first SMBus port is registered, i2c-core will try to attach I2C devices to it, while at the same time i2c-piix4 is registering the second SMBus port. So SB800_PIIX4_SMB_IDX is changed while piix4_access_sb800 accesses it and chaos happens.

Quoted 5 lines
Quoted 2 lines

I think if we had proper locking in piix4_setup_sb800() then you should be able to load jc42 first and then i2c-piix4 and it should work fine.

Since the problem remains even after your patch, I'll try to provide more information about the issue.

OK, I think I see what's going on here.

The i2c buses begin to exist for the kernel when i2c_add_adapter() is called. This happens in piix4_add_adapter(), which is called from two sites: piix4_probe() directly for the legacy devices and for the aux SMBus controller, and piix4_add_adapters_sb800() for the muxed main controller of the SB800.

Now if you look at piix4_add_adapters_sb800(), you can see that right _after_ calling piix4_add_adapter(), the code grabs the adapter-specific data (struct i2c_piix4_adapdata *adapdata) and sets two fields there: sb800_main and port. While sb800_main isn't needed before module removal in piix4_adap_remove(), port is used in piix4_access_sb800(), which can be called immediately after i2c_add_adapter() returns (possibly even slightly before that.)

So we have a small window during which piix4_access_sb800() can be called and adapdata->port has not been set yet, so it still has its default value of 0 for all ports. Hence the kernel reads from port 0 for all of SMBus channels 0, 2, 3 and 4. This completely explains the symptoms you described.

Additionally I see that the algorithm is overwritten also right after piix4_add_adapter() is called. So odds are that piix4_access_sb800() isn't even called when the already loaded jc42 or w83795 driver probes for devices. Instead piix4_access() is called, so no locking and no port selection takes place at all.

The only right way to handle this is to set all fields of the i2c_adapter _and_ its associated struct i2c_piix4_adapdata _before_ i2c_add_adapter() is called. Anything else is just racy by design.

I'll post a patch for you to try shortly.

Signature · 2 lines
Jean Delvare
SUSE L3 Support
author
#11 lore raw

Hi Jean,

Quoted 62 lines

Am 25.01.2016 um 12:13 schrieb Jean Delvare <[email protected]>:

Hi Christian,

Quoted 23 lines · fetzerch
Quoted 22 lines · Jean Delvare
Quoted 10 lines · Christian Fetzer

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.

I think I can explain it. In piix4_setup_sb800() you touch the SB800_PIIX4_SMB_IDX port without first taking the mutex that protects it. You only take the mutex on transactions (in piix4_access_sb800) not during initialization. If self-probing I2C device drivers such as jc42 are already loaded before you load i2c-piix4, then as soon as the first SMBus port is registered, i2c-core will try to attach I2C devices to it, while at the same time i2c-piix4 is registering the second SMBus port. So SB800_PIIX4_SMB_IDX is changed while piix4_access_sb800 accesses it and chaos happens.

Quoted 5 lines
Quoted 2 lines

I think if we had proper locking in piix4_setup_sb800() then you should be able to load jc42 first and then i2c-piix4 and it should work fine.

Since the problem remains even after your patch, I'll try to provide more information about the issue.

OK, I think I see what's going on here.

The i2c buses begin to exist for the kernel when i2c_add_adapter() is called. This happens in piix4_add_adapter(), which is called from two sites: piix4_probe() directly for the legacy devices and for the aux SMBus controller, and piix4_add_adapters_sb800() for the muxed main controller of the SB800.

Now if you look at piix4_add_adapters_sb800(), you can see that right _after_ calling piix4_add_adapter(), the code grabs the adapter-specific data (struct i2c_piix4_adapdata *adapdata) and sets two fields there: sb800_main and port. While sb800_main isn't needed before module removal in piix4_adap_remove(), port is used in piix4_access_sb800(), which can be called immediately after i2c_add_adapter() returns (possibly even slightly before that.)

So we have a small window during which piix4_access_sb800() can be called and adapdata->port has not been set yet, so it still has its default value of 0 for all ports. Hence the kernel reads from port 0 for all of SMBus channels 0, 2, 3 and 4. This completely explains the symptoms you described.

Additionally I see that the algorithm is overwritten also right after piix4_add_adapter() is called. So odds are that piix4_access_sb800() isn't even called when the already loaded jc42 or w83795 driver probes for devices. Instead piix4_access() is called, so no locking and no port selection takes place at all.

Quoted 9 lines

The only right way to handle this is to set all fields of the i2c_adapter _and_ its associated struct i2c_piix4_adapdata _before_ i2c_add_adapter() is called. Anything else is just racy by design.

I'll post a patch for you to try shortly.

Signature · 2 lines
Jean Delvare
SUSE L3 Support

Thanks for the explanation, I’ve successfully tested your change and can confirm that it fixes the problem.
I’ve loaded i2c_piix4, jc42 and w83795 in all possible permutations and sensors is now able to correctly access the sensor values independently from the module load order.

Cheers,
Christian