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

(19 comments)

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

https://review.coreboot.org/c/coreboot/+/84552/comment/d533ee4b_6b7f144a?usp... :
PS11, Line 102: ANY_PIRQ
any reason for not using fixed IRQ for DPTF ?

https://review.coreboot.org/c/coreboot/+/84552/comment/47252a31_31987f82?usp... :
PS11, Line 118: 		},
Why did we skipped IRQ assignment for PCIE RP 11 and 12 ?

Ideally you can use PTL-UH config to implement PCIE RP > 10 as well

https://review.coreboot.org/c/coreboot/+/84552/comment/0326f9a0_c76a7702?usp... :
PS11, Line 136: 	{
did we remove ISH intentionally ?

https://review.coreboot.org/c/coreboot/+/84552/comment/f4cf2e5f_682041bf?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;
              : 	}
              : }
```

static void fill_fsps_microcode_params(FSP_S_CONFIG *s_cfg,
    	const struct soc_intel_meteorlake_config *config)
{
    const struct microcode *microcode_file;
    size_t microcode_len;

/* Locate microcode and pass to FSP-S for 2nd microcode loading */
    microcode_file = intel_microcode_find();

if (microcode_file != NULL) {
    	microcode_len = get_microcode_size(microcode_file);
    	if (microcode_len != 0) {
    		/* Update CPU Microcode patch base address/size */
    		s_cfg->MicrocodeRegionBase = (uint32_t)(uintptr_t)microcode_file;
    		s_cfg->MicrocodeRegionSize = (uint32_t)microcode_len;
    	}
    }
}

static void fill_fsps_cpu_params(FSP_S_CONFIG *s_cfg,
    	const struct soc_intel_meteorlake_config *config)
{
    /*
     * FIXME: FSP assumes ownership of the APs (Application Processors)
     * upon passing `NULL` pointer to the CpuMpPpi FSP-S UPD.
     * Hence, pass a valid pointer to the CpuMpPpi UPD unconditionally.
     * This would avoid APs from getting hijacked by FSP while coreboot
     * decides to set SkipMpInit UPD.
     */
    s_cfg->CpuMpPpi = (uintptr_t)mp_fill_ppi_services_data();

/*
     * Fill `2nd microcode loading FSP UPD` if FSP is running CPU feature
     * programming.
     */
    if (CONFIG(USE_FSP_FEATURE_PROGRAM_ON_APS))
    	fill_fsps_microcode_params(s_cfg, config);
}

```

https://review.coreboot.org/c/coreboot/+/84552/comment/ff29395a_310bb97c?usp... :
PS11, Line 301: 	max_port = get_max_tcss_port();
              : 	for (i = 0; i < max_port; i++)
```
    for (int i = 0; i < MAX_TYPE_C_PORTS; i++) {
```

https://review.coreboot.org/c/coreboot/+/84552/comment/ee55e2ae_169eefe7?usp... :
PS11, Line 323: 0
`CONFIG_SOC_INTEL_CRASHLOG` ???

https://review.coreboot.org/c/coreboot/+/84552/comment/a061008a_e45eb561?usp... :
PS11, Line 381: 	s_cfg->Device4Enable = is_devfn_enabled(PCI_DEVFN_DPTF);
```
    /* Set TccActivationOffset */
    s_cfg->TccActivationOffset = config->tcc_offset;
```

https://review.coreboot.org/c/coreboot/+/84552/comment/2fdbc9b4_b8b0972c?usp... :
PS11, Line 383: 
don't we need the API below?

```
static void fill_fsps_uart_params(FSP_S_CONFIG *s_cfg,
    	const struct soc_intel_meteorlake_config *config)
{
    ASSERT(ARRAY_SIZE(s_cfg->SerialIoUartAutoFlow) > CONFIG_UART_FOR_CONSOLE);
    s_cfg->SerialIoUartAutoFlow[CONFIG_UART_FOR_CONSOLE] = 0;
}
```

