[PATCH v2 0/3] Enable ACPI-defined peripherals on i2c-piix4 SMBus

[PATCH v2 0/3] Enable ACPI-defined peripherals on i2c-piix4 SMBus

[Andrew Cooks]

These patches 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.

v2:
  count the adapters, instead of misusing port numbers

Andrew Cooks (3):
  i2c: piix4: Fix SMBus port selection for AMD Family 16h chips
  i2c: piix4: fix number of SMBus ports on HUDSON2
  i2c: add ACPI support for i2c-piix4

 drivers/i2c/busses/i2c-piix4.c | 41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] i2c: piix4: Fix SMBus port selection for AMD Family 16h chips

[Andrew Cooks]

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

The SMBus port selection register is common to multiple 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 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

Signed-off-by: Andrew Cooks <andrew.cooks@opengear.com>
---
 drivers/i2c/busses/i2c-piix4.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 462948e..b92ea5d 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -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
@@ -360,12 +360,12 @@ 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_HUDSON2_SMBUS:
 		case PCI_DEVICE_ID_AMD_KERNCZ_SMBUS:
 			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:
 			piix4_port_sel_sb800 = SB800_PIIX4_PORT_IDX_ALT;
 			piix4_port_mask_sb800 = SB800_PIIX4_PORT_IDX_MASK;
-- 
2.7.4

[PATCH v2 1/3] i2c: piix4: Fix SMBus port selection for AMD Family 16h chips

[Andrew Cooks]

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

The SMBus port selection register is common to multiple 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 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

Signed-off-by: Andrew Cooks <andrew.cooks@opengear.com>
---
 drivers/i2c/busses/i2c-piix4.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 462948e..b92ea5d 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -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
@@ -360,12 +360,12 @@ 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_HUDSON2_SMBUS:
 		case PCI_DEVICE_ID_AMD_KERNCZ_SMBUS:
 			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:
 			piix4_port_sel_sb800 = SB800_PIIX4_PORT_IDX_ALT;
 			piix4_port_mask_sb800 = SB800_PIIX4_PORT_IDX_MASK;
-- 
2.7.4

[PATCH v2 2/3] i2c: piix4: fix number of SMBus ports on HUDSON2

[Andrew Cooks]

This prevents bus timeouts and resets on Hudson2 (Family 16h Models
30h), when probing reserved Port 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 Models 30h-3Fh,

Signed-off-by: Andrew Cooks <andrew.cooks@opengear.com>
---
 drivers/i2c/busses/i2c-piix4.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index b92ea5d..cc38dc3 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -82,6 +82,13 @@
 /* Multi-port constants */
 #define PIIX4_MAX_ADAPTERS 4
 
+/*
+ * Main adapter port count. At least one (Port 0) plus up to 3 additional
+ * (Ports 2-4)
+ */
+#define SB800_MAIN_PORTS 4
+#define HUDSON2_MAIN_PORTS 2 /* HUDSON2, reserves Port 3 and Port 4 */
+
 /* SB800 constants */
 #define SB800_PIIX4_SMB_IDX		0xcd6
 
@@ -802,6 +809,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,
@@ -858,10 +866,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 = SB800_MAIN_PORTS;
+	}
+
+	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]);
@@ -874,7 +889,7 @@ static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba,
 error:
 	dev_err(&dev->dev,
 		"Error setting up SB800 adapters. Unregistering!\n");
