Attention is currently required from: Jérémy Compostella, Kapil Porwal, Paul Menzel, Pranava Y N, Ronak Kanabar.
Subrata Banik 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:
(2 comments)
File src/soc/intel/pantherlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/84552/comment/77294b63_fbd31690?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; : } : }
- 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.
there are two options to perform MP Init
1. USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI <--- FSP runs coreboot APIs 2. USE_INTEL_FSP_MP_INIT <--- FSP runs own APIs
in case #2 , filling the CpuMpPpi UPD won't harm. if you wish to use #2 (which we don't plan to use) then please select USE_INTEL_FSP_MP_INIT config.
some more details: https://review.coreboot.org/c/coreboot/+/66706
- Sure, I'll do that tomorrow.
https://review.coreboot.org/c/coreboot/+/84552/comment/ce2b40f7_f19f1273?usp... : PS11, Line 301: max_port = get_max_tcss_port(); : for (i = 0; i < max_port; i++)
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.
If we have static configurations like Kconfig and/or macros, it's better to depend on them instead of functions. We don't want to override that function. However, I'll leave it up to you if you'd prefer to change other APIs to macros.