i2c: fix piix4 aux port number

PATCH In review Andrew Cooks 2 participants 5 messages Nov 23, 2017 – Dec 14, 2017 1 message not in the archive

Subsystems (2)

I2C subsystem host drivers I2C/smbus controller drivers for pc

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Andrew Cooks

author

2017-11-23 03:10 UTC #1 lore raw

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

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

---
 drivers/i2c/busses/i2c-piix4.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/i2c/busses/i2c-piix4.c +6 -0

index 174579d..9260cfa 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),
+				      port);
+	}
+
 	snprintf(adap->name, sizeof(adap->name),
 		"SMBus PIIX4 adapter%s at %04x", name, smba);

Signature · 1 line

2.7.4

Jean Delvare

2017-12-07 14:37 UTC #2 lore raw

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

Hi Andrew,

Quoted 16 lines · 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 <[email protected]>
---
 drivers/i2c/busses/i2c-piix4.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/i2c/busses/i2c-piix4.c +0 -0
index 174579d..9260cfa 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;

drivers/i2c/busses/i2c-piix4.c static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba, · line 848

Quoted 10 lines

drivers/i2c/busses/i2c-piix4.c +6 -0

 
+	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.

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Andrew Cooks

author

2017-11-23 03:09 UTC #3 lore raw

PATCH 2/2 i2c: fix piix4 aux port number

In reply to a message not in the archive

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

---
 drivers/i2c/busses/i2c-piix4.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/i2c/busses/i2c-piix4.c +1 -1

index 9260cfa..f980f0b 100644
@@ -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);
 	}

Signature · 1 line

2.7.4

Jean Delvare

2017-12-07 14:29 UTC #4 lore raw

Hi Andrew,

Quoted 16 lines · 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 <[email protected]>
---
 drivers/i2c/busses/i2c-piix4.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-piix4.c +0 -0
index 9260cfa..f980f0b 100644

drivers/i2c/busses/i2c-piix4.c static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id) · line 981

Quoted 8 lines

drivers/i2c/busses/i2c-piix4.c +1 -1

@@ -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.

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Andrew Cooks

author

2017-12-14 03:31 UTC #5 lore raw

Hi Jean

Quoted 24 lines · Jean Delvare

Hi Andrew,
Quoted 21 lines · 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 <[email protected]>
---
 drivers/i2c/busses/i2c-piix4.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-piix4.c +1 -1
index 9260cfa..f980f0b 100644
@@ -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,

Quoted 8 lines

drivers/i2c/busses/i2c-piix4.c static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id) · line 981
Quoted 3 lines
drivers/i2c/busses/i2c-piix4.c +0 -0
 				  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.

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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