Furquan Shaikh 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. Shared power resource: Would both the devices be children of the same power resource? What if they live on completely different pci buses? i.e. devices on pci 14.1 and pci 14.2 sharing the same power resource. How would that be handled?
2. Additionally, in case of shared power resources, would the callers require two separate power resources if only enable line is shared but not the reset line? What would be the hierarchy of devices in that case?
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 change to split addition of power resource and power states, does it help the caller to know if power resource addition was successful?
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/acpi_device.c@... PS7, Line 569: 2 ARRAY_SIZE(dev_states)
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?
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.
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. What do you think about renaming:
acpi_add_power_res --> acpi_declare_power_res acpi_add_scoped_power_res --> acpi_declare_scoped_power_res acpi_add_power_states --> acpi_add_device_power_res
Power state is a very different thing as per ACPI Spec -- _PS0, _PS1, ... _PSC, etc.
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:
acpigen_write_declare_power_res acpigen_write_add_power_res
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?
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... PS7, Line 364: power_states device_res
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 this new structure.
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 3: depends on HAVE_ACPI_TABLES
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.
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?
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?
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 20: string Is string.h intentionally placed after chip.h?
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 22: void static
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?
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. The assumption here is that the first child of the power resource is always going to be this device. If the device has siblings, the siblings can not use the same power resource and developers have to be careful about always putting this device as the first child of the power resource. I feel that could be error prone.
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 within the scope which is not what we want for the device?