Jonathan Zhang 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:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34636/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34636/13//COMMIT_MSG@9 PS13, Line 9: Summary: : This patch adds: : * A framework to use VPD binary blob 2.0 data to configure : FSP UPD. : * A library to configure HyperThreading FSP UPD variable. :
these should be removed
Ack
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. […]
This patchset adds a framework and an example to use the framework. More UPD processing are expected to be added in future. The thinking is: * functions such as vpd_get_bool() are provided by vpd driver. * functions such as set_upd_hyper_threading() are provided by SOC/FSP layer. * board chooses what UPD variables can be customized for the board, in board_configure_upd(). Today there is only one board (monolake), and thus this patchset does not touch SOC/FSP layer, but it can be done in future.
https://review.coreboot.org/c/coreboot/+/34636/13/src/mainboard/ocp/monolake... File src/mainboard/ocp/monolake/vpd_fsp.h:
https://review.coreboot.org/c/coreboot/+/34636/13/src/mainboard/ocp/monolake... PS13, Line 20: HyperThreading
FYI in Chrome OS we usually prefer to name VPD values in lower_case_with_underline. […]
In FSP UPD variable definition, CamelCase is used. The VPD key/value pairs are added by ODM engineers (before shipping) and by board owner (at run time). For the sake of avoiding confusion, it is preferred to keep the exact string with CamelCase if any as the VPD key name.