Change in coreboot[master]: Revert "intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK"

[Lean Sheng Tan (Code Review)]

Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Nathaniel L Desimone, Subrata Banik, Marshall Dawson, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: Revert "intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK"
......................................................................

Revert "intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK"

This reverts commit c9871505f115f0e5722bf23b13f4390c8d76d0da.

Reason for revert: <INSERT REASONING HERE>

Change-Id: I46889a3860a0704f9011c8227005207dcb740d0e
---
M src/drivers/intel/fsp2_0/Kconfig
M src/drivers/intel/fsp2_0/memory_init.c
2 files changed, 12 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/35324/1

diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig
index 541ec47..3fb39d5 100644
--- a/src/drivers/intel/fsp2_0/Kconfig
+++ b/src/drivers/intel/fsp2_0/Kconfig
@@ -152,11 +152,6 @@
      without reinitializing stack pointer. This feature is
      supported Icelake onwards.

-config FSP_TEMP_RAM_SIZE
-	hex
-	default 0x10000
-	depends on FSP_USES_CB_STACK
-
 config VERIFY_HOBS
    bool "Verify the FSP hand-off-blocks"
    default n
diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c
index 9789c96..0797c2e 100644
--- a/src/drivers/intel/fsp2_0/memory_init.c
+++ b/src/drivers/intel/fsp2_0/memory_init.c
@@ -34,8 +34,6 @@
 #include <fsp/memory_init.h>
 #include <types.h>

-static uint8_t temp_ram[CONFIG_FSP_TEMP_RAM_SIZE] __aligned(sizeof(uint64_t));
-
 /* TPM MRC hash functionality depends on vboot starting before memory init. */
 _Static_assert(!CONFIG(FSP2_0_USES_TPM_MRC_HASH) ||
           CONFIG(VBOOT_STARTS_IN_BOOTBLOCK),
@@ -163,7 +161,6 @@

return CB_SUCCESS;
 }
-
 static enum cb_err setup_fsp_stack_frame(FSPM_ARCH_UPD *arch_upd,
    	const struct memranges *memmap)
 {
@@ -171,6 +168,17 @@
    uintptr_t stack_end;

/*
+	 * FSP 2.1 version would use same stack as coreboot instead of
+	 * setting up seprate stack frame. FSP 2.1 would not relocate stack
+	 * top and does not reinitialize stack pointer.
+	 */
+	if (CONFIG(FSP_USES_CB_STACK)) {
+		arch_upd->StackBase = (void *)_car_stack_start;
+		arch_upd->StackSize = CONFIG_DCACHE_BSP_STACK_SIZE;
+		return CB_SUCCESS;
+	}
+
+	/*
     * FSPM_UPD passed here is populated with default values
     * provided by the blob itself. We let FSPM use top of CAR
     * region of the size it requests.
@@ -189,19 +197,8 @@
    				bool s3wake, uint32_t fsp_version,
    				const struct memranges *memmap)
 {
-	/*
-	 * FSP 2.1 version would use same stack as coreboot instead of
-	 * setting up separate stack frame. FSP 2.1 would not relocate stack
-	 * top and does not reinitialize stack pointer. The parameters passed
-	 * as StackBase and StackSize are actually for temporary RAM and HOBs
-	 * and are not related to FSP stack at all.
-	 */
-	if (CONFIG(FSP_USES_CB_STACK)) {
-		arch_upd->StackBase = temp_ram;
-		arch_upd->StackSize = sizeof(temp_ram);
-	} else if (setup_fsp_stack_frame(arch_upd, memmap)) {
+	if (setup_fsp_stack_frame(arch_upd, memmap))
    	return CB_ERR;
-	}

fsp_fill_mrc_cache(arch_upd, fsp_version);

[Lean Sheng Tan (Code Review)]

Lean Sheng Tan has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35324 )

Change subject: Revert "intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK"
......................................................................

Abandoned

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35324 )

Change subject: Revert "intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK"
......................................................................

Patch Set 1:

can you please share the reason for revert ?