Messages in this thread | | | From | Hans de Goede <> | Subject | Re: [PATCH v2 3/3] x86/tsc_msr: Make MSR derived TSC frequency more accurate | Date | Fri, 7 Feb 2020 09:31:56 +0100 |
| |
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@redhat.com> 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@linux.intel.com> Cc: Tony Luck <tony.luck@intel.com> Cc: Gayatri Kammela <gayatri.kammela@intel.com>
> ... > >> + 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
| |