Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44137 )
Change subject: soc/intel/skylake: acpi: drop HWP's dependency on EIST ......................................................................
Patch Set 3:
(1 comment)
The basis of the change (i.e., that the chip features are independent) is valid, though I'm not clear on the benefit/need unless there's some Intel chips that now support HWP but not EIST. Most of the Intel-based systems that don't set "eist_enable" in their devicetree are probably doing it by accidental oversight, e.g.:
https://review.coreboot.org/c/coreboot/+/29664
particularly since coreboot is most often used by Linux (where the pstate driver typical forces on EIST or HWP) vs Windows (which expects the features to be advertised/enable by the firmware before the OS attempts to use them). In the past there's been some informal chats before about whether eist_enable should be enabled by default or disappear entirely. The current code does force an unnecessary relationship between HWP and EIST though it does help to force people to remember to enable EIST.
What I describe here is mostly trying to provide some background/content on the situation rather than be an argument against the change. IMO, the the current way these features are structured in coreboot aren't ideal but addressing them would be a much bigger change that's probably outside the scope of what anyone wants to do right now.
https://review.coreboot.org/c/coreboot/+/44137/3/src/soc/intel/skylake/acpi.... File src/soc/intel/skylake/acpi.c:
https://review.coreboot.org/c/coreboot/+/44137/3/src/soc/intel/skylake/acpi.... PS3, Line 402: { I'm not knowledgable about the coreboot coding standard, though in general I think it tries to avoid braces on single-line statements.
-
Matt Delco (Code Review)