John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34145 )
Change subject: src/cpu/intel: Add sanity check for cpu turbo mode capability ......................................................................
Patch Set 6:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34145/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34145/5//COMMIT_MSG@9 PS5, Line 9: properly
proper
Ack
https://review.coreboot.org/c/coreboot/+/34145/5//COMMIT_MSG@9 PS5, Line 9: intended : for enabling.
selected to be enabled
Ack
https://review.coreboot.org/c/coreboot/+/34145/5/src/cpu/intel/turbo/turbo.c File src/cpu/intel/turbo/turbo.c:
https://review.coreboot.org/c/coreboot/+/34145/5/src/cpu/intel/turbo/turbo.c... PS5, Line 107: cpuid_regs = cpuid(CPUID_LEAF_PM); : turbo_cap = !!(cpuid_regs.eax & PM_CAP_TURBO_MODE);
Why is this function checking cap again since get_turbo_state() already does that?
It is intended if get_turbo_state() results in TURBO_DISABLED as "if (!turbo_cap && !turbo_en)" scenario. enable_turbo() then intends to enable turbo and check its status.
https://review.coreboot.org/c/coreboot/+/34145/5/src/cpu/intel/turbo/turbo.c... PS5, Line 112: turbo_state = TURBO_UNAVAILABLE;
get_turbo_state() in line 101 ensures that turbo_state is not TURBO_UNAVAILABLE. […]
If get_turbo_state() in line 101 ensures that turbo_state is NOT TURBO_UNAVAILABLE, get_turbo_state then updates turbo_state to either TURBO_ENABLED (fsp already enabled turbo) or TURBO_DISABLED (fsp does not enable turbo). Only in the case of TURBO_DISABLED, enable_turbo() tries to enable turbo, checks its status and updates turbo_state properly.
https://review.coreboot.org/c/coreboot/+/34145/5/src/cpu/intel/turbo/turbo.c... PS5, Line 120: turbo_state = get_global_turbo_state();
Else case doesn't make much sense. […]
Ack