[L] Change in coreboot[main]: soc/intel/pantherlake: Add FSP-S programming
Sept. 30, 2024
11:41 p.m.
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>
410
Age (days ago)
410
Last active (days ago)
0 comments
1 participants
participants (1)
-
Jérémy Compostella (Code Review)