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 11:
(20 comments)
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.h File src/drivers/vpd/vpd.h:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.h@7 PS10, Line 7: #include "vpd_lib.h"
This should go below the header guard
Ack
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.c@107 PS10, Line 107: void
why not just do 'const char *value' here so we don't need to re-cast later?
Done
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.c@111 PS10, Line 111: return
returning
Done
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.c@111 PS10, Line 111: vpd_find_romstage
vpd_find
Done
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.c@119 PS10, Line 119: char
const char
Done
https://review.coreboot.org/c/coreboot/+/34634/6/src/drivers/vpd/vpd_common.... File src/drivers/vpd/vpd_common.c:
https://review.coreboot.org/c/coreboot/+/34634/6/src/drivers/vpd/vpd_common.... PS6, Line 68: const uint8_t *value, int32_t value_len, void *arg)
please, no spaces at the start of a line
Ack
https://review.coreboot.org/c/coreboot/+/34634/2/src/drivers/vpd/vpd_fsp.c File src/drivers/vpd/vpd_fsp.c:
https://review.coreboot.org/c/coreboot/+/34634/2/src/drivers/vpd/vpd_fsp.c@2... PS2, Line 26: || value_len != 1
In case of binary type variable, it is good to check the value length; this will find out situation […]
Ack
https://review.coreboot.org/c/coreboot/+/34634/2/src/drivers/vpd/vpd_fsp.c@4... PS2, Line 42: else : return false; : }
Done
Ack
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_lib.c File src/drivers/vpd/vpd_lib.c:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_lib.c@... PS10, Line 8: * : * SPDX-License-Identifier:.*GPL-2.0-or-later
We're not using SPDX identifiers for coreboot. […]
Ack
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_lib.c@... PS10, Line 23: get_vpd_size
can we just move this to vpd. […]
Ack
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_lib.c@... PS10, Line 67: printk(BIOS_CRIT, "binary blob size:%d\n", size);
Seems like a debug print that got left in by accident?
Ack
https://review.coreboot.org/c/coreboot/+/34634/9/src/drivers/vpd/vpd_premem.... File src/drivers/vpd/vpd_premem.c:
https://review.coreboot.org/c/coreboot/+/34634/9/src/drivers/vpd/vpd_premem.... PS9, Line 57: blob->ro_base = (uint8_t *)(sizeof(struct google_vpd_info) + rdev_mmap_full(&vpd));
line over 96 characters
Ack
https://review.coreboot.org/c/coreboot/+/34634/9/src/drivers/vpd/vpd_premem.... PS9, Line 68: blob->rw_base = (uint8_t *)(sizeof(struct google_vpd_info) + rdev_mmap_full(&vpd));
line over 96 characters
Ack
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... File src/drivers/vpd/vpd_premem.c:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 35: no VPD at all? nothing to do then
Return if no VPD at all.
Done
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 36: (ro_vpd_size == 0) && (rw_vpd_size == 0)
if (a == b && c == d) […]
Done
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 42: blob->ro_base = NULL; : blob->ro_size = -1; : blob->rw_base = NULL; : blob->rw_size = -1;
Since these aren't actually used (only assigned), you can remove these lines. […]
Thanks David. These statements initialize global static variable, so it is better to have them. Now ro_size and rw_size are initialized to 0 instead of -1, to be consistent with vpd_cbmem.c. Good point.
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 49: if (ro_vpd_size != 0) {
if (ro_vpd_size) {
Ack
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 57: sizeof(struct google_vpd_info) + : rdev_mmap_full(&vpd)
not a big deal, but we usually write (ptr + number) instead of (number + ptr).
Done
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 61: if (rw_vpd_size != 0) {
if (rw_vpd_size) {
Done
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 78: struct vpd_blob *blob = car_get_var_ptr(&g_vpd_blob); : if (!blob) : return;
if we're already using car, why not just make calling vpd_load_blob automatically inside vpd_get_buf […]
vpd_load_blob() needs to be called only once during boot time, similar to "RAMSTAGE_CBMEM_INIT_HOOK(cbmem_add_cros_vpd)".