Change in coreboot[master]: pciexp: retrain PCIe link at lower speed if no link

[Jonathan Kollasch (Code Review)]

Jonathan Kollasch has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42683 )

Change subject: pciexp: retrain PCIe link at lower speed if no link
......................................................................

pciexp: retrain PCIe link at lower speed if no link

Needed for PCIe x4 dual-port Marvell NIC (sky2) on Sandy/Ivy Bridge
and 6-series PCH (if not other chipsets), where the link will not come
up at 5GT/s (or 8GT/s?), but will if explicitly trained at 2.5GT/s.

Change-Id: I7ba15f7c13463356c6417f41b44d045aacfde4cc
---
M src/device/pciexp_device.c
M src/include/device/pci_def.h
2 files changed, 41 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/42683/1

diff --git a/src/device/pciexp_device.c b/src/device/pciexp_device.c
index f04d865..aeb44f7 100644
--- a/src/device/pciexp_device.c
+++ b/src/device/pciexp_device.c
@@ -112,6 +112,38 @@
    }
 }

+static void pciexp_limit_speed(struct device *dev, unsigned int cap)
+{
+	u16 x, y;
+
+	x = pci_read_config16(dev, cap + PCI_EXP_FLAGS);
+
+	/* is this new enough to support LNKCTL2? */
+	if ((x & PCI_EXP_FLAGS_VERS) <= 1)
+		return;
+
+	y = pci_read_config16(dev, cap + PCI_EXP_LNKCTL2);
+	y &= PCI_EXP_LNKCTL2_TLS;
+
+	do {
+		/* link up? */
+		x = pci_read_config16(dev, cap + PCI_EXP_LNKSTA);
+		x = (x & PCI_EXP_LNKSTA_LW) >> 4;
+		if (x > 0)
+			return;
+
+		printk(BIOS_INFO, "%s: link down; retraining at target link speed %u\n",
+		       dev_path(dev), y);
+
+		x = pci_read_config16(dev, cap + PCI_EXP_LNKCTL2);
+		x &= ~PCI_EXP_LNKCTL2_TLS;
+		x |= y & PCI_EXP_LNKCTL2_TLS;
+		pci_write_config16(dev, cap + PCI_EXP_LNKCTL2, x);
+
+		pciexp_retrain_link(dev, cap);
+	} while (--y > 0);
+}
+
 static void pciexp_enable_clock_power_pm(struct device *endp, unsigned int endp_cap)
 {
    /* check if per port clk req is supported in device */
@@ -488,6 +520,12 @@

void pciexp_scan_bridge(struct device *dev)
 {
+	unsigned int cap;
+
+	cap = pci_find_capability(dev, PCI_CAP_ID_PCIE);
+	if (cap)
+		pciexp_limit_speed(dev, cap);
+	
    do_pci_scan_bridge(dev, pciexp_scan_bus);
    pciexp_enable_ltr(dev);
 }
diff --git a/src/include/device/pci_def.h b/src/include/device/pci_def.h
index 07ba4a2..b351e42 100644
--- a/src/include/device/pci_def.h
+++ b/src/include/device/pci_def.h
@@ -432,6 +432,7 @@
 #define  PCI_EXP_LNKCTL_CCC	0x40	/* Common Clock COnfiguration */
 #define  PCI_EXP_EN_CLK_PM	0x100	/* Enable Clock Power Management */
 #define PCI_EXP_LNKSTA		18	/* Link Status */
+#define  PCI_EXP_LNKSTA_LW	0x3F0	/* Link Width */
 #define  PCI_EXP_LNKSTA_LT	0x800	/* Link Training */
 #define  PCI_EXP_LNKSTA_SLC	0x1000	/* Slot Clock Configuration */
 #define PCI_EXP_SLTCAP		20	/* Slot Capabilities */
@@ -446,6 +447,8 @@
 #define  PCI_EXP_RTCTL_CRSSVE	0x10	/* CRS Software Visibility Enable */
 #define PCI_EXP_RTCAP		30	/* Root Capabilities */
 #define PCI_EXP_RTSTA		32	/* Root Status */
+#define PCI_EXP_LNKCTL2		48	/* Link Control 2 */
+#define  PCI_EXP_LNKCTL2_TLS	0x000f	/* Target Link Speed */

/* Extended Capabilities (PCI-X 2.0 and Express) */
 #define PCI_EXT_CAP_ID(header)		(header & 0x0000ffff)

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42683 )

Change subject: pciexp: retrain PCIe link at lower speed if no link
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42683/1/src/device/pciexp_device.c 
File src/device/pciexp_device.c:

