Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39346 )
Change subject: soc/intel/tigerlake: Add processor power limits control support ......................................................................
Patch Set 22:
(11 comments)
This is looking pretty good! Thanks, Sumeet. I think breaking this up into a few CLs might make it a little more digestible.
My suggestion: 1) A patch to add the set_power_limits functionality into soc/intel/common 2) A patch to add the Kconfigs to enable the one in soc/intel/common (CPU_INTEL_COMMON & SOC_INTEL_COMMON_BLOCK_POWER_LIMIT and friends), and also remove the extra versions of set_power_limits(). This will be the big one as it will also have the struct power_limits_config that needs to be handled everywhere. 3) Continue like you did on top of this patch.
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/cannonlake/f... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/cannonlake/f... PS22, Line 157: struct soc_power_limits_config *soc_confg; const
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/dptf.asl:
PS22: What made this file necessary?
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/power_limit.h:
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/common/block... PS22, Line 28: Used to configure Maybe say 'default' ?
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/common/block... PS22, Line 33: uint32_t This should be OK as uint8, I don't think you can set PL1 more than 255 W 😊 uint16_t if we want to consider the case of desktop or server SKUs at some point in the future.
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/common/block... PS22, Line 35: tdp_pl2_override Same thing? will uint8_t or uint16_t work?
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/common/block... PS22, Line 37: tdp_psyspl2 Same
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/common/block... PS22, Line 39: tdp_psyspl3 Same
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/common/block... PS22, Line 41: tdp_psyspl3_time Same
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/common/block... PS22, Line 43: tdp_psyspl3_dutycycle Same
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/common/block... PS22, Line 45: tdp_pl4 Same
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/common/block... PS22, Line 47: psys_pmax Same