Hung-Te Lin 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:
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?
No. As said, I think CBMEM specific (RAMSTAGE_CBMEM_INIT_HOOK) should go vpd_cbmem.c. vpd_gets and vpd_find should have the action abstracted into a function, that is provided by vpd_cbmem.c and/or cbmem_preram.c. For example,
// vpd.c, vpd_gets
void *buffer = vpd_get_buffer(vpd_type, &size); ....
// vpd_cbmem.c void *vpd_get_buffer(enum type, size_t *size) { // read data from CBMEM ... }
// vpd_preram.c void *vpd_get_buffer(enum type, size_t *size) { // use static buffer, or re-read if needed. }