Change in coreboot[master]: nb/intel/sandybridge: Store CPUID in ctrl struct

[Angel Pons (Code Review)]

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39720 )

Change subject: nb/intel/sandybridge: Store CPUID in ctrl struct
......................................................................

nb/intel/sandybridge: Store CPUID in ctrl struct

Instead of storing an int with a single bit of information taken from
the CPUID, we might as well store the actual CPUID.

Tested on Asus P8Z77-V LX2, still boots fine.

Change-Id: I6ac435fb83900a52890f823e7614055061299e23
Signed-off-by: Angel Pons 
---
M src/northbridge/intel/sandybridge/raminit.c
M src/northbridge/intel/sandybridge/raminit_common.h
2 files changed, 6 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/39720/1

diff --git a/src/northbridge/intel/sandybridge/raminit.c b/src/northbridge/intel/sandybridge/raminit.c
index 34fb499..d332a47 100644
--- a/src/northbridge/intel/sandybridge/raminit.c
+++ b/src/northbridge/intel/sandybridge/raminit.c
@@ -229,7 +229,7 @@

static int try_init_dram_ddr3(ramctr_timing *ctrl, int fast_boot, int s3resume, int me_uma_size)
 {
-	if (ctrl->sandybridge)
+	if (IS_SANDY_CPU(ctrl->cpu))
    	return try_init_dram_ddr3_snb(ctrl, fast_boot, s3resume, me_uma_size);
    else
    	return try_init_dram_ddr3_ivb(ctrl, fast_boot, s3resume, me_uma_size);
@@ -242,7 +242,6 @@
    spd_raw_data spds[4];
    struct region_device rdev;
    ramctr_timing *ctrl_cached;
-	u32 cpu;

MCHBAR32(SAPMCTL) |= 1;

@@ -315,8 +314,7 @@
    	ctrl.tCK = min_tck;

/* Get architecture */
-		cpu = cpu_get_cpuid();
-		ctrl.sandybridge = IS_SANDY_CPU(cpu);
+		ctrl.cpu = cpu_get_cpuid();

/* Get DDR3 SPD data */
    	memset(spds, 0, sizeof(spds));
@@ -336,8 +334,7 @@
    	ctrl.tCK = min_tck;

/* Get architecture */
-		cpu = cpu_get_cpuid();
-		ctrl.sandybridge = IS_SANDY_CPU(cpu);
+		ctrl.cpu = cpu_get_cpuid();

/* Reset DDR3 frequency */
    	dram_find_spds_ddr3(spds, &ctrl);
diff --git a/src/northbridge/intel/sandybridge/raminit_common.h b/src/northbridge/intel/sandybridge/raminit_common.h
index 0735cea..0047158 100644
--- a/src/northbridge/intel/sandybridge/raminit_common.h
+++ b/src/northbridge/intel/sandybridge/raminit_common.h
@@ -76,7 +76,9 @@

typedef struct ramctr_timing_st {
    u16 spd_crc[NUM_CHANNELS][NUM_SLOTS];
-	int sandybridge;
+
+	/* CPUID value */
+	u32 cpu;

/* DDR base_freq = 100 Mhz / 133 Mhz */
    u8 base_freq;

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39720 )

Change subject: nb/intel/sandybridge: Store CPUID in ctrl struct
......................................................................

Patch Set 1: Code-Review-1

(1 comment)

This uncovers a rather nasty bug.

https://review.coreboot.org/c/coreboot/+/39720/1/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/raminit.c:

https://review.coreboot.org/c/coreboot/+/39720/1/src/northbridge/intel/sandy... 
PS1, Line 299: 		err = try_init_dram_ddr3(&ctrl, fast_boot, s3resume, me_uma_size);
If the CPU was replaced, but the DIMMs were not, this would be called with the wrong information. This happens both with and without this patch.

Solution? At the beginning of the function, or even as a parameter:

const u32 cpuid = cpu_get_cpuid();

Where SPDs are checked (block above), also check if the CPUID is the same. If there's a mismatch, disable fast boot.

Where ctrl.cpu is set, just use the cpuid variable.

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: nb/intel/sandybridge: Store CPUID in ctrl struct
......................................................................

nb/intel/sandybridge: Store CPUID in ctrl struct

Instead of storing an int with a single bit of information taken from
the CPUID, we might as well store the actual CPUID.

Tested on Asus P8Z77-V LX2, still boots fine.

Change-Id: I6ac435fb83900a52890f823e7614055061299e23
Signed-off-by: Angel Pons 
---
M src/northbridge/intel/sandybridge/raminit.c
M src/northbridge/intel/sandybridge/raminit_common.h
2 files changed, 8 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/39720/2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39720 )

Change subject: nb/intel/sandybridge: Store CPUID in ctrl struct
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39720/1/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/raminit.c:

https://review.coreboot.org/c/coreboot/+/39720/1/src/northbridge/intel/sandy... 
PS1, Line 299: 		err = try_init_dram_ddr3(&ctrl, fast_boot, s3resume, me_uma_size);
> If the CPU was replaced, but the DIMMs were not, this would be called with the wrong information. […]

Done

[Patrick Rudolph (Code Review)]

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

Change subject: nb/intel/sandybridge: Store CPUID in ctrl struct
......................................................................

Patch Set 3:

