Change in coreboot[master]: [WIP] intel/cannonlake: Implement PCIe RP devicetree update

[Nico Huber (Code Review)]

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36651 )

Change subject: [WIP] intel/cannonlake: Implement PCIe RP devicetree update
......................................................................

[WIP] intel/cannonlake: Implement PCIe RP devicetree update

This might need updates to devicetrees that are already patched based on
assumptions about FSP's behaivior.

Change-Id: I4d76f38c222b74053c6a2f80b492d4660ab4db6d
Signed-off-by: Nico Huber 
---
M src/soc/intel/cannonlake/chip.c
1 file changed, 20 insertions(+), 0 deletions(-)

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

diff --git a/src/soc/intel/cannonlake/chip.c b/src/soc/intel/cannonlake/chip.c
index 0ce2f1a..3fd10f8 100644
--- a/src/soc/intel/cannonlake/chip.c
+++ b/src/soc/intel/cannonlake/chip.c
@@ -20,6 +20,7 @@
 #include <intelblocks/acpi.h>
 #include <intelblocks/cfg.h>
 #include <intelblocks/itss.h>
+#include <intelblocks/pcie_rp.h>
 #include <intelblocks/xdci.h>
 #include <romstage_handoff.h>
 #include <soc/intel/common/vbt.h>
@@ -28,6 +29,19 @@

#include "chip.h"

+static const struct pcie_rp_group pch_lp_rp_groups[] = {
+	{ .slot = PCH_DEV_SLOT_PCIE,	.offset = 0, .count = 8 },
+	{ .slot = PCH_DEV_SLOT_PCIE_1,	.offset = 8, .count = 8 },
+	{ 0 }
+};
+
+static const struct pcie_rp_group pch_h_rp_groups[] = {
+	{ .slot = PCH_DEV_SLOT_PCIE,	.offset =  0, .count = 8 },
+	{ .slot = PCH_DEV_SLOT_PCIE_1,	.offset =  8, .count = 8 },
+	{ .slot = PCH_DEV_SLOT_PCIE_2,	.offset = 16, .count = 8 },
+	{ 0 }
+};
+
 #if CONFIG(HAVE_ACPI_TABLES)
 const char *soc_acpi_name(const struct device *dev)
 {
@@ -194,6 +208,12 @@
    cnl_configure_pads(NULL, 0);

soc_fill_gpio_pm_configuration();
+
+	/* swap enabled PCI ports in device tree if needed */
+	if (CONFIG(SOC_INTEL_CANNONLAKE_PCH_H))
+		pcie_rp_update_devicetree(pch_h_rp_groups);
+	else
+		pcie_rp_update_devicetree(pch_lp_rp_groups);
 }

static void pci_domain_set_resources(struct device *dev)

