Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34664 )
Change subject: lib/stage_cache: Refactor Kconfig options ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34664/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34664/2//COMMIT_MSG@12 PS2, Line 12: Platforms with SMM_TSEG=y always need to implement : stage_cache_external_region(). It is allowed to return with a : region of size 0 to effectively disable the cache.
It just happens to be that all platforms with SMM_TSEG also implement stage_cache_external_region() […]
I believe amd/stoneyridge has a hard requirement to have stage_cache for S3 since it uses stage_cache_add_raw() to maintain some platform configuration (volatile storage in S3_DATA_BLOCK). It would be the same with AGESA but it currently uses CBMEM exclusively there, instead of (just slightly more secure) TSEG.
I find the requirement to implement TSEG_STAGE_CACHE support function lesser maintenance burden than adding yet another Kconfig around TSEG to tell explicitly that it is supported.
Besides, without a hard requirement (like with _add_raw() mentioned above), platforms can also 'select NO_STAGE_CACHE'.
BTW: I expect these older intel platforms to evolve to use smm_subregion(SMM_SUBREGION_CACHE,xx) that appears to be the current API for slicing the TSEG. This can obsolete stage_cache_external_region() function entirely.
https://review.coreboot.org/c/coreboot/+/34664/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/34664/1/src/Kconfig@269 PS1, Line 269: romstage
not true since cbmem is initialized in romstage?
Done