Re: [PATCH v2 3/3] x86/tsc_msr: Make MSR derived TSC frequency more accurate

[Andy Shevchenko]

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 ?

...

> +       struct {
> +               u32 multiplier;
> +               u32 divider;
> +       } pairs[MAX_NUM_FREQS];

Perhaps pairs -> muldiv ?

...

> +       .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?

...

> +       .pairs = { { 0, 0 }, { 1, 1 }, { 4, 3 } },

And maybe list all of them always? (I'm fine with either approach).

...

> +/* 24 MHz crystal? : 24 * 13 / 4 = 78 MHz */

Perhaps Cc to LGM SoC developers team (they did it recently, so, they
have to know).

...

> +       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.

> +       } else {
> +               freq = freq_desc->freqs[index];
> +               res = freq * ratio;
> +       }

-- 
With Best Regards,
Andy Shevchenko