Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32188 )
Change subject: WIP: Power resource stub ......................................................................
Patch Set 2:
(21 comments)
This CL should be broken down into multiple smaller CLs:
1. Adding probed property to sconfig to support probed devices 2. Adding power resource device to device tree 3. Adding power resource driver for ACPI 4. Adding helper function for enabling power in bootblock 5. Moving all mainboards using power resource to the newly added device 6. Get rid of power resource properties from I2C generic and SPI devices. 7. Get rid of probed property
https://review.coreboot.org/#/c/32188/2/src/device/device_const.c File src/device/device_const.c:
https://review.coreboot.org/#/c/32188/2/src/device/device_const.c@94 PS2, Line 94: ) || !filter
https://review.coreboot.org/#/c/32188/2/src/device/device_const.c@183 PS2, Line 183: 0 This should use the power resource instance for comparison.
https://review.coreboot.org/#/c/32188/2/src/device/device_util.c File src/device/device_util.c:
https://review.coreboot.org/#/c/32188/2/src/device/device_util.c@236 PS2, Line 236: snprintf(buffer, sizeof(buffer), "POWER_RES"); break;
https://review.coreboot.org/#/c/32188/2/src/device/device_util.c@236 PS2, Line 236: " print power resource instance as well.
https://review.coreboot.org/#/c/32188/2/src/drivers/generic/power_resource/M... File src/drivers/generic/power_resource/Makefile.inc:
https://review.coreboot.org/#/c/32188/2/src/drivers/generic/power_resource/M... PS2, Line 1: y $(DRIVERS_GENERIC_POWER_RESOURCE)
https://review.coreboot.org/#/c/32188/2/src/drivers/generic/power_resource/c... File src/drivers/generic/power_resource/chip.h:
https://review.coreboot.org/#/c/32188/2/src/drivers/generic/power_resource/c... PS2, Line 19: device_power Why is this required?
https://review.coreboot.org/#/c/32188/2/src/drivers/generic/power_resource/c... PS2, Line 21: #if CONFIG(HAVE_ACPI_TABLES) No need to guard.
https://review.coreboot.org/#/c/32188/2/src/drivers/generic/power_resource/c... PS2, Line 22: You will need a config structure to associate with this device.
https://review.coreboot.org/#/c/32188/2/src/drivers/generic/power_resource/c... PS2, Line 23: power_resource_fill_ssdt Can you please provide some description as to what this function does?
https://review.coreboot.org/#/c/32188/2/src/drivers/generic/power_resource/p... File src/drivers/generic/power_resource/power_resource.h:
PS2: Why is this file required when there is a chip.h?
https://review.coreboot.org/#/c/32188/2/src/drivers/generic/power_resource/p... File src/drivers/generic/power_resource/power_resource.c:
https://review.coreboot.org/#/c/32188/2/src/drivers/generic/power_resource/p... PS2, Line 26: #if CONFIG(HAVE_ACPI_TABLES) This file should be compiled in only if HAVE_ACPI_TABLES is enabled. No need to guard here.
https://review.coreboot.org/#/c/32188/2/src/drivers/generic/power_resource/p... PS2, Line 48: fill_ssdt Why not just add the content of fill_ssdt here directly?
https://review.coreboot.org/#/c/32188/2/src/drivers/i2c/generic/chip.h File src/drivers/i2c/generic/chip.h:
https://review.coreboot.org/#/c/32188/2/src/drivers/i2c/generic/chip.h@a53 PS2, Line 53: This change needs to be done in multiple CLs: 1. Add power resource device in one CL 2. Move all mainboards using power resource to use the new device in next 3. Remove these objects in next CL
https://review.coreboot.org/#/c/32188/2/src/drivers/i2c/generic/generic.c File src/drivers/i2c/generic/generic.c:
https://review.coreboot.org/#/c/32188/2/src/drivers/i2c/generic/generic.c@27 PS2, Line 27: power_resource.h> Just include chip.h here
https://review.coreboot.org/#/c/32188/2/src/drivers/i2c/generic/generic.c@12... PS2, Line 128: pr_config && pr_config is already checked for NULL in i2c_generic_add_gpios_to_crs.
https://review.coreboot.org/#/c/32188/2/src/include/device_power.h File src/include/device_power.h:
PS2: Why is this file required? Definition for drivers_generic_power_resource_config should be part of chip.h in device/power_resource
https://review.coreboot.org/#/c/32188/2/src/lib/bootblock.c File src/lib/bootblock.c:
https://review.coreboot.org/#/c/32188/2/src/lib/bootblock.c@34 PS2, Line 34: enable_power_resources I think it would be better to guard this with a special Kconfig so that it is done only on boards that need it.
https://review.coreboot.org/#/c/32188/2/src/lib/bootblock.c@36 PS2, Line 36: power_resource_deassert_reset First step should be to enable power in bootblock. Deassert reset needs to happen in ramstage before probing.
https://review.coreboot.org/#/c/32188/2/src/lib/device_power.c File src/lib/device_power.c:
https://review.coreboot.org/#/c/32188/2/src/lib/device_power.c@21 PS2, Line 21: power_resource_deassert_reset This function should enable power to the device.
https://review.coreboot.org/#/c/32188/2/src/lib/device_power.c@34 PS2, Line 34: if (dev->link_list->children->path.type != : DEVICE_PATH_POWER_RES) It would be better to scan through all children of current device to find the power resource.
https://review.coreboot.org/#/c/32188/2/util/sconfig/main.c File util/sconfig/main.c:
https://review.coreboot.org/#/c/32188/2/util/sconfig/main.c@518 PS2, Line 518: 2 bit 1 is used by hidden property. So, this won't work. Also, why does this override enabled and hidden?
I think it would be better to use bit 2 for probed and continue using bit 0 for on/off and bit 1 for hidden. And if device status is probed, you can set it to on = 1 and probed = 1 which would set enabled and probed flags. Later after probing if device is not found then enabled can be set to 0.