https://review.coreboot.org/c/coreboot/+/42683/1/src/device/pciexp_device.c@... 
PS1, Line 528: 	
trailing whitespace

[Paul Menzel (Code Review)]

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

Change subject: pciexp: retrain PCIe link at lower speed if no link
......................................................................

Patch Set 1:

Signed-off-by line is missing.

[Jonathan Kollasch (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: pciexp: retrain PCIe link at lower speed if no link
......................................................................

pciexp: retrain PCIe link at lower speed if no link

Needed for PCIe x4 dual-port Marvell NIC (sky2) on Sandy/Ivy Bridge
and 6-series PCH (if not other chipsets), where the link will not come
up at 5GT/s (or 8GT/s?), but will if explicitly trained at 2.5GT/s.

Change-Id: I7ba15f7c13463356c6417f41b44d045aacfde4cc
Signed-off-by: Jonathan A. Kollasch 
---
M src/device/pciexp_device.c
M src/include/device/pci_def.h
2 files changed, 41 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/42683/2

[Patrick Rudolph (Code Review)]

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

Change subject: pciexp: retrain PCIe link at lower speed if no link
......................................................................

Patch Set 2:

That setting should be made part of devicetree and thus configurable for each PCIe port.

Are you sure that coreboot isn't just booting too fast and thus the device isn't ready when PCI scanning happens?

[Angel Pons (Code Review)]

Attention is currently required from: Jonathan Kollasch.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42683 )

Change subject: pciexp: retrain PCIe link at lower speed if no link
......................................................................

Patch Set 2:

(1 comment)

File src/device/pciexp_device.c:

https://review.coreboot.org/c/coreboot/+/42683/comment/22849b70_52927947 
PS2, Line 115: static void pciexp_limit_speed(struct device *dev, unsigned int cap)
             : {
             : 	u16 x, y;
             : 
             : 	x = pci_read_config16(dev, cap + PCI_EXP_FLAGS);
             : 
             : 	/* is this new enough to support LNKCTL2? */
             : 	if ((x & PCI_EXP_FLAGS_VERS) <= 1)
             : 		return;
             : 
             : 	y = pci_read_config16(dev, cap + PCI_EXP_LNKCTL2);
             : 	y &= PCI_EXP_LNKCTL2_TLS;
             : 
             : 	do {
             : 		/* link up? */
             : 		x = pci_read_config16(dev, cap + PCI_EXP_LNKSTA);
             : 		x = (x & PCI_EXP_LNKSTA_LW) >> 4;
             : 		if (x > 0)
             : 			return;
             : 
             : 		printk(BIOS_INFO, "%s: link down; retraining at target link speed %u\n",
             : 		       dev_path(dev), y);
             : 
             : 		x = pci_read_config16(dev, cap + PCI_EXP_LNKCTL2);
             : 		x &= ~PCI_EXP_LNKCTL2_TLS;
             : 		x |= y & PCI_EXP_LNKCTL2_TLS;
             : 		pci_write_config16(dev, cap + PCI_EXP_LNKCTL2, x);
             : 
             : 		pciexp_retrain_link(dev, cap);
             : 	} while (--y > 0);
             : }
Uh, please do not name things `x` and `y` when they have better names:

static void pciexp_limit_speed(struct device *dev, unsigned int cap)
 {
    /* Is this device new enough to support LNKCTL2? */
    const u16 pciexp_flags = pci_read_config16(dev, cap + PCI_EXP_FLAGS);
    if ((pciexp_flags & PCI_EXP_FLAGS_VERS) <= 1)
    	return;

u16 target_speed = pci_read_config16(dev, cap + PCI_EXP_LNKCTL2);
    target_speed &= PCI_EXP_LNKCTL2_TLS;
    do {
    	/* Link up? */
    	const u16 lnk_sta = pci_read_config16(dev, cap + PCI_EXP_LNKSTA);
    	if ((lnk_sta & PCI_EXP_LNKSTA_LW) >> 4)
    		return;

printk(BIOS_INFO, "%s: link down; retraining at target link speed %u\n",
    	       dev_path(dev), target_speed);

pci_update_config16(dev, cap + PCI_EXP_LNKCTL2, ~PCI_EXP_LNKCTL2_TLS,
    			target_speed & PCI_EXP_LNKCTL2_TLS);
    	pciexp_retrain_link(dev, cap);
    } while (--target_speed > 0);
 }

I would rename the function as well, since the name may suggest that it always limits PCIe speed.

[S] Change in coreboot[master]: pciexp: retrain PCIe link at lower speed if no link

[Stefan Reinauer (Code Review)]

Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42683?usp=email )

Change subject: pciexp: retrain PCIe link at lower speed if no link
......................................................................

Abandoned