Skip to contentSkip to conversation

Enable ACPI-defined peripherals on i2c-piix4 SMBus

PATCH Superseded Andrew Cooks 1 participant 5 messages Jan 30, 2018 – Feb 19, 2018

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

v4 cover Enable ACPI-defined peripherals on i2c-piix4 SMBus

These patches fix a couple of issues with the i2c-piix4 driver on AMD Family 16h Model 30h SoCs and add ACPI-based enumeration to the i2c-piix4 driver.

Some I2C peripherals, eg. PCA953x IO expander, are not discovered by the probe or detect mechanisms when attached to an SMBus controller that uses the i2c-piix4 SMBus driver.

ACPI provides a mechanism to define these peripherals and the controller port that they're attached to.

Changes:
v4:
remove unnecessary SB800_MAIN_PORTS constant
reduce piix4_remove change
v3:
take chip revision into account when determining port selection register
v2:
count the adapters, instead of misusing port numbers

Andrew Cooks (3):
i2c: piix4: Fix port selection for AMD Family 16h Model 30h
i2c: piix4: fix probing of reserved ports on AMD
i2c: piix4: add ACPI support

 drivers/i2c/busses/i2c-piix4.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)
Signature · 1 line
2.7.4
#1 lore raw

PATCH v4 1/3 i2c: piix4: Fix port selection for AMD Family 16h Model 30h

Family 16h Model 30h SMBus controller has the same port selection register as described and fixed in commit 0fe16195f891 ("i2c: piix4: Fix SMBus port selection for AMD Family 17h chips")

commit 6befa3fde65fe ("i2c: piix4: Support alternative port selection register") also fixed the port selection for Hudson2. Unfortunately the AMD naming and PCI Device IDs aren't particularly helpful here.

The SMBus port selection register is common to the following Families and models, as documented in AMD's publicly available BIOS and Kernel Developer Guides:

50742 - Family 15h Model 60h-6Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) 55072 - Family 15h Model 70h-7Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) 52740 - Family 16h Model 30h-3Fh (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS)

The Hudson2 PCI Device ID (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS) is shared between Bolton FCH and Family 16h Model 30h, but the location of the SmBus0Sel port selection bits are different:

51192 - Bolton Register Reference Guide

We distinguish between Bolton and Family 16h Model 30h using the PCI Revision ID:

Bolton is device 0x780b, revision 0x15
Family 16h Model 30h is device 0x780b, revision 0x1F Family 15h Model 60h and 70h are both device 0x790b, revision 0x4A.

The following additional public AMD BKDG documents were checked and do not share the same port selection register:

42301 - Family 15h Model 00h-0Fh doesn't mention any 42300 - Family 15h Model 10h-1Fh doesn't mention any 49125 - Family 15h Model 30h-3Fh doesn't mention any

48751 - Family 16h Model 00h-0Fh uses the previously supported

         index register SB800_PIIX4_PORT_IDX_ALT at 0x2e
---
 drivers/i2c/busses/i2c-piix4.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
drivers/i2c/busses/i2c-piix4.c +5 -7
index 174579d..5c90a44 100644
@@ -99,7 +99,7 @@
 #define SB800_PIIX4_PORT_IDX_MASK	0x06
 #define SB800_PIIX4_PORT_IDX_SHIFT	1
 
-/* On kerncz, SmBus0Sel is at bit 20:19 of PMx00 DecodeEn */
+/* On kerncz and Hudson2, SmBus0Sel is at bit 20:19 of PMx00 DecodeEn */
 #define SB800_PIIX4_PORT_IDX_KERNCZ		0x02
 #define SB800_PIIX4_PORT_IDX_MASK_KERNCZ	0x18
 #define SB800_PIIX4_PORT_IDX_SHIFT_KERNCZ	3
