Attention is currently required from: Arthur Heymans, Christian Walter, David Hendricks, Jincheng Li, Jonathan Zhang, Lean Sheng Tan, Shuo Liu, TangYiwei, Tim Chu.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81317?usp=email )
Change subject: soc/intel/xeon_sp/gnr: Use OCP_VPD drivers ......................................................................
Patch Set 44:
(1 comment)
File src/soc/intel/xeon_sp/gnr/Kconfig:
https://review.coreboot.org/c/coreboot/+/81317/comment/451a1b57_f322efa0 : PS44, Line 19: select OCP_VPD I don't think selecting (requiring) `OCP_VPD` is a good idea. At the very least, please consider doing this:
``` static int get_fsp_mem_log_level(void) { /* TODO: Implement the option API using VPD */ if (CONFIG(OCP_VPD)) return get_int_from_vpd_range(FSP_MEM_LOG_LEVEL, FSP_MEM_LOG_LEVEL_DEFAULT, 0, 4); else return FSP_MEM_LOG_LEVEL_DEFAULT; }
static bool get_fsp_log_enable(void) { /* TODO: Implement the option API using VPD */ if (CONFIG(OCP_VPD)) return get_bool_from_vpd(FSP_LOG, FSP_LOG_DEFAULT); else return FSP_LOG_DEFAULT; }
void platform_fsp_memory_init_params_cb(FSPM_UPD *mupd, uint32_t version) { /* Prologue omitted for brevity */ m_cfg->serialDebugMsgLvl = get_fsp_mem_log_level(); m_cfg->SerialIoUartDebugEnable = get_fsp_enable_log(); } ```
(Note: not build tested, might need to replace the if statements with preprocessor if the non-VPD case doesn't build)
Including the TODOs. Ideally, SoC code would use the option API, and OCP would implement the option API using VPD:
``` void platform_fsp_memory_init_params_cb(FSPM_UPD *mupd, uint32_t version) { /* Prologue omitted for brevity */ m_cfg->serialDebugMsgLvl = get_uint_option("fsp_log_level", DEFAULT_FSP_LOG_LEVEL); m_cfg->SerialIoUartDebugEnable = !!get_uint_option("fsp_enable_log", DEFAULT_FSP_ENABLE_LOG); } ```
This way, if someone else wants to use another option API (e.g. EFI variable store, for use with an edk2 payload), they don't need to change anything in SoC code.
I understand that implementing the option API using VPD as a backend is not trivial. So, my suggestion to get this change unblocked is to avoid depending on `OCP_VPD` for now; eventually, the plan is to use the option API for maximum flexibility.