[coreboot-gerrit] Change in coreboot[master]: drivers/vpd: add framework to search VPD in romstage

[Name of user not set (Code Review)]

[e-mail] 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 5:
> Patch Set 5:
> > > If needed we may even create vpd_preram or vpd_romstage for that.
>
> When I say this I was referring to vpd_preram.c or vpd_romstage.c, not functions.

Got it. this is what I did in patch set 5.
> > a. create a vpd_romstage_gets() and vpd_romstage_find() call interface for romstage callers.
>
> Probably no. I think your current version made it harder for people to figure out the right calling sequence.

Not sure why patch set 5 made it harder for people to figure out the right calling sequence. Do you mean the caller who uses the vpd_romstage functions? Or do you mean people who develop vpd_romstage functions and vpd_common functions? With patch set 5, the calling sequencer is the same as that of vpd.
> We can have stage name in file level if needed, but better not in function name.

I would think having different function names helps code read-ability. But if you desire, I will change.
> It's better to keep similar interfaces, but if needed we may still add extra param. I'd recommend
> Change vpd_cbmem to vpd_cache
> Change cbmem_add_cros_vpd to vpd_load_cache

I wonder if this is an over kill. In romstage, VPD data is accessed directly through memory mapped flash. All is needed for accessing is the base address and size. I am not sure if we want to add a vpd_cache structure to hold such info.
> inside vpd_load_cache, have a way to decide allocating from CBMEM, or a static variable (using malloc).

vpd_load_cache() is only for romstage, isn't it? In this case, it will not allocate from CBMEM.
> In this way we can get almost everything unchanged and keep the only minimal difference in single place.

With patch set 5, the vpd.c is quite small. The majority code is to construct CB_MEM. Very light logic is in vpd.c for run time operations. Do we need to further reduce the logic?

Thanks for the review!