Attention is currently required from: Furquan Shaikh, Rizwan Qureshi, Tim Wawrzynczak, Subrata Banik, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55225 )
Change subject: soc/intel/alderlake/romstage: Refactor soc_memory_init_params function ......................................................................
Patch Set 1:
(5 comments)
File src/soc/intel/alderlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55225/comment/8462001c_7358d9b1 PS1, Line 95: memory
fill_mrc_memory_init_params()?
I would drop the `memory_init_params` part because FSP-M is not just about memory init (although the biggest part of FSP-M is memory init). I would also use shorter names.
So, how about using `fill_fspm_*_params()` for the function names? The `*` would be the name of each category, e.g. this function would be `fill_fspm_mrc_params()` or `fill_fspm_memory_params()`.
https://review.coreboot.org/c/coreboot/+/55225/comment/d141c468_da656360 PS1, Line 187: static void disable_pcie_clksrc_memory_init_params(FSP_M_CONFIG *m_cfg, : const struct soc_intel_alderlake_config *config) : { : unsigned int i; : : for (i = 0; i < CONFIG_MAX_PCIE_CLOCK_SRC; i++) { : if (config->pcie_clk_config_flag[i] & PCIE_CLK_FREE_RUNNING) : m_cfg->PcieClkSrcUsage[i] = FSP_CLK_FREE_RUNNING; : else if (config->pcie_clk_config_flag[i] & PCIE_CLK_LAN) : m_cfg->PcieClkSrcUsage[i] = FSP_CLK_LAN; : else : m_cfg->PcieClkSrcUsage[i] = FSP_CLK_NOTUSED; : m_cfg->PcieClkSrcClkReq[i] = FSP_CLK_NOTUSED; : } : } : : static void fill_pch_pcie_memory_init_params(FSP_M_CONFIG *m_cfg, : const struct soc_intel_alderlake_config *config) : { : m_cfg->PcieRpEnableMask = pcie_rp_enable_mask(get_pch_pcie_rp_table()); : pcie_rp_init(m_cfg, m_cfg->PcieRpEnableMask, PCH_PCIE_RP, config->pch_pcie_rp, : CONFIG_MAX_PCH_ROOT_PORTS); : } : : static void fill_cpu_pcie_memory_init_params(FSP_M_CONFIG *m_cfg, : const struct soc_intel_alderlake_config *config) : { : m_cfg->CpuPcieRpEnableMask = pcie_rp_enable_mask(get_cpu_pcie_rp_table()); : pcie_rp_init(m_cfg, m_cfg->CpuPcieRpEnableMask, CPU_PCIE_RP, config->cpu_pcie_rp, : CONFIG_MAX_CPU_ROOT_PORTS); : } I wouldn't create these three functions. Instead, just put this code inside `fill_pcie_rp_memory_init_params()` directly.
https://review.coreboot.org/c/coreboot/+/55225/comment/084bd351_ed92d87d PS1, Line 219: static void fill_pcie_rp_memory_init_params(FSP_M_CONFIG *m_cfg, I'd place this function right below the definition of `pcie_rp_init` (near the top of this file).
https://review.coreboot.org/c/coreboot/+/55225/comment/6728c0b1_a4770fc6 PS1, Line 235: /* Enable ISH controller */ Isn't what this comment says a biiiiit obvious? 😉
https://review.coreboot.org/c/coreboot/+/55225/comment/b8da8bcd_b0e986af PS1, Line 320: /* Fill IGD related FSP-M UPDs */ : fill_igd_memory_init_params(m_cfg, config); : : /* Fill Memory related FSP-M UPDs */ : fill_memory_memory_init_params(m_cfg, config); : : /* Fill CPU related FSP-M UPDs */ : fill_cpu_memory_init_params(m_cfg, config); : : /* Fill security related FSP-M UPDs */ : fill_security_memory_init_params(m_cfg); : : /* Fill UART debug related FSP-M UPDs */ : fill_uart_memory_init_params(m_cfg); : : /* Fill IPU related FSP-M UPDs */ : fill_ipu_memory_init_params(m_cfg); : : /* Fill SMBUS related FSP-M UPDs */ : fill_smbus_memory_init_params(m_cfg); : : /* Fill Miscellaneous FSP-M UPDs */ : fill_misc_memory_init_params(m_cfg, config); : : /* Fill Audio related FSP-M UPDs */ : fill_audio_memory_init_params(m_cfg, config); : : /* Fill PCIE related RP FSP-M UPDs */ : fill_pcie_rp_memory_init_params(m_cfg, config); : : /* Fill ISH related FSP-M UPD */ : fill_ish_memory_init_params(m_cfg); : : /* Fill TCSS related FSP-M UPD */ : fill_tcss_memory_init_params(m_cfg); : : /* Fill USB4/TBT related FSP-M UPD */ : fill_usb4_memory_init_params(m_cfg); : : /* Fill VT-d related FSP-M UPD */ : fill_vtd_memory_init_params(m_cfg); : : /* Fill trace related FSP-M UPD */ : fill_trace_memory_init_params(m_cfg); I don't think we need a comment on every function call.
Another thing: if you update all functions to have the same signature (even if `config` is unused), we could turn this into a table:
const void (*fill_fspm_params[])(FSP_M_CONFIG *m_cfg, const struct soc_intel_alderlake_config *config) = { fill_igd_memory_init_params, fill_memory_memory_init_params, fill_cpu_memory_init_params, fill_security_memory_init_params, fill_uart_memory_init_params, fill_ipu_memory_init_params, fill_smbus_memory_init_params, fill_misc_memory_init_params, fill_audio_memory_init_params, fill_pcie_rp_memory_init_params, fill_ish_memory_init_params, fill_tcss_memory_init_params, fill_usb4_memory_init_params, fill_vtd_memory_init_params, fill_trace_memory_init_params, }; for (size_t i = 0; i < ARRAY_SIZE(fill_fspm_params_list); i++) fill_fspm_params[i](m_cfg, config);
However, just because we can do something doesn't mean we should.