Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 17:
(10 comments)
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@244 PS17, Line 244: default !NO_RELOCATABLE_STAGES This equates STAGE_CACHE with RELOCATABLE_STAGES/RELOCATABLE_RAMSTAGE. Is that really a good assumption to make? I understand that it is true at this point for the current platforms. But what about this: If a platform does not support relocatable ramstage, but still wants to use stage cache to stash refcode/S3 data for example, would that be a valid use case?
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@248 PS17, Line 248: ramstage to be built as a : relocatable module Is it right to mention ramstage here since you seem to be making it more of a generic relocatable stages option now?
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@258 PS17, Line 258: . It would be good to mention that the platform is responsible for providing storage for this external stage cache.
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@262 PS17, Line 262: select RELOCATABLE_MODULES RELOCATABLE_MODULES can be set to true based on !NO_RELOCATABLE_STAGES? i.e. EXTERNAL_STAGE_CACHE or CBMEM_STAGE_CACHE don't need to select them, right?
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@264 PS17, Line 264: enable enables
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@266 PS17, Line 266: chiset chipset
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@272 PS17, Line 272: enable enables
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@279 PS17, Line 279: NO_RELOCATABLE_STAGES || This condition does not look completely right to me. I believe it should just be based on !HAVE_ACPI_RESUME. Same comment as above for STAGE_CACHE.
https://review.coreboot.org/c/coreboot/+/33116/17/src/arch/x86/Makefile.inc File src/arch/x86/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33116/17/src/arch/x86/Makefile.inc@... PS17, Line 378: CONFIG_STAGE_CACHE It just feels a bit twisted that ramstage being a rmodule is dependent on availability of stage cache. Should we still retain CONFIG_RELOCATABLE_RAMSTAGE?
https://review.coreboot.org/c/coreboot/+/33116/17/src/include/stage_cache.h File src/include/stage_cache.h:
https://review.coreboot.org/c/coreboot/+/33116/17/src/include/stage_cache.h@... PS17, Line 40: CONFIG_NO_STAGE_CACHE This should be updated as well.