jonzhang@fb.com has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34501 )
Change subject: Get HyperThreading setting from RW_VPD for FSP UPD ......................................................................
Get HyperThreading setting from RW_VPD for FSP UPD
Summary: This patch adds a library function in vpd_decode.h which is available in romstage, and thus can be used to customize FSP UPD setting.
Fetch key named as "HyperThreading" from RW_VPD which is a VPD partition. If there is a valid value, use the value to set FSP UPD HyperThreading variable.
If RW_VPD is not found, or if "HyperThreading" key does not exist, or if the value of "HyperThreading" key is neither '1' nor '0', default value is used.
BDW-DE SOC code is updated to use this function, if VPD is configured.
Test Plan: * Build an OCP MonoLake coreboot image, run following command to initialize RW_VPD and insert HyperThreading key: vpd -f build/coreboot.rom -O -i RW_VPD -s 'HyperThreading=0' * Flash the image to MonoLake, boot and observe following message in boot log: Detected 16 CPU threads
If RW_VPD partition does not exist, or if HyperThreading key/value pair does not exist, the boot log has: Detected 32 CPU threads
Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Ice6ffe3f7a14c7f09637c5dcfc47e551a57f42f1 --- M src/drivers/vpd/vpd_decode.c M src/drivers/vpd/vpd_decode.h M src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c 3 files changed, 105 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/34501/1
diff --git a/src/drivers/vpd/vpd_decode.c b/src/drivers/vpd/vpd_decode.c index 527c508..4638971 100644 --- a/src/drivers/vpd/vpd_decode.c +++ b/src/drivers/vpd/vpd_decode.c @@ -6,6 +6,8 @@ * This is a copy from upstream: * https://chromium.googlesource.com/chromiumos/platform/vpd/+/master/lib/vpd_d... */ +#include <device/mmio.h> +#include <string.h> #include "vpd_decode.h"
static int vpd_decode_len( @@ -95,3 +97,63 @@
return VPD_DECODE_OK; } + +/** + * search: Find first instance of string in a given region + * @param p string to find + * @param a start address of region to search + * @param lengthp length of string to search for + * @param lengtha length of region to search in + * @return offset offset from start address a where string was found. + * If string not found, return lengtha. + */ +static uintptr_t search(char *p, u8 *a, unsigned int lengthp, + unsigned int lengtha) +{ + int i, j; + + /* Searching */ + for (j = 0; j <= lengtha - lengthp; j++) { + for (i = 0; i < lengthp && p[i] == a[i + j]; i++) + ; + if (i >= lengthp) + return j; + } + return (uintptr_t)lengtha; +} + +/* + * Fetch HyperThreading key/value from a VPD partition, eg. RW_VPD. + * + * If a key with matching name is found, and if the value is + * either '1' or '0', the return value is set to true, + * and *hyper_threading is set accordingly. + * Otherwise, the return value is set to false. + * + * search_address points to the VPD parition memory address. + * search_length is the siae of such VPD partition. + */ +bool get_hyper_threading(u8 *hyper_threading, + void *search_address, size_t search_length) +{ + char key[] = "HyperThreading"; + uintptr_t offset; + + offset = search(key, search_address, strlen(key), search_length); + if (offset == search_length) { + return false; + } + + /* move to value field, and bypass the byte holding length of value */ + /* The size of key field is sizeof(key) - 1 */ + offset += sizeof(key); + *hyper_threading = read8(search_address + offset); + if (*hyper_threading == '1') + *hyper_threading = 1; + else if (*hyper_threading == '0') + *hyper_threading = 0; + else + return false; + + return true; +} diff --git a/src/drivers/vpd/vpd_decode.h b/src/drivers/vpd/vpd_decode.h index 5d595f3..25de12d 100644 --- a/src/drivers/vpd/vpd_decode.h +++ b/src/drivers/vpd/vpd_decode.h @@ -48,4 +48,18 @@ const u32 max_len, const u8 *input_buf, u32 *consumed, vpd_decode_callback callback, void *callback_arg);
+/* + * Fetch HyperThreading key/value from a VPD partition, eg. RW_VPD. + * + * If a key with matching name is found, and if the value is + * either '1' or '0', the return value is set to true, + * and *hyper_threading is set accordingly. + * Otherwise, the return value is set to false. + * + * search_address points to the VPD parition memory address. + * search_length is the siae of such VPD partition. + */ +bool get_hyper_threading(u8 *hyper_threading, + void *search_address, size_t search_length); + #endif /* __VPD_DECODE_H */ diff --git a/src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c b/src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c index edb313e..c5b2874 100644 --- a/src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c +++ b/src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c @@ -22,7 +22,9 @@ #include <cbmem.h> #include <device/device.h> #include <device/pci_def.h> +#include <device/mmio.h> #include <drivers/intel/fsp1_0/fsp_util.h> +#include <drivers/vpd/vpd_decode.h> #include <soc/pci_devs.h> #include <soc/romstage.h> #include <fsp.h> @@ -48,6 +50,11 @@ */ static void ConfigureDefaultUpdData(UPD_DATA_REGION *UpdData) { + struct region_device rdev; + void *search_address = NULL; + size_t search_length = -1; + u8 hyper_threading; + /* * Serial Port */ @@ -121,6 +128,28 @@ UpdData->Ehci2Enable = 1; else UpdData->Ehci2Enable = 0; + + /* + * 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; + } + } + } + } }
/* Set up the Broadwell-DE specific structures for the call into the FSP */
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34501 )
Change subject: Get HyperThreading setting from RW_VPD for FSP UPD ......................................................................
Patch Set 1:
(1 comment)
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
jonzhang@fb.com has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34501 )
Change subject: Get HyperThreading setting from RW_VPD for FSP UPD ......................................................................
Patch Set 1:
Please help to review this patch.
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, David Hendricks, Hung-Te Lin, Philipp Deppenwiese, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34501
to look at the new patch set (#2).
Change subject: Get HyperThreading setting from RW_VPD for FSP UPD ......................................................................
Get HyperThreading setting from RW_VPD for FSP UPD
Summary: This patch adds a library function in vpd_decode.h which is available in romstage, and thus can be used to customize FSP UPD setting.
Fetch key named as "HyperThreading" from RW_VPD which is a VPD partition. If there is a valid value, use the value to set FSP UPD HyperThreading variable.
If RW_VPD is not found, or if "HyperThreading" key does not exist, or if the value of "HyperThreading" key is neither '1' nor '0', default value is used.
BDW-DE SOC code is updated to use this function, if VPD is configured.
Test Plan: * Build an OCP MonoLake coreboot image, run following command to initialize RW_VPD and insert HyperThreading key: vpd -f build/coreboot.rom -O -i RW_VPD -s 'HyperThreading=0' * Flash the image to MonoLake, boot and observe following message in boot log: Detected 16 CPU threads
If RW_VPD partition does not exist, or if HyperThreading key/value pair does not exist, the boot log has: Detected 32 CPU threads
Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Ice6ffe3f7a14c7f09637c5dcfc47e551a57f42f1 --- M src/drivers/vpd/vpd_decode.c M src/drivers/vpd/vpd_decode.h M src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c 3 files changed, 105 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/34501/2
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, David Hendricks, Hung-Te Lin, Philipp Deppenwiese, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34501
to look at the new patch set (#3).
Change subject: Get HyperThreading setting from RW_VPD for FSP UPD ......................................................................
Get HyperThreading setting from RW_VPD for FSP UPD
Summary: This patch introduces a framework to use settings in VPD to customize FSP behavior through UPD settings. It gets HyperThreading setting from RW_VPD for FSP UPD HyperThreading configuration.
It adds a library function in vpd_decode.h which is available in romstage, and thus can be used to customize FSP UPD setting.
Fetch key named as "HyperThreading" from RW_VPD which is a VPD partition. If there is a valid value, use the value to set FSP UPD HyperThreading variable.
If RW_VPD is not found, or if "HyperThreading" key does not exist, or if the value of "HyperThreading" key is neither '1' nor '0', default value is used.
BDW-DE SOC code is updated to use this function, if VPD is configured.
Test Plan: * Build an OCP MonoLake coreboot image, run following command to initialize RW_VPD and insert HyperThreading key: vpd -f build/coreboot.rom -O -i RW_VPD -s 'HyperThreading=0' * Flash the image to MonoLake, boot and observe following message in boot log: Detected 16 CPU threads
If RW_VPD partition does not exist, or if HyperThreading key/value pair does not exist, the boot log has: Detected 32 CPU threads
Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Ice6ffe3f7a14c7f09637c5dcfc47e551a57f42f1 --- M src/drivers/vpd/vpd_decode.c M src/drivers/vpd/vpd_decode.h M src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c 3 files changed, 105 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/34501/3
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34501 )
Change subject: Get HyperThreading setting from RW_VPD for FSP UPD ......................................................................
Patch Set 3: Code-Review-1
(2 comments)
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. Please keep it untouched so we can sync with upstream in future.
You may instead put that in your board-specific logic, or a new one in vpd folder.
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? https://chromium.googlesource.com/chromiumos/platform/vpd/+/master/
If not, then this should be in your own VPD module, not with vpd_decode (which is for CrOS VPD).
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34501 )
Change subject: Get HyperThreading setting from RW_VPD for FSP UPD ......................................................................
Patch Set 3:
(1 comment)
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
board_fsp_configure_upd_data(UPD_DATA_REGION *UpdData);
Then you can in your board handle your own VPD, and update the HT setting.
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, David Hendricks, Hung-Te Lin, Philipp Deppenwiese, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34501
to look at the new patch set (#4).
Change subject: Framework to get settings from RW_VPD to configure FSP UPD ......................................................................
Framework to get settings from RW_VPD to configure FSP UPD
This patch adds following: * A framework to use VPD binary blob 2.0 data to configure FSP UPD. * A library to configure HyperThreading FSP UPD variable. * Change for MonoLake platform to use such framework and library.
The framework is available in romstage, and thus can be used to customize FSP UPD setting. The framework currently inludes a function to take care of binary type UPD variables.
The HyperThreading library function fetchs key named as "HyperThreading" from RW_VPD which is a VPD partition. If there is a valid value, use the value to set FSP UPD HyperThreading variable.
If RW_VPD is not found, or if "HyperThreading" key does not exist, or if the value of "HyperThreading" key is neither '1' nor '0', default value is used.
BDW-DE SOC code is updated to use this function, if VPD is configured.
Test Plan: * Build an OCP MonoLake coreboot image, run following command to initialize RW_VPD and insert HyperThreading key: vpd -f build/coreboot.rom -O -i RW_VPD -s 'HyperThreading=0' * Flash the image to MonoLake, boot and observe following message in boot log: Detected 16 CPU threads
If RW_VPD partition does not exist, or if HyperThreading key/value pair does not exist, the boot log has: Detected 32 CPU threads
Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Ice6ffe3f7a14c7f09637c5dcfc47e551a57f42f1 --- M src/drivers/vpd/Makefile.inc A src/drivers/vpd/vpd_fsp.c A src/drivers/vpd/vpd_fsp.h M src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c 4 files changed, 286 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/34501/4
build bot (Jenkins) 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:
(4 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@1... PS4, Line 135: if (decodeLen(max_len - *consumed, suspect code indent for conditional statements (16, 32)
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)
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 55: const char *board_upd_vars[] = {"HyperThreading"}; char * array declaration might be better as static const
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
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
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, David Hendricks, Hung-Te Lin, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34501
to look at the new patch set (#5).
Change subject: Framework to get settings from RW_VPD to configure FSP UPD ......................................................................
Framework to get settings from RW_VPD to configure FSP UPD
This patch adds following: * A framework to use VPD binary blob 2.0 data to configure FSP UPD. * A library to configure HyperThreading FSP UPD variable. * Change for MonoLake platform to use such framework and library.
The framework is available in romstage, and thus can be used to customize FSP UPD setting. The framework currently inludes a function to take care of binary type UPD variables.
Code of such framwork is in vpd driver. Support for a UPD variable (HyperThreading) is added. The function matches key/value pair from binary blob of RW_VPD which is a VPD partition. If there is a valid value, use the value to set FSP UPD HyperThreading variable.
If RW_VPD is not found, or if "HyperThreading" key does not exist, or if the value of "HyperThreading" key is neither '1' nor '0', default value is used.
BDW-DE SOC code is updated to define a weak function board_fsp_configure_upd_data().
OCP MonoLake mainboard code is updated to implement such weak function by calling the framwork.
Test Plan: * Build an OCP MonoLake coreboot image, run following command to initialize RW_VPD and insert HyperThreading key: vpd -f build/coreboot.rom -O -i RW_VPD -s 'HyperThreading=0' * Flash the image to MonoLake, boot and observe following message in boot log: Detected 16 CPU threads
If RW_VPD partition does not exist, or if HyperThreading key/value pair does not exist, the boot log has: Detected 32 CPU threads
Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Ice6ffe3f7a14c7f09637c5dcfc47e551a57f42f1 --- M src/drivers/intel/fsp1_0/fsp_util.h M src/drivers/vpd/Makefile.inc A src/drivers/vpd/vpd_fsp.c A src/drivers/vpd/vpd_fsp.h M src/mainboard/ocp/monolake/romstage.c M src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c M src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.h 7 files changed, 223 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/34501/5
build bot (Jenkins) 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 5:
(2 comments)
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
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
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Patrick Rudolph, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34501
to look at the new patch set (#6).
Change subject: Framework to get settings from RW_VPD to configure FSP UPD ......................................................................
Framework to get settings from RW_VPD to configure FSP UPD
This patch adds following: * A framework to use VPD binary blob 2.0 data to configure FSP UPD. * A library to configure HyperThreading FSP UPD variable. * Change for MonoLake platform to use such framework and library.
The framework is available in romstage, and thus can be used to customize FSP UPD setting. The framework currently inludes a function to take care of binary type UPD variables.
Code of such framwork is in vpd driver. Support for a UPD variable (HyperThreading) is added. The function matches key/value pair from binary blob of RW_VPD which is a VPD partition. If there is a valid value, use the value to set FSP UPD HyperThreading variable.
If RW_VPD is not found, or if "HyperThreading" key does not exist, or if the value of "HyperThreading" key is neither '1' nor '0', default value is used.
BDW-DE SOC code is updated to define a weak function board_fsp_configure_upd_data().
OCP MonoLake mainboard code is updated to implement such weak function by calling the framwork.
Test Plan: * Build an OCP MonoLake coreboot image, run following command to initialize RW_VPD and insert HyperThreading key: vpd -f build/coreboot.rom -O -i RW_VPD -s 'HyperThreading=0' * Flash the image to MonoLake, boot and observe following message in boot log: Detected 16 CPU threads
If RW_VPD partition does not exist, or if HyperThreading key/value pair does not exist, the boot log has: Detected 32 CPU threads
Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Ice6ffe3f7a14c7f09637c5dcfc47e551a57f42f1 --- M src/drivers/intel/fsp1_0/fsp_util.h M src/drivers/vpd/Makefile.inc A src/drivers/vpd/vpd_fsp.c A src/drivers/vpd/vpd_fsp.h M src/mainboard/ocp/monolake/romstage.c M src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c M src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.h 7 files changed, 223 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/34501/6
build bot (Jenkins) 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 6:
(3 comments)
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
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
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
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Patrick Rudolph, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34501
to look at the new patch set (#7).
Change subject: Framework to get settings from RW_VPD to configure FSP UPD ......................................................................
Framework to get settings from RW_VPD to configure FSP UPD
This patch adds following: * A framework to use VPD binary blob 2.0 data to configure FSP UPD. * A library to configure HyperThreading FSP UPD variable. * Change for MonoLake platform to use such framework and library.
The framework is available in romstage, and thus can be used to customize FSP UPD setting. The framework currently inludes a function to take care of binary type UPD variables.
Code of such framwork is in vpd driver. Support for a UPD variable (HyperThreading) is added. The function matches key/value pair from binary blob of RW_VPD which is a VPD partition. If there is a valid value, use the value to set FSP UPD HyperThreading variable.
If RW_VPD is not found, or if "HyperThreading" key does not exist, or if the value of "HyperThreading" key is neither '1' nor '0', default value is used.
BDW-DE SOC code is updated to define a weak function board_fsp_configure_upd_data().
OCP MonoLake mainboard code is updated to implement such weak function by calling the framwork.
Test Plan: * Build an OCP MonoLake coreboot image, run following command to initialize RW_VPD and insert HyperThreading key: vpd -f build/coreboot.rom -O -i RW_VPD -s 'HyperThreading=0' * Flash the image to MonoLake, boot and observe following message in boot log: Detected 16 CPU threads
If RW_VPD partition does not exist, or if HyperThreading key/value pair does not exist, the boot log has: Detected 32 CPU threads
Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Ice6ffe3f7a14c7f09637c5dcfc47e551a57f42f1 --- M src/drivers/intel/fsp1_0/fsp_util.h M src/drivers/vpd/Makefile.inc A src/drivers/vpd/vpd_fsp.c A src/drivers/vpd/vpd_fsp.h M src/mainboard/ocp/monolake/romstage.c M src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c M src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.h 7 files changed, 219 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/34501/7
build bot (Jenkins) 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 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34501/7/src/mainboard/ocp/monolake/... File src/mainboard/ocp/monolake/romstage.c:
https://review.coreboot.org/c/coreboot/+/34501/7/src/mainboard/ocp/monolake/... PS7, Line 81: const char *board_upd_vars[] = {FSP_VAR_HYPERTHREADING}; char * array declaration might be better as static const
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Patrick Rudolph, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34501
to look at the new patch set (#8).
Change subject: Framework to get settings from RW_VPD to configure FSP UPD ......................................................................
Framework to get settings from RW_VPD to configure FSP UPD
This patch adds following: * A framework to use VPD binary blob 2.0 data to configure FSP UPD. * A library to configure HyperThreading FSP UPD variable. * Change for MonoLake platform to use such framework and library.
The framework is available in romstage, and thus can be used to customize FSP UPD setting. The framework currently inludes a function to take care of binary type UPD variables.
Code of such framwork is in vpd driver. Support for a UPD variable (HyperThreading) is added. The function matches key/value pair from binary blob of RW_VPD which is a VPD partition. If there is a valid value, use the value to set FSP UPD HyperThreading variable.
If RW_VPD is not found, or if "HyperThreading" key does not exist, or if the value of "HyperThreading" key is neither '1' nor '0', default value is used.
BDW-DE SOC code is updated to define a weak function board_fsp_configure_upd_data().
OCP MonoLake mainboard code is updated to implement such weak function by calling the framwork.
Test Plan: * Build an OCP MonoLake coreboot image, run following command to initialize RW_VPD and insert HyperThreading key: vpd -f build/coreboot.rom -O -i RW_VPD -s 'HyperThreading=0' * Flash the image to MonoLake, boot and observe following message in boot log: Detected 16 CPU threads
If RW_VPD partition does not exist, or if HyperThreading key/value pair does not exist, the boot log has: Detected 32 CPU threads
Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Ice6ffe3f7a14c7f09637c5dcfc47e551a57f42f1 --- M src/drivers/vpd/Makefile.inc A src/drivers/vpd/vpd_fsp.c A src/drivers/vpd/vpd_fsp.h M src/mainboard/ocp/monolake/romstage.c M src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c M src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.h 6 files changed, 220 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/34501/8
build bot (Jenkins) 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 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34501/8/src/mainboard/ocp/monolake/... File src/mainboard/ocp/monolake/romstage.c:
https://review.coreboot.org/c/coreboot/+/34501/8/src/mainboard/ocp/monolake/... PS8, Line 81: const char *board_upd_vars[] = {FSP_VAR_HYPERTHREADING}; char * array declaration might be better as static const
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Patrick Rudolph, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34501
to look at the new patch set (#9).
Change subject: Framework to get settings from RW_VPD to configure FSP UPD ......................................................................
Framework to get settings from RW_VPD to configure FSP UPD
This patch adds following: * A framework to use VPD binary blob 2.0 data to configure FSP UPD. * A library to configure HyperThreading FSP UPD variable. * Change for MonoLake platform to use such framework and library.
The framework is available in romstage, and thus can be used to customize FSP UPD setting. The framework currently inludes a function to take care of binary type UPD variables.
Code of such framwork is in vpd driver. Support for a UPD variable (HyperThreading) is added. The function matches key/value pair from binary blob of RW_VPD which is a VPD partition. If there is a valid value, use the value to set FSP UPD HyperThreading variable.
If RW_VPD is not found, or if "HyperThreading" key does not exist, or if the value of "HyperThreading" key is neither '1' nor '0', default value is used.
BDW-DE SOC code is updated to define a weak function board_fsp_configure_upd_data().
OCP MonoLake mainboard code is updated to implement such weak function by calling the framwork.
Test Plan: * Build an OCP MonoLake coreboot image, run following command to initialize RW_VPD and insert HyperThreading key: vpd -f build/coreboot.rom -O -i RW_VPD -s 'HyperThreading=0' * Flash the image to MonoLake, boot and observe following message in boot log: Detected 16 CPU threads
If RW_VPD partition does not exist, or if HyperThreading key/value pair does not exist, the boot log has: Detected 32 CPU threads
Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Ice6ffe3f7a14c7f09637c5dcfc47e551a57f42f1 --- M src/drivers/vpd/Makefile.inc A src/drivers/vpd/vpd_fsp.c A src/drivers/vpd/vpd_fsp.h M src/mainboard/ocp/monolake/romstage.c M src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c M src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.h 6 files changed, 219 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/34501/9
build bot (Jenkins) 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 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34501/9/src/mainboard/ocp/monolake/... File src/mainboard/ocp/monolake/romstage.c:
https://review.coreboot.org/c/coreboot/+/34501/9/src/mainboard/ocp/monolake/... PS9, Line 81: const char *board_upd_vars[] = {FSP_VAR_HYPERTHREADING}; char * array declaration might be better as static const
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Patrick Rudolph, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34501
to look at the new patch set (#10).
Change subject: Framework to get settings from RW_VPD to configure FSP UPD ......................................................................
Framework to get settings from RW_VPD to configure FSP UPD
This patch adds following: * A framework to use VPD binary blob 2.0 data to configure FSP UPD. * A library to configure HyperThreading FSP UPD variable. * Change for MonoLake platform to use such framework and library.
The framework is available in romstage, and thus can be used to customize FSP UPD setting. The framework currently inludes a function to take care of binary type UPD variables.
Code of such framwork is in vpd driver. Support for a UPD variable (HyperThreading) is added. The function matches key/value pair from binary blob of RW_VPD which is a VPD partition. If there is a valid value, use the value to set FSP UPD HyperThreading variable.
If RW_VPD is not found, or if "HyperThreading" key does not exist, or if the value of "HyperThreading" key is neither '1' nor '0', default value is used.
BDW-DE SOC code is updated to define a weak function board_fsp_configure_upd_data().
OCP MonoLake mainboard code is updated to implement such weak function by calling the framwork.
Test Plan: * Build an OCP MonoLake coreboot image, run following command to initialize RW_VPD and insert HyperThreading key: vpd -f build/coreboot.rom -O -i RW_VPD -s 'HyperThreading=0' * Flash the image to MonoLake, boot and observe following message in boot log: Detected 16 CPU threads
If RW_VPD partition does not exist, or if HyperThreading key/value pair does not exist, the boot log has: Detected 32 CPU threads
Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Ice6ffe3f7a14c7f09637c5dcfc47e551a57f42f1 --- M src/drivers/vpd/Makefile.inc A src/drivers/vpd/vpd_fsp.c A src/drivers/vpd/vpd_fsp.h M src/mainboard/ocp/monolake/romstage.c M src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c M src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.h 6 files changed, 219 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/34501/10
build bot (Jenkins) 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:
(1 comment)
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 81: const char *board_upd_vars[] = {FSP_VAR_HYPERTHREADING}; char * array declaration might be better as static const
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
Patrick Rudolph 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:
(1 comment)
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
That is because you mix FSP1.0 with Broadwell DE. Please move the Broadwell DE specific parts to src/soc/intel/fsp_boardwell_de/. src/drivers should not contain platform specific code.
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 129: board_fsp_configure_upd_data(UpdData); there's already romstage_fsp_rt_buffer_callback(). No need for another weak function.
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.
jonzhang@fb.com has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34501 )
Change subject: Framework to get settings from RW_VPD to configure FSP UPD ......................................................................
Abandoned
This patch is replaced by patch set starting with https://review.coreboot.org/c/coreboot/+/34634
-
build bot (Jenkins) (Code Review) -
Hung-Te Lin (Code Review)
-
Name of user not set (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)