@@ -359,18 +359,16 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 
 	/* Find which register is used for port selection */
 	if (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD) {
-		switch (PIIX4_dev->device) {
-		case PCI_DEVICE_ID_AMD_KERNCZ_SMBUS:
+		if ((PIIX4_dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) ||
+		    (PIIX4_dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS &&
+			   PIIX4_dev->revision >= 0x1F)) {
 			piix4_port_sel_sb800 = SB800_PIIX4_PORT_IDX_KERNCZ;
 			piix4_port_mask_sb800 = SB800_PIIX4_PORT_IDX_MASK_KERNCZ;
 			piix4_port_shift_sb800 = SB800_PIIX4_PORT_IDX_SHIFT_KERNCZ;
-			break;
-		case PCI_DEVICE_ID_AMD_HUDSON2_SMBUS:
-		default:
+		} else {
 			piix4_port_sel_sb800 = SB800_PIIX4_PORT_IDX_ALT;
 			piix4_port_mask_sb800 = SB800_PIIX4_PORT_IDX_MASK;
 			piix4_port_shift_sb800 = SB800_PIIX4_PORT_IDX_SHIFT;
-			break;
 		}
 	} else {
 		mutex_lock(&piix4_mutex_sb800);
Signature · 1 line
2.7.4
#2 lore raw

Re: [PATCH v4 1/3] i2c: piix4: Fix port selection for AMD Family 16h Model 30h

Any comments?

Quoted 81 lines · Andrew Cooks

Family 16h Model 30h SMBus controller has the same port selection register as described and fixed in commit 0fe16195f891 ("i2c: piix4: Fix SMBus port selection for AMD Family 17h chips")

commit 6befa3fde65fe ("i2c: piix4: Support alternative port selection register") also fixed the port selection for Hudson2. Unfortunately the AMD naming and PCI Device IDs aren't particularly helpful here.

The SMBus port selection register is common to the following Families and models, as documented in AMD's publicly available BIOS and Kernel Developer Guides:

50742 - Family 15h Model 60h-6Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) 55072 - Family 15h Model 70h-7Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) 52740 - Family 16h Model 30h-3Fh (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS)

The Hudson2 PCI Device ID (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS) is shared between Bolton FCH and Family 16h Model 30h, but the location of the SmBus0Sel port selection bits are different:

51192 - Bolton Register Reference Guide

We distinguish between Bolton and Family 16h Model 30h using the PCI Revision ID:

Bolton is device 0x780b, revision 0x15
Family 16h Model 30h is device 0x780b, revision 0x1F Family 15h Model 60h and 70h are both device 0x790b, revision 0x4A.

The following additional public AMD BKDG documents were checked and do not share the same port selection register:

42301 - Family 15h Model 00h-0Fh doesn't mention any 42300 - Family 15h Model 10h-1Fh doesn't mention any 49125 - Family 15h Model 30h-3Fh doesn't mention any

48751 - Family 16h Model 00h-0Fh uses the previously supported

         index register SB800_PIIX4_PORT_IDX_ALT at 0x2e
---
 drivers/i2c/busses/i2c-piix4.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
drivers/i2c/busses/i2c-piix4.c +5 -7
index 174579d..5c90a44 100644
@@ -99,7 +99,7 @@
 #define SB800_PIIX4_PORT_IDX_MASK	0x06
 #define SB800_PIIX4_PORT_IDX_SHIFT	1
 
-/* On kerncz, SmBus0Sel is at bit 20:19 of PMx00 DecodeEn */
+/* On kerncz and Hudson2, SmBus0Sel is at bit 20:19 of PMx00 DecodeEn */
 #define SB800_PIIX4_PORT_IDX_KERNCZ		0x02
 #define SB800_PIIX4_PORT_IDX_MASK_KERNCZ	0x18
 #define SB800_PIIX4_PORT_IDX_SHIFT_KERNCZ	3
@@ -359,18 +359,16 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 
 	/* Find which register is used for port selection */
 	if (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD) {
-		switch (PIIX4_dev->device) {
-		case PCI_DEVICE_ID_AMD_KERNCZ_SMBUS:
+		if ((PIIX4_dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) ||
+		    (PIIX4_dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS &&
+			   PIIX4_dev->revision >= 0x1F)) {
 			piix4_port_sel_sb800 = SB800_PIIX4_PORT_IDX_KERNCZ;
 			piix4_port_mask_sb800 = SB800_PIIX4_PORT_IDX_MASK_KERNCZ;
 			piix4_port_shift_sb800 = SB800_PIIX4_PORT_IDX_SHIFT_KERNCZ;
-			break;
-		case PCI_DEVICE_ID_AMD_HUDSON2_SMBUS:
-		default:
+		} else {
 			piix4_port_sel_sb800 = SB800_PIIX4_PORT_IDX_ALT;
 			piix4_port_mask_sb800 = SB800_PIIX4_PORT_IDX_MASK;
 			piix4_port_shift_sb800 = SB800_PIIX4_PORT_IDX_SHIFT;
-			break;
 		}
 	} else {
 		mutex_lock(&piix4_mutex_sb800);
#3 lore raw

PATCH v4 2/3 i2c: piix4: fix probing of reserved ports on AMD

In reply to Andrew Cooks

Prevent bus timeouts and resets on Family 16h Model 30h), by not probing reserved Ports 3 and 4.

According to the AMD BIOS and Kernel Developer's Guides (BKDG), Port 3 and Port 4 are reserved on the following devices:
- Family 15h Model 60h-6Fh,
- Family 15h Model 70h-7Fh,
- Family 16h Model 30h-3Fh,

---
 drivers/i2c/busses/i2c-piix4.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-piix4.c +15 -4
index 5c90a44..01f1610 100644
@@ -80,7 +80,8 @@
 #define PIIX4_BLOCK_DATA	0x14
 
 /* Multi-port constants */
-#define PIIX4_MAX_ADAPTERS 4
+#define PIIX4_MAX_ADAPTERS	4
+#define HUDSON2_MAIN_PORTS	2 /* HUDSON2, KERNCZ reserves ports 3, 4 */
 
 /* SB800 constants */
 #define SB800_PIIX4_SMB_IDX		0xcd6
@@ -800,6 +801,7 @@ MODULE_DEVICE_TABLE (pci, piix4_ids);
 
 static struct i2c_adapter *piix4_main_adapters[PIIX4_MAX_ADAPTERS];
 static struct i2c_adapter *piix4_aux_adapter;
+static int piix4_adapter_count;
 
 static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 			     bool sb800_main, u8 port, bool notify_imc,
@@ -849,6 +851,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 	}
 
 	*padap = adap;
