Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35036 )
Change subject: arch/x86/acpi: Refactor power resource-related functionality ......................................................................
Patch Set 13: Code-Review+1
(8 comments)
Mostly nits. Overall LGTM.
https://review.coreboot.org/c/coreboot/+/35036/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35036/13//COMMIT_MSG@10 PS13, Line 10: reset GPIO Wouldn't it be wrong to have a design where two devices share only the reset GPIO? The reason that enable GPIO sharing is okay is because that is the first thing in power sequencing. So, power resources for one device can be enabled independently even with virtual resources in place. However, if reset GPIO is shared, you would always violate the power sequencing since one device will be enabled before the other.
https://review.coreboot.org/c/coreboot/+/35036/13//COMMIT_MSG@11 PS13, Line 11: say one has a much longer : reset_enable_delay time than the other Maybe I am nitpicking, but the boot time impact would be the same irrespective of whether the power resources are separate or shared. Kernel doesn't have the construct of saying we have already spent T1 amount of time from enable->reset for device 1, so we just need to spend T2 - T1 before deasserting reset for device 2.
https://review.coreboot.org/c/coreboot/+/35036/13//COMMIT_MSG@13 PS13, Line 13: helps How? The commit message talks about the motivation and the goal for doing this change, but not what the change really is.
https://review.coreboot.org/c/coreboot/+/35036/13//COMMIT_MSG@17 PS13, Line 17: It would be good to have a BUG to track the work you are doing.
https://review.coreboot.org/c/coreboot/+/35036/13/src/arch/x86/acpigen.c File src/arch/x86/acpigen.c:
https://review.coreboot.org/c/coreboot/+/35036/13/src/arch/x86/acpigen.c@110... PS13, Line 1109: Can you please add a comment indicating what this is actually writing in ASL?
https://review.coreboot.org/c/coreboot/+/35036/13/src/arch/x86/include/arch/... File src/arch/x86/include/arch/acpi_device.h:
https://review.coreboot.org/c/coreboot/+/35036/13/src/arch/x86/include/arch/... PS13, Line 399: Add Declare
https://review.coreboot.org/c/coreboot/+/35036/13/src/arch/x86/include/arch/... PS13, Line 407: add nit: declare
https://review.coreboot.org/c/coreboot/+/35036/13/src/arch/x86/include/arch/... PS13, Line 412: Adds nit: Declares