[PATCH 1/3] x86/tsc_msr: Use named struct initializers

[PATCH 1/3] x86/tsc_msr: Use named struct initializers

[Hans de Goede]

Use named struct initializers for the freq_desc struct-s initialization
and change the "u8 msr_plat" to a "bool use_msr_plat" to make its meaning
more clear instead of relying on a comment to explain it.

Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/x86/kernel/tsc_msr.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/tsc_msr.c b/arch/x86/kernel/tsc_msr.c
index e0cbe4f2af49..5fa41ac3feb1 100644
--- a/arch/x86/kernel/tsc_msr.c
+++ b/arch/x86/kernel/tsc_msr.c
@@ -22,10 +22,10 @@
  * read in MSR_PLATFORM_ID[12:8], otherwise in MSR_PERF_STAT[44:40].
  * Unfortunately some Intel Atom SoCs aren't quite compliant to this,
  * so we need manually differentiate SoC families. This is what the
- * field msr_plat does.
+ * field use_msr_plat does.
  */
 struct freq_desc {
-	u8 msr_plat;	/* 1: use MSR_PLATFORM_INFO, 0: MSR_IA32_PERF_STATUS */
+	bool use_msr_plat;
 	u32 freqs[MAX_NUM_FREQS];
 };
 
@@ -35,31 +35,39 @@ struct freq_desc {
  * by MSR based on SDM.
  */
 static const struct freq_desc freq_desc_pnw = {
-	0, { 0, 0, 0, 0, 0, 99840, 0, 83200 }
+	.use_msr_plat = false,
+	.freqs = { 0, 0, 0, 0, 0, 99840, 0, 83200 },
 };
 
 static const struct freq_desc freq_desc_clv = {
-	0, { 0, 133200, 0, 0, 0, 99840, 0, 83200 }
+	.use_msr_plat = false,
+	.freqs = { 0, 133200, 0, 0, 0, 99840, 0, 83200 },
 };
 
 static const struct freq_desc freq_desc_byt = {
-	1, { 83300, 100000, 133300, 116700, 80000, 0, 0, 0 }
+	.use_msr_plat = true,
+	.freqs = { 83300, 100000, 133300, 116700, 80000, 0, 0, 0 },
 };
 
 static const struct freq_desc freq_desc_cht = {
-	1, { 83300, 100000, 133300, 116700, 80000, 93300, 90000, 88900, 87500 }
+	.use_msr_plat = true,
+	.freqs = { 83300, 100000, 133300, 116700, 80000, 93300, 90000,
+		   88900, 87500 },
 };
 
 static const struct freq_desc freq_desc_tng = {
-	1, { 0, 100000, 133300, 0, 0, 0, 0, 0 }
+	.use_msr_plat = true,
+	.freqs = { 0, 100000, 133300, 0, 0, 0, 0, 0 },
 };
 
 static const struct freq_desc freq_desc_ann = {
-	1, { 83300, 100000, 133300, 100000, 0, 0, 0, 0 }
+	.use_msr_plat = true,
+	.freqs = { 83300, 100000, 133300, 100000, 0, 0, 0, 0 },
 };
 
 static const struct freq_desc freq_desc_lgm = {
-	1, { 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000 }
+	.use_msr_plat = true,
+	.freqs = { 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000 },
 };
 
 static const struct x86_cpu_id tsc_msr_cpu_ids[] = {
@@ -91,7 +99,7 @@ unsigned long cpu_khz_from_msr(void)
 		return 0;
 
 	freq_desc = (struct freq_desc *)id->driver_data;
-	if (freq_desc->msr_plat) {
+	if (freq_desc->use_msr_plat) {
 		rdmsr(MSR_PLATFORM_INFO, lo, hi);
 		ratio = (lo >> 8) & 0xff;
 	} else {
-- 
2.24.1

[PATCH 2/3] x86/tsc_msr: Fix MSR_FSB_FREQ mask for Cherry Trail devices

[Hans de Goede]

According to the "Intel 64 and IA-32 Architectures Software Developer’s
Manual Volume 4: Model-Specific Registers" on Cherry Trail (Airmont)
devices the 4 lowest bits of the MSR_FSB_FREQ mask indicate the bus freq
unlike on e.g. Bay Trail where only the lowest 3 bits are used.

This is also the reason why MAX_NUM_FREQS is defined as 9, since
Cherry Trail SoCs have 9 possible frequencies, so we need to mask
the lo value from the MSR with 0x0f, not with 0x07 otherwise the
9th frequency will get interpreted as the 1st.

This commits bumps MAX_NUM_FREQS to 16 to avoid any possibility of
addressing the array out of bounds and makes the mask part of
the cpufreq struct so that we can set it per model.

While at it also log an error when the index points to an uninitialized
part of the freqs lookup-table.

Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/x86/kernel/tsc_msr.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/tsc_msr.c b/arch/x86/kernel/tsc_msr.c
index 5fa41ac3feb1..95030895fffa 100644
--- a/arch/x86/kernel/tsc_msr.c
+++ b/arch/x86/kernel/tsc_msr.c
@@ -15,7 +15,7 @@
 #include <asm/param.h>
 #include <asm/tsc.h>
 
-#define MAX_NUM_FREQS	9
+#define MAX_NUM_FREQS	16 /* 4 bits to select the frequency */
 
 /*
  * If MSR_PERF_STAT[31] is set, the maximum resolved bus ratio can be
@@ -27,6 +27,7 @@
 struct freq_desc {
 	bool use_msr_plat;
 	u32 freqs[MAX_NUM_FREQS];
+	u32 mask;
 };
 
 /*
@@ -37,37 +38,44 @@ struct freq_desc {
 static const struct freq_desc freq_desc_pnw = {
 	.use_msr_plat = false,
 	.freqs = { 0, 0, 0, 0, 0, 99840, 0, 83200 },
+	.mask = 0x07,
 };
 
 static const struct freq_desc freq_desc_clv = {
 	.use_msr_plat = false,
 	.freqs = { 0, 133200, 0, 0, 0, 99840, 0, 83200 },
+	.mask = 0x07,
 };
 
 static const struct freq_desc freq_desc_byt = {
 	.use_msr_plat = true,
 	.freqs = { 83300, 100000, 133300, 116700, 80000, 0, 0, 0 },
+	.mask = 0x07,
 };
 
 static const struct freq_desc freq_desc_cht = {
 	.use_msr_plat = true,
 	.freqs = { 83300, 100000, 133300, 116700, 80000, 93300, 90000,
 		   88900, 87500 },
+	.mask = 0x0f,
 };
 
 static const struct freq_desc freq_desc_tng = {
 	.use_msr_plat = true,
 	.freqs = { 0, 100000, 133300, 0, 0, 0, 0, 0 },
+	.mask = 0x07,
 };
 
 static const struct freq_desc freq_desc_ann = {
 	.use_msr_plat = true,
 	.freqs = { 83300, 100000, 133300, 100000, 0, 0, 0, 0 },
+	.mask = 0x0f,
 };
 
 static const struct freq_desc freq_desc_lgm = {
 	.use_msr_plat = true,
 	.freqs = { 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000 },
+	.mask = 0x0f,
 };
 
 static const struct x86_cpu_id tsc_msr_cpu_ids[] = {
@@ -93,6 +101,7 @@ unsigned long cpu_khz_from_msr(void)
 	const struct freq_desc *freq_desc;
 	const struct x86_cpu_id *id;
 	unsigned long res;
+	int index;
 
 	id = x86_match_cpu(tsc_msr_cpu_ids);
 	if (!id)
@@ -109,13 +118,17 @@ unsigned long cpu_khz_from_msr(void)
 
 	/* Get FSB FREQ ID */
 	rdmsr(MSR_FSB_FREQ, lo, hi);
+	index = lo & freq_desc->mask;
 
 	/* Map CPU reference clock freq ID(0-7) to CPU reference clock freq(KHz) */
-	freq = freq_desc->freqs[lo & 0x7];
+	freq = freq_desc->freqs[index];
 
 	/* TSC frequency = maximum resolved freq * maximum resolved bus ratio */
 	res = freq * ratio;
 
+	if (freq == 0)
+		pr_err("Error MSR_FSB_FREQ index %d is unknown\n", index);
+
 #ifdef CONFIG_X86_LOCAL_APIC
 	lapic_timer_period = (freq * 1000) / HZ;
 #endif
-- 
2.24.1

[PATCH 3/3] x86/tsc_msr: Make MSR derived TSC frequency more accurate

[Hans de Goede]

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.

As far as we know Bay Trail SoCs use a 25 MHz crystal and Cherry Trail
uses a 19.2 MHz crystal, the crystal is the source clk for a root PLL
which outputs 1600 and 100 MHz. It is unclear if the root PLL outputs are
used directly by the CPU clock PLL or if there is another PLL in between.

This does not matter though, we can model the chain of PLLs as a single
PLL with a quotient equal to the quotients of all PLLs in the chain
multiplied.

So we can create a simplified model of the CPU clock setup using a
reference clock of 100 MHz plus a quotient which gets us as close to the
frequency from the DSM as possible.

For the 83.3 MHz example from above this would give us 100 MHz * 5 / 6 =
83 and 1/3 MHz, which matches exactly what has been measured on actual hw.

This commit makes the tsc_msr.c code use a simplified PLL model with a
reference clock of 100 MHz for all Bay and Cherry Trail models.

This has been tested on the following models:

              CPU freq before:        CPU freq after this commit:
Intel N2840   2165.800 MHz            2166.667 MHz
Intel Z3736   1332.800 MHz            1333.333 MHz
Intel Z3775   1466.300 MHz            1466.667 MHz
Intel Z8350   1440.000 MHz            1440.000 MHz
Intel Z8750   1600.000 MHz            1600.000 MHz

This fixes the time drifting by about 1 second per hour (20 - 30 seconds
per day) on (some) devices which rely on the tsc_msr.c code to determine
the TSC frequency.

Cc: stable@vger.kernel.org
Reported-by: Vipul Kumar <vipulk0511@gmail.com>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/x86/kernel/tsc_msr.c | 90 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 80 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/tsc_msr.c b/arch/x86/kernel/tsc_msr.c
index 95030895fffa..4331f6d83cab 100644
--- a/arch/x86/kernel/tsc_msr.c
+++ b/arch/x86/kernel/tsc_msr.c
@@ -17,6 +17,23 @@
 
 #define MAX_NUM_FREQS	16 /* 4 bits to select the frequency */
 
+/*
+ * The frequency numbers in the DSM are e.g. 83.3 MHz, which does not contain a
+ * lot of accuracy which leads to clock drift. As far as we know Bay Trail SoCs
+ * use a 25 MHz crystal and Cherry Trail uses a 19.2 MHz crystal, the crystal
+ * is the source clk for a root PLL which outputs 1600 and 100 MHz. It is
+ * unclear if the root PLL outputs are used directly by the CPU clock PLL or
+ * if there is another PLL in between.
+ * This does not matter though, we can model the chain of PLLs as a single PLL
+ * with a quotient equal to the quotients of all PLLs in the chain multiplied.
+ * So we can create a simplified model of the CPU clock setup using a reference
+ * clock of 100 MHz plus a quotient which gets us as close to the frequency
+ * from the DSM as possible.
+ * For the 83.3 MHz example from above this would give us 100 MHz * 5 / 6 =
+ * 83 and 1/3 MHz, which matches exactly what has been measured on actual hw.
+ */
+#define REFERENCE_KHZ 100000
+
 /*
  * If MSR_PERF_STAT[31] is set, the maximum resolved bus ratio can be
  * read in MSR_PLATFORM_ID[12:8], otherwise in MSR_PERF_STAT[44:40].
@@ -26,6 +43,14 @@
  */
 struct freq_desc {
 	bool use_msr_plat;
+	struct {
+		u32 multiplier;
+		u32 divider;
+	} pairs[MAX_NUM_FREQS];
+	/*
+	 * Some CPU frequencies in the SDM do not map to known PLL freqs, in
+	 * that case the pairs arrays is empty and the freqs array is used.
+	 */
 	u32 freqs[MAX_NUM_FREQS];
 	u32 mask;
 };
@@ -47,31 +72,64 @@ static const struct freq_desc freq_desc_clv = {
 	.mask = 0x07,
 };
 
+/*
+ * Bay Trail SDM MSR_FSB_FREQ frequencies simplified PLL model:
+ *  000:   100 *  5 /  6  =  83.3333 MHz
+ *  001:   100 *  1 /  1  = 100.0000 MHz
+ *  010:   100 *  4 /  3  = 133.3333 MHz
+ *  011:   100 *  7 /  6  = 116.6667 MHz
+ *  100:   100 *  4 /  5  =  80.0000 MHz
+ */
 static const struct freq_desc freq_desc_byt = {
 	.use_msr_plat = true,
-	.freqs = { 83300, 100000, 133300, 116700, 80000, 0, 0, 0 },
+	.pairs = { { 5, 6 }, { 1, 1 }, { 4, 3 }, { 7, 6 }, { 4, 5 } },
 	.mask = 0x07,
 };
 
+/*
+ * Cherry Trail SDM MSR_FSB_FREQ frequencies simplified PLL model:
+ * 0000:   100 *  5 /  6  =  83.3333 MHz
+ * 0001:   100 *  1 /  1  = 100.0000 MHz
+ * 0010:   100 *  4 /  3  = 133.3333 MHz
+ * 0011:   100 *  7 /  6  = 116.6667 MHz
+ * 0100:   100 *  4 /  5  =  80.0000 MHz
+ * 0101:   100 * 14 / 15  =  93.3333 MHz
+ * 0110:   100 *  9 / 10  =  90.0000 MHz
+ * 0111:   100 *  8 /  9  =  88.8889 MHz
+ * 1000:   100 *  7 /  8  =  87.5000 MHz
+ */
 static const struct freq_desc freq_desc_cht = {
 	.use_msr_plat = true,
-	.freqs = { 83300, 100000, 133300, 116700, 80000, 93300, 90000,
-		   88900, 87500 },
+	.pairs = { {  5,  6 }, { 1,  1 }, { 4, 3 }, { 7, 6 }, { 4, 5 },
+		   { 14, 15 }, { 9, 10 }, { 8, 9 }, { 7, 8 } },
 	.mask = 0x0f,
 };
 
+/*
+ * Merriefield (BYT MID) SDM MSR_FSB_FREQ frequencies simplified PLL model:
+ * 0001:   100 *  1 /  1  = 100.0000 MHz
+ * 0010:   100 *  4 /  3  = 133.3333 MHz
+ */
 static const struct freq_desc freq_desc_tng = {
 	.use_msr_plat = true,
-	.freqs = { 0, 100000, 133300, 0, 0, 0, 0, 0 },
+	.pairs = { { 0, 0 }, { 1, 1 }, { 4, 3 } },
 	.mask = 0x07,
 };
 
+/*
+ * Moorefield (CHT MID) SDM MSR_FSB_FREQ frequencies simplified PLL model:
+ * 0000:   100 *  5 /  6  =  83.3333 MHz
+ * 0001:   100 *  1 /  1  = 100.0000 MHz
+ * 0010:   100 *  4 /  3  = 133.3333 MHz
+ * 0011:   100 *  1 /  1  = 100.0000 MHz
+ */
 static const struct freq_desc freq_desc_ann = {
 	.use_msr_plat = true,
-	.freqs = { 83300, 100000, 133300, 100000, 0, 0, 0, 0 },
+	.pairs = { { 5, 6 }, { 1, 1 }, { 4, 3 }, { 1, 1 } },
 	.mask = 0x0f,
 };
 
+/* 24 MHz crystal? : 24 * 13 / 4 = 78 MHz */
 static const struct freq_desc freq_desc_lgm = {
 	.use_msr_plat = true,
 	.freqs = { 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000 },
@@ -120,11 +178,23 @@ unsigned long cpu_khz_from_msr(void)
 	rdmsr(MSR_FSB_FREQ, lo, hi);
 	index = lo & freq_desc->mask;
 
-	/* Map CPU reference clock freq ID(0-7) to CPU reference clock freq(KHz) */
-	freq = freq_desc->freqs[index];
-
-	/* TSC frequency = maximum resolved freq * maximum resolved bus ratio */
-	res = freq * ratio;
+	/*
+	 * Note this also catches cases where the index points to an unpopulated
+	 * part of pairs, in that case the else will set freq and res to 0.
+	 */
+	if (freq_desc->pairs[index].divider) {
+		freq = DIV_ROUND_CLOSEST(REFERENCE_KHZ *
+					    freq_desc->pairs[index].multiplier,
+					 freq_desc->pairs[index].divider);
+		/* Multiply by ratio before the divide for better accuracy */
+		res = DIV_ROUND_CLOSEST(REFERENCE_KHZ *
+					    freq_desc->pairs[index].multiplier *
+					    ratio,
+					freq_desc->pairs[index].divider);
+	} else {
+		freq = freq_desc->freqs[index];
+		res = freq * ratio;
+	}
 
 	if (freq == 0)
 		pr_err("Error MSR_FSB_FREQ index %d is unknown\n", index);
-- 
2.24.1

Re: [PATCH 3/3] x86/tsc_msr: Make MSR derived TSC frequency more accurate

[Peter Zijlstra]

On Thu, Jan 30, 2020 at 12:52:55PM +0100, Hans de Goede wrote:

> This does not matter though, we can model the chain of PLLs as a single
> PLL with a quotient equal to the quotients of all PLLs in the chain
> multiplied.
> 
> So we can create a simplified model of the CPU clock setup using a
> reference clock of 100 MHz plus a quotient which gets us as close to the
> frequency from the DSM as possible.

s/DSM/SDM/ ?

> For the 83.3 MHz example from above this would give us 100 MHz * 5 / 6 =
> 83 and 1/3 MHz, which matches exactly what has been measured on actual hw.
> 
> This commit makes the tsc_msr.c code use a simplified PLL model with a
> reference clock of 100 MHz for all Bay and Cherry Trail models.


> + * Bay Trail SDM MSR_FSB_FREQ frequencies simplified PLL model:
> + *  000:   100 *  5 /  6  =  83.3333 MHz
> + *  001:   100 *  1 /  1  = 100.0000 MHz
> + *  010:   100 *  4 /  3  = 133.3333 MHz
> + *  011:   100 *  7 /  6  = 116.6667 MHz
> + *  100:   100 *  4 /  5  =  80.0000 MHz

> + * Cherry Trail SDM MSR_FSB_FREQ frequencies simplified PLL model:
> + * 0000:   100 *  5 /  6  =  83.3333 MHz
> + * 0001:   100 *  1 /  1  = 100.0000 MHz
> + * 0010:   100 *  4 /  3  = 133.3333 MHz
> + * 0011:   100 *  7 /  6  = 116.6667 MHz
> + * 0100:   100 *  4 /  5  =  80.0000 MHz
> + * 0101:   100 * 14 / 15  =  93.3333 MHz
> + * 0110:   100 *  9 / 10  =  90.0000 MHz
> + * 0111:   100 *  8 /  9  =  88.8889 MHz
> + * 1000:   100 *  7 /  8  =  87.5000 MHz

> + * Merriefield (BYT MID) SDM MSR_FSB_FREQ frequencies simplified PLL model:
> + * 0001:   100 *  1 /  1  = 100.0000 MHz
> + * 0010:   100 *  4 /  3  = 133.3333 MHz

> + * Moorefield (CHT MID) SDM MSR_FSB_FREQ frequencies simplified PLL model:
> + * 0000:   100 *  5 /  6  =  83.3333 MHz
> + * 0001:   100 *  1 /  1  = 100.0000 MHz
> + * 0010:   100 *  4 /  3  = 133.3333 MHz
> + * 0011:   100 *  1 /  1  = 100.0000 MHz

Unless I'm going cross-eyed, that's 4 times the exact same table.

Do we want to use the Cherry Trail table (for being the most complete)
for all of them?

RE: [PATCH 3/3] x86/tsc_msr: Make MSR derived TSC frequency more accurate

[David Laight]

From: Peter Zijlstra
> Sent: 30 January 2020 13:43
...
> > + * Bay Trail SDM MSR_FSB_FREQ frequencies simplified PLL model:
> > + *  000:   100 *  5 /  6  =  83.3333 MHz
> > + *  001:   100 *  1 /  1  = 100.0000 MHz
> > + *  010:   100 *  4 /  3  = 133.3333 MHz
> > + *  011:   100 *  7 /  6  = 116.6667 MHz
> > + *  100:   100 *  4 /  5  =  80.0000 MHz
> 
> > + * Cherry Trail SDM MSR_FSB_FREQ frequencies simplified PLL model:
> > + * 0000:   100 *  5 /  6  =  83.3333 MHz
> > + * 0001:   100 *  1 /  1  = 100.0000 MHz
> > + * 0010:   100 *  4 /  3  = 133.3333 MHz
> > + * 0011:   100 *  7 /  6  = 116.6667 MHz
> > + * 0100:   100 *  4 /  5  =  80.0000 MHz
> > + * 0101:   100 * 14 / 15  =  93.3333 MHz
> > + * 0110:   100 *  9 / 10  =  90.0000 MHz
> > + * 0111:   100 *  8 /  9  =  88.8889 MHz
> > + * 1000:   100 *  7 /  8  =  87.5000 MHz
> 
> > + * Merriefield (BYT MID) SDM MSR_FSB_FREQ frequencies simplified PLL model:
> > + * 0001:   100 *  1 /  1  = 100.0000 MHz
> > + * 0010:   100 *  4 /  3  = 133.3333 MHz
> 
> > + * Moorefield (CHT MID) SDM MSR_FSB_FREQ frequencies simplified PLL model:
> > + * 0000:   100 *  5 /  6  =  83.3333 MHz
> > + * 0001:   100 *  1 /  1  = 100.0000 MHz
> > + * 0010:   100 *  4 /  3  = 133.3333 MHz
> > + * 0011:   100 *  1 /  1  = 100.0000 MHz
> 
> Unless I'm going cross-eyed, that's 4 times the exact same table.

Apart from the very last line which duplicates 100MHz.
And the fact that some entries are missing (presumed invalid?)
for certain cpu.

If the tables are ever used for setting the frequency
then the valid range (and values?) would need to be known.

I did wonder if the 'mask' was necessary?
Are the unused bits reserved and zero?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 3/3] x86/tsc_msr: Make MSR derived TSC frequency more accurate

[Hans de Goede]

Hi,

On 30-01-2020 14:43, Peter Zijlstra wrote:
> On Thu, Jan 30, 2020 at 12:52:55PM +0100, Hans de Goede wrote:
> 
>> This does not matter though, we can model the chain of PLLs as a single
>> PLL with a quotient equal to the quotients of all PLLs in the chain
>> multiplied.
>>
>> So we can create a simplified model of the CPU clock setup using a
>> reference clock of 100 MHz plus a quotient which gets us as close to the
>> frequency from the DSM as possible.
> 
> s/DSM/SDM/ ?

Yes.

>> For the 83.3 MHz example from above this would give us 100 MHz * 5 / 6 =
>> 83 and 1/3 MHz, which matches exactly what has been measured on actual hw.
>>
>> This commit makes the tsc_msr.c code use a simplified PLL model with a
>> reference clock of 100 MHz for all Bay and Cherry Trail models.
> 
> 
>> + * Bay Trail SDM MSR_FSB_FREQ frequencies simplified PLL model:
>> + *  000:   100 *  5 /  6  =  83.3333 MHz
>> + *  001:   100 *  1 /  1  = 100.0000 MHz
>> + *  010:   100 *  4 /  3  = 133.3333 MHz
>> + *  011:   100 *  7 /  6  = 116.6667 MHz
>> + *  100:   100 *  4 /  5  =  80.0000 MHz
> 
>> + * Cherry Trail SDM MSR_FSB_FREQ frequencies simplified PLL model:
>> + * 0000:   100 *  5 /  6  =  83.3333 MHz
>> + * 0001:   100 *  1 /  1  = 100.0000 MHz
>> + * 0010:   100 *  4 /  3  = 133.3333 MHz
>> + * 0011:   100 *  7 /  6  = 116.6667 MHz
>> + * 0100:   100 *  4 /  5  =  80.0000 MHz
>> + * 0101:   100 * 14 / 15  =  93.3333 MHz
>> + * 0110:   100 *  9 / 10  =  90.0000 MHz
>> + * 0111:   100 *  8 /  9  =  88.8889 MHz
>> + * 1000:   100 *  7 /  8  =  87.5000 MHz
> 
>> + * Merriefield (BYT MID) SDM MSR_FSB_FREQ frequencies simplified PLL model:
>> + * 0001:   100 *  1 /  1  = 100.0000 MHz
>> + * 0010:   100 *  4 /  3  = 133.3333 MHz
> 
>> + * Moorefield (CHT MID) SDM MSR_FSB_FREQ frequencies simplified PLL model:
>> + * 0000:   100 *  5 /  6  =  83.3333 MHz
>> + * 0001:   100 *  1 /  1  = 100.0000 MHz
>> + * 0010:   100 *  4 /  3  = 133.3333 MHz
>> + * 0011:   100 *  1 /  1  = 100.0000 MHz
> 
> Unless I'm going cross-eyed, that's 4 times the exact same table.

Correct, except that the not listed values on the none Cherry Trail
table are undefined in the SDM, so we should probably deny them
(or as the old code was doing simply return 0).

And at least the Moorefield (CHT MID) table is different for 0011, that
is again 100 MHz like 0001 instead of 116.6667 as it is for BYT and CHT.

Note that the Merriefield (BYT MID) and Moorefield (CHT MID) values are
based on the old code I've not seen those values in the current latest
version of the SDM.

> Do we want to use the Cherry Trail table (for being the most complete)
> for all of them?

The old code had per model tables, and at least for BYT the SDM specifies
that only the lowest 3 bits of the MSR_FSB_FREQ register should be used,
the rest is marked as reserved (with no defined/default value given for them)
and on BYT only 000 - 100 are defined as being used, note 101 - 111 are
not marked as reserved, they are simply not documented for BYT.
_4_ bits should be used and there are frequency values defined for 0000 - 1000
again 1001 - 1111 are not marked as reserved, they are simply undocumented.

And for both MID variants I have no docs.

Regards,

Hans

Re: [PATCH 3/3] x86/tsc_msr: Make MSR derived TSC frequency more accurate

[Hans de Goede]

HI,

On 30-01-2020 16:21, David Laight wrote:
> From: Peter Zijlstra
>> Sent: 30 January 2020 13:43
> ...
>>> + * Bay Trail SDM MSR_FSB_FREQ frequencies simplified PLL model:
>>> + *  000:   100 *  5 /  6  =  83.3333 MHz
>>> + *  001:   100 *  1 /  1  = 100.0000 MHz
>>> + *  010:   100 *  4 /  3  = 133.3333 MHz
>>> + *  011:   100 *  7 /  6  = 116.6667 MHz
>>> + *  100:   100 *  4 /  5  =  80.0000 MHz
>>
>>> + * Cherry Trail SDM MSR_FSB_FREQ frequencies simplified PLL model:
>>> + * 0000:   100 *  5 /  6  =  83.3333 MHz
>>> + * 0001:   100 *  1 /  1  = 100.0000 MHz
>>> + * 0010:   100 *  4 /  3  = 133.3333 MHz
>>> + * 0011:   100 *  7 /  6  = 116.6667 MHz
>>> + * 0100:   100 *  4 /  5  =  80.0000 MHz
>>> + * 0101:   100 * 14 / 15  =  93.3333 MHz
>>> + * 0110:   100 *  9 / 10  =  90.0000 MHz
>>> + * 0111:   100 *  8 /  9  =  88.8889 MHz
>>> + * 1000:   100 *  7 /  8  =  87.5000 MHz
>>
>>> + * Merriefield (BYT MID) SDM MSR_FSB_FREQ frequencies simplified PLL model:
>>> + * 0001:   100 *  1 /  1  = 100.0000 MHz
>>> + * 0010:   100 *  4 /  3  = 133.3333 MHz
>>
>>> + * Moorefield (CHT MID) SDM MSR_FSB_FREQ frequencies simplified PLL model:
>>> + * 0000:   100 *  5 /  6  =  83.3333 MHz
>>> + * 0001:   100 *  1 /  1  = 100.0000 MHz
>>> + * 0010:   100 *  4 /  3  = 133.3333 MHz
>>> + * 0011:   100 *  1 /  1  = 100.0000 MHz
>>
>> Unless I'm going cross-eyed, that's 4 times the exact same table.
> 
> Apart from the very last line which duplicates 100MHz.
> And the fact that some entries are missing (presumed invalid?)
> for certain cpu.
> 
> If the tables are ever used for setting the frequency
> then the valid range (and values?) would need to be known.
> 
> I did wonder if the 'mask' was necessary?
> Are the unused bits reserved and zero?

They are reserved without having a defined value, the appear to
usually be 0 but I would rather not depend on that.

Regards,

Hans

RE: [PATCH 3/3] x86/tsc_msr: Make MSR derived TSC frequency more accurate

[David Laight]

From: Hans de Goede
> Sent: 30 January 2020 15:55
...
> >> + * Moorefield (CHT MID) SDM MSR_FSB_FREQ frequencies simplified PLL model:
> >> + * 0000:   100 *  5 /  6  =  83.3333 MHz
> >> + * 0001:   100 *  1 /  1  = 100.0000 MHz
> >> + * 0010:   100 *  4 /  3  = 133.3333 MHz
> >> + * 0011:   100 *  1 /  1  = 100.0000 MHz
> >
> > Unless I'm going cross-eyed, that's 4 times the exact same table.
> 
> Correct, except that the not listed values on the none Cherry Trail
> table are undefined in the SDM, so we should probably deny them
> (or as the old code was doing simply return 0).
> 
> And at least the Moorefield (CHT MID) table is different for 0011, that
> is again 100 MHz like 0001 instead of 116.6667 as it is for BYT and CHT.
> 
> Note that the Merriefield (BYT MID) and Moorefield (CHT MID) values are
> based on the old code I've not seen those values in the current latest
> version of the SDM.

I wonder if Moorefield:11 is an old typo?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 3/3] x86/tsc_msr: Make MSR derived TSC frequency more accurate

[Hans de Goede]

Hi,

On 30-01-2020 17:02, David Laight wrote:
> From: Hans de Goede
>> Sent: 30 January 2020 15:55
> ...
>>>> + * Moorefield (CHT MID) SDM MSR_FSB_FREQ frequencies simplified PLL model:
>>>> + * 0000:   100 *  5 /  6  =  83.3333 MHz
>>>> + * 0001:   100 *  1 /  1  = 100.0000 MHz
>>>> + * 0010:   100 *  4 /  3  = 133.3333 MHz
>>>> + * 0011:   100 *  1 /  1  = 100.0000 MHz
>>>
>>> Unless I'm going cross-eyed, that's 4 times the exact same table.
>>
>> Correct, except that the not listed values on the none Cherry Trail
>> table are undefined in the SDM, so we should probably deny them
>> (or as the old code was doing simply return 0).
>>
>> And at least the Moorefield (CHT MID) table is different for 0011, that
>> is again 100 MHz like 0001 instead of 116.6667 as it is for BYT and CHT.
>>
>> Note that the Merriefield (BYT MID) and Moorefield (CHT MID) values are
>> based on the old code I've not seen those values in the current latest
>> version of the SDM.
> 
> I wonder if Moorefield:11 is an old typo?

I have no idea. Andy if you can find any docs on the MSR_FSB_FREQ values
for Merriefield (BYT MID) and Moorefield (CHT MID) that would be great,
if not I suggest we stick with what we have.

Regards,

Hans

Re: [PATCH 3/3] x86/tsc_msr: Make MSR derived TSC frequency more accurate

[Andy Shevchenko]

On Thu, Jan 30, 2020 at 6:04 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 30-01-2020 17:02, David Laight wrote:

> I have no idea. Andy if you can find any docs on the MSR_FSB_FREQ values
> for Merriefield (BYT MID) and Moorefield (CHT MID) that would be great,
> if not I suggest we stick with what we have.

First of all, Merrifield (Silvermont based Atom for phones, FYI: Intel
Edison uses it) and Moorefield (Airmont) have nothing to do with code
names Baytrail and Cherrytrail respectively.
So, please don't confuse people.

I'll try to find some information.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] x86/tsc_msr: Make MSR derived TSC frequency more accurate

[Hans de Goede]

Hi,

On 30-01-2020 17:52, Andy Shevchenko wrote:
> On Thu, Jan 30, 2020 at 6:04 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 30-01-2020 17:02, David Laight wrote:
> 
>> I have no idea. Andy if you can find any docs on the MSR_FSB_FREQ values
>> for Merriefield (BYT MID) and Moorefield (CHT MID) that would be great,
>> if not I suggest we stick with what we have.
> 
> First of all, Merrifield (Silvermont based Atom for phones, FYI: Intel
> Edison uses it) and Moorefield (Airmont) have nothing to do with code
> names Baytrail and Cherrytrail respectively.
> So, please don't confuse people.

Ok, sorry, I've prepped a v2 of this patch in my local tree with
the following changelog:

Changes in v2:
-Do not refer to Merrifield / Moorefield as BYT / CHT, they only share the
  CPU core design and otherwise are significantly different

> I'll try to find some information.

OK, I'll wait a bit with sending out the v2 then.

Regards,

Hans