Patch Set 9:

Patch Set 9:

Patch Set 9:

Patch Set 9:

Patch Set 9:

Patch Set 9:

Patch Set 9:

Patch Set 9:

Patch Set 9: Code-Review-1
 
this is also used to cache postcar stage uitside cbmem

this config is mainly depends on relocatable ranstage config, not sure why we have created a config which has root dependency on ramstage but use across romstage, postcar and ramstage

postcarstage is always relocatable. so the depency is on either have_relocatable_ramstage or postcar_stage

then, i guess kconfig name should reflect the same. isn't it ?

Yes. How about renaming it to "config EXTERNAL_STAGE_CACHE" that depends on RELOCATABLE_RAMSTAGE || POSTCAR_STAGE ?

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?

There already is the Kconfig option NO_STAGE_CACHE which defaults to 'y' on !HAVE_ACPI_RESUME. The program loader functions already use that Kconfig option to determine whether to load stages into the cache. To deal with platforms that HAVE_ACPI_RESUME but no relocatable ramstage (but HUGE_ACPI_BACKUP) those weak functions exist. This is however something we ought to deprecate...

I think it is a good idea to reverse the logic of that Kconfig option and go with your suggestion.

Thanks Furquan for writing such detailed review. Yes, today coreboot stage_cache location is unnecessarily depends on certain stage (ramstage) which shouldn't be case.

i will try to make changes as below

1. Define 2 kconfig (A) EXTERNAL_STAGE_CACHE (B) INTERNAL/CBMEM_STAGE_CACHE which are dependent only on !NO_STAGE_CACHE and are mutually exclusive
2. Platform should able to select (A) or (B)
3. Modify .inc as applicable with those configs.
4. Move FSP stage_cache logic into common code

Furquan, one question, do you think (A) or (B) can be also optional? i mean platform can decide to skip stage_cache as well ?

If NO_STAGE_CACHE is true, then yes, there is no stage cache and so (A) and (B) don't make sense. But, if a platform does not select NO_STAGE_CACHE, it will need either (A) or (B).

View Change

To view, visit change 33116. To unsubscribe, or for help writing mail filters, visit settings.