Paul Menzel 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:
(12 comments)
Please check to use the new 96 character text width.
https://review.coreboot.org/c/coreboot/+/34501/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34501/10//COMMIT_MSG@7 PS10, Line 7: Framework to get settings from RW_VPD to configure FSP UPD Please make it a statement by using a verb (in imperative mood).
Add framework to get settings from RW_VPD to configure FSP UPD
https://review.coreboot.org/c/coreboot/+/34501/10//COMMIT_MSG@13 PS10, Line 13: * Change for MonoLake platform to use such framework and library. Please split out the mainboard code into a separate commit.
https://review.coreboot.org/c/coreboot/+/34501/10/src/drivers/vpd/vpd_fsp.c File src/drivers/vpd/vpd_fsp.c:
https://review.coreboot.org/c/coreboot/+/34501/10/src/drivers/vpd/vpd_fsp.c@... PS10, Line 5: * Use of this source code is governed by a BSD-style license that can be Add blank line above?
https://review.coreboot.org/c/coreboot/+/34501/10/src/drivers/vpd/vpd_fsp.c@... PS10, Line 24: * During the process, necessary checking is done, such as making I’d add blank blank lines between paragraphs.
https://review.coreboot.org/c/coreboot/+/34501/10/src/drivers/vpd/vpd_fsp.c@... PS10, Line 21: * Internal function to process UPD variable of boolean type. : * Match the variable name with key name in the key/value pair, : * use the value to set *val. : * During the process, necessary checking is done, such as making : * sure the value length is 1, and value is either '1' or '0'. Please use the full text-width of (now) 96 characters.
https://review.coreboot.org/c/coreboot/+/34501/10/src/drivers/vpd/vpd_fsp.c@... PS10, Line 35: printk(BIOS_ERR, "key_len: %d, value_len: %d\n", key_len, value_len); Please write a sentence for error level messages.
https://review.coreboot.org/c/coreboot/+/34501/10/src/drivers/vpd/vpd_fsp.c@... PS10, Line 41: if (key[i] != upd_var[i]) Please give a warning(?)/error.
https://review.coreboot.org/c/coreboot/+/34501/10/src/drivers/vpd/vpd_fsp.c@... PS10, Line 64: u8 I believe uint8_t is preferred.
https://review.coreboot.org/c/coreboot/+/34501/10/src/drivers/vpd/vpd_fsp.c@... PS10, Line 74: set sets
https://review.coreboot.org/c/coreboot/+/34501/10/src/drivers/vpd/vpd_fsp.c@... PS10, Line 89: if (len_board_upd_vars > len_upd_vars) Add an error message?
https://review.coreboot.org/c/coreboot/+/34501/10/src/mainboard/ocp/monolake... File src/mainboard/ocp/monolake/romstage.c:
https://review.coreboot.org/c/coreboot/+/34501/10/src/mainboard/ocp/monolake... PS10, Line 102: /* : * 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. : */ Use tabs for indentation.
https://review.coreboot.org/c/coreboot/+/34501/10/src/soc/intel/fsp_broadwel... File src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c:
https://review.coreboot.org/c/coreboot/+/34501/10/src/soc/intel/fsp_broadwel... PS10, Line 125: /* : * Call board specific implementation to use data stored : * in VPD binary blob to configure FSP UPD variables. : */ Use tabs for indentation.