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

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

[Florian Eckert]

I am working wit OpenWrt which has recently switched the kernel version
from 5.4 to 5.10 on x86 Target [1] in its master branch.

I am using a APU3 board from PC-Engine [2].

The APU3 board has an SMBus [3] device (Intel PIIX4 and compatible
(ATI/AMD/Serverworks/Broadcom/SMSC) to communicate with additional 
connected I2C devices.

On This SMBus there is a IO expander from microchip connect [4] via the 
SMBus (i2c).
I used this microchip IO expander to control additional LEDs, as the 
APU3 only has 3.

So far, everything has worked fine, because I had wirten a platform 
device for this.
Everything was recognized and compiled cleanly and I could control the 
LEDs from the user-land.

Due to the following change [5] between 5.4 and 5.10 by removing the 
platform data support in
the IO expander mcp23s08, my plaform device does not compile anymore,
I can no longer use the platform device pattern for this kind of device.

The only possibility I can think of now is to make this device known
to the kernel via a dynamic ACPI SSDT table. I have already tried 
various
things but I can't get the driver [4] to feel responsible for this 
device.

I have used the following links that were provided by "Andy Shevchenko" 
to me
to understand the concept begind ACPI SSDT handling. Thanks for that.

https://connect.linaro.org/resources/lvc21f/lvc21f-304/
https://www.youtube.com/watch?v=nlKjUAv3RL0&ab_channel=OSDNConf
https://stackoverflow.com/questions/65727454/
https://stackoverflow.com/questions/60105101/
https://stackoverflow.com/questions/54768841/
https://stackoverflow.com/questions/46095840/
https://github.com/westeri/meta-acpi/tree/master/recipes-bsp/acpi-tables/samples/

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

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

     Device (\_SB.PCI0.SBUS.BUS0)
     {
         Name (_CID, "smbus")
         NAME (_ADR, Zero)
         Device (PIN)
         {
             Name (_HID, "PRP0001")
             Name (_DDN, "io expander")
             Name (_CRS, ResourceTemplate () {
                 I2cSerialBus (
                     0x24,                   // Bus address
                     ControllerInitiated,    // Don't care
                     400000,                 // Fast mode (400 kHz)
                     AddressingMode7Bit,     // 7-bit addressing
                     "\\_SB.PCI0.SBUS.BUS0", // I2C host controller
                     0                       // Must be 0
                 )
             })

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

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

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)
../../../devices/pci0000:00/0000:00:14.0/i2c-2 (SMBus PIIX4 adapter port 
1 at 0b20)


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

Therefore I believe the following applies

+------+    +------+
| PCI0 |--->| SMB0 |--> i2c client A (0x24)
|      |    |      |
+------+    +------+


I have enabled the following kernel config parameters for ACPI SSDT:
CONFIG_ACPI_CUSTOM_METHOD
CONFIG_CONFIGFS_FS
CONFIG_ACPI_CONFIGFS
CONFIG_ACPI_DEBUG

The goal would be that the ACPI mapping for the i2c-pii4 and the 
connected pinctrl-mcp23s08 exactly works as
show in the video [5] from Andy Shevchenko.

I think others will have the same problem in the future when they update 
the kernel on an X86 embedded device
which does not support device trees and also now no platform data 
handling.


- Florian

[1] 
https://git.openwrt.org/?p=openwrt/openwrt.git;a=commit;h=64be0eadc17988f29d0a4b89569840d917746498
[2] https://pcengines.ch/apu.htm
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/i2c/busses/i2c-piix4.c?h=v5.10.68
[4] 
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/pinctrl/pinctrl-max77620.c?h=v5.10.68
[5] 
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/pinctrl/pinctrl-mcp23s08.c?h=v5.10.68&id=6aba6ed879b3471903c8ada28ba968a268df6143
[6] https://www.youtube.com/watch?v=nlKjUAv3RL0&ab_channel=OSDNConf
[7] 
https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/southbridge/amd/pi/hudson/acpi/fch.asl#29

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

[Mika Westerberg]

Hi,

On Thu, Sep 23, 2021 at 04:17:13PM +0200, Florian Eckert wrote:
> I am working wit OpenWrt which has recently switched the kernel version
> from 5.4 to 5.10 on x86 Target [1] in its master branch.
> 
> I am using a APU3 board from PC-Engine [2].
> 
> The APU3 board has an SMBus [3] device (Intel PIIX4 and compatible
> (ATI/AMD/Serverworks/Broadcom/SMSC) to communicate with additional connected
> I2C devices.
> 
> On This SMBus there is a IO expander from microchip connect [4] via the
> SMBus (i2c).
> I used this microchip IO expander to control additional LEDs, as the APU3
> only has 3.
> 
> So far, everything has worked fine, because I had wirten a platform device
> for this.
> Everything was recognized and compiled cleanly and I could control the LEDs
> from the user-land.
> 
> Due to the following change [5] between 5.4 and 5.10 by removing the
> platform data support in
> the IO expander mcp23s08, my plaform device does not compile anymore,
> I can no longer use the platform device pattern for this kind of device.
> 
> The only possibility I can think of now is to make this device known
> to the kernel via a dynamic ACPI SSDT table. I have already tried various
> things but I can't get the driver [4] to feel responsible for this device.
> 
> I have used the following links that were provided by "Andy Shevchenko" to
> me
> to understand the concept begind ACPI SSDT handling. Thanks for that.
> 
> https://connect.linaro.org/resources/lvc21f/lvc21f-304/
> https://www.youtube.com/watch?v=nlKjUAv3RL0&ab_channel=OSDNConf
> https://stackoverflow.com/questions/65727454/
> https://stackoverflow.com/questions/60105101/
> https://stackoverflow.com/questions/54768841/
> https://stackoverflow.com/questions/46095840/
> https://github.com/westeri/meta-acpi/tree/master/recipes-bsp/acpi-tables/samples/
> 
> This is my aml file that I tried with. It loads but nothing happens.
> 
> DefinitionBlock ("mcp23s08.aml", "SSDT", 5, "", "IO", 2)
> {
>     External (\_SB.PCI0.SBUS, DeviceObj)
> 
>     Device (\_SB.PCI0.SBUS.BUS0)
>     {
>         Name (_CID, "smbus")
>         NAME (_ADR, Zero)
>         Device (PIN)
>         {
>             Name (_HID, "PRP0001")
>             Name (_DDN, "io expander")
>             Name (_CRS, ResourceTemplate () {
>                 I2cSerialBus (
>                     0x24,                   // Bus address
>                     ControllerInitiated,    // Don't care
>                     400000,                 // Fast mode (400 kHz)
>                     AddressingMode7Bit,     // 7-bit addressing
>                     "\\_SB.PCI0.SBUS.BUS0", // I2C host controller
>                     0                       // Must be 0
>                 )
>             })
> 
>             Name (_DSD, Package () {
>                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                 Package () {
>                     Package () { "compatible", Package () {
> "microchip,mcp23017" } },
>                 }
>             })
>         }
>     }
> }
> 
> In Coreboot the SMBus named SBUS and is on address 0x0014000 [7].
> 
> 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)
> ../../../devices/pci0000:00/0000:00:14.0/i2c-2 (SMBus PIIX4 adapter port 1
> at 0b20)
> 
> 
> The mcp23s08 is connected to the i2c-0 with address 0x24
> 
> Therefore I believe the following applies
> 
> +------+    +------+
> | PCI0 |--->| SMB0 |--> i2c client A (0x24)
> |      |    |      |
> +------+    +------+
> 
> 
> I have enabled the following kernel config parameters for ACPI SSDT:
> CONFIG_ACPI_CUSTOM_METHOD
> CONFIG_CONFIGFS_FS
> CONFIG_ACPI_CONFIGFS
> CONFIG_ACPI_DEBUG

How do you load the SSDT? Via initrd or something else? You may need to
enable CONFIG_ACPI_TABLE_UPGRADE too. Also do you see in dmesg that the
SSDT is loaded?

Then I suggest checking the below attributes:

 ../../../devices/pci0000:00/0000:00:14.0/firmware_node/path
 ../../../devices/pci0000:00/0000:00:14.0/i2c-0/firmware_node/path

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

[Andy Shevchenko]

On Thu, Sep 23, 2021 at 5:26 PM Florian Eckert <fe@dev.tdt.de> 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

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

[Andy Shevchenko]

On Thu, Sep 23, 2021 at 11:26 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Sep 23, 2021 at 5:26 PM Florian Eckert <fe@dev.tdt.de> wrote:

...

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

One more thing, I have no idea how max77620 is related to all this
(you mentioned it in [4]).


-- 
With Best Regards,
Andy Shevchenko

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

[Florian Eckert]

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

Yes the device is a mcp23017, but the driver is the pinctrl_mcp23s08 [1]

root@dev ~ # lsmod | grep mcp
pinctrl_mcp23s08       16384  0
regmap_core            45056  1 pinctrl_mcp23s08,[permanent]

> Summary:
> 1) _CID notation is wrong in ASL;

I got it

> 2) driver seems supports the _ADR schema which you have used in ASL;

