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

Jérémy Compostella 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 12:

(19 comments)

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

https://review.coreboot.org/c/coreboot/+/84552/comment/db19fb2e_f046c552?usp=email :
PS11, 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


https://review.coreboot.org/c/coreboot/+/84552/comment/b8e9ecf8_e1dac57b?usp=email :
PS11, Line 118: 		},
> Why did we skipped IRQ assignment for PCIE RP 11 and 12 ? […]
Done


https://review.coreboot.org/c/coreboot/+/84552/comment/66e1f526_6daa0897?usp=email :
PS11, Line 136: 	{
> did we remove ISH intentionally ?
Done


https://review.coreboot.org/c/coreboot/+/84552/comment/ec11268a_384aa58f?usp=email :
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:
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.


https://review.coreboot.org/c/coreboot/+/84552/comment/a24e4917_8dcace20?usp=email :
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.


https://review.coreboot.org/c/coreboot/+/84552/comment/457c3a14_78f16209?usp=email :
PS11, Line 323: 0
> `CONFIG_SOC_INTEL_CRASHLOG` ???
Done


https://review.coreboot.org/c/coreboot/+/84552/comment/3265428a_fbfdbcdd?usp=email :
PS11, 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.


https://review.coreboot.org/c/coreboot/+/84552/comment/8ea1bf9c_1a50df56?usp=email :
PS11, Line 383: 
> don't we need the API below? […]
No we do not, `SerialIoUartAutoFlow` is 0 by default on Panther Lake.


https://review.coreboot.org/c/coreboot/+/84552/comment/2d60e8ec_a431c97e?usp=email :
PS11, Line 444: 
              : 	/* Settings per board. */
> https://review.coreboot. […]
Done


https://review.coreboot.org/c/coreboot/+/84552/comment/b1262e45_3fc22d13?usp=email :
PS11, Line 495: 	s_cfg->Enable8254ClockGating = !CONFIG(USE_LEGACY_8254_TIMER);
              : 	s_cfg->Enable8254ClockGatingOnS3 = !CONFIG(USE_LEGACY_8254_TIMER);
> https://review.coreboot. […]
Done


https://review.coreboot.org/c/coreboot/+/84552/comment/e19fb585_db8fdd0a?usp=email :
PS11, 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.


https://review.coreboot.org/c/coreboot/+/84552/comment/ca8051f7_2c115ce2?usp=email :
PS11, 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


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


https://review.coreboot.org/c/coreboot/+/84552/comment/15fab55e_5dcd0e34?usp=email :
PS11, Line 564: aix
> should be either `iaa` or `iax`
Done


https://review.coreboot.org/c/coreboot/+/84552/comment/763a50e2_f2cb71fb?usp=email :
PS11, 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.


https://review.coreboot.org/c/coreboot/+/84552/comment/c4f6948a_53d31c17?usp=email :
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 en […]
The default being the same than in MTL, I removed this function.


https://review.coreboot.org/c/coreboot/+/84552/comment/32d76a0f_f768246f?usp=email :
PS11, 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()`


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


https://review.coreboot.org/c/coreboot/+/84552/comment/701a64d1_ff073fa2?usp=email :
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' […]
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 https://review.coreboot.org/c/coreboot/+/84552?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Iea26d962748116fa84afdb4afcba1098a64b6989
Gerrit-Change-Number: 84552
Gerrit-PatchSet: 12
Gerrit-Owner: Jérémy Compostella <jeremy.compostella@intel.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal@google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter@mailbox.org>
Gerrit-Reviewer: Pranava Y N <pranavayn@google.com>
Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar@intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik@google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org>
Gerrit-Attention: Subrata Banik <subratabanik@google.com>
Gerrit-Attention: Paul Menzel <paulepanter@mailbox.org>
Gerrit-Attention: Kapil Porwal <kapilporwal@google.com>
Gerrit-Attention: Ronak Kanabar <ronak.kanabar@intel.com>
Gerrit-Attention: Pranava Y N <pranavayn@google.com>
Gerrit-Comment-Date: Mon, 30 Sep 2024 23:41:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik@google.com>