Date: Wed, 9 Nov 2016 22:25:24 +0100 (CET)
From: Thomas Gleixner <tglx@linutronix.de>
To: Bin Gao <bin.gao@intel.com>
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
Subject: Re: [PATCH 2/2] x86: use KNOWN_FREQ and RELIABLE TSC flags on certain
 processors/SoCs
In-Reply-To: <1478020482-231459-3-git-send-email-bin.gao@intel.com>
Message-ID: <alpine.DEB.2.20.1611092209440.3501@nanos>
References: <1478020482-231459-1-git-send-email-bin.gao@intel.com> <1478020482-231459-3-git-send-email-bin.gao@intel.com>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1457
Lines: 45

On Tue, 1 Nov 2016, Bin Gao wrote:
> @@ -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.

> +	/*
> +	 * 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?

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.

Thanks,

	tglx