Change in coreboot[master]: soc/intel/xeon_sp: Use native CAR teardown

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48467 )

Change subject: soc/intel/xeon_sp: Use native CAR teardown
......................................................................

soc/intel/xeon_sp: Use native CAR teardown

This cleans up the postcar frame setup, which now gets used instead of
just going with TempRamExit MTRR's (Note that CPU init overwrites them
anyway).

TESTED on ocp/deltalake.

Change-Id: I756c2d479fef859a460696300422f08013a300f1
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/cpx/Kconfig
M src/soc/intel/xeon_sp/memmap.c
2 files changed, 22 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/48467/1

diff --git a/src/soc/intel/xeon_sp/cpx/Kconfig b/src/soc/intel/xeon_sp/cpx/Kconfig
index 369d474..ce92ed5 100644
--- a/src/soc/intel/xeon_sp/cpx/Kconfig
+++ b/src/soc/intel/xeon_sp/cpx/Kconfig
@@ -47,6 +47,14 @@
      says this needs to be 256KiB, but practice show this needs to
      be a lot more.

+config NO_FSP_TEMP_RAM_EXIT
+	bool
+	default y
+
+config INTEL_CAR_NEM
+	bool
+	default y
+
 config CPU_MICROCODE_CBFS_LOC
    hex
    default 0xfff0fdc0
diff --git a/src/soc/intel/xeon_sp/memmap.c b/src/soc/intel/xeon_sp/memmap.c
index cd81754..0af0ad2 100644
--- a/src/soc/intel/xeon_sp/memmap.c
+++ b/src/soc/intel/xeon_sp/memmap.c
@@ -29,17 +29,22 @@

void fill_postcar_frame(struct postcar_frame *pcf)
 {
-	/*
-	 * We need to make sure ramstage will be run cached. At this
-	 * point exact location of ramstage in cbmem is not known.
-	 * Instruct postcar to cache 16 megs under cbmem top which is
-	 * a safe bet to cover ramstage.
-	 */
-	uintptr_t top_of_ram = (uintptr_t)cbmem_top();
+	const uintptr_t top_of_ram = (uintptr_t)cbmem_top();
+	uintptr_t cbmem_base;
+	size_t cbmem_size;

+	/* Try account for the CBMEM region currently used and for future use */
+	cbmem_get_region((void **)&cbmem_base, &cbmem_size);
    printk(BIOS_DEBUG, "top_of_ram = 0x%lx\n", top_of_ram);
-	top_of_ram -= 16 * MiB;
-	postcar_frame_add_mtrr(pcf, top_of_ram, 16 * MiB, MTRR_TYPE_WRBACK);
+	printk(BIOS_DEBUG, "cbmem base_ptr: 0x%lx, size: 0x%lx\n", cbmem_base, cbmem_size);
+	/* Assume 4MiB will be enough for future cbmem objects (FSP-S, ramstage, ...) */
+	cbmem_base -= 4 * MiB;
+	cbmem_base = ALIGN_DOWN(cbmem_base, 4 * MiB);
+
+	/* Align the top to make sure we don't use too many MTRR's */
+	cbmem_size = ALIGN_UP(top_of_ram - cbmem_base, 4 * MiB);
+
+	postcar_frame_add_mtrr(pcf, cbmem_base, cbmem_size, MTRR_TYPE_WRBACK);
    /* Cache the TSEG region */
    if (CONFIG(TSEG_STAGE_CACHE))
    	postcar_enable_tseg_cache(pcf);

[Angel Pons (Code Review)]

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

Change subject: soc/intel/xeon_sp: Use native CAR teardown
......................................................................

Patch Set 1: Code-Review+1

(2 comments)

https://review.coreboot.org/c/coreboot/+/48467/1/src/soc/intel/xeon_sp/cpx/K... 
File src/soc/intel/xeon_sp/cpx/Kconfig:

https://review.coreboot.org/c/coreboot/+/48467/1/src/soc/intel/xeon_sp/cpx/K... 
PS1, Line 51: bool
Specifying types shouldn't be needed here

https://review.coreboot.org/c/coreboot/+/48467/1/src/soc/intel/xeon_sp/cpx/K... 
PS1, Line 54: INTEL_CAR_NEM
Shouldn't this be selected?

[Marc Jones (Code Review)]

Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48467 )

