Date: Thu, 10 Nov 2016 15:20:02 -0800
From: Bin Gao <bin.gao@linux.intel.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>, H Peter Anvin <hpa@zytor.com>,
        x86@kernel.org, Peter Zijlstra <peterz@infradead.org>,
        linux-kernel@vger.kernel.org, Bin Gao <bin.gao@intel.com>
Subject: Re: Re: [PATCH 2/2] x86: use KNOWN_FREQ and RELIABLE TSC flags on
 certain processors/SoCs
Message-ID: <20161110232001.GB217763@worksta>
References: <1478020482-231459-1-git-send-email-bin.gao@intel.com>
 <1478020482-231459-3-git-send-email-bin.gao@intel.com>
 <alpine.DEB.2.20.1611092209440.3501@nanos>
 <4460FA1017EA3844B646E90DA4E984057E2ECB85@ORSMSX112.amr.corp.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4460FA1017EA3844B646E90DA4E984057E2ECB85@ORSMSX112.amr.corp.intel.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2340
Lines: 59

> > @@ -702,6 +702,15 @@ unsigned long native_calibrate_tsc(void)
> >  		}
> >  	}
> >  
> > +	setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
> 
> I can understand the one below, but this one changes existing behaviour w/o explaining why this is correct and desired. If at all then this wants to be a seperate patch and not just mingled in your goldmont update.

native_calibrate_tsc() implements determining TSC frequency via CPUID.
The purpose to add X86_FEATURE_TSC_KNOWN_FREQ flag is exactly for this case:
TSC frequency determined via CPUID or MSR are always correct and the whole
calibration should be skipped.

I will create a seperate patch for this to ensure it's not confusing with
the MSR related change below.

> 
> > +	/*
> > +	 * For Atom SoCs TSC is the only reliable clocksource.
> > +	 * Mark TSC reliable so no watchdog on it.
> > +	 */
> > +	if (boot_cpu_data.x86_model == INTEL_FAM6_ATOM_GOLDMONT)
> > +		setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
> > +
> >  	return crystal_khz * ebx_numerator / eax_denominator;  }
> >  
> > diff --git a/arch/x86/kernel/tsc_msr.c b/arch/x86/kernel/tsc_msr.c 
> > index 0fe720d..d6aa75a 100644
> > --- a/arch/x86/kernel/tsc_msr.c
> > +++ b/arch/x86/kernel/tsc_msr.c
> > @@ -100,5 +100,9 @@ unsigned long cpu_khz_from_msr(void)  #ifdef 
> > CONFIG_X86_LOCAL_APIC
> >  	lapic_timer_frequency = (freq * 1000) / HZ;  #endif
> > +
> > +	setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
> > +	setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
> 
> Why is this automatically reliable and of known frequency?

As I said above, TSC frequency determined by CPUID or MSR is always considered
"known" because it is reported by HW.
Regarding the reliable, unfortunately however, there is no a HW way to report
it. We were told by silicon design team it's "reliable".

> 
> This evades the long term TSC calibration and also disables the watchdog, which might break stuff left and right.
> 
> Please makes these changes one by one and explain why they are correct on their own, preferrably with some substantial backfrom from the hw folks.

Yes we confirmed with HW folks. TSC count is guaranteed to monotonically
increase at the fixed frequency even during S3/S0i3 state on these platforms.
This change will be seperate from CPUID related change in next revision.

> 
> Thanks,
> 
> 	tglx
> 
>