Marx Wang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
soc/intel/cannonlake: Add tdc config for CML
Add tdc defaults for CML: 1.TdcEnable 2.TdcPowerLimit
UG=b:148912093 BRANCH=None TEST=build coreboot and check FSP log.
Signed-off-by: Marx Wang marx.wang@intel.com Change-Id: Ie4b17e5b4ce41c1adb436ae5646f0d8578a440e8 --- M src/soc/intel/cannonlake/include/soc/vr_config.h M src/soc/intel/cannonlake/vr_config.c 2 files changed, 114 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38741/1
diff --git a/src/soc/intel/cannonlake/include/soc/vr_config.h b/src/soc/intel/cannonlake/include/soc/vr_config.h index 1390b17..ce6f0ab 100644 --- a/src/soc/intel/cannonlake/include/soc/vr_config.h +++ b/src/soc/intel/cannonlake/include/soc/vr_config.h @@ -53,10 +53,16 @@ /* AC and DC Loadline in 1/100 mOhms. Range is 0-6249 */ uint16_t ac_loadline; uint16_t dc_loadline; + + /* TDC Power Limit in 1/8 A units */ + uint16_t tdc_powerlimit; + + };
#define VR_CFG_AMP(i) (uint16_t)((i) * 4) #define VR_CFG_MOHMS(i) (uint16_t)((i) * 100) +#define VR_CFG_TDC_AMP(i) ((i) * 8)
/* VrConfig Settings for 4 domains * 0 = System Agent, 1 = IA Core, @@ -85,6 +91,14 @@ [VR_GT_SLICED] = VR_CFG_MOHMS(gt_sl), \ }
+#define VR_CFG_ALL_DOMAINS_TDC(sa, ia, gt_unsl, gt_sl) \ + { \ + [VR_SYSTEM_AGENT] = VR_CFG_TDC_AMP(sa), \ + [VR_IA_CORE] = VR_CFG_TDC_AMP(ia), \ + [VR_GT_UNSLICED] = VR_CFG_TDC_AMP(gt_unsl), \ + [VR_GT_SLICED] = VR_CFG_TDC_AMP(gt_sl), \ + } + void fill_vr_domain_config(void *params, int domain, const struct vr_config *cfg);
diff --git a/src/soc/intel/cannonlake/vr_config.c b/src/soc/intel/cannonlake/vr_config.c index 5fadcf4..58880b1 100644 --- a/src/soc/intel/cannonlake/vr_config.c +++ b/src/soc/intel/cannonlake/vr_config.c @@ -419,6 +419,98 @@ return 1520; }
+static uint16_t get_sku_tdc_powerlimit(int domain) +{ + const uint16_t tdp = cpu_get_power_max(); + config_t *cfg = config_of_soc(); + + static uint16_t mch_id = 0; + if (!mch_id) { + struct device *dev = pcidev_path_on_root(SA_DEVFN_ROOT); + mch_id = dev ? pci_read_config16(dev, PCI_DEVICE_ID) : 0xffff; + } + + switch (mch_id) { + case PCI_DEVICE_ID_INTEL_CML_ULT: /* fallthrough */ + case PCI_DEVICE_ID_INTEL_CML_ULT_6_2: { + uint16_t tdc[NUM_VR_DOMAINS] = + VR_CFG_ALL_DOMAINS_TDC(4, 58, 22, 22); + + if (cfg->cpu_pl2_4_cfg == baseline) + tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(48); + + return tdc[domain]; + } + case PCI_DEVICE_ID_INTEL_CML_ULT_2_2: { + const uint16_t tdc[NUM_VR_DOMAINS] = + VR_CFG_ALL_DOMAINS_TDC(4, 24, 22, 22); + return tdc[domain]; + } + case PCI_DEVICE_ID_INTEL_CML_H_4_2: { + uint16_t tdc[NUM_VR_DOMAINS] = + VR_CFG_ALL_DOMAINS_TDC(10, 80, 25, 25); + + if (cfg->cpu_pl2_4_cfg == baseline) + tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(60); + + return tdc[domain]; + } + case PCI_DEVICE_ID_INTEL_CML_H: { + uint16_t tdc[NUM_VR_DOMAINS] = + VR_CFG_ALL_DOMAINS_TDC(10, 92, 25, 25); + + if (cfg->cpu_pl2_4_cfg == baseline) + tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(80); + + return tdc[domain]; + } + case PCI_DEVICE_ID_INTEL_CML_H_8_2: { + uint16_t tdc[NUM_VR_DOMAINS] = + VR_CFG_ALL_DOMAINS_TDC(10, 125, 25, 25); + + if (tdp >= 65) /* 65W */ + if (cfg->cpu_pl2_4_cfg == baseline) + tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(117); + else + tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(146); + else /* 45W */ + if (cfg->cpu_pl2_4_cfg == baseline) + tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(86); + else + tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(125); + + return tdc[domain]; + } + case PCI_DEVICE_ID_INTEL_CML_S_G0G1_P0P1_6_2: { + uint16_t tdc[NUM_VR_DOMAINS] = + VR_CFG_ALL_DOMAINS_TDC(10, 74, 28, 28); + + if (tdp >= 125) /* 125W */ + tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(132); + else if (tdp >= 65) /* 80W or 65W */ + tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(104); + else /* 35W */ + tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(74); + + return tdc[domain]; + } + case PCI_DEVICE_ID_INTEL_CML_S_P0P1_8_2: + case PCI_DEVICE_ID_INTEL_CML_S_P0P1_10_2: { + uint16_t tdc[NUM_VR_DOMAINS] = + VR_CFG_ALL_DOMAINS_TDC(10, 100, 28, 28); + + if (tdp > 35) /* 125W or 80W or 65W */ + tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(175); + + return tdc[domain]; + } + default: + printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config\n", mch_id); + } + + return 0; +} + void fill_vr_domain_config(void *params, int domain, const struct vr_config *chip_cfg) { @@ -442,7 +534,6 @@ vr_params->Psi4Enable[domain] = cfg->psi4enable; vr_params->ImonSlope[domain] = cfg->imon_slope; vr_params->ImonOffset[domain] = cfg->imon_offset; - /* If board provided non-zero value, use it. */ if (cfg->voltage_limit) vr_params->VrVoltageLimit[domain] = cfg->voltage_limit; @@ -463,4 +554,12 @@ vr_params->DcLoadline[domain] = cfg->dc_loadline; else vr_params->DcLoadline[domain] = get_sku_ac_dc_loadline(domain); + + if (cfg->tdc_powerlimit) + vr_params->TdcPowerLimit[domain] = cfg->tdc_powerlimit; + else + vr_params->TdcPowerLimit[domain] = get_sku_tdc_powerlimit(domain); + + vr_params->TdcEnable[domain] = 1; + }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38741/1/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/vr_config.h:
https://review.coreboot.org/c/coreboot/+/38741/1/src/soc/intel/cannonlake/in... PS1, Line 60: trailing whitespace
https://review.coreboot.org/c/coreboot/+/38741/1/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/1/src/soc/intel/cannonlake/vr... PS1, Line 432: trailing whitespace
https://review.coreboot.org/c/coreboot/+/38741/1/src/soc/intel/cannonlake/vr... PS1, Line 509: } trailing whitespace
Hello Thejaswani Putta, Patrick Rudolph, EricR Lai, Edward O'Callaghan, Kane Chen, Mathew King, Duncan Laurie, John Su, build bot (Jenkins), Furquan Shaikh, Jamie Chen,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38741
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
soc/intel/cannonlake: Add tdc config for CML
Add tdc defaults for CML: 1.TdcEnable 2.TdcPowerLimit
UG=b:148912093 BRANCH=None TEST=build coreboot and check FSP log.
Signed-off-by: Marx Wang marx.wang@intel.com Change-Id: Ie4b17e5b4ce41c1adb436ae5646f0d8578a440e8 --- M src/soc/intel/cannonlake/include/soc/vr_config.h M src/soc/intel/cannonlake/vr_config.c 2 files changed, 113 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38741/2
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 2: Code-Review+1
LGTM, we will verify it.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 2:
(7 comments)
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 445: Don’t remove this.
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 422: static uint16_t get_sku_tdc_powerlimit(int domain) What does a power limit of 0 mean (in the very end)?
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 428: if (!mch_id) { This check does not make sense, as the value is clear.
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 430: : 0xffff Instead of assigning the value, return early?
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 434: case PCI_DEVICE_ID_INTEL_CML_ULT: /* fallthrough */ I think the comment is not needed here, as there is no code between the case statements.
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 475: tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(146); Use ternary operator instead?
tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(117) : VR_CFG_TDC_AMP(146);
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 508: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config\n", mch_id); Please extend the error message by explaining the consequences for the device.
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 422: static uint16_t get_sku_tdc_powerlimit(int domain)
What does a power limit of 0 mean (in the very end)?
0 means auto/Intel defaults
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 428: if (!mch_id) {
This check does not make sense, as the value is clear.
mch_id is defined as "static" which means the value is stored.
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 430: : 0xffff
Instead of assigning the value, return early?
actually, it follows the code style in "skylake\vr_config.c" and other "get_sku_xxx" functions.
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 475: tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(146);
Use ternary operator instead? […]
that's a good suggestion.
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 508: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config\n", mch_id);
Please extend the error message by explaining the consequences for the device.
will add "and use Auto/default configurations" for unknown MCH"
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 445:
Don’t remove this.
Done
Hello Thejaswani Putta, Patrick Rudolph, EricR Lai, Edward O'Callaghan, Kane Chen, Mathew King, Duncan Laurie, build bot (Jenkins), John Su, Furquan Shaikh, Jamie Chen,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38741
to look at the new patch set (#3).
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
soc/intel/cannonlake: Add tdc config for CML
Add tdc defaults for CML: 1.TdcEnable 2.TdcPowerLimit
UG=b:148912093 BRANCH=None TEST=build coreboot and check FSP log.
Signed-off-by: Marx Wang marx.wang@intel.com Change-Id: Ie4b17e5b4ce41c1adb436ae5646f0d8578a440e8 --- M src/soc/intel/cannonlake/include/soc/vr_config.h M src/soc/intel/cannonlake/vr_config.c 2 files changed, 107 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38741/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/vr... PS3, Line 472: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(117) : VR_CFG_TDC_AMP(146); line over 96 characters
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/vr... PS3, Line 474: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(86) : VR_CFG_TDC_AMP(125); line over 96 characters
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/vr... PS3, Line 502: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config and apply Auto/default settings\n", mch_id); line over 96 characters
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 3:
(7 comments)
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 445:
Done
Done
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 422: static uint16_t get_sku_tdc_powerlimit(int domain)
0 means auto/Intel defaults
Done
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 428: if (!mch_id) {
mch_id is defined as "static" which means the value is stored.
Done
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 430: : 0xffff
actually, it follows the code style in "skylake\vr_config.c" and other "get_sku_xxx" functions.
Done
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 434: case PCI_DEVICE_ID_INTEL_CML_ULT: /* fallthrough */
I think the comment is not needed here, as there is no code between the case statements.
Done
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 475: tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(146);
that's a good suggestion.
Done
https://review.coreboot.org/c/coreboot/+/38741/2/src/soc/intel/cannonlake/vr... PS2, Line 508: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config\n", mch_id);
will add "and use Auto/default configurations" for unknown MCH"
Done
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/vr_config.h:
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/in... PS3, Line 57: TDC Can you make this comment say 'Thermal Design Current' so it is clear without needing to decode an acronym?
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/vr... PS3, Line 558: 1 Should this always be enabled or is there a reason a mainboard might want it disabled?
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/vr... PS3, Line 559: extra newline
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/vr... PS3, Line 558: 1
Should this always be enabled or is there a reason a mainboard might want it disabled?
Hi Marx, I think you can make another tdc_enable config in devtree. pls also remember enable this for hatch.
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 3:
(6 comments)
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/vr_config.h:
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/in... PS3, Line 57: TDC
Can you make this comment say 'Thermal Design Current' so it is clear without needing to decode an a […]
that's a good suggestion.
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/vr... PS3, Line 472: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(117) : VR_CFG_TDC_AMP(146);
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/vr... PS3, Line 474: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(86) : VR_CFG_TDC_AMP(125);
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/vr... PS3, Line 502: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config and apply Auto/default settings\n", mch_id);
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/vr... PS3, Line 558: 1
Hi Marx, […]
will add "tdc_disable" in the device_tree for users to configure so that it will be enabled by default if users don't add it in the device_tree.
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/vr... PS3, Line 559:
extra newline
Done
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/vr_config.h:
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/in... PS3, Line 57: TDC
that's a good suggestion.
Done
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/3/src/soc/intel/cannonlake/vr... PS3, Line 502: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config and apply Auto/default settings\n", mch_id);
Done
Done
Hello Thejaswani Putta, Patrick Rudolph, EricR Lai, Edward O'Callaghan, Kane Chen, Mathew King, Duncan Laurie, build bot (Jenkins), John Su, Furquan Shaikh, Jamie Chen,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38741
to look at the new patch set (#4).
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
soc/intel/cannonlake: Add tdc config for CML
Add Thermal Design Current(TDC) defaults for CML: 1.TdcEnable 2.TdcPowerLimit
UG=b:148912093 BRANCH=None TEST=build coreboot and check FSP log.
Signed-off-by: Marx Wang marx.wang@intel.com Change-Id: Ie4b17e5b4ce41c1adb436ae5646f0d8578a440e8 --- M src/soc/intel/cannonlake/include/soc/vr_config.h M src/soc/intel/cannonlake/vr_config.c 2 files changed, 112 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38741/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 4:
(7 comments)
https://review.coreboot.org/c/coreboot/+/38741/4/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/vr_config.h:
https://review.coreboot.org/c/coreboot/+/38741/4/src/soc/intel/cannonlake/in... PS4, Line 58: * this is set to 0 */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38741/4/src/soc/intel/cannonlake/in... PS4, Line 59: uint8_t tdc_disable; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38741/4/src/soc/intel/cannonlake/in... PS4, Line 59: uint8_t tdc_disable; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38741/4/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/4/src/soc/intel/cannonlake/vr... PS4, Line 472: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(117) : VR_CFG_TDC_AMP(146); line over 96 characters
https://review.coreboot.org/c/coreboot/+/38741/4/src/soc/intel/cannonlake/vr... PS4, Line 474: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(86) : VR_CFG_TDC_AMP(125); line over 96 characters
https://review.coreboot.org/c/coreboot/+/38741/4/src/soc/intel/cannonlake/vr... PS4, Line 502: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config and apply Auto/default settings\n", mch_id); line over 96 characters
https://review.coreboot.org/c/coreboot/+/38741/4/src/soc/intel/cannonlake/vr... PS4, Line 553: if(cfg->tdc_disable) space required before the open parenthesis '('
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38741/4/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/4/src/soc/intel/cannonlake/vr... PS4, Line 472: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(117) : VR_CFG_TDC_AMP(146);
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/38741/4/src/soc/intel/cannonlake/vr... PS4, Line 474: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(86) : VR_CFG_TDC_AMP(125);
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/38741/4/src/soc/intel/cannonlake/vr... PS4, Line 502: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config and apply Auto/default settings\n", mch_id);
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/38741/4/src/soc/intel/cannonlake/vr... PS4, Line 553: if(cfg->tdc_disable)
space required before the open parenthesis '('
Done
Hello Thejaswani Putta, Patrick Rudolph, EricR Lai, Edward O'Callaghan, Kane Chen, Mathew King, Duncan Laurie, build bot (Jenkins), John Su, Furquan Shaikh, Jamie Chen,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38741
to look at the new patch set (#5).
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
soc/intel/cannonlake: Add tdc config for CML
Add Thermal Design Current(TDC) defaults for CML: 1.TdcEnable 2.TdcPowerLimit
UG=b:148912093 BRANCH=None TEST=build coreboot and check FSP log.
Signed-off-by: Marx Wang marx.wang@intel.com Change-Id: Ie4b17e5b4ce41c1adb436ae5646f0d8578a440e8 --- M src/soc/intel/cannonlake/include/soc/vr_config.h M src/soc/intel/cannonlake/vr_config.c 2 files changed, 112 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38741/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 5:
(6 comments)
https://review.coreboot.org/c/coreboot/+/38741/5/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/vr_config.h:
https://review.coreboot.org/c/coreboot/+/38741/5/src/soc/intel/cannonlake/in... PS5, Line 58: * this is set to 0 */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38741/5/src/soc/intel/cannonlake/in... PS5, Line 59: uint8_t tdc_disable; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38741/5/src/soc/intel/cannonlake/in... PS5, Line 59: uint8_t tdc_disable; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38741/5/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/5/src/soc/intel/cannonlake/vr... PS5, Line 472: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(117) : VR_CFG_TDC_AMP(146); line over 96 characters
https://review.coreboot.org/c/coreboot/+/38741/5/src/soc/intel/cannonlake/vr... PS5, Line 474: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(86) : VR_CFG_TDC_AMP(125); line over 96 characters
https://review.coreboot.org/c/coreboot/+/38741/5/src/soc/intel/cannonlake/vr... PS5, Line 502: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config and apply Auto/default settings\n", mch_id); line over 96 characters
Hello Thejaswani Putta, Patrick Rudolph, EricR Lai, Edward O'Callaghan, Kane Chen, Mathew King, Duncan Laurie, build bot (Jenkins), John Su, Furquan Shaikh, Jamie Chen,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38741
to look at the new patch set (#6).
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
soc/intel/cannonlake: Add tdc config for CML
Add Thermal Design Current(TDC) defaults for CML: 1.TdcEnable 2.TdcPowerLimit
UG=b:148912093 BRANCH=None TEST=build coreboot and check FSP log.
Signed-off-by: Marx Wang marx.wang@intel.com Change-Id: Ie4b17e5b4ce41c1adb436ae5646f0d8578a440e8 --- M src/soc/intel/cannonlake/include/soc/vr_config.h M src/soc/intel/cannonlake/vr_config.c 2 files changed, 112 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38741/6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... PS6, Line 472: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(117) : VR_CFG_TDC_AMP(146); line over 96 characters
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... PS6, Line 474: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(86) : VR_CFG_TDC_AMP(125); line over 96 characters
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... PS6, Line 502: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config and apply Auto/default settings\n", mch_id); line over 96 characters
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 6:
(9 comments)
https://review.coreboot.org/c/coreboot/+/38741/5/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/vr_config.h:
https://review.coreboot.org/c/coreboot/+/38741/5/src/soc/intel/cannonlake/in... PS5, Line 58: * this is set to 0 */
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/38741/5/src/soc/intel/cannonlake/in... PS5, Line 59: uint8_t tdc_disable;
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/38741/5/src/soc/intel/cannonlake/in... PS5, Line 59: uint8_t tdc_disable;
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/38741/5/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/5/src/soc/intel/cannonlake/vr... PS5, Line 472: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(117) : VR_CFG_TDC_AMP(146);
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/38741/5/src/soc/intel/cannonlake/vr... PS5, Line 474: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(86) : VR_CFG_TDC_AMP(125);
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/38741/5/src/soc/intel/cannonlake/vr... PS5, Line 502: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config and apply Auto/default settings\n", mch_id);
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... PS6, Line 472: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(117) : VR_CFG_TDC_AMP(146);
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... PS6, Line 474: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(86) : VR_CFG_TDC_AMP(125);
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... PS6, Line 502: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config and apply Auto/default settings\n", mch_id);
line over 96 characters
Done
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... PS6, Line 472: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(117) : VR_CFG_TDC_AMP(146);
Done
how about if if (cfg->cpu_pl2_4_cfg == baseline) and tdc[VR_IA_CORE] = (tip >=65) ? :
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... PS6, Line 502: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config and apply Auto/default settings\n", mch_id);
Done
Should fix the indent
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... PS6, Line 554: vr_params->TdcEnable[domain] = 0; vr_params->TdcEnable[domain] = !cfg->tdc_disable
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... PS6, Line 472: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(117) : VR_CFG_TDC_AMP(146);
how about if if (cfg->cpu_pl2_4_cfg == baseline) and tdc[VR_IA_CORE] = (tip >=65) ? :
i think the original one is more straightforward. keep untouched.
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... PS6, Line 502: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config and apply Auto/default settings\n", mch_id);
Should fix the indent
good finding.
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... PS6, Line 554: vr_params->TdcEnable[domain] = 0;
vr_params->TdcEnable[domain] = !cfg->tdc_disable
that's a good suggestion.
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... PS6, Line 502: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config and apply Auto/default settings\n", mch_id);
good finding.
I found "switch/case" does NOT have an indent in other codes from coreboot. keep untouched.
Hello Thejaswani Putta, Patrick Rudolph, EricR Lai, Edward O'Callaghan, Kane Chen, Mathew King, Duncan Laurie, build bot (Jenkins), John Su, Furquan Shaikh, Jamie Chen,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38741
to look at the new patch set (#7).
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
soc/intel/cannonlake: Add tdc config for CML
Add Thermal Design Current(TDC) defaults for CML: 1.TdcEnable 2.TdcPowerLimit
UG=b:148912093 BRANCH=None TEST=build coreboot and check FSP log.
Signed-off-by: Marx Wang marx.wang@intel.com Change-Id: Ie4b17e5b4ce41c1adb436ae5646f0d8578a440e8 --- M src/soc/intel/cannonlake/include/soc/vr_config.h M src/soc/intel/cannonlake/vr_config.c 2 files changed, 112 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38741/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38741/7/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/7/src/soc/intel/cannonlake/vr... PS7, Line 472: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? \ Avoid unnecessary line continuations
https://review.coreboot.org/c/coreboot/+/38741/7/src/soc/intel/cannonlake/vr... PS7, Line 475: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? \ Avoid unnecessary line continuations
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38741/7/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/7/src/soc/intel/cannonlake/vr... PS7, Line 472: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? \
Avoid unnecessary line continuations
Done
https://review.coreboot.org/c/coreboot/+/38741/7/src/soc/intel/cannonlake/vr... PS7, Line 475: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? \
Avoid unnecessary line continuations
Done
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 7:
(2 comments)
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38741/7/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/7/src/soc/intel/cannonlake/vr... PS7, Line 472: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? \
Done
the reason to add "line continuation" is because of this line containing 96+ characters.
https://review.coreboot.org/c/coreboot/+/38741/7/src/soc/intel/cannonlake/vr... PS7, Line 475: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? \
Done
the reason to add "line continuation" is because of this line containing 96+ characters.
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38741/7/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/7/src/soc/intel/cannonlake/vr... PS7, Line 472: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? \
the reason to add "line continuation" is because of this line containing 96+ characters.
Done
https://review.coreboot.org/c/coreboot/+/38741/7/src/soc/intel/cannonlake/vr... PS7, Line 475: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? \
the reason to add "line continuation" is because of this line containing 96+ characters.
Done
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... PS6, Line 472: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? VR_CFG_TDC_AMP(117) : VR_CFG_TDC_AMP(146);
i think the original one is more straightforward. keep untouched.
you can save the chars if you switch it
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/7/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/7/src/soc/intel/cannonlake/vr... PS7, Line 472: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? \
Done
you can just use newline like this:https://github.com/coreboot/coreboot/blob/master/src/mainboard/google/dralli...
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... PS6, Line 502: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config and apply Auto/default settings\n", mch_id);
I found "switch/case" does NOT have an indent in other codes from coreboot. keep untouched.
I mean yo should use indent like this : https://github.com/coreboot/coreboot/blob/master/src/mainboard/google/hatch/...
Hello Thejaswani Putta, Patrick Rudolph, EricR Lai, Edward O'Callaghan, Kane Chen, Mathew King, Duncan Laurie, build bot (Jenkins), John Su, Furquan Shaikh, Jamie Chen,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38741
to look at the new patch set (#8).
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
soc/intel/cannonlake: Add tdc config for CML
Add Thermal Design Current(TDC) defaults for CML: 1.TdcEnable 2.TdcPowerLimit
UG=b:148912093 BRANCH=None TEST=build coreboot and check FSP log.
Signed-off-by: Marx Wang marx.wang@intel.com Change-Id: Ie4b17e5b4ce41c1adb436ae5646f0d8578a440e8 --- M src/soc/intel/cannonlake/include/soc/vr_config.h M src/soc/intel/cannonlake/vr_config.c 2 files changed, 114 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38741/8
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/6/src/soc/intel/cannonlake/vr... PS6, Line 502: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config and apply Auto/default settings\n", mch_id);
I mean yo should use indent like this : https://github. […]
Done
https://review.coreboot.org/c/coreboot/+/38741/7/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/7/src/soc/intel/cannonlake/vr... PS7, Line 472: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? \
you can just use newline like this:https://github. […]
Done
https://review.coreboot.org/c/coreboot/+/38741/7/src/soc/intel/cannonlake/vr... PS7, Line 475: tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? \
Done
Done
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 8: Code-Review+2
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 8:
I can build this CL at my side with Drallion.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 9:
(7 comments)
https://review.coreboot.org/c/coreboot/+/38741/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38741/9//COMMIT_MSG@9 PS9, Line 9: Current(TDC) Please add a space before (.
https://review.coreboot.org/c/coreboot/+/38741/9//COMMIT_MSG@11 PS9, Line 11: 2.TdcPowerLimit Please add a space after the enumerator(?).
- TdcEnable
- TdcPowerLimit
https://review.coreboot.org/c/coreboot/+/38741/9//COMMIT_MSG@15 PS9, Line 15: check FSP log Check for what?
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/vr_config.h:
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/in... PS9, Line 58: * Please remove.
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/in... PS9, Line 61: /* Thermal Design Current(TDC) Power Limit in 1/8 A units */ Please add a space before the (.
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/vr... PS9, Line 506: 0x%x %#x
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/vr... PS9, Line 507: "Auto/default settings\n", mch_id); 1. Strings should be on one line. 2. Maybe: Unknown MCH (%#x) in VR-config, so apply defaults
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38741/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38741/9//COMMIT_MSG@13 PS9, Line 13: UG BUG
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/vr_config.h:
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/in... PS9, Line 67: #define VR_CFG_TDC_AMP(i) ((i) * 8) should this have a cast as well?
Hello Patrick Rudolph, Kane Chen, Duncan Laurie, Shelley Chen, build bot (Jenkins), John Su, Furquan Shaikh, Jamie Chen, Thejaswani Putta, EricR Lai, Edward O'Callaghan, Mathew King, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38741
to look at the new patch set (#10).
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
soc/intel/cannonlake: Add tdc config for CML
Add Thermal Design Current (TDC) defaults for CML: 1. TdcEnable 2. TdcPowerLimit
BUG=b:148912093 BRANCH=None TEST=build coreboot and check TdcEnable and TdcPowerLimit for each domain in FSP log
Signed-off-by: Marx Wang marx.wang@intel.com Change-Id: Ie4b17e5b4ce41c1adb436ae5646f0d8578a440e8 --- M src/soc/intel/cannonlake/include/soc/vr_config.h M src/soc/intel/cannonlake/vr_config.c 2 files changed, 114 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38741/10
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 10:
(9 comments)
https://review.coreboot.org/c/coreboot/+/38741/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38741/9//COMMIT_MSG@9 PS9, Line 9: Current(TDC)
Please add a space before (.
Done
https://review.coreboot.org/c/coreboot/+/38741/9//COMMIT_MSG@11 PS9, Line 11: 2.TdcPowerLimit
Please add a space after the enumerator(?). […]
Done
https://review.coreboot.org/c/coreboot/+/38741/9//COMMIT_MSG@13 PS9, Line 13: UG
BUG
Done
https://review.coreboot.org/c/coreboot/+/38741/9//COMMIT_MSG@15 PS9, Line 15: check FSP log
Check for what?
Done
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/vr_config.h:
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/in... PS9, Line 58: *
Please remove.
all the codes from corboot have this style, you could refer to ex, "gpio.c"
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/in... PS9, Line 61: /* Thermal Design Current(TDC) Power Limit in 1/8 A units */
Please add a space before the (.
Done
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/in... PS9, Line 67: #define VR_CFG_TDC_AMP(i) ((i) * 8)
should this have a cast as well?
Done
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/vr... PS9, Line 506: 0x%x
%#x
Done
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/vr... PS9, Line 507: "Auto/default settings\n", mch_id);
- Strings should be on one line. […]
Done
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/vr... PS9, Line 506: 0x%x
Done
0x%x is easier to read for debugging and also consistent with other debug messages.
Hello Patrick Rudolph, Kane Chen, Duncan Laurie, Shelley Chen, build bot (Jenkins), John Su, Furquan Shaikh, Jamie Chen, Thejaswani Putta, EricR Lai, Edward O'Callaghan, Mathew King, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38741
to look at the new patch set (#11).
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
soc/intel/cannonlake: Add tdc config for CML
Add Thermal Design Current (TDC) defaults for CML: 1. TdcEnable 2. TdcPowerLimit
BUG=b:148912093 BRANCH=None TEST=build coreboot and check TdcEnable and TdcPowerLimit for each domain in FSP log
Signed-off-by: Marx Wang marx.wang@intel.com Change-Id: Ie4b17e5b4ce41c1adb436ae5646f0d8578a440e8 --- M src/soc/intel/cannonlake/include/soc/vr_config.h M src/soc/intel/cannonlake/vr_config.c 2 files changed, 113 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38741/11
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/11/src/soc/intel/cannonlake/v... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/11/src/soc/intel/cannonlake/v... PS11, Line 506: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config, so apply defaults\n", mch_id); line over 96 characters
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/11/src/soc/intel/cannonlake/v... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/11/src/soc/intel/cannonlake/v... PS11, Line 506: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config, so apply defaults\n", mch_id);
line over 96 characters
Done
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/12/src/soc/intel/cannonlake/v... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/12/src/soc/intel/cannonlake/v... PS12, Line 506: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config, so apply defaults\n", mch_id); line over 96 characters
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 12:
(1 comment)
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/12/src/soc/intel/cannonlake/v... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/12/src/soc/intel/cannonlake/v... PS12, Line 506: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config, so apply defaults\n", mch_id);
line over 96 characters
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38741/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38741/12//COMMIT_MSG@16 PS12, Line 16: FSP log How do you check that? Is that in the coreboot console output, for example `cbmem -1`?
https://review.coreboot.org/c/coreboot/+/38741/12/src/soc/intel/cannonlake/i... File src/soc/intel/cannonlake/include/soc/vr_config.h:
https://review.coreboot.org/c/coreboot/+/38741/12/src/soc/intel/cannonlake/i... PS12, Line 58: * Remove.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/vr_config.h:
https://review.coreboot.org/c/coreboot/+/38741/9/src/soc/intel/cannonlake/in... PS9, Line 58: *
all the codes from corboot have this style, you could refer to ex, "gpio. […]
Ah, the file should be fixed in a follow-up then.
https://doc.coreboot.org/coding_style.html#commenting
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/12/src/soc/intel/cannonlake/i... File src/soc/intel/cannonlake/include/soc/vr_config.h:
https://review.coreboot.org/c/coreboot/+/38741/12/src/soc/intel/cannonlake/i... PS12, Line 58: *
Remove.
Sorry, I missed your reply. This should be fixed in a follow-up.
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 12:
(2 comments)
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38741/12//COMMIT_MSG@16 PS12, Line 16: FSP log
How do you check that? Is that in the coreboot console output, for example `cbmem -1`?
It needs to add "USE=fw_debug", cros_workon/emerge intel_cmlfsp project and capture the serial log while the device is booting up. Since intel_cmlfsp is a private project, I think anyone who has the access right to intel_cmlfsp should know how to get FSP log. Anyway, I'll add more details about how to generate FSP log.
https://review.coreboot.org/c/coreboot/+/38741/12/src/soc/intel/cannonlake/i... File src/soc/intel/cannonlake/include/soc/vr_config.h:
https://review.coreboot.org/c/coreboot/+/38741/12/src/soc/intel/cannonlake/i... PS12, Line 58: *
Sorry, I missed your reply. This should be fixed in a follow-up.
ok. will follow the coding style.
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 12:
(2 comments)
Patch Set 12:
(2 comments)
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38741/12//COMMIT_MSG@16 PS12, Line 16: FSP log
It needs to add "USE=fw_debug", cros_workon/emerge intel_cmlfsp project and capture the serial log w […]
Done
https://review.coreboot.org/c/coreboot/+/38741/12/src/soc/intel/cannonlake/i... File src/soc/intel/cannonlake/include/soc/vr_config.h:
https://review.coreboot.org/c/coreboot/+/38741/12/src/soc/intel/cannonlake/i... PS12, Line 58: *
ok. will follow the coding style.
Done
Marx Wang has uploaded a new patch set (#13) to the change originally created by Marx Wang. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
soc/intel/cannonlake: Add tdc config for CML
Add Thermal Design Current (TDC) defaults for CML: 1. TdcEnable 2. TdcPowerLimit
BUG=b:148912093 BRANCH=None TEST=build coreboot and intel FSP with fw_debug enabled, flash image to the device, capture the log from the serial port during boot-up and check TdcEnable and TdcPowerLimit for each domain in captured log
Signed-off-by: Marx Wang marx.wang@intel.com Change-Id: Ie4b17e5b4ce41c1adb436ae5646f0d8578a440e8 --- M src/soc/intel/cannonlake/include/soc/vr_config.h M src/soc/intel/cannonlake/vr_config.c 2 files changed, 113 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38741/13
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/13/src/soc/intel/cannonlake/v... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/13/src/soc/intel/cannonlake/v... PS13, Line 425: This could be const too.
Hello Patrick Rudolph, Kane Chen, Duncan Laurie, Shelley Chen, build bot (Jenkins), John Su, Furquan Shaikh, Jamie Chen, Thejaswani Putta, EricR Lai, Edward O'Callaghan, Mathew King, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38741
to look at the new patch set (#14).
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
soc/intel/cannonlake: Add tdc config for CML
Add Thermal Design Current (TDC) defaults for CML: 1. TdcEnable 2. TdcPowerLimit
BUG=b:148912093 BRANCH=None TEST=build coreboot and Intel FSP with fw_debug enabled, flash image to the device, capture the log from the serial port during boot-up and check TdcEnable and TdcPowerLimit for each domain in captured log
Signed-off-by: Marx Wang marx.wang@intel.com Change-Id: Ie4b17e5b4ce41c1adb436ae5646f0d8578a440e8 --- M src/soc/intel/cannonlake/include/soc/vr_config.h M src/soc/intel/cannonlake/vr_config.c 2 files changed, 113 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38741/14
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/14/src/soc/intel/cannonlake/v... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/14/src/soc/intel/cannonlake/v... PS14, Line 506: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config, so apply defaults\n", mch_id); line over 96 characters
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38741/13/src/soc/intel/cannonlake/v... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/13/src/soc/intel/cannonlake/v... PS13, Line 425:
This could be const too.
Done
https://review.coreboot.org/c/coreboot/+/38741/14/src/soc/intel/cannonlake/v... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/14/src/soc/intel/cannonlake/v... PS14, Line 506: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config, so apply defaults\n", mch_id);
line over 96 characters
Done
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/15/src/soc/intel/cannonlake/v... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/15/src/soc/intel/cannonlake/v... PS15, Line 506: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config, so apply defaults\n", mch_id); line over 96 characters
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/15/src/soc/intel/cannonlake/v... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/15/src/soc/intel/cannonlake/v... PS15, Line 506: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config, so apply defaults\n", mch_id);
line over 96 characters
Done
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 15: Code-Review+1
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 15:
Hi Tim and Shelley
Could you help to review this CL?
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/15/src/soc/intel/cannonlake/v... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/15/src/soc/intel/cannonlake/v... PS15, Line 506: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config, so apply defaults\n", mch_id);
Done
This looks like it is still over the limit?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 15: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/38741/15//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38741/15//COMMIT_MSG@7 PS15, Line 7: tdc nit, this should be uppercase: TDC
https://review.coreboot.org/c/coreboot/+/38741/15/src/soc/intel/cannonlake/v... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/15/src/soc/intel/cannonlake/v... PS15, Line 506: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config, so apply defaults\n", mch_id);
This looks like it is still over the limit?
I believe so. I think you can drop the "default" clause, or use the same string as in other functions (without "so apply defaults")
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add tdc config for CML ......................................................................
Patch Set 15:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38741/15//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38741/15//COMMIT_MSG@7 PS15, Line 7: tdc
nit, this should be uppercase: TDC
will change it. thanks
https://review.coreboot.org/c/coreboot/+/38741/15/src/soc/intel/cannonlake/v... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/15/src/soc/intel/cannonlake/v... PS15, Line 506: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config, so apply defaults\n", mch_id);
I believe so. […]
this is from Paul's comments in patch#9: 1. Strings should be on one line. 2. Maybe: Unknown MCH (%#x) in VR-config, so apply defaults And I saw many cases in other coreboot codes where one line contains 96+ characters so that I left it unmodified.
Hello Patrick Rudolph, Angel Pons, Kane Chen, Duncan Laurie, Shelley Chen, John Su, build bot (Jenkins), Furquan Shaikh, Jamie Chen, Thejaswani Putta, EricR Lai, Edward O'Callaghan, Mathew King, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38741
to look at the new patch set (#16).
Change subject: soc/intel/cannonlake: Add TDC config for CML ......................................................................
soc/intel/cannonlake: Add TDC config for CML
Add Thermal Design Current (TDC) defaults for CML: 1. TdcEnable 2. TdcPowerLimit
BUG=b:148912093 BRANCH=None TEST=build coreboot and Intel FSP with fw_debug enabled, flash image to the device, capture the log from the serial port during boot-up and check TdcEnable and TdcPowerLimit for each domain in captured log
Signed-off-by: Marx Wang marx.wang@intel.com Change-Id: Ie4b17e5b4ce41c1adb436ae5646f0d8578a440e8 --- M src/soc/intel/cannonlake/include/soc/vr_config.h M src/soc/intel/cannonlake/vr_config.c 2 files changed, 113 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38741/16
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add TDC config for CML ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/15/src/soc/intel/cannonlake/v... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/15/src/soc/intel/cannonlake/v... PS15, Line 506: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config, so apply defaults\n", mch_id);
this is from Paul's comments in patch#9: […]
I think Angel means you should consistent the log with https://review.coreboot.org/c/coreboot/+/38741/15/src/soc/intel/cannonlake/v...
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add TDC config for CML ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/15/src/soc/intel/cannonlake/v... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/15/src/soc/intel/cannonlake/v... PS15, Line 506: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config, so apply defaults\n", mch_id);
I think Angel means you should consistent the log with https://review.coreboot. […]
I understand. adding "apply defaults" is Paul's suggestion. please read my previous reply.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add TDC config for CML ......................................................................
Patch Set 16:
Patch Set 16:
(1 comment)
If so, you should modify #320 line as well. This is my meaning.
Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add TDC config for CML ......................................................................
Patch Set 16:
Patch Set 16:
Patch Set 16:
(1 comment)
If so, you should modify #320 line as well. This is my meaning.
please focus on what I upstreamed.
Hello Patrick Rudolph, Angel Pons, Kane Chen, Duncan Laurie, Shelley Chen, build bot (Jenkins), John Su, Furquan Shaikh, Jamie Chen, Thejaswani Putta, EricR Lai, Edward O'Callaghan, Mathew King, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38741
to look at the new patch set (#17).
Change subject: soc/intel/cannonlake: Add TDC config for CML ......................................................................
soc/intel/cannonlake: Add TDC config for CML
Add Thermal Design Current (TDC) defaults for CML: 1. TdcEnable 2. TdcPowerLimit
BUG=b:148912093 BRANCH=None TEST=build coreboot and Intel FSP with fw_debug enabled, flash image to the device, capture the log from the serial port during boot-up and check TdcEnable and TdcPowerLimit for each domain in captured log
Signed-off-by: Marx Wang marx.wang@intel.com Change-Id: Ie4b17e5b4ce41c1adb436ae5646f0d8578a440e8 --- M src/soc/intel/cannonlake/include/soc/vr_config.h M src/soc/intel/cannonlake/vr_config.c 2 files changed, 113 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38741/17
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add TDC config for CML ......................................................................
Patch Set 18: Code-Review+2
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add TDC config for CML ......................................................................
Patch Set 18: Code-Review+2
SGTM
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add TDC config for CML ......................................................................
Patch Set 18: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/15/src/soc/intel/cannonlake/v... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/38741/15/src/soc/intel/cannonlake/v... PS15, Line 506: printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config, so apply defaults\n", mch_id);
I understand. adding "apply defaults" is Paul's suggestion. please read my previous reply.
@Eric yes, I meant that. Apologies if I was not clear enough.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add TDC config for CML ......................................................................
soc/intel/cannonlake: Add TDC config for CML
Add Thermal Design Current (TDC) defaults for CML: 1. TdcEnable 2. TdcPowerLimit
BUG=b:148912093 BRANCH=None TEST=build coreboot and Intel FSP with fw_debug enabled, flash image to the device, capture the log from the serial port during boot-up and check TdcEnable and TdcPowerLimit for each domain in captured log
Signed-off-by: Marx Wang marx.wang@intel.com Change-Id: Ie4b17e5b4ce41c1adb436ae5646f0d8578a440e8 Reviewed-on: https://review.coreboot.org/c/coreboot/+/38741 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: EricR Lai ericr_lai@compal.corp-partner.google.com Reviewed-by: John Su john_su@compal.corp-partner.google.com Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/cannonlake/include/soc/vr_config.h M src/soc/intel/cannonlake/vr_config.c 2 files changed, 113 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved John Su: Looks good to me, approved EricR Lai: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/include/soc/vr_config.h b/src/soc/intel/cannonlake/include/soc/vr_config.h index 1390b17..456db5b 100644 --- a/src/soc/intel/cannonlake/include/soc/vr_config.h +++ b/src/soc/intel/cannonlake/include/soc/vr_config.h @@ -53,10 +53,18 @@ /* AC and DC Loadline in 1/100 mOhms. Range is 0-6249 */ uint16_t ac_loadline; uint16_t dc_loadline; + + /* Thermal Design Current (TDC) Power Limit will take effect when + this is set to 0 */ + uint8_t tdc_disable; + + /* Thermal Design Current (TDC) Power Limit in 1/8 A units */ + uint16_t tdc_powerlimit; };
#define VR_CFG_AMP(i) (uint16_t)((i) * 4) #define VR_CFG_MOHMS(i) (uint16_t)((i) * 100) +#define VR_CFG_TDC_AMP(i) (uint16_t)((i) * 8)
/* VrConfig Settings for 4 domains * 0 = System Agent, 1 = IA Core, @@ -85,6 +93,14 @@ [VR_GT_SLICED] = VR_CFG_MOHMS(gt_sl), \ }
+#define VR_CFG_ALL_DOMAINS_TDC(sa, ia, gt_unsl, gt_sl) \ + { \ + [VR_SYSTEM_AGENT] = VR_CFG_TDC_AMP(sa), \ + [VR_IA_CORE] = VR_CFG_TDC_AMP(ia), \ + [VR_GT_UNSLICED] = VR_CFG_TDC_AMP(gt_unsl), \ + [VR_GT_SLICED] = VR_CFG_TDC_AMP(gt_sl), \ + } + void fill_vr_domain_config(void *params, int domain, const struct vr_config *cfg);
diff --git a/src/soc/intel/cannonlake/vr_config.c b/src/soc/intel/cannonlake/vr_config.c index 5fadcf4..bd73d15 100644 --- a/src/soc/intel/cannonlake/vr_config.c +++ b/src/soc/intel/cannonlake/vr_config.c @@ -419,6 +419,96 @@ return 1520; }
+static uint16_t get_sku_tdc_powerlimit(int domain) +{ + const uint16_t tdp = cpu_get_power_max(); + const config_t *cfg = config_of_soc(); + + static uint16_t mch_id = 0; + if (!mch_id) { + struct device *dev = pcidev_path_on_root(SA_DEVFN_ROOT); + mch_id = dev ? pci_read_config16(dev, PCI_DEVICE_ID) : 0xffff; + } + + switch (mch_id) { + case PCI_DEVICE_ID_INTEL_CML_ULT: + case PCI_DEVICE_ID_INTEL_CML_ULT_6_2: { + uint16_t tdc[NUM_VR_DOMAINS] = + VR_CFG_ALL_DOMAINS_TDC(4, 58, 22, 22); + + if (cfg->cpu_pl2_4_cfg == baseline) + tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(48); + + return tdc[domain]; + } + case PCI_DEVICE_ID_INTEL_CML_ULT_2_2: { + const uint16_t tdc[NUM_VR_DOMAINS] = + VR_CFG_ALL_DOMAINS_TDC(4, 24, 22, 22); + return tdc[domain]; + } + case PCI_DEVICE_ID_INTEL_CML_H_4_2: { + uint16_t tdc[NUM_VR_DOMAINS] = + VR_CFG_ALL_DOMAINS_TDC(10, 80, 25, 25); + + if (cfg->cpu_pl2_4_cfg == baseline) + tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(60); + + return tdc[domain]; + } + case PCI_DEVICE_ID_INTEL_CML_H: { + uint16_t tdc[NUM_VR_DOMAINS] = + VR_CFG_ALL_DOMAINS_TDC(10, 92, 25, 25); + + if (cfg->cpu_pl2_4_cfg == baseline) + tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(80); + + return tdc[domain]; + } + case PCI_DEVICE_ID_INTEL_CML_H_8_2: { + uint16_t tdc[NUM_VR_DOMAINS] = + VR_CFG_ALL_DOMAINS_TDC(10, 125, 25, 25); + + if (tdp >= 65) /* 65W */ + tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? + VR_CFG_TDC_AMP(117) : + VR_CFG_TDC_AMP(146); + else /* 45W */ + tdc[VR_IA_CORE] = (cfg->cpu_pl2_4_cfg == baseline) ? + VR_CFG_TDC_AMP(86) : + VR_CFG_TDC_AMP(125); + + return tdc[domain]; + } + case PCI_DEVICE_ID_INTEL_CML_S_G0G1_P0P1_6_2: { + uint16_t tdc[NUM_VR_DOMAINS] = + VR_CFG_ALL_DOMAINS_TDC(10, 74, 28, 28); + + if (tdp >= 125) /* 125W */ + tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(132); + else if (tdp >= 65) /* 80W or 65W */ + tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(104); + else /* 35W */ + tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(74); + + return tdc[domain]; + } + case PCI_DEVICE_ID_INTEL_CML_S_P0P1_8_2: + case PCI_DEVICE_ID_INTEL_CML_S_P0P1_10_2: { + uint16_t tdc[NUM_VR_DOMAINS] = + VR_CFG_ALL_DOMAINS_TDC(10, 100, 28, 28); + + if (tdp > 35) /* 125W or 80W or 65W */ + tdc[VR_IA_CORE] = VR_CFG_TDC_AMP(175); + + return tdc[domain]; + } + default: + printk(BIOS_ERR, "ERROR: Unknown MCH (0x%x) in VR-config\n", mch_id); + } + + return 0; +} + void fill_vr_domain_config(void *params, int domain, const struct vr_config *chip_cfg) { @@ -463,4 +553,11 @@ vr_params->DcLoadline[domain] = cfg->dc_loadline; else vr_params->DcLoadline[domain] = get_sku_ac_dc_loadline(domain); + + vr_params->TdcEnable[domain] = !cfg->tdc_disable; + + if (cfg->tdc_powerlimit) + vr_params->TdcPowerLimit[domain] = cfg->tdc_powerlimit; + else + vr_params->TdcPowerLimit[domain] = get_sku_tdc_powerlimit(domain); }
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38741 )
Change subject: soc/intel/cannonlake: Add TDC config for CML ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38741/19/src/soc/intel/cannonlake/i... File src/soc/intel/cannonlake/include/soc/vr_config.h:
https://review.coreboot.org/c/coreboot/+/38741/19/src/soc/intel/cannonlake/i... PS19, Line 59: tdc_disable all SoC default to 0, even those that doesn't specify a TDC. What does FSP if this is enabled and TDC powerlimit is 0?
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Duncan Laurie (Code Review)
-
EricR Lai (Code Review)
-
John Su (Code Review)
-
Kane Chen (Code Review)
-
Marx Wang (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Tim Wawrzynczak (Code Review)