Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37466 )
Change subject: soc/intel/cannonlake: Add VR config for CFL, CNL and WHL ......................................................................
Patch Set 4:
(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.
Done
https://review.coreboot.org/c/coreboot/+/37466/2/src/soc/intel/cannonlake/vr... PS2, Line 121: ?
13A
Done
https://review.coreboot.org/c/coreboot/+/37466/2/src/soc/intel/cannonlake/vr... PS2, Line 121: * CNL-U (15W) ? ? 0
34A
Done
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?
It will be converted to int even without a cast
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.
Done
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 […]
Done
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.
Done
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.
Done