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