Attention is currently required from: Andrey Petrov, Eric Lai, Kapil Porwal, Ronak Kanabar, Tarun Tuli.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75899?usp=email )
Change subject: soc/intel/meteorlake: Use MRC version to fetch/store the MRC cache ......................................................................
Patch Set 4:
(3 comments)
This change is ready for review.
File src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h:
https://review.coreboot.org/c/coreboot/+/75899/comment/401a7336_c5f54169 : PS4, Line 116: "MRC VERSION"
tab
Acknowledged
https://review.coreboot.org/c/coreboot/+/75899/comment/3d1bd279_2cb3c12c : PS4, Line 116: "MRC VERSION"
tab
done
File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/75899/comment/76587420_467bbe76 : PS3, Line 265: version = fsp_mrc_version();
One of the requirements we set forth on this originally was to avoid this mandatory retrain scenario. I believe it is solveable.
e.g. (just summarizing the examples here) (1) Today a device has FSP Version (X) and MRC version (A) (2) Device is upgraded to the new version that has this feature, but the MRC version underlying is the same (3) Device was upgraded to FSP Version (Y) and MRC Version (A) (4a) Old logic would say X != Y so therefore invalidate the cache and retrain. Retraining would happen and we would store "Y" as the version. (4b) New logic would say X != A so therefore invalidate the cache and retrain. Retraining would happen and we would store "A" as the version.
I agree with you that one mrc training would happen with new logic as logic (bt that is in goodness of the future to incorporate this feature). But if you want to avoid that then i've some suggestion as below.
To resolve, is it worth in this case then adding a simple translation table that says if FSP Version == X then MRC Version = A for this transitional phase (would not be require for FSP Versions > X).
I don't want to add both the logic check going forward in FSP driver code in upstream because it is not practical that we have either of "fsp version" or "mrc version" in mrc_cache. My point is that we should know the platform and underlying version logic (between fsp_version or mrc_versopn) while storing the MRC_CACHE. if we don't know then we can lagging in validation. in reality we should have an idea about target platform and mrc_cache version is getting used.
For example: prior to MTL is using FSP version for mrc_cache and future platform will use mrc_version.
what you have suggested will work on existing device (as i have suggested earlier for example ADL/RPL) but for future platform this will be consider as W/A as we know for sure that MRC version is used when we select FSP_HEADER_HAS_MRC_VERSION_INFO config. What is the point of checking the fsp_version check in such a case?
I agree with you that we can avoid one additional mrc training while we land this feature on existing in-field device but I can suggest two approach
existing devices are branched out from mainline hence, we can add the suggested check inside branched out repository to avoid both the fsp version or mrc version check.
devices are in development i.e., in coreboot upstream, we don't keep such ambiguous checks between FSP version or MRC version rather use clear logic using FSP_HEADER_HAS_MRC_VERSION_INFO config.
I have one more idea, let me make some modifications in this cl and I hope i could be able to address ur concern more naturally.
As I have started thinking about an alternative approach to mitigate the req that you have mentioned.
I'm wondering how we would be able to know the MRC version in the existing method that you have suggested here: ```To resolve, is it worth in this case then adding a simple translation table that says if FSP Version == X then MRC Version = A for this transitional phase (would not be require for FSP Versions > X).``` as the only source of truth for mrc_cache for us so far is FSP version and we don't maintain a mapping table between FSP version <-> mrc version hence, we won;t be able to mention what is the MRC version packaged inside FSP version (say X in your example). wondering how do u know the mrc version is `A` in current scenario.
I agree that in the future we will be able to know the MRC version more easily but how do you think we will be able to first incorporate this feature to understand the mapping between FSP version to MRC version? Because in your request unless we know the mrc version as well prior calling into FSP-M, we won't be able to fully incorporate the requirement that you have (avoid one additional retraining).
Example:
Assume we are landing new FSP (say Y) that incorporate the fix to know the MRC version prior calling into the FSP-M
coreboot read that MRC version from #1. (say A)
coreboot can also read the FSP version from header (say Y)
coreboot wish to check if the stored MRC cache version is matching with #1
#4 will fail as the existing MRC cache is stored against previous FSP version (say X)
As per your suggestion, you are asking to check the MRC version associated with previous FSP version (X) to know if FSP had the same MRC cache (A)? This would avoid the retraining and use the existing cache undoubtedly.
My question is how do you know what was the MRC version associated with the existing FSP version (X)? There is one way to know that based on parsing the FVI hob but that happens during ramstage but we wished to know this information early inside romstage (even prior calling into FSP-M).
Are you suggesting to check in (submit a CL first to introduce the mapping) some mapping structure between FSP version <-> mrc version inside coreboot code which can be used to retrieve the mrc version based on input FSP version (to meet the #6 req?). Isn't this method going to be some kind of brute force method and not sure if we can introduce the mapping table in coreboot because the Intel FSP version and MRC version are restricted pieces of the information which we might not be exposing unless the license agreement says so.
We had offline sync and agreed to take a two step approach between devices that are in active development (for example: MTL,. where Intel can share the FSP fix to implement this feature) vs shipped devices (for example: ADL, RPL etc. need to work with the intel team to know possible FSP version with this feature being implemented).
For now, we will focus on MTL platform where the feature is available coreboot changes are only required to enable this feature. for previous gen platforms, we will wait to hear back from Intel and then take action. Mostly likely changes to get fixes in previous FSP platform is hard and it might need more tweaking hence, better to relying on the specific FW branch to incorporate such changes rather adding the one time W/A in upstream coreboot.