Change subject: soc/intel/xeon_sp: Use native CAR teardown
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/48467/1/src/soc/intel/xeon_sp/cpx/K... 
File src/soc/intel/xeon_sp/cpx/Kconfig:

https://review.coreboot.org/c/coreboot/+/48467/1/src/soc/intel/xeon_sp/cpx/K... 
PS1, Line 51: bool
> Specifying types shouldn't be needed here

What does this do? I don't see it used anywhere.

https://review.coreboot.org/c/coreboot/+/48467/1/src/soc/intel/xeon_sp/cpx/K... 
PS1, Line 54: INTEL_CAR_NEM
> Shouldn't this be selected?

I think so too.

[Marc Jones (Code Review)]

Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48467 )

Change subject: soc/intel/xeon_sp: Use native CAR teardown
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48467/1/src/soc/intel/xeon_sp/cpx/K... 
File src/soc/intel/xeon_sp/cpx/Kconfig:

https://review.coreboot.org/c/coreboot/+/48467/1/src/soc/intel/xeon_sp/cpx/K... 
PS1, Line 51: bool
> What does this do? I don't see it used anywhere.

oh I see it is added in the previous patch. I think this should also be a select in the xeon_sp Kconfig SOC_INTEL_COOPERLAKE_SP

[Arthur Heymans (Code Review)]

Hello Marc Jones, build bot (Jenkins), Anjaneya "Reddy" Chagam, Patrick Georgi, Jonathan Zhang, Christian Walter, Angel Pons, Subrata Banik, Patrick Rudolph,

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

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

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

Change subject: soc/intel/xeon_sp: Use native CAR teardown
......................................................................

soc/intel/xeon_sp: Use native CAR teardown

This cleans up the postcar frame setup, which now gets used instead of
just going with TempRamExit MTRR's (Note that CPU init overwrites them
anyway).

TESTED on ocp/deltalake and ocp/tiogapass.

Change-Id: I756c2d479fef859a460696300422f08013a300f1
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/Kconfig
M src/soc/intel/xeon_sp/memmap.c
2 files changed, 16 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/48467/2

[Arthur Heymans (Code Review)]

Attention is currently required from: Marc Jones, Angel Pons.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48467 )

Change subject: soc/intel/xeon_sp: Use native CAR teardown
......................................................................

Patch Set 1:

(2 comments)

File src/soc/intel/xeon_sp/cpx/Kconfig:

https://review.coreboot.org/c/coreboot/+/48467/comment/66acfc40_e7c48eff 
PS1, Line 51: bool
> oh I see it is added in the previous patch. […]

Done. This works too on tiogapass so I moved it to xeon_sp/Kconfig

https://review.coreboot.org/c/coreboot/+/48467/comment/16a14b6b_4a13fad0 
PS1, Line 54: INTEL_CAR_NEM
> I think so too.

Done

[Angel Pons (Code Review)]

Attention is currently required from: Marc Jones, Arthur Heymans.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48467 )

Change subject: soc/intel/xeon_sp: Use native CAR teardown
......................................................................

Patch Set 2: Code-Review+2

[Paul Menzel (Code Review)]

Attention is currently required from: Marc Jones, Arthur Heymans.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48467 )

Change subject: soc/intel/xeon_sp: Use native CAR teardown
......................................................................

Patch Set 2:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/48467/comment/623bef29_5ffbd693 
PS2, Line 11: anyway).
Nit: I’d put the dot before the (, and add a separate to the sentence inside the (). (Even remove the ().)

[Arthur Heymans (Code Review)]

Attention is currently required from: Marc Jones.
Hello build bot (Jenkins), Marc Jones, Anjaneya "Reddy" Chagam, Patrick Georgi, Jonathan Zhang, Christian Walter, Angel Pons, Subrata Banik, Patrick Rudolph,

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

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

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

Change subject: soc/intel/xeon_sp: Use native CAR teardown
......................................................................

soc/intel/xeon_sp: Use native CAR teardown

This cleans up the postcar frame setup, which now gets used instead of
just going with TempRamExit MTRR's.

Note that ramstage CPU init sets up different final MTRRs anyway.

TESTED on ocp/deltalake and ocp/tiogapass.

Change-Id: I756c2d479fef859a460696300422f08013a300f1
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/Kconfig
M src/soc/intel/xeon_sp/memmap.c
2 files changed, 16 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/48467/4

[Angel Pons (Code Review)]

Attention is currently required from: Marc Jones, Paul Menzel, Arthur Heymans.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48467 )

