Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for running romstage in DRAM ......................................................................
Patch Set 6:
(3 comments)
I feel this messes up the 'big picture' of CAR and romstage. Can't you just use custom linker script in place of car.ld?
You already have several TODOs left behind like bootblock-y += cache_as_ram.S. Last time someone approved this from AMD contractors, it took 5 years or more to have them addressed. And it was not AMD contractor who sorted it out.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 36: _romstage_in_ram_continue: Comment says it's a jump. So you could just define the label without any preprocessor stuff here?
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 49: /* clear CAR_GLOBAL area as it is not shared */ None of this CAR is really correct when you already have DRAM...
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/memlayout.ld@6... PS6, Line 61: #include <soc/romstage.ld> To include car.ld for this concept seems invalid to me. Also, we have ENV_CACHE_AS_RAM that should be adjusted accordingly and you should have CACHE_AS_RAM=n in your .config.