You must bump MRC_CACHE_VERSION too to invalidate old mrc caches.

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: nb/intel/sandybridge: Store CPUID in ctrl struct
......................................................................

nb/intel/sandybridge: Store CPUID in ctrl struct

Instead of storing an int with a single bit of information taken from
the CPUID, we might as well store the actual CPUID. And since we are
changing the definition of the saved data, bump the version number.

Tested on Asus P8Z77-V LX2, still boots fine.

Change-Id: I6ac435fb83900a52890f823e7614055061299e23
Signed-off-by: Angel Pons 
---
M src/northbridge/intel/sandybridge/raminit.c
M src/northbridge/intel/sandybridge/raminit_common.h
2 files changed, 9 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/39720/4

[HAOUAS Elyes (Code Review)]

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

Change subject: nb/intel/sandybridge: Store CPUID in ctrl struct
......................................................................

Patch Set 5: Code-Review+1

[Patrick Georgi (Code Review)]

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

Change subject: nb/intel/sandybridge: Store CPUID in ctrl struct
......................................................................

Patch Set 5: Code-Review+2

[Patrick Georgi (Code Review)]

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

Change subject: nb/intel/sandybridge: Store CPUID in ctrl struct
......................................................................

nb/intel/sandybridge: Store CPUID in ctrl struct

Instead of storing an int with a single bit of information taken from
the CPUID, we might as well store the actual CPUID. And since we are
changing the definition of the saved data, bump the version number.

Tested on Asus P8Z77-V LX2, still boots fine.

Change-Id: I6ac435fb83900a52890f823e7614055061299e23
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39720
Reviewed-by: HAOUAS Elyes 
Reviewed-by: Patrick Georgi 
Tested-by: build bot (Jenkins) 
---
M src/northbridge/intel/sandybridge/raminit.c
M src/northbridge/intel/sandybridge/raminit_common.h
2 files changed, 9 insertions(+), 10 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved
  HAOUAS Elyes: Looks good to me, but someone else must approve

diff --git a/src/northbridge/intel/sandybridge/raminit.c b/src/northbridge/intel/sandybridge/raminit.c
index dc99913..2a3e4d7 100644
--- a/src/northbridge/intel/sandybridge/raminit.c
+++ b/src/northbridge/intel/sandybridge/raminit.c
@@ -227,20 +227,19 @@

static int try_init_dram_ddr3(ramctr_timing *ctrl, int fast_boot, int s3resume, int me_uma_size)
 {
-	if (ctrl->sandybridge)
+	if (IS_SANDY_CPU(ctrl->cpu))
    	return try_init_dram_ddr3_snb(ctrl, fast_boot, s3resume, me_uma_size);
    else
    	return try_init_dram_ddr3_ivb(ctrl, fast_boot, s3resume, me_uma_size);
 }

-static void init_dram_ddr3(int min_tck, int s3resume)
+static void init_dram_ddr3(int min_tck, int s3resume, const u32 cpuid)
 {
    int me_uma_size, cbmem_was_inited, fast_boot, err;
    ramctr_timing ctrl;
    spd_raw_data spds[4];
    struct region_device rdev;
    ramctr_timing *ctrl_cached;
-	u32 cpu;

MCHBAR32(SAPMCTL) |= 1;

@@ -313,8 +312,7 @@
    	ctrl.tCK = min_tck;

/* Get architecture */
-		cpu = cpu_get_cpuid();
-		ctrl.sandybridge = IS_SANDY_CPU(cpu);
+		ctrl.cpu = cpuid;

/* Get DDR3 SPD data */
    	memset(spds, 0, sizeof(spds));
@@ -334,8 +332,7 @@
    	ctrl.tCK = min_tck;

/* Get architecture */
-		cpu = cpu_get_cpuid();
-		ctrl.sandybridge = IS_SANDY_CPU(cpu);
+		ctrl.cpu = cpuid;

/* Reset DDR3 frequency */
    	dram_find_spds_ddr3(spds, &ctrl);
@@ -385,5 +382,5 @@

timestamp_add_now(TS_BEFORE_INITRAM);

-	init_dram_ddr3(get_mem_min_tck(), s3resume);
+	init_dram_ddr3(get_mem_min_tck(), s3resume, cpu_get_cpuid());
 }
diff --git a/src/northbridge/intel/sandybridge/raminit_common.h b/src/northbridge/intel/sandybridge/raminit_common.h
index 18a69af..0cbac8a 100644
--- a/src/northbridge/intel/sandybridge/raminit_common.h
+++ b/src/northbridge/intel/sandybridge/raminit_common.h
@@ -44,7 +44,7 @@
 /*
  * WARNING: Do not forget to increase MRC_CACHE_VERSION when the saved data is changed!
  */
-#define MRC_CACHE_VERSION 1
+#define MRC_CACHE_VERSION 2

typedef struct odtmap_st {
    u16 rttwr;
@@ -82,7 +82,9 @@
 /* WARNING: Do not forget to increase MRC_CACHE_VERSION when this struct is changed! */
 typedef struct ramctr_timing_st {
    u16 spd_crc[NUM_CHANNELS][NUM_SLOTS];
-	int sandybridge;
+
+	/* CPUID value */
+	u32 cpu;

/* DDR base_freq = 100 Mhz / 133 Mhz */
    u8 base_freq;