Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34636 )
Change subject: mb/ocp/monolake: use RW_VPD to configure FSP UPD ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34636/13/src/mainboard/ocp/monolake... File src/mainboard/ocp/monolake/romstage.c:
https://review.coreboot.org/c/coreboot/+/34636/13/src/mainboard/ocp/monolake... PS13, Line 78: void *rw_vpd_addr = NULL; : size_t rw_vpd_size = -1; : UPD_DATA_REGION *UpdData = FspRtBuffer->Common.UpdDataRgnPtr; : : /* : * 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)) { : rw_vpd_size = get_vpd_size("RW_VPD", rw_vpd_addr); : if (rw_vpd_size == 0) : return; : : board_configure_upd(rw_vpd_addr, rw_vpd_size, UpdData); : printk(FSP_INFO_LEVEL, : "Found and Processed VPD binary blob in RW_VPD.\n"); : } I feel there's too much overhead here. Why not just put everything in romstage file with just 2 functions? (this is also why I think we should simplify the VPD APIs just like how CBMEM version works today):
bool vpd_get_bool(const char *name, vpd_region, bool default);
static void board_configure_upd() { UPD_DATA_REGION *UpdData = FspRtBuffer->Common.UpdDataRgnPtr; if (vpd_get_bool(FSP_VAR_HYPERTHREADING, VPD_RW, False) UpdData->HyperThreading = val; }
...
if (CONFIG(VPD)) board_configure_upd();