Message-ID: <0d361322-87aa-af48-492c-e8c4983bb35b@redhat.com>
Date:   Wed, 29 Jan 2020 14:21:40 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>
Cc:     vipul kumar <vipulk0511@...il.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        linux-kernel@...r.kernel.org, Stable <stable@...r.kernel.org>,
        Srikanth Krishnakar <Srikanth_Krishnakar@...tor.com>,
        Cedric Hombourger <Cedric_Hombourger@...tor.com>,
        x86@...nel.org, Len Brown <len.brown@...el.com>,
        Vipul Kumar <vipul_kumar@...tor.com>
Subject: Re: [v3] x86/tsc: Unset TSC_KNOWN_FREQ and TSC_RELIABLE flags on
 Intel Bay Trail SoC

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@...hat.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 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

Powered by blists - more mailing lists