Attention is currently required from: Kapil Porwal, Paul Menzel, Pranava Y N, Ronak Kanabar, Subrata Banik.

View Change

19 comments:

File src/soc/intel/pantherlake/fsp_params.c:
- Patch Set #11, Line 102: ANY_PIRQ
  any reason for not using fixed IRQ for DPTF ?
  I checked the EDS vol 2 and it turns out that similarily to Meteor Lake, the The Dynamic Tuning Technology (DTT) device IRQ is not programmable and is INT_A/PIRQ_A (IRQ 16).
  I updated the code accordingly
- Patch Set #11, Line 118: },
  Why did we skipped IRQ assignment for PCIE RP 11 and 12 ? […]
  Done
- Patch Set #11, Line 136: {
  did we remove ISH intentionally ?
  Done
- Patch Set #11, 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:
1. `SkipMpInit` UPD does not exist anymore so the comment and the "workaround" do not mean anything anymore.
2. 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.
- Patch Set #11, 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.
- Patch Set #11, Line 323: 0
  `CONFIG_SOC_INTEL_CRASHLOG` ???
  Done
- Patch Set #11, Line 381: s_cfg->Device4Enable = is_devfn_enabled(PCI_DEVFN_DPTF);
``` […]
This is a FSP-M UPD parameter now and it is being set already.
- Patch Set #11, Line 383:
  don't we need the API below? […]
  No we do not, `SerialIoUartAutoFlow` is 0 by default on Panther Lake.
- Patch Set #11, Line 444:
```
	/* Settings per board. */
```
  https://review.coreboot. […]
  Done
- Patch Set #11, Line 495:
```
	s_cfg->Enable8254ClockGating = !CONFIG(USE_LEGACY_8254_TIMER);
	s_cfg->Enable8254ClockGatingOnS3 = !CONFIG(USE_LEGACY_8254_TIMER);
```
  https://review.coreboot. […]
  Done
- Patch Set #11, Line 511:
  can you please add `fill_fsps_ufs_params`
  In the spirit of https://review.coreboot.org/c/coreboot/+/83635/comment/6fa5d845_bbe0dcd7/ UFS is just not supported yet. UFS UPDs defaulting to not enabled, there is nothing to do at the moment.
- Patch Set #11, Line 536: fill_fsps_misc_power_params
  need to port Cls if not, please justify […]
  - [69680 soc/intel/meteorlake: Skip setting D0I3 bit for HECI devices](https://review.coreboot.org/c/coreboot/+/69680): Done
  - [71653 soc/intel/meteorlake: Set max Pkg C-states to Auto](https://review.coreboot.org/c/coreboot/+/71653): `PkgCStateLimit` default is is already Auto (0xff)
  - [76826 soc/intel/meteorlake: Hook up UPD for C1 C-state auto-demotion](https://review.coreboot.org/c/coreboot/+/76826): Done
  - [78250 mb/google/{rex,ovis}: Disable package C-state auto demotion](https://review.coreboot.org/c/coreboot/+/78250): Added the support. Use to be implemented at the board level based on PnP analysis (outside the scope of this patch).
  - [72709 soc/intel/meteorlake: Enable V1p05-PHY supply external FET control](https://review.coreboot.org/c/coreboot/+/72709): Done
  - [73061 soc/intel/meteorlake: Add PM Energy Report feature option](https://review.coreboot.org/c/coreboot/+/73061): Done
- Patch Set #11, Line 555: /* Fill MIC privacy settings */
  does this also applicable when someone choose not to use soundwire interface ?
  Done
- Patch Set #11, Line 564: aix
  should be either `iaa` or `iax`
  Done
- Patch Set #11, Line 567: Iax
  shouldn't this UPD named as `IaaEnable`?
  IAX is an early name for IAA and I assume the UPD kept the early name.
- Patch Set #11, 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 en […]
  The default being the same than in MTL, I removed this function.
- Patch Set #11, Line 602: {
  earlier we were allowing mainboard to override ? it's always like this […]
  We still do, we do it in `platform_fsp_silicon_init_params_cb()` which is the function calling all the UPD setter main functions like `mainboard_silicon_init_params()` or `soc_silicon_init_params()` and therefore it makes more than having it hidden at the beginning of `soc_silicon_init_params()`
- Patch Set #11, Line 642:
```
	/* Override settings per board if required. */
	mainboard_update_soc_chip_config(config);
```
  follow the previous comment.
  Done
- Patch Set #11, 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' […]
  We already verified it with the FSP team and also verified it works a few weeks ago. **This is not a hack.** Most of those UPD are not necessary anymore and therefore do exist anymore.

To view, visit change 84552. To unsubscribe, or for help writing mail filters, visit settings.