Change in ...coreboot[master]: cpu/intel/common: Extend FSB detection to cover TSC

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31340

Change subject: cpu/intel/common: Extend FSB detection to cover TSC
......................................................................

cpu/intel/common: Extend FSB detection to cover TSC

Change-Id: Ib7f1815b3fac7a610f7203720d526eac152a1648
Signed-off-by: Kyösti Mälkki 
---
M src/cpu/intel/common/fsb.c
1 file changed, 23 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/31340/1

diff --git a/src/cpu/intel/common/fsb.c b/src/cpu/intel/common/fsb.c
index 83220de..d06739c 100644
--- a/src/cpu/intel/common/fsb.c
+++ b/src/cpu/intel/common/fsb.c
@@ -18,14 +18,13 @@
 #include <console/console.h>
 #include <commonlib/helpers.h>

-static int get_fsb(void)
+static int get_fsb_tsc(int *fsb, int *ratio)
 {
    struct cpuinfo_x86 c;
    static const short core_fsb[8] = { -1, 133, -1, 166, -1, 100, -1, -1 };
    static const short core2_fsb[8] = { 266, 133, 200, 166, 333, 100, 400, -1 };
    static const short f2x_fsb[8] = { 100, 133, 200, 166, 333, -1, -1, -1 };
    msr_t msr;
-	int ret = -2;

get_fms(&c, cpuid_eax(1));
    switch (c.x86) {
@@ -33,49 +32,61 @@
    	switch (c.x86_model) {
    	case 0xe:  /* Core Solo/Duo */
    	case 0x1c: /* Atom */
-			ret = core_fsb[rdmsr(MSR_FSB_FREQ).lo & 7];
+			*fsb = core_fsb[rdmsr(MSR_FSB_FREQ).lo & 7];
+			*ratio = rdmsr(MSR_EBC_FREQUENCY_ID).lo >> 24;
    		break;
    	case 0xf:  /* Core 2 or Xeon */
    	case 0x17: /* Enhanced Core */
-			ret = core2_fsb[rdmsr(MSR_FSB_FREQ).lo & 7];
+			*fsb = core2_fsb[rdmsr(MSR_FSB_FREQ).lo & 7];
+			*ratio = rdmsr(MSR_EBC_FREQUENCY_ID).lo >> 24;
    		break;
    	case 0x25: /* Nehalem BCLK fixed at 133MHz */
-			ret = 133;
+			*fsb = 133;
+			*ratio = (rdmsr(MSR_PLATFORM_INFO).lo >> 8) & 0xff;
    		break;
    	case 0x2a: /* SandyBridge BCLK fixed at 100MHz */
    	case 0x3a: /* IvyBridge BCLK fixed at 100MHz */
    	case 0x3c: /* Haswell BCLK fixed at 100MHz */
    	case 0x45: /* Haswell-ULT BCLK fixed at 100MHz */
-			ret = 100;
+			*fsb = 100;
+			*ratio = (rdmsr(MSR_PLATFORM_INFO).lo >> 8) & 0xff;
    		break;
    	}
    	break;
    case 0xf: /* Netburst */
    	msr = rdmsr(MSR_EBC_FREQUENCY_ID);
+		*ratio = msr.lo >> 24;
    	switch (c.x86_model) {
    	case 0x2:
-			ret = f2x_fsb[(msr.lo >> 16) & 7];
+			*fsb = f2x_fsb[(msr.lo >> 16) & 7];
    		break;
    	case 0x3:
    	case 0x4:
    	case 0x6:
-			ret = core2_fsb[(msr.lo >> 16) & 7];
+			*fsb = core2_fsb[(msr.lo >> 16) & 7];
    		break;
    	}
+	default:
+		return -2;
    }
-	return ret;
+	if (*fsb > 0)
+		return 0;
+	return -1;
 }

int get_ia32_fsb(void)
 {
-	int ret;
+	int ret, fsb, ratio;

-	ret = get_fsb();
+	ret = get_fsb_tsc(&fsb, &ratio);
    if (ret == -1)
    	printk(BIOS_ERR, "FSB not found\n");
    if (ret == -2)
    	printk(BIOS_ERR, "CPU not supported\n");
-	return ret;
+	if (ret < 0)
+		return ret;
+
+	return fsb;
 }

