Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34170 )
Change subject: libpayload/x86: Try to discover invariant TSC rate ......................................................................
Patch Set 4:
(5 comments)
This might need checking how it behaves with Depthcharge. I was actually wondering how CrOS devices handle it... is the PIT enabled? do you have a different timer?
I can confirm in the volteer (TGL) next to me that these two patches allow me to call `get_cpu_speed()` without hanging 😊. Without these, it hangs on the call, presumably b/c we leave the PIT disabled. We haven't used PIT for timing since some old skylake boards I see. Current boards AFAICT just use the timestamp counter
It's all about using the TSC, but you need to know it's rate. Still wonder how you avoid calling get_cpu_speed().
https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x8... File payloads/libpayload/arch/x86/timer.c:
https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x8... PS4, Line 90: ecx
yeah I guess it wouldn't work b/c of the macro.
Thanks Angel. Indeed, I just wanted to avoid the cast. Doesn't matter much though I guess, could change it?
https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x8... PS4, Line 106: switch (cpuid_model()) { : case 0x4e: /* SKL-U/Y */ : case 0x5e: /* SKL-S/H */ : case 0x8e: /* KBL-U/Y */ : case 0x9e: /* KBL-S/H */ : nominal = 24000000; : break; : case 0x5c: /* APL */ : nominal = 19200000; : break; : default: : return -1; : } : } : : return nominal * num / denom / 1000; : } : : /** : * @brief Returns three times the bus clock in kHz : * : * 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) : { : if (cpuid_family() != 6) : return -1; : : switch (cpuid_model()) { : case 0x25: /* Nehalem */ : return 400 * 1000; /* 133 MHz */ : case 0x2a: /* SandyBridge */ : case 0x3a: /* IvyBridge */ : case 0x3c: /* Haswell */ : case 0x3d: /* Broadwell */ : case 0x45: /* Haswell-ULT */ : case 0x46: /* Haswell-GT3e */ : return 300 * 1000; /* 100 MHz */ : default: : return -1; : } : }
suggestion: an enum for the results of cpuid_model() would be more readable than comments
I like that!
https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x8... PS4, Line 172: /*
newline above
Is that mandatory? Currently, the whole function does either what is above the empty line or what is below... Adding another empty line would make me want to split the second part into another function. Could do that ofc.
https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x8... PS4, Line 176: _rdmsr(MSR_PLATFORM_INFO) >> 8
well, maybe even both
lol, I can definitely drop those below. But would really not like to add unneeded ones. Unneeded brackets => people don't learn C => people make mistakes.
https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x8... PS4, Line 186: cpu_khz
hm, somehow I seem to miss what this global variable is useful for (yeah I know, it was there before […]
The whole point of this file is to discover the TSC rate. So you can later do rdtsc()*x/cpu_khz to get a monotonic time in s/ms/us/ns.
-
Nico Huber (Code Review)