Attention is currently required from: Arthur Heymans, Tim Wawrzynczak, Patrick Rudolph. Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59360 )
Change subject: soc/intel/alderlake, soc/common: Add method to determine the cpu type ......................................................................
Patch Set 4:
(5 comments)
File src/soc/intel/common/block/acpi/Kconfig:
https://review.coreboot.org/c/coreboot/+/59360/comment/714efd7c_56287033 PS3, Line 50: Generate CPPCv3 entries for Intel hybrid processors
That is not what the code does in this CL.
Ack
File src/soc/intel/common/block/acpi/cpu_hybrid.c:
https://review.coreboot.org/c/coreboot/+/59360/comment/2a8fad4b_55f5ea25 PS1, Line 25: return global_cpu_type_bitmask;
The bitmask is updated in the multi core context, but accessed by only BSP. […]
set_cpu_type_bitmask() is run in multi core context during BS_DEV_INIT_CHIPS, but global_cpu_type_bitmaks() is run by BSP during BS_WRITE_TABLES. As these 2 function get executed during different times of execution, so global_cpu_type_bitmaks() is threadsafe.
File src/soc/intel/common/block/acpi/cpu_hybrid.c:
https://review.coreboot.org/c/coreboot/+/59360/comment/2e9e43cf_5d8fccb7 PS3, Line 37: set_cpu_type_bitmask
Is it really useful to encode this information in a uint32_t? It won't scale past 32 big cores? Why […]
I agree with you, since bitmask is 32 bits, it wont' scale up beyond 32 cores. It did without using bitmask this time. I have used array to map index to cpu type information.
Why not add the cpu type to struct device?
It will not help as the CPPCv3 package has to exposed in the ascending order of LAPIC Ids ( core which has smaller LAPIC id value should be referred as CPU#0 , next immediate bigger LAPIC id's core id should be CPU#1 ..).
In the CPPC3 package all elements are same across all CPUs except nominal performance whose values depends on the core type scaling factor. While exposing a CPPC3 package for a CPU, coreboot has to populate the nominal performance correctly as per CPU type. Hence, having mapping between core index to core type helps to populate the CPPCv3 package information correctly.
https://review.coreboot.org/c/coreboot/+/59360/comment/d6691077_50d757f9 PS3, Line 43: get_cpu_index
global_cpu_type_bitmaks will overflow if there are more than 32 CPUs.
Ack
File src/soc/intel/common/block/include/intelblocks/acpi.h:
https://review.coreboot.org/c/coreboot/+/59360/comment/adbdb475_bf4157ff PS3, Line 11: : #define CPUID_CORE_TYPE_INTEL_ATOM 0x20 : #define CPUID_CORE_TYPE_INTEL_CORE 0x40
Maybe use an enum for this and also in the return value for the functions.
Ack