Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33142 )
Change subject: Kconfig: Create coreboot separate stage kconfigs ......................................................................
Patch Set 2:
Patch Set 2:
but they can also indicate the presence and desire of a particular stage.
Well, they don't at the moment (e.g. in the verstage case). I'm trying to argue that it's cleaner and easier to keep it that way and handle presence of a stage separately.
There's 2 options on indicating positive selection of the present of the stages:
- Annotate all the soc/higher level Kconfig selections (most likely higher change churn than just arch).
- Update arch/ to select the presence of the stage. One-time change per arch. Adding new archs will be straight forward since they'll just copy what's there and not need to know about yet another Kconfig (previous versions of this patch did that).
They both need to deal with conditionally including and excluding things such as CONFIG_RAMPAYLOAD that may be selectable. So one might need to bite the bullet and update more, like in soc, along with some special helpers.
The soc/higher level Kconfigs are currently driving things as they are the root. I'm not sure why there would be unnecessary duplication.
Sorry, I don't really understand what you're saying. Which options do you mean by "soc/higher level Kconfig"? Do you mean when an SoC's Kconfig file says 'select ARCH_RAMSTAGE_X86'? I don't think we should touch that at all. I think it's fine if ARCH_RAMSTAGE_X86 is enabled even if you're building with RAMPAYLOAD. All that option does is make sure CC_ramstage is initialized to something. That doesn't hurt anybody even if no ramstage actually gets built.
[Subrata] Julius, I was kind of dealing with this same opinion for a while that its okay that I'm building coreboot with RAMPAYLOAD option enable so, i should be able to boot to payload from postcar (or what ever stage earlier) and let still coreboot build ramstage as it has ARCH_tool chain enable and ramstage is also default select, even ramstage also getting build and keeping as part of CBFS. But this has 2 problem
1. Ramstage is not a tiny stage, it still takes around 200KB of cbfs space (RO, RW-A/B, 200*3=600KB) 2. Adding to boot time, with RAMPAYLOAD is enable and having ramstage inside CBFS vs not inside CBFS is adding up to 100ms of boot time.
Thats the reason, i have started with experiment of coreboot should be able to build without ramstage. Later Ron and Aaron also share the thought process that coreboot should be that moduler so one can pick or drop certain stage if not required. But today i could see coreboot grown into some much of dependency with ramstage (good example of smm and ramstage)
I think the positive selection of the presence of a stage should only be decided by the feature Kconfig that may add/exclude that stage (e.g. SEPARATE_VERSTAGE or RAMPAYLOAD), and nothing else. The Kconfigs that select the toolchain should just be orthogonal to that. If you absolutely want to have a CONFIG_ENABLE_RAMSTAGE then I guess we can add that, but it should just be initialized with 'default y if !RAMPAYLOAD', and it shouldn't be intertwined with ARCH_RAMSTAGE_X86.