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.