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.