Attention is currently required from: Subrata Banik, Paul Menzel. Tarun Tuli has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63969 )
Change subject: soc/intel/alderlake: provide a list of D-states to enter LPM ......................................................................
Patch Set 12:
(4 comments)
File src/soc/intel/alderlake/acpi.c:
https://review.coreboot.org/c/coreboot/+/63969/comment/1691398d_8fe89fce PS12, Line 179: min_pci_d_states
wondering if we have any way to reduce this static array. […]
Yes we could instead add statements for each device to populate at run time if we feel that improves clarity/readability. I'm somewhat indifferent on which may be cleaner so am open to the consensus.
In your second suggestion, are you proposing to include the D-states within the devicetree itself and propagating those up?
https://review.coreboot.org/c/coreboot/+/63969/comment/818b0aeb_32964f1f PS12, Line 470:
does it make sense to check if device is actually enabled or not ? if not then set to `undef`?
For clarification, we basically won't emit anything if the device is not enabled. Although the structure has an enabled field, it isn't really used for us.
Tim and I did discuss briefly and decided it would be cleaner than have a table also filled with disabled devices as it doesn't add much to the insight of our LPM constraints.
https://review.coreboot.org/c/coreboot/+/63969/comment/c241b9fb_b0113d78 PS12, Line 524: if (min_state == UNDEF) : min_state = D0;
looks like you are setting min_state = D0 for all devices with UNDEF ? in that case, do we still nee […]
The way I probably should have handled this is soc_lpi_include_device() should check for this condition (e.g. a device is enabled, but doesn't have it's D-state defined) and return false if that scenario. In that way, we shouldn't emit anything for that particular device and likely should just report a error instead.
File src/soc/intel/common/block/acpi/pep.c:
https://review.coreboot.org/c/coreboot/+/63969/comment/86f08892_f14d3eb2 PS12, Line 115: /* SoC returned more than zero devices so use those. */ : printk(BIOS_INFO, "Returning SoC specific constraint package for %d devices\n", num_devices)
is this for debug purpose ?
Yes. Will just show on boot how many devices we have emitted in this table.