Attention is currently required from: Kapil Porwal, Paul Menzel, Pranava Y N, Ronak Kanabar, Subrata Banik.
Jérémy Compostella has posted comments on this change by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/84552?usp=email )
Change subject: soc/intel/pantherlake: Add FSP-S programming ......................................................................
Patch Set 13:
(3 comments)
File src/soc/intel/pantherlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/84552/comment/17c378b3_7dac1b79?usp... : PS11, Line 250: static void fill_fsps_cpu_params(FSP_S_CONFIG *s_cfg, : const struct soc_intel_pantherlake_config *config) : { : const struct microcode *microcode; : size_t length; : : if (CONFIG(USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI)) : s_cfg->CpuMpPpi = (uintptr_t)mp_fill_ppi_services_data(); : : if (CONFIG(USE_FSP_FEATURE_PROGRAM_ON_APS)) { : /* Locate microcode and pass to FSP-S for 2nd microcode loading */ : microcode = intel_microcode_find(); : if (!microcode) : return; : : length = get_microcode_size(microcode); : if (!length) : return; : : /* Update CPU Microcode patch base address/size */ : s_cfg->MicrocodeRegionBase = (uint32_t)(uintptr_t)microcode; : s_cfg->MicrocodeRegionSize = (uint32_t)length; : } : }
Could you please provide details about why you want it that way ? Here are few notes: […]
2. USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI is not enforced by pantherlake Kconfig which only relies on the default settings so I believe it should be guard as a mainboard could decide otherwise. This kconfig is specific to this use-case where coreboot let FSP use coreboot MP PPI. In addition, we should not be worried about loosing any CPU cycles thanks to compiler optimization anyway. 3. Sure, I'll do that tomorrow.
https://review.coreboot.org/c/coreboot/+/84552/comment/71aae0fc_209c0aa4?usp... : PS11, Line 301: max_port = get_max_tcss_port(); : for (i = 0; i < max_port; i++)
`get_max_tcss_port()` is what has been used everywhere in the pantherlake SoC code to get the numb […]
It would definitely save few CPU cycles. Should I replace the call to `get_max_tcss_port()` everywhere ? What about `get_max_pcie_port()`? or `get_max_i2c_port()`? Do you think we should also replace them with their corresponding constants ? At this point we could even get rid of the soc_info.c saving a few bytes of binary space and as a result a few extra cycles in the process. I am sure this is going to hurt code readability and as a result maintainability but if it is really what you desire, I can do it.
File src/soc/intel/pantherlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/84552/comment/375bc07b_34016b94?usp... : PS13, Line 637: if (is_devfn_enabled(PCI_DEVFN_HDA)) {
Done