Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35266 )
Change subject: drivers/intel/fsp2_0: Use generic defines for early storage ......................................................................
drivers/intel/fsp2_0: Use generic defines for early storage
Add the ability to run memory_init.c in an enviornment where there exists no CAR storage. CONFIG(RESET_VECTOR_IN_RAM) uses DRAM as early storage and created using a .ld file without _car symbols.
Substitute _car* with #defines and account for symbols generated when RESET_VECTOR_IN_RAM is active.
Change-Id: Ie9d102c3c1110bbb54ce788ec432da1a27e2f61f Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/drivers/intel/fsp2_0/memory_init.c 1 file changed, 22 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/35266/1
diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 56771e2..b209786 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -34,6 +34,20 @@ #include <fsp/memory_init.h> #include <types.h>
+#if CONFIG(RESET_VECTOR_IN_RAM) + #define EARLY_STORAGE_START _earlyram_region_start + #define EARLY_STORAGE_USED (_earlyram_unallocated_start - _earlyram_region_start) + #define END_OF_ALLOCATED _earlyram_region_end + #define BSP_STACK_BASE _earlyram_stack_start + #define BSP_STACK_SIZE CONFIG_EARLYRAM_BSP_STACK_SIZE +#else + #define EARLY_STORAGE_START _car_region_start + #define EARLY_STORAGE_USED (_car_unallocated_start - _car_region_start) + #define END_OF_ALLOCATED _car_region_end + #define BSP_STACK_BASE _car_stack_start + #define BSP_STACK_SIZE CONFIG_DCACHE_BSP_STACK_SIZE +#endif + /* 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), @@ -174,17 +188,18 @@ * 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; + arch_upd->StackBase = (void *)BSP_STACK_BASE; + arch_upd->StackSize = 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. + * provided by the blob itself. We let FSPM use unallocated + * space at the top of CAR or EARLYRAM region for the size + * it requests. */ - stack_end = (uintptr_t)_car_region_end; + stack_end = (uintptr_t)END_OF_ALLOCATED; stack_begin = stack_end - arch_upd->StackSize; if (check_region_overlap(memmap, "FSPM stack", stack_begin, stack_end) != CB_SUCCESS) @@ -404,8 +419,8 @@
/* Build up memory map of romstage address space including CAR. */ memranges_init_empty(&memmap, &freeranges[0], ARRAY_SIZE(freeranges)); - memranges_insert(&memmap, (uintptr_t)_car_region_start, - _car_unallocated_start - _car_region_start, 0); + memranges_insert(&memmap, (uintptr_t)EARLY_STORAGE_START, + EARLY_STORAGE_USED, 0); memranges_insert(&memmap, (uintptr_t)_program, REGION_SIZE(program), 0);
if (!CONFIG(FSP_M_XIP))
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35266 )
Change subject: drivers/intel/fsp2_0: Use generic defines for early storage ......................................................................
Patch Set 1:
Marshall, I am trying to get a decision between CB:35165 CB:35233 promptly so we know which one to rebase on the next time around.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35266 )
Change subject: drivers/intel/fsp2_0: Use generic defines for early storage ......................................................................
Patch Set 1: Code-Review+1
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35266 )
Change subject: drivers/intel/fsp2_0: Use generic defines for early storage ......................................................................
Patch Set 1:
Patch Set 1:
Marshall, I am trying to get a decision between CB:35165 CB:35233 promptly so we know which one to rebase on the next time around.
I need to run to a meeting, but will look at these when I get back. I considered putting some space into car.ld to positively allocate it, then modify fsp2_0 to use that instead. I wasn't 100% confident that I wouldn't break Intel systems, in the process, though.
BTW, I really disliked this particular PS and I assumed it would be required to change due to whatever changes come before it.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35266 )
Change subject: drivers/intel/fsp2_0: Use generic defines for early storage ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Marshall, I am trying to get a decision between CB:35165 CB:35233 promptly so we know which one to rebase on the next time around.
I need to run to a meeting, but will look at these when I get back. I considered putting some space into car.ld to positively allocate it, then modify fsp2_0 to use that instead. I wasn't 100% confident that I wouldn't break Intel systems, in the process, though.
Seems we have decided on CB:35233.
BTW, I really disliked this particular PS and I assumed it would be required to change due to whatever changes come before it.
Good, I dislike this too :) I will rebase and update this, there should not be much left to deal with.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Richard Spiegel, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35266
to look at the new patch set (#2).
Change subject: _WIP_ drivers/intel/fsp2_0: Use generic defines for early storage ......................................................................
_WIP_ drivers/intel/fsp2_0: Use generic defines for early storage
Add the ability to run memory_init.c in an enviornment where there exists no CAR storage. CONFIG(RESET_VECTOR_IN_RAM) uses DRAM as early storage and created using a .ld file without _car symbols.
Substitute _car* with #defines and account for symbols generated when RESET_VECTOR_IN_RAM is active.
Change-Id: Ie9d102c3c1110bbb54ce788ec432da1a27e2f61f Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/drivers/intel/fsp2_0/memory_init.c 1 file changed, 20 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/35266/2
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35266 )
Change subject: _WIP_ drivers/intel/fsp2_0: Use generic defines for early storage ......................................................................
Patch Set 1:
BTW, I really disliked this particular PS and I assumed it would be required to change due to whatever changes come before it.
Good, I dislike this too :) I will rebase and update this, there should not be much left to deal with.
I see "intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK" and will include that in my next push. However, I have a new problem now so don't be alarmed when this patch is still in the stack. I'm not yet certain how I want to fix it, so I'm open to suggestions.
Right now we're still creating a 64K romstage.elf, and the .bss falls within _program (after the bulk of the functionality but below the reset vector). The large amount of space used for temp_ram in memory_init.c blows up the linking.
(a) Keeping it within _program has a nice side effect of zeroing it at build/compress time, so I'd need to expand my hybrid romstage for the flexibility of being >64K.
or (b) Theoretically my .bss could be relocated to the DRAM I'm burning for early temp storage. So far that hasn't worked the way I want. If it goes there, the BSP will need to explicitly clear that region, as the DRAM comes up with garbage in it. (Possibly data used during training; not sure.)
BTW, AFAIK the FSP_USES_CB_STACK symbol is for FSP 2.1 and we're building 2.0 images currently. We want FSP_USES_CB_STACK, and it's also our intent to change to 2.1 before production, but we're trying to keep as much to established specs as possible.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35266 )
Change subject: _WIP_ drivers/intel/fsp2_0: Use generic defines for early storage ......................................................................
Patch Set 2:
Patch Set 1:
BTW, I really disliked this particular PS and I assumed it would be required to change due to whatever changes come before it.
Good, I dislike this too :) I will rebase and update this, there should not be much left to deal with.
I see "intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK" and will include that in my next push. However, I have a new problem now so don't be alarmed when this patch is still in the stack. I'm not yet certain how I want to fix it, so I'm open to suggestions.
Well see CB:35289 also. One half of the &memmap reference is gone with FSP_USES_CB_STACK.
Those several _car_xxx symbols are used for runtime-checking of static build-time parameters, and to fail there just hits die()? IMHO you could skip both those check_region_overlap() calls and setup build such that you guarantee FSP-M program does not overlap with romstage.elf. Then you could leave the _car_xxx reference untouched and they hit garbage collection.
Right now we're still creating a 64K romstage.elf, and the .bss falls within _program (after the bulk of the functionality but below the reset vector). The large amount of space used for temp_ram in memory_init.c blows up the linking.
Right. It was anticipated we want to customize temp_ram allocation size with SoC config.. sort of waiting for Intel to fix that, like the recent comments there says. Without raminit, I don't know if you need much of a heap at all.
(a) Keeping it within _program has a nice side effect of zeroing it at build/compress time, so I'd need to expand my hybrid romstage for the flexibility of being >64K.
or (b) Theoretically my .bss could be relocated to the DRAM I'm burning for early temp storage. So far that hasn't worked the way I want. If it goes there, the BSP will need to explicitly clear that region, as the DRAM comes up with garbage in it. (Possibly data used during training; not sure.)
BTW, AFAIK the FSP_USES_CB_STACK symbol is for FSP 2.1 and we're building 2.0 images currently. We want FSP_USES_CB_STACK, and it's also our intent to change to 2.1 before production, but we're trying to keep as much to established specs as possible.
It was added prior to FSP2.1. It's a separate discussion if FSP_USES_CB_STACK is such a good design choice to take. Also your plan to run AP stacks from same space does not help the semantics here.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35266 )
Change subject: _WIP_ drivers/intel/fsp2_0: Use generic defines for early storage ......................................................................
Patch Set 2:
Marshall, maybe abandon this and work/discuss on CB:35289 ?
Marshall Dawson has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35266 )
Change subject: _WIP_ drivers/intel/fsp2_0: Use generic defines for early storage ......................................................................
Abandoned
has outlived its usefulness - closest relative is https://review.coreboot.org/c/coreboot/+/36110/1
-
Kyösti Mälkki (Code Review) -
Marshall Dawson (Code Review)
-
Richard Spiegel (Code Review)