Attention is currently required from: Arthur Heymans, Christian Walter, David Hendricks, Jincheng Li, Jonathan Zhang, Lean Sheng Tan, Shuo Liu, TangYiwei, Tim Chu.
1 comment:
File src/soc/intel/xeon_sp/gnr/Kconfig:
Patch Set #44, 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.
To view, visit change 81317. To unsubscribe, or for help writing mail filters, visit settings.