[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 13:

(3 comments)

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

https://review.coreboot.org/c/coreboot/+/84552/comment/6b0d3550_10e83072?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:
> `SkipMpInit` UPD does not exist anymore so the comment and the "workaround" do not mean anything anymore.
> IMO, creating an separate `fill_fsps_microcode_params()` to be called from `fill_fsps_cpu_params()` do not make sense. It should either not be separated (as I am suggesting) or be called directly from `soc_silicon_init_params()`. Otherwise it makes the code unnecessarily cumbersome.

The `CONFIG_USE_FSP_FEATURE_PROGRAM_ON_APS` config is enabled in MTL, with an agreement between Google and Intel's FSP team that Intel would follow up during the SOC generation. (b/219061518) I agree that if we don't have a way to drop `CONFIG_USE_FSP_FEATURE_PROGRAM_ON_APS=n`, then the `fill_fsps_microcode_params()` API call might not be valuable.

to summarize my request

1. Hopefully the Intel FSP team will pick `CONFIG_USE_FSP_FEATURE_PROGRAM_ON_APS=n` like we discussion

2. Do we need the below `if` guard. looks like we don't need any guarding.
```

if (CONFIG(USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI))
    	s_cfg->CpuMpPpi = (uintptr_t)mp_fill_ppi_services_data();
```

3. Can we use a helper function for below code to just make the code modular.

```
    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;
    }
```

https://review.coreboot.org/c/coreboot/+/84552/comment/d3399a9f_0e6202e8?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 number of TCSS port.

my point is `MAX_TYPE_C_PORTS` will return the same port number statically w/o an overhead of function call ?

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

https://review.coreboot.org/c/coreboot/+/84552/comment/497d463c_8472b73f?usp... :
PS13, Line 637: 	if (is_devfn_enabled(PCI_DEVFN_HDA)) {
```
    if (!is_devfn_enabled(PCI_DEVFN_HDA))
       return;
```