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 12:
(5 comments)
Thanks! I think this is pretty close to what I'd expect now. Just few more nits so we can make the interface more clean:
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.h File src/drivers/vpd/vpd.h:
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.h@32 PS12, Line 32: void **ro_base, uint32_t *ro_size, : void **rw_base, uint32_t *rw_size If 'struct vpd_blob' is already declared also in header file, I think we should just replace this by vpd_blob., i.e,
void vpd_load_blob(struct vpd_blob *blob);
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.h@35 PS12, Line 35: /* : * returns the size of data in a VPD 2.0 formatted fmap region, or 0. : * Also sets *base as the region's base address. : */ : int32_t get_vpd_size(const char *fmap_name, int32_t *base); why do we need to publish this function?
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.h@45 PS12, Line 45: void **ro_base, uint32_t *ro_size, : void **rw_base, uint32_t *rw_size ... so vpd_get_buffers may use 'struct vpd_blob' as well.
If we can use static variable in pre-ram, this can even be
const struct vpd_blob *vpd_get_blobs(void);
And the caller can simply call it to figure out the buffer information.
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.c@52 PS12, Line 52: for consistency we want to keep <=80 columns.
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.c@89 PS12, Line 89: if (blob->initialized == false) : return; so who's going to update blob->initialized?