Change in coreboot[master]: soc/intel/cannonlake: Add kconfig to override CPU flex ratio
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36864 )
Change subject: soc/intel/cannonlake: Add kconfig to override CPU flex ratio ......................................................................
soc/intel/cannonlake: Add kconfig to override CPU flex ratio
This patch provides options to override FIT default CPU flex ratio from coreboot code. CONFIG_CPU_FLEX_RATIO to provide the required CPU ratio and CONFIG_OVERRIDE_CPU_FLEX_RATIO to tell if override is required.
TEST=Without override flex_ratio is 0 and verified booting to OS after overriding with flex_ratio value 5.
Change-Id: Ib01650f52f3d402f669e7e7f5b28a648b86f08ec Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 17 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/36864/1
diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index 5731cff..3f7afea 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -332,4 +332,17 @@ hex default 0x40000 # 256KB
+config OVERRIDE_CPU_FLEX_RATIO + bool + help + Provide an option to override CPU ratio value. + +config CPU_FLEX_RATIO + hex + depends on OVERRIDE_CPU_FLEX_RATIO + default 0 + help + CPU ratio value controls the maximum processor non-turbo ratio. + Valid Range 0 to 63. CPU Ratio is 0 when disabled. + endif diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c index 996c135..5c0328b 100644 --- a/src/soc/intel/cannonlake/romstage/fsp_params.c +++ b/src/soc/intel/cannonlake/romstage/fsp_params.c @@ -77,7 +77,10 @@ /* Set CpuRatio to match existing MSR value */ msr_t flex_ratio; flex_ratio = rdmsr(MSR_FLEX_RATIO); - m_cfg->CpuRatio = (flex_ratio.lo >> 8) & 0xff; + if (CONFIG(OVERRIDE_CPU_FLEX_RATIO)) + m_cfg->CpuRatio = CONFIG_CPU_FLEX_RATIO; + else + m_cfg->CpuRatio = (flex_ratio.lo >> 8) & 0xff;
/* If ISH is enabled, enable ISH elements */ if (!dev)
Hello Aaron Durbin, Patrick Rudolph, Tim Wawrzynczak, Aamir Bohra, Maulik V Vaghela, Tim Wawrzynczak, Duncan Laurie, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36864
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Add kconfig to override CPU flex ratio ......................................................................
soc/intel/cannonlake: Add kconfig to override CPU flex ratio
This patch provides options to override FIT default CPU flex ratio from coreboot code. CONFIG_CPU_FLEX_RATIO to provide the required CPU ratio and CONFIG_OVERRIDE_CPU_FLEX_RATIO to tell if override is required.
TEST=Without override flex_ratio is 0 and verified booting to OS after overriding with flex_ratio value 5.
Change-Id: Ib01650f52f3d402f669e7e7f5b28a648b86f08ec Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 17 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/36864/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36864 )
Change subject: soc/intel/cannonlake: Add kconfig to override CPU flex ratio ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36864/2/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/36864/2/src/soc/intel/cannonlake/Kc... PS2, Line 334: : config OVERRIDE_CPU_FLEX_RATIO : bool : help : Provide an option to override CPU ratio value. Do we want to just add this to different boards' config struct, so it lives in devicetree instead?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36864 )
Change subject: soc/intel/cannonlake: Add kconfig to override CPU flex ratio ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36864/2/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/36864/2/src/soc/intel/cannonlake/Kc... PS2, Line 334: : config OVERRIDE_CPU_FLEX_RATIO : bool : help : Provide an option to override CPU ratio value.
Do we want to just add this to different boards' config struct, so it lives in devicetree instead?
i believe you mean to use chip.h config ?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36864 )
Change subject: soc/intel/cannonlake: Add kconfig to override CPU flex ratio ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36864/2/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/36864/2/src/soc/intel/cannonlake/Kc... PS2, Line 334: : config OVERRIDE_CPU_FLEX_RATIO : bool : help : Provide an option to override CPU ratio value.
i believe you mean to use chip. […]
Yes
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36864 )
Change subject: soc/intel/cannonlake: Add kconfig to override CPU flex ratio ......................................................................
Patch Set 2:
(1 comment)
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)) This seems backwards. Why not have coreboot program the FLEX ration and still pass on what is programmed by coreboot.
Hello Aaron Durbin, Patrick Rudolph, Tim Wawrzynczak, Aamir Bohra, Maulik V Vaghela, Tim Wawrzynczak, Duncan Laurie, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36864
to look at the new patch set (#3).
Change subject: soc/intel/cannonlake: Add chip config to override CPU flex ratio ......................................................................
soc/intel/cannonlake: Add chip config to override CPU flex ratio
This patch provides options to override FIT default CPU flex ratio from coreboot code. cpu_ratio to provide the required CPU ratio and cpu_ratio_override to tell if override is required.
TEST=Without override flex_ratio is 0 and verified booting to OS after overriding with flex_ratio value 5.
Change-Id: Ib01650f52f3d402f669e7e7f5b28a648b86f08ec Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 18 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/36864/3
Subrata Banik 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:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36864/2/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/36864/2/src/soc/intel/cannonlake/Kc... PS2, Line 334: : config OVERRIDE_CPU_FLEX_RATIO : bool : help : Provide an option to override CPU ratio value.
Yes
Done
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))
This seems backwards. […]
we could do that for sure but trying to leverage more from existing setting without writing code into CB.
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:
(1 comment)
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? Having yet another Kconfig/devicetree value should be avoided especially since it is hardly mainboard specific.
Subrata Banik 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:
(1 comment)
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
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.
Subrata Banik 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.
make sense will check with ==1
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?
Was trying to echo what ever header says.
https://github.com/coreboot/coreboot/blob/master/src/vendorcode/intel/fsp/fs...
CPU ratio value 0 means don't override
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.
https://github.com/coreboot/coreboot/blob/master/src/vendorcode/intel/fsp/fs...
What is the reset default of flex_ratio? If it's always 0 then this code is just bogus.
FIT will set it to Default 0, which means will allow CPU to boot it its HFM max, for an example: 2100MHz on some SKU.
Whereelse we wish to boot to lower freq like 1500Mhz thats the purpose of this code
Hello Aaron Durbin, Patrick Rudolph, Tim Wawrzynczak, Aamir Bohra, Maulik V Vaghela, Tim Wawrzynczak, Duncan Laurie, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36864
to look at the new patch set (#4).
Change subject: soc/intel/cannonlake: Add chip config to override CPU flex ratio ......................................................................
soc/intel/cannonlake: Add chip config to override CPU flex ratio
This patch provides options to override FIT default CPU flex ratio from coreboot code. cpu_ratio to provide the required CPU ratio and cpu_ratio_override to tell if override is required.
TEST=Without override flex_ratio is 0 and verified booting to OS after overriding with flex_ratio value 5.
Change-Id: Ib01650f52f3d402f669e7e7f5b28a648b86f08ec Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 19 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/36864/4
Subrata Banik 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 4:
(2 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;
make sense will check with ==1
Done
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
Was trying to echo what ever header says. […]
Done
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 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36864/4/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/36864/4/src/soc/intel/cannonlake/ch... PS4, Line 441: uint8_t cpu_ratio; If 0 means no override, why do you need the option cpu_ratio_override above?
Hello Aaron Durbin, Patrick Rudolph, Tim Wawrzynczak, Aamir Bohra, Maulik V Vaghela, Tim Wawrzynczak, Duncan Laurie, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36864
to look at the new patch set (#5).
Change subject: soc/intel/cannonlake: Add chip config to override CPU flex ratio ......................................................................
soc/intel/cannonlake: Add chip config to override CPU flex ratio
This patch provides options to override FIT default CPU flex ratio from coreboot code. cpu_ratio to provide the required CPU ratio.
Note: Don't override the flex ratio of cpu_ratio is 0.
TEST=Without override flex_ratio is 0 and verified booting to OS after overriding with flex_ratio value 5.
Change-Id: Ib01650f52f3d402f669e7e7f5b28a648b86f08ec Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 12 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/36864/5
Subrata Banik 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 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36864/4/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/36864/4/src/soc/intel/cannonlake/ch... PS4, Line 441: uint8_t cpu_ratio;
If 0 means no override, why do you need the option cpu_ratio_override above?
great feedback. Done
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 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36864/5/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/36864/5/src/soc/intel/cannonlake/ro... PS5, Line 79: flex_ratio = rdmsr(MSR_FLEX_RATIO); Is this still needed? If you don't want FSP to touch it then 0 should be passed?
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 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36864/5/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/36864/5/src/soc/intel/cannonlake/ch... PS5, Line 434: cpu_ratio call it cpu_ratio_override to make it clear that's optional?
Hello Aaron Durbin, Patrick Rudolph, Tim Wawrzynczak, Aamir Bohra, Maulik V Vaghela, Tim Wawrzynczak, Duncan Laurie, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36864
to look at the new patch set (#6).
Change subject: soc/intel/cannonlake: Add chip config to override CPU flex ratio ......................................................................
soc/intel/cannonlake: Add chip config to override CPU flex ratio
This patch provides options to override FIT default CPU flex ratio from coreboot code. cpu_ratio_override to provide the required CPU ratio.
Note: Don't override the flex ratio of cpu_ratio is 0.
TEST=Without override flex_ratio is 0 and verified booting to OS after overriding with flex_ratio value 5.
Change-Id: Ib01650f52f3d402f669e7e7f5b28a648b86f08ec Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 16 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/36864/6
Subrata Banik 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 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36864/5/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/36864/5/src/soc/intel/cannonlake/ro... PS5, Line 79: flex_ratio = rdmsr(MSR_FLEX_RATIO);
Is this still needed? If you don't want FSP to touch it then 0 should be passed?
please check the latest patch set. move this into else condition where cb won't override the cpu flex ratio and FSP will read MSR to know FIT programmed default value.
Subrata Banik 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 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36864/5/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/36864/5/src/soc/intel/cannonlake/ch... PS5, Line 434: cpu_ratio
call it cpu_ratio_override to make it clear that's optional?
done
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 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36864/5/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/36864/5/src/soc/intel/cannonlake/ro... PS5, Line 79: flex_ratio = rdmsr(MSR_FLEX_RATIO);
please check the latest patch set. move this into else condition where cb won't override the cpu flex ratio and FSP will read MSR to know FIT programmed default value.
No I meant why read it at all and not pass 0 to FSP if you don't want overrides?
Subrata Banik 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 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36864/5/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/36864/5/src/soc/intel/cannonlake/ro... PS5, Line 79: flex_ratio = rdmsr(MSR_FLEX_RATIO);
please check the latest patch set. […]
because FIT has option to set the default value (other than 0) as well and if an user override FIT setting to change value to something else like 5 may be then in that case, i will fake and tell FSP its always 0, which might not be correct. So the purpose is FSP will read FIT value from MSR and pass into UPD to it know what is CPU operating freq.
if i do pass 0 here forcefully then there might be gap when user set 5 in FIT and FSP is reading 0 as CB fake that.
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 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36864/5/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/36864/5/src/soc/intel/cannonlake/ro... PS5, Line 79: flex_ratio = rdmsr(MSR_FLEX_RATIO);
because FIT has option to set the default value (other than 0) as well and if an user override FIT setting to change value to something else like 5 may be then in that case, i will fake and tell FSP its always 0, which might not be correct. So the purpose is FSP will read FIT value from MSR and pass into UPD to it know what is CPU operating freq.
if i do pass 0 here forcefully then there might be gap when user set 5 in FIT and FSP is reading 0 as CB fake that.
I don't get it. 0 here just means that FSP should not override the flex_ratio whathever other mechanism like FIT set it up? Also please add some comments about the FIT mechanism in this code.
Paul Menzel 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 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36864/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36864/6//COMMIT_MSG@13 PS6, Line 13: of if
https://review.coreboot.org/c/coreboot/+/36864/6/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/36864/6/src/soc/intel/cannonlake/ch... PS6, Line 429: * Override CPU ratio value: No *flex* in the comment?
https://review.coreboot.org/c/coreboot/+/36864/6/src/soc/intel/cannonlake/ch... PS6, Line 430: * CPU ratio value controls the maximum processor non-turbo ratio Maybe add the data sheet and section, where this is described.
https://review.coreboot.org/c/coreboot/+/36864/6/src/soc/intel/cannonlake/ch... PS6, Line 432: * FSP to skip cpu_ratio override if cpu_ratio set to 0 FSP skips cpu_ratio override if cpu_ratio is 0.
Nico Huber 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 6:
(2 comments)
I have read through the comments but still don't understand a lot of things:
Please explain why a higher ratio results in a lower speed.
Please explain in detail what the `CpuRatio` UPD actually does. How does it relate to `BootFrequency`?
Also, it seems to me that the measurements are void, because the currently simplified tsc_freq_mhz() implementation doesn't support "flex ratio". I think we should implement tsc_freq_mhz() correctly first, before jumping to any conclusions.
Additionally, for the interested reader: flex ratio adapts the maximum non-turbo ratio. And is mainly a feature to influence the TSC rate...
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 79: flex_ratio = rdmsr(MSR_FLEX_RATIO); If I understand the EDS correctly, this register's contents survive a reset. So the code here doesn't make much sense. It might read a stale value from a previous boot instead.
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 […]
Can you please explain why it is faster with a lower clock speed?
Subrata Banik 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 6:
(4 comments)
Patch Set 6:
(2 comments)
I have read through the comments but still don't understand a lot of things:
Please explain why a higher ratio results in a lower speed.
Please explain in detail what the `CpuRatio` UPD actually does. How does it relate to `BootFrequency`?
Also, it seems to me that the measurements are void, because the currently simplified tsc_freq_mhz() implementation doesn't support "flex ratio". I think we should implement tsc_freq_mhz() correctly first, before jumping to any conclusions.
Please don't assume thing just because there is one issue going on where tsc_freq is in question. I can ensures that issue doesn't have anything related to this work. We have run some experiment to minimize platform power consumption by running CPU at lower speed and found that running at lower freq provides better boot time number specifically running below code
https://chromium.googlesource.com/chromiumos/platform/vboot/+/refs/heads/mas...
Additionally, for the interested reader: flex ratio adapts the maximum non-turbo ratio. And is mainly a feature to influence the TSC rate...
We are using CPU speed independent timer hence running at HFM or LFM doesn't matter to TSC
https://review.coreboot.org/c/coreboot/+/36864/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36864/6//COMMIT_MSG@13 PS6, Line 13: of
if
Done
https://review.coreboot.org/c/coreboot/+/36864/6/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/36864/6/src/soc/intel/cannonlake/ch... PS6, Line 429: * Override CPU ratio value:
No *flex* in the comment?
Done
https://review.coreboot.org/c/coreboot/+/36864/6/src/soc/intel/cannonlake/ch... PS6, Line 430: * CPU ratio value controls the maximum processor non-turbo ratio
Maybe add the data sheet and section, where this is described.
its part of SDM.
https://review.coreboot.org/c/coreboot/+/36864/6/src/soc/intel/cannonlake/ch... PS6, Line 432: * FSP to skip cpu_ratio override if cpu_ratio set to 0
FSP skips cpu_ratio override if cpu_ratio is 0.
Done
Subrata Banik 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 6:
(2 comments)
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))
Can you please explain why it is faster with a lower clock speed?
we are suspecting cache misses more while running this for loop at higher freq, comparing core/uncore/memory freq. https://chromium.googlesource.com/chromiumos/platform/vboot/+/refs/heads/mas...
also submitted CL to optimized that code in vboot
also please refer to some study doc which we are also looking at to proof this theory.
https://software.intel.com/en-us/forums/software-tuning-performance-optimiza...
https://review.coreboot.org/c/coreboot/+/36864/5/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/36864/5/src/soc/intel/cannonlake/ro... PS5, Line 79: flex_ratio = rdmsr(MSR_FLEX_RATIO);
because FIT has option to set the default value (other than 0) as well and if an user override FIT […]
please read latest patchset chip.h description, it covers your concern
Subrata Banik 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 6:
(1 comment)
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 79: flex_ratio = rdmsr(MSR_FLEX_RATIO);
If I understand the EDS correctly, this register's contents survive a reset. […]
no, not like that, this register always hold the snap shot of what cpu softstap has set. By passing CpuRatio UPD we are actually overriding that cpu softstrap which also could have programmed using FIT. But its good practice to avoid FIT and use more code to set this. Condition that it will hit a reset if FIT cpu softstrap and given overriden values are not same, So it will override new value and hit a soft reset to apply the same freq for cpu. Thats the reason its written sticky in BWG/SDM
Hello Aaron Durbin, Patrick Rudolph, Tim Wawrzynczak, Aamir Bohra, Maulik V Vaghela, Tim Wawrzynczak, Duncan Laurie, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36864
to look at the new patch set (#7).
Change subject: soc/intel/cannonlake: Add chip config to override CPU flex ratio ......................................................................
soc/intel/cannonlake: Add chip config to override CPU flex ratio
This patch provides options to override FIT default CPU flex ratio from coreboot code. cpu_ratio_override to provide the required CPU ratio.
Note: Don't override the flex ratio if cpu_ratio is 0.
TEST=Without override flex_ratio is 0 and verified booting to OS after overriding with flex_ratio value 5.
Change-Id: Ib01650f52f3d402f669e7e7f5b28a648b86f08ec Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 21 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/36864/7
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 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36864/7/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/36864/7/src/soc/intel/cannonlake/ch... PS7, Line 433: FIT tool What is FIT tool? Afaik only cbfstool knows how to add microcode FIT entries. Maybe just state which FIT entry has to possibility for configuring this?
Hello Aaron Durbin, Patrick Rudolph, Tim Wawrzynczak, Aamir Bohra, Maulik V Vaghela, Tim Wawrzynczak, Duncan Laurie, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36864
to look at the new patch set (#8).
Change subject: soc/intel/cannonlake: Add chip config to override CPU flex ratio ......................................................................
soc/intel/cannonlake: Add chip config to override CPU flex ratio
This patch provides options to override descriptor default CPU flex ratio from coreboot code. cpu_ratio_override to provide the required CPU ratio.
Note: Don't override the flex ratio if cpu_ratio is 0.
TEST=Without override flex_ratio is 0 and verified booting to OS after overriding with flex_ratio value 5.
Change-Id: Ib01650f52f3d402f669e7e7f5b28a648b86f08ec Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 21 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/36864/8
Subrata Banik 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 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36864/7/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/36864/7/src/soc/intel/cannonlake/ch... PS7, Line 433: FIT tool
What is FIT tool? Afaik only cbfstool knows how to add microcode FIT entries. […]
realized that FIT might not the correct term to use for rather descriptor holds good and FIT is nothing but populating descriptor option. So no need to depends on FIT as such
Nico Huber 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 8:
(1 comment)
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 79: flex_ratio = rdmsr(MSR_FLEX_RATIO);
no, not like that, this register always hold the snap shot of what cpu softstap has set. […]
Yes, thanks. And sorry, I published a stale comment here.
Nico Huber 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 8:
(1 comment)
I have read through the comments but still don't understand a lot of things:
Please explain why a higher ratio results in a lower speed.
Please explain in detail what the `CpuRatio` UPD actually does. How does it relate to `BootFrequency`?
Also, it seems to me that the measurements are void, because the currently simplified tsc_freq_mhz() implementation doesn't support "flex ratio". I think we should implement tsc_freq_mhz() correctly first, before jumping to any conclusions.
Please don't assume thing just because there is one issue going on where tsc_freq is in question. I can ensures that issue doesn't have anything related to this work.
No, I was assuming things because the SDM implies the opposite behaviour for CPUID+0x16 (that it doesn't reflect the actual speed). Please don't just accept such documentation problems. If the documentation isn't fixed and there is no comment in the code about the broken documentation, you have to expect such disarray.
We have run some experiment to minimize platform power consumption by running CPU at lower speed and found that running at lower freq provides better boot time number specifically running below code
https://chromium.googlesource.com/chromiumos/platform/vboot/+/refs/heads/mas...
Additionally, for the interested reader: flex ratio adapts the maximum non-turbo ratio. And is mainly a feature to influence the TSC rate...
We are using CPU speed independent timer hence running at HFM or LFM doesn't matter to TSC
The current ratio doesn't matter, but the flex ratio does. You can see that the timestamps stay correct with different flex-ratio settings while CPUID +0x16 EAX changes. So the TSC rate has to change too.
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 are suspecting cache misses more while running this for loop at higher freq, comparing core/uncor […]
Heh, interesting. I would have suspected that such things can impact performance a little, but a 3x speed increase is quite impressive.
Nico Huber 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 8:
(1 comment)
I would still like to have a detailed documentation of the UPD. It's still unclear to me how the system is affected after boot, as there is also `BootFrequency` which seems independent from this.
https://review.coreboot.org/c/coreboot/+/36864/8/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/36864/8/src/soc/intel/cannonlake/ch... PS8, Line 434: * Default cpu flex ratio is 0 ensures booting with HFM. This is actually controlled by the `BootFrequency` UPD. At least if we can trust its documentation. The 0 merely means that the max non-turbo stays at it's default value.
Subrata Banik 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 8:
(1 comment)
Patch Set 8:
(1 comment)
I would still like to have a detailed documentation of the UPD. It's still unclear to me how the system is affected after boot, as there is also `BootFrequency` which seems independent from this.
I guess you can refer to BWG chapter 11 title power management
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))
Heh, interesting. I would have suspected that such things can impact […]
yes I remember Kyosti always had question why 1001 is taking that long time. so we have answer for that hopefully now
Subrata Banik 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 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36864/8/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/36864/8/src/soc/intel/cannonlake/ch... PS8, Line 434: * Default cpu flex ratio is 0 ensures booting with HFM.
This is actually controlled by the `BootFrequency` UPD. At least if we […]
i have confirmed that this is correct UPD to configure flex ratio in FSP and it doesn't change since.
and yes, 0 means Max non-turbo max which is HFM, but for simplicity i should write non-turbo max agree
Hello Aaron Durbin, Patrick Rudolph, Tim Wawrzynczak, Aamir Bohra, Maulik V Vaghela, Tim Wawrzynczak, Duncan Laurie, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36864
to look at the new patch set (#9).
Change subject: soc/intel/cannonlake: Add chip config to override CPU flex ratio ......................................................................
soc/intel/cannonlake: Add chip config to override CPU flex ratio
This patch provides options to override descriptor default CPU flex ratio from coreboot code. cpu_ratio_override to provide the required CPU ratio.
Note: Don't override the flex ratio if cpu_ratio is 0.
TEST=Without override flex_ratio is 0 and verified booting to OS after overriding with flex_ratio value 5.
Change-Id: Ib01650f52f3d402f669e7e7f5b28a648b86f08ec Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 21 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/36864/9
Nico Huber 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 9:
I would still like to have a detailed documentation of the UPD. It's still unclear to me how the system is affected after boot, as there is also `BootFrequency` which seems independent from this.
I guess you can refer to BWG chapter 11 title power management
Which BWG, document number please. Is this another document beside the "BIOS Specification"? I often confuse the two. Does your BWG only document the hardware or FSP too? Because in all the hardware documentation it says "Flex Ratio" and in FSP it says "CpuRatio". Please, where is it documented that that two mean the very same thing?
I trust you that the two are the same. But if it is not documented (e.g. in the FSP integration guide) then people will have these discussions over and over again, wasting a lot of time. And if you can do anything about it, please tell FSP teams to not invent new names. This is not the only occurence.
Subrata Banik 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 9:
Patch Set 9:
I would still like to have a detailed documentation of the UPD. It's still unclear to me how the system is affected after boot, as there is also `BootFrequency` which seems independent from this.
I guess you can refer to BWG chapter 11 title power management
Which BWG, document number please.
Its SKL CPU BWG (we have two BWG, one for CPU and another for PCH). doc 33391
Is this another document beside the "BIOS Specification"? I often confuse the two. Does your BWG only document the hardware or FSP too? Because in all the hardware documentation it says "Flex Ratio" and in FSP it says "CpuRatio". Please, where is it documented that that two mean the very same thing?
I guess its discrepancy in name which we can raise a request to fix, i will take a note. Its both same as you can read the help text from FSP-M header.
I trust you that the two are the same. But if it is not documented (e.g. in the FSP integration guide) then people will have these discussions over and over again, wasting a lot of time. And if you can do anything about it, please tell FSP teams to not invent new names. This is not the only occurence.
yes, got your point and its valid feedback.
Nico Huber 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 9:
Patch Set 9:
Patch Set 9:
I would still like to have a detailed documentation of the UPD. It's still unclear to me how the system is affected after boot, as there is also `BootFrequency` which seems independent from this.
I guess you can refer to BWG chapter 11 title power management
Which BWG, document number please.
Its SKL CPU BWG (we have two BWG, one for CPU and another for PCH). doc 33391
Thanks. Alas, I can't find it. It might need a different level of access. But it's good to know that more documentation exists.
I was looking mostly at 550049, but realized by now that it doesn't say "BWG" but "BIOS Specification".
Is this another document beside the "BIOS Specification"? I often confuse the two. Does your BWG only document the hardware or FSP too? Because in all the hardware documentation it says "Flex Ratio" and in FSP it says "CpuRatio". Please, where is it documented that that two mean the very same thing?
I guess its discrepancy in name which we can raise a request to fix, i will take a note. Its both same as you can read the help text from FSP-M header.
I must be looking at a different header. Mine just says: 0-63 and 0 is disabled. Nothing about Flex Ratio, reboots, changed TSC, etc.
Subrata Banik 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 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
I would still like to have a detailed documentation of the UPD. It's still unclear to me how the system is affected after boot, as there is also `BootFrequency` which seems independent from this.
I guess you can refer to BWG chapter 11 title power management
Which BWG, document number please.
Its SKL CPU BWG (we have two BWG, one for CPU and another for PCH). doc 33391
Thanks. Alas, I can't find it. It might need a different level of access. But it's good to know that more documentation exists.
I was looking mostly at 550049, but realized by now that it doesn't say "BWG" but "BIOS Specification".
That doesn't have power management chapter ?
Is this another document beside the "BIOS Specification"? I often confuse the two. Does your BWG only document the hardware or FSP too? Because in all the hardware documentation it says "Flex Ratio" and in FSP it says "CpuRatio". Please, where is it documented that that two mean the very same thing?
I guess its discrepancy in name which we can raise a request to fix, i will take a note. Its both same as you can read the help text from FSP-M header.
I must be looking at a different header. Mine just says: 0-63 and 0 is disabled. Nothing about Flex Ratio, reboots, changed TSC, etc.
yes, its same and i will pass this feedback to improve the help text and name going forward
Nico Huber 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 9:
I would still like to have a detailed documentation of the UPD. It's still unclear to me how the system is affected after boot, as there is also `BootFrequency` which seems independent from this.
I guess you can refer to BWG chapter 11 title power management
Which BWG, document number please.
Its SKL CPU BWG (we have two BWG, one for CPU and another for PCH). doc 33391
Thanks. Alas, I can't find it. It might need a different level of access. But it's good to know that more documentation exists.
I was looking mostly at 550049, but realized by now that it doesn't say "BWG" but "BIOS Specification".
That doesn't have power management chapter ?
It does, but it doesn't document FSP. I was hoping that you have something there that documents FSP/Intel's reference code, e.g. how the UPD names map to reality.
Is this another document beside the "BIOS Specification"? I often confuse the two. Does your BWG only document the hardware or FSP too? Because in all the hardware documentation it says "Flex Ratio" and in FSP it says "CpuRatio". Please, where is it documented that that two mean the very same thing?
I guess its discrepancy in name which we can raise a request to fix, i will take a note. Its both same as you can read the help text from FSP-M header.
I must be looking at a different header. Mine just says: 0-63 and 0 is disabled. Nothing about Flex Ratio, reboots, changed TSC, etc.
yes, its same and i will pass this feedback to improve the help text and name going forward
Thanks!
Tim Wawrzynczak 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 9: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/36864/9/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/36864/9/src/soc/intel/cannonlake/ro... PS9, Line 77: if (config->cpu_ratio_override) : m_cfg->CpuRatio = config->cpu_ratio_override; : If you have to use braces in one branch, please use them in all branches.
Subrata Banik 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 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36864/9/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/36864/9/src/soc/intel/cannonlake/ro... PS9, Line 77: if (config->cpu_ratio_override) : m_cfg->CpuRatio = config->cpu_ratio_override; :
If you have to use braces in one branch, please use them in all branches.
Done
Hello Aaron Durbin, Patrick Rudolph, Tim Wawrzynczak, Aamir Bohra, Maulik V Vaghela, Tim Wawrzynczak, Duncan Laurie, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36864
to look at the new patch set (#10).
Change subject: soc/intel/cannonlake: Add chip config to override CPU flex ratio ......................................................................
soc/intel/cannonlake: Add chip config to override CPU flex ratio
This patch provides options to override descriptor default CPU flex ratio from coreboot code. cpu_ratio_override to provide the required CPU ratio.
Note: Don't override the flex ratio if cpu_ratio is 0.
TEST=Without override flex_ratio is 0 and verified booting to OS after overriding with flex_ratio value 5.
Change-Id: Ib01650f52f3d402f669e7e7f5b28a648b86f08ec Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 21 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/36864/10
Tim Wawrzynczak 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 10: Code-Review+2
Hello Aaron Durbin, Patrick Rudolph, Tim Wawrzynczak, Aamir Bohra, Maulik V Vaghela, Tim Wawrzynczak, Duncan Laurie, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36864
to look at the new patch set (#11).
Change subject: soc/intel/cannonlake: Add chip config to override CPU flex ratio ......................................................................
soc/intel/cannonlake: Add chip config to override CPU flex ratio
This patch provides options to override descriptor default CPU flex ratio from coreboot code. cpu_ratio_override to provide the required CPU ratio.
Note: Don't override the flex ratio if cpu_ratio is 0.
BUG=b:142264107 TEST=Without override flex_ratio is 0 and verified booting to OS after overriding with flex_ratio value 5.
Change-Id: Ib01650f52f3d402f669e7e7f5b28a648b86f08ec Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 21 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/36864/11
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36864 )
Change subject: soc/intel/cannonlake: Add chip config to override CPU flex ratio ......................................................................
soc/intel/cannonlake: Add chip config to override CPU flex ratio
This patch provides options to override descriptor default CPU flex ratio from coreboot code. cpu_ratio_override to provide the required CPU ratio.
Note: Don't override the flex ratio if cpu_ratio is 0.
BUG=b:142264107 TEST=Without override flex_ratio is 0 and verified booting to OS after overriding with flex_ratio value 5.
Change-Id: Ib01650f52f3d402f669e7e7f5b28a648b86f08ec Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/36864 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 21 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h index 17afdd1..507290f 100644 --- a/src/soc/intel/cannonlake/chip.h +++ b/src/soc/intel/cannonlake/chip.h @@ -424,6 +424,19 @@ * Bit 0: MISCCFG_GPDLCGEN */ uint8_t gpio_pm[TOTAL_GPIO_COMM]; + + /* + * Override CPU flex ratio value: + * CPU ratio value controls the maximum processor non-turbo ratio. + * Valid Range 0 to 63. + * + * In general descriptor provides option to set default cpu flex ratio. + * Default cpu flex ratio is 0 ensures booting with non-turbo max frequency. + * Thats the reason FSP skips cpu_ratio override if cpu_ratio is 0. + * + * Only override CPU flex ratio if don't want to boot with non-turbo max. + */ + uint8_t cpu_ratio_override; };
typedef struct soc_intel_cannonlake_config config_t; diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c index 996c135..5c74d4a 100644 --- a/src/soc/intel/cannonlake/romstage/fsp_params.c +++ b/src/soc/intel/cannonlake/romstage/fsp_params.c @@ -74,10 +74,14 @@ m_cfg->SkipMpInit = !CONFIG_USE_INTEL_FSP_MP_INIT; #endif
- /* Set CpuRatio to match existing MSR value */ - msr_t flex_ratio; - flex_ratio = rdmsr(MSR_FLEX_RATIO); - m_cfg->CpuRatio = (flex_ratio.lo >> 8) & 0xff; + if (config->cpu_ratio_override) { + m_cfg->CpuRatio = config->cpu_ratio_override; + } else { + /* Set CpuRatio to match existing MSR value */ + msr_t flex_ratio; + flex_ratio = rdmsr(MSR_FLEX_RATIO); + m_cfg->CpuRatio = (flex_ratio.lo >> 8) & 0xff; + }
/* If ISH is enabled, enable ISH elements */ if (!dev)
-
Arthur Heymans (Code Review) -
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)