| Message ID | 20150817120305.GB1552@lahna.fi.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Mika, Thanks for taking a look. On Mon Aug 17 15:03, Mika Westerberg wrote: > On Fri, Aug 14, 2015 at 12:31:32PM -0700, Dustin Byford wrote: > > Name (_DSD, Package () > > { > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > Package () { > > Package (2) { "compatible", "nxp,pca9548" }, > > } > > Nice, you are using _DSD :-) Yes, and I've got some other patches related to that. I'll keep sending, but the relative youth of _DSD does bring up a few higher level issues (for me at least). One thing at a time though, stay tuned. > > I had to: > > > > 1) Find and set an ACPI companion for the "virtual" I2C adapters created > > for each mux channel. > > > > 2) Make sure to scan adap.dev when registering devices under each mux > > channel. > I think the current code in I2C core is not actually doing the right > thing according the ACPI spec at least. To my understanding you can have > device with I2cSerialBus resource _anywhere_ in the namespace, not just > directly below the host controller. It's the ResourceSource attribute > that tells the corresponding host controller. I think you're right. > I wonder if it helps if we scan the whole namespace for devices with > I2cSerialBus that matches the just registered adapter? Something like > the patch below. Looks reasonable to me. Let me work with the patch for a bit and see if I can make it work in my system. --Dustin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mika, On Mon Aug 17 15:03, Mika Westerberg wrote: > I think the current code in I2C core is not actually doing the right > thing according the ACPI spec at least. To my understanding you can have > device with I2cSerialBus resource _anywhere_ in the namespace, not just > directly below the host controller. It's the ResourceSource attribute > that tells the corresponding host controller. > > I wonder if it helps if we scan the whole namespace for devices with > I2cSerialBus that matches the just registered adapter? Something like > the patch below. I've been working with the patch you suggested below. > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > index c83e4d13cfc5..2a309d27421a 100644 > --- a/drivers/i2c/i2c-core.c > +++ b/drivers/i2c/i2c-core.c ... > static void acpi_i2c_register_devices(struct i2c_adapter *adap) > { > - acpi_handle handle; > acpi_status status; > > - if (!adap->dev.parent) > - return; > - > - handle = ACPI_HANDLE(adap->dev.parent); > - if (!handle) > + if (!adap->dev.parent || !has_acpi_companion(adap->dev.parent)) > return; > > - status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, > + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, > + ACPI_I2C_MAX_SCAN_DEPTH, > acpi_i2c_add_device, NULL, > adap, NULL); On my systems (which admittedly all define their i2c clients below the controller) this works as expected, i.e. there's no change in behavior. As far as I can tell it more accurately implements the spec. However, it doesn't quite solve my problem. When acpi_i2c_register_devices(adap) is called on the "virtual" controller that is created for an i2c mux channel, the adap->dev.parent (set to the parent i2c bus for the mux) does not have an acpi companion. That ultimately causes acpi_i2c_add_device() to never find a match. I'll recap a bit since it's been a while and I've learned a few things that might affect the discussion. For now, I'll focus on my proposed ASL for an I2C mux using device properties. Lets say we have i2c hardware attached like this: i801 controller (PCI) pca9548 8-channel mux (address 0x70) lm75 temperature sensor (channel 0 on the mux with address 0x50) I think this is a sensible way to represent it: Device (MUX0) { Name (_ADR, 0x70) Name (_HID, "PRP0001") Name (_CRS, ResourceTemplate() { I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED, AddressingMode7Bit, "^^SMB2", 0x00, ResourceConsumer,,) }) Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { Package (2) { "compatible", "nxp,pca9548" }, } }) // MUX channel 0 Device (CH00) { Name (_ADR, 0x0) Device (TMP0) { Name (_ADR, 0x50) Name (_CRS, ResourceTemplate() { I2cSerialBus (0x60, ControllerInitiated, I2C_SPEED, AddressingMode7Bit, "^CH00", 0x00, ResourceConsumer,,) }) Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { Package (2) { "compatible", "national,lm75" }, } }) } } } The new thing here is using _ADR to treat each mux channel as a device and referencing those devices elsewhere (CH00). I arrived at this because it seems to fit the ACPI model reasonably well* and it's easy to implement (just like in other callers to acpi_preset_companion()) * by reasonably well, I think it's clear and works naturally but this use of _ADR isn't explicitly defined in the spec [ACPI 6, Table 6-168 (page 278)] Hopefully the spec ambiguity isn't too much effort to clarify. I think it's a good change. But, perhaps it's unnecessary. Any feedback on whether this ASL seem like the right way to go for device property i2c muxes? If not, is there an acceptable alternative? I wonder how muxes are handled otherwise? Hopefully not ASL methods :) thanks, --Dustin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 29, 2015 at 04:19:12PM -0700, Dustin Byford wrote: > Hi Mika, > > On Mon Aug 17 15:03, Mika Westerberg wrote: > > I think the current code in I2C core is not actually doing the right > > thing according the ACPI spec at least. To my understanding you can have > > device with I2cSerialBus resource _anywhere_ in the namespace, not just > > directly below the host controller. It's the ResourceSource attribute > > that tells the corresponding host controller. > > > > I wonder if it helps if we scan the whole namespace for devices with > > I2cSerialBus that matches the just registered adapter? Something like > > the patch below. > > I've been working with the patch you suggested below. > > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > > index c83e4d13cfc5..2a309d27421a 100644 > > --- a/drivers/i2c/i2c-core.c > > +++ b/drivers/i2c/i2c-core.c > > ... > > > static void acpi_i2c_register_devices(struct i2c_adapter *adap) > > { > > - acpi_handle handle; > > acpi_status status; > > > > - if (!adap->dev.parent) > > - return; > > - > > - handle = ACPI_HANDLE(adap->dev.parent); > > - if (!handle) > > + if (!adap->dev.parent || !has_acpi_companion(adap->dev.parent)) > > return; > > > > - status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, > > + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, > > + ACPI_I2C_MAX_SCAN_DEPTH, > > acpi_i2c_add_device, NULL, > > adap, NULL); > > On my systems (which admittedly all define their i2c clients below the > controller) this works as expected, i.e. there's no change in behavior. > As far as I can tell it more accurately implements the spec. > > > However, it doesn't quite solve my problem. When > acpi_i2c_register_devices(adap) is called on the "virtual" controller > that is created for an i2c mux channel, the adap->dev.parent (set to the > parent i2c bus for the mux) does not have an acpi companion. That > ultimately causes acpi_i2c_add_device() to never find a match. > > I'll recap a bit since it's been a while and I've learned a few things > that might affect the discussion. For now, I'll focus on my proposed > ASL for an I2C mux using device properties. > > Lets say we have i2c hardware attached like this: > > i801 controller (PCI) > pca9548 8-channel mux (address 0x70) > lm75 temperature sensor (channel 0 on the mux with address 0x50) > > I think this is a sensible way to represent it: > > Device (MUX0) { > Name (_ADR, 0x70) This.. > Name (_HID, "PRP0001") > Name (_CRS, ResourceTemplate() > { > I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED, > AddressingMode7Bit, "^^SMB2", 0x00, > ResourceConsumer,,) > }) > Name (_DSD, Package () > { > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > Package () { > Package (2) { "compatible", "nxp,pca9548" }, > } > }) > > // MUX channel 0 > Device (CH00) { > Name (_ADR, 0x0) > > Device (TMP0) { > Name (_ADR, 0x50) ... and this are not needed. I2cSerialBus already contains the address. Also I think you need to have "PRP0001" here as well. > Name (_CRS, ResourceTemplate() > { > I2cSerialBus (0x60, ControllerInitiated, I2C_SPEED, > AddressingMode7Bit, "^CH00", 0x00, > ResourceConsumer,,) > }) > Name (_DSD, Package () > { > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > Package () { > Package (2) { "compatible", "national,lm75" }, > } > }) > } > } > } > > The new thing here is using _ADR to treat each mux channel as a device > and referencing those devices elsewhere (CH00). I arrived at this > because it seems to fit the ACPI model reasonably well* and it's easy to > implement (just like in other callers to acpi_preset_companion()) > > * by reasonably well, I think it's clear and works naturally but this > use of _ADR isn't explicitly defined in the spec [ACPI 6, Table 6-168 > (page 278)] > > Hopefully the spec ambiguity isn't too much effort to clarify. I think > it's a good change. But, perhaps it's unnecessary. Any feedback on > whether this ASL seem like the right way to go for device property i2c > muxes? Well to me your ASL looks reasonable. Usage of _ADR to specify mux channel seems natural and follows other buses which have an entry in that table. I don't know if it is forbidden to invent this kind of things that are not explicitly mentioned in the spec, though. > If not, is there an acceptable alternative? I wonder how muxes are > handled otherwise? Hopefully not ASL methods :) I cannot think of any better way. I have never seen ACPI DSDT/SSDT containing I2C mux before. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, September 30, 2015 12:43:36 PM Mika Westerberg wrote: > On Tue, Sep 29, 2015 at 04:19:12PM -0700, Dustin Byford wrote: > > Hi Mika, > > > > On Mon Aug 17 15:03, Mika Westerberg wrote: > > > I think the current code in I2C core is not actually doing the right > > > thing according the ACPI spec at least. To my understanding you can have > > > device with I2cSerialBus resource _anywhere_ in the namespace, not just > > > directly below the host controller. It's the ResourceSource attribute > > > that tells the corresponding host controller. > > > > > > I wonder if it helps if we scan the whole namespace for devices with > > > I2cSerialBus that matches the just registered adapter? Something like > > > the patch below. > > > > I've been working with the patch you suggested below. > > > > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > > > index c83e4d13cfc5..2a309d27421a 100644 > > > --- a/drivers/i2c/i2c-core.c > > > +++ b/drivers/i2c/i2c-core.c > > > > ... > > > > > static void acpi_i2c_register_devices(struct i2c_adapter *adap) > > > { > > > - acpi_handle handle; > > > acpi_status status; > > > > > > - if (!adap->dev.parent) > > > - return; > > > - > > > - handle = ACPI_HANDLE(adap->dev.parent); > > > - if (!handle) > > > + if (!adap->dev.parent || !has_acpi_companion(adap->dev.parent)) > > > return; > > > > > > - status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, > > > + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, > > > + ACPI_I2C_MAX_SCAN_DEPTH, > > > acpi_i2c_add_device, NULL, > > > adap, NULL); > > > > On my systems (which admittedly all define their i2c clients below the > > controller) this works as expected, i.e. there's no change in behavior. > > As far as I can tell it more accurately implements the spec. > > > > > > However, it doesn't quite solve my problem. When > > acpi_i2c_register_devices(adap) is called on the "virtual" controller > > that is created for an i2c mux channel, the adap->dev.parent (set to the > > parent i2c bus for the mux) does not have an acpi companion. That > > ultimately causes acpi_i2c_add_device() to never find a match. > > > > I'll recap a bit since it's been a while and I've learned a few things > > that might affect the discussion. For now, I'll focus on my proposed > > ASL for an I2C mux using device properties. > > > > Lets say we have i2c hardware attached like this: > > > > i801 controller (PCI) > > pca9548 8-channel mux (address 0x70) > > lm75 temperature sensor (channel 0 on the mux with address 0x50) > > > > I think this is a sensible way to represent it: > > > > Device (MUX0) { > > Name (_ADR, 0x70) > > This.. > > > Name (_HID, "PRP0001") > > Name (_CRS, ResourceTemplate() > > { > > I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED, > > AddressingMode7Bit, "^^SMB2", 0x00, > > ResourceConsumer,,) > > }) > > Name (_DSD, Package () > > { > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > Package () { > > Package (2) { "compatible", "nxp,pca9548" }, > > } > > }) > > > > // MUX channel 0 > > Device (CH00) { > > Name (_ADR, 0x0) > > > > Device (TMP0) { > > Name (_ADR, 0x50) > > ... and this are not needed. I2cSerialBus already contains the address. > > Also I think you need to have "PRP0001" here as well. The idea is to use _ADR kind of instead of "PRP0001" to express the "you don't need a driver for this" idea AFAICS. > > Name (_CRS, ResourceTemplate() > > { > > I2cSerialBus (0x60, ControllerInitiated, I2C_SPEED, > > AddressingMode7Bit, "^CH00", 0x00, > > ResourceConsumer,,) > > }) > > Name (_DSD, Package () > > { > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > Package () { > > Package (2) { "compatible", "national,lm75" }, > > } > > }) > > } > > } > > } > > > > The new thing here is using _ADR to treat each mux channel as a device > > and referencing those devices elsewhere (CH00). I arrived at this > > because it seems to fit the ACPI model reasonably well* and it's easy to > > implement (just like in other callers to acpi_preset_companion()) > > > > * by reasonably well, I think it's clear and works naturally but this > > use of _ADR isn't explicitly defined in the spec [ACPI 6, Table 6-168 > > (page 278)] Yes, it is a gray area, but I think it is reasonable. > > Hopefully the spec ambiguity isn't too much effort to clarify. I think > > it's a good change. But, perhaps it's unnecessary. Any feedback on > > whether this ASL seem like the right way to go for device property i2c > > muxes? > > Well to me your ASL looks reasonable. Usage of _ADR to specify mux > channel seems natural and follows other buses which have an entry in > that table. I don't know if it is forbidden to invent this kind of > things that are not explicitly mentioned in the spec, though. > > > If not, is there an acceptable alternative? I wonder how muxes are > > handled otherwise? Hopefully not ASL methods :) > > I cannot think of any better way. I have never seen ACPI DSDT/SSDT > containing I2C mux before. Me neither. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 30, 2015 at 02:52:28PM +0200, Rafael J. Wysocki wrote: > > > Device (CH00) { > > > Name (_ADR, 0x0) > > > > > > Device (TMP0) { > > > Name (_ADR, 0x50) > > > > ... and this are not needed. I2cSerialBus already contains the address. > > > > Also I think you need to have "PRP0001" here as well. > > The idea is to use _ADR kind of instead of "PRP0001" to express the "you > don't need a driver for this" idea AFAICS. But I think it needs a driver and it even includes "compatible" string below :-) > > > Name (_CRS, ResourceTemplate() > > > { > > > I2cSerialBus (0x60, ControllerInitiated, I2C_SPEED, > > > AddressingMode7Bit, "^CH00", 0x00, > > > ResourceConsumer,,) > > > }) > > > Name (_DSD, Package () > > > { > > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > > Package () { > > > Package (2) { "compatible", "national,lm75" }, > > > } > > > }) > > > } > > > } > > > } -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed Sep 30 16:57, Mika Westerberg wrote: > On Wed, Sep 30, 2015 at 02:52:28PM +0200, Rafael J. Wysocki wrote: > > > > Device (CH00) { > > > > Name (_ADR, 0x0) > > > > > > > > Device (TMP0) { > > > > Name (_ADR, 0x50) > > > > > > ... and this are not needed. I2cSerialBus already contains the address. > > > > > > Also I think you need to have "PRP0001" here as well. > > > > The idea is to use _ADR kind of instead of "PRP0001" to express the "you > > don't need a driver for this" idea AFAICS. > > But I think it needs a driver and it even includes "compatible" string > below :-) Right, CH00 has just an _ADR, but TMP0 really needs a PRP0001. > > > > Name (_CRS, ResourceTemplate() > > > > { > > > > I2cSerialBus (0x60, ControllerInitiated, I2C_SPEED, > > > > AddressingMode7Bit, "^CH00", 0x00, > > > > ResourceConsumer,,) > > > > }) > > > > Name (_DSD, Package () > > > > { > > > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > > > Package () { > > > > Package (2) { "compatible", "national,lm75" }, > > > > } > > > > }) > > > > } > > > > } > > > > } --Dustin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index c83e4d13cfc5..2a309d27421a 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -94,27 +94,40 @@ struct gsb_buffer { }; } __packed; -static int acpi_i2c_add_resource(struct acpi_resource *ares, void *data) +struct acpi_i2c_lookup { + struct i2c_board_info *info; + acpi_handle adapter_handle; + acpi_handle device_handle; +}; + +static int acpi_i2c_find_address(struct acpi_resource *ares, void *data) { - struct i2c_board_info *info = data; + struct acpi_i2c_lookup *lookup = data; + struct i2c_board_info *info = lookup->info; + struct acpi_resource_i2c_serialbus *sb; + acpi_handle adapter_handle; + acpi_status status; - if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) { - struct acpi_resource_i2c_serialbus *sb; + if (info->addr || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS) + return 1; - sb = &ares->data.i2c_serial_bus; - if (!info->addr && sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) { - info->addr = sb->slave_address; - if (sb->access_mode == ACPI_I2C_10BIT_MODE) - info->flags |= I2C_CLIENT_TEN; - } - } else if (!info->irq) { - struct resource r; + sb = &ares->data.i2c_serial_bus; + if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C) + return 1; - if (acpi_dev_resource_interrupt(ares, 0, &r)) - info->irq = r.start; + /* + * Extract the ResourceSource and make sure that the handle matches + * with the I2C adapter handle. + */ + status = acpi_get_handle(lookup->device_handle, + sb->resource_source.string_ptr, + &adapter_handle); + if (ACPI_SUCCESS(status) && adapter_handle == lookup->adapter_handle) { + info->addr = sb->slave_address; + if (sb->access_mode == ACPI_I2C_10BIT_MODE) + info->flags |= I2C_CLIENT_TEN; } - /* Tell the ACPI core to skip this resource */ return 1; } @@ -123,6 +136,8 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level, { struct i2c_adapter *adapter = data; struct list_head resource_list; + struct acpi_i2c_lookup lookup; + struct resource_entry *entry; struct i2c_board_info info; struct acpi_device *adev; int ret; @@ -135,14 +150,37 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level, memset(&info, 0, sizeof(info)); info.fwnode = acpi_fwnode_handle(adev); + memset(&lookup, 0, sizeof(lookup)); + lookup.adapter_handle = ACPI_HANDLE(adapter->dev.parent); + lookup.device_handle = handle; + lookup.info = &info; + + /* + * Look up for I2cSerialBus resource with ResourceSource that + * matches with this adapter. + */ INIT_LIST_HEAD(&resource_list); ret = acpi_dev_get_resources(adev, &resource_list, - acpi_i2c_add_resource, &info); + acpi_i2c_find_address, &lookup); acpi_dev_free_resource_list(&resource_list); if (ret < 0 || !info.addr) return AE_OK; + /* Then fill IRQ number if any */ + ret = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); + if (ret < 0) + return AE_OK; + + resource_list_for_each_entry(entry, &resource_list) { + if (resource_type(entry->res) == IORESOURCE_IRQ) { + info.irq = entry->res->start; + break; + } + } + + acpi_dev_free_resource_list(&resource_list); + adev->power.flags.ignore_parent = true; strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type)); if (!i2c_new_device(adapter, &info)) { @@ -155,6 +193,8 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level, return AE_OK; } +#define ACPI_I2C_MAX_SCAN_DEPTH 32 + /** * acpi_i2c_register_devices - enumerate I2C slave devices behind adapter * @adap: pointer to adapter @@ -165,17 +205,13 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level, */ static void acpi_i2c_register_devices(struct i2c_adapter *adap) { - acpi_handle handle; acpi_status status; - if (!adap->dev.parent) - return; - - handle = ACPI_HANDLE(adap->dev.parent); - if (!handle) + if (!adap->dev.parent || !has_acpi_companion(adap->dev.parent)) return; - status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, + ACPI_I2C_MAX_SCAN_DEPTH, acpi_i2c_add_device, NULL, adap, NULL); if (ACPI_FAILURE(status))