Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34913 )
Change subject: rules: Add ability to disable ENV_CACHE_AS_RAM ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2:
This should probably be in src/arch/x86/Kconfig and be called X86_NO_CACHE_AS_RAM or something like that, since non-x86 devices also don't use cache-as-RAM but they're not going to select this. On the other hand, there's already CONFIG_CACHE_AS_RAM... why don't you just use that instead?
CB:34806 intentionally removed it. Per the original use, CACHE_AS_RAM=n would have indicated romstage has to be built with romcc. Not a good idea to completely flip the definition, technically it would have worked.
(Also, honestly, this whole thing looks to me again like hacks upon hacks to support a weird design you probably shouldn't have picked in the first place. I assume you need this because your "romstage in DRAM" still has __PRE_RAM__ defined... but that doesn't make any sense in itself already! __PRE_RAM__ literally means "this is not running in RAM", but your "romstage" *is* running in RAM. All over coreboot people have made assumptions about what this means that you are breaking. I guess you could hack up the Makefile instead to remove the -D__PRE_RAM__ from romstage for your SoC, but then you'd break a ton of other assumptions all over coreboot that romstage always runs with __PRE_RAM__. I think the only way you could really get this to not be a huge mess is by not using the romstage for something it wasn't meant to be, and instead putting your stuff into a custom ramstage entry point, or maybe an expansion of the postcar concept, or even another new stage specific to your SoC.)
I don't think the situation is that bad with __PRE_RAM__. I will bulk replace most of "#ifndef __PRE_RAM__" with "#if ENV_RAMSTAGE". Sometimes it may need to be "ENV_RAMSTAGE || ENV_SMM". I am hoping to see use of __PRE_RAM__ outside <rules.h> can be totally avoided.
https://review.coreboot.org/c/coreboot/+/34913/2/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/34913/2/src/include/rules.h@287 PS2, Line 287: #define ENV_CACHE_AS_RAM (CONFIG(ARCH_X86) && !CONFIG(NO_ENV_CACHE_AS_RAM)) I would have made it look like this.
CONFIG(ARCH_X86) && !CONFIG(RESET_VECTOR_IN_DRAM)
In my approach, this RESET_VECTOR_IN_DRAM would also conditionalise uses of x86/car.ld in x86/memlayout.ld.
Looks like preram_symbols_available() in symbols.h needs a fix too.