Attention is currently required from: Felix Held, Kyösti Mälkki.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78133?usp=email )
Change subject: cpu/intel/model_206ax: Print supported C-states ......................................................................
Patch Set 6:
(7 comments)
File src/arch/x86/cpu_common.c:
https://review.coreboot.org/c/coreboot/+/78133/comment/74904756_29946a39 : PS5, Line 196: int cpu_get_c_state_support(const int lvl)
Is the input parameter 'lvl' really a 'state'. […]
Moved to separate commit.
https://review.coreboot.org/c/coreboot/+/78133/comment/1f384839_f762b51e : PS5, Line 201: 1
might be good to have a define for this bit
Done
File src/cpu/intel/model_206ax/acpi.c:
https://review.coreboot.org/c/coreboot/+/78133/comment/d1f78068_cbe5f1eb : PS5, Line 103: h, l
it would make the code easier to read if you'd use 'state' and 'substate' as variable names
Done
https://review.coreboot.org/c/coreboot/+/78133/comment/d6b6a6b2_6bd18fa9 : PS5, Line 104:
maybe add a comment that C0 is always supported; was wondering why i was initialized as 1, but that […]
yes, added a comment to clarify.
https://review.coreboot.org/c/coreboot/+/78133/comment/8132fb79_55fde0dc : PS5, Line 109: > l
i'd drop this or make it != 0 and drop the + 1 in line 106 too; at least for me that would make the […]
That would not result in the same behaviour. cpu_get_c_state_support doesn't return a boolean, but an integer of supported sub-levels. Also the ACPI resource holds a C state starting at C1, so +1 must be added to get the correct CPU C-state.
https://review.coreboot.org/c/coreboot/+/78133/comment/8ad8f63c_37e22b37 : PS5, Line 110: strcpy(&str[strlen(str)], c_state_names[i]); : strcpy(&str[strlen(str)], " ");
i'd just use printk directly instead of assembling a string and then printing it. […]
I ported the code from MPinit, which runs in parallel and made a mess when using printk, but here it's fine.
https://review.coreboot.org/c/coreboot/+/78133/comment/de684a05_dc51bc55 : PS5, Line 128: print_supported_cstates();
While there is a call to cpu_get_c_state_support() with cpuid() calls, ACPI table creation only runs […]
Sounds good. Will do on the next push.