https://review.coreboot.org/c/coreboot/+/84552/comment/3226c1f1_6a9995e0?usp... :
PS11, Line 444: 
              : 	/* Settings per board. */
https://review.coreboot.org/c/coreboot/+/71643

https://review.coreboot.org/c/coreboot/+/84552/comment/72f6ad0b_1a709db0?usp... :
PS11, Line 495: 	s_cfg->Enable8254ClockGating = !CONFIG(USE_LEGACY_8254_TIMER);
              : 	s_cfg->Enable8254ClockGatingOnS3 = !CONFIG(USE_LEGACY_8254_TIMER);
https://review.coreboot.org/c/coreboot/+/70423

https://review.coreboot.org/c/coreboot/+/84552/comment/2e23e90a_af33acc3?usp... :
PS11, Line 511: 
can you please add `fill_fsps_ufs_params`

https://review.coreboot.org/c/coreboot/+/84552/comment/2bbbbe77_a84efb97?usp... :
PS11, Line 536: fill_fsps_misc_power_params
need to port Cls if not, please justify

1. https://review.coreboot.org/c/coreboot/+/69680
2. https://review.coreboot.org/c/coreboot/+/71653
3. https://review.coreboot.org/c/coreboot/+/76826
4. https://review.coreboot.org/c/coreboot/+/78250
5. https://review.coreboot.org/c/coreboot/+/72709
6. https://review.coreboot.org/c/coreboot/+/73061

https://review.coreboot.org/c/coreboot/+/84552/comment/cab3e07d_5510ac97?usp... :
PS11, Line 555: 	/* Fill MIC privacy settings */
does this also applicable when someone choose not to use soundwire interface ?

https://review.coreboot.org/c/coreboot/+/84552/comment/90e6b089_88dcc2c4?usp... :
PS11, Line 564: aix
should be either `iaa` or `iax`

https://review.coreboot.org/c/coreboot/+/84552/comment/7c2adf75_448b2e65?usp... :
PS11, Line 567: Iax
shouldn't this UPD named as `IaaEnable`?

https://review.coreboot.org/c/coreboot/+/84552/comment/e75dca9b_1d1d3033?usp... :
PS11, Line 570: fill_fsps_pch_pm_params
It looks like we didn't have this code in MTL, which makes me think the default update policy was enough to set the correct value?

If we do need to override these values, could you add support for chip config instead of hardcoding it?

https://review.coreboot.org/c/coreboot/+/84552/comment/94eaadeb_4093b37a?usp... :
PS11, Line 602: {
earlier we were allowing mainboard to override ? it's always like this

caller ---> SOC override ---> mainboard override

```
mainboard_update_soc_chip_config
```

https://review.coreboot.org/c/coreboot/+/84552/comment/c712560c_c3eb6654?usp... :
PS11, Line 642: 	/* Override settings per board if required. */
              : 	mainboard_update_soc_chip_config(config);
follow the previous comment.

https://review.coreboot.org/c/coreboot/+/84552/comment/bf55ea5b_58e3f332?usp... :
PS11, Line 683: 	efi_uintn_t logo, blt_size;
              : 	uint32_t logo_size;
              : 
              : 	fsp_convert_bmp_to_gop_blt(&logo, &logo_size,
              : 				   &supd->FspsConfig.BltBufferAddress,
              : 				   &blt_size,
              : 				   &supd->FspsConfig.LogoPixelHeight,
              : 				   &supd->FspsConfig.LogoPixelWidth);
can you please follow to ensure FSP logo ptr and required UPDs are getting filled otherwise FSP won't be able to render the splash screen

```
    fsp_convert_bmp_to_gop_blt(&supd->FspsConfig.LogoPtr,
    		 &supd->FspsConfig.LogoSize,
    		 &supd->FspsConfig.BltBufferAddress,
    		 &supd->FspsConfig.BltBufferSize,
    		 &supd->FspsConfig.LogoPixelHeight,
    		 &supd->FspsConfig.LogoPixelWidth);
```

if those UPDs are not exposed into the FSP header, please ensure to publish it beforehand,

this is hack imo.