/**

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31340 )

Change subject: cpu/intel/common: Extend FSB detection to cover TSC
......................................................................

Patch Set 1: Code-Review+1

Tested how?

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31340 )

Change subject: cpu/intel/common: Extend FSB detection to cover TSC
......................................................................

Patch Set 1:

There's a problem with missig MSR_PLATFORM_INFO. We could check if that is defined as architectural MSR and could be declared in common header.

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31340 )

Change subject: cpu/intel/common: Extend FSB detection to cover TSC
......................................................................

Patch Set 1:
> There's a problem with missig MSR_PLATFORM_INFO. We could check if
> that is defined as architectural MSR and could be declared in
> common header.

the name for that msr is "MSR_FSB_CLOCK_VCC". maybe we need to rename it ...

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31340 )

Change subject: cpu/intel/common: Extend FSB detection to cover TSC
......................................................................

Patch Set 2: Code-Review+1

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31340 )

Change subject: cpu/intel/common: Extend FSB detection to cover TSC
......................................................................

Patch Set 2:

The error below is hit.

```
    CC         smm/cpu/intel/common/fsb.o
src/cpu/intel/common/fsb.c: In function 'get_ia32_fsb':
src/cpu/intel/common/fsb.c:72:5: error: 'fsb' may be used uninitialized in this function [-Werror=maybe-uninitialized]
  if (*fsb > 0)
     ^
src/cpu/intel/common/fsb.c:79:11: note: 'fsb' was declared here
  int ret, fsb, ratio;
           ^~~
cc1: all warnings being treated as errors
```

Change in coreboot[master]: cpu/intel/common: Extend FSB detection to cover TSC

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31340 )

Change subject: cpu/intel/common: Extend FSB detection to cover TSC
......................................................................

Patch Set 5:

This change is ready for review.

Change in coreboot[master]: cpu/intel/common: Extend FSB detection to cover TSC

[Kyösti Mälkki (Code Review)]

Hello Patrick Rudolph, HAOUAS Elyes, Paul Menzel, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31340

to look at the new patch set (#6).

Change subject: cpu/intel/common: Extend FSB detection to cover TSC
......................................................................

cpu/intel/common: Extend FSB detection to cover TSC

Use the same CPUID switch block to resolve the multiplier
to derive TSC from FSB/BCLK frequency.

Do not return 0 as base frequency.

Change-Id: Ib7f1815b3fac7a610f7203720d526eac152a1648
Signed-off-by: Kyösti Mälkki 
---
M src/cpu/intel/common/fsb.c
1 file changed, 52 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/31340/6

Change in coreboot[master]: cpu/intel/common: Extend FSB detection to cover TSC

[Kyösti Mälkki (Code Review)]

Hello Patrick Rudolph, HAOUAS Elyes, Paul Menzel, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31340

to look at the new patch set (#8).

Change subject: cpu/intel/common: Extend FSB detection to cover TSC
......................................................................

cpu/intel/common: Extend FSB detection to cover TSC

Use the same CPUID switch block to resolve the multiplier
to derive TSC from FSB/BCLK frequency.

Do not return 0 as base frequency.

Change-Id: Ib7f1815b3fac7a610f7203720d526eac152a1648
Signed-off-by: Kyösti Mälkki 
---
M src/cpu/intel/common/fsb.c
1 file changed, 54 insertions(+), 16 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/31340/8

Change in coreboot[master]: cpu/intel/common: Extend FSB detection to cover TSC

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31340 )

Change subject: cpu/intel/common: Extend FSB detection to cover TSC
......................................................................

Patch Set 8: Code-Review+1

Change in coreboot[master]: cpu/intel/common: Extend FSB detection to cover TSC

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31340 )

Change subject: cpu/intel/common: Extend FSB detection to cover TSC
......................................................................

Patch Set 9: Code-Review+2

Change in coreboot[master]: cpu/intel/common: Extend FSB detection to cover TSC

[Name of user not set (Code Review)]

[e-mail] has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31340 )

Change subject: cpu/intel/common: Extend FSB detection to cover TSC
......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/c/coreboot/+/31340/10/src/cpu/intel/common/fsb.c 
File src/cpu/intel/common/fsb.c:

https://review.coreboot.org/c/coreboot/+/31340/10/src/cpu/intel/common/fsb.c... 
PS10, Line 61: 			*fsb = 100;
Please add this, so I can just fix Rangeley acpi.c in 35348.
static const short rangeley_bclk[4] = { 83, 100, 133, 116 };
*fsb = rangeley_bclk[rdmsr(MSR_FSB_FREQ).lo & 3];

Change in coreboot[master]: cpu/intel/common: Extend FSB detection to cover TSC

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31340 )

Change subject: cpu/intel/common: Extend FSB detection to cover TSC
......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/c/coreboot/+/31340/10/src/cpu/intel/common/fsb.c 
File src/cpu/intel/common/fsb.c:

https://review.coreboot.org/c/coreboot/+/31340/10/src/cpu/intel/common/fsb.c... 
PS10, Line 61: 			*fsb = 100;
> Please add this, so I can just fix Rangeley acpi.c in 35348. […]

It is easier to fix Rangeley after CB:34200.

Change in coreboot[master]: cpu/intel/common: Extend FSB detection to cover TSC

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/31340 )

Change subject: cpu/intel/common: Extend FSB detection to cover TSC
......................................................................

cpu/intel/common: Extend FSB detection to cover TSC

Use the same CPUID switch block to resolve the multiplier
to derive TSC from FSB/BCLK frequency.

Do not return 0 as base frequency.

Change-Id: Ib7f1815b3fac7a610f7203720d526eac152a1648
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/31340
Reviewed-by: HAOUAS Elyes 
Tested-by: build bot (Jenkins) 
---
M src/cpu/intel/common/fsb.c
1 file changed, 54 insertions(+), 16 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  HAOUAS Elyes: Looks good to me, approved

diff --git a/src/cpu/intel/common/fsb.c b/src/cpu/intel/common/fsb.c
index 2d86abd..5ad98d4 100644
--- a/src/cpu/intel/common/fsb.c
+++ b/src/cpu/intel/common/fsb.c
@@ -14,6 +14,7 @@
 #include <arch/early_variables.h>
 #include <cpu/cpu.h>
 #include <cpu/x86/msr.h>
+#include <cpu/x86/tsc.h>
 #include <cpu/intel/speedstep.h>
 #include <cpu/intel/fsb.h>
 #include <console/console.h>
@@ -21,15 +22,18 @@
 #include <delay.h>

static u32 g_timer_fsb CAR_GLOBAL;
+static u32 g_timer_tsc CAR_GLOBAL;

-static int get_fsb(void)
+/* This is not an architectural MSR. */
+#define MSR_PLATFORM_INFO 0xce
+
+static int get_fsb_tsc(int *fsb, int *ratio)
 {
    struct cpuinfo_x86 c;
    static const short core_fsb[8] = { -1, 133, -1, 166, -1, 100, -1, -1 };
    static const short core2_fsb[8] = { 266, 133, 200, 166, 333, 100, 400, -1 };
    static const short f2x_fsb[8] = { 100, 133, 200, 166, 333, -1, -1, -1 };
    msr_t msr;
-	int ret = -2;

get_fms(&c, cpuid_eax(1));
    switch (c.x86) {
@@ -37,53 +41,75 @@
    	switch (c.x86_model) {
    	case 0xe:  /* Core Solo/Duo */
    	case 0x1c: /* Atom */
-			ret = core_fsb[rdmsr(MSR_FSB_FREQ).lo & 7];
+			*fsb = core_fsb[rdmsr(MSR_FSB_FREQ).lo & 7];
+			*ratio = rdmsr(MSR_EBC_FREQUENCY_ID).lo >> 24;
    		break;
    	case 0xf:  /* Core 2 or Xeon */
    	case 0x17: /* Enhanced Core */
-			ret = core2_fsb[rdmsr(MSR_FSB_FREQ).lo & 7];
+			*fsb = core2_fsb[rdmsr(MSR_FSB_FREQ).lo & 7];
+			*ratio = rdmsr(MSR_EBC_FREQUENCY_ID).lo >> 24;
    		break;
    	case 0x25: /* Nehalem BCLK fixed at 133MHz */
-			ret = 133;
+			*fsb = 133;
+			*ratio = (rdmsr(MSR_PLATFORM_INFO).lo >> 8) & 0xff;
    		break;
    	case 0x2a: /* SandyBridge BCLK fixed at 100MHz */
    	case 0x3a: /* IvyBridge BCLK fixed at 100MHz */
    	case 0x3c: /* Haswell BCLK fixed at 100MHz */
    	case 0x45: /* Haswell-ULT BCLK fixed at 100MHz */
    	case 0x4d: /* Rangeley BCLK fixed at 100MHz */
-			ret = 100;
+			*fsb = 100;
+			*ratio = (rdmsr(MSR_PLATFORM_INFO).lo >> 8) & 0xff;
    		break;
+		default:
+			return -2;
    	}
    	break;
    case 0xf: /* Netburst */
    	msr = rdmsr(MSR_EBC_FREQUENCY_ID);
+		*ratio = msr.lo >> 24;
    	switch (c.x86_model) {
    	case 0x2:
-			ret = f2x_fsb[(msr.lo >> 16) & 7];
+			*fsb = f2x_fsb[(msr.lo >> 16) & 7];
    		break;
    	case 0x3:
    	case 0x4:
    	case 0x6:
-			ret = core2_fsb[(msr.lo >> 16) & 7];
+			*fsb = core2_fsb[(msr.lo >> 16) & 7];
    		break;
+		default:
+			return -2;
    	}
+		break;
+	default:
+		return -2;
    }
-	return ret;
+	if (*fsb > 0)
+		return 0;
+	return -1;
 }

