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 21:57, Thomas Gleixner wrote:
> Andy,
>
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> writes:
> > On Wed, Jan 29, 2020 at 04:13:39PM +0100, Thomas Gleixner wrote:
> > > Andy, can you please make sure that people inside Intel who can look
> > > into the secrit documentation confirm what we are aiming for?
> > >
> > > Ideally they should provide the X-tal frequency and the mult/div pair
> > > themself :)
> >
> > So, I don't have access to the CPU core documentation (and may be will not be
> > given), nevertheless I dug a bit to what I have for Cherrytrail. So, the XTAL
> > is 19.2MHz, which becomes 100MHz and 1600MHz by some root PLL, then, the latter
> > two frequencies are being used by another PLL to provide a reference clock (*)
> > to PLL which derives CPU clock.
> >
> > *) According to colleagues of mine it's a fixed rate source.
> >
> > That's all what I have.
>
> I'm surely not blaming you for this, you're just the messenger.
>
> Just to make it entirely clear. We are wasting days already due to the
> fact that Intel, who designs, specifies and most importantly sells these
> CPUs is either unable or unwilling to provide accurate information about
> the trivial and essential information to support these CPUs:
>
>      1) The crystal frequency
>
>      2) The nominator/denominator pair to calculate the TSC frequency
>         from #1
>
> The numbers which are in the kernel have been provided by Intel, but
> they are inaccurate as we have proven.
>
> Sure, we can reverse engineer the exact numbers assumed that we have
> access to all variants of affected devices and enough spare time to
> waste.
>
> But why should we do that?
>
> Intel has the exact numbers at their fingertip and is just not providing
> them for whatever reasons (I really don't want to know).
>
> So instead of wasting our precious time further, I'm going to apply the
> patch below unless Intel comes forth with the information they should
> have provided many years ago.

Thomas, although I fully agree with your sentiment here, especially since
I've been spending pretty much the entirety of my day on this for the last
2 days, I do not think such a patch would be of great service to our end-users...

Between your initial "model the PLL" idea and Andy's provided info I've
come up with a patch which although not pretty I believe addresses this.

I'm running some final tests now and then I will post the patch series
for this upstream.

Regards,

Hans






> Thanks,
>
>          tglx
>
> 8<--------------
>   arch/x86/kernel/tsc_msr.c |    9 +++++++++
>   1 file changed, 9 insertions(+)
>
> --- a/arch/x86/kernel/tsc_msr.c
> +++ b/arch/x86/kernel/tsc_msr.c
> @@ -73,6 +73,13 @@ static const struct x86_cpu_id tsc_msr_c
>   	{}
>   };
>   
> +static char msr_warning[] = \
> +	"The TSC/APIC timer frequency for your CPU is guesswork.\n\n"	\
> +	"It is derived from frequency tables provided by Intel.\n"	\
> +	"These tables are demonstrably inaccurate, but Intel is\n"	\
> +	"either unable or unwilling to provide the correct data.\n"	\
> +	"Please report this to Intel and not on LKML.\n";
> +
>   /*
>    * MSR-based CPU/TSC frequency discovery for certain CPUs.
>    *
> @@ -90,6 +97,8 @@ unsigned long cpu_khz_from_msr(void)
>   	if (!id)
>   		return 0;
>   
> +	WARN_ONCE(1, "%s\n", msr_warning);
> +
>   	freq_desc = (struct freq_desc *)id->driver_data;
>   	if (freq_desc->msr_plat) {
>   		rdmsr(MSR_PLATFORM_INFO, lo, hi);