Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add new ENV_xxx definitions ......................................................................
Patch Set 10:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34939/10/src/arch/x86/include/arch/... File src/arch/x86/include/arch/early_variables.h:
https://review.coreboot.org/c/coreboot/+/34939/10/src/arch/x86/include/arch/... PS10, Line 23: /* Just defined locally to avoid rewriting this file. */ That... seems like an odd reason? Why not change it right away?
https://review.coreboot.org/c/coreboot/+/34939/10/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/34939/10/src/include/rules.h@269 PS10, Line 269: #define ENV_ROMSTAGE_OR_BEFORE \ I guess this works for all architectures, but I still think it sounds very clunky. Why not ENV_PRE_DRAM (or flip to ENV_PROGRAM_IN_DRAM)?
It also means that this will be true for the romstage in DRAM for pistachio. Is that actually what we want? I think in most cases you really want to know whether you're actually running from DRAM (e.g. below you have to explicitly OR in RESET_VECTOR_IN_DRAM because that's not reflected with this macro).
https://review.coreboot.org/c/coreboot/+/34939/10/src/include/rules.h@276 PS10, Line 276: !CONFIG(ARCH_X86) When I read "in RAM", I'm thinking DRAM. If you want this to include SRAM execution, how about flipping it around and calling it ENV_PROGRAM_IN_CAR? (Then again that sounds pretty close to ENV_CACHE_AS_RAM and I'm not sure why you're trying to get rid of that anyway? I think that was a good name...)
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@269 PS5, Line 269: #define ENV_BEFORE_POSTCAR \
Well.. […]
I think for most of us working on Arm, when we say "RAM" we mean DRAM. There's still sometimes a need to check whether we're in an SRAM or DRAM stage, and we used __PRE_RAM__ for that before.
https://review.coreboot.org/c/coreboot/+/34939/10/src/include/symbols.h File src/include/symbols.h:
https://review.coreboot.org/c/coreboot/+/34939/10/src/include/symbols.h@75 PS10, Line 75: return !CONFIG(ARCH_X86) || ENV_ROMSTAGE_OR_BEFORE; I don't think(?) this should be true for pistachio romstage? (Because it's defined in car.ld and I assume they're not using that?) Maybe another reason to have a macro that's actually about whether we're linked into DRAM instead?