On Tue, Jan 28, 2020 at 11:39:28PM +0100, Thomas Gleixner wrote:
Hans de Goede hdegoede@redhat.com 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 MHzNotice 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 MHzI 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.
And if you look at CPUID 15H, it provides the crystal frequency and the crystal to TSC ratio with a nominator / denominator pair. IOW a proper description of the PLL.
[1]: 9df461eca18f ("spi: pxa2xx: replace ugly table by approximation")