Change subject: soc/intel/xeon_sp: Use native CAR teardown
......................................................................

Patch Set 4:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/48467/comment/358376c3_c32ab54f 
PS2, Line 11: anyway).
> Nit: I’d put the dot before the (, and add a separate to the sentence inside the (). […]

Done

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/xeon_sp: Use native CAR teardown
......................................................................

soc/intel/xeon_sp: Use native CAR teardown

This cleans up the postcar frame setup, which now gets used instead of
just going with TempRamExit MTRR's.

Note that ramstage CPU init sets up different final MTRRs anyway.

TESTED on ocp/deltalake and ocp/tiogapass.

Change-Id: I756c2d479fef859a460696300422f08013a300f1
Signed-off-by: Arthur Heymans 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/48467
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M src/soc/intel/xeon_sp/Kconfig
M src/soc/intel/xeon_sp/memmap.c
2 files changed, 16 insertions(+), 9 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/intel/xeon_sp/Kconfig b/src/soc/intel/xeon_sp/Kconfig
index 6c10c35..d84a80e 100644
--- a/src/soc/intel/xeon_sp/Kconfig
+++ b/src/soc/intel/xeon_sp/Kconfig
@@ -67,6 +67,8 @@
    select HAVE_SMI_HANDLER
    select X86_SMM_LOADER_VERSION2
    select REG_SCRIPT
+	select NO_FSP_TEMP_RAM_EXIT
+	select INTEL_CAR_NEM # For postcar only now

config MAINBOARD_USES_FSP2_0
      bool
diff --git a/src/soc/intel/xeon_sp/memmap.c b/src/soc/intel/xeon_sp/memmap.c
index cd81754..0af0ad2 100644
--- a/src/soc/intel/xeon_sp/memmap.c
+++ b/src/soc/intel/xeon_sp/memmap.c
@@ -29,17 +29,22 @@

void fill_postcar_frame(struct postcar_frame *pcf)
 {
-	/*
-	 * We need to make sure ramstage will be run cached. At this
-	 * point exact location of ramstage in cbmem is not known.
-	 * Instruct postcar to cache 16 megs under cbmem top which is
-	 * a safe bet to cover ramstage.
-	 */
-	uintptr_t top_of_ram = (uintptr_t)cbmem_top();
+	const uintptr_t top_of_ram = (uintptr_t)cbmem_top();
+	uintptr_t cbmem_base;
+	size_t cbmem_size;

+	/* Try account for the CBMEM region currently used and for future use */
+	cbmem_get_region((void **)&cbmem_base, &cbmem_size);
    printk(BIOS_DEBUG, "top_of_ram = 0x%lx\n", top_of_ram);
-	top_of_ram -= 16 * MiB;
-	postcar_frame_add_mtrr(pcf, top_of_ram, 16 * MiB, MTRR_TYPE_WRBACK);
+	printk(BIOS_DEBUG, "cbmem base_ptr: 0x%lx, size: 0x%lx\n", cbmem_base, cbmem_size);
+	/* Assume 4MiB will be enough for future cbmem objects (FSP-S, ramstage, ...) */
+	cbmem_base -= 4 * MiB;
+	cbmem_base = ALIGN_DOWN(cbmem_base, 4 * MiB);
+
+	/* Align the top to make sure we don't use too many MTRR's */
+	cbmem_size = ALIGN_UP(top_of_ram - cbmem_base, 4 * MiB);
+
+	postcar_frame_add_mtrr(pcf, cbmem_base, cbmem_size, MTRR_TYPE_WRBACK);
    /* Cache the TSEG region */
    if (CONFIG(TSEG_STAGE_CACHE))
    	postcar_enable_tseg_cache(pcf);