Re: [PATCH v2 3/3] x86/tsc_msr: Make MSR derived TSC frequency more accurate
From: Hans de Goede
Date: Fri Feb 07 2020 - 03:32:15 EST
Hi All,
On 2/5/20 5:59 PM, Andy Shevchenko wrote:
On Wed, Feb 5, 2020 at 5:34 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
The "Intel 64 and IA-32 Architectures Software Developerâs Manual
Volume 4: Model-Specific Registers" has the following table for the
values from freq_desc_byt:
000B: 083.3 MHz
001B: 100.0 MHz
010B: 133.3 MHz
011B: 116.7 MHz
100B: 080.0 MHz
Notice how for e.g the 83.3 MHz value there are 3 significant digits,
which translates to an accuracy of a 1000 ppm, where as your typical
crystal oscillator is 20 - 100 ppm, so the accuracy of the frequency
format used in the Software Developerâs Manual is not really helpful.
As far as we know Bay Trail SoCs use a 25 MHz crystal and Cherry Trail
uses a 19.2 MHz crystal, the crystal is the source clk for a root PLL
which outputs 1600 and 100 MHz. It is unclear if the root PLL outputs are
used directly by the CPU clock PLL or if there is another PLL in between.
This does not matter though, we can model the chain of PLLs as a single
PLL with a quotient equal to the quotients of all PLLs in the chain
multiplied.
So we can create a simplified model of the CPU clock setup using a
reference clock of 100 MHz plus a quotient which gets us as close to the
frequency from the SDM as possible.
For the 83.3 MHz example from above this would give us 100 MHz * 5 / 6 =
83 and 1/3 MHz, which matches exactly what has been measured on actual hw.
This commit makes the tsc_msr.c code use a simplified PLL model with a
reference clock of 100 MHz for all Bay and Cherry Trail models.
This has been tested on the following models:
CPU freq before: CPU freq after this commit:
Intel N2840 2165.800 MHz 2166.667 MHz
Intel Z3736 1332.800 MHz 1333.333 MHz
Intel Z3775 1466.300 MHz 1466.667 MHz
Intel Z8350 1440.000 MHz 1440.000 MHz
Intel Z8750 1600.000 MHz 1600.000 MHz
This fixes the time drifting by about 1 second per hour (20 - 30 seconds
per day) on (some) devices which rely on the tsc_msr.c code to determine
the TSC frequency.
Thanks for this effort!
...
+#define REFERENCE_KHZ 100000
Perhaps TSC_REFERENCE_KHZ ?
Ok, changed to TSC_REFERENCE_KHZ for v3
...
+ struct {
+ u32 multiplier;
+ u32 divider;
+ } pairs[MAX_NUM_FREQS];
Perhaps pairs -> muldiv ?
Ok, changed to muldiv for v3
...
+ .pairs = { { 5, 6 }, { 1, 1 }, { 4, 3 }, { 7, 6 }, { 4, 5 },
+ { 14, 15 }, { 9, 10 }, { 8, 9 }, { 7, 8 } },
Maybe 4 per line or alike (8 per line) for better understanding which
muldiv maps to which value?
Ok, changed for v3.
...
+ .pairs = { { 0, 0 }, { 1, 1 }, { 4, 3 } },
And maybe list all of them always? (I'm fine with either approach).
I prefer to just list the valid ones.
...
+/* 24 MHz crystal? : 24 * 13 / 4 = 78 MHz */
Perhaps Cc to LGM SoC developers team (they did it recently, so, they
have to know).
Ok, I've added the following people to the Cc for v3 based on the Sob-s and Cc-s of:
0cc5359d8fd45bc("x86/cpu: Update init data for new Airmont CPU model"):
Cc: Rahul Tanwar <rahul.tanwar@xxxxxxxxxxxxxxx>
Cc: Tony Luck <tony.luck@xxxxxxxxx>
Cc: Gayatri Kammela <gayatri.kammela@xxxxxxxxx>
...
+ if (freq_desc->pairs[index].divider) {
+ freq = DIV_ROUND_CLOSEST(REFERENCE_KHZ *
+ freq_desc->pairs[index].multiplier,
+ freq_desc->pairs[index].divider);
Maybe helper?
+ /* Multiply by ratio before the divide for better accuracy */
+ res = DIV_ROUND_CLOSEST(REFERENCE_KHZ *
+ freq_desc->pairs[index].multiplier *
+ ratio,
+ freq_desc->pairs[index].divider);
...which may be used here as well.
Nah, I would prefer to keep this as is. I'm never a fan of single line
helpers they just make it harder to see what the code is actually doing.
Regards,
Hans