Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34501 )
Change subject: Framework to get settings from RW_VPD to configure FSP UPD ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34501/4/src/drivers/vpd/vpd_fsp.c File src/drivers/vpd/vpd_fsp.c:
https://review.coreboot.org/c/coreboot/+/34501/4/src/drivers/vpd/vpd_fsp.c@5... PS4, Line 59: set_upd_hyper_threading The "HyperThreading" string is duplicated 3 times - 2 in this module and 1 in the fsp file. I think that can be simplified a lot.
https://review.coreboot.org/c/coreboot/+/34501/4/src/drivers/vpd/vpd_fsp.c@1... PS4, Line 115: vpd_iterate this should be replaced by directly calling upstream vpd_decode_string
https://review.coreboot.org/c/coreboot/+/34501/4/src/drivers/vpd/vpd_fsp.c@1... PS4, Line 135: decodeLen there's no decodeLen anymore
https://review.coreboot.org/c/coreboot/+/34501/4/src/soc/intel/fsp_broadwell... File src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c:
https://review.coreboot.org/c/coreboot/+/34501/4/src/soc/intel/fsp_broadwell... PS4, Line 45: board specific FSP UPD customized function. board means src/mainboard. static functions can't be customized by that.
I think a more correct implementation is to make this a weak empty one, that in your src/mainboard, do all the VPD query, and then set right FSP UPD data.
https://review.coreboot.org/c/coreboot/+/34501/4/src/soc/intel/fsp_broadwell... PS4, Line 150: /* : * If RW_VPD VPD partition exists, search key/value pairs : * to see if there are relevant FSP UPD variable setting(s). : * If so, use such setting(s) to customize FSP behavior. : */ : if (CONFIG(VPD)) { : if (!fmap_locate_area_as_rdev("RW_VPD", &rdev)) { : rdev_chain(&rdev, &rdev, GOOGLE_VPD_2_0_OFFSET, : region_device_sz(&rdev) - GOOGLE_VPD_2_0_OFFSET); : rw_vpd_addr = rdev_mmap_full(&rdev); : if (rw_vpd_addr != NULL) { : rw_vpd_size = region_device_sz(&rdev); : /* Skip the VPD info header */ : rw_vpd_addr += sizeof(struct google_vpd_info); : rw_vpd_size -= sizeof(struct google_vpd_info); : /* : * vpd_iterate is called to process key/value pairs in : * RW_VPD iteratively. In such processing, board specific : * callback function board_fsp_configure_upd_data is called. : the whole stuff should be board-specific (src/mainboard) I believe, since - per SOC level, it should not have hard-coded VPD section name - per SOC level, it should not depend on libVPD, struct google_vpd_info etc