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

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

[Andrew Cooks]

Family 16h Model 30h 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")

commit 6befa3fde65fe437f588da490c07a114393ce229 ("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

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

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 462948e..89692f4 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
@@ -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);
-- 
2.7.4

[PATCH 2/3] i2c: piix4: fix number of ports on Family 16h Model 30h

[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 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 89692f4..9763241 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
 
@@ -800,6 +807,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,
@@ -856,10 +864,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]);
@@ -872,7 +887,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]);
@@ -993,12 +1008,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

Re: [2/3] i2c: piix4: fix number of ports on Family 16h Model 30h

[Guenter Roeck]

On Mon, Jan 29, 2018 at 01:54:19PM +1000, Andrew Cooks wrote:
> 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 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 89692f4..9763241 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)
> + */

This is a bit misleading. Which adapter has only one port ?

> +#define SB800_MAIN_PORTS 4
> +#define HUDSON2_MAIN_PORTS 2 /* HUDSON2, reserves Port 3 and Port 4 */
> +
A tab between define and value might be helpful. 
Not sure though why both PIIX4_MAX_ADAPTERS and SB800_MAIN_PORTS
are needed.

>  /* SB800 constants */
>  #define SB800_PIIX4_SMB_IDX		0xcd6
>  
> @@ -800,6 +807,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;

This variable is never set, only decreased.

>  
>  static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
>  			     bool sb800_main, u8 port, bool notify_imc,
> @@ -856,10 +864,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]);
> @@ -872,7 +887,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) {

This is wrong, even if piix4_adapter_count was initialized.

>  		adapdata = i2c_get_adapdata(piix4_main_adapters[port]);
>  		if (adapdata->smba) {
>  			i2c_del_adapter(piix4_main_adapters[port]);
> @@ -993,12 +1008,10 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
>  
>  static void piix4_remove(struct pci_dev *dev)
>  {
> -	int port = PIIX4_MAX_ADAPTERS;

A one line change would do it just as well.
	int port = piix4_adapter_count;

> -
> -	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;
>  		}
>  	}
>  

Re: [2/3] i2c: piix4: fix number of ports on Family 16h Model 30h

[Andrew Cooks]

Hi Guenter

Thanks for your review!

On 29/01/18 15:58, Guenter Roeck wrote:
> On Mon, Jan 29, 2018 at 01:54:19PM +1000, Andrew Cooks wrote:
>> 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 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 89692f4..9763241 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)
>> + */
> 
> This is a bit misleading. Which adapter has only one port ?
The main adapter has at least one port (Port 0), but may have up to four in total, depending on the chip version. Ports are labeled 0, 2, 3, 4 (with 1 being reserved).
The aux adapter has only one port and is labeled "port 1" in this driver, but not in the AMD documentation.

If the comment isn't helpful, I think I'll just remove it.

> 
>> +#define SB800_MAIN_PORTS 4
>> +#define HUDSON2_MAIN_PORTS 2 /* HUDSON2, reserves Port 3 and Port 4 */
>> +
> A tab between define and value might be helpful. 

will fix.

> Not sure though why both PIIX4_MAX_ADAPTERS and SB800_MAIN_PORTS
> are needed.
PIIX4_MAX_ADAPTERS keeps the max array length.
SB800_MAIN_PORTS maintains the existing behavior for the many chips previous chips. It would be a big job to audit all of them.

Their meaning is slightly different, but I think I'll just drop SB800_MAIN_PORTS and reuse PIIX4_MAX_ADAPTERS. I guess more invasive refactoring to remove the fixed size array is also possible.

> 
>>  /* SB800 constants */
>>  #define SB800_PIIX4_SMB_IDX		0xcd6
>>  
>> @@ -800,6 +807,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;
> 
> This variable is never set, only decreased.
Thanks, will fix.

> 
>>  
>>  static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
>>  			     bool sb800_main, u8 port, bool notify_imc,
>> @@ -856,10 +864,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]);
>> @@ -872,7 +887,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) {
> 
> This is wrong, even if piix4_adapter_count was initialized.
Yes, how careless of me. Will fix.

> 
>>  		adapdata = i2c_get_adapdata(piix4_main_adapters[port]);
>>  		if (adapdata->smba) {
>>  			i2c_del_adapter(piix4_main_adapters[port]);
>> @@ -993,12 +1008,10 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
>>  
>>  static void piix4_remove(struct pci_dev *dev)
>>  {
>> -	int port = PIIX4_MAX_ADAPTERS;
> 
> A one line change would do it just as well.
> 	int port = piix4_adapter_count;
Will fix.
> 
>> -
>> -	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;
>>  		}
>>  	}
>>  
Thanks again for the review and feedback!

a.

[PATCH 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 9763241..97eb6ae 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -843,6 +843,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 3/3] i2c: add ACPI support for i2c-piix4

[Andy Shevchenko]

On Mon, Jan 29, 2018 at 5:54 AM, Andrew Cooks <andrew.cooks@opengear.com> wrote:
> 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

> +       if (has_acpi_companion(&dev->dev)) {
> +               acpi_preset_companion(&adap->dev,
> +                                     ACPI_COMPANION(&dev->dev),
> +                                     piix4_adapter_count++);
> +       }

Wouldn't be enough just following:

     ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&dev->dev));

