Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 19 Apr 2019 13:50:37 -0700
From:   Jacob Pan <jacob.jun.pan@linux.intel.com>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Daniel Drake <drake@endlessm.com>, Len Brown <lenb@kernel.org>,
        x86@kernel.org, LKML <linux-kernel@vger.kernel.org>,
        linux@endlessm.com, rjw@rjwysocki.net,
        "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
        jacob.jun.pan@linux.intel.com
Subject: Re: Detecting x86 LAPIC timer frequency from CPUID data
Message-ID: <20190419135037.5bf5750e@jacob-builder>
In-Reply-To: <alpine.DEB.2.21.1904191042180.3174@nanos.tec.linutronix.de>
References: <20190419083533.32388-1-drake@endlessm.com>
        <alpine.DEB.2.21.1904191042180.3174@nanos.tec.linutronix.de>
Organization: OTC
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 19 Apr 2019 10:57:10 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Fri, 19 Apr 2019, Daniel Drake wrote:
> > On Fri, Apr 19, 2019 at 6:30 AM Thomas Gleixner
> > <tglx@linutronix.de> wrote:  
> > >    Time Stamp Counter/Core Crystal Clock Information (0x15):
> > >       TSC/clock ratio = 168/2
> > >       nominal core crystal clock = 0 Hz
> > >
> > >    Processor Frequency Information (0x16):
> > >       Core Base Frequency (MHz) = 0x834 (2100)
> > >       Core Maximum Frequency (MHz) = 0xed8 (3800)
> > >       Bus (Reference) Frequency (MHz) = 0x64 (100)
> > >
> > > Assuming that TSC and local APIC timer run from the same
> > > frequency on these modern machines.
> > >
> > >        2100MHz * 2 / 168 = 25MHz
> > >
> > > and disabling the tsc deadline timer tells me:
> > >
> > >   ..... calibration result: 24999
> > >
> > > Close enough.  
> > 
> > I tested all the Intel SoC generations we have on hand. The
> > assumption that the core crystal clock feeds the APIC seems to be
> > consistently true.
> > 
> > (Please note that all the following results are done with
> > CONFIG_HZ=250, which is why the "calibration result" is 4x higher
> > than HZ=1000 as used in previous mails)
> > 
> > In the easy case, the low cost platforms do not support CPUID.0x16
> > (so no CPU frequency reporting), but they do tell us the core
> > crystal clock, which is consistent with the APIC calibration
> > result:  
> 
> ...
> 
> > And the 4 higher-end SoCs that we have available for testing all
> > report crystal clock 0Hz from CPUID 0x15, but by combining the
> > CPUID.0x16 base frequency with the CPUID.0x15 TSC/clock ratio, the
> > crystal frequency can be calculated as you describe, and it
> > consistently matches the APIC timer calibration result.  
> 
> ...
> 
> > Is this data convincing enough or should we additionally wait for
> > some comments from Intel?  
> 
> For me it's pretty convincing, but having some confirmation from Intel
> wouldn't be a bad thing.
>  
> > I came up with the patch below. However, upon testing, I realised
> > that, at least for the platforms I have in hand, only the first
> > hunk is really needed. We don't need to use your magic calculation
> > to find the crystal frequency because Intel already told us!
> > native_calibrate_tsc() already hardcodes the crystal frequency for
> > Kabylake, and Amber/Whiskey/Coffee also report the 0x8e/0x9e
> > Kabylake model codes.  
> 
> I'd rather replace these model checks with math. These tables are
> horrible to maintain.
> 
> > Plus ApolloLake/GeminiLake do report the crystal frequency in
> > CPUID.0x15 so that is covered too.  
> 
> > While looking around this code I also spotted something curious.
> > In calibrate_APIC_clock() for the case where lapic_timer_frequency
> > has been externally provided, we have:
> > 		lapic_clockevent.max_delta_ns =
> > 			clockevent_delta2ns(0x7FFFFF,
> > &lapic_clockevent); lapic_clockevent.max_delta_ticks = 0x7FFFFF;
> > 
> > But in the case where we calibrate, we have:
> > 	lapic_clockevent.max_delta_ns =
> > 		clockevent_delta2ns(0x7FFFFFFF, &lapic_clockevent);
> > 	lapic_clockevent.max_delta_ticks = 0x7FFFFFFF;
> > 
> > 0x7FFFFF vs 0x7FFFFFFF, is that intentional?  
> 
> I don't think so. Looks like a failed copy and paste. Cc'ed Jacob, he
> might know.
> 
At the time of v2.6.35 both places use 0x7FFFFF. But later this patch
increased the latter to 0x7FFFFFFF but forgot the first part. So I
guess it is not exactly a failed copy and paste.

commit 4aed89d6b515b9185351706ca95cd712c9d8d6a3
Author: Pierre Tardy <pierre.tardy@intel.com>
Date:   Thu Jan 6 16:23:29 2011 +0100

    x86, lapic-timer: Increase the max_delta to 31 bits
    

> Thanks,
> 
> 	tglx

[Jacob Pan]