+	piix4_adapter_count++;
 	return 0;
 }
 
@@ -856,10 +859,17 @@ static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba,
 				    bool notify_imc)
 {
 	struct i2c_piix4_adapdata *adapdata;
-	int port;
+	int port, port_count;
 	int retval;
 
-	for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
+	if (dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS ||
+	    dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) {
+		port_count = HUDSON2_MAIN_PORTS;
+	} else {
+		port_count = PIIX4_MAX_ADAPTERS;
+	}
+
+	for (port = 0; port < port_count; port++) {
 		retval = piix4_add_adapter(dev, smba, true, port, notify_imc,
 					   piix4_main_port_names_sb800[port],
 					   &piix4_main_adapters[port]);
@@ -889,6 +899,7 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int retval;
 	bool is_sb800 = false;
+	piix4_adapter_count = 0;
 
 	if ((dev->vendor == PCI_VENDOR_ID_ATI &&
 	     dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
@@ -993,7 +1004,7 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
 
 static void piix4_remove(struct pci_dev *dev)
 {
-	int port = PIIX4_MAX_ADAPTERS;
+	int port = piix4_adapter_count;
 
 	while (--port >= 0) {
 		if (piix4_main_adapters[port]) {
Signature · 1 line
2.7.4
#4 lore raw

PATCH v4 3/3 i2c: piix4: add ACPI support

In reply to Andrew Cooks

Enable the i2c-piix4 SMBus controller driver to enumerate I2C slave devices using ACPI. It builds on the related I2C mux device work in commit 8eb5c87a92c0 ("i2c: add ACPI support for I2C mux ports")

In the i2c-piix4 driver the adapters are enumerated as:
Main SMBus adapter Port 0, Port 2, ..., aux port (i.e., ASF adapter)

However, in the AMD BKDG documentation[1], the implied order of ports is:
Main SMBus adapter Port 0, ASF adapter, Port 2, Port 3, ...

This ordering difference is unfortunate, and we assume that ACPI developers will use the Linux ordering, as follows:

Device(SBUS) {
        Name(_ADR, 0x00140000)
        Name(_HID, "SMB0001")

        Device(CH00) {
                Name(_ADR, 0) // main SMBus adapter, Port 0
                Name (_STR, Unicode ("Main SMBus adapter, Port 0"))
        }

        Device(CH01) {
                Name(_ADR, 1) // main SMBus Port 2 (Port 1 label is undefined)
                Name (_STR, Unicode ("Main SMBus adapter, Port 2"))
        }

        Device (CH02) {
                Name(_ADR, 2) // ASF adapter (what Linux calls 'aux port')
                Name (_STR, Unicode ("AUX / ASF SMBus adapter"))
        }
}

[1] 52740 BIOS and Kernel Developer's Guide (BKDG) for AMD Family 16h Models 30h-3Fh Processors

---
 drivers/i2c/busses/i2c-piix4.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/i2c/busses/i2c-piix4.c +6 -0
index 01f1610..9a6cdc8 100644
@@ -837,6 +837,12 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 	/* set up the sysfs linkage to our parent device */
 	adap->dev.parent = &dev->dev;
 
+	if (has_acpi_companion(&dev->dev)) {
+		acpi_preset_companion(&adap->dev,
+				ACPI_COMPANION(&dev->dev),
+				piix4_adapter_count);
+	}
+
 	snprintf(adap->name, sizeof(adap->name),
 		"SMBus PIIX4 adapter%s at %04x", name, smba);
 
Signature · 1 line
2.7.4