diff options
| author | Nico Huber <nico.h@gmx.de> | 2021-03-07 01:39:18 +0100 |
|---|---|---|
| committer | Patrick Georgi <pgeorgi@google.com> | 2021-03-15 06:04:38 +0000 |
| commit | 968ef759887b4da09115a6b2d2e2afa223091792 (patch) | |
| tree | f9fff4f429080a168255596eadd5bb65d80cbd45 /src/include | |
| parent | ec2cbecf9368878ba6c2fbb42d02213ef8d04a91 (diff) | |
| download | coreboot-968ef759887b4da09115a6b2d2e2afa223091792.tar.xz | |
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>
Diffstat (limited to 'src/include')
| -rw-r--r-- | src/include/device/pci.h | 2 | ||||
| -rw-r--r-- | src/include/device/pci_def.h | 4 |
2 files changed, 5 insertions, 1 deletions
diff --git a/src/include/device/pci.h b/src/include/device/pci.h index e80cb22907..db7cc69c8d 100644 --- a/src/include/device/pci.h +++ b/src/include/device/pci.h @@ -31,7 +31,7 @@ struct pci_operations { /* 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 b0558fa67e..d18e750520 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 |