ACPI: Can I use I2cSerialBus with a PCI I2C controller?

ACPI: Can I use I2cSerialBus with a PCI I2C controller?

[Ben Gardner]

Hi everyone,

I am building a custom board based on the Bayley Bay reference design, using
the Intel E3845 CPU.
I have a few I2C devices on the board that are connected to I2C buses 1-3.
The I2C controllers use the i2c-designware-pci driver.

I am currently using i2c_register_board_info() in a driver to describe what
is on the I2C buses. That seems to work just fine.

However, I would like to use the ACPI I2cSerialBus() macro to specify the
devices on those buses and have Linux automatically create the devices.

I am defining the device as shown below (in scope \_SB).
That creates an ACPI device under /sys/bus/acpi/devices/.

// M24C02 EEPROM on I2C-3 addr 0x57
Device (EEP0) {
  Name (_ADR, 1)
  Name (_CID, Package() { "24c02" })
  Method (_CRS, 0, NotSerialized) {
    Name (RBUF, ResourceTemplate () {
      I2cSerialBus (0x0057, ControllerInitiated,
        400000, AddressingMode7Bit, "\\_SB.I2C3",
        0x00, ResourceConsumer,,)
    })
    Return (RBUF)
  }
}

The problem is that the I2C controller is a PCI device. The logic that scans
for attached devices in acpi_i2c_register_devices() in i2c-core.c gives up
because the adapter is not an ACPI device.

  handle = ACPI_HANDLE(adap->dev.parent);
  if (!handle)
    return;

I am using coreboot/SeaBIOS as the BIOS, so I have full control over
the ACPI tables.

I tried enabling the I2C ACPI devices and using the i2c-designware-platform
driver, but that didn't work because Linux changes the resource allocation
for the PCI I2C devices if I specify the resources for the I2C device in ACPI.

The relevent kernel logs are:
[    0.105628] pci 0000:00:18.3: [8086:0f43] type 00 class 0x0c8000
[    0.105656] pci 0000:00:18.3: reg 0x10: [mem 0xd0723000-0xd0723fff]
[    0.105670] pci 0000:00:18.3: reg 0x14: [mem 0xd0724000-0xd0724fff]
...
[    0.122970] pci 0000:00:18.3: can't claim BAR 0 [mem
0xd0723000-0xd0723fff]: address conflict with 80860F43:00 [mem
0xd0723000-0xd0723fff]
...
[    0.136530] pci 0000:00:18.3: BAR 0: assigned [mem 0x80602000-0x80602fff]

The i2c-designware-platform driver tries to use the old BAR0 address
(0xd0723000), which obviously doesn't work.

So my questions are:
  Can I use I2cSerialBus with a PCI I2C controller?
  If so, what am I doing incorrectly?


For reference, here are the interesting parts of the ACPI device definition.
The variable \S3B0 holds the BAR0 value that coreboot sets up.
8086:0f43 is the PCI device ID. 0:18.3 is the PCI bus, device, and function.

Device (I2C3)
{
  Name (_HID, "80860F43")
  Name (_UID, 3)
  Name (_DDN, "I2C Controller #3")
  Name (_ADR, 0x00180003)

  /* Standard Mode: HCNT, LCNT, SDA Hold Time */
  Name (SSCN, Package () { 0x200, 0x200, 0x6 })

  /* Fast Mode: HCNT, LCNT, SDA Hold Time */
  Name (FMCN, Package () { 0x55, 0x99, 0x6 })

  Name (RBUF, ResourceTemplate()
  {
    /* BAR0._BAS is replaced in _CRS */
    Memory32Fixed (ReadWrite, 0, 0x1000, BAR0)
    Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive,,,)
    {
      LPSS_I2C3_IRQ
    }
    FixedDMA (0x10, 0x0, Width32Bit, )
    FixedDMA (0x11, 0x1, Width32Bit, )
  })

  Method (_CRS)
  {
    CreateDwordField (^RBUF, ^BAR0._BAS, RBAS)
    Store (\S3B0, RBAS)
    Return (^RBUF)
  }
  ...
}

Thanks,
Ben Gardner

Re: ACPI: Can I use I2cSerialBus with a PCI I2C controller?

[Mika Westerberg]