This refers to the i2c-0, doesn't it?
My mcp23s08 device is located at the i2c-0 on address 0x24.

> 3) something fishy about I/O addresses in the sysfs (is it a typo when
> you composed the email?);

No

I have asked myself the same question.
Something is not right.
There was a change regarding the Hudson2 in the driver, maybe something 
went wrong [2]?

> 4) it's unclear what you did with ASL to get it loaded;

On my development device I did a `iasl dsl/mcp23017.dsl`
Of the following dsl

$ cat dsl/mcp23017.dsl
DefinitionBlock ("mcp23017.aml", "SSDT", 5, "", "MCP23017", 4)
{
     External (\_SB.PCI0.SBUS, DeviceObj)

     Scope (\_SB.PCI0.SBUS)
     {
         Device (GPIO)
         {
             Name (_HID, "PRP0001")
             Name (_DDN, "MCP23017 gpio expander")
             Name (_ADR, Zero)
             Name (_CRS, ResourceTemplate () {
                 I2cSerialBus (
                     0x24,                   // Bus address
                     ControllerInitiated,    // Don't care
                     400000,                 // Fast mode (400 kHz)
                     AddressingMode7Bit,     // 7-bit addressing
                     "\\_SB.PCI0.SBUS",      // I2C host controller
                     0                       // Must be 0
                 )
             })

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

After that I copied this to my APU3 Target and executed
  the following commands:
mkdir /sys/kernel/config/acpi/table/mcp23017

cat mcp23s08.aml > /sys/kernel/config/table/mcp23017

> 5) as Mika suggested, have you checked the kernel configuration?

I have now switched on the suggested option
CONFIG_ACPI_CUSTOM_METHOD=y
CONFIG_ACPI_TABLE_UPGRADE=y
CONFIG_CONFIGFS_FS=y
CONFIG_ACPI_CONFIGFS=y
CONFIG_ACPI_DEBUG=y

But this did not solved my issue loading ssdt during runtime.

The output of the aml on the target:

cat /sys/kernel/config/acpi/table/mcp23017/aml
SSDTsMCP23017INTL \/_SB_PCI0SBUSK
                                  \/_SB_PCI0SBUS[H
MCP23017 gpio expande_AD_CRS&
#$\_SB.PCI0.SBUS_DSD?
microchip,mcp23017


My iasl version:

iasl --version
Illegal option: --

Intel ACPI Component Architecture
ASL+ Optimizing Compiler/Disassembler version 20181213
Copyright (c) 2000 - 2018 Intel Corporation


What else can I do?
The initrd option does not work with OpenWrt.
Maybe I can get further if you can give me a good HEX for debug_level 
and the debug_layer?

---
Regards Florian

[1] 
https://github.com/torvalds/linux/blob/master/drivers/pinctrl/pinctrl-mcp23s08_i2c.c
[2] 
https://github.com/torvalds/linux/commit/528d53a1592b0e27c423f7cafc1df85f77fc1163#diff-aa95f6311d1fcc4d85955b153a2510e853807546ac8e0d3aa0310ac30d236147

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

[Andy Shevchenko]

On Thu, Sep 30, 2021 at 1:40 AM Florian Eckert <fe@dev.tdt.de> wrote:

...

> > 2) driver seems supports the _ADR schema which you have used in ASL;
>
> This refers to the i2c-0, doesn't it?

Quite likely, but no guarantees. The number of the controller is
issued in order of enumeration which may be not deterministic. What is
real _ADR mapping here is that:

0 == physical port 0
1 == physical port 1
...
n == physical port n

> My mcp23s08 device is located at the i2c-0 on address 0x24.

...

> > 4) it's unclear what you did with ASL to get it loaded;
>
> On my development device I did a `iasl dsl/mcp23017.dsl`
> Of the following dsl
>
> $ cat dsl/mcp23017.dsl
> DefinitionBlock ("mcp23017.aml", "SSDT", 5, "", "MCP23017", 4)
> {
>      External (\_SB.PCI0.SBUS, DeviceObj)
>
>      Scope (\_SB.PCI0.SBUS)
>      {

According to the 2) above and as we learnt from the actual code this
missed one level of the devices, i.e. I2C port

Device (I2C0)
{
        Name (_ADR, Zero)

>          Device (GPIO)
>          {

>              Name (_HID, "PRP0001")
>              Name (_ADR, Zero)

This is against specification as I told you already. _CID/_HID may not
be together with _ADR.

>              Name (_CRS, ResourceTemplate () {
>                  I2cSerialBus (
>                      0x24,                   // Bus address
>                      ControllerInitiated,    // Don't care
>                      400000,                 // Fast mode (400 kHz)
>                      AddressingMode7Bit,     // 7-bit addressing
>                      "\\_SB.PCI0.SBUS",      // I2C host controller
>                      0                       // Must be 0
>                  )
>              })
>
>              Name (_DSD, Package () {
>                  ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                  Package () {
>                      Package () { "compatible", Package () {
> "microchip,mcp23017" } },
>                  }
>              })
>          }
>      }
> }
>
> After that I copied this to my APU3 Target and executed
>   the following commands:
> mkdir /sys/kernel/config/acpi/table/mcp23017
>
> cat mcp23s08.aml > /sys/kernel/config/table/mcp23017

Yeah, but your I2C controller already been enumerated, so this won't
have any effect due to half-baked ACPI table in the firmware
(coreboot) which should be

Device (SBUS)
{
    Name (_ADR, 0x00140000)
    Device (PORT0)
    {
       Name (_ADR, Zero)
    }
    ...
    Device(PORTn)
    {
        Name (_ADR, n)
    }
}

...

> > 5) as Mika suggested, have you checked the kernel configuration?
>
> I have now switched on the suggested option
> CONFIG_ACPI_CUSTOM_METHOD=y
> CONFIG_ACPI_TABLE_UPGRADE=y
> CONFIG_CONFIGFS_FS=y
> CONFIG_ACPI_CONFIGFS=y
> CONFIG_ACPI_DEBUG=y
>
> But this did not solved my issue loading ssdt during runtime.

