Attention is currently required from: Maulik V Vaghela, Sugnan Prabhu S, Tim Wawrzynczak, Sridhar Siricilla, Patrick Rudolph. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59124 )
Change subject: soc/intel/common: Add CPU related APIs ......................................................................
Patch Set 1:
(12 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/59124/comment/b1ddf10e_31dd3efe PS1, Line 9: The patch defines APIs to get CPU max non-turbo ratio, bus frequency and nit: possible to add just function name and one liner to make it easy to understand looking at commit msg itself ?
https://review.coreboot.org/c/coreboot/+/59124/comment/3d31b513_afad8e5a PS1, Line 12: Verified on Brya can you please complete the sentence ?
File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/59124/comment/7b94cd14_0ce777c2 PS1, Line 277: Add a help text ? /* * PLATFORM_INFO MSR (0xCE) Bits 15:8 tells * MAX_NON_TURBO_LIM_RATIO. */
https://review.coreboot.org/c/coreboot/+/59124/comment/aa6989e1_0fc21eaf PS1, Line 190: int looking at return statement at line 196 below, shouldn't this be "bool" ?
https://review.coreboot.org/c/coreboot/+/59124/comment/e5092b21_d60e2cae PS1, Line 192: res may be ? cpuid_regs
https://review.coreboot.org/c/coreboot/+/59124/comment/5bfd8157_287d5f2f PS1, Line 194: 7 can you please use a macro ?
https://review.coreboot.org/c/coreboot/+/59124/comment/0ed14518_18f5e59e PS1, Line 195: BIT(15) can you please use a meaningful macro for this ?
https://review.coreboot.org/c/coreboot/+/59124/comment/ff057c2b_83b9c820 PS1, Line 198: FALSE in that way, you don't need if and else separately
return !!(cpuid_regs.edx & BIT(15));
https://review.coreboot.org/c/coreboot/+/59124/comment/6e3221ca_e484d76c PS1, Line 204: ecx may be ?
return cpuid_ecx(CPUID_PROCESSOR_FREQUENCY);
https://review.coreboot.org/c/coreboot/+/59124/comment/75158e00_0fb7389d PS1, Line 301: (uint8_t) you don't need this typecasting.
Also, thinking if you can refactor https://github.com/coreboot/coreboot/blob/master/src/soc/intel/common/block/... to make use of your function as below
/* * Set PERF_CTL MSR (0x199) P_Req with * cpu_get_max_non_turbo_ratio() value. */ void cpu_set_p_state_to_max_non_turbo_ratio(void) { msr_t perf_ctl;
perf_ctl.lo = cpu_get_max_non_turbo_ratio(); perf_ctl.hi = 0;
set_perf_control_msr(perf_ctl); }
File src/soc/intel/common/block/include/intelblocks/cpulib.h:
https://review.coreboot.org/c/coreboot/+/59124/comment/06a8ff5e_6c6fe863 PS1, Line 15: unsigned int uint32_t ?
https://review.coreboot.org/c/coreboot/+/59124/comment/26c256bb_6017ad75 PS1, Line 21: int bool ?