On Tue, Oct 20, 2015 at 02:47:43PM -0500, Ben Gardner wrote:
> Hi everyone,
> 
> I am building a custom board based on the Bayley Bay reference design, using
> the Intel E3845 CPU.
> I have a few I2C devices on the board that are connected to I2C buses 1-3.
> The I2C controllers use the i2c-designware-pci driver.
> 
> I am currently using i2c_register_board_info() in a driver to describe what
> is on the I2C buses. That seems to work just fine.
> 
> However, I would like to use the ACPI I2cSerialBus() macro to specify the
> devices on those buses and have Linux automatically create the devices.
> 
> I am defining the device as shown below (in scope \_SB).
> That creates an ACPI device under /sys/bus/acpi/devices/.
> 
> // M24C02 EEPROM on I2C-3 addr 0x57
> Device (EEP0) {
>   Name (_ADR, 1)
>   Name (_CID, Package() { "24c02" })
>   Method (_CRS, 0, NotSerialized) {
>     Name (RBUF, ResourceTemplate () {
>       I2cSerialBus (0x0057, ControllerInitiated,
>         400000, AddressingMode7Bit, "\\_SB.I2C3",
>         0x00, ResourceConsumer,,)
>     })
>     Return (RBUF)
>   }
> }
> 
> The problem is that the I2C controller is a PCI device. The logic that scans
> for attached devices in acpi_i2c_register_devices() in i2c-core.c gives up
> because the adapter is not an ACPI device.
> 
>   handle = ACPI_HANDLE(adap->dev.parent);
>   if (!handle)
>     return;
> 
> I am using coreboot/SeaBIOS as the BIOS, so I have full control over
> the ACPI tables.
> 
> I tried enabling the I2C ACPI devices and using the i2c-designware-platform
> driver, but that didn't work because Linux changes the resource allocation
> for the PCI I2C devices if I specify the resources for the I2C device in ACPI.
> 
> The relevent kernel logs are:
> [    0.105628] pci 0000:00:18.3: [8086:0f43] type 00 class 0x0c8000
> [    0.105656] pci 0000:00:18.3: reg 0x10: [mem 0xd0723000-0xd0723fff]
> [    0.105670] pci 0000:00:18.3: reg 0x14: [mem 0xd0724000-0xd0724fff]
> ...
> [    0.122970] pci 0000:00:18.3: can't claim BAR 0 [mem
> 0xd0723000-0xd0723fff]: address conflict with 80860F43:00 [mem
> 0xd0723000-0xd0723fff]
> ...
> [    0.136530] pci 0000:00:18.3: BAR 0: assigned [mem 0x80602000-0x80602fff]
> 
> The i2c-designware-platform driver tries to use the old BAR0 address
> (0xd0723000), which obviously doesn't work.
> 
> So my questions are:
>   Can I use I2cSerialBus with a PCI I2C controller?

Yes you can.

That's what we do all the time for Intel hardware.

>   If so, what am I doing incorrectly?

Let me see..