-	while (--port >= 0) {
+	while (--piix4_adapter_count >= 0) {
 		adapdata = i2c_get_adapdata(piix4_main_adapters[port]);
 		if (adapdata->smba) {
 			i2c_del_adapter(piix4_main_adapters[port]);
@@ -995,12 +1010,10 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
 
 static void piix4_remove(struct pci_dev *dev)
 {
-	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;
+	while (--piix4_adapter_count >= 0) {
+		if (piix4_main_adapters[piix4_adapter_count]) {
+			piix4_adap_remove(piix4_main_adapters[piix4_adapter_count]);
+			piix4_main_adapters[piix4_adapter_count] = NULL;
 		}
 	}
 
-- 
2.7.4

[PATCH v2 2/3] i2c: piix4: fix number of SMBus ports on HUDSON2

[Andrew Cooks]

This prevents bus timeouts and resets on Hudson2 (Family 16h Models
30h), when probing reserved Port 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 Models 30h-3Fh,

Signed-off-by: Andrew Cooks <andrew.cooks@opengear.com>
---
 drivers/i2c/busses/i2c-piix4.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index b92ea5d..cc38dc3 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -82,6 +82,13 @@
 /* Multi-port constants */
 #define PIIX4_MAX_ADAPTERS 4
 
+/*
+ * Main adapter port count. At least one (Port 0) plus up to 3 additional
+ * (Ports 2-4)
+ */
+#define SB800_MAIN_PORTS 4
+#define HUDSON2_MAIN_PORTS 2 /* HUDSON2, reserves Port 3 and Port 4 */
+
 /* SB800 constants */
 #define SB800_PIIX4_SMB_IDX		0xcd6
 
@@ -802,6 +809,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,
@@ -858,10 +866,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 = SB800_MAIN_PORTS;
+	}
+
+	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]);
@@ -874,7 +889,7 @@ static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba,
 error:
 	dev_err(&dev->dev,
 		"Error setting up SB800 adapters. Unregistering!\n");
-	while (--port >= 0) {
+	while (--piix4_adapter_count >= 0) {
 		adapdata = i2c_get_adapdata(piix4_main_adapters[port]);
 		if (adapdata->smba) {
 			i2c_del_adapter(piix4_main_adapters[port]);
@@ -995,12 +1010,10 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
 
 static void piix4_remove(struct pci_dev *dev)
 {
-	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;
+	while (--piix4_adapter_count >= 0) {
+		if (piix4_main_adapters[piix4_adapter_count]) {
+			piix4_adap_remove(piix4_main_adapters[piix4_adapter_count]);
+			piix4_main_adapters[piix4_adapter_count] = NULL;
 		}
 	}
 
-- 
2.7.4

[PATCH v2 3/3] i2c: add ACPI support for i2c-piix4

[Andrew Cooks]

This enables 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.

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

Signed-off-by: Andrew Cooks <andrew.cooks@opengear.com>
---
 drivers/i2c/busses/i2c-piix4.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index cc38dc3..65e6390 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -845,6 +845,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);
 
-- 
2.7.4

[PATCH v2 3/3] i2c: add ACPI support for i2c-piix4

[Andrew Cooks]

This enables 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.

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

Signed-off-by: Andrew Cooks <andrew.cooks@opengear.com>
---
 drivers/i2c/busses/i2c-piix4.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index cc38dc3..65e6390 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -845,6 +845,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);
 
-- 
2.7.4

Re: [PATCH v2 1/3] i2c: piix4: Fix SMBus port selection for AMD Family 16h chips

[Andrew Cooks]

Hi Jean

On 14/12/17 13:11, Andrew Cooks wrote:
> HUDSON2 SMBus controller has the same port selection register as
> described and fixed in commit 0fe16195f89173652cf111d7b384941b00c5aabd
> ("i2c: piix4: Fix SMBus port selection for AMD Family 17h chips")
> 
> The SMBus port selection register is common to multiple 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 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

I just noticed that commit 6befa3fde65fe437f588da490c07a114393ce229 ("i2c: piix4: Support alternative port selection register") also fixed the port selection for Hudson2, and this patch is incorrect. Unfortunately the AMD naming and PCI Device IDs aren't particularly helpful here.

In both the 51192 Bolton Register Reference Guide[1] and 52740 BKDG for Family 16h Model 30h-3Fh[2], the PCI Device ID for the SMBus controller is 0x780b, but the location of the SmBus0Sel port selection bits are different.

We might be able to distinguish between them using the Revision ID in PCI register 0x08 (in addition to the PCI Device ID, of course).

  Bolton [1] is Revision 0x15
  Family 16h Model 30h (Hudson2?)[2] is Revision 0x1F
  Family 15h Model 70h [3] uses 0x4A
  Family 15h Model 60h [4] uses 0x4A

Thoughts?


Andrew


[1] http://support.amd.com/TechDocs/51192_Bolton_FCH_RRG.pdf
[2] http://support.amd.com/TechDocs/52740_16h_Models_30h-3Fh_BKDG.pdf
[3] http://support.amd.com/TechDocs/55072_AMD_Family_15h_Models_70h-7Fh_BKDG.pdf
[4] http://support.amd.com/TechDocs/50742_15h_Models_60h-6Fh_BKDG.pdf

[PATCH v3 0/3] Enable ACPI-defined peripherals on i2c-piix4 SMBus

[Andrew Cooks]

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:
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 SMBus port selection for AMD Family 16h chips
  i2c: piix4: fix number of SMBus ports on HUDSON2
  i2c: add ACPI support for i2c-piix4

 drivers/i2c/busses/i2c-piix4.c | 41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

-- 
2.7.4