Attention is currently required from: Subrata Banik, Tim Wawrzynczak, 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 13:
(9 comments)
File src/soc/intel/alderlake/acpi.c:
https://review.coreboot.org/c/coreboot/+/63969/comment/d9afeb7e_d5accc16 PS12, Line 179: min_pci_d_states
To me, I think it makes more sense to store a map of PCI device/path to its required D-state, e.g. […]
This is actually what I started with. I went with the index hash approach as it ends up in an overall simpler implementation and faster lookup (as we probably can't guarantee any order in this approach). The cost is something that ends up being less readable.
It seems the trade-off may be worth it so let me change it back over.
https://review.coreboot.org/c/coreboot/+/63969/comment/5e5a8c2e_08f01295 PS12, Line 470:
soc_lpi_include_device() already includes a check for that.
Yup precisely!
File src/soc/intel/alderlake/acpi.c:
https://review.coreboot.org/c/coreboot/+/63969/comment/0a8268ff_7d3ab9f2 PS13, Line 23: #define CPU_PATH_PREFIX "\_SB.CP"
See `CONFIG_ACPI_CPU_STRING`
Ack. Thanks for the hint.
https://review.coreboot.org/c/coreboot/+/63969/comment/c326577a_da63cf16 PS13, Line 116: typedef enum { : D0, /* 0 */ : D1, /* 1 */ : D2, /* 2 */ : D3, /* 3 */ : UNDEF : } D_STATES;
In coreboot, we don't usually use a lot of typedefs, and not all capitalized names, either so […]
Ack And yes it could apply if the next suggestion is done. (no need for undef)
https://review.coreboot.org/c/coreboot/+/63969/comment/0ccb302a_8e00a6cf PS13, Line 440: if (dev && dev->enabled) { : switch (dev->path.type) { : case DEVICE_PATH_PCI: : return (min_pci_d_states[dev->path.pci.devfn] != UNDEF); : : case DEVICE_PATH_APIC: : return true; : : default: : return false; : } : } : return false;
if you take my suggestion from above, this simplifies to: […]
I believe we would still want to add a check in here that the entry we're interested in actually exists (as we don't even want to include that device in the count we emit/emit it out if it doesn't).
https://review.coreboot.org/c/coreboot/+/63969/comment/2df39651_1173ac1e PS13, Line 455: extern int cpu_get_apic_id(int logical_cpu);
`#include <cpu/cpu. […]
Ack
https://review.coreboot.org/c/coreboot/+/63969/comment/d18cb982_0dce2345 PS13, Line 499: default: : /* Unhandled */ : acpigen_emit_namestring(NULL); : break; : }
Probably print an error instead? soc_lpi_include_device() should take care of this case already.
Yes. Correct, in theory we should actually never get this, but to not overall ruin the output structure in case we ever do (since we need a default case anyways). Added a error as well though for good measure.
https://review.coreboot.org/c/coreboot/+/63969/comment/30498432_e1096bec PS13, Line 505: 1
symbolic constants (e.g. […]
Ack
https://review.coreboot.org/c/coreboot/+/63969/comment/c330744e_402b3cf2 PS13, Line 521: D0;
nit: […]
Ack