Patch Set 18:
Patch Set 18: Code-Review-2
That variable "RELOCATABLE_RAMSTAGE" has some amount of conceptional significance over the stage caches. We have also used it as criteria for board removals in past discussion on the mailing list.
I am too much unhappy about the Kconfig just disappering like this, thus my -2 here now, until we have some more reviewer opinions. At least, can you keep the Kconfig as a placeholder and keep all the existing cases of CONFIG(RELOCATABLE_RAMSTAGE) unmodified?
yes, i can keep RELOCATABLE_RAMSTAGE KConfig as is but if i don't modify .inc and .c then what this CL will do.
Please review with an opinion what we like to achieve and share feedback if you think its not meeting your requirement.
I believe review comment has enough description (from Furquan, Arthur) about why we started cleaning this stage_cache logic. Today entire stage_cache logic is tied with RAMSTAGE which shouldn't be the case as i understand, stage_cache is something well beyond ramstage.
And kconfig is not just disappearing its been replaced with some code which might not meet your requirement, i want to understand that.
For simplicity, here is feedback from Furquan
Stage cache is used by 3 components:
1. Refcode --> Added/used by romstage and/or ramstage
2. Ramstage if it is relocatable --> Added/used by romstage or postcar (if it exists)
3. Postcar --> Added/used by romstage
Originally, stage cache started out with #1 and #2, but then it was added for postcar as well. Stage cache can exist in two locations:
1. External to CBMEM -- Requires SoC/mainboard to provide an area for this. I believe most SoCs put this in SMM sub-region
2. Internal to CBMEM -- Provided by cbmem_stage_cache.c
If stage cache exists: Out of the above 3 components, refcode and postcar always are added to stage cache (either internal or external). However, ramstage is added to stage cache (internal/external) dependent upon the RELOCATABLE_RAMSTAGE config.
However, the config option for relocatable ramstage seems to have been incorrectly applied to more than ramstage. In addition to that, it looks like FSP is providing the implementation of stage_cache_external_region which is nothing to do with FSP (I believe it got moved there because FSP was adding refcode to stage cache?).
All of this can be untangled by doing something like:
1. Define configs for location of stage cache i.e. EXTERNAL_STAGE_CACHE and CBMEM_STAGE_CACHE which are dependent only on STAGE_CACHE config and are mutually exclusive.
2. Since stage cache support is a property of the platform, it doesn't have to be dependent on things like relocatable ramstage i.e. Platform can have stage cache support (internal / external) independent of other properties. Thus, add appropriate stage_cache files depending upon EXTERNAL_STAGE_CACHE/CBMEM_STAGE_CACHE to romstage, postcar and ramstage. (Basically any stage that exists).
3. Romstage and ramstage can add refcode and postcar to stage cache dependent on the selection of STAGE_CACHE. stage_cache_* APIs will take care of adding those to the right location. Probably we can get rid of the weak functions for stage_cache_*.
4. Romstage or Postcar (if present) will add ramstage to stage cache only if RELOCATABLE_RAMSTAGE is selected and STAGE_CACHE is enabled by the platform. Location of stage cache will be taken care of by stage_cache_* APIs.
5. Finally stage_cache implementation in FSP should be moved to intel/common/block/ since it has nothing to do with the FSP itself.
Thoughts?
To view, visit change 33116. To unsubscribe, or for help writing mail filters, visit settings.