Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add new ENV_xxx definitions ......................................................................
Patch Set 5:
(8 comments)
https://review.coreboot.org/c/coreboot/+/34939/5/src/cpu/Kconfig File src/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/34939/5/src/cpu/Kconfig@19 PS5, Line 19: default n
Did this sneak in here? Add some comments on usage and meaning?
This was anticipation for CB:32414 without pulling that commit into my branch.
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 \
If this is supposed to be the new generic __PRE_RAM__, I'd prefer a more generic name like ENV_PRE_R […]
Well.. anything PRE_RAM was already wrong for ARMs where we are in SRAM/DRAM starting from bootblock already. And with case of Picasso, ARCH_X86 && RESET_VECTOR_IN_RAM, anything PRE_RAM becomes obscure as well. I think we have ~15 cases of __PRE_RAM__ use that actually need solving, all the rest is just noise (spurious guards).
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@273 PS5, Line 273: (ENV_BEFORE_POSTCAR || ENV_POSTCAR) This can be defined listign PRE_RAM stages explicitly, to avoid ENV_BEFORE_POSTCAR.
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@278 PS5, Line 278: #define ENV_HAS_DATA_SECTION ENV_PROGRAM_IN_RAM
Yes, non-x86 stages always have a .data section and ARCH_STAGE_HAS... […]
I'll try to update this, leveraging existing defines from memlayout.h.
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@298 PS5, Line 298: #define ENV_CACHE_AS_RAM !ENV_PROGRAM_IN_RAM
Do we even need two things that are just exact opposites of another? I think in that case, having on […]
Not sure, maybe not. All I know is that with __PRE_RAM__, use of !__PRE_RAM__ became obscure once we added __SMM__ and ENV_POSTCAR. I was about suggest to not allow the testing with a negation.
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/stddef.h File src/include/stddef.h:
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/stddef.h@48 PS5, Line 48: #define MAYBE_BSS
I think this doesn't really reflect what it means anymore, especially if someone were to stumble acr […]
I would do the renaming in separate commit. Either before or after this one.
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/stddef.h@52 PS5, Line 52: #define ALLOC_LARGE_BUFFER MAYBE_BSS
I don't think we should hide this in another macro. […]
Aaron should comment, his suggestion.
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/symbols.h File src/include/symbols.h:
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/symbols.h@75 PS5, Line 75: return !CONFIG(ARCH_X86) || ENV_BEFORE_POSTCAR;
So... […]
Test is _BEFORE_ postcar. This is for the RESET_VECTOR_IN_RAM case from CB:32414, were we would never have ENV_CACHE_AS_RAM==1.