Change in coreboot[master]: mainboard/ocp/monolake: Hide NIC IIO root ports earlier

[Andrey Petrov (Code Review)]

Andrey Petrov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35968 )

Change subject: mainboard/ocp/monolake: Hide NIC IIO root ports earlier
......................................................................

mainboard/ocp/monolake: Hide NIC IIO root ports earlier

It turned on some SKUs FSP hangs in Notify stage if IIO root ports are
disabled after MemoryInit. Because of that explicitly hide in romstage.

TEST=the patch was ran on affected HW and success was reported

Change-Id: I6a2a405f729df14f46bcf34a24e66e8ba9415f9d
Signed-off-by: Andrey Petrov 
---
M src/mainboard/ocp/monolake/devicetree.cb
M src/mainboard/ocp/monolake/romstage.c
2 files changed, 9 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/35968/1

diff --git a/src/mainboard/ocp/monolake/devicetree.cb b/src/mainboard/ocp/monolake/devicetree.cb
index 26c95d5..6a8bef1 100644
--- a/src/mainboard/ocp/monolake/devicetree.cb
+++ b/src/mainboard/ocp/monolake/devicetree.cb
@@ -4,8 +4,6 @@
    end
    device domain 0 on
    	device pci 00.0 on end # SoC router
-		device pci 02.2 off end # IOU0 port C, 10GbE
-		device pci 02.3 off end # IOU0 port D, 10GbE
    	device pci 14.0 on end # xHCI Controller
    	device pci 19.0 on end # Gigabit LAN Controller
    	device pci 1d.0 on end # EHCI Controller
diff --git a/src/mainboard/ocp/monolake/romstage.c b/src/mainboard/ocp/monolake/romstage.c
index ef41b77..4a1a235 100644
--- a/src/mainboard/ocp/monolake/romstage.c
+++ b/src/mainboard/ocp/monolake/romstage.c
@@ -26,7 +26,7 @@
 #include <soc/pci_devs.h>
 #include <soc/lpc.h>
 #include <soc/gpio.h>
-
+#include <soc/ubox.h>

/* Define the strings for UPD variables that could be customized */
 #define FSP_VAR_HYPERTHREADING "HyperThreading"
@@ -207,6 +207,14 @@
    	printk(BIOS_EMERG, "Detected broken platform state. Issuing full reset\n");
    	full_reset();
    }
+
+	/*
+	 * Explicitly hide internal root port IIO devices that GbE device is connected to.
+	 * We can't use devicetree for this since FSP seemingly gets confused if we hide
+	 * after MemoryInit API was called.
+	 */
+	iio_hide(PCIE_IIO_PORT_2_DEV, PCIE_IIO_PORT_2C_FUNC);
+	iio_hide(PCIE_IIO_PORT_2_DEV, PCIE_IIO_PORT_2D_FUNC);
 }

