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 1:
(4 comments)
Patch Set 1:
What is this patch actually solving (at a high level)? I feel like it's just adding some more vines to the config jungle for no real benefit other than for people to trip over. Don't we already know which stages we have from other options? In general, coreboot always has a bootblock, a romstage and a ramstage. It has a verstage if CONFIG_SEPARATE_VERSTAGE is set. With your new feature, it does not have a ramstage if the main Kconfig for that feature is set (is that CONFIG_RAMPAYLOAD? I didn't keep track...).
So if you want something you can depend ramstage-specific stuff on, can't you just do 'depends on !RAMPAYLOAD'? Why does it need a separate option that's tied so weirdly into the architecture Kconfigs? Sure, if we eventually have 4 different cases where coreboot somehow doesn't have a ramstage it might make sense to factor those all out into a common option, but I don't really see that happening anytime soon?
[Subrata] coreboot somehow doesn't have a ramstage it might make sense to factor those all out into a common option, this is the whole purpose of this patch tree to build and boot platform without ramstage
https://review.coreboot.org/#/c/33142/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33142/1/src/Kconfig@21 PS1, Line 21: config STAGE_BOOTBLOCK
This, to me anyway, is confusing. […]
Ack
https://review.coreboot.org/#/c/33142/1/src/Kconfig@21 PS1, Line 21: config STAGE_BOOTBLOCK
Can we add ENABLE or PRESENT to these names? I feel like they read better when used.
Ack
https://review.coreboot.org/#/c/33142/1/src/Kconfig@23 PS1, Line 23: default y if ARCH_BOOTBLOCK_PPC64 || ARCH_BOOTBLOCK_ARMV8_64 || ARCH_BOOTBLOCK_ARM64 || ARCH_BOOTBLOCK_ARMV7 || ARCH_BOOTBLOCK_ARMV7_M || ARCH_BOOTBLOCK_ARMV7_R || ARCH_BOOTBLOCK_ARM || ARCH_BOOTBLOCK_ARMV4 || ARCH_BOOTBLOCK_RISCV || ARCH_BOOTBLOCK_X86_32 || ARCH_BOOTBLOCK_X86_64 || ARCH_BOOTBLOCK_MIPS
Would it be better to select this config from these Kconfig locations and have this one default to […]
Done
https://review.coreboot.org/#/c/33142/1/src/Kconfig@23 PS1, Line 23: default y if
Because of where these are at the top of the the Kconfig tree, individual platform or chips can't up […]
Ack