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