[coreboot-gerrit] Change in coreboot[master]: device/pciexp_device: Set values numerically instead of as bitmask

[Patrick Georgi (Code Review)]

Patrick Georgi has uploaded this change for review. ( https://review.coreboot.org/22740


Change subject: device/pciexp_device: Set values numerically instead of as bitmask
......................................................................

device/pciexp_device: Set values numerically instead of as bitmask

As noted on linux-pci, we have a weird way to handling "value" and
"scale" fields that are supposed to contain numerical values: we encode
them as a bitfield.
Instead define the two fields (offset and mask) and use numbers.

Another issue, not fixed in this CL, is that we write hard-coded values
while these fields really need to contain the max() of acceptable delays
of the downstream devices. That way the controller can decide whether or
not to enter a deeper power management state. It's noted as a TODO.

Change-Id: I895b9fe2ee438d3958c2d787e70a84d73eaa49d2
Signed-off-by: Patrick Georgi <pgeorgi at google.com>
Found-by: Bjorn Helgaas <bhelgaas at google.com>
---
M src/device/pciexp_device.c
M src/include/device/pciexp.h
2 files changed, 18 insertions(+), 4 deletions(-)



  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/22740/1

diff --git a/src/device/pciexp_device.c b/src/device/pciexp_device.c
index a177204..c020159 100644
--- a/src/device/pciexp_device.c
+++ b/src/device/pciexp_device.c
@@ -277,8 +277,14 @@
 	pci_update_config32(root, root_cap + 0x0c , 0xffffff04,
 		(endp_power_on_value << 3) | (power_on_scale));
 
-	pci_update_config32(root, root_cap + 0x08, ~0xe3ff0000,
-		(1 << 21) | (1 << 23) | (1 << 30));
+	/* TODO: 0xa0, 2 are values that work on some chipsets but really
+	 * should be determined dynamically by looking at downstream devices.
+	 */
+	pci_update_config32(root, root_cap + 0x08,
+		~(ASPM_LTR_L12_THRESHOLD_VALUE_MASK |
+			ASPM_LTR_L12_THRESHOLD_SCALE_MASK),
+		(0xa0 << ASPM_LTR_L12_THRESHOLD_VALUE_OFFSET) |
+		(2 << ASPM_LTR_L12_THRESHOLD_SCALE_OFFSET));
 
 	pci_update_config32(root, root_cap + 0x08, ~0x1f,
 		L1SubStateSupport);
@@ -287,8 +293,11 @@
 		pci_update_config32(dev_t, end_cap + 0x0c , 0xffffff04,
 			(endp_power_on_value << 3) | (power_on_scale));
 
-		pci_update_config32(dev_t, end_cap + 0x08, ~0xe3ff0000,
-			(1 << 21) | (1 << 23) | (1 << 30));
+		pci_update_config32(dev_t, end_cap + 0x08,
+			~(ASPM_LTR_L12_THRESHOLD_VALUE_MASK |
+				ASPM_LTR_L12_THRESHOLD_SCALE_MASK),
+			(0xa0 << ASPM_LTR_L12_THRESHOLD_VALUE_OFFSET) |
+			(2 << ASPM_LTR_L12_THRESHOLD_SCALE_OFFSET));
 
 		pci_update_config32(dev_t, end_cap + 0x08, ~0x1f,
 			L1SubStateSupport);
diff --git a/src/include/device/pciexp.h b/src/include/device/pciexp.h
index f3df1a5..e7f2120 100644
--- a/src/include/device/pciexp.h
+++ b/src/include/device/pciexp.h
@@ -9,6 +9,11 @@
 	PCIE_ASPM_BOTH = 3,
 };
 
+#define ASPM_LTR_L12_THRESHOLD_VALUE_OFFSET 16
+#define ASPM_LTR_L12_THRESHOLD_VALUE_MASK (0x3ff << ASPM_LTR_L12_THRESHOLD_VALUE_OFFSET)
+#define ASPM_LTR_L12_THRESHOLD_SCALE_OFFSET 29
+#define ASPM_LTR_L12_THRESHOLD_SCALE_MASK (0x7 << ASPM_LTR_L12_THRESHOLD_SCALE_OFFSET)
+
 void pciexp_scan_bus(struct bus *bus, unsigned int min_devfn,
 			     unsigned int max_devfn);
 

-- 
To view, visit https://review.coreboot.org/22740
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I895b9fe2ee438d3958c2d787e70a84d73eaa49d2
Gerrit-Change-Number: 22740
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Georgi <pgeorgi at google.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20171205/4a6c0d73/attachment.html>