Change in coreboot[master]: device: Enable ASPM for TBT PCIe root ports

[John Zhao (Code Review)]

John Zhao has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47686 )

Change subject: device: Enable ASPM for TBT PCIe root ports
......................................................................

device: Enable ASPM for TBT PCIe root ports

The virtual/generic device under TBT PCIe root ports has path type as
DEVICE_PATH_GENERIC. While scanning the pcie bus, the generic device
blocks its root ports configuration. This change adds device path type
check and enables ASPM for TBT root ports.

BUG=b:173207454
TEST=Built image and booted to kernel on Voxel board. Verified both of
the TBT Root ports 00:07.0 and 00:07.1 ASPM are enabled.

Signed-off-by: John Zhao 
Change-Id: I82ffaeb5a8821d9034d8dae9d98d3b5953a9608b
---
M src/device/pciexp_device.c
1 file changed, 3 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/47686/1

diff --git a/src/device/pciexp_device.c b/src/device/pciexp_device.c
index 8d4bb98..e85fe87 100644
--- a/src/device/pciexp_device.c
+++ b/src/device/pciexp_device.c
@@ -446,7 +446,7 @@
    unsigned int root_cap, cap;

cap = pci_find_capability(dev, PCI_CAP_ID_PCIE);
-	if (!cap)
+	if ( !cap && (dev->path.type != DEVICE_PATH_GENERIC) )
    	return;

root_cap = pci_find_capability(root, PCI_CAP_ID_PCIE);
@@ -480,7 +480,8 @@
    pci_scan_bus(bus, min_devfn, max_devfn);

for (child = bus->children; child; child = child->sibling) {
-		if (child->path.type != DEVICE_PATH_PCI)
+		if ( (child->path.type != DEVICE_PATH_PCI) &&
+		     (child->path.type != DEVICE_PATH_GENERIC) )
    		continue;
    	if ((child->path.pci.devfn < min_devfn) ||
    	    (child->path.pci.devfn > max_devfn)) {

[build bot (Jenkins) (Code Review)]

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

Change subject: device: Enable ASPM for TBT PCIe root ports
......................................................................

Patch Set 1:

(4 comments)

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

https://review.coreboot.org/c/coreboot/+/47686/1/src/device/pciexp_device.c@... 
PS1, Line 449: 	if ( !cap && (dev->path.type != DEVICE_PATH_GENERIC) )
space prohibited after that open parenthesis '('

https://review.coreboot.org/c/coreboot/+/47686/1/src/device/pciexp_device.c@... 
PS1, Line 449: 	if ( !cap && (dev->path.type != DEVICE_PATH_GENERIC) )
space prohibited before that close parenthesis ')'

https://review.coreboot.org/c/coreboot/+/47686/1/src/device/pciexp_device.c@... 
PS1, Line 483: 		if ( (child->path.type != DEVICE_PATH_PCI) &&
space prohibited after that open parenthesis '('

https://review.coreboot.org/c/coreboot/+/47686/1/src/device/pciexp_device.c@... 
PS1, Line 484: 		     (child->path.type != DEVICE_PATH_GENERIC) )
space prohibited before that close parenthesis ')'

[John Zhao (Code Review)]

Hello build bot (Jenkins), Duncan Laurie, Utkarsh H Patel, Sukumar Ghorai,

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

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

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

Change subject: device: Enable ASPM for TBT PCIe root ports
......................................................................

device: Enable ASPM for TBT PCIe root ports

The virtual/generic device under TBT PCIe root ports has path type as
DEVICE_PATH_GENERIC. While scanning the pcie bus, the generic device
blocks its root ports configuration. This change adds device path type
check and enables ASPM for TBT root ports.

BUG=b:173207454
TEST=Built image and booted to kernel on Voxel board. Verified both of
the TBT Root ports 00:07.0 and 00:07.1 ASPM are enabled.

Signed-off-by: John Zhao 
Change-Id: I82ffaeb5a8821d9034d8dae9d98d3b5953a9608b
---
M src/device/pciexp_device.c
1 file changed, 3 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/47686/2

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47686 )

Change subject: device: Enable ASPM for TBT PCIe root ports
......................................................................

Patch Set 2:

(4 comments)

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

https://review.coreboot.org/c/coreboot/+/47686/1/src/device/pciexp_device.c@... 
PS1, Line 449: 	if ( !cap && (dev->path.type != DEVICE_PATH_GENERIC) )
> space prohibited before that close parenthesis ')'

done

https://review.coreboot.org/c/coreboot/+/47686/1/src/device/pciexp_device.c@... 
PS1, Line 449: 	if ( !cap && (dev->path.type != DEVICE_PATH_GENERIC) )
> space prohibited after that open parenthesis '('

done

https://review.coreboot.org/c/coreboot/+/47686/1/src/device/pciexp_device.c@... 
PS1, Line 483: 		if ( (child->path.type != DEVICE_PATH_PCI) &&
> space prohibited after that open parenthesis '('

done

https://review.coreboot.org/c/coreboot/+/47686/1/src/device/pciexp_device.c@... 
PS1, Line 484: 		     (child->path.type != DEVICE_PATH_GENERIC) )
> space prohibited before that close parenthesis ')'

done

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47686 )

