Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 8:
Patch Set 8:
(3 comments)
This is much better now, but I still think it'll be even better if we can have vpd_gets and vpd_getbool in same usage for both RAM stage and ROM stage. They can then live in a shared (maybe vpd_common) place.
In fact, from your current implementation I wonder if it'll make more sense to have
vpd.c // common utils, which based on a function like vpd_load_blob(type) vpd_cbmem.c // fetch and build cache in CBMEM, providing vpd_load_blob from CBMEM vpd_romstage.c, or vpd_nocache.c // providing vpd_load_blob directly
Thanks Hung-Te. I will take care of the 3 specific comments, and I am working on a version using CAR_GLOBAL to store blobs' base address and size.
On the other hand, my understanding of your general comments is that you are thinking about a different structure from the current design. With your design, vpd.c is linked to both stages, and provides library functions to both stages. In this case, build time #ifdef ENV_ROMSTAGE (or ENV_RAMSTAGE) is needed, isn't it? For example, RAMSTAGE_CBMEM_INIT_HOOK statement should only be built into ramstage; in vpd_gets() and vpd_find(), some different actions are needed for ramstage and for romstage. Is this preferred?