[PATCH 1/2] i2c: add ACPI support for i2c-piix4

[PATCH 1/2] 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")

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 174579d..9260cfa 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -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),
+				      port);
+	}
+
 	snprintf(adap->name, sizeof(adap->name),
 		"SMBus PIIX4 adapter%s at %04x", name, smba);
 
-- 
2.7.4

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

[Jean Delvare]

Hi Andrew,

On Thu, 23 Nov 2017 13:09:37 +1000, Andrew Cooks 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")
> 
> 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 174579d..9260cfa 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -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),
> +				      port);
> +	}
> +
>  	snprintf(adap->name, sizeof(adap->name),
>  		"SMBus PIIX4 adapter%s at %04x", name, smba);
>  

Looks good to me but I think you have the patches in the wrong order.
First we must get the port number right, and then you can instantiate
the devices from the ACPI data. If you do it the other way around then
you have a transient situation where you instantiate a device where it
does not exist.

-- 
Jean Delvare
SUSE L3 Support

[PATCH 2/2] i2c: fix piix4 aux port number

[Andrew Cooks]

Let the aux port use port number one (not zero), to match the AMD
documentation and enable mapping ACPI _ADR to port number.

This fixes ACPI-based enumeration of I2C slave peripherals that are
defined for the aux SMBus port.

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

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 9260cfa..f980f0b 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -975,7 +975,7 @@ 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, false, 0, false,
+		piix4_add_adapter(dev, retval, false, 1, false,
 				  is_sb800 ? piix4_aux_port_name_sb800 : "",
 				  &piix4_aux_adapter);
 	}
-- 
2.7.4

Re: [PATCH 2/2] i2c: fix piix4 aux port number

[Jean Delvare]

Hi Andrew,

On Thu, 23 Nov 2017 13:09:38 +1000, Andrew Cooks wrote:
> Let the aux port use port number one (not zero), to match the AMD
> documentation and enable mapping ACPI _ADR to port number.
> 
> This fixes ACPI-based enumeration of I2C slave peripherals that are
> defined for the aux SMBus port.
> 
> Signed-off-by: Andrew Cooks <andrew.cooks@opengear.com>
> ---
>  drivers/i2c/busses/i2c-piix4.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 9260cfa..f980f0b 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -975,7 +975,7 @@ 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, false, 0, false,
> +		piix4_add_adapter(dev, retval, false, 1, false,
>  				  is_sb800 ? piix4_aux_port_name_sb800 : "",
>  				  &piix4_aux_adapter);
>  	}

The port number has consequences. In piix4_adap_remove, port 0 is
handled differently. We assume that for each controller (main or aux)
exactly one adapter has port number 0. Your change above breaks this
assumption.

Also, if the port numbers are supposed to match the documentation, and
the aux controller is port 1, then I wonder how are the muxed ports of
the main controller numbered? The driver numbers them from 1 to 3, but
I guess the documentation numbers them from 2 to 4 to avoid colliding
with the aux controller? I have vague memories of discussion this
before... If it is important that aux port number matches the
documentation then I guess the same holds for the muxed ports on the
main controller.

If we number muxed ports from 2 to 4 then the test in piix4_adap_remove
could be simply changed to check for adapdata->port <= 1.

Please note that I just sent a fix for this specific function, so any
patch touching the same area should go on top of it. I'll bounce it to
you.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/2] i2c: fix piix4 aux port number

[Andrew Cooks]

Hi Jean

On 08/12/17 00:29, Jean Delvare wrote:
> Hi Andrew,
> 
> On Thu, 23 Nov 2017 13:09:38 +1000, Andrew Cooks wrote:
>> Let the aux port use port number one (not zero), to match the AMD
>> documentation and enable mapping ACPI _ADR to port number.
>>
>> This fixes ACPI-based enumeration of I2C slave peripherals that are
>> defined for the aux SMBus port.
>>
>> Signed-off-by: Andrew Cooks <andrew.cooks@opengear.com>
>> ---
>>  drivers/i2c/busses/i2c-piix4.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
>> index 9260cfa..f980f0b 100644
>> --- a/drivers/i2c/busses/i2c-piix4.c
>> +++ b/drivers/i2c/busses/i2c-piix4.c
>> @@ -975,7 +975,7 @@ 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, false, 0, false,
>> +		piix4_add_adapter(dev, retval, false, 1, false,
>>  				  is_sb800 ? piix4_aux_port_name_sb800 : "",
>>  				  &piix4_aux_adapter);
>>  	}
> 
> The port number has consequences. In piix4_adap_remove, port 0 is
> handled differently. We assume that for each controller (main or aux)
> exactly one adapter has port number 0. Your change above breaks this
> assumption.

I see the problem, thanks.

> 
> Also, if the port numbers are supposed to match the documentation, and
> the aux controller is port 1, then I wonder how are the muxed ports of
> the main controller numbered? The driver numbers them from 1 to 3, but
> I guess the documentation numbers them from 2 to 4 to avoid colliding
> with the aux controller? I have vague memories of discussion this
> before... If it is important that aux port number matches the
> documentation then I guess the same holds for the muxed ports on the
> main controller.

The documentation[1][2][3] uses labels Port 0 (00b), Port 2 (01b), Port 3 (10b), Port 4 (11b).

This led me down a rabbit hole of thinking that port 1 is intended to be the aux controller, but now I think that the labels in the documentation is unfortunate, because the labels don't match the binary values. (If 01b had been reserved for Port 1 it would've been fine, but it's not.)

Matching the adapter order of the documentation would have been nice, because ACPI developers rely on that as a reference. If the adapter ordering is different between ACPI and the driver, the peripherals will be mapped to the incorrect adapter.

In my particular case I can fix the ACPI DSDT to match the enumeration order of the driver right now, but I won't be able to change it easily. Similarly, there might be other users who need a different enumeration order and who aren't able to change their ACPI tables as easily.

> 
> If we number muxed ports from 2 to 4 then the test in piix4_adap_remove
> could be simply changed to check for adapdata->port <= 1.

I think the existing piix4_adap_remove code is correct and that using the port in the way I initially proposed is incorrect.

> 
> Please note that I just sent a fix for this specific function, so any
> patch touching the same area should go on top of it. I'll bounce it to
> you.
> 

I've sent a version 2 of the patch set, based on top of that fix.

Thanks!

Andrew

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