jonzhang@fb.com 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 10:
(18 comments)
Patch Set 10:
I need some guidance here. This patch breaks some boards. If USE_FSP_1_0 is defined, the code looks for HyperThreading member in "struct _UPD_DATA_REGION". There are 10 boards either does not have such member in the struct, or does not have fsp.h defined. https://qa.coreboot.org/job/coreboot-gerrit/99551/.
Since my interest is OCP MonoLake board, I could make the framework effective for this board only; however, if possible it is better to have the framework as a library (eg. in src/drivers/vpd).
Thanks, Jonathan
https://review.coreboot.org/c/coreboot/+/34501/1/src/drivers/vpd/vpd_decode.... File src/drivers/vpd/vpd_decode.c:
https://review.coreboot.org/c/coreboot/+/34501/1/src/drivers/vpd/vpd_decode.... PS1, Line 143: if (offset == search_length) {
braces {} are not necessary for single statement blocks
Done
https://review.coreboot.org/c/coreboot/+/34501/3/src/drivers/vpd/vpd_decode.... File src/drivers/vpd/vpd_decode.c:
https://review.coreboot.org/c/coreboot/+/34501/3/src/drivers/vpd/vpd_decode.... PS3, Line 6: This is a copy from upstream: : * https://chromium.googlesource.com/chromiumos/platform/vpd/+/master/lib/vpd_d...
this is a direct copy from upstream. […]
Done
https://review.coreboot.org/c/coreboot/+/34501/3/src/drivers/vpd/vpd_decode.... PS3, Line 110: search
Are you using CrOS VPD 2.0 format? […]
Done
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. […]
Done
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
Done
https://review.coreboot.org/c/coreboot/+/34501/4/src/drivers/vpd/vpd_fsp.c@1... PS4, Line 135: if (decodeLen(max_len - *consumed,
suspect code indent for conditional statements (16, 32)
Done
https://review.coreboot.org/c/coreboot/+/34501/4/src/drivers/vpd/vpd_fsp.c@1... PS4, Line 135: decodeLen
there's no decodeLen anymore
Done
https://review.coreboot.org/c/coreboot/+/34501/4/src/drivers/vpd/vpd_fsp.c@1... PS4, Line 148: if (decodeLen(max_len - *consumed,
suspect code indent for conditional statements (16, 32)
Done
https://review.coreboot.org/c/coreboot/+/34501/5/src/mainboard/ocp/monolake/... File src/mainboard/ocp/monolake/romstage.c:
https://review.coreboot.org/c/coreboot/+/34501/5/src/mainboard/ocp/monolake/... PS5, Line 77: const char *board_upd_vars[] = {FSP_VAR_HYPERTHREADING};
char * array declaration might be better as static const
Done
https://review.coreboot.org/c/coreboot/+/34501/5/src/mainboard/ocp/monolake/... PS5, Line 121: if (vpd_decode_string(rw_vpd_size, rw_vpd_addr, &consumed,
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/34501/6/src/mainboard/ocp/monolake/... File src/mainboard/ocp/monolake/romstage.c:
https://review.coreboot.org/c/coreboot/+/34501/6/src/mainboard/ocp/monolake/... PS6, Line 77: const char *board_upd_vars[] = {FSP_VAR_HYPERTHREADING};
char * array declaration might be better as static const
If it is declared as static const, following build error happens: Forbidden global variables in romstage: /home/jonzhang/local/work/vpd/osf-partner-template/coreboot-monolake/util/crossgcc/xgcc/bin/i386-elf-nm: 'build/cbfs/fallback/romstage_null.offenders': No such file make: *** [build/cbfs/fallback/romstage.debug] Error 1 make: *** Deleting file `build/cbfs/fallback/romstage.debug'
https://review.coreboot.org/c/coreboot/+/34501/6/src/mainboard/ocp/monolake/... PS6, Line 121: if (vpd_decode_string(rw_vpd_size, rw_vpd_addr, &consumed,
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/34501/6/src/mainboard/ocp/monolake/... PS6, Line 122: board_vpd_process, (void *)UpdData) == VPD_DECODE_FAIL)
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/34501/3/src/soc/intel/fsp_broadwell... File src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c:
https://review.coreboot.org/c/coreboot/+/34501/3/src/soc/intel/fsp_broadwell... PS3, Line 132: : /* : * 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)) { : search_address = rdev_mmap_full(&rdev); : if (search_address != NULL) { : search_length = region_device_sz(&rdev); : /* : * If there is a valid value for HyperThreading key in RW_VPD, : * use the value to customize FSP UPD HyperThreading variable. : */ : if (get_hyper_threading(&hyper_threading, search_address, : search_length) == true) { : UpdData->HyperThreading = hyper_threading; : } : } : } : }
I wonder if it'll be easier to implement this as a board-specific callback, say […]
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/34501/4/src/soc/intel/fsp_broadwell... PS4, Line 55: const char *board_upd_vars[] = {"HyperThreading"};
char * array declaration might be better as static const
Done
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 […]
Done
https://review.coreboot.org/c/coreboot/+/34501/4/src/soc/intel/fsp_broadwell... PS4, Line 172: board_fsp_configure_upd_data, UpdData) == VPD_FAIL)
line over 96 characters
Done