Aaron Durbin 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.
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.
Do you have a proposal for handling all the various stages and a way to select them in the positive as well as handling the user-configurable options? Just add kconfigs?
1. assert in positive *no* <STAGE> needed 2. assert in positive *yes* <STAGE> needed, but it depends on !*no* <STAGE>? And default of y?