Attention is currently required from: Arthur Heymans, Nico Huber, Paul Menzel, Ravindra, Sridhar Siricilla, Subrata Banik, Michael Niewöhner, Patrick Rudolph. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59359 )
Change subject: soc/intel/common: Implement ACPI CPPCv3 package to support hybrid core ......................................................................
Patch Set 7:
(7 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/59359/comment/48291d36_0b55bd74 PS7, Line 15: XPPPC `XPPC`
File src/soc/intel/common/block/acpi/cpu_hybrid.c:
https://review.coreboot.org/c/coreboot/+/59359/comment/71360317_a7402d40 PS7, Line 26: u8 global_cpu_type[MAX_CPU_COUNT]; please don't reach into other modules' data; use an accessor function
https://review.coreboot.org/c/coreboot/+/59359/comment/1faaf074_0343345b PS7, Line 62: void __weak get_cpu_scaling_factor(u16 *big_core_scal_factor, u16 *small_core_scal_factor) : { : } : : bool __weak cpu_is_nominal_freq_supported(void) : { : return 0; : } I don't think the weak definitions are necessary; this file is already guarded by the Kconfig SOC_INTEL_COMMON_BLOCK_ACPI_CPU_HYBRID, so if an SoC selects that, then it also has to implement these methods as part of that contract.
https://review.coreboot.org/c/coreboot/+/59359/comment/c2f0aa52_fabb3d1a PS7, Line 115: 12 `sizeof(scope)`
https://review.coreboot.org/c/coreboot/+/59359/comment/56ba0c09_43f7ecc1 PS7, Line 118: 16 `sizeof(scope)`
File src/soc/intel/common/block/include/intelblocks/acpi.h:
https://review.coreboot.org/c/coreboot/+/59359/comment/7de508d4_b6e10bda PS7, Line 21: /* It gets scaling factor for small and big core */ Since this has to be implemented per-SoC, I'd add a comment that mentions that and also prefix it with `soc_`
https://review.coreboot.org/c/coreboot/+/59359/comment/c2bf418f_11ef764c PS7, Line 139: bool cpu_is_nominal_freq_supported(void); Since this has to be implemented per-SoC, I'd add a comment that mentions that and also prefix it with `soc_`