Attention is currently required from: Máté Kukri, Paul Menzel, Varshit Pandya.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81529?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: mb/dell/optiplex_9020: Implement late HWM initialization ......................................................................
Patch Set 9:
(2 comments)
File src/mainboard/dell/optiplex_9020/mainboard.c:
https://review.coreboot.org/c/coreboot/+/81529/comment/a78f6db0_967c4274 : PS9, Line 296: static uint8_t get_temp_target(void) : { : uint8_t val = rdmsr(0x1a2).lo >> 8 & 0xff; : if (!val) : val = 20; : return 0x95 - val; : } : : static uint16_t get_pkg_power(void) : { : uint8_t rapl_power_unit = rdmsr(0x606).lo & 0xf; : if (rapl_power_unit) : rapl_power_unit = 2 << (rapl_power_unit - 1); : uint16_t pkg_power_info = rdmsr(0x614).lo & 0x7fff; : if (pkg_power_info / rapl_power_unit > 0x41) : return 32; : else : return 16; : } looks to me that those aren't mainboard-specific, but soc-specific. would be good to move those to the soc code. i'm ok with this being done as a follow-up cleanup though. also please use the existing defines for the msrs instead of magic values
File src/mainboard/dell/optiplex_9020/sch5555_ec.c:
https://review.coreboot.org/c/coreboot/+/81529/comment/61635a97_d49abc4e : PS9, Line 8: ec_read this function name is already used by the generic ec access code, so would be good to rename the two functions in this file to something more specific like ec_read_sch555x. at least i find it a bit confusing when there's a function with the same name as the generic one that has a different function signature. from a quick look, the common ec_read/ec_write probably can't be used here due to a different interface