Attention is currently required from: Andrey Petrov, Kapil Porwal, Ronak Kanabar, Subrata Banik.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75921?usp=email )
Change subject: {commonlib/drivers}: Have option to store MRC version inside CBMEM ......................................................................
Patch Set 3:
(5 comments)
File src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h:
https://review.coreboot.org/c/coreboot/+/75921/comment/fe4fc6ce_848706c7 : PS3, Line 116: CBMEM_ID_MRC_VERSION
Why is a new TAG needed?
This tag is need to pass the information between romstage and ramstage. as in romstage creates the data and ramstage consumers it. Hence, ramstage uses either the MRC version or FSP-M version (depending on the config) when updating the MRC cache.
ok
File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/75921/comment/8cc79597_02126a92 : PS3, Line 61: CBMEM_ID_FSPM_VERSION maybe: CONFIG(MRC_CACHE_USING_MRC_VERSION) ? CBMEM_ID_MRC_VERSION : CBMEM_ID_FSPM_VERSION ?
https://review.coreboot.org/c/coreboot/+/75921/comment/9ab4f2c7_a3f25f2b : PS3, Line 64: FSP-M maybe: %s and CONFIG(MRC_CACHE_USING_MRC_VERSION) ? "MRC" : "FSP-M".
https://review.coreboot.org/c/coreboot/+/75921/comment/064cf382_8539d750 : PS3, Line 43: Failed to add cbmem entry id (%d).\n", cbmem_id); the error message become less readable, now it prints a number rather than saying what is wrong.
https://review.coreboot.org/c/coreboot/+/75921/comment/65c19b5e_0fad5b1a : PS3, Line 75: if (CONFIG(MRC_CACHE_USING_MRC_VERSION)) : do_cbmem_version_entry(CBMEM_ID_MRC_VERSION, version); : else : do_cbmem_version_entry(CBMEM_ID_FSPM_VERSION, version);
Given that the creation of MRC and FSPM version cbmem is mutually exclusive. Does it even make sense to have a different ID? Can't you rename the existing one to CBMEM_ID_FSPM_OR_MRC_VERSION?
its meaningful to have different IDs as they are associated with different string
Ok