Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35036 )
Change subject: drivers/generic: Add new driver for power resource pseudo device ......................................................................
Patch Set 7:
(19 comments)
https://review.coreboot.org/c/coreboot/+/35036/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35036/7//COMMIT_MSG@17 PS7, Line 17: chip drivers/generic/power_resource : register "enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_A1)" : device generic 0 on : chip drivers/i2c/generic : register ... : end : end : end
How do you envision handling cases like: […]
1) Involves more work, which I have planned for future patches. Right now, they would have to be separate power resources. 2) What comes to mind for me would be to create separate "virtual" power resources, one for each GPIO (enable, reset, stop), link them all in _PR0/_PR3, and use the 'Level' parameter to dictate the order they execute in.
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/acpi_device.c@... PS7, Line 550: void
Just wondering if this should return a value indicating if the power resource was added? With this c […]
Good idea, can return a bool.
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/acpi_device.c@... PS7, Line 569: 2
ARRAY_SIZE(dev_states)
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/acpi_device.c@... PS7, Line 573: const char *name,
This might fit on the above line within the character count limit?
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpi_device.h:
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... PS7, Line 411: const char *name,
This will fit on the previous line.
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... PS7, Line 418: acpi_add_power_states
Technically, this is adding power resource for a device as per the definition of _PR0 and _PR3. […]
That's a very fair point, I like your suggestion.
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpigen.h:
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... PS7, Line 362: acpigen_write_power_res
... and then these would have to be: […]
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... PS7, Line 363: power_resource_name
This can just be name?
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... PS7, Line 364: power_states
device_res
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... File src/drivers/generic/power_resource/Kconfig:
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 1: DRIVERS_GENERIC_POWER_RESOURCE
I think this should ultimately be auto-selected by i2c and spi drivers once you move all boards to t […]
Ack
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 3:
depends on HAVE_ACPI_TABLES
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... File src/drivers/generic/power_resource/chip.h:
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 22: extern struct chip_operations drivers_generic_power_resource_ops;
I don't think this is required.
Either that or extern it in i2c/generic.c and spi/acpi.c to know if the parent device is a power resource or not
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 47: void power_resource_fill_ssdt(struct device *dev);
Why is this exposed outside this driver?
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... File src/drivers/generic/power_resource/power_resource.c:
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 18: #include <console/console.h>
Is this required?
It is now with a print for failing to add the power resource.
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 20: string
Is string.h intentionally placed after chip. […]
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 22: void
static
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/i2c/generic/gen... File src/drivers/i2c/generic/generic.c:
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/i2c/generic/gen... PS7, Line 63: #if CONFIG(DRIVERS_GENERIC_POWER_RESOURCE)
Any reason not to use if (CONFIG(DRIVERS_GENERIC_POWER_RESOURCE)) instead of #if?
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/i2c/generic/gen... PS7, Line 65: dev->bus->children == dev
This is going to be tricky. […]
That's a good point, it seems now like this check is not really doing anything; really just want to check that the dev->bus->dev->chip_ops is pointing to the correct object; it can be anywhere in the list of child devices.
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/i2c/generic/gen... PS7, Line 73: scope
Is this being put in because the way acpi_device_scope() works, it will add power resource device wi […]
Yes. I can add a comment; otherwise acpigen_add_i2c() will use the incorrect resource and the kernel will not be able to locate the I2C controller.