Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37466 )
Change subject: soc/intel/cannonlake: Add VR config for CFL and WHL ......................................................................
Patch Set 2:
(8 comments)
https://review.coreboot.org/c/coreboot/+/37466/2/src/soc/intel/cannonlake/vr... File src/soc/intel/cannonlake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/37466/2/src/soc/intel/cannonlake/vr... PS2, Line 33: 0x0 Why not changes these values to decimal as well.
https://review.coreboot.org/c/coreboot/+/37466/2/src/soc/intel/cannonlake/vr... PS2, Line 121: ? 13A
https://review.coreboot.org/c/coreboot/+/37466/2/src/soc/intel/cannonlake/vr... PS2, Line 121: * CNL-U (15W) ? ? 0 34A
https://review.coreboot.org/c/coreboot/+/37466/2/src/soc/intel/cannonlake/vr... PS2, Line 151: 8.5 Shouldn't you add a cast to VR_CFG_AMP(i) ((i) * 4) when values like this are used?
https://review.coreboot.org/c/coreboot/+/37466/2/src/soc/intel/cannonlake/vr... PS2, Line 305: vr_params->IccMax[domain] = cfg->icc_max; This is handled below.
https://review.coreboot.org/c/coreboot/+/37466/2/src/soc/intel/cannonlake/vr... PS2, Line 306: vr_params->VrVoltageLimit[domain] = cfg->voltage_limit; Isn't it an idea to make this conditional as well so we don't have to put 1520 in every device tree and can just leave out the value unless it should differ from the default?
https://review.coreboot.org/c/coreboot/+/37466/2/src/soc/intel/cannonlake/vr... PS2, Line 307: vr_params->AcLoadline[domain] = cfg->ac_loadline; : vr_params->DcLoadline[domain] = cfg->dc_loadline; These lines can be removed, they are handled below.
https://review.coreboot.org/c/coreboot/+/37466/2/src/soc/intel/cannonlake/vr... PS2, Line 320: if (cfg->dc_loadline) Adding a blank line before this will make it more readable.