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:
Patch Set 22:
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.
Yes, there is a dependency. I think it can be broken by first just adding a patch to copy the functionality to common code, then another patch to switch the devicetree's over to the new common code, and then one last one to remove the old set_power_limits() functions from each SoC. I think that will compile and also make each patch easier to review.
while entering my previous comments, I missed this one. sorry for that.