-static int set_timer_fsb(void)
+static void resolve_timebase(void)
 {
-	int ret = get_fsb();
+	int ret, fsb, ratio;

-	if (ret > 0) {
-		car_set_var(g_timer_fsb, ret);
-		return 0;
+	ret = get_fsb_tsc(&fsb, &ratio);
+	if (ret == 0) {
+		u32 tsc = 100 * DIV_ROUND_CLOSEST(ratio * fsb, 100);
+		car_set_var(g_timer_fsb, fsb);
+		car_set_var(g_timer_tsc, tsc);
+		return;
    }
+
    if (ret == -1)
    	printk(BIOS_ERR, "FSB not found\n");
    if (ret == -2)
    	printk(BIOS_ERR, "CPU not supported\n");
-	return -1;
+
+	/* Set some semi-ridiculous defaults. */
+	car_set_var(g_timer_fsb, 500);
+	car_set_var(g_timer_tsc, 5000);
+	return;
 }

u32 get_timer_fsb(void)
@@ -94,10 +120,22 @@
    if (fsb > 0)
    	return fsb;

-	set_timer_fsb();
+	resolve_timebase();
    return car_get_var(g_timer_fsb);
 }

+unsigned long tsc_freq_mhz(void)
+{
+	u32 tsc;
+
+	tsc = car_get_var(g_timer_tsc);
+	if (tsc > 0)
+		return tsc;
+
+	resolve_timebase();
+	return car_get_var(g_timer_tsc);
+}
+
 /**
  * @brief Returns three times the FSB clock in MHz
  *

Change in coreboot[master]: cpu/intel/common: Extend FSB detection to cover TSC

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31340 )

Change subject: cpu/intel/common: Extend FSB detection to cover TSC
......................................................................

Patch Set 11:

(1 comment)

https://review.coreboot.org/c/coreboot/+/31340/11/src/cpu/intel/common/fsb.c 
File src/cpu/intel/common/fsb.c:

https://review.coreboot.org/c/coreboot/+/31340/11/src/cpu/intel/common/fsb.c... 
PS11, Line 50: 			*ratio = rdmsr(MSR_EBC_FREQUENCY_ID).lo >> 24;
This rdmsr resets my getac p470 (family 06, model 0f, stepping 06), despite the claim of table 2-48 in docid #335592 that it should return information on scalable bus speed configuration.

Replacing it with *ratio = 1; fixes it for me. Even _adding_ that line after this (ie. the MSR value is disregarded), but keeping the rdmsr in returns to the reset loop.

Change in coreboot[master]: cpu/intel/common: Extend FSB detection to cover TSC

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/31340 )

Change subject: cpu/intel/common: Extend FSB detection to cover TSC
......................................................................

Change in coreboot[master]: cpu/intel/common: Extend FSB detection to cover TSC

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31340 )

Change subject: cpu/intel/common: Extend FSB detection to cover TSC
......................................................................

Patch Set 11:

(1 comment)

https://review.coreboot.org/c/coreboot/+/31340/11/src/cpu/intel/common/fsb.c 
File src/cpu/intel/common/fsb.c:

https://review.coreboot.org/c/coreboot/+/31340/11/src/cpu/intel/common/fsb.c... 
PS11, Line 50: 			*ratio = rdmsr(MSR_EBC_FREQUENCY_ID).lo >> 24;
> This rdmsr resets my getac p470 (family 06, model 0f, stepping 06), despite the claim of table 2-48  […]

Hmm.. looking at CB:34200, which would remove i945/udelay.c, IA32_PERF_STATUS MSR 0x198 would have the ratio we want for tsc_freq_mhz() implementation?

Motivation for these changes is to have single udelay() for different stages, and have it run from monotonic timer.

Change in coreboot[master]: cpu/intel/common: Extend FSB detection to cover TSC

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31340 )

Change subject: cpu/intel/common: Extend FSB detection to cover TSC
......................................................................

Patch Set 11:

CB:35548 is candidate to fix this instead of revert. Apparently we hit bad docs and I don't have hardware to test core/core2/atom.