Change in coreboot[master]: device/pciexp: Match Max_Payload_Size between ends of a link
Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37769 )
Change subject: device/pciexp: Match Max_Payload_Size between ends of a link ......................................................................
device/pciexp: Match Max_Payload_Size between ends of a link
Ends of a PCIe link may advertise different Max_Payload_Size in their PCIe Express Capabilities, Device Capabilities block.
For correct operation, both ends of the link need to have their Device Control Max_Payload_Size programmed to match and not exceed the other end's Device Capabilities.
Fixes: https://ticket.coreboot.org/issues/218
Change-Id: I8b1de13e9c73abb30e5ccc792918bb4f81e5fe84 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/device/pciexp_device.c 1 file changed, 43 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/37769/1
diff --git a/src/device/pciexp_device.c b/src/device/pciexp_device.c index c73c548..f76fbf5 100644 --- a/src/device/pciexp_device.c +++ b/src/device/pciexp_device.c @@ -12,6 +12,7 @@ */
#include <console/console.h> +#include <commonlib/helpers.h> #include <delay.h> #include <device/device.h> #include <device/pci.h> @@ -408,6 +409,45 @@ printk(BIOS_INFO, "ASPM: Enabled %s\n", aspm_type_str[apmc]); }
+/* + * Set max payload size of endpoint in accordance with max payload size of root port. + */ +static void pciexp_set_max_payload_size(struct device *root, unsigned int root_cap, + struct device *endp, unsigned int endp_cap) +{ + unsigned int endp_max_payload, root_max_payload, max_payload; + u16 endp_devctl, root_devctl; + u32 endp_devcap, root_devcap; + + /* Get max payload size supported by endpoint */ + endp_devcap = pci_read_config32(endp, endp_cap + PCI_EXP_DEVCAP); + endp_max_payload = endp_devcap & PCI_EXP_DEVCAP_PAYLOAD; + + /* Get max payload size supported by root port */ + root_devcap = pci_read_config32(root, root_cap + PCI_EXP_DEVCAP); + root_max_payload = root_devcap & PCI_EXP_DEVCAP_PAYLOAD; + + /* Set max payload to smaller of the reported device capability. */ + max_payload = MIN(endp_max_payload, root_max_payload); + if (max_payload > 5) { + printk(BIOS_ERR, "PCIe: Max_Payload_Size field restricted from %d to 5\n", + max_payload); + max_payload = 5; + } + + endp_devctl = pci_read_config16(endp, endp_cap + PCI_EXP_DEVCTL); + endp_devctl &= ~PCI_EXP_DEVCTL_PAYLOAD; + endp_devctl |= max_payload << 5; + pci_write_config16(endp, endp_cap + PCI_EXP_DEVCTL, endp_devctl); + + root_devctl = pci_read_config16(root, root_cap + PCI_EXP_DEVCTL); + root_devctl &= ~PCI_EXP_DEVCTL_PAYLOAD; + endp_devctl |= max_payload << 5; + pci_write_config16(root, root_cap + PCI_EXP_DEVCTL, root_devctl); + + printk(BIOS_INFO, "PCIe: Max_Payload_Size adjusted to %d\n", (1 << (max_payload + 7))); +} + static void pciexp_tune_dev(struct device *dev) { struct device *root = dev->bus->dev; @@ -436,6 +476,9 @@ /* Check for and enable ASPM */ if (CONFIG(PCIEXP_ASPM)) pciexp_enable_aspm(root, root_cap, dev, cap); + + /* Adjust Max_Payload_Size of link ends. */ + pciexp_set_max_payload_size(root, root_cap, dev, cap); }
void pciexp_scan_bus(struct bus *bus, unsigned int min_devfn,
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37769 )
Change subject: device/pciexp: Match Max_Payload_Size between ends of a link ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37769/1/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/37769/1/src/device/pciexp_device.c@... PS1, Line 430: /* Set max payload to smaller of the reported device capability. */ Perhaps somewhere comment or print why you're capping the field to 5. I assume because 6 and 7 are still reserved, however I'm looking at an 8 year old copy of the spec.
https://review.coreboot.org/c/coreboot/+/37769/1/src/device/pciexp_device.c@... PS1, Line 448: PCIe: Max_Payload_Size adjusted to %d The console output likely makes it obvious what B:D:F this is. If not, you could maybe add that to the printk.
https://review.coreboot.org/c/coreboot/+/37769/1/src/device/pciexp_device.c@... PS1, Line 482: } Meh, the spec says you don't need to do this for each function in a multi-function device. Could this be conditional on PCI_FUNC(dev->Lpath.pci.devfn)) > 0?
Hello Aaron Durbin, Aamir Bohra, Marshall Dawson, Дмитрий Понаморев, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37769
to look at the new patch set (#2).
Change subject: device/pciexp: Match Max_Payload_Size between ends of a link ......................................................................
device/pciexp: Match Max_Payload_Size between ends of a link
Ends of a PCIe link may advertise different Max_Payload_Size in their PCIe Express Capabilities, Device Capabilities block.
For correct operation, both ends of the link need to have their Device Control Max_Payload_Size programmed to match and not exceed the other end's Device Capabilities.
Fixes: https://ticket.coreboot.org/issues/218
Change-Id: I8b1de13e9c73abb30e5ccc792918bb4f81e5fe84 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/device/pciexp_device.c 1 file changed, 44 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/37769/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37769 )
Change subject: device/pciexp: Match Max_Payload_Size between ends of a link ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/37769/1/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/37769/1/src/device/pciexp_device.c@... PS1, Line 430: /* Set max payload to smaller of the reported device capability. */
Perhaps somewhere comment or print why you're capping the field to 5. […]
Done
https://review.coreboot.org/c/coreboot/+/37769/1/src/device/pciexp_device.c@... PS1, Line 445: endp_devctl |= max_payload << 5; Oh dear :(
https://review.coreboot.org/c/coreboot/+/37769/1/src/device/pciexp_device.c@... PS1, Line 448: PCIe: Max_Payload_Size adjusted to %d
The console output likely makes it obvious what B:D:F this is. […]
I'll see about the consistency of the logging separately.
https://review.coreboot.org/c/coreboot/+/37769/1/src/device/pciexp_device.c@... PS1, Line 482: }
Meh, the spec says you don't need to do this for each function in a multi-function device. […]
That's probably true for all the pciexp_xx() calls we make above. I could consider some followup work on this, if someone has suitable PCIe topology hardware to test on.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37769 )
Change subject: device/pciexp: Match Max_Payload_Size between ends of a link ......................................................................
Patch Set 2: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/37769/1/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/37769/1/src/device/pciexp_device.c@... PS1, Line 445: endp_devctl |= max_payload << 5;
Oh dear :(
Ew, I missed that one.
https://review.coreboot.org/c/coreboot/+/37769/1/src/device/pciexp_device.c@... PS1, Line 482: }
That's probably true for all the pciexp_xx() calls we make above. […]
Ack. I seem to recall some inconsistent wording in the spec, so they should probably each be reconsidered independently.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37769 )
Change subject: device/pciexp: Match Max_Payload_Size between ends of a link ......................................................................
device/pciexp: Match Max_Payload_Size between ends of a link
Ends of a PCIe link may advertise different Max_Payload_Size in their PCIe Express Capabilities, Device Capabilities block.
For correct operation, both ends of the link need to have their Device Control Max_Payload_Size programmed to match and not exceed the other end's Device Capabilities.
Fixes: https://ticket.coreboot.org/issues/218
Change-Id: I8b1de13e9c73abb30e5ccc792918bb4f81e5fe84 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37769 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/device/pciexp_device.c 1 file changed, 44 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Marshall Dawson: Looks good to me, approved
diff --git a/src/device/pciexp_device.c b/src/device/pciexp_device.c index c73c548..479891c 100644 --- a/src/device/pciexp_device.c +++ b/src/device/pciexp_device.c @@ -12,6 +12,7 @@ */
#include <console/console.h> +#include <commonlib/helpers.h> #include <delay.h> #include <device/device.h> #include <device/pci.h> @@ -408,6 +409,46 @@ printk(BIOS_INFO, "ASPM: Enabled %s\n", aspm_type_str[apmc]); }
+/* + * Set max payload size of endpoint in accordance with max payload size of root port. + */ +static void pciexp_set_max_payload_size(struct device *root, unsigned int root_cap, + struct device *endp, unsigned int endp_cap) +{ + unsigned int endp_max_payload, root_max_payload, max_payload; + u16 endp_devctl, root_devctl; + u32 endp_devcap, root_devcap; + + /* Get max payload size supported by endpoint */ + endp_devcap = pci_read_config32(endp, endp_cap + PCI_EXP_DEVCAP); + endp_max_payload = endp_devcap & PCI_EXP_DEVCAP_PAYLOAD; + + /* Get max payload size supported by root port */ + root_devcap = pci_read_config32(root, root_cap + PCI_EXP_DEVCAP); + root_max_payload = root_devcap & PCI_EXP_DEVCAP_PAYLOAD; + + /* Set max payload to smaller of the reported device capability. */ + max_payload = MIN(endp_max_payload, root_max_payload); + if (max_payload > 5) { + /* Values 6 and 7 are reserved in PCIe 3.0 specs. */ + printk(BIOS_ERR, "PCIe: Max_Payload_Size field restricted from %d to 5\n", + max_payload); + max_payload = 5; + } + + endp_devctl = pci_read_config16(endp, endp_cap + PCI_EXP_DEVCTL); + endp_devctl &= ~PCI_EXP_DEVCTL_PAYLOAD; + endp_devctl |= max_payload << 5; + pci_write_config16(endp, endp_cap + PCI_EXP_DEVCTL, endp_devctl); + + root_devctl = pci_read_config16(root, root_cap + PCI_EXP_DEVCTL); + root_devctl &= ~PCI_EXP_DEVCTL_PAYLOAD; + root_devctl |= max_payload << 5; + pci_write_config16(root, root_cap + PCI_EXP_DEVCTL, root_devctl); + + printk(BIOS_INFO, "PCIe: Max_Payload_Size adjusted to %d\n", (1 << (max_payload + 7))); +} + static void pciexp_tune_dev(struct device *dev) { struct device *root = dev->bus->dev; @@ -436,6 +477,9 @@ /* Check for and enable ASPM */ if (CONFIG(PCIEXP_ASPM)) pciexp_enable_aspm(root, root_cap, dev, cap); + + /* Adjust Max_Payload_Size of link ends. */ + pciexp_set_max_payload_size(root, root_cap, dev, cap); }
void pciexp_scan_bus(struct bus *bus, unsigned int min_devfn,
-
Kyösti Mälkki (Code Review) -
Marshall Dawson (Code Review)
-
Patrick Georgi (Code Review)