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.