Tim Wawrzynczak 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:
(2 comments)
Patch Set 4:
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
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 this seems unneeded, you could just pass nominal for ecx, and if it's 0, perform the model checks
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
-
Tim Wawrzynczak (Code Review)