Hello Nico Huber,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39830
to review the following change.
Change subject: libpayload/x86: Try to discover invariant TSC rate ......................................................................
libpayload/x86: Try to discover invariant TSC rate
We can skip the PIT-based TSC calibration if we can derive the invariant TSC rate from platform data. This is also necessary if the PIT is dis- abled, which is the default, for instance, on Coffee Lake CPUs.
Change-Id: I34eed1cbf14a8bf3fe6d58f51a4be139f4d30759 Signed-off-by: Nico Huber nico.huber@secunet.com --- M payloads/libpayload/arch/x86/timer.c 1 file changed, 58 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/39830/1
diff --git a/payloads/libpayload/arch/x86/timer.c b/payloads/libpayload/arch/x86/timer.c index bf0c30a..6b07e17 100644 --- a/payloads/libpayload/arch/x86/timer.c +++ b/payloads/libpayload/arch/x86/timer.c @@ -34,6 +34,10 @@
#include <libpayload.h> #include <arch/rdtsc.h> +#include <arch/cpuid.h> +#include <arch/msr.h> + +#define MSR_PLATFORM_INFO 0xce
/** * @ingroup arch @@ -46,7 +50,7 @@ * * @return The CPU speed in kHz. */ -unsigned int get_cpu_speed(void) +static unsigned int calibrate_pit(void) { unsigned long long start, end; const uint32_t clock_rate = 1193182; // 1.193182 MHz @@ -72,7 +76,59 @@ * clock_rate / (interval * 1000). Multiply that by the number of * measured clocks to get the kHz value. */ - cpu_khz = (end - start) * clock_rate / (1000 * interval); + return (end - start) * clock_rate / (1000 * interval); +} + +/** + * @brief Returns three times the bus clock in MHz + * + * The result of calculations with the returned value shall be divided by 3. + * This helps to avoid rounding errors. + */ +static int get_bus_khz_x3(void) +{ + uint32_t eax, ebx, ecx, edx; + cpuid(1, eax, ebx, ecx, edx); + + /* Check for family 6 */ + if (((eax & 0xff00000) >> (20 - 4)) | ((eax & 0xf00) >> 8 != 6)) + return -1; + + switch (((eax & 0xf0000) >> (16 - 4)) | ((eax & 0xf0) >> 4)) { + case 0x25: /* Nehalem */ + return 400 * 1000; /* 133 MHz */ + case 0x2a: /* SandyBridge */ + case 0x3a: /* IvyBridge */ + case 0x3c: /* Haswell */ + case 0x45: /* Haswell-ULT */ + case 0x9e: /* Coffee Lake H */ + return 300 * 1000; /* 100 MHz */ + default: + return -1; + } +} + +/* + * Systems with an invariant TSC report the multiplier (maximum + * non-turbo ratio) in MSR_PLATFORM_INFO[15:8]. + */ +static int get_cpu_khz_fast(void) +{ + const int bus_x3 = get_bus_khz_x3(); + if (bus_x3 <= 0) + return -1; + + const unsigned int mult = _rdmsr(MSR_PLATFORM_INFO) >> 8 & 0xff; + return (bus_x3 * mult) / 3; +} + +unsigned int get_cpu_speed(void) +{ + const int cpu_khz_fast = get_cpu_khz_fast(); + if (cpu_khz_fast > 0) + cpu_khz = (unsigned int)cpu_khz_fast; + else + cpu_khz = calibrate_pit();
return cpu_khz; }
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39830 )
Change subject: libpayload/x86: Try to discover invariant TSC rate ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39830/1/payloads/libpayload/arch/x8... File payloads/libpayload/arch/x86/timer.c:
https://review.coreboot.org/c/coreboot/+/39830/1/payloads/libpayload/arch/x8... PS1, Line 87: */ In case of error return `-1`.
https://review.coreboot.org/c/coreboot/+/39830/1/payloads/libpayload/arch/x8... PS1, Line 95: return -1; Why not have this check in `get_cpu_khz_fast`?
https://review.coreboot.org/c/coreboot/+/39830/1/payloads/libpayload/arch/x8... PS1, Line 100: SandyBridge Sandy Bridge
https://review.coreboot.org/c/coreboot/+/39830/1/payloads/libpayload/arch/x8... PS1, Line 101: IvyBridge Ivy Bridge
https://review.coreboot.org/c/coreboot/+/39830/1/payloads/libpayload/arch/x8... PS1, Line 107: return -1; Add a debug message for the default case?
https://review.coreboot.org/c/coreboot/+/39830/1/payloads/libpayload/arch/x8... PS1, Line 128: if (cpu_khz_fast > 0) As 0 is regarded also an error, I’d use `unsigned int` as return values and treat 0 as error.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39830 )
Change subject: libpayload/x86: Try to discover invariant TSC rate ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39830/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39830/1//COMMIT_MSG@12 PS1, Line 12: Tested how?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39830 )
Change subject: libpayload/x86: Try to discover invariant TSC rate ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/39830/2/payloads/libpayload/arch/x8... File payloads/libpayload/arch/x86/timer.c:
https://review.coreboot.org/c/coreboot/+/39830/2/payloads/libpayload/arch/x8... PS2, Line 100: case 0x2a: /* SandyBridge */ Does Sandy Bridge always use 100 MHz?
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/39830?usp=email )
Change subject: libpayload/x86: Try to discover invariant TSC rate ......................................................................
Abandoned
-
Angel Pons (Code Review) -
Felix Singer (Code Review)
-
Paul Menzel (Code Review)
-
Stefan Reinauer (Code Review)