Hello Furquan Shaikh, Nick Vaccaro,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39330
to review the following change.
Change subject: soc/intel/tigerlake: add PcieRpAspm and PchPmPciePllSsc ......................................................................
soc/intel/tigerlake: add PcieRpAspm and PchPmPciePllSsc
Expose PcieRpAspm and PchPmPciePllSsc to devicetree.cb to allow overriding settings to disable Aspm and Spread Spectrum from the devicetree.
BUG=b:142961277 BRANCH=master TEST=none
Change-Id: Ib3067811b36f45f44527c2c81be74abf2d40addf Signed-off-by: Nick Vaccaro nvaccaro@google.com Reviewed-on: https://chrome-internal-review.googlesource.com/c/chromeos/third_party/coreb... Reviewed-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_tgl.c 2 files changed, 13 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/39330/1
diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h index e57abe8..523b9991 100644 --- a/src/soc/intel/tigerlake/chip.h +++ b/src/soc/intel/tigerlake/chip.h @@ -112,6 +112,9 @@ uint8_t PchHdaIDispLinkFrequency; uint8_t PchHdaIDispCodecDisconnect;
+ /* Spread Spectrum config */ + uint8_t PchPmPciePllSsc; + /* PCIe Root Ports */ uint8_t PcieRpEnable[CONFIG_MAX_ROOT_PORTS]; /* PCIe output clocks type to PCIe devices. @@ -122,6 +125,9 @@ * clksrc. */ uint8_t PcieClkSrcClkReq[CONFIG_MAX_PCIE_CLOCKS];
+ /* Aspm config */ + uint8_t PcieRpAspm[CONFIG_MAX_ROOT_PORTS]; + /* SMBus */ uint8_t SmbusEnable;
diff --git a/src/soc/intel/tigerlake/fsp_params_tgl.c b/src/soc/intel/tigerlake/fsp_params_tgl.c index 0587b88..19d1e55 100644 --- a/src/soc/intel/tigerlake/fsp_params_tgl.c +++ b/src/soc/intel/tigerlake/fsp_params_tgl.c @@ -44,6 +44,13 @@
for (int i = 0; i < CONFIG_SOC_INTEL_UART_DEV_MAX; i++) params->SerialIoUartMode[i] = config->SerialIoUartMode[i]; + + /* Configure Aspm */ + for (int i = 0; i < ARRAY_SIZE(config->PcieRpAspm); i++) + params->PcieRpAspm[i] = config->PcieRpAspm[i]; + + /* Configure Spread Spectrum */ + params->PchPmPciePllSsc = config->PchPmPciePllSsc; }
static const pci_devfn_t serial_io_dev[] = {
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39330 )
Change subject: soc/intel/tigerlake: add PcieRpAspm and PchPmPciePllSsc ......................................................................
Patch Set 1:
My second upstream patch. I want to see that I have got it right with these two, before attempting to create more.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39330 )
Change subject: soc/intel/tigerlake: add PcieRpAspm and PchPmPciePllSsc ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39330/1/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_tgl.c:
https://review.coreboot.org/c/coreboot/+/39330/1/src/soc/intel/tigerlake/fsp... PS1, Line 48: /* Configure Aspm */ : for (int i = 0; i < ARRAY_SIZE(config->PcieRpAspm); i++) : params->PcieRpAspm[i] = config->PcieRpAspm[i]; : : /* Configure Spread Spectrum */ : params->PchPmPciePllSsc = config->PchPmPciePllSsc; There are some follow-up changes that get rid of these.
Also, I don't think it is correct to set params->PchPmPciePllSsc unconditionally. Its default value is 0xFF and if board does not want to override it, then it should be set to 0xFF. Same with PcieRpAspm.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39330 )
Change subject: soc/intel/tigerlake: add PcieRpAspm and PchPmPciePllSsc ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39330/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39330/1//COMMIT_MSG@10 PS1, Line 10: Aspm ASPM
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39330 )
Change subject: soc/intel/tigerlake: add PcieRpAspm and PchPmPciePllSsc ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39330/1/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/39330/1/src/soc/intel/tigerlake/chi... PS1, Line 128: Aspm ASPM
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39330 )
Change subject: soc/intel/tigerlake: add PcieRpAspm and PchPmPciePllSsc ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39330/1/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_tgl.c:
https://review.coreboot.org/c/coreboot/+/39330/1/src/soc/intel/tigerlake/fsp... PS1, Line 48: /* Configure Aspm */ : for (int i = 0; i < ARRAY_SIZE(config->PcieRpAspm); i++) : params->PcieRpAspm[i] = config->PcieRpAspm[i]; : : /* Configure Spread Spectrum */ : params->PchPmPciePllSsc = config->PchPmPciePllSsc;
There are some follow-up changes that get rid of these. […]
We don't need this change even for ES1. We can use it with FSP default
Jes Klinke has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/39330 )
Change subject: soc/intel/tigerlake: add PcieRpAspm and PchPmPciePllSsc ......................................................................
Abandoned
It seems this CL is not needed in upstream
-
Furquan Shaikh (Code Review) -
Jes Klinke (Code Review)
-
Paul Menzel (Code Review)
-
Wonkyu Kim (Code Review)