/**

[Patrick Rudolph (Code Review)]

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

Change subject: mainboard/ocp/monolake: Hide NIC IIO root ports earlier
......................................................................

Patch Set 1:

Can't you use the same (ramstage) code that reads the devicetree? If the device is marked disabled there, hide the device in SoC romstage.

Change in coreboot[master]: mainboard/ocp/monolake: Hide IIO root ports before memory init

[Andrey Petrov (Code Review)]

Hello Patrick Rudolph, Jonathan Zhang, Matt DeVillier, Johnny Lin, David Hendricks, Philipp Deppenwiese, build bot (Jenkins),

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

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

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

Change subject: mainboard/ocp/monolake: Hide IIO root ports before memory init
......................................................................

mainboard/ocp/monolake: Hide IIO root ports before memory init

It turned on some SKUs FSP hangs in Notify stage if IIO root ports are
disabled after MemoryInit. To address that hide IIO root ports earlier
in romstage.

TEST=the patch was ran on affected HW and success was reported

Change-Id: I6a2a405f729df14f46bcf34a24e66e8ba9415f9d
Signed-off-by: Andrey Petrov 
---
M src/mainboard/ocp/monolake/romstage.c
M src/soc/intel/fsp_broadwell_de/include/soc/ubox.h
M src/soc/intel/fsp_broadwell_de/romstage/romstage.c
M src/soc/intel/fsp_broadwell_de/southcluster.c
M src/soc/intel/fsp_broadwell_de/ubox.c
5 files changed, 44 insertions(+), 23 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/35968/2

Change in coreboot[master]: mainboard/ocp/monolake: Hide IIO root ports before memory init

[Andrey Petrov (Code Review)]

Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35968 )

Change subject: mainboard/ocp/monolake: Hide IIO root ports before memory init
......................................................................

Patch Set 2:
> Patch Set 1:
>
> Can't you use the same (ramstage) code that reads the devicetree? If the device is marked disabled there, hide the device in SoC romstage.

I adjusted the code to go through root ports at romstage and hide ports that are turned off in devicetree

Change in coreboot[master]: mainboard/ocp/monolake: Hide IIO root ports before memory init

[Patrick Rudolph (Code Review)]

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

Change subject: mainboard/ocp/monolake: Hide IIO root ports before memory init
......................................................................

Patch Set 2: Code-Review+1

(3 comments)

https://review.coreboot.org/c/coreboot/+/35968/2/src/soc/intel/fsp_broadwell... 
File src/soc/intel/fsp_broadwell_de/romstage/romstage.c:

https://review.coreboot.org/c/coreboot/+/35968/2/src/soc/intel/fsp_broadwell... 
PS2, Line 119: .
:

https://review.coreboot.org/c/coreboot/+/35968/2/src/soc/intel/fsp_broadwell... 
PS2, Line 158: Call
Move below early_iio_hide()

https://review.coreboot.org/c/coreboot/+/35968/2/src/soc/intel/fsp_broadwell... 
PS2, Line 163: early_iio_hide
Add comment explaining why it's done here.

Change in coreboot[master]: mainboard/ocp/monolake: Hide IIO root ports before memory init

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35968 )

Change subject: mainboard/ocp/monolake: Hide IIO root ports before memory init
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/35968/2/src/mainboard/ocp/monolake/... 
File src/mainboard/ocp/monolake/romstage.c:

https://review.coreboot.org/c/coreboot/+/35968/2/src/mainboard/ocp/monolake/... 
PS2, Line 29: include <soc/ubox.h>
Is this change related to the described patch? Or do you just clean it up here?
Seems like nothing else was changed here so why this include?

https://review.coreboot.org/c/coreboot/+/35968/2/src/soc/intel/fsp_broadwell... 
File src/soc/intel/fsp_broadwell_de/romstage/romstage.c:

https://review.coreboot.org/c/coreboot/+/35968/2/src/soc/intel/fsp_broadwell... 
PS2, Line 119: 0
You could use here %d and then provide BUS0 as parameter to be consistent. But this is just cosmetics

Change in coreboot[master]: mainboard/ocp/monolake: Hide IIO root ports before memory init

[Patrick Rudolph (Code Review)]

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

Change subject: mainboard/ocp/monolake: Hide IIO root ports before memory init
......................................................................

Patch Set 2:

Please also update Documentation/soc/intel/fsp/index.md with the bug you found.

Change in coreboot[master]: mainboard/ocp/monolake: Hide IIO root ports before memory init

[Andrey Petrov (Code Review)]

Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35968 )

Change subject: mainboard/ocp/monolake: Hide IIO root ports before memory init
......................................................................

Patch Set 2:

(5 comments)

https://review.coreboot.org/c/coreboot/+/35968/2/src/mainboard/ocp/monolake/... 
File src/mainboard/ocp/monolake/romstage.c:

https://review.coreboot.org/c/coreboot/+/35968/2/src/mainboard/ocp/monolake/... 
PS2, Line 29: include <soc/ubox.h>
> Is this change related to the described patch? Or do you just clean it up here? […]

Ack

https://review.coreboot.org/c/coreboot/+/35968/2/src/soc/intel/fsp_broadwell... 
File src/soc/intel/fsp_broadwell_de/romstage/romstage.c:

https://review.coreboot.org/c/coreboot/+/35968/2/src/soc/intel/fsp_broadwell... 
PS2, Line 119: 0
> You could use here %d and then provide BUS0 as parameter to be consistent. […]

Ack

https://review.coreboot.org/c/coreboot/+/35968/2/src/soc/intel/fsp_broadwell... 
PS2, Line 119: .
> :

Ack

https://review.coreboot.org/c/coreboot/+/35968/2/src/soc/intel/fsp_broadwell... 
PS2, Line 158: Call
> Move below early_iio_hide()

Ack

https://review.coreboot.org/c/coreboot/+/35968/2/src/soc/intel/fsp_broadwell... 
PS2, Line 163: early_iio_hide
> Add comment explaining why it's done here.

Ack

Change in coreboot[master]: mainboard/ocp/monolake: Hide IIO root ports before memory init

[Andrey Petrov (Code Review)]

Hello Werner Zeh, Patrick Rudolph, Patrick Rudolph, Jonathan Zhang, Matt DeVillier, Johnny Lin, David Hendricks, Philipp Deppenwiese, build bot (Jenkins),

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

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

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

Change subject: mainboard/ocp/monolake: Hide IIO root ports before memory init
......................................................................

mainboard/ocp/monolake: Hide IIO root ports before memory init

It turned on some SKUs FSP hangs in Notify stage if IIO root ports are
disabled after MemoryInit. To address that hide IIO root ports earlier
in romstage.

TEST=the patch was ran on affected HW and success was reported

Change-Id: I6a2a405f729df14f46bcf34a24e66e8ba9415f9d
Signed-off-by: Andrey Petrov 
---
M Documentation/soc/intel/fsp/index.md
M src/soc/intel/fsp_broadwell_de/include/soc/ubox.h
M src/soc/intel/fsp_broadwell_de/romstage/romstage.c
M src/soc/intel/fsp_broadwell_de/southcluster.c
M src/soc/intel/fsp_broadwell_de/ubox.c
5 files changed, 52 insertions(+), 24 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/35968/3

Change in coreboot[master]: mainboard/ocp/monolake: Hide IIO root ports before memory init

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35968 )

Change subject: mainboard/ocp/monolake: Hide IIO root ports before memory init
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: mainboard/ocp/monolake: Hide IIO root ports before memory init

[Patrick Georgi (Code Review)]

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

Change subject: mainboard/ocp/monolake: Hide IIO root ports before memory init
......................................................................

mainboard/ocp/monolake: Hide IIO root ports before memory init

It turned on some SKUs FSP hangs in Notify stage if IIO root ports are
disabled after MemoryInit. To address that hide IIO root ports earlier
in romstage.

TEST=the patch was ran on affected HW and success was reported

Change-Id: I6a2a405f729df14f46bcf34a24e66e8ba9415f9d
Signed-off-by: Andrey Petrov 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35968
Tested-by: build bot (Jenkins) 
Reviewed-by: Werner Zeh 
---
M Documentation/soc/intel/fsp/index.md
M src/soc/intel/fsp_broadwell_de/include/soc/ubox.h
M src/soc/intel/fsp_broadwell_de/romstage/romstage.c
M src/soc/intel/fsp_broadwell_de/southcluster.c
M src/soc/intel/fsp_broadwell_de/ubox.c
5 files changed, 52 insertions(+), 24 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Werner Zeh: Looks good to me, approved

diff --git a/Documentation/soc/intel/fsp/index.md b/Documentation/soc/intel/fsp/index.md
index cd7fe0b..aac7b35 100644
--- a/Documentation/soc/intel/fsp/index.md
+++ b/Documentation/soc/intel/fsp/index.md
@@ -21,6 +21,12 @@
   * Workaround: Don't disable this PCI device
   * Issue on public tracker: [Issue 13]

+* FSP Notify(EnumInitPhaseAfterPciEnumeration) hangs if 00:02.03/00:02.03 are hidden
+  * Release MR2
+  * Seems to get stuck on some SKUs only if hidden after MemoryInit
+  * Workaround: Hide before MemoryInit
+  * Issue on public tracker: [Issue 35]
+
 ### KabylakeFsp
 * MfgId and ModulePartNum in the DIMM_INFO struct are empty
   * Release 3.7.1
@@ -59,4 +65,5 @@
 [Issue 13]: https://github.com/IntelFsp/FSP/issues/13
 [Issue 15]: https://github.com/IntelFsp/FSP/issues/15
 [Issue 22]: https://github.com/IntelFsp/FSP/issues/22
+[Issue 35]: https://github.com/IntelFsp/FSP/issues/35

diff --git a/src/soc/intel/fsp_broadwell_de/include/soc/ubox.h b/src/soc/intel/fsp_broadwell_de/include/soc/ubox.h
index baaeac5..3c2e6f5 100644
--- a/src/soc/intel/fsp_broadwell_de/include/soc/ubox.h
+++ b/src/soc/intel/fsp_broadwell_de/include/soc/ubox.h
@@ -40,6 +40,5 @@

#define UBOX_DEVHIDE0			0xb0

-void iio_hide(const uint8_t devno, const uint8_t funcno);
-
+void iio_hide(DEVTREE_CONST struct device *dev);
 #endif
diff --git a/src/soc/intel/fsp_broadwell_de/romstage/romstage.c b/src/soc/intel/fsp_broadwell_de/romstage/romstage.c
index b0fad3f..8438b10 100644
--- a/src/soc/intel/fsp_broadwell_de/romstage/romstage.c
+++ b/src/soc/intel/fsp_broadwell_de/romstage/romstage.c
@@ -94,6 +94,37 @@
    pci_mmio_write_config32(ubox_dev, UBOX_UART_ENABLE, ubox_uart_en);
 }

+static void early_iio_hide(void)
+{
+	DEVTREE_CONST struct device *dev;
+
+	const pci_devfn_t iio_rootport[] = {
+		PCI_DEVFN(PCIE_IIO_PORT_1_DEV, PCIE_IIO_PORT_1A_FUNC),
+		PCI_DEVFN(PCIE_IIO_PORT_1_DEV, PCIE_IIO_PORT_1B_FUNC),
+		PCI_DEVFN(PCIE_IIO_PORT_2_DEV, PCIE_IIO_PORT_2A_FUNC),
+		PCI_DEVFN(PCIE_IIO_PORT_2_DEV, PCIE_IIO_PORT_2B_FUNC),
+		PCI_DEVFN(PCIE_IIO_PORT_2_DEV, PCIE_IIO_PORT_2C_FUNC),
+		PCI_DEVFN(PCIE_IIO_PORT_2_DEV, PCIE_IIO_PORT_2D_FUNC),
+		PCI_DEVFN(PCIE_IIO_PORT_3_DEV, PCIE_IIO_PORT_3A_FUNC),
+		PCI_DEVFN(PCIE_IIO_PORT_3_DEV, PCIE_IIO_PORT_3B_FUNC),
+		PCI_DEVFN(PCIE_IIO_PORT_3_DEV, PCIE_IIO_PORT_3C_FUNC),
+		PCI_DEVFN(PCIE_IIO_PORT_3_DEV, PCIE_IIO_PORT_3D_FUNC),
+		};
+
+	/* Walk through IIO root ports and hide if it is disabled in devtree */
+	for (int i = 0; i < ARRAY_SIZE(iio_rootport); i++) {
+		dev = pcidev_path_on_bus(BUS0, iio_rootport[i]);
+		if (dev && !dev->enabled) {
+			printk(BIOS_DEBUG, "Hiding IIO root port: %d:%d.%d\n",
+					BUS0,
+					PCI_SLOT(iio_rootport[i]),
+					PCI_FUNC(iio_rootport[i]));
+			iio_hide(dev);
+		}
+	}
+
+}
+
 /* Entry from cache-as-ram.inc. */
 void *asmlinkage main(FSP_INFO_HEADER *fsp_info_header)
 {
@@ -121,14 +152,15 @@
    init_rtc();
    setup_gpio_io_address();

+	/* Hide before MemoryInit since hiding later seems to break FSP */
+	early_iio_hide();
    timestamp_add_now(TS_BEFORE_INITRAM);
-
+	post_code(0x48);
    /*
     * Call early init to initialize memory and chipset. This function returns
     * to the romstage_main_continue function with a pointer to the HOB
     * structure.
     */
-	post_code(0x48);
    printk(BIOS_DEBUG, "Starting the Intel FSP (early_init)\n");
    fsp_early_init(fsp_info_header);
    die_with_post_code(POST_INVALID_VENDOR_BINARY,
diff --git a/src/soc/intel/fsp_broadwell_de/southcluster.c b/src/soc/intel/fsp_broadwell_de/southcluster.c
index d1981fd..fb8af87 100644
--- a/src/soc/intel/fsp_broadwell_de/southcluster.c
+++ b/src/soc/intel/fsp_broadwell_de/southcluster.c
@@ -257,24 +257,11 @@
    const int slot = PCI_SLOT(dev->path.pci.devfn);
    const int func = PCI_FUNC(dev->path.pci.devfn);

-	switch (slot) {
-	case PCIE_IIO_PORT_0_DEV:
-		die("should not hide PCH link");
-	case PCIE_IIO_PORT_1_DEV: /* fallthrough */
-	case PCIE_IIO_PORT_2_DEV: /* fallthrough */
-	case PCIE_IIO_PORT_3_DEV: /* fallthrough */
-		printk(BIOS_DEBUG, "%s: Disabling IOU bridge %02x.%01x\n", dev_path(dev), slot,
-			func);
-		iio_hide(slot, func);
-		break;
-	default:
-		printk(BIOS_DEBUG, "%s: Disabling device: %02x.%01x\n", dev_path(dev), slot,
-			func);
-		/* Ensure memory, io, and bus master are all disabled */
-		reg32 = pci_read_config32(dev, PCI_COMMAND);
-		reg32 &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO);
-		pci_write_config32(dev, PCI_COMMAND, reg32);
-	}
+	printk(BIOS_DEBUG, "%s: Disabling device: %02x.%01x\n", dev_path(dev), slot, func);
+	/* Ensure memory, io, and bus master are all disabled */
+	reg32 = pci_read_config32(dev, PCI_COMMAND);
+	reg32 &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO);
+	pci_write_config32(dev, PCI_COMMAND, reg32);
 }

#if CONFIG(HAVE_ACPI_TABLES)
diff --git a/src/soc/intel/fsp_broadwell_de/ubox.c b/src/soc/intel/fsp_broadwell_de/ubox.c
index d7352ad..e3e55e0 100644
--- a/src/soc/intel/fsp_broadwell_de/ubox.c
+++ b/src/soc/intel/fsp_broadwell_de/ubox.c
@@ -18,10 +18,13 @@
 #include <stdint.h>
 #include <soc/ubox.h>

-void iio_hide(const uint8_t devno, const uint8_t funcno)
+void iio_hide(DEVTREE_CONST struct device *dev)
 {
    pci_devfn_t ubox_dev;
+	uint8_t slot, func;

+	slot = PCI_SLOT(dev->path.pci.devfn);
+	func = PCI_FUNC(dev->path.pci.devfn);
    ubox_dev = PCI_DEV(get_busno1(), UBOX_DEV, UBOX_FUNC);
-	pci_or_config32(ubox_dev, UBOX_DEVHIDE0 + funcno * 4, 1 << devno);
+	pci_or_config32(ubox_dev, UBOX_DEVHIDE0 + func * 4, 1 << slot);
 }