It won't as explained in 4) above.

> The output of the aml on the target:
>
> cat /sys/kernel/config/acpi/table/mcp23017/aml

Don't use `cat` against binary files. But we don't really need that.
It's the same as after iasl.

...

> My iasl version:
>
> iasl --version
> Illegal option: --
>
> Intel ACPI Component Architecture
> ASL+ Optimizing Compiler/Disassembler version 20181213

Please, update to something newer.

> Copyright (c) 2000 - 2018 Intel Corporation

...

Current summary:
 1) you still have issues with ASL
 2) there is boot ordering issues with the controller itself

> What else can I do?

So, the course of actions as I see is:
- fix the asl
- fix the coreboot to represent ports of the I2C controller
- switch to use initramfs method (or EFI)
- if possible, to unbind and bind back the i2c controller after AML is
loaded via configfs (it may be not, if it's occupied by some
peripheral devices / drivers that are crucial to system to work)

> The initrd option does not work with OpenWrt.

Why not? If indeed so (but I don't believe in this), OpenWRT has a bug
/ missed feature, which must be fixed there.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, Sep 30, 2021 at 9:15 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Sep 30, 2021 at 1:40 AM Florian Eckert <fe@dev.tdt.de> wrote:

...

> > > 5) as Mika suggested, have you checked the kernel configuration?
> >
> > I have now switched on the suggested option
> > CONFIG_ACPI_CUSTOM_METHOD=y
> > CONFIG_ACPI_TABLE_UPGRADE=y
> > CONFIG_CONFIGFS_FS=y
> > CONFIG_ACPI_CONFIGFS=y
> > CONFIG_ACPI_DEBUG=y
> >
> > But this did not solved my issue loading ssdt during runtime.
>
> It won't as explained in 4) above.

It won't _alone_ solve it, i.o.w. it's required but not sufficient.
Means you have to keep these configuration options enabled.

-- 
With Best Regards,
Andy Shevchenko