Change in ...coreboot[master]: soc/intel/apl: Add chip.h setting for PCIe ASPM

[Thomas Heijligen (Code Review)]

Hello Nico Huber,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/31351

to review the following change.

Change subject: soc/intel/apl: Add chip.h setting for PCIe ASPM
......................................................................

soc/intel/apl: Add chip.h setting for PCIe ASPM

We don't use a direct mapping to the UPD values so we don't have to set
it to the default `auto` in all devicetrees.

Change-Id: I169722c3c63be66772cb57a429ec7b83230fa234
Signed-off-by: Nico Huber 
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/chip.h
2 files changed, 18 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/31351/1

diff --git a/src/soc/intel/apollolake/chip.c b/src/soc/intel/apollolake/chip.c
index 3634509..cfe8d4a 100644
--- a/src/soc/intel/apollolake/chip.c
+++ b/src/soc/intel/apollolake/chip.c
@@ -669,6 +669,7 @@
 {
    FSP_S_CONFIG *silconfig = &silupd->FspsConfig;
    static struct soc_intel_apollolake_config *cfg;
+	int i;

/* Load VBT before devicetree-specific config. */
    silconfig->GraphicsConfigPtr = (uintptr_t)vbt_get();
@@ -693,6 +694,11 @@
    memcpy(silconfig->PcieRpHotPlug, cfg->pcie_rp_hotplug_enable,
           sizeof(silconfig->PcieRpHotPlug));

+	for (i = 0; i < ARRAY_SIZE(silconfig->PcieRpAspm); ++i) {
+		if (cfg->pcie_rp_aspm[i] != ASPM_IGNORE)
+			silconfig->PcieRpAspm[i] = cfg->pcie_rp_aspm[i] - 1;
+	}
+
    switch (cfg->serirq_mode) {
    case SERIRQ_QUIET:
    	silconfig->SirqEnable = 1;
diff --git a/src/soc/intel/apollolake/chip.h b/src/soc/intel/apollolake/chip.h
index b9e368c..b8f9f8c 100644
--- a/src/soc/intel/apollolake/chip.h
+++ b/src/soc/intel/apollolake/chip.h
@@ -57,6 +57,18 @@
    /* De-emphasis enable configuration for each PCIe root port */
    uint8_t pcie_rp_deemphasis_enable[MAX_PCIE_PORTS];

+	/* ASPM enable setting for each PCIe root port */
+	enum {
+		ASPM_IGNORE = 0,
+		/* Enumeration values below are off-by-one compared to the
+		   UPD to have the default 0 ignore the devicetree setting: */
+		ASPM_DISABLED,
+		ASPM_L0S,
+		ASPM_L1,
+		ASPM_L0SL1,
+		ASPM_AUTO,
+	} pcie_rp_aspm[MAX_PCIE_PORTS];
+
    /* [14:8] DDR mode Number of dealy elements.Each = 125pSec.
     * [6:0] SDR mode Number of dealy elements.Each = 125pSec.
     */

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31351 )

Change subject: soc/intel/apl: Add chip.h setting for PCIe ASPM
......................................................................

Patch Set 1: Code-Review+1

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31351 )

Change subject: soc/intel/apl: Add chip.h setting for PCIe ASPM
......................................................................

Patch Set 1:

I'm actually not sure anymore if we need this sync'ed with FSP.
coreboot can make its own decisions later, it seems.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31351 )

Change subject: soc/intel/apl: Add chip.h setting for PCIe ASPM
......................................................................

Patch Set 3:

Will have to check if this is really needed. If there were
documentation about the UPDs, I could tell: i.e. does this
enable ASPM? or does it only enabled its capabilities? In
the latter, coreboot could handle it on its own.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31351 )

Change subject: soc/intel/apl: Add chip.h setting for PCIe ASPM
......................................................................

Patch Set 3: Code-Review-2
> Will have to check if this is really needed. If there were
> documentation about the UPDs, I could tell: i.e. does this
> enable ASPM? or does it only enabled its capabilities? In
> the latter, coreboot could handle it on its own.

Blocking until I figured this out.

Change in coreboot[master]: soc/intel/apl: Add chip.h setting for PCIe ASPM

[Felix Singer (Code Review)]

Felix Singer has uploaded a new patch set (#4) to the change originally created by Thomas Heijligen. ( https://review.coreboot.org/c/coreboot/+/31351 )

Change subject: soc/intel/apl: Add chip.h setting for PCIe ASPM
......................................................................

soc/intel/apl: Add chip.h setting for PCIe ASPM

We don't use a direct mapping to the UPD values so we don't have to set
it to the default `auto` in all devicetrees.

Change-Id: I169722c3c63be66772cb57a429ec7b83230fa234
Signed-off-by: Nico Huber 
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/chip.h
2 files changed, 18 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/31351/4