?

Or if this increment is needed:

struct acpi_device *adev = ACPI_COMPANION(&dev->dev);

if (adev) {
    ACPI_COMPANION_SET(&adap->dev, adev);
    piix4_adapter_count++);
}

-- 
With Best Regards,
Andy Shevchenko

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

[Andrew Cooks]

Hi Andy

Thanks for the review!

On 29/01/18 23:30, Andy Shevchenko wrote:
> On Mon, Jan 29, 2018 at 5:54 AM, Andrew Cooks <andrew.cooks@opengear.com> wrote:
>> 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
> 
>> +       if (has_acpi_companion(&dev->dev)) {
>> +               acpi_preset_companion(&adap->dev,
>> +                                     ACPI_COMPANION(&dev->dev),
>> +                                     piix4_adapter_count++);
>> +       }
> 
> Wouldn't be enough just following:
> 
>      ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&dev->dev));

That seems much cleaner, but it doesn't seem to do what I need (my devices don't show up). I'm still investigating.

> 
> ?
> 
> Or if this increment is needed:
> 
> struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
> 
> if (adev) {
>     ACPI_COMPANION_SET(&adap->dev, adev);
>     piix4_adapter_count++);
> }
> 

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

[Andrew Cooks]

Hi Andy

On 29/01/18 23:30, Andy Shevchenko wrote:
> On Mon, Jan 29, 2018 at 5:54 AM, Andrew Cooks <andrew.cooks@opengear.com> wrote:
>> 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
> 
>> +       if (has_acpi_companion(&dev->dev)) {
>> +               acpi_preset_companion(&adap->dev,
>> +                                     ACPI_COMPANION(&dev->dev),
>> +                                     piix4_adapter_count++);
>> +       }
> 
> Wouldn't be enough just following:
> 
>      ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&dev->dev));

In the ACPI DSDT, I've modeled the topology as follows:

Device(SBUS) {
        Name(_ADR, 0x00140000)
        Name(_HID, "SMB0001") // SMBus CMI spec uses (invalid hid) "SMBUS01"

        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"))
        }
}

That is why each piix4 adapter matches a CH0? device that is descendant from the SBUS device in ACPI.

It is similar to the topology shown in commit 8eb5c87a ("i2c: add ACPI support for I2C mux ports")

> 
> ?
> 
> Or if this increment is needed:
> 
> struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
> 
> if (adev) {
>     ACPI_COMPANION_SET(&adap->dev, adev);
>     piix4_adapter_count++);
> }
> 

Thanks!

a.

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

[Andy Shevchenko]

On Tue, Jan 30, 2018 at 3:43 AM, Andrew Cooks <andrew.cooks@opengear.com> wrote:
> On 29/01/18 23:30, Andy Shevchenko wrote:
>> On Mon, Jan 29, 2018 at 5:54 AM, Andrew Cooks <andrew.cooks@opengear.com> wrote:
>>> 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
>>
>>> +       if (has_acpi_companion(&dev->dev)) {
>>> +               acpi_preset_companion(&adap->dev,
>>> +                                     ACPI_COMPANION(&dev->dev),
>>> +                                     piix4_adapter_count++);
>>> +       }
>>
>> Wouldn't be enough just following:
>>
>>      ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&dev->dev));
>
> In the ACPI DSDT, I've modeled the topology as follows:
>
> Device(SBUS) {
>         Name(_ADR, 0x00140000)
>         Name(_HID, "SMB0001") // SMBus CMI spec uses (invalid hid) "SMBUS01"
>
>         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"))
>         }
> }
>
> That is why each piix4 adapter matches a CH0? device that is descendant from the SBUS device in ACPI.
>
> It is similar to the topology shown in commit 8eb5c87a ("i2c: add ACPI support for I2C mux ports")

Ah, I see now. Your initial approach seems fine.

Perhaps even new helper can be made for users (currently i2c mux,
sdio, libata) in the future.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Jan 30, 2018 at 8:22 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Jan 30, 2018 at 3:43 AM, Andrew Cooks <andrew.cooks@opengear.com> wrote:
>> On 29/01/18 23:30, Andy Shevchenko wrote:
>>> On Mon, Jan 29, 2018 at 5:54 AM, Andrew Cooks <andrew.cooks@opengear.com> wrote:

>>>> +       if (has_acpi_companion(&dev->dev)) {
>>>> +               acpi_preset_companion(&adap->dev,
>>>> +                                     ACPI_COMPANION(&dev->dev),
>>>> +                                     piix4_adapter_count++);
>>>> +       }

>> It is similar to the topology shown in commit 8eb5c87a ("i2c: add ACPI support for I2C mux ports")
>
> Ah, I see now. Your initial approach seems fine.
>
> Perhaps even new helper can be made for users (currently i2c mux,
> sdio, libata) in the future.

FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko