Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43904 )
Change subject: cpu/intel/common: Fill cpu voltage in SMBIOS tables ......................................................................
Patch Set 7: Code-Review+2
(3 comments)
https://review.coreboot.org/c/coreboot/+/43904/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43904/7//COMMIT_MSG@9 PS7, Line 9: Introduce a weak function to let the platform code provide the processor : voltage in 100mV units. : : Implement the function on Intel platforms using the MSR_PERF_STATUS msr. : On other platforms the processor voltage still reads as unknown. It's generally a good idea to put generic and platform-specific changes into separate commits. Just for the future, this was still easy enough to review.
https://review.coreboot.org/c/coreboot/+/43904/7/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/43904/7/src/arch/x86/smbios.c@497 PS7, Line 497: unsigned int __weak smbios_cpu_get_voltage(void) Just a thought: It's generally a good idea to encode units into identifiers. Then we need less easy-to-ignore comments about it. e.g.
unsigned int __weak smbios_cpu_get_voltage_100mV(void)
https://review.coreboot.org/c/coreboot/+/43904/7/src/arch/x86/smbios.c@500 PS7, Line 500: } People started to move such weak default functions into `smbios_defaults.c`. Not sure what to do about it as there are also the new functions above.