Attention is currently required from: Tarun Tuli, Sumeet R Pawnikar, Eric Lai.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74380 )
Change subject: soc/intel/meteoerlake: set power limits dynamically ......................................................................
Patch Set 2:
(5 comments)
File src/soc/intel/meteorlake/chip.h:
https://review.coreboot.org/c/coreboot/+/74380/comment/7b76bc16_a043c8f4 PS2, Line 33: static const struct wondering if u have more consumers of this data structure beyond systemagent.c file ?
File src/soc/intel/meteorlake/systemagent.c:
https://review.coreboot.org/c/coreboot/+/74380/comment/8cf761c4_793a02ab PS2, Line 152: soc_systemagent_init i would have refactored this code as below ``` void soc_systemagent_init(struct device *dev) { /* Enable Power Aware Interrupt Routing */ enable_power_aware_intr(); /* Configure TDP */ configure_tdp(dev); } ``` inside configure_tdp: ``` void configure_tdp(struct device *dev) { struct device *sa; /* Get System Agent PCI ID */ sa = pcidev_path_on_root(PCI_DEVFN_ROOT); sa_pci_id = sa ? pci_read_config16(sa, PCI_DEVICE_ID) : 0xFFFF; if (sa_pci_id == 0xFFFF) { printk(BIOS_WARNING, "Unknown SA PCI Device!\n"); return; } ...
} ```
https://review.coreboot.org/c/coreboot/+/74380/comment/783c11a7_71d13647 PS2, Line 168: 0xFFFF shouldn't we skip the below operations if ID is 0xFFFF? what is the point of running into the `for` loop when id itself is not valid?
https://review.coreboot.org/c/coreboot/+/74380/comment/54326f5f_760745b8 PS2, Line 183: if (i == ARRAY_SIZE(cpuid_to_mtl)) { can u use a bool to know if config_tdp is successful ?
https://review.coreboot.org/c/coreboot/+/74380/comment/93a001c7_7c9bc8ef PS2, Line 188: printk(BIOS_DEBUG, "Configured power limits for SA ID: 0x%4x\n", sa_pci_id); please move this line into line#178 to avoid a else case here.