Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43904 )
Change subject: cpu/intel/common: Fill cpu voltage in SMBIOS tables ......................................................................
cpu/intel/common: Fill cpu voltage in SMBIOS tables
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.
Change-Id: I31a7efcbeede50d986a1c096a4a59a316e09f825 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/arch/x86/smbios.c M src/cpu/intel/common/Kconfig M src/cpu/intel/common/Makefile.inc A src/cpu/intel/common/voltage.c M src/cpu/intel/haswell/Kconfig M src/cpu/intel/model_2065x/Kconfig M src/include/smbios.h M src/soc/intel/common/block/cpu/Kconfig 8 files changed, 31 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/43904/1
diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c index 9a7029b..8600fba 100644 --- a/src/arch/x86/smbios.c +++ b/src/arch/x86/smbios.c @@ -575,6 +575,12 @@ return 0; /* Unknown */ }
+/* Returns the processor voltage in 100mV units */ +unsigned int __weak smbios_cpu_get_voltage(void) +{ + return 0; /* Unknown */ +} + const char *__weak smbios_system_sku(void) { return ""; @@ -748,6 +754,9 @@ t->max_speed = smbios_cpu_get_max_speed_mhz(); t->current_speed = smbios_cpu_get_current_speed_mhz(); } + if (smbios_cpu_get_voltage() > 0) + t->voltage = 0x80 | smbios_cpu_get_voltage(); + *current += len; return len; } diff --git a/src/cpu/intel/common/Kconfig b/src/cpu/intel/common/Kconfig index 0f2a652..1e8bb6c 100644 --- a/src/cpu/intel/common/Kconfig +++ b/src/cpu/intel/common/Kconfig @@ -22,6 +22,9 @@ config CPU_INTEL_COMMON_TIMEBASE bool
+config CPU_INTEL_COMMON_VOLTAGE + bool + config CPU_INTEL_COMMON_HYPERTHREADING bool
diff --git a/src/cpu/intel/common/Makefile.inc b/src/cpu/intel/common/Makefile.inc index 1612012..0dda339 100644 --- a/src/cpu/intel/common/Makefile.inc +++ b/src/cpu/intel/common/Makefile.inc @@ -1,5 +1,6 @@ ramstage-$(CONFIG_CPU_INTEL_COMMON) += common_init.c ramstage-$(CONFIG_CPU_INTEL_COMMON_HYPERTHREADING) += hyperthreading.c +ramstage-$(CONFIG_CPU_INTEL_COMMON_VOLTAGE) += voltage.c
ifeq ($(CONFIG_CPU_INTEL_COMMON_TIMEBASE),y) bootblock-y += fsb.c diff --git a/src/cpu/intel/common/voltage.c b/src/cpu/intel/common/voltage.c new file mode 100644 index 0000000..82765b0 --- /dev/null +++ b/src/cpu/intel/common/voltage.c @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <cpu/x86/msr.h> +#include <smbios.h> + +/* This is not an architectural MSR. */ +#define MSR_PERF_STATUS 0x198 + +#if CONFIG(GENERATE_SMBIOS_TABLES) +unsigned int smbios_cpu_get_voltage(void) +{ + return (rdmsr(MSR_PERF_STATUS).hi & 0xffff) * 10 / 8192; +} +#endif diff --git a/src/cpu/intel/haswell/Kconfig b/src/cpu/intel/haswell/Kconfig index 54cb99f..127ae82 100644 --- a/src/cpu/intel/haswell/Kconfig +++ b/src/cpu/intel/haswell/Kconfig @@ -22,6 +22,7 @@ select PARALLEL_MP select CPU_INTEL_COMMON select CPU_INTEL_COMMON_TIMEBASE + select CPU_INTEL_COMMON_VOLTAGE
config SMM_TSEG_SIZE hex diff --git a/src/cpu/intel/model_2065x/Kconfig b/src/cpu/intel/model_2065x/Kconfig index 39beb22..6cb2e4c 100644 --- a/src/cpu/intel/model_2065x/Kconfig +++ b/src/cpu/intel/model_2065x/Kconfig @@ -19,6 +19,7 @@ select TSC_SYNC_MFENCE select CPU_INTEL_COMMON select CPU_INTEL_COMMON_TIMEBASE + select CPU_INTEL_COMMON_VOLTAGE select PARALLEL_MP
config SMM_TSEG_SIZE diff --git a/src/include/smbios.h b/src/include/smbios.h index 25ea897..48a4de4 100644 --- a/src/include/smbios.h +++ b/src/include/smbios.h @@ -40,6 +40,7 @@
unsigned int smbios_cpu_get_max_speed_mhz(void); unsigned int smbios_cpu_get_current_speed_mhz(void); +unsigned int smbios_cpu_get_voltage(void);
const char *smbios_mainboard_manufacturer(void); const char *smbios_mainboard_product_name(void); diff --git a/src/soc/intel/common/block/cpu/Kconfig b/src/soc/intel/common/block/cpu/Kconfig index 3c29b24..70343fc 100644 --- a/src/soc/intel/common/block/cpu/Kconfig +++ b/src/soc/intel/common/block/cpu/Kconfig @@ -1,5 +1,6 @@ config SOC_INTEL_COMMON_BLOCK_CPU bool + select CPU_INTEL_COMMON_VOLTAGE default n help This option selects Intel Common CPU Model support code
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43904
to look at the new patch set (#2).
Change subject: cpu/intel/common: Fill cpu voltage in SMBIOS tables ......................................................................
cpu/intel/common: Fill cpu voltage in SMBIOS tables
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.
Change-Id: I31a7efcbeede50d986a1c096a4a59a316e09f825 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/arch/x86/smbios.c M src/cpu/intel/common/Kconfig M src/cpu/intel/common/Makefile.inc A src/cpu/intel/common/voltage.c M src/cpu/intel/haswell/Kconfig M src/cpu/intel/model_2065x/Kconfig M src/include/smbios.h M src/soc/intel/common/block/cpu/Kconfig 8 files changed, 32 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/43904/2
Marcello Sylvester Bauer 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 2:
diff `dmidecode -t4` on a ThinkPad X220: ``` 3c3 < SMBIOS 2.8 present. ---
SMBIOS 3.0 present.
5c5 < Handle 0x0004, DMI type 4, 42 bytes ---
Handle 0x0004, DMI type 4, 48 bytes
45c45 < Max Speed: Unknown ---
Max Speed: 2600 MHz
47c47 < Status: Populated, Enabled ---
Status: Unpopulated
```
voltage is still unknown
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43904
to look at the new patch set (#3).
Change subject: cpu/intel/common: Fill cpu voltage in SMBIOS tables ......................................................................
cpu/intel/common: Fill cpu voltage in SMBIOS tables
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.
Tested on Intel CFL. The CPU voltage is correctly advertised.
Change-Id: I31a7efcbeede50d986a1c096a4a59a316e09f825 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/arch/x86/smbios.c M src/cpu/intel/common/Kconfig M src/cpu/intel/common/Makefile.inc A src/cpu/intel/common/voltage.c M src/cpu/intel/haswell/Kconfig M src/cpu/intel/model_2065x/Kconfig M src/include/smbios.h M src/soc/intel/common/block/cpu/Kconfig 8 files changed, 32 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/43904/3
Angel Pons 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 3: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/43904/3/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/43904/3/src/arch/x86/smbios.c@754 PS3, Line 754: if (smbios_cpu_get_voltage() > 0) I don't like that this function is called twice
https://review.coreboot.org/c/coreboot/+/43904/3/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/43904/3/src/soc/intel/common/block/... PS3, Line 3: select CPU_INTEL_COMMON I don't really like that we need to select CPU_INTEL_COMMON here. Could we move the CPU_INTEL_COMMON_VOLTAGE symbol so that it doesn't depend on CPU_INTEL_COMMON?
Angel Pons has uploaded a new patch set (#4) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/43904 )
Change subject: cpu/intel/common: Fill cpu voltage in SMBIOS tables ......................................................................
cpu/intel/common: Fill cpu voltage in SMBIOS tables
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.
Tested on Intel CFL. The CPU voltage is correctly advertised.
Change-Id: I31a7efcbeede50d986a1c096a4a59a316e09f825 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/arch/x86/smbios.c M src/cpu/intel/common/Kconfig M src/cpu/intel/common/Makefile.inc A src/cpu/intel/common/voltage.c M src/cpu/intel/haswell/Kconfig M src/cpu/intel/model_2065x/Kconfig M src/include/smbios.h M src/soc/intel/common/block/cpu/Kconfig 8 files changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/43904/4
Angel Pons 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 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43904/3/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/43904/3/src/arch/x86/smbios.c@754 PS3, Line 754: if (smbios_cpu_get_voltage() > 0)
I don't like that this function is called twice
Done
https://review.coreboot.org/c/coreboot/+/43904/3/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/43904/3/src/soc/intel/common/block/... PS3, Line 3: select CPU_INTEL_COMMON
I don't really like that we need to select CPU_INTEL_COMMON here. […]
Done
Tim Chu 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 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43904/4/src/cpu/intel/common/voltag... File src/cpu/intel/common/voltage.c:
https://review.coreboot.org/c/coreboot/+/43904/4/src/cpu/intel/common/voltag... PS4, Line 12: return (rdmsr(MSR_PERF_STATUS).hi & 0xffff) * 10 / 8192; A question about this function. It seems not all intel platforms support this field(such as CPX). Not sure it is fine to select this function in SOC_INTEL_COMMON_BLOCK_CPU.
HAOUAS Elyes 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 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43904/4/src/cpu/intel/common/voltag... File src/cpu/intel/common/voltage.c:
https://review.coreboot.org/c/coreboot/+/43904/4/src/cpu/intel/common/voltag... PS4, Line 12: 0xffff Core Voltage = MSR_PERF_STATUS[37:32] * (float) 1/(2^13). so, please why "0xffff"?
Jonathan Zhang 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 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43904/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/43904/4/src/soc/intel/common/block/... PS4, Line 3: select CPU_INTEL_COMMON_VOLTAGE Along the same line as Tim commented in https://review.coreboot.org/c/coreboot/+/43904/4/src/cpu/intel/common/voltag..., CPX-SP does have CONFIG_SOC_INTEL_COMMON_BLOCK_CPU selected, but its MSR 198h does not have those fields (doc 604926), so it should not have CPU_INTEL_COMMON_VOLTAGE selected.
Patrick Rudolph 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 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43904/4/src/cpu/intel/common/voltag... File src/cpu/intel/common/voltage.c:
https://review.coreboot.org/c/coreboot/+/43904/4/src/cpu/intel/common/voltag... PS4, Line 12: return (rdmsr(MSR_PERF_STATUS).hi & 0xffff) * 10 / 8192;
A question about this function. It seems not all intel platforms support this field(such as CPX). […]
What does it return? Does it crash?
Tim Chu 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 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43904/4/src/cpu/intel/common/voltag... File src/cpu/intel/common/voltage.c:
https://review.coreboot.org/c/coreboot/+/43904/4/src/cpu/intel/common/voltag... PS4, Line 12: return (rdmsr(MSR_PERF_STATUS).hi & 0xffff) * 10 / 8192;
What does it return? Does it crash?
It won't crash. It returns 6, and finally get 0.6V in smbios table.
Jonathan Zhang 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 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43904/4/src/cpu/intel/common/voltag... File src/cpu/intel/common/voltage.c:
https://review.coreboot.org/c/coreboot/+/43904/4/src/cpu/intel/common/voltag... PS4, Line 12: return (rdmsr(MSR_PERF_STATUS).hi & 0xffff) * 10 / 8192;
It won't crash. It returns 6, and finally get 0.6V in smbios table.
What is the expected value for CPX-SP?
Tim Chu 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 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43904/4/src/cpu/intel/common/voltag... File src/cpu/intel/common/voltage.c:
https://review.coreboot.org/c/coreboot/+/43904/4/src/cpu/intel/common/voltag... PS4, Line 12: return (rdmsr(MSR_PERF_STATUS).hi & 0xffff) * 10 / 8192;
What is the expected value for CPX-SP?
1.6V is expected.
Tim Chu has uploaded a new patch set (#5) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/43904 )
Change subject: cpu/intel/common: Fill cpu voltage in SMBIOS tables ......................................................................
cpu/intel/common: Fill cpu voltage in SMBIOS tables
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.
Tested on Intel CFL. The CPU voltage is correctly advertised.
Change-Id: I31a7efcbeede50d986a1c096a4a59a316e09f825 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/arch/x86/smbios.c M src/cpu/intel/common/Kconfig M src/cpu/intel/common/Makefile.inc A src/cpu/intel/common/voltage.c M src/cpu/intel/haswell/Kconfig M src/cpu/intel/model_2065x/Kconfig M src/include/smbios.h 7 files changed, 37 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/43904/5
Tim Chu 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 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43904/4/src/cpu/intel/common/voltag... File src/cpu/intel/common/voltage.c:
https://review.coreboot.org/c/coreboot/+/43904/4/src/cpu/intel/common/voltag... PS4, Line 12: 0xffff
Core Voltage = MSR_PERF_STATUS[37:32] * (float) 1/(2^13). […]
current voltage = float([47:32] / (2^13)). so it is 0xffff
https://review.coreboot.org/c/coreboot/+/43904/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/43904/4/src/soc/intel/common/block/... PS4, Line 3: select CPU_INTEL_COMMON_VOLTAGE
Along the same line as Tim commented in https://review.coreboot. […]
I remove this line and keep other changes. Not sure this is fine for other platform.
Jonathan Zhang 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 5: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/43904/5/src/cpu/intel/common/Makefi... File src/cpu/intel/common/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/43904/5/src/cpu/intel/common/Makefi... PS5, Line 3: ramstage-$(CONFIG_CPU_INTEL_COMMON_HYPERTHREADING) += hyperthreading.c why do we add this line?
https://review.coreboot.org/c/coreboot/+/43904/4/src/cpu/intel/common/voltag... File src/cpu/intel/common/voltage.c:
https://review.coreboot.org/c/coreboot/+/43904/4/src/cpu/intel/common/voltag... PS4, Line 12: return (rdmsr(MSR_PERF_STATUS).hi & 0xffff) * 10 / 8192;
1.6V is expected.
Ack
https://review.coreboot.org/c/coreboot/+/43904/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/43904/4/src/soc/intel/common/block/... PS4, Line 3: select CPU_INTEL_COMMON_VOLTAGE
I remove this line and keep other changes. Not sure this is fine for other platform.
Ack
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 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43904/5/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/43904/5/src/arch/x86/smbios.c@505 PS5, Line 505: } This seems unrelated. Also, the function already exists, AFAICS.
https://review.coreboot.org/c/coreboot/+/43904/5/src/cpu/intel/common/voltag... File src/cpu/intel/common/voltage.c:
https://review.coreboot.org/c/coreboot/+/43904/5/src/cpu/intel/common/voltag... PS5, Line 9: #if CONFIG(GENERATE_SMBIOS_TABLES) There is no need to guard it. If we build without SMBIOS tables, it will simply not be linked into the final binary.
https://review.coreboot.org/c/coreboot/+/43904/5/src/cpu/intel/model_2065x/K... File src/cpu/intel/model_2065x/Kconfig:
https://review.coreboot.org/c/coreboot/+/43904/5/src/cpu/intel/model_2065x/K... PS5, Line 4: if CPU_INTEL_MODEL_2065X Was this confirmed? In the SDM it's only documented since Sandy Bridge.
Tim Chu has uploaded a new patch set (#6) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/43904 )
Change subject: cpu/intel/common: Fill cpu voltage in SMBIOS tables ......................................................................
cpu/intel/common: Fill cpu voltage in SMBIOS tables
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.
Tested on Intel CFL. The CPU voltage is correctly advertised.
Change-Id: I31a7efcbeede50d986a1c096a4a59a316e09f825 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/arch/x86/smbios.c M src/cpu/intel/common/Kconfig M src/cpu/intel/common/Makefile.inc A src/cpu/intel/common/voltage.c M src/cpu/intel/haswell/Kconfig M src/cpu/intel/model_2065x/Kconfig M src/include/smbios.h 7 files changed, 31 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/43904/6
Tim Chu has uploaded a new patch set (#7) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/43904 )
Change subject: cpu/intel/common: Fill cpu voltage in SMBIOS tables ......................................................................
cpu/intel/common: Fill cpu voltage in SMBIOS tables
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.
Tested on Intel CFL. The CPU voltage is correctly advertised.
Change-Id: I31a7efcbeede50d986a1c096a4a59a316e09f825 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/arch/x86/smbios.c M src/cpu/intel/common/Kconfig M src/cpu/intel/common/Makefile.inc A src/cpu/intel/common/voltage.c M src/cpu/intel/haswell/Kconfig M src/include/smbios.h 6 files changed, 28 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/43904/7
Tim Chu 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:
(4 comments)
https://review.coreboot.org/c/coreboot/+/43904/5/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/43904/5/src/arch/x86/smbios.c@505 PS5, Line 505: }
This seems unrelated. Also, the function already exists, AFAICS.
Removed.
https://review.coreboot.org/c/coreboot/+/43904/5/src/cpu/intel/common/Makefi... File src/cpu/intel/common/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/43904/5/src/cpu/intel/common/Makefi... PS5, Line 3: ramstage-$(CONFIG_CPU_INTEL_COMMON_HYPERTHREADING) += hyperthreading.c
why do we add this line?
Removed.
https://review.coreboot.org/c/coreboot/+/43904/5/src/cpu/intel/common/voltag... File src/cpu/intel/common/voltage.c:
https://review.coreboot.org/c/coreboot/+/43904/5/src/cpu/intel/common/voltag... PS5, Line 9: #if CONFIG(GENERATE_SMBIOS_TABLES)
There is no need to guard it. If we build without SMBIOS tables, it will simply […]
Done.
https://review.coreboot.org/c/coreboot/+/43904/5/src/cpu/intel/model_2065x/K... File src/cpu/intel/model_2065x/Kconfig:
https://review.coreboot.org/c/coreboot/+/43904/5/src/cpu/intel/model_2065x/K... PS5, Line 4: if CPU_INTEL_MODEL_2065X
Was this confirmed? In the SDM it's only documented since Sandy Bridge.
Actually, I'm not sure about that. I removed CPU_INTEL_COMMON_VOLTAGE here, and it can be selected if needed.
Angel Pons 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+1
Jonathan Zhang 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+1
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.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43904 )
Change subject: cpu/intel/common: Fill cpu voltage in SMBIOS tables ......................................................................
cpu/intel/common: Fill cpu voltage in SMBIOS tables
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.
Tested on Intel CFL. The CPU voltage is correctly advertised.
Change-Id: I31a7efcbeede50d986a1c096a4a59a316e09f825 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/43904 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Jonathan Zhang jonzhang@fb.com Reviewed-by: Nico Huber nico.h@gmx.de --- M src/arch/x86/smbios.c M src/cpu/intel/common/Kconfig M src/cpu/intel/common/Makefile.inc A src/cpu/intel/common/voltage.c M src/cpu/intel/haswell/Kconfig M src/include/smbios.h 6 files changed, 28 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve Jonathan Zhang: Looks good to me, but someone else must approve
diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c index dc676cf..da77284 100644 --- a/src/arch/x86/smbios.c +++ b/src/arch/x86/smbios.c @@ -493,6 +493,12 @@ return SMBIOS_CACHE_OP_MODE_UNKNOWN; /* Unknown */ }
+/* Returns the processor voltage in 100mV units */ +unsigned int __weak smbios_cpu_get_voltage(void) +{ + return 0; /* Unknown */ +} + static size_t get_number_of_caches(struct cpuid_result res_deterministic_cache) { size_t max_logical_cpus_sharing_cache = 0; @@ -595,6 +601,7 @@
static int smbios_write_type4(unsigned long *current, int handle) { + unsigned int cpu_voltage; struct cpuid_result res; struct smbios_type4 *t = (struct smbios_type4 *)*current; int len = sizeof(struct smbios_type4); @@ -686,6 +693,9 @@ } } t->processor_characteristics = characteristics | smbios_processor_characteristics(); + cpu_voltage = smbios_cpu_get_voltage(); + if (cpu_voltage > 0) + t->voltage = 0x80 | cpu_voltage;
*current += len; return len; diff --git a/src/cpu/intel/common/Kconfig b/src/cpu/intel/common/Kconfig index 01f2721..7f9033c 100644 --- a/src/cpu/intel/common/Kconfig +++ b/src/cpu/intel/common/Kconfig @@ -32,6 +32,9 @@
endif
+config CPU_INTEL_COMMON_VOLTAGE + bool + config CPU_INTEL_COMMON_SMM bool default y if CPU_INTEL_COMMON diff --git a/src/cpu/intel/common/Makefile.inc b/src/cpu/intel/common/Makefile.inc index 8b81a12..530ecee 100644 --- a/src/cpu/intel/common/Makefile.inc +++ b/src/cpu/intel/common/Makefile.inc @@ -1,5 +1,6 @@ ramstage-$(CONFIG_CPU_INTEL_COMMON) += common_init.c ramstage-$(CONFIG_CPU_INTEL_COMMON) += hyperthreading.c +ramstage-$(CONFIG_CPU_INTEL_COMMON_VOLTAGE) += voltage.c
ifeq ($(CONFIG_CPU_INTEL_COMMON_TIMEBASE),y) bootblock-y += fsb.c diff --git a/src/cpu/intel/common/voltage.c b/src/cpu/intel/common/voltage.c new file mode 100644 index 0000000..38951a0 --- /dev/null +++ b/src/cpu/intel/common/voltage.c @@ -0,0 +1,12 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <cpu/x86/msr.h> +#include <smbios.h> + +/* This is not an architectural MSR. */ +#define MSR_PERF_STATUS 0x198 + +unsigned int smbios_cpu_get_voltage(void) +{ + return (rdmsr(MSR_PERF_STATUS).hi & 0xffff) * 10 / 8192; +} diff --git a/src/cpu/intel/haswell/Kconfig b/src/cpu/intel/haswell/Kconfig index fbfa714..0d3d132 100644 --- a/src/cpu/intel/haswell/Kconfig +++ b/src/cpu/intel/haswell/Kconfig @@ -20,6 +20,7 @@ select CPU_INTEL_COMMON select CPU_INTEL_COMMON_TIMEBASE select HAVE_ASAN_IN_ROMSTAGE + select CPU_INTEL_COMMON_VOLTAGE
config SMM_TSEG_SIZE hex diff --git a/src/include/smbios.h b/src/include/smbios.h index 6a19655..4ddf438 100644 --- a/src/include/smbios.h +++ b/src/include/smbios.h @@ -40,6 +40,7 @@
unsigned int smbios_cpu_get_max_speed_mhz(void); unsigned int smbios_cpu_get_current_speed_mhz(void); +unsigned int smbios_cpu_get_voltage(void);
const char *smbios_mainboard_manufacturer(void); const char *smbios_mainboard_product_name(void);
-
Angel Pons (Code Review) -
HAOUAS Elyes (Code Review)
-
Jonathan Zhang (Code Review)
-
Marcello Sylvester Bauer (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Tim Chu (Code Review)