Re: Add a SSDT ACPI description to recognize my I2C device connected via SMBus

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Thu, Sep 23, 2021 at 5:26 PM Florian Eckert <fe@xxxxxxxxxx> wrote:

...

> This is my aml file that I tried with. It loads but nothing happens.

It's ASL, have you compiled it?

> DefinitionBlock ("mcp23s08.aml", "SSDT", 5, "", "IO", 2)
> {
>      External (\_SB.PCI0.SBUS, DeviceObj)
>
>      Device (\_SB.PCI0.SBUS.BUS0)
>      {

>          Name (_CID, "smbus")
>          NAME (_ADR, Zero)

This seems not right.

First of all, using _ADR along with _HID or _CID is against ACPI
specification. Second, the _CID value is against specification. (AR:
Please, drop _CID)

Third, what does _ADR == 0 mean? In the ACPI the _ADR == 0 for the PCI
device is only allowed for the PCI Host Bridge. What you need to put
here is the address of the PCI device, but it looks like you added the
BUS0 device which is not needed at all and ACPI tables already provide
it. Share (decompiled) DSDT of the device in question somewhere and we
can check this. (Okay, nevermind, I found something, see below)

>          Device (PIN)
>          {
>              Name (_HID, "PRP0001")
>              Name (_DDN, "io expander")
>              Name (_CRS, ResourceTemplate () {
>                  I2cSerialBus (
>                      0x24,                   // Bus address

Bus?! It's a slave address, i.e. your MCP chip address.

>                      ControllerInitiated,    // Don't care
>                      400000,                 // Fast mode (400 kHz)
>                      AddressingMode7Bit,     // 7-bit addressing
>                      "\\_SB.PCI0.SBUS.BUS0", // I2C host controller

Should be double checked, see above. Otherwise it seems good.

>                      0                       // Must be 0
>                  )
>              })
>
>              Name (_DSD, Package () {
>                  ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                  Package () {
>                      Package () { "compatible", Package () {
> "microchip,mcp23017" } },

Have you read https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
just in case?

(I think compatible should be enough, but who knows)

>                  }
>              })
>          }
>      }
> }
>
> In Coreboot the SMBus named SBUS and is on address 0x0014000 [7].

Exactly. It means 00:14.0 in BDF notation.

> But I'm not sure if that's right at all.
> Somehow I don't understand how the io expander is connected to SMBus.
> According to my research, however, it should fit.
>
> The SMBus device driver i2c-piix4 creates 3 I2C devices:
> ls -la /sys/bus/i2c/devices/
> ../../../devices/pci0000:00/0000:00:14.0/i2c-0 (SMBus PIIX4 adapter port
> 0 at 0b00)
> ../../../devices/pci0000:00/0000:00:14.0/i2c-1 (SMBus PIIX4 adapter port
> 2 at 0b00)

Same I/O for two different ports?!

> ../../../devices/pci0000:00/0000:00:14.0/i2c-2 (SMBus PIIX4 adapter port
> 1 at 0b20)

Ah, it looks like a multifunctional device. In that case you have to
be sure the driver of the I2C controller is ready for the ACPI
enumeration (seems not). Basically you may use _ADR == 0, 1, ... for
children, but you need to document this and agree with AMD on the use.

Okay, it seems it has this:
  https://elixir.bootlin.com/linux/v5.15-rc2/source/drivers/i2c/i2c-mux.c#L396
which should populate a firmware node to a certain child.

> The mcp23s08 is connected to the i2c-0 with address 0x24

The mcp23s08 can not be connected to I2C. It's a SPI device.
Which chip do you actually have? I believe it's MCP23017 or MCP23018,
which is I2C.


Summary:
1) _CID notation is wrong in ASL;
2) driver seems supports the _ADR schema which you have used in ASL;
3) something fishy about I/O addresses in the sysfs (is it a typo when
you composed the email?);
4) it's unclear what you did with ASL to get it loaded;
5) as Mika suggested, have you checked the kernel configuration?

Otherwise I can't see anything else suspicious here.

-- 
With Best Regards,
Andy Shevchenko