Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36864 )
Change subject: soc/intel/cannonlake: Add chip config to override CPU flex ratio ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36864/3/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/36864/3/src/soc/intel/cannonlake/ch... PS3, Line 428: /* : * Override CPU ratio value: : * 0: Use FIT programmed default value, : * 1: coreboot to override CPU flex ratio : */ : uint8_t cpu_ratio_override; This is not what you code does. Any non zero value will allow overrides.
https://review.coreboot.org/c/coreboot/+/36864/3/src/soc/intel/cannonlake/ch... PS3, Line 437: * CPU ratio value controls the maximum processor non-turbo ratio : * Valid Range 0 to 63. CPU Ratio is 0 when disabled What does it even mean for a CPU ratio to be disabled?
https://review.coreboot.org/c/coreboot/+/36864/2/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/36864/2/src/soc/intel/cannonlake/ro... PS2, Line 80: if (CONFIG(OVERRIDE_CPU_FLEX_RATIO))
we could do that for sure but trying to leverage more from existing setting without writing code into CB.
In older code the flex ratio is set based on other MSR's, MSR_CONFIG_TDP_NOMINAL. Can the same not be done here too?
yes, you are right, in old code we have flex ratio programming in bootblock itself but that was intended to boot with HFM (the same we already achieving using FIT tool setting the offset), thats the reason we have removed those flex ratio programming code in coreboot.
Now after debug looks like we are seeing ~100ms saving in depthcharge/vboot_reference code whle running a for loop to verify kernel if we are running at lower freq than HFM. hence we are trying to set the lower freq of boot using said UPD.
Having yet another Kconfig/devicetree value should be avoided especially since it is hardly mainboard specific.
Anyway we are filling UPD to tell FSP what is CPU ratio we are booting to avoid cpu ratio override now to save ~100ms (which might be platform specific) we are overriding FIT programmed flex ratio in coreboot
The integration documentation is way too vague to understand what this is doing. What is the reset default of flex_ratio? If it's always 0 then this code is just bogus.