Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Sumeet R Pawnikar, Patrick Rudolph. Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54676 )
Change subject: ADL: Add SKU specific power limits support ......................................................................
Patch Set 3:
(3 comments)
File src/mainboard/intel/adlrvp/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/54676/comment/cb180afd_12755d76 PS3, Line 151: Always prefer to keep the MB and SoC changes in separate CLs.
File src/soc/intel/alderlake/chip.h:
https://review.coreboot.org/c/coreboot/+/54676/comment/ce5a0019_6d0e8b03 PS3, Line 20: /* Types of different SKUs */ : #define ADL_P_POWER_LIMITS_282_CORE 1 : #define ADL_P_POWER_LIMITS_482_CORE 2 : #define ADL_P_POWER_LIMITS_682_CORE 3 : #define ADL_M_POWER_LIMITS_282_CORE 4 : #define POWER_LIMITS_MAX 5 Seems like enum is a more appropriate choice for this. Also there are some nits about white-spaces. Some have tabs while some have single space.
File src/soc/intel/alderlake/systemagent.c:
https://review.coreboot.org/c/coreboot/+/54676/comment/b66b5e8b_2ac26f9f PS3, Line 78: sa_pci_id Do we need to ensure that the power limits are filled in for the concerned SoC SKU? Or atleast print a warning?