Change in coreboot[master]: drivers/intel/fsp2_0: add Kconfig for separate stack in DRAM
Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44406 )
Change subject: drivers/intel/fsp2_0: add Kconfig for separate stack in DRAM ......................................................................
drivers/intel/fsp2_0: add Kconfig for separate stack in DRAM
soc/amd/picasso selected FSP_USES_CB_STACK even though it is FSP 2.0 based, so it doesn't reuse coreboot's stack, but sets up its own stack. In contrast to all other FSP 2.0 based platforms, this stack isn't in the CAR region, since AMD Picasso doesn't support CAR and the DRAM is already available when the x86 cores are released from reset. Selecting FSP_USES_CB_STACK ended up doing the right thing, but is semantically wrong. This patch introduces the new Kconfig option FSP_USES_STACK_IN_DRAM that will also result in the right code path in the FSP driver being taken.
BUG=b:155501050 TEST=Mandolin still boots.
Change-Id: Icd0ff8e17a535e2c247793b64f4b0565887183d8 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c M src/soc/amd/picasso/Kconfig 3 files changed, 19 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/44406/1
diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig index 3caa04a..2d04a0b 100644 --- a/src/drivers/intel/fsp2_0/Kconfig +++ b/src/drivers/intel/fsp2_0/Kconfig @@ -130,14 +130,24 @@ without reinitializing stack pointer. This feature is supported Icelake onwards.
+config FSP_USES_STACK_IN_DRAM + bool + default n + help + Enable support for FSP to use a DRAM region as stack. This is used in + non-CAR platforms where FSP still uses its own separate stack. + config FSP_TEMP_RAM_SIZE hex - depends on FSP_USES_CB_STACK + depends on FSP_USES_CB_STACK || FSP_USES_STACK_IN_DRAM help - The amount of anticipated heap usage in CAR by FSP to setup HOB. - This configuration is applicable for FSP specification using shared - stack with coreboot/bootloader. - Sync this value with Platform FSP integration guide recommendation. + The amount of memory coreboot reserves for the FSP to use. In the + case of FSP 2.1 and newer that share the stack with coreboot instead + of having its own stack, this is the amount of anticipated heap usage + in CAR by FSP to setup HOB and needs to be the recommened value from + the Platform FSP integration guide. In the case of the FSP having its + own stack that will be placed in DRAM and not in CAR, this is the + amount of memory the FSP needs for its stack and heap.
config FSP2_0_USES_TPM_MRC_HASH bool diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 7f5d389..8190c5e 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -184,8 +184,9 @@ * 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. + * Non-CAR FSP 2.0 platforms pass a DRAM location for the FSP stack. */ - if (CONFIG(FSP_USES_CB_STACK)) { + if (CONFIG(FSP_USES_CB_STACK) || CONFIG(FSP_USES_STACK_IN_DRAM)) { arch_upd->StackBase = temp_ram; arch_upd->StackSize = sizeof(temp_ram); } else if (setup_fsp_stack_frame(arch_upd, memmap)) { diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index 526900a..8ebe9b6 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -53,7 +53,7 @@ select PLATFORM_USES_FSP2_0 select FSP_COMPRESS_FSP_M_LZMA select FSP_COMPRESS_FSP_S_LZMA - select FSP_USES_CB_STACK + select FSP_USES_STACK_IN_DRAM select UDK_2017_BINDING select HAVE_CF9_RESET select SUPPORT_CPU_UCODE_IN_CBFS @@ -377,7 +377,7 @@
config FSP_TEMP_RAM_SIZE hex - depends on FSP_USES_CB_STACK + depends on FSP_USES_STACK_IN_DRAM default 0x40000 help The amount of coreboot-allocated heap and stack usage by the FSP.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44406 )
Change subject: drivers/intel/fsp2_0: add Kconfig for separate stack in DRAM ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44406/1/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/44406/1/src/drivers/intel/fsp2_0/Kc... PS1, Line 147: in CAR by FSP to setup HOB and needs to be the recommened value from 'recommened' may be misspelled - perhaps 'recommended'?
Hello build bot (Jenkins), Furquan Shaikh, Marshall Dawson, Andrey Petrov, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44406
to look at the new patch set (#2).
Change subject: drivers/intel/fsp2_0: add Kconfig for separate stack in DRAM ......................................................................
drivers/intel/fsp2_0: add Kconfig for separate stack in DRAM
soc/amd/picasso selected FSP_USES_CB_STACK even though it is FSP 2.0 based, so it doesn't reuse coreboot's stack, but sets up its own stack. In contrast to all other FSP 2.0 based platforms, this stack isn't in the CAR region, since AMD Picasso doesn't support CAR and the DRAM is already available when the x86 cores are released from reset. Selecting FSP_USES_CB_STACK ended up doing the right thing, but is semantically wrong. This patch introduces the new Kconfig option FSP_USES_STACK_IN_DRAM that will also result in the right code path in the FSP driver being taken.
BUG=b:155501050 TEST=Mandolin still boots.
Change-Id: Icd0ff8e17a535e2c247793b64f4b0565887183d8 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c M src/soc/amd/picasso/Kconfig 3 files changed, 19 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/44406/2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44406 )
Change subject: drivers/intel/fsp2_0: add Kconfig for separate stack in DRAM ......................................................................
Patch Set 2:
would it be a better idea to invert the logic and have FSP_USES_SEPARATE_STACK_IN_CAR Kconfig option? That wouldn't add another Kconfig option that results in the same behaviour
Hello build bot (Jenkins), Furquan Shaikh, Marshall Dawson, Andrey Petrov, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44406
to look at the new patch set (#3).
Change subject: drivers/intel/fsp2_0: don't select FSP_USES_CB_STACK on FSP 2.0 platform ......................................................................
drivers/intel/fsp2_0: don't select FSP_USES_CB_STACK on FSP 2.0 platform
soc/amd/picasso selected FSP_USES_CB_STACK even though it is FSP 2.0 based, so it doesn't reuse coreboot's stack, but sets up its own stack. In contrast to all other FSP 2.0 based platforms, this stack isn't in the CAR region, since AMD Picasso doesn't support CAR and the DRAM is already available when the x86 cores are released from reset. Selecting FSP_USES_CB_STACK ended up doing the right thing, but is semantically wrong. Instead of wrongly selecting FSP_USES_CB_STACK in soc/amd/picasso we take the corresponding code path if ENV_CACHE_AS_RAM is false which is only the case for non-CAR platforms.
BUG=b:155501050 TEST=Timeless build results in identical binary for Mandolin.
Change-Id: Icd0ff8e17a535e2c247793b64f4b0565887183d8 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c M src/soc/amd/picasso/Kconfig 3 files changed, 9 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/44406/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44406 )
Change subject: drivers/intel/fsp2_0: don't select FSP_USES_CB_STACK on FSP 2.0 platform ......................................................................
Patch Set 3: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44406 )
Change subject: drivers/intel/fsp2_0: don't select FSP_USES_CB_STACK on FSP 2.0 platform ......................................................................
Patch Set 3: Code-Review+2
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44406 )
Change subject: drivers/intel/fsp2_0: don't select FSP_USES_CB_STACK on FSP 2.0 platform ......................................................................
Patch Set 3: Code-Review+2
Hello build bot (Jenkins), Furquan Shaikh, Marshall Dawson, Andrey Petrov, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44406
to look at the new patch set (#4).
Change subject: drivers/intel/fsp2_0: don't select FSP_USES_CB_STACK on FSP 2.0 platform ......................................................................
drivers/intel/fsp2_0: don't select FSP_USES_CB_STACK on FSP 2.0 platform
soc/amd/picasso selected FSP_USES_CB_STACK even though it is FSP 2.0 based, so it doesn't reuse coreboot's stack, but sets up its own stack. In contrast to all other FSP 2.0 based platforms, this stack isn't in the CAR region, since AMD Picasso doesn't support CAR and the DRAM is already available when the x86 cores are released from reset. Selecting FSP_USES_CB_STACK ended up doing the right thing, but is semantically wrong. Instead of wrongly selecting FSP_USES_CB_STACK in soc/amd/picasso we take the corresponding code path if ENV_CACHE_AS_RAM is false which is only the case for non-CAR platforms.
BUG=b:155501050 TEST=Timeless build results in an identical binary for amd/mandolin, asrock/h110m-dvs and intel/coffeelake_rvp11 which cover all 3 cases here.
Change-Id: Icd0ff8e17a535e2c247793b64f4b0565887183d8 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c M src/soc/amd/picasso/Kconfig 3 files changed, 9 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/44406/4
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44406 )
Change subject: drivers/intel/fsp2_0: don't select FSP_USES_CB_STACK on FSP 2.0 platform ......................................................................
drivers/intel/fsp2_0: don't select FSP_USES_CB_STACK on FSP 2.0 platform
soc/amd/picasso selected FSP_USES_CB_STACK even though it is FSP 2.0 based, so it doesn't reuse coreboot's stack, but sets up its own stack. In contrast to all other FSP 2.0 based platforms, this stack isn't in the CAR region, since AMD Picasso doesn't support CAR and the DRAM is already available when the x86 cores are released from reset. Selecting FSP_USES_CB_STACK ended up doing the right thing, but is semantically wrong. Instead of wrongly selecting FSP_USES_CB_STACK in soc/amd/picasso we take the corresponding code path if ENV_CACHE_AS_RAM is false which is only the case for non-CAR platforms.
BUG=b:155501050 TEST=Timeless build results in an identical binary for amd/mandolin, asrock/h110m-dvs and intel/coffeelake_rvp11 which cover all 3 cases here.
Change-Id: Icd0ff8e17a535e2c247793b64f4b0565887183d8 Signed-off-by: Felix Held felix-coreboot@felixheld.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/44406 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c M src/soc/amd/picasso/Kconfig 3 files changed, 9 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Furquan Shaikh: Looks good to me, approved Marshall Dawson: Looks good to me, approved
diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig index 3caa04a..00bfd67 100644 --- a/src/drivers/intel/fsp2_0/Kconfig +++ b/src/drivers/intel/fsp2_0/Kconfig @@ -132,12 +132,14 @@
config FSP_TEMP_RAM_SIZE hex - depends on FSP_USES_CB_STACK help - The amount of anticipated heap usage in CAR by FSP to setup HOB. - This configuration is applicable for FSP specification using shared - stack with coreboot/bootloader. - Sync this value with Platform FSP integration guide recommendation. + The amount of memory coreboot reserves for the FSP to use. In the + case of FSP 2.1 and newer that share the stack with coreboot instead + of having its own stack, this is the amount of anticipated heap usage + in CAR by FSP to setup HOB and needs to be the recommended value from + the Platform FSP integration guide. In the case of the FSP having its + own stack that will be placed in DRAM and not in CAR, this is the + amount of memory the FSP needs for its stack and heap.
config FSP2_0_USES_TPM_MRC_HASH bool diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 7f5d389..57a0520 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -184,8 +184,9 @@ * 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. + * Non-CAR FSP 2.0 platforms pass a DRAM location for the FSP stack. */ - if (CONFIG(FSP_USES_CB_STACK)) { + if (CONFIG(FSP_USES_CB_STACK) || !ENV_CACHE_AS_RAM) { arch_upd->StackBase = temp_ram; arch_upd->StackSize = sizeof(temp_ram); } else if (setup_fsp_stack_frame(arch_upd, memmap)) { diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index 8001b7a..1b83000 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -53,7 +53,6 @@ select PLATFORM_USES_FSP2_0 select FSP_COMPRESS_FSP_M_LZMA select FSP_COMPRESS_FSP_S_LZMA - select FSP_USES_CB_STACK select UDK_2017_BINDING select HAVE_CF9_RESET select SUPPORT_CPU_UCODE_IN_CBFS @@ -377,7 +376,6 @@
config FSP_TEMP_RAM_SIZE hex - depends on FSP_USES_CB_STACK default 0x40000 help The amount of coreboot-allocated heap and stack usage by the FSP.
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Felix Held (Code Review)
-
Furquan Shaikh (Code Review)
-
Marshall Dawson (Code Review)