Re: [v3] x86/tsc: Unset TSC_KNOWN_FREQ and TSC_RELIABLE flags on Intel Bay Trail SoC

[Hans de Goede]

Hi,

On 29-01-2020 14:03, Andy Shevchenko wrote:
> On Tue, Jan 28, 2020 at 11:39:28PM +0100, Thomas Gleixner wrote:
> > Hans de Goede <hdegoede@xxxxxxxxxx> writes:
> > > Ok, I have been testing this on various devices and I'm pretty sure now
> > > that my initial hunch is correct. The problem is that the accuracy of
> > > the FSB frequency as listed in the Intel docs is not so great:
> >
> > Thanks for doing that.
>
> +1!
>
> > > 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.
> >
> > The SDM is not always helpful :)
> >
> > > So the 00 part of 83300 which I'm suggesting to replace with 33 in
> > > essence is not specified and when the tsc_msr.c code was written /
> > > Bay Trail support was added the value from the datasheet was simply
> > > padded with zeros.
> > >
> > > There is already a hint that that likely is not correct in the values
> > > from the Software Developerâs Manual, we have values ending at 3.3,
> > > but also at 6.7, which to me feels like it is 6.66666666666667 rounded
> > > up and thus the 3.3 likely is 3.33333333333333.
> > >
> > > Test 1: Intel(R) Celeron(R) CPU  N2840  @ 2.16GHz"
> > > --------------------------------------------------
> > >
> > > As said I've also ran some tests. The first device I have tested is
> > > a HP stream 11 x360 with an "Intel(R) Celeron(R) CPU  N2840  @ 2.16GHz"
> > > (from /proc/cpuinfo) this is the "laptop' version of Bay Trail rather
> > > then the tablet version, so like Vipul's case I can comment out the 2
> > > lines setting the TSC_KNOWN_FREQ and TSC_RELIABLE flags and get
> > > "Refined TSC clocksource calibration". I've also added the changes with
> > > the extra pr_info calls which you requested. Here is the relevant output
> > > from a kernel with the 2 flags commented out + your pr_info changes,
> > > note I changed the REF_CLOCK format from %x to %d as that seems easier
> > > to interpret to me.
> > >
> > > [    0.000000] MSR_PINFO: 0000060000001a00 -> 26
> > > [    0.000000] MSR_FSBF: 0000000000000000
> > > [    0.000000] REF_CLOCK: 83000
> > > [    0.000000] tsc: Detected 2165.800 MHz processor
> > > [    3.586805] tsc: Refined TSC clocksource calibration: 2166.666 MHz
> > >
> > > And with my suggested change:
> > >
> > > [    0.000000] MSR_PINFO: 0000060000001a00 -> 26
> > > [    0.000000] MSR_FSBF: 0000000000000000
> > > [    0.000000] REF_CLOCK: 83333
> > > [    0.000000] tsc: Detected 2166.658 MHz processor
> > > [    3.587326] tsc: Refined TSC clocksource calibration: 2166.667 MHz
> > >
> > > Note we are still 0.009 MHz of from the refined calibration, so my
> > > suggestion to really fix this would be to change the freqs part
> > > of struct freq_desc to be in Hz rather then KHz and then calculate
> > > res as:
> > >
> > > res = DIV_ROUND_CLOSEST(freq * ratio, 1000); /* res is in KHz */
> >
> > That makes a log of sense.
>
> ...
>
> > Looking at the table again:
> >
> > >      000B: 083.3 MHz
> > >      001B: 100.0 MHz
> > >      010B: 133.3 MHz
> > >      011B: 116.7 MHz
> > >      100B: 080.0 MHz
> >
> > I don't know what the crystal frequency of this CPU is, but usually the
> > frequencies are the same accross a SoC family. The E3800 baytrail
> > definitely runs with a 25Mhz crystal.
> >
> > So using 25MHz as crystal frequency;
> >
> > 000:   25 * 20 / 6  =  83.3333
> > 001:   25 *  4 / 1  = 100.0000
> > 010:   25 * 16 / 3  = 133.3333
> > 011:   25 * 28 / 6  = 116.6666
> > 100:   25 * 16 / 5  =  80.0000
> >
> > So the tables for the various SoCs should have the crystal frequency and
> > the multiplier / divider pairs for each step. That makes the math simple
> > and accurate.
>
> Completely agree here. I used to fix magic tables [1] when product engineers
> considers data in the documentation like carved in stone. So, I'm not surprised
> we have one here.
>
> > Typical crystal frequencies are 19.2, 24 and 25Mhz.
>
> Hans, I think Cherrytrail may be affected by this as the others.
> CHT AFAIK uses 19.2MHz xtal.

Are you sure?

The first 5 entries of the CHT MSR_FSB_FREQ documentation exactly
match those of the BYT documentation (which has only 5 entries),
which suggests to me that CHT is also using a 25 MHz crystal.

I can also make the other CHT only frequencies when assuming a 25
MHz crystal, here is a bit from the patch I'm working on for this:

/*
 * Cherry Trail SDM MSR_FSB_FREQ frequencies to PLL settings map:
 * 0000:   25 * 20 /  6  =  83.3333 MHz
 * 0001:   25 *  4 /  1  = 100.0000 MHz
 * 0010:   25 * 16 /  3  = 133.3333 MHz
 * 0011:   25 * 28 /  6  = 116.6667 MHz
 * 0100:   25 * 16 /  5  =  80.0000 MHz
 * 0101:   25 * 56 / 15  =  93.3333 MHz
 * 0110:   25 * 18 /  5  =  90.0000 MHz
 * 0111:   25 * 32 /  9  =  88.8889 MHz
 * 1000:   25 *  7 /  2  =  87.5000 MHz
 */

The only one which is possibly suspicious here is this line:

 * 0111:   25 * 32 /  9  =  88.8889 MHz

The SDM says 88.9 MHz for this one.

Regards,

Hans