> For reference, here are the interesting parts of the ACPI device definition.
> The variable \S3B0 holds the BAR0 value that coreboot sets up.
> 8086:0f43 is the PCI device ID. 0:18.3 is the PCI bus, device, and function.
> 
> Device (I2C3)
> {
>   Name (_HID, "80860F43")
>   Name (_UID, 3)

Drop the _HID and _UID.

>   Name (_DDN, "I2C Controller #3")
>   Name (_ADR, 0x00180003)

This will bind the PCI device to this ACPI device if the address is
correct.

>   /* Standard Mode: HCNT, LCNT, SDA Hold Time */
>   Name (SSCN, Package () { 0x200, 0x200, 0x6 })
> 
>   /* Fast Mode: HCNT, LCNT, SDA Hold Time */
>   Name (FMCN, Package () { 0x55, 0x99, 0x6 })
> 
>   Name (RBUF, ResourceTemplate()
>   {
>     /* BAR0._BAS is replaced in _CRS */
>     Memory32Fixed (ReadWrite, 0, 0x1000, BAR0)
>     Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive,,,)
>     {
>       LPSS_I2C3_IRQ
>     }

You should not populate BAR and IRQ.

>     FixedDMA (0x10, 0x0, Width32Bit, )
>     FixedDMA (0x11, 0x1, Width32Bit, )

These you don't need as the designware I2C does not currently support
DMA.

So you can just drop the whole _CRS and make your I2C host controller
device to look like:

Device (I2C3)
{
	Name (_ADR, 0x00180003)

	/* Standard Mode: HCNT, LCNT, SDA Hold Time */
	Name (SSCN, Package () { 0x200, 0x200, 0x6 })

	/* Fast Mode: HCNT, LCNT, SDA Hold Time */
	Name (FMCN, Package () { 0x55, 0x99, 0x6 })

	Device (EEP0)
	{
		Name (_CID, Package() { "24c02" })
		Name (_CRS, ResourceTemplate () {
			I2cSerialBus (0x0057, ControllerInitiated, 400000,
				      AddressingMode7Bit, "\\_SB.I2C3", 0x00,
				      ResourceConsumer,,)
		})
	}
}

Re: ACPI: Can I use I2cSerialBus with a PCI I2C controller?

[Ben Gardner]

Thanks for the help!

>> So my questions are:
>>   Can I use I2cSerialBus with a PCI I2C controller?
>
> Yes you can.
>
> That's what we do all the time for Intel hardware.

That is good to hear.
...

> So you can just drop the whole _CRS and make your I2C host controller
> device to look like:
>
> Device (I2C3)
> {
>         Name (_ADR, 0x00180003)
>
>         /* Standard Mode: HCNT, LCNT, SDA Hold Time */
>         Name (SSCN, Package () { 0x200, 0x200, 0x6 })
>
>         /* Fast Mode: HCNT, LCNT, SDA Hold Time */
>         Name (FMCN, Package () { 0x55, 0x99, 0x6 })
>
>         Device (EEP0)
>         {
>                 Name (_CID, Package() { "24c02" })
>                 Name (_CRS, ResourceTemplate () {
>                         I2cSerialBus (0x0057, ControllerInitiated, 400000,
>                                       AddressingMode7Bit, "\\_SB.I2C3", 0x00,
>                                       ResourceConsumer,,)
>                 })
>         }
> }

OK. I tried that.
It looks like the device is nested properly in the ACPI tree:

I see this symlink:
/sys/bus/acpi/devices/24C02:00 ->
../../../devices/LNXSYSTM:00/LNXSYBUS:00/device:16/24C02:00

# for x in /sys/bus/acpi/devices/* ; do if [ -e $x/path ] ; then
printf "%-30.30s  %s\n" "$(cat $x/path)" "$x" ; fi ; done | sort |
grep I2C3
\_SB_.I2C3                      device:16
\_SB_.I2C3.EEP0                 24C02:00

But no I2C device are created under /sys/bus/i2c/devices/.
# ll /sys/bus/i2c/devices/
lrwxrwxrwx    1 root     root             0 Oct 21 11:30 i2c-1 ->
../../../devices/pci0000:00/0000:00:18.1/i2c-1
lrwxrwxrwx    1 root     root             0 Oct 21 11:30 i2c-2 ->
../../../devices/pci0000:00/0000:00:18.2/i2c-2
lrwxrwxrwx    1 root     root             0 Oct 21 11:30 i2c-3 ->
../../../devices/pci0000:00/0000:00:18.3/i2c-3

I'll have to dig further to find where it is failing in i2c_core.c. I
assume it will be in the same spot.
Is there a way to see PCI to ACPI associations?

BTW -  I am using Linux kernel version 4.2.3.

Thanks,
Ben

Re: ACPI: Can I use I2cSerialBus with a PCI I2C controller?

[Mika Westerberg]

On Wed, Oct 21, 2015 at 06:14:27PM -0500, Ben Gardner wrote:
> Thanks for the help!
> 
> >> So my questions are:
> >>   Can I use I2cSerialBus with a PCI I2C controller?
> >
> > Yes you can.
> >
> > That's what we do all the time for Intel hardware.
> 
> That is good to hear.
> ...
> 
> > So you can just drop the whole _CRS and make your I2C host controller
> > device to look like:
> >
> > Device (I2C3)
> > {
> >         Name (_ADR, 0x00180003)
> >
> >         /* Standard Mode: HCNT, LCNT, SDA Hold Time */
> >         Name (SSCN, Package () { 0x200, 0x200, 0x6 })
> >
> >         /* Fast Mode: HCNT, LCNT, SDA Hold Time */
> >         Name (FMCN, Package () { 0x55, 0x99, 0x6 })
> >
> >         Device (EEP0)
> >         {
> >                 Name (_CID, Package() { "24c02" })
> >                 Name (_CRS, ResourceTemplate () {
> >                         I2cSerialBus (0x0057, ControllerInitiated, 400000,
> >                                       AddressingMode7Bit, "\\_SB.I2C3", 0x00,
> >                                       ResourceConsumer,,)
> >                 })
> >         }
> > }
> 
> OK. I tried that.
> It looks like the device is nested properly in the ACPI tree:
> 
> I see this symlink:
> /sys/bus/acpi/devices/24C02:00 ->
> ../../../devices/LNXSYSTM:00/LNXSYBUS:00/device:16/24C02:00
> 
> # for x in /sys/bus/acpi/devices/* ; do if [ -e $x/path ] ; then
> printf "%-30.30s  %s\n" "$(cat $x/path)" "$x" ; fi ; done | sort |
> grep I2C3
> \_SB_.I2C3                      device:16
> \_SB_.I2C3.EEP0                 24C02:00
> 
> But no I2C device are created under /sys/bus/i2c/devices/.
> # ll /sys/bus/i2c/devices/
> lrwxrwxrwx    1 root     root             0 Oct 21 11:30 i2c-1 ->
> ../../../devices/pci0000:00/0000:00:18.1/i2c-1
> lrwxrwxrwx    1 root     root             0 Oct 21 11:30 i2c-2 ->
> ../../../devices/pci0000:00/0000:00:18.2/i2c-2
> lrwxrwxrwx    1 root     root             0 Oct 21 11:30 i2c-3 ->
> ../../../devices/pci0000:00/0000:00:18.3/i2c-3
> 
> I'll have to dig further to find where it is failing in i2c_core.c. I
> assume it will be in the same spot.
> Is there a way to see PCI to ACPI associations?

Yes, the PCI device has a symlink "firmware_node" which points to the
ACPI device.

Here an example from Skylake system (which uses PCI mode for LPSS
devices):

# ls -l /sys/bus/pci/devices/0000\:00\:15.1/firmware_node
lrwxrwxrwx    1 root     root             0 Oct 22 08:00 /sys/bus/pci/devices/0000:00:15.1/firmware_node -> ../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:6f

# cat /sys/bus/pci/devices/0000\:00\:15.1/firmware_node/path 
\_SB_.PCI0.I2C1

Re: ACPI: Can I use I2cSerialBus with a PCI I2C controller?

[Ben Gardner]

>> Is there a way to see PCI to ACPI associations?
>
> Yes, the PCI device has a symlink "firmware_node" which points to the
> ACPI device.
>
> Here an example from Skylake system (which uses PCI mode for LPSS
> devices):
>
> # ls -l /sys/bus/pci/devices/0000\:00\:15.1/firmware_node
> lrwxrwxrwx    1 root     root             0 Oct 22 08:00 /sys/bus/pci/devices/0000:00:15.1/firmware_node -> ../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:6f
>
> # cat /sys/bus/pci/devices/0000\:00\:15.1/firmware_node/path
> \_SB_.PCI0.I2C1

I see the problem now.
Your I2C controller is under the PCI0 device on your Skylake system,
mine was not.

So, I moved the definition of all the PCI devices that are on PCI bus
0 to under PCI0.
After that, all the PCI devices found their ACPI companion and the
firmware_node symlink is present and correct.
I2C-core creates a symlink for the attached device on the right controller.
/sys/bus/i2c/devices/i2c-24C02:00 ->
../../../devices/pci0000:00/0000:00:18.3/i2c-3/i2c-24C02:00
So that part is solved. (And I'll have to send a patch to coreboot...)

The next issue is that the I2C-core isn't matching the device to the
"at24" driver, which has the alias "24c02".

I used to instantiate the device from user space with:
# echo 24c02 0x57 > /sys/bus/i2c/devices/i2c-3/new_device
That now shows the address is busy, so it is creating the device, just
not linking it to the driver.

I'll dig further and see what I can see.

Thanks for your help.
Ben

Re: ACPI: Can I use I2cSerialBus with a PCI I2C controller?

[Ben Gardner]

> The next issue is that the I2C-core isn't matching the device to the
> "at24" driver, which has the alias "24c02".

Here is what I found.
i2c-core is creating the device with the ACPI name "24C02:00".
The at24 driver uses "24c02" as the alias.
i2c-code is matching devices to drivers using strcmp().

Result: no match. ("24c02" != "24C02:00").

If I modify acpi_i2c_add_device() to cut off the name at the ':' and
covert to lowercase when populating info.type, it matches and works.
I must be missing something here, because this would have never worked as-is.
I'll ask on the I2C mailing list.

Thanks again,
Ben

Re: ACPI: Can I use I2cSerialBus with a PCI I2C controller?

[Mika Westerberg]

[-- Attachment #1: Type: text/plain, Size: 1802 bytes --]

On Thu, Oct 22, 2015 at 12:17:58PM -0500, Ben Gardner wrote:
> > The next issue is that the I2C-core isn't matching the device to the
> > "at24" driver, which has the alias "24c02".
> 
> Here is what I found.
> i2c-core is creating the device with the ACPI name "24C02:00".
> The at24 driver uses "24c02" as the alias.
> i2c-code is matching devices to drivers using strcmp().
> 
> Result: no match. ("24c02" != "24C02:00").
> 
> If I modify acpi_i2c_add_device() to cut off the name at the ':' and
> covert to lowercase when populating info.type, it matches and works.
> I must be missing something here, because this would have never worked as-is.
> I'll ask on the I2C mailing list.

You should either use proper _HID/_CID for the device or put "PRP0001"
to the _HID and let the match happen with DT .compatible strings. I've
attached a hack that I use locally.

The corresponding ASL fragment would look like:


        Device (AT24)
        {
            Name (_HID, "PRP0001")

            Method (_CRS, 0, Serialized) {
                Name (UBUF, ResourceTemplate () {
                    I2cSerialBus (0x50, ControllerInitiated, 0x00061A80,
                        AddressingMode7Bit, "\\_SB.I2C6",
                        0x00, ResourceConsumer)
                })
                Return (UBUF)
            }

            Name (_DSD, Package () {
                ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                Package () {
                    Package () {"compatible", "atmel,24c02"},
                    Package () {"size", 256},
                    Package () {"pagesize", 32},
                    Package () {"abs-value", 1},
                },
            })

            Method (_STA, 0, NotSerialized)
            {
                Return (0xF)
            }
        }

[-- Attachment #2: 0001-misc-at24-Make-use-of-device-property-API.patch --]
[-- Type: text/plain, Size: 3164 bytes --]

>From 78792d4e759f023975700222caffa7a20f77fcf9 Mon Sep 17 00:00:00 2001
From: Mika Westerberg <mika.westerberg@linux.intel.com>
Date: Mon, 29 Sep 2014 13:11:07 +0300
Subject: [PATCH] misc/at24: Make use of device property API

Not-Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/misc/eeprom/at24.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index c6cb7f8f325e..d2963b5632d4 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -20,9 +20,9 @@
 #include <linux/log2.h>
 #include <linux/bitops.h>
 #include <linux/jiffies.h>
-#include <linux/of.h>
 #include <linux/i2c.h>
 #include <linux/platform_data/at24.h>
+#include <linux/property.h>
 
 /*
  * I2C EEPROMs from most vendors are inexpensive and mostly interchangeable.
@@ -443,26 +443,24 @@ static ssize_t at24_macc_write(struct memory_accessor *macc, const char *buf,
 
 /*-------------------------------------------------------------------------*/
 
-#ifdef CONFIG_OF
-static void at24_get_ofdata(struct i2c_client *client,
+static int at24_get_fwdata(struct i2c_client *client,
 		struct at24_platform_data *chip)
 {
-	const __be32 *val;
-	struct device_node *node = client->dev.of_node;
-
-	if (node) {
-		if (of_get_property(node, "read-only", NULL))
-			chip->flags |= AT24_FLAG_READONLY;
-		val = of_get_property(node, "pagesize", NULL);
-		if (val)
-			chip->page_size = be32_to_cpup(val);
-	}
+	if (device_property_present(&client->dev, "read-only"))
+		chip->flags |= AT24_FLAG_READONLY;
+	if (device_property_read_u16(&client->dev, "pagesize", &chip->page_size))
+		return -ENODEV;
+	if (device_property_read_u32(&client->dev, "size", &chip->byte_len))
+		return -ENODEV;
+
+	return 0;
 }
-#else
-static void at24_get_ofdata(struct i2c_client *client,
-		struct at24_platform_data *chip)
-{ }
-#endif /* CONFIG_OF */
+
+static const struct of_device_id at24_of_match[] = {
+	{ .compatible = "atmel,24c02" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, at24_of_match);
 
 static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
@@ -477,7 +475,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	if (client->dev.platform_data) {
 		chip = *(struct at24_platform_data *)client->dev.platform_data;
-	} else {
+	} else if (id) {
 		if (!id->driver_data)
 			return -ENODEV;
 
@@ -493,10 +491,15 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		chip.page_size = 1;
 
 		/* update chipdata if OF is present */
-		at24_get_ofdata(client, &chip);
+		at24_get_fwdata(client, &chip);
 
 		chip.setup = NULL;
 		chip.context = NULL;
+	} else {
+		memset(&chip, 0, sizeof(chip));
+		err = at24_get_fwdata(client, &chip);
+		if (err)
+			return err;
 	}
 
 	if (!is_power_of_2(chip.byte_len))
@@ -661,6 +664,7 @@ static int at24_remove(struct i2c_client *client)
 static struct i2c_driver at24_driver = {
 	.driver = {
 		.name = "at24",
+		.of_match_table = at24_of_match,
 	},
 	.probe = at24_probe,
 	.remove = at24_remove,
-- 
2.6.1

Re: ACPI: Can I use I2cSerialBus with a PCI I2C controller?

[Mika Westerberg]

On Fri, Oct 23, 2015 at 11:20:54AM +0300, Mika Westerberg wrote:
> On Thu, Oct 22, 2015 at 12:17:58PM -0500, Ben Gardner wrote:
> > > The next issue is that the I2C-core isn't matching the device to the
> > > "at24" driver, which has the alias "24c02".
> > 
> > Here is what I found.
> > i2c-core is creating the device with the ACPI name "24C02:00".
> > The at24 driver uses "24c02" as the alias.
> > i2c-code is matching devices to drivers using strcmp().
> > 
> > Result: no match. ("24c02" != "24C02:00").
> > 
> > If I modify acpi_i2c_add_device() to cut off the name at the ':' and
> > covert to lowercase when populating info.type, it matches and works.
> > I must be missing something here, because this would have never worked as-is.
> > I'll ask on the I2C mailing list.
> 
> You should either use proper _HID/_CID for the device or put "PRP0001"
> to the _HID and let the match happen with DT .compatible strings. I've
> attached a hack that I use locally.

Alternatively you can use patch by Andy here:

https://patchwork.ozlabs.org/patch/524923

Re: ACPI: Can I use I2cSerialBus with a PCI I2C controller?

[Ben Gardner]

On Fri, Oct 23, 2015 at 3:20 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Thu, Oct 22, 2015 at 12:17:58PM -0500, Ben Gardner wrote:
>> > The next issue is that the I2C-core isn't matching the device to the
>> > "at24" driver, which has the alias "24c02".
>>
>> Here is what I found.
>> i2c-core is creating the device with the ACPI name "24C02:00".
>> The at24 driver uses "24c02" as the alias.
>> i2c-code is matching devices to drivers using strcmp().
>>
>> Result: no match. ("24c02" != "24C02:00").
>>
>> If I modify acpi_i2c_add_device() to cut off the name at the ':' and
>> covert to lowercase when populating info.type, it matches and works.
>> I must be missing something here, because this would have never worked as-is.
>> I'll ask on the I2C mailing list.
>
> You should either use proper _HID/_CID for the device or put "PRP0001"
> to the _HID and let the match happen with DT .compatible strings. I've
> attached a hack that I use locally.
>
> The corresponding ASL fragment would look like:
>
>
>         Device (AT24)
>         {
>             Name (_HID, "PRP0001")
>
>             Method (_CRS, 0, Serialized) {
>                 Name (UBUF, ResourceTemplate () {
>                     I2cSerialBus (0x50, ControllerInitiated, 0x00061A80,
>                         AddressingMode7Bit, "\\_SB.I2C6",
>                         0x00, ResourceConsumer)
>                 })
>                 Return (UBUF)
>             }
>
>             Name (_DSD, Package () {
>                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                 Package () {
>                     Package () {"compatible", "atmel,24c02"},
>                     Package () {"size", 256},
>                     Package () {"pagesize", 32},
>                     Package () {"abs-value", 1},
>                 },
>             })
>
>             Method (_STA, 0, NotSerialized)
>             {
>                 Return (0xF)
>             }
>         }

I like this approach, as one change to the driver will support all ACPI devices.
I'll give it a try.

Thanks,
Ben

Re: ACPI: Can I use I2cSerialBus with a PCI I2C controller?

[Ben Gardner]

On Fri, Oct 23, 2015 at 12:24 PM, Ben Gardner <gardner.ben@gmail.com> wrote:
> On Fri, Oct 23, 2015 at 3:20 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
>> On Thu, Oct 22, 2015 at 12:17:58PM -0500, Ben Gardner wrote:
>>> > The next issue is that the I2C-core isn't matching the device to the
>>> > "at24" driver, which has the alias "24c02".
>>>
>>> Here is what I found.
>>> i2c-core is creating the device with the ACPI name "24C02:00".
>>> The at24 driver uses "24c02" as the alias.
>>> i2c-code is matching devices to drivers using strcmp().
>>>
>>> Result: no match. ("24c02" != "24C02:00").
>>>
>>> If I modify acpi_i2c_add_device() to cut off the name at the ':' and
>>> covert to lowercase when populating info.type, it matches and works.
>>> I must be missing something here, because this would have never worked as-is.
>>> I'll ask on the I2C mailing list.
>>
>> You should either use proper _HID/_CID for the device or put "PRP0001"
>> to the _HID and let the match happen with DT .compatible strings. I've
>> attached a hack that I use locally.
>>
>> The corresponding ASL fragment would look like:
>>
>>
>>         Device (AT24)
>>         {
>>             Name (_HID, "PRP0001")
>>
>>             Method (_CRS, 0, Serialized) {
>>                 Name (UBUF, ResourceTemplate () {
>>                     I2cSerialBus (0x50, ControllerInitiated, 0x00061A80,
>>                         AddressingMode7Bit, "\\_SB.I2C6",
>>                         0x00, ResourceConsumer)
>>                 })
>>                 Return (UBUF)
>>             }
>>
>>             Name (_DSD, Package () {
>>                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>                 Package () {
>>                     Package () {"compatible", "atmel,24c02"},
>>                     Package () {"size", 256},
>>                     Package () {"pagesize", 32},
>>                     Package () {"abs-value", 1},
>>                 },
>>             })
>>
>>             Method (_STA, 0, NotSerialized)
>>             {
>>                 Return (0xF)
>>             }
>>         }
>
> I like this approach, as one change to the driver will support all ACPI devices.
> I'll give it a try.

This approach works well enough.

Would you mind if I cleaned up the patch and submitted it for inclusion?
Or is there a reason why this isn't upstream?

Thanks,
Ben

Re: ACPI: Can I use I2cSerialBus with a PCI I2C controller?

[Mika Westerberg]

On Mon, Oct 26, 2015 at 02:56:09PM -0500, Ben Gardner wrote:
> This approach works well enough.

Great.

> Would you mind if I cleaned up the patch and submitted it for inclusion?

Not at all.

> Or is there a reason why this isn't upstream?

I just haven't had time to clean it up ;-)

Re: ACPI: Can I use I2cSerialBus with a PCI I2C controller?

[Dustin Byford]

Hi Mika,

On Fri Oct 23 11:20, Mika Westerberg wrote:

> You should either use proper _HID/_CID for the device or put "PRP0001"
> to the _HID and let the match happen with DT .compatible strings. I've
> attached a hack that I use locally.

I have a similar hack over here.  I have a question though:

> The corresponding ASL fragment would look like:
> 
> 
>         Device (AT24)
>         {
>             Name (_HID, "PRP0001")
> 
>             Method (_CRS, 0, Serialized) {
>                 Name (UBUF, ResourceTemplate () {
>                     I2cSerialBus (0x50, ControllerInitiated, 0x00061A80,
>                         AddressingMode7Bit, "\\_SB.I2C6",
>                         0x00, ResourceConsumer)
>                 })
>                 Return (UBUF)
>             }
> 
>             Name (_DSD, Package () {
>                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                 Package () {
>                     Package () {"compatible", "atmel,24c02"},

The "c02" in 24c02 also indicates the size.  I've always found it a
little awkward when you could have a compatible string that disagrees
with firmware properties.  Should we do something about that?

For example, is the more generic string, "atmel,at24" better?  I'm not
sure I like that approach in general, but it works well for the at24
devices.  at25 does it the same way.

>                     Package () {"size", 256},
>                     Package () {"pagesize", 32},
>                     Package () {"abs-value", 1},
>                 },
>             })
> 
>             Method (_STA, 0, NotSerialized)
>             {
>                 Return (0xF)
>             }
>         }

		--Dustin

Re: ACPI: Can I use I2cSerialBus with a PCI I2C controller?

[Mika Westerberg]

On Tue, Oct 27, 2015 at 02:11:11PM -0700, Dustin Byford wrote:
> Hi Mika,
> 
> On Fri Oct 23 11:20, Mika Westerberg wrote:
> 
> > You should either use proper _HID/_CID for the device or put "PRP0001"
> > to the _HID and let the match happen with DT .compatible strings. I've
> > attached a hack that I use locally.
> 
> I have a similar hack over here.  I have a question though:
> 
> > The corresponding ASL fragment would look like:
> > 
> > 
> >         Device (AT24)
> >         {
> >             Name (_HID, "PRP0001")
> > 
> >             Method (_CRS, 0, Serialized) {
> >                 Name (UBUF, ResourceTemplate () {
> >                     I2cSerialBus (0x50, ControllerInitiated, 0x00061A80,
> >                         AddressingMode7Bit, "\\_SB.I2C6",
> >                         0x00, ResourceConsumer)
> >                 })
> >                 Return (UBUF)
> >             }
> > 
> >             Name (_DSD, Package () {
> >                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >                 Package () {
> >                     Package () {"compatible", "atmel,24c02"},
> 
> The "c02" in 24c02 also indicates the size.  I've always found it a
> little awkward when you could have a compatible string that disagrees
> with firmware properties.  Should we do something about that?
>
> For example, is the more generic string, "atmel,at24" better?  I'm not
> sure I like that approach in general, but it works well for the at24
> devices.  at25 does it the same way.

I think DT version actually identifies this if the device node is called
"at24". I'm not sure if we can do that with ACPI though.

I agree that we could use "atmel,at24" here but that requires
corresponding binding to be added to
Documentation/devicetree/bindings/i2c/trivial-devices.txt and review
from DT folks. Definitely worth doing.

Alternatively we may use the same _HID "INT3499" as Andy is doing in his
series with the below properties.

> >                     Package () {"size", 256},
> >                     Package () {"pagesize", 32},
> >                     Package () {"abs-value", 1},
> >                 },
> >             })
> > 
> >             Method (_STA, 0, NotSerialized)
> >             {
> >                 Return (0xF)
> >             }
> >         }
> 
> 		--Dustin

Re: ACPI: Can I use I2cSerialBus with a PCI I2C controller?

[Ben Gardner]

Hi,

On Wed, Oct 28, 2015 at 4:01 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Oct 27, 2015 at 02:11:11PM -0700, Dustin Byford wrote:
>> Hi Mika,
>>
>> On Fri Oct 23 11:20, Mika Westerberg wrote:
>>
>> > You should either use proper _HID/_CID for the device or put "PRP0001"
>> > to the _HID and let the match happen with DT .compatible strings. I've
>> > attached a hack that I use locally.
>>
>> I have a similar hack over here.  I have a question though:
>>
>> > The corresponding ASL fragment would look like:
>> >
>> >
>> >         Device (AT24)
>> >         {
>> >             Name (_HID, "PRP0001")
>> >
>> >             Method (_CRS, 0, Serialized) {
>> >                 Name (UBUF, ResourceTemplate () {
>> >                     I2cSerialBus (0x50, ControllerInitiated, 0x00061A80,
>> >                         AddressingMode7Bit, "\\_SB.I2C6",
>> >                         0x00, ResourceConsumer)
>> >                 })
>> >                 Return (UBUF)
>> >             }
>> >
>> >             Name (_DSD, Package () {
>> >                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> >                 Package () {
>> >                     Package () {"compatible", "atmel,24c02"},
>>
>> The "c02" in 24c02 also indicates the size.  I've always found it a
>> little awkward when you could have a compatible string that disagrees
>> with firmware properties.  Should we do something about that?
>>
>> For example, is the more generic string, "atmel,at24" better?  I'm not
>> sure I like that approach in general, but it works well for the at24
>> devices.  at25 does it the same way.
>
> I think DT version actually identifies this if the device node is called
> "at24". I'm not sure if we can do that with ACPI though.
>
> I agree that we could use "atmel,at24" here but that requires
> corresponding binding to be added to
> Documentation/devicetree/bindings/i2c/trivial-devices.txt and review
> from DT folks. Definitely worth doing.

It looks like you are supposed to have the first compatible entry be
the exact manufacturer,model.
http://devicetree.org/Device_Tree_Usage#Understanding_the_compatible_Property

I am considering using a generic compatible entry ("linux,at24") to
match the Linux at24 driver.
In my case, I'm using a M24C02 from ST, so the compatible line would be:

  Package () {"compatible", Package () {"st,m24c02", "linux,at24"}},
  Package () {"size", 256},
  Package () {"pagesize", 16},

The "linux,at24" would require the other properties (size and
pagesize) to be present.
I've seen something similar done with a few other devices.

> Alternatively we may use the same _HID "INT3499" as Andy is doing in his
> series with the below properties.

The only issue with reusing INT3499 is that it is configured to be a
24C08 in the at24 driver.
Reusing the 24C08 HID for a 24C02 or other chip might be a bit confusing.

Ben

Re: ACPI: Can I use I2cSerialBus with a PCI I2C controller?

[Mika Westerberg]

On Fri, Oct 30, 2015 at 11:51:01AM -0500, Ben Gardner wrote:
> It looks like you are supposed to have the first compatible entry be
> the exact manufacturer,model.
> http://devicetree.org/Device_Tree_Usage#Understanding_the_compatible_Property
> 
> I am considering using a generic compatible entry ("linux,at24") to
> match the Linux at24 driver.
> In my case, I'm using a M24C02 from ST, so the compatible line would be:
> 
>   Package () {"compatible", Package () {"st,m24c02", "linux,at24"}},
>   Package () {"size", 256},
>   Package () {"pagesize", 16},
> 
> The "linux,at24" would require the other properties (size and
> pagesize) to be present.
> I've seen something similar done with a few other devices.

Using "linux,foo" is not a good idea and I'm pretty sure DT maintainers
will not take such patches. The compatible (and other properties as
well) should describe hardware not software.

> > Alternatively we may use the same _HID "INT3499" as Andy is doing in his
> > series with the below properties.
> 
> The only issue with reusing INT3499 is that it is configured to be a
> 24C08 in the at24 driver.
> Reusing the 24C08 HID for a 24C02 or other chip might be a bit confusing.

It could default to 24c08 but if it has properties the driver should use
those to determine size etc.