[coreboot-gerrit] [L] Change in coreboot[main]: soc/intel/pantherlake: Add FSP-S programming

[Subrata Banik (Code Review)]

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 15:

(2 comments)

File src/soc/intel/pantherlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/84552/comment/fd2a6b87_aebfcf30?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 <&#x2014; FSP runs coreboot APIs
> > USE_INTEL_FSP_MP_INIT <&#x2014; FSP runs own APIs
> >
> > in case #2 , filling the CpuMpPpi UPD won't harm.
>
> That  is simply not true. If `CpuMpPpi` is set while `USE_INTEL_FSP_MP_INIT=y` and `CONFIG_USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI=n` the FSP hit some exceptions inside coreboot MP functions because some initialization were not performed. If Is set to `NULL` which clearly is what is appropriate under this configuration when the CPU initialization execute just fine.
> > if you wish to use #2 (which we don't plan to use) then please select USE_INTEL_FSP_MP_INIT config.
>
> I do not plan on changing the default configuration, this is just about setting the UPD appropriately.

updated code looks good to me

https://review.coreboot.org/c/coreboot/+/84552/comment/18d7fd53_170d13d6?usp... :
PS11, Line 301: 	max_port = get_max_tcss_port();
              : 	for (i = 0; i < max_port; i++)
> There was no way I would introduce inconsistent code as I would have if I had followed your proposal so I created [84616: soc/intel/pantherlake: Remove soc_info.[hc] interface](https://review.coreboot.org/c/coreboot/+/84616) instead. Obviously, the CPU cycles saving is completely ridiculous

can you please explain why this is "completely ridiculous" ? (ideally I would like to avoid making use of such a word while communicating to someone)

Have you compared the assembly instruction between below codes (code 1 and code 2)?

Code 1

```
    max_port = get_max_tcss_port();
    for (i = 0; i < max_port; i++)
    	if (config->tcss_ports[i].en
```

Code 2

```
    for (i = 0; i < MAX_TYPE_C_PORTS; i++)
    	if (config->tcss_ports[i].en
```

here is the code analysis report

Function call involved in code 1: get_max_tcss_port() - This involves a function call overhead (pushing arguments, jumping to the function, executing the function, returning the value).

Code 2 is likely more optimized in terms of instruction count. Function calls generally involve more instructions than accessing a constant value.