Enable ACPI-defined peripherals on i2c-piix4 SMBus

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.

Quoted 3 lines · Jean Delvare

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.

Can you tell a little bit more about what devices are behind the smbus ? I recall the G-412 SoCs (such as on apu2+ boards) have an Hudson inside and fall into this category. (I'll have to check when back in office), so (as the apu2 platform driver maintainer) I'm very interested in this.

Does the probing need some special BIOS support (or do the necessary table entries already come from aegesa) ?

I have to admit, I'm still confused by the AMD documentation - haven't found a clear documentation on what peripherals exactly are in the G-412 SoC, just puzzled together that the FCH seems to be an Hudson, probably v2. There also seems to be some relation between smbus and gpio, but the gpio's are directly memory-mapped - no idea whether they just share the same base address register or the gpios are really behind smbus and some hw logic directy maps them into mmio space ... Do you happen to have some more information on that ?

By the way: I'm considering collecting some hw documentation in the kernel tree (maybe Documentation/hardware/...) - do you folks think that's a good idea ?

Unfortunately not. I only picked up from where Andrew Cooks left, due to me being way too slow to review his patches.

I was able to test the first 2 patches which fix bugs, but not the 3rd one which deals with ACPI devices. There does not seem to be any such device on the 2 test machines I have remotely access to.

Quoted 2 lines

Does the probing need some special BIOS support (or do the necessary table entries already come from aegesa) ?

I assume that ACPI devices are declared in one of the ACPI tables, so it comes from the "BIOS", yes, whatever form it takes these days.

I remember noticing long ago that SMBus ports were using GPIO pins, so these pins could be used for SMBus or for any other purpose.

I could
not find any way to figure out from the registers if a given pin pair was used for SMBus or not, which is pretty bad because it means we are blindly instantiating ALL possible SMBus ports even if some of the pins are used for a completely different purpose.

Quoted 3 lines

By the way: I'm considering collecting some hw documentation in the kernel tree (maybe Documentation/hardware/...) - do you folks think that's a good idea ?

No. Only documentation specifically related to the Linux kernel should live in the kernel tree. OS-neutral documentation must go somewhere else.

On 02.08.19 14:51, Jean Delvare wrote:

Hi,

Quoted 3 lines

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.

Can you tell a little bit more about what devices are behind the smbus ? I recall the G-412 SoCs (such as on apu2+ boards) have an Hudson inside and fall into this category. (I'll have to check when back in office), so (as the apu2 platform driver maintainer) I'm very interested in this.

Does the probing need some special BIOS support (or do the necessary table entries already come from aegesa) ?

I have to admit, I'm still confused by the AMD documentation - haven't found a clear documentation on what peripherals exactly are in the G-412 SoC, just puzzled together that the FCH seems to be an Hudson, probably v2. There also seems to be some relation between smbus and gpio, but the gpio's are directly memory-mapped - no idea whether they just share the same base address register or the gpios are really behind smbus and some hw logic directy maps them into mmio space ... Do you happen to have some more information on that ?

By the way: I'm considering collecting some hw documentation in the kernel tree (maybe Documentation/hardware/...) - do you folks think that's a good idea ?

--mtx

Hi Enrico,

Quoted 8 lines · metux IT consult

Quoted 3 lines · Jean Delvare

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.

Can you tell a little bit more about what devices are behind the smbus ? I recall the G-412 SoCs (such as on apu2+ boards) have an Hudson inside and fall into this category. (I'll have to check when back in office), so (as the apu2 platform driver maintainer) I'm very interested in this.

Unfortunately not. I only picked up from where Andrew Cooks left, due to me being way too slow to review his patches. I did not want his work to be lost. I was able to test the first 2 patches which fix bugs, but not the 3rd one which deals with ACPI devices. There does not seem to be any such device on the 2 test machines I have remotely access to.

Quoted 2 lines

Does the probing need some special BIOS support (or do the necessary table entries already come from aegesa) ?

I assume that ACPI devices are declared in one of the ACPI tables, so it comes from the "BIOS", yes, whatever form it takes these days.

Quoted 9 lines

I have to admit, I'm still confused by the AMD documentation - haven't found a clear documentation on what peripherals exactly are in the G-412 SoC, just puzzled together that the FCH seems to be an Hudson, probably v2. There also seems to be some relation between smbus and gpio, but the gpio's are directly memory-mapped - no idea whether they just share the same base address register or the gpios are really behind smbus and some hw logic directy maps them into mmio space ... Do you happen to have some more information on that ?

I remember noticing long ago that SMBus ports were using GPIO pins, so these pins could be used for SMBus or for any other purpose. I could not find any way to figure out from the registers if a given pin pair was used for SMBus or not, which is pretty bad because it means we are blindly instantiating ALL possible SMBus ports even if some of the pins are used for a completely different purpose. It was over 1 year ago though, so I don't remember the details, and my findings then may not apply to the most recent hardware.

Quoted 3 lines

By the way: I'm considering collecting some hw documentation in the kernel tree (maybe Documentation/hardware/...) - do you folks think that's a good idea ?

No. Only documentation specifically related to the Linux kernel should live in the kernel tree. OS-neutral documentation must go somewhere else.

Quoted 3 lines

Unfortunately not. I only picked up from where Andrew Cooks left, due to me being way too slow to review his patches.

@Andrew: can you tell us more about this ?

Quoted 4 lines

I was able to test the first 2 patches which fix bugs, but not the 3rd one which deals with ACPI devices. There does not seem to be any such device on the 2 test machines I have remotely access to.

Did you already test on apu2/apu3 ?
If not, maybe you could prepare a queue that I could test.

My initial work is based on a board that is similar to the APU2, but has additional peripherals connected to the smbus, including a NCT7491 thermal monitor/fan controller and PCA6524 GPIO controller. These are simply peripherals on a board variant, not 'platform' devices, so I didn't want to follow the platform driver approach that the APU2 GPIO driver uses.

SMBus (and I2C) peripherals can generally not be enumerated without some firmware support. It is possible to probe for specific devices on the bus (eg sensors-detect) but in general it is not feasible to let every supported device driver probe the bus for its device. ACPI and Devicetree provides the kernel with metadata for the device: type, address, calibrated set points for temperature, etc.

Since the peripherals are not standard platform devices, they are not described by the ACPI tables provided by Coreboot or AMD, but it's not too difficult to create supplementary device description tables (ACPI) for non-standard devices. These can be added to coreboot, supplied to qemu as additional firmware files (see -acpitable arg), or built into the kernel (see https://www.kernel.org/doc/Documentation/acpi/ssdt-overlays.txt)

ACPI may be an ugly abomination, but it's what we're stuck with on x86 and it can only improve when more people get their hands on it.

You might find it helpful to look at the coreboot source for the APU2 (src/mainboard/pcengines/apu2/gpio_ftns.h)

Just so I get this correct: This is all about instantiating the devices sitting on the SMBus,

but you are okay with Jean's patches? Or is this discussion affecting patch 3? (/me knows not much about ACPI'n'stuff)

Yes, I'm struggling to find out which devices are connected to the smbus, in case there're some already inside the SoC (instead of just entirely board specific).

Quoted 3 lines

but you are okay with Jean's patches? Or is this discussion affecting patch 3? (/me knows not much about ACPI'n'stuff)

I'm fine with them, just wondered whether BIOS needs to add some extra support for the probing, that is not already somehow coming w/ aegesa.

Unfortunately, we cannot rely on board vendors to do that right and cope w/ old and incomplete firmware. (that's also the reason why I'm *not* using acpi gpio entries for the apu2+ board family).

On 11.08.19 04:52, Andrew Cooks wrote:

Hi,

Quoted 1 line

My initial work is based on a board that is similar to the APU2, but has additional peripherals connected to the smbus, including a NCT7491 thermal monitor/fan controller and PCA6524 GPIO controller. These are simply peripherals on a board variant, not 'platform' devices, so I didn't want to follow the platform driver approach that the APU2 GPIO driver uses.

Why are they not platform devices ?

IMHO, a platform device is something not on a bus that can do the probing/enumeration (like eg. pci or usb does).

Can these devices be probed directly by the bus they're connected to, or do you have a different definition of the term "platform device" ?

:O

Quoted 1 line

SMBus (and I2C) peripherals can generally not be enumerated without some firmware support. It is possible to probe for specific devices on the bus (eg sensors-detect) but in general it is not feasible to let every supported device driver probe the bus for its device. ACPI and Devicetree provides the kernel with metadata for the device: type, address, calibrated set points for temperature, etc.

Yes, I know. My question was whether these particular devices - if they're in the SoC itself - do need some extra support in BIOS (acpi entries, etc), that's not already in aegesa. Or in other words:
do I need to patch my BIOS ?

I was assuming these devices are in the SoC itself - correct me if I'm wrong.

Quoted 2 lines

Since the peripherals are not standard platform devices, they are not described by the ACPI tables provided by Coreboot or AMD, but it's not too difficult to create supplementary device description tables (ACPI) for non-standard devices. These can be added to coreboot, supplied to qemu as additional firmware files (see -acpitable arg), or built into the kernel (see https://www.kernel.org/doc/Documentation/acpi/ssdt-overlays.txt)

Oh, I didn't know about SSDT overlays yet. That's interesting.

Is it also possible to describe things like leds-gpio or gpio-keyboard ? Could be a nice idea to trim down my apu2 driver to not much more than a piece of configuration (just like we'd do on a DT-based system)

Quoted 2 lines

ACPI may be an ugly abomination, but it's what we're stuck with on x86 and it can only improve when more people get their hands on it.

hmm, I'm planning to introduce DT to x86 (also patch up apu'2 coreboot do deliver a DT to the kernel) ... but just lacking time to get my hands dirty on this topic.

Quoted 1 line

You might find it helpful to look at the coreboot source for the APU2 (src/mainboard/pcengines/apu2/gpio_ftns.h)

In which version ?

--mtx

From: Andrew Cooks <[email protected]>

Family 16h Model 30h SMBus controller needs the same port selection fix as described and fixed in commit 0fe16195f891 ("i2c: piix4: Fix SMBus port selection for AMD Family 17h chips")

commit 6befa3fde65f ("i2c: piix4: Support alternative port selection register") also fixed the port selection for Hudson2, but unfortunately this is not the exact same device and 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 <[email protected]>

Signed-off-by Jean Delvare <[email protected]>

Cc [email protected] [v4.6+]

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 Model 30h-3Fh

Based on earlier work by Andrew Cooks.

Reported-by Andrew Cooks <[email protected]>

Signed-off-by Jean Delvare <[email protected]>

Enable 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. We assume that ACPI developers will use the AMD documentation ordering, so we have to pass an extra parameter to piix4_add_adapter().

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

Based on earlier work by Andrew Cooks.

Reported-by Andrew Cooks <[email protected]>

Signed-off-by Jean Delvare <[email protected]>