Change subject: device: Enable ASPM for TBT PCIe root ports
......................................................................

Patch Set 2:

I'm not sure a root port without an attached endpoint should be able to enable ASPM, how can it know the device will be capable of it?

If you remove the generic device from 0:7.x in chipset.cb it also does not enable ASPM, so this is not restoring any previous behavior and uses the generic device to force the link to do things that I don't think it should be able to if no device is present.

In addition, PCI endpoints that exist on a root port at boot time are already handled properly so repeats things twice on root ports where there is actually a PCI endpoint attached.

do_pci_scan_bridge for PCI: 00:1c.0
 PCI: pci_scan_bus for bus 2c
 PCI: 2c:00.0 [17a0/0000] ops
 GL9755: configure ASPM and LTR
 PCI: 2c:00.0 [17a0/9755] enabled
 GENERIC: 0.0 enabled
 Enabling Common Clock Configuration
 L1 Sub-State supported from root port 28
 L1 Sub-State Support = 0xf
 CommonModeRestoreTime = 0xff
 Power On Value = 0x1f, Power On Scale = 0x2
 ASPM: Enabled L1
 PCIe: Max_Payload_Size adjusted to 128
-scan_bus: bus PCI: 00:1c.0 finished in 0 msecs
+Enabling Common Clock Configuration
+L1 Sub-State supported from root port 28
+L1 Sub-State Support = 0xf
+CommonModeRestoreTime = 0xff
+Power On Value = 0x1f, Power On Scale = 0x2
+ASPM: Enabled L1
+PCIe: Max_Payload_Size adjusted to 128
+scan_bus: bus PCI: 00:1c.0 finished in 8 msecs

[John Zhao (Code Review)]

Attention is currently required from: John Zhao.
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47686 )

Change subject: device: Enable ASPM for TBT PCIe root ports
......................................................................

Patch Set 2:

(1 comment)

Patchset:

PS2:
> Patch Set 2:
>
> I'm not sure a root port without an attached endpoint should be able to enable ASPM, how can it know the device will be capable of it?
>
> If you remove the generic device from 0:7.x in chipset.cb it also does not enable ASPM, so this is not restoring any previous behavior and uses the generic device to force the link to do things that I don't think it should be able to if no device is present.
>
> In addition, PCI endpoints that exist on a root port at boot time are already handled properly so repeats things twice on root ports where there is actually a PCI endpoint attached.
>
> do_pci_scan_bridge for PCI: 00:1c.0
>  PCI: pci_scan_bus for bus 2c
>  PCI: 2c:00.0 [17a0/0000] ops
>  GL9755: configure ASPM and LTR
>  PCI: 2c:00.0 [17a0/9755] enabled
>  GENERIC: 0.0 enabled
>  Enabling Common Clock Configuration
>  L1 Sub-State supported from root port 28
>  L1 Sub-State Support = 0xf
>  CommonModeRestoreTime = 0xff
>  Power On Value = 0x1f, Power On Scale = 0x2
>  ASPM: Enabled L1
>  PCIe: Max_Payload_Size adjusted to 128
> -scan_bus: bus PCI: 00:1c.0 finished in 0 msecs
> +Enabling Common Clock Configuration
> +L1 Sub-State supported from root port 28
> +L1 Sub-State Support = 0xf
> +CommonModeRestoreTime = 0xff
> +Power On Value = 0x1f, Power On Scale = 0x2
> +ASPM: Enabled L1
> +PCIe: Max_Payload_Size adjusted to 128
> +scan_bus: bus PCI: 00:1c.0 finished in 8 msecs

Kernel does not enable ASPM for TBT root ports. Kernel driver only handles ASPM enabling for attached devices. 
Bridges and buses behind the bridge are scanned at boot time. i.e for PCI 00:1c.0, its endpoint(bus->children) has path.type->DEVICE_PATH_PCI and its ASPM is enabled. 00:07.x configuration is skipped because its endpoints has path.type->DEVICE_PATH_GENERIC. If removing generic device from 00:07.x in chipset.cb, its endpoint is NULL which also causes configuration skipped and the TBT root ports ASPM are not enabled.
This TBT root ports ASPM enabling features has been applied across various NDA and DA validations. No negative feedback.

[XS] Change in coreboot[master]: device: Enable ASPM for TBT PCIe root ports

[Stefan Reinauer (Code Review)]

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

Change subject: device: Enable ASPM for TBT PCIe root ports
......................................................................

Abandoned