Sumeet R Pawnikar 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:
(10 comments)
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:
- A patch to add the set_power_limits functionality into soc/intel/common
- 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.
- Continue like you did on top of this patch.
Thanks for these suggestions. I am working on this and will try to incorporate. Still I observe there is dependencies between mb/google/.../devicetree.cb files and src/soc/intel/.../* files.
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
Ack
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' ?
Ok
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 […]
I would consider to use uint16_t considering the need 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?
planning to use uint16_t
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/common/block... PS22, Line 37: tdp_psyspl2
Same
same as above, planning to use uint16_t
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/common/block... PS22, Line 39: tdp_psyspl3
Same
same as above, planning to use uint16_t
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/common/block... PS22, Line 41: tdp_psyspl3_time
Same
same as above, planning to use uint16_t
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/common/block... PS22, Line 43: tdp_psyspl3_dutycycle
Same
same as above, planning to use uint16_t
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/common/block... PS22, Line 45: tdp_pl4
Same
same as above planning to use uint16_t
https://review.coreboot.org/c/coreboot/+/39346/22/src/soc/intel/common/block... PS22, Line 47: psys_pmax
Same
same as above planning to use uint16_t