Attention is currently required from: Paul Menzel, Reka Norman, Ryan Lin, Subrata Banik, Sumeet R Pawnikar, Zhuohao Lee.
ChiaLing has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75679?usp=email )
Change subject: soc/intel/jasperlake: Add per-SKU power limits ......................................................................
Patch Set 7:
(2 comments)
File src/mainboard/google/dedede/variants/blipper/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/75679/comment/63a28416_d0a97fa7 : PS5, Line 62: register "power_limits_config" = "{ : .tdp_pl1_override = 6, : .tdp_pl2_override = 20, : }"
Sorry, this is actually not equivalent because the baseboard sets pl4 too. […]
Hi Reka,
I set all skus power limit in dedede baseboard including PL1, PL2, PL4. If the override values in each variant are equivalent to default value, I think it don't need to set again since devicetree can take care of them. But I'm not sure if my understanding is correct or not. How do you think? Should I still put sku power limit default value back here?
File src/soc/intel/jasperlake/systemagent.c:
https://review.coreboot.org/c/coreboot/+/75679/comment/70c60d51_74bcfdde : PS5, Line 71: /* Choose power limits configuration based on the CPU SA PCI ID and : * CPU TDP value. */
Use the common code as per this CL https://review.coreboot.org/c/coreboot/+/74620 […]
Hi Sumeet, I'm quite confuse because your patch is to get PLX value from project ramstage.c pwr_limits structure, which contain each CPU PCI DID power limit default value. But my patch here is for setting all PLX value to MSR register. I think they are not the same. Could you please explain more? Also, I check the same file in soc/intel/meteorlake/systemagent.c. On meteorlake, set_power_limit() api and corresponding "for" loop is the same as I use in soc/intel/jasperlake/systemagent.c. Could you explain more about which part I should change?