Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/51328 )
Change subject: pciexp_device: Rewrite LTR configuration ......................................................................
pciexp_device: Rewrite LTR configuration
I was bugged by spurious "Failed to enable LTR" messages for years. Looking at the the current algorithm, it is flawed in multiple ways:
* It looks like the author didn't know they implemented a recursive algorithm (pciexp_enable_ltr()) inside another recursive algorithm (pciexp_scan_bridge()). Thus, at every tree level, everything is run again for the whole sub- tree.
* LTR is enabled no matter if `.set_ltr_max_latencies` is implemented or not. Leaving the endpoints' LTR settings at 0: They are told to always report zero tolerance. In theory, depending on the root-complex implementation, this may result in higher power consumption than without LTR messages.
* `.set_ltr_max_latencies` is only considered for the direct parent of a device. Thus, even with it implemented, an endpoint below a (non-root) bridge may suffer from the 0 settings as described above.
* Due to the double-recursive nature, LTR is enabled starting with the endpoints, then moving up the tree, while the PCIe spec tells us to do it in the exact opposite order.
With the current implementation of pciexp_scan_bridge(), it is hard to hook anything in that runs for each device from top to bottom. So the proposed solution still adds some redundancy:
First, for every device that uses pciexp_scan_bus(), we enable LTR if possible (see below). Then, when returning from the bus- scanning recursion, we enable LTR for every device and configure the maximum latencies (if supported). The latter runs again on all bridges, because it's hard to know if pciexp_scan_bus() was used for them.
When to enable LTR:
* For all devices that implement `.set_ltr_max_latencies`. * For all devices below a bridge that has it enabled already.
Change-Id: I2c5b8658f1fc8cec15e8b0824464c6fc9bee7e0e Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/51328 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/device/pciexp_device.c M src/include/device/pci.h M src/include/device/pci_def.h M src/soc/intel/broadwell/pch/pcie.c M src/soc/intel/common/block/pcie/pcie.c 5 files changed, 87 insertions(+), 57 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, but someone else must approve Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/device/pciexp_device.c b/src/device/pciexp_device.c index 70f6514..d8ed5d9 100644 --- a/src/device/pciexp_device.c +++ b/src/device/pciexp_device.c @@ -127,66 +127,92 @@ pci_write_config16(endp, endp_cap + PCI_EXP_LNKCTL, lnkctl); }
-static void pciexp_config_max_latency(struct device *root, struct device *dev) +static bool _pciexp_ltr_supported(struct device *dev, unsigned int cap) { - unsigned int cap; - cap = pciexp_find_extended_cap(dev, PCIE_EXT_CAP_LTR_ID); - if ((cap) && (root->ops->ops_pci != NULL) && - (root->ops->ops_pci->set_ltr_max_latencies != NULL)) - root->ops->ops_pci->set_ltr_max_latencies(dev, cap + 4); + return pci_read_config16(dev, cap + PCI_EXP_DEVCAP2) & PCI_EXP_DEVCAP2_LTR; }
-static bool pciexp_is_ltr_supported(struct device *dev, unsigned int cap) +static bool _pciexp_ltr_enabled(struct device *dev, unsigned int cap) { - unsigned int val; - - val = pci_read_config16(dev, cap + PCI_EXP_DEVCAP2); - - if (val & PCI_EXP_DEVCAP2_LTR) - return true; - - return false; + return pci_read_config16(dev, cap + PCI_EXP_DEVCTL2) & PCI_EXP_DEV2_LTR; }
-static void pciexp_configure_ltr(struct device *dev) +static bool _pciexp_enable_ltr(struct device *parent, unsigned int parent_cap, + struct device *dev, unsigned int cap) { - unsigned int cap; - - cap = pci_find_capability(dev, PCI_CAP_ID_PCIE); - - /* - * Check if capability pointer is valid and - * device supports LTR mechanism. - */ - if (!cap || !pciexp_is_ltr_supported(dev, cap)) { - printk(BIOS_INFO, "Failed to enable LTR for dev = %s\n", - dev_path(dev)); - return; + if (!_pciexp_ltr_supported(dev, cap)) { + printk(BIOS_DEBUG, "%s: No LTR support\n", dev_path(dev)); + return false; }
- cap += PCI_EXP_DEVCTL2; + if (_pciexp_ltr_enabled(dev, cap)) + return true;
- /* Enable LTR for device */ - pci_update_config32(dev, cap, ~PCI_EXP_DEV2_LTR, PCI_EXP_DEV2_LTR); + if (parent && + (parent->path.type != DEVICE_PATH_PCI || + !_pciexp_ltr_supported(parent, parent_cap) || + !_pciexp_ltr_enabled(parent, parent_cap))) + return false;
- /* Configure Max Snoop Latency */ - pciexp_config_max_latency(dev->bus->dev, dev); + pci_or_config16(dev, cap + PCI_EXP_DEVCTL2, PCI_EXP_DEV2_LTR); + printk(BIOS_INFO, "%s: Enabled LTR\n", dev_path(dev)); + return true; }
static void pciexp_enable_ltr(struct device *dev) { - struct bus *bus; - struct device *child; + const unsigned int cap = pci_find_capability(dev, PCI_CAP_ID_PCIE); + if (!cap) + return;
- for (bus = dev->link_list ; bus ; bus = bus->next) { - for (child = bus->children; child; child = child->sibling) { - if (child->path.type != DEVICE_PATH_PCI) - continue; - pciexp_configure_ltr(child); - if (child->ops && child->ops->scan_bus) - pciexp_enable_ltr(child); - } + /* + * If we have get_ltr_max_latencies(), treat `dev` as the root. + * If not, let _pciexp_enable_ltr() query the parent's state. + */ + struct device *parent = NULL; + unsigned int parent_cap = 0; + if (!dev->ops->ops_pci || !dev->ops->ops_pci->get_ltr_max_latencies) { + parent = dev->bus->dev; + parent_cap = pci_find_capability(dev, PCI_CAP_ID_PCIE); + if (!parent_cap) + return; } + + (void)_pciexp_enable_ltr(parent, parent_cap, dev, cap); +} + +static bool pciexp_get_ltr_max_latencies(struct device *dev, u16 *max_snoop, u16 *max_nosnoop) +{ + /* Walk the hierarchy up to find get_ltr_max_latencies(). */ + do { + if (dev->ops->ops_pci && dev->ops->ops_pci->get_ltr_max_latencies) + break; + if (dev->bus->dev == dev || dev->bus->dev->path.type != DEVICE_PATH_PCI) + return false; + dev = dev->bus->dev; + } while (true); + + dev->ops->ops_pci->get_ltr_max_latencies(max_snoop, max_nosnoop); + return true; +} + +static void pciexp_configure_ltr(struct device *parent, unsigned int parent_cap, + struct device *dev, unsigned int cap) +{ + if (!_pciexp_enable_ltr(parent, parent_cap, dev, cap)) + return; + + const unsigned int ltr_cap = pciexp_find_extended_cap(dev, PCIE_EXT_CAP_LTR_ID); + if (!ltr_cap) + return; + + u16 max_snoop, max_nosnoop; + if (!pciexp_get_ltr_max_latencies(dev, &max_snoop, &max_nosnoop)) + return; + + pci_write_config16(dev, ltr_cap + PCI_LTR_MAX_SNOOP, max_snoop); + pci_write_config16(dev, ltr_cap + PCI_LTR_MAX_NOSNOOP, max_nosnoop); + printk(BIOS_INFO, "%s: Programmed LTR max latencies\n", dev_path(dev)); }
static unsigned char pciexp_L1_substate_cal(struct device *dev, unsigned int endp_cap, @@ -471,12 +497,17 @@
/* Adjust Max_Payload_Size of link ends. */ pciexp_set_max_payload_size(root, root_cap, dev, cap); + + pciexp_configure_ltr(root, root_cap, dev, cap); }
void pciexp_scan_bus(struct bus *bus, unsigned int min_devfn, unsigned int max_devfn) { struct device *child; + + pciexp_enable_ltr(bus->dev); + pci_scan_bus(bus, min_devfn, max_devfn);
for (child = bus->children; child; child = child->sibling) { @@ -493,7 +524,6 @@ void pciexp_scan_bridge(struct device *dev) { do_pci_scan_bridge(dev, pciexp_scan_bus); - pciexp_enable_ltr(dev); }
/** Default device operations for PCI Express bridges */ diff --git a/src/include/device/pci.h b/src/include/device/pci.h index e80cb22..db7cc69 100644 --- a/src/include/device/pci.h +++ b/src/include/device/pci.h @@ -31,7 +31,7 @@ /* set the Subsystem IDs for the PCI device */ void (*set_subsystem)(struct device *dev, unsigned int vendor, unsigned int device); - void (*set_ltr_max_latencies)(struct device *dev, unsigned int off); + void (*get_ltr_max_latencies)(u16 *max_snoop, u16 *max_nosnoop); };
struct pci_driver { diff --git a/src/include/device/pci_def.h b/src/include/device/pci_def.h index b0558fa..d18e750 100644 --- a/src/include/device/pci_def.h +++ b/src/include/device/pci_def.h @@ -518,6 +518,10 @@ #define PCI_PWR_CAP 12 /* Capability */ #define PCI_PWR_CAP_BUDGET(x) ((x) & 1) /* Included in system budget */
+/* Latency Tolerance Reporting */ +#define PCI_LTR_MAX_SNOOP 4 +#define PCI_LTR_MAX_NOSNOOP 6 + /* * The PCI interface treats multi-function devices as independent * devices. The slot/function address of each device is encoded diff --git a/src/soc/intel/broadwell/pch/pcie.c b/src/soc/intel/broadwell/pch/pcie.c index 05619f8..26fde0b 100644 --- a/src/soc/intel/broadwell/pch/pcie.c +++ b/src/soc/intel/broadwell/pch/pcie.c @@ -609,17 +609,15 @@ root_port_commit_config(); }
-static void pcie_set_ltr_max_latencies(struct device *dev, unsigned int off) +static void pcie_get_ltr_max_latencies(u16 *max_snoop, u16 *max_nosnoop) { - /* Set max snoop and non-snoop latency for Broadwell */ - pci_write_config32(dev, off, - PCIE_LTR_MAX_NO_SNOOP_LATENCY_3146US << 16 | - PCIE_LTR_MAX_SNOOP_LATENCY_3146US); + *max_snoop = PCIE_LTR_MAX_SNOOP_LATENCY_3146US; + *max_nosnoop = PCIE_LTR_MAX_NO_SNOOP_LATENCY_3146US; }
static struct pci_operations pcie_ops = { .set_subsystem = pci_dev_set_subsystem, - .set_ltr_max_latencies = pcie_set_ltr_max_latencies, + .get_ltr_max_latencies = pcie_get_ltr_max_latencies, };
static struct device_operations device_ops = { diff --git a/src/soc/intel/common/block/pcie/pcie.c b/src/soc/intel/common/block/pcie/pcie.c index 346c0ff..f7f0902 100644 --- a/src/soc/intel/common/block/pcie/pcie.c +++ b/src/soc/intel/common/block/pcie/pcie.c @@ -45,16 +45,14 @@ pci_write_config16(dev, PCI_SEC_STATUS, reg16); }
-static void pcie_set_ltr_max_latencies(struct device *dev, unsigned int offset) +static void pcie_get_ltr_max_latencies(u16 *max_snoop, u16 *max_nosnoop) { - /* Set max snoop and non-snoop latency for the SOC */ - pci_write_config32(dev, offset, - PCIE_LTR_MAX_NO_SNOOP_LATENCY_3146US << 16 | - PCIE_LTR_MAX_SNOOP_LATENCY_3146US); + *max_snoop = PCIE_LTR_MAX_SNOOP_LATENCY_3146US; + *max_nosnoop = PCIE_LTR_MAX_NO_SNOOP_LATENCY_3146US; }
static struct pci_operations pcie_ops = { - .set_ltr_max_latencies = pcie_set_ltr_max_latencies, + .get_ltr_max_latencies = pcie_get_ltr_max_latencies, .set_subsystem = pci_dev_set_subsystem, };