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 15:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.h File src/drivers/vpd/vpd.h:
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.h@26 PS15, Line 26: extern struct vpd_blob g_vpd_blob; : : /* : * This function loads g_vpd_blob CAR_GLOBAL variable. : * The variable is initialized if it was not. : */ : const struct vpd_blob *vpd_load_blob(void); : : /* : * This function gets the base address and size of : * buffers for RO_VPD/RW_VPD binary blobs, and sets : * the struct. : */ : void vpd_get_buffers(struct vpd_blob *blob); Thanks for all the effort here. I think we're pretty close!
From my last comment in c#12:
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.
This seems not really used in the end. Is there a reason for that?
I'm not super familiar with CAR (it's actually dummy for ARM) so maybe you can help me to justify why CAR is needed:
1. Does the CAR_GLOBAL help to share same vpd_blob data between bootblock/romstage / even ram stage? (i.e., so we don't need to read and initialize twice?
2. Callers should use vpd_get_buffers, so I'm not sure we need to put g_vpd_blob as public/extern? Shouldn't it be a provider (vpd_get_buffer) specific info?
I think a more common design style is to have the interface in public and details (and variable) in the individual module file, i.e:
// vpd.h -- struct vpd_blob { ... }; /* load the blob from SPI directly into, only used internally by vpd driver */ int vpd_load_blob(struct vpd_blob *blob); /* finds cached (and load if haven't) VPD blobs */ const struct *vpd_blob vpd_get_buffers(void);
// vpd_preram static struct vpd_blob vpd_blob;
const struct *vpd_blob vpd_get_buffers(void) { if (!vpd_blob.initialized) vpd_load_blob(&vpd_blob); return &vpd_blob; }
// vpd_cbmem static struct *vpd_blob vpd_blob;
const struct *vpd_blob vpd_get_buffers(void) { if (!vpd_blob.initialized) { // find CBMEM // fill vpd_blob if CBMEM is available // set initialized } return &vpd_blob; }
If there're reasons we can't do this and must rely on CAR please feel free to let me know (and put that in the comments or description). Thanks!
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.c@106 PS15, Line 106: - remove the extra space
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.c@201 PS15, Line 201: size add a "assert(size > 0)" before calling the MIN.
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd_cbmem.... File src/drivers/vpd/vpd_cbmem.c:
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd_cbmem.... PS15, Line 83: blob should we set blob->initialized to True ?