[Nico Huber (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#2).

Change subject: [WIP] intel/cannonlake: Implement PCIe RP devicetree update
......................................................................

[WIP] intel/cannonlake: Implement PCIe RP devicetree update

This might need updates to devicetrees that are already patched based on
assumptions about FSP's behaivior.

Change-Id: I4d76f38c222b74053c6a2f80b492d4660ab4db6d
Signed-off-by: Nico Huber 
---
M src/soc/intel/cannonlake/chip.c
1 file changed, 20 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/36651/2

[Nico Huber (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#3).

Change subject: [WIP] intel/cannonlake: Implement PCIe RP devicetree update
......................................................................

[WIP] intel/cannonlake: Implement PCIe RP devicetree update

This might need updates to devicetrees that are already patched based on
assumptions about FSP's behaivior.

Change-Id: I4d76f38c222b74053c6a2f80b492d4660ab4db6d
Signed-off-by: Nico Huber 
---
M src/soc/intel/cannonlake/chip.c
1 file changed, 20 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/36651/3

[Nico Huber (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#6).

Change subject: [WIP] intel/cannonlake: Implement PCIe RP devicetree update
......................................................................

[WIP] intel/cannonlake: Implement PCIe RP devicetree update

This might need updates to devicetrees that are already patched
based on assumptions about FSP's behavior.

Change-Id: I4d76f38c222b74053c6a2f80b492d4660ab4db6d
Signed-off-by: Nico Huber 
---
M src/soc/intel/cannonlake/chip.c
1 file changed, 20 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/36651/6

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36651 )

Change subject: [WIP] intel/cannonlake: Implement PCIe RP devicetree update
......................................................................

Patch Set 8: Code-Review+1

[Christian Walter (Code Review)]

Christian Walter has uploaded a new patch set (#11) to the change originally created by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/36651 )

Change subject: [WIP] intel/cannonlake: Implement PCIe RP devicetree update
......................................................................

[WIP] intel/cannonlake: Implement PCIe RP devicetree update

This might need updates to devicetrees that are already patched
based on assumptions about FSP's behavior.

Change-Id: I4d76f38c222b74053c6a2f80b492d4660ab4db6d
Signed-off-by: Nico Huber 
---
M src/soc/intel/cannonlake/chip.c
1 file changed, 21 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/36651/11

[Christian Walter (Code Review)]

Christian Walter has uploaded a new patch set (#23) to the change originally created by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/36651 )

Change subject: [WIP] intel/cannonlake: Implement PCIe RP devicetree update
......................................................................

[WIP] intel/cannonlake: Implement PCIe RP devicetree update

This might need updates to devicetrees that are already patched
based on assumptions about FSP's behavior.

Change-Id: I4d76f38c222b74053c6a2f80b492d4660ab4db6d
Signed-off-by: Nico Huber 
---
M src/soc/intel/cannonlake/chip.c
1 file changed, 21 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/36651/23

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36651 )

Change subject: [WIP] intel/cannonlake: Implement PCIe RP devicetree update
......................................................................

Patch Set 23: Code-Review+2

Tested on CFL. Works fine.

Change in coreboot[master]: intel/cannonlake: Implement PCIe RP devicetree update

[Nico Huber (Code Review)]

Hello build bot (Jenkins), Christian Walter, Angel Pons, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#24).

Change subject: intel/cannonlake: Implement PCIe RP devicetree update
......................................................................

intel/cannonlake: Implement PCIe RP devicetree update

Some existing devicetrees were manually adapted to anticipate
root-port switching. Now, their PCI-device on/off settings should
just reflect the `PcieRpEnable` state and configuration happens
on the PCI function that was assigned at reset.

Change-Id: I4d76f38c222b74053c6a2f80b492d4660ab4db6d
Signed-off-by: Nico Huber 
---
M src/mainboard/google/hatch/variants/duffy/overridetree.cb
M src/mainboard/google/hatch/variants/kaisa/overridetree.cb
M src/mainboard/google/hatch/variants/puff/overridetree.cb
M src/mainboard/google/sarien/variants/arcada/devicetree.cb
M src/mainboard/system76/lemp9/devicetree.cb
M src/soc/intel/cannonlake/chip.c
6 files changed, 30 insertions(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/36651/24

Change in coreboot[master]: intel/cannonlake: Implement PCIe RP devicetree update

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36651 )

Change subject: intel/cannonlake: Implement PCIe RP devicetree update
......................................................................

Patch Set 24: Code-Review+1

Change in coreboot[master]: intel/cannonlake: Implement PCIe RP devicetree update

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36651 )

Change subject: intel/cannonlake: Implement PCIe RP devicetree update
......................................................................

Patch Set 24:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36651/24/src/mainboard/google/hatch... 
File src/mainboard/google/hatch/variants/duffy/overridetree.cb:

https://review.coreboot.org/c/coreboot/+/36651/24/src/mainboard/google/hatch... 
PS24, Line 297: 		device pci 1c.6 on
Earlier we always had to keep the function 0 enabled even if it was unused and SoC supported the RP swap. Is that not required anymore?

Change in coreboot[master]: intel/cannonlake: Implement PCIe RP devicetree update

[Nico Huber (Code Review)]

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

Change subject: intel/cannonlake: Implement PCIe RP devicetree update
......................................................................

Patch Set 24:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36651/24/src/mainboard/google/hatch... 
File src/mainboard/google/hatch/variants/duffy/overridetree.cb:

https://review.coreboot.org/c/coreboot/+/36651/24/src/mainboard/google/hatch... 
PS24, Line 297: 		device pci 1c.6 on
> SoC supported the RP swap.

What do you mean exactly? HW support? FSP support? coreboot support?

The history, AFAIK it, is roughly: before Sandy Bridge, we ignored the topic. From
Sandy to FSP, we used the original (reset state) function numbers in the devicetree.
coreboot was responsible to patch things after it switched ports. Starting with FSP,
things got messy. FSP has its own mind about switching ports (can be because we
asked it to disable function 0, but can also be that FSP decided to disable it
because no downstream device was detected). Skylake got some code to patch the
devicetree (which almost worked correctly), but other FSP platform didn't.

This new solution for FSP platforms also uses the original function numbers in the
devicetree. It patches everything before PCI enumeration. So that should have no
special requirements on the devicetree. The older, coreboot-native solutions had
the requirement to list function 0 in the devicetree. But we used the devicetree
on/off to decide if it should be disabled, so an `off` must have been valid.

Change in coreboot[master]: intel/cannonlake: Implement PCIe RP devicetree update

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36651 )

Change subject: intel/cannonlake: Implement PCIe RP devicetree update
......................................................................

Patch Set 24: Code-Review+2

Change in coreboot[master]: intel/cannonlake: Implement PCIe RP devicetree update

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36651 )

Change subject: intel/cannonlake: Implement PCIe RP devicetree update
......................................................................

Patch Set 24: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/36651/24/src/mainboard/google/hatch... 
File src/mainboard/google/hatch/variants/duffy/overridetree.cb:

https://review.coreboot.org/c/coreboot/+/36651/24/src/mainboard/google/hatch... 
PS24, Line 297: 		device pci 1c.6 on
> What do you mean exactly? HW support? FSP support? coreboot support?

I meant coreboot support like `pcie_rp_update_devicetree`.

Thanks for the background and explanation. I looked through the code and it looks like it should be fine to actually have func0 set to off as you outlined -- at least I don't see a path which would behave differently if that was the case.

Change in coreboot[master]: intel/cannonlake: Implement PCIe RP devicetree update

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36651 )

Change subject: intel/cannonlake: Implement PCIe RP devicetree update
......................................................................

Patch Set 24: Code-Review+1

Looks good, can't test

Change in coreboot[master]: intel/cannonlake: Implement PCIe RP devicetree update

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36651 )

Change subject: intel/cannonlake: Implement PCIe RP devicetree update
......................................................................

intel/cannonlake: Implement PCIe RP devicetree update

Some existing devicetrees were manually adapted to anticipate
root-port switching. Now, their PCI-device on/off settings should
just reflect the `PcieRpEnable` state and configuration happens
on the PCI function that was assigned at reset.

Change-Id: I4d76f38c222b74053c6a2f80b492d4660ab4db6d
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/36651
Tested-by: build bot (Jenkins) 
Reviewed-by: Edward O'Callaghan 
Reviewed-by: Patrick Rudolph 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Angel Pons 
---
M src/mainboard/google/hatch/variants/duffy/overridetree.cb
M src/mainboard/google/hatch/variants/kaisa/overridetree.cb
M src/mainboard/google/hatch/variants/puff/overridetree.cb
M src/mainboard/google/sarien/variants/arcada/devicetree.cb
M src/mainboard/system76/lemp9/devicetree.cb
M src/soc/intel/cannonlake/chip.c
6 files changed, 30 insertions(+), 13 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Patrick Rudolph: Looks good to me, approved
  Angel Pons: Looks good to me, but someone else must approve
  Edward O'Callaghan: Looks good to me, but someone else must approve

diff --git a/src/mainboard/google/hatch/variants/duffy/overridetree.cb b/src/mainboard/google/hatch/variants/duffy/overridetree.cb
index d7acbd7..4e7692c 100644
--- a/src/mainboard/google/hatch/variants/duffy/overridetree.cb
+++ b/src/mainboard/google/hatch/variants/duffy/overridetree.cb
@@ -294,7 +294,7 @@
    		end
    	end #I2C #4
    	device pci 1a.0 on  end # eMMC
-		device pci 1c.0 on
+		device pci 1c.6 on
    		chip drivers/net
    			register "customized_leds" = "0x05af"
    			register "wake" = "GPE0_DW1_07" # GPP_C7
@@ -305,8 +305,7 @@
    			register "device_index" = "0"
    			device pci 00.0 on end
    		end
-		end # FSP requires func0 be enabled.
-		device pci 1c.6 on  end # RTL8111H Ethernet NIC (becomes RP1).
+		end # RTL8111H Ethernet NIC
    	device pci 1d.2 on end # PCI Express Port 11 (X2 NVMe)
    	device pci 1e.3 off end # GSPI #1
    end
diff --git a/src/mainboard/google/hatch/variants/kaisa/overridetree.cb b/src/mainboard/google/hatch/variants/kaisa/overridetree.cb
index f5e85bd..c7655ac 100644
--- a/src/mainboard/google/hatch/variants/kaisa/overridetree.cb
+++ b/src/mainboard/google/hatch/variants/kaisa/overridetree.cb
@@ -294,7 +294,7 @@
    		end
    	end #I2C #4
    	device pci 1a.0 on  end # eMMC
-		device pci 1c.0 on
+		device pci 1c.6 on
    		chip drivers/net
    			register "customized_leds" = "0x05af"
    			register "wake" = "GPE0_DW1_07" # GPP_C7
@@ -305,8 +305,7 @@
    			register "device_index" = "0"
    			device pci 00.0 on end
    		end
-		end # FSP requires func0 be enabled.
-		device pci 1c.6 on  end # RTL8111H Ethernet NIC (becomes RP1).
+		end # RTL8111H Ethernet NIC
    	device pci 1d.2 on end # PCI Express Port 11 (X2 NVMe)
    	device pci 1e.3 off end # GSPI #1
    end
diff --git a/src/mainboard/google/hatch/variants/puff/overridetree.cb b/src/mainboard/google/hatch/variants/puff/overridetree.cb
index 31efc4a..83fcf9a 100644
--- a/src/mainboard/google/hatch/variants/puff/overridetree.cb
+++ b/src/mainboard/google/hatch/variants/puff/overridetree.cb
@@ -297,7 +297,7 @@
    		end
    	end #I2C #4
    	device pci 1a.0 on  end # eMMC
-		device pci 1c.0 on
+		device pci 1c.6 on
    		chip drivers/net
    			register "customized_leds" = "0x05af"
    			register "wake" = "GPE0_DW1_07" # GPP_C7
@@ -308,8 +308,7 @@
    			register "device_index" = "0"
    			device pci 00.0 on end
    		end
-		end # FSP requires func0 be enabled.
-		device pci 1c.6 on  end # RTL8111H Ethernet NIC (becomes RP1).
+		end # RTL8111H Ethernet NIC
    	device pci 1d.2 on end # PCI Express Port 11 (X2 NVMe)
    	device pci 1e.3 off end # GSPI #1
    end
diff --git a/src/mainboard/google/sarien/variants/arcada/devicetree.cb b/src/mainboard/google/sarien/variants/arcada/devicetree.cb
index 6bc3df1..d1d9b03 100644
--- a/src/mainboard/google/sarien/variants/arcada/devicetree.cb
+++ b/src/mainboard/google/sarien/variants/arcada/devicetree.cb
@@ -373,10 +373,10 @@
    	device pci 1c.5 off end # PCI Express Port 6
    	device pci 1c.6 off end # PCI Express Port 7
    	device pci 1c.7 off end # PCI Express Port 8
-		device pci 1d.0 on
+		device pci 1d.0 off end # PCI Express Port 9
+		device pci 1d.1 on
    		smbios_slot_desc "SlotTypeM2Socket1_SD" "SlotLengthOther" "2230" "SlotDataBusWidth1X"
-		end # PCI Express Port 9
-		device pci 1d.1 on  end # PCI Express Port 10
+		end # PCI Express Port 10
    	device pci 1d.2 on  end # PCI Express Port 11
    	device pci 1d.3 off end # PCI Express Port 12
    	device pci 1d.4 on
diff --git a/src/mainboard/system76/lemp9/devicetree.cb b/src/mainboard/system76/lemp9/devicetree.cb
index 3fa2c17..6cf0fff 100644
--- a/src/mainboard/system76/lemp9/devicetree.cb
+++ b/src/mainboard/system76/lemp9/devicetree.cb
@@ -207,7 +207,7 @@
    	device pci 19.1 off end # I2C #5
    	device pci 19.2 on  end # UART #2
    	device pci 1a.0 off end # eMMC
-		device pci 1c.0 on  end # PCI Express Port 1
+		device pci 1c.0 off end # PCI Express Port 1
    	device pci 1c.1 off end # PCI Express Port 2
    	device pci 1c.2 off end # PCI Express Port 3
    	device pci 1c.3 off end # PCI Express Port 4
diff --git a/src/soc/intel/cannonlake/chip.c b/src/soc/intel/cannonlake/chip.c
index 51678ad..ef85215 100644
--- a/src/soc/intel/cannonlake/chip.c
+++ b/src/soc/intel/cannonlake/chip.c
@@ -7,6 +7,7 @@
 #include <intelblocks/acpi.h>
 #include <intelblocks/cfg.h>
 #include <intelblocks/itss.h>
+#include <intelblocks/pcie_rp.h>
 #include <intelblocks/xdci.h>
 #include <romstage_handoff.h>
 #include <soc/intel/common/vbt.h>
@@ -16,6 +17,19 @@

#include "chip.h"

+static const struct pcie_rp_group pch_lp_rp_groups[] = {
+	{ .slot = PCH_DEV_SLOT_PCIE,	.count = 8 },
+	{ .slot = PCH_DEV_SLOT_PCIE_1,	.count = 8 },
+	{ 0 }
+};
+
+static const struct pcie_rp_group pch_h_rp_groups[] = {
+	{ .slot = PCH_DEV_SLOT_PCIE,	.count = 8 },
+	{ .slot = PCH_DEV_SLOT_PCIE_1,	.count = 8 },
+	{ .slot = PCH_DEV_SLOT_PCIE_2,	.count = 8 },
+	{ 0 }
+};
+
 #if CONFIG(HAVE_ACPI_TABLES)
 const char *soc_acpi_name(const struct device *dev)
 {
@@ -166,6 +180,12 @@
    cnl_configure_pads(NULL, 0);

soc_gpio_pm_configuration();
+
+	/* swap enabled PCI ports in device tree if needed */
+	if (CONFIG(SOC_INTEL_CANNONLAKE_PCH_H))
+		pcie_rp_update_devicetree(pch_h_rp_groups);
+	else
+		pcie_rp_update_devicetree(pch_lp_rp_groups);
 }

static struct device_operations pci_domain_ops = {

Change in coreboot[master]: intel/cannonlake: Implement PCIe RP devicetree update

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36651 )

Change subject: intel/cannonlake: Implement PCIe RP devicetree update
......................................................................

Patch Set 25:

Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4
Emulation targets:
"QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/4027
"QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4026
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4025
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/4024

Please note: This test is under development and might not be accurate at all!