Attention is currently required from: Nico Huber, Paul Menzel, Tim Wawrzynczak, Arthur Heymans, Michael Niewöhner. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58221 )
Change subject: soc/intel/cannonlake: Enable Energy/Performance Bias control ......................................................................
Patch Set 1:
(2 comments)
Patchset:
PS1:
Sorry, I was wrong. Even FSP sets it by default (EnergyEfficientPState is 1 in bsf). […]
Um, CB:34458 already took care of it.
File src/soc/intel/cannonlake/cpu.c:
https://review.coreboot.org/c/coreboot/+/58221/comment/4ea522b5_38d9fd93 PS1, Line 54: 1 << 18
tbh I'd prefer if these bits were defined where MSR_POWER_CTL is, but since the others aren't too, l […]
I don't think it's worth defining a macro for something used only once, as a comment can already provide sufficient information without the hassle of having to find and open the file with the macro definition.
However, this isn't the case for platforms using soc/intel/common MSR definitions; if the bit definitions are the same for all supported CPUs, then they could be defined once in there. I still believe it's not worth the hassle in this case.
I don't think the same applies to macros for MSR numbers and, by extension, register offsets. Finding out which occurrences of a magic number correspond to accesses to the register one is looking for isn't trivial, having a defined name makes this significantly easier. And since bit definitions are associated to a register, one can check the places where a register is being accessed to find out if a given bit is written to, even if the bit doesn't have a defined name.
TL;DR: I like the current approach.