Attention is currently required from: Nico Huber, Paul Menzel, Tim Wawrzynczak, Ravindra, Subrata Banik, Arthur Heymans, Michael Niewöhner, Patrick Rudolph. Sridhar Siricilla 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 3:
(15 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/59359/comment/b6d3bdd6_b0e2ff43 PS1, Line 7: Implements
Implement
Done
https://review.coreboot.org/c/coreboot/+/59359/comment/0bb653a0_3f2b8c34 PS1, Line 11: update
updates
Done
https://review.coreboot.org/c/coreboot/+/59359/comment/8a3468cc_9dce34a6 PS1, Line 18: generate
generates
Done
https://review.coreboot.org/c/coreboot/+/59359/comment/7a8263d1_b82c6e4d PS1, Line 20: update
updates
Done
https://review.coreboot.org/c/coreboot/+/59359/comment/8a36bd6e_f01fa8d3 PS1, Line 24: Cpu
CPU
Done
https://review.coreboot.org/c/coreboot/+/59359/comment/9cba2a0d_da94dc54 PS1, Line 25: cpu
CPU
Ack
https://review.coreboot.org/c/coreboot/+/59359/comment/7d070699_2ec4c8fc PS1, Line 25: Nominal
frequency or performance
Ack
https://review.coreboot.org/c/coreboot/+/59359/comment/ce94754d_73fd2605 PS1, Line 27: It also updates GNVS structure.
It also adds the new members to the GNVS structure, as those are consumed(?) ….
Ack
https://review.coreboot.org/c/coreboot/+/59359/comment/96f3f719_7fb4ac0a PS1, Line 29: TEST=Verified on Brya
How?
Ack
https://review.coreboot.org/c/coreboot/+/59359/comment/66ff69e6_65be3a9d PS1, Line 34: methods to generate ACPI code.
????
Ack
https://review.coreboot.org/c/coreboot/+/59359/comment/c0450f13_3a436a93 PS1, Line 36: Change-Id: Icd5ea9e70bebd1e66d3cea2bcf8a6678e5cc95ca
One Change-Id should be enough.
Ack
File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/59359/comment/4ab79980_c0e79d14 PS1, Line 376: int
unsigned int? But the signature of `cpu_init_cppc_config()` seems to use `uint32_t`.
Ack
File src/soc/intel/common/block/acpi/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/59359/comment/1c4a1dd0_b8c873f3 PS1, Line 28: SFBC, 16, // 0x48 - 0x49 Indicates Scaling factor for Big core : SFSC, 16, // 0x50 - 0x51 Indicates Scaling Factor for Small Core : NMFQ, 16, // 0x52 - 0x53 Indicates Nominal Frequency : CORE, 32, // 0x54 - 0x57 Each marked bit indicates Big Core corresponding to core index : INFS, 8, // 0x58 - Nominal Frequency is supported :
but with coreboot producing AML at runtime, it is rarely actually required to use GNVS, and we pre […]
Correct, I have updated the patch without using GNVS. Thanks!
File src/soc/intel/common/block/acpi/cpu_hybrid.c:
https://review.coreboot.org/c/coreboot/+/59359/comment/a44b5870_b4b91ca9 PS1, Line 66: pscope = (char *) malloc(12);
You can just allocate these on the stack. No need for malloc.
Ack
File src/soc/intel/common/block/acpi/cpu_hybrid.c:
https://review.coreboot.org/c/coreboot/+/59359/comment/3dd7e7ab_f2aa2ab7 PS3, Line 63: get_cpu_type_bitmask global_cpu_type_bitmask can be accessed by acpi_get_cpu_hybrid_info() directly, so get_cpu_type_bitmask() will be removed in later patch.