Jamie Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37189 )
Change subject: soc/intel/cannonlake: Add IccMax limitation for CML U22 ......................................................................
soc/intel/cannonlake: Add IccMax limitation for CML U22
IccMax[IA_CORE] for U22 is 35A. Add a limitation to avoid IccMax setting over 35A in fill_vr_domain_config function.
BUG:b:145094963 BRANCH: None TEST: build coreboot and fsp with enabling fw_debug. Flashed to device and checked the log. IccMax of IA_CORE have been setted correctly.
Change-Id: I2ae8feee3073884109e0d171511c5ac01064ffd7 Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/soc/intel/cannonlake/vr_config.c 1 file changed, 16 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/37189/1
diff --git a/src/soc/intel/cannonlake/vr_config.c b/src/soc/intel/cannonlake/vr_config.c index 25270d8..671d93d 100644 --- a/src/soc/intel/cannonlake/vr_config.c +++ b/src/soc/intel/cannonlake/vr_config.c @@ -14,6 +14,9 @@ * */
+#include <console/console.h> +#include <device/pci_ids.h> +#include <device/pci_ops.h> #include <fsp/api.h> #include <soc/ramstage.h> #include <soc/vr_config.h> @@ -74,6 +77,8 @@ { FSP_S_CONFIG *vr_params = (FSP_S_CONFIG *)params; const struct vr_config *cfg; + struct device *sa_root; + uint16_t did;
if (domain < 0 || domain >= NUM_VR_DOMAINS) return; @@ -96,4 +101,15 @@ vr_params->VrVoltageLimit[domain] = cfg->voltage_limit; vr_params->AcLoadline[domain] = cfg->ac_loadline; vr_params->DcLoadline[domain] = cfg->dc_loadline; + + /* Limit IA_CORE IccMax if CPU SKU is U22 */ + if (domain == VR_IA_CORE) { + sa_root = pcidev_path_on_root(SA_DEVFN_ROOT); + did = pci_read_config16(sa_root, PCI_DEVICE_ID); + if (did == PCI_DEVICE_ID_INTEL_CML_ULT_2_2 && + cfg->icc_max > VR_CFG_AMP(35)) { + printk(BIOS_DEBUG, "VR_CONFIG: IccMax[IA_CORE] has been limited to 35A\n"); + vr_params->IccMax[domain] = VR_CFG_AMP(35); + } + } }
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37189 )
Change subject: soc/intel/cannonlake: Add IccMax limitation for CML U22 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37189/1/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/37189/1/src/soc/intel/cannonlake/vr... PS1, Line 111: printk(BIOS_DEBUG, "VR_CONFIG: IccMax[IA_CORE] has been limited to 35A\n"); over 80 chars limit
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37189 )
Change subject: soc/intel/cannonlake: Add IccMax limitation for CML U22 ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37189/1/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/37189/1/src/soc/intel/cannonlake/vr... PS1, Line 109: did can't we check CPUID ? or its same ?
https://review.coreboot.org/c/coreboot/+/37189/1/src/soc/intel/cannonlake/vr... PS1, Line 111: printk(BIOS_DEBUG, "VR_CONFIG: IccMax[IA_CORE] has been limited to 35A\n");
over 80 chars limit
right now we have 96 i guess.
question is do you need this debug msg ?
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37189 )
Change subject: soc/intel/cannonlake: Add IccMax limitation for CML U22 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37189/1/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/37189/1/src/soc/intel/cannonlake/vr... PS1, Line 111: printk(BIOS_DEBUG, "VR_CONFIG: IccMax[IA_CORE] has been limited to 35A\n");
right now we have 96 i guess. […]
the width of this line is 100 now. is 96 a hard requirement now?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37189 )
Change subject: soc/intel/cannonlake: Add IccMax limitation for CML U22 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37189/1/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/37189/1/src/soc/intel/cannonlake/vr... PS1, Line 111: printk(BIOS_DEBUG, "VR_CONFIG: IccMax[IA_CORE] has been limited to 35A\n");
the width of this line is 100 now. […]
i could see CB guideline is now updated to 96 characters per line
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37189 )
Change subject: soc/intel/cannonlake: Add IccMax limitation for CML U22 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37189/1/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/37189/1/src/soc/intel/cannonlake/vr... PS1, Line 111: printk(BIOS_DEBUG, "VR_CONFIG: IccMax[IA_CORE] has been limited to 35A\n");
i could see CB guideline is now updated to 96 characters per line
ok, Jamie you might want to reduce the log a little bit. the width of this line is 100 now.
Hello Patrick Rudolph, Subrata Banik, Kane Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37189
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Add IccMax limitation for CML U22 ......................................................................
soc/intel/cannonlake: Add IccMax limitation for CML U22
IccMax[IA_CORE] for U22 is 35A. Add a limitation to avoid IccMax setting over 35A in fill_vr_domain_config function.
BUG:b:145094963 BRANCH: None TEST: build coreboot and fsp with enabling fw_debug. Flashed to device and checked the log. IccMax of IA_CORE have been setted correctly.
Change-Id: I2ae8feee3073884109e0d171511c5ac01064ffd7 Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/soc/intel/cannonlake/vr_config.c 1 file changed, 16 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/37189/2
Jamie Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37189 )
Change subject: soc/intel/cannonlake: Add IccMax limitation for CML U22 ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37189/1/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/37189/1/src/soc/intel/cannonlake/vr... PS1, Line 109: did
can't we check CPUID ? or its same ?
CML v1 U6/U4/U2 have the same CPUID. So I use Did to identify them.
https://review.coreboot.org/c/coreboot/+/37189/1/src/soc/intel/cannonlake/vr... PS1, Line 111: printk(BIOS_DEBUG, "VR_CONFIG: IccMax[IA_CORE] has been limited to 35A\n");
ok, Jamie you might want to reduce the log a little bit. the width of this line is 100 now.
Reduce the length to 90.
Harry Pan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37189 )
Change subject: soc/intel/cannonlake: Add IccMax limitation for CML U22 ......................................................................
Patch Set 2:
Open question: Can we leverage SKL/KBL work of get_sku_icc_max()?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37189 )
Change subject: soc/intel/cannonlake: Add IccMax limitation for CML U22 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37189/1/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/37189/1/src/soc/intel/cannonlake/vr... PS1, Line 109: did
CML v1 U6/U4/U2 have the same CPUID. […]
Can you add a comment to that effect?
Jamie Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37189 )
Change subject: soc/intel/cannonlake: Add IccMax limitation for CML U22 ......................................................................
Patch Set 2:
Patch Set 2:
Open question: Can we leverage SKL/KBL work of get_sku_icc_max()?
get_sku_icc_max() on SKY/KBL is do the same thing like the CML FSP (See Fsp mCpuVrOverrideTable at CometLakeSiliconPkg/Cpu/Library/Private/PeiCpuInitLib/CpuInitPreResetCpl.c).
I want to add a limitation for U22 CPU because no matter go through FSP VR override table or device tree, both have wrong IccMax[IA_CORE] configure.
Use FSP VR override table, IccMax[U42,U62] = 85A and IccMax[U22] = 35A ,but 85A is over the hatch VR design. Hatch is using 70A VR design. Use device tree configure, IccMax[U22,U42,U62] = 70A, IccMax[U22] is over the EDS define 35A.
But it is a good reference code, I will use the same variable name at my next patchset.
Hello Patrick Rudolph, Subrata Banik, Tim Wawrzynczak, Kane Chen, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37189
to look at the new patch set (#3).
Change subject: mb/google/hatch: Add IccMax limitation for CML U22 ......................................................................
mb/google/hatch: Add IccMax limitation for CML U22
IccMax of IA_CORE for U22 is smaller than U42 and U62. Add a limitation to fill_vr_domain_config. IccMax of IA_CORE will be setted to 35A when using CML U22.
BUG:b:145094963 BRANCH: None TEST: build coreboot and fsp with enabling fw_debug. Flashed to device and checked the log. IccMax of IA_CORE have been setted correctly.
Change-Id: I2ae8feee3073884109e0d171511c5ac01064ffd7 Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/soc/intel/cannonlake/vr_config.c 1 file changed, 19 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/37189/3
Jamie Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37189 )
Change subject: mb/google/hatch: Add IccMax limitation for CML U22 ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37189/1/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/37189/1/src/soc/intel/cannonlake/vr... PS1, Line 109: did
Can you add a comment to that effect?
I changed the variable name to mch_id at pathset3 for easy understanding. Also add the EDS information here.
Is it good for you?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37189 )
Change subject: mb/google/hatch: Add IccMax limitation for CML U22 ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37189/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37189/3//COMMIT_MSG@11 PS3, Line 11: setted set
https://review.coreboot.org/c/coreboot/+/37189/3//COMMIT_MSG@15 PS3, Line 15: enabling enabled
https://review.coreboot.org/c/coreboot/+/37189/3//COMMIT_MSG@17 PS3, Line 17: have been setted was set
Jamie Chen has uploaded a new patch set (#4) to the change originally created by Jamie Chen. ( https://review.coreboot.org/c/coreboot/+/37189 )
Change subject: mb/google/hatch: Add IccMax limitation for CML U22 ......................................................................
mb/google/hatch: Add IccMax limitation for CML U22
IccMax of IA_CORE for U22 is smaller than U42 and U62. Add a limitation to fill_vr_domain_config. IccMax of IA_CORE will be set to 35A when using CML U22.
BUG:b:145094963 BRANCH: None TEST: build coreboot and fsp with enabled fw_debug. Flashed to device and checked the log. IccMax of IA_CORE was set correctly.
Change-Id: I2ae8feee3073884109e0d171511c5ac01064ffd7 Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/soc/intel/cannonlake/vr_config.c 1 file changed, 19 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/37189/4
Jamie Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37189 )
Change subject: mb/google/hatch: Add IccMax limitation for CML U22 ......................................................................
Patch Set 4:
(3 comments)
Patch Set 3:
(3 comments)
Thank you
https://review.coreboot.org/c/coreboot/+/37189/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37189/3//COMMIT_MSG@11 PS3, Line 11: setted
set
Done
https://review.coreboot.org/c/coreboot/+/37189/3//COMMIT_MSG@15 PS3, Line 15: enabling
enabled
Done
https://review.coreboot.org/c/coreboot/+/37189/3//COMMIT_MSG@17 PS3, Line 17: have been setted
was set
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37189 )
Change subject: mb/google/hatch: Add IccMax limitation for CML U22 ......................................................................
Patch Set 4:
Jamie, have you looked at the following change: https://review.coreboot.org/c/coreboot/+/37466
Jamie Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37189 )
Change subject: mb/google/hatch: Add IccMax limitation for CML U22 ......................................................................
Patch Set 4:
Patch Set 4:
Jamie, have you looked at the following change: https://review.coreboot.org/c/coreboot/+/37466
Yes, I will post CML data to this CL.
Jamie Chen has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37189 )
Change subject: mb/google/hatch: Add IccMax limitation for CML U22 ......................................................................
Abandoned
solution at https://review.coreboot.org/c/coreboot/+/37874
-
Furquan Shaikh (Code Review) -
Harry Pan (Code Review)
-
Jamie Chen (Code Review)
-
Kane Chen (Code Review)
-
Paul Menzel (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)