Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36273 )
Change subject: lib/cbmem_top: Add a common cbmem_top implementation ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36273/13/src/lib/cbmem_top.c File src/lib/cbmem_top.c:
https://review.coreboot.org/c/coreboot/+/36273/13/src/lib/cbmem_top.c@22 PS13, Line 22: {
Sorry for nitpicking, but I think it's clearer if we do the following:
if (ENV_ROMSTAGE) { utilize cbmem_top_romstage() } else if (ENV_RAMSTAGE || ENV_POSTCAR) { if (CONFIG(RAMSTAGE_CBMEM_TOP_ARG) return _cbmem_top_ptr; }
return NULL;
The combinations of the logic you have below means we'd be calling out to cbmem_top_romstage() in ramstage under certain conditions which I think is wrong. Being explicit means we aren't implicitly creating such situations. Thoughts? That or cbmem_top_romstage() is not the correct name for this function.
Ok about making things more explicit. In a later patch series the logic is flipped with CONFIG_RAMSTAGE_CBMEM_TOP_ARG becoming CONFIG_RAMSTAGE_USE_CBMEM_TOP_ROMSTAGE. Is the name bad? My train of thought was that the non-default is to use the same implementation as in romstage. Although that is not necessarily the case if one decides to link other code... Should it be made explicit with a cbmem_top_ramstage callback?