Change in ...coreboot[master]: device/pciexp_device: Add set_L1_ss_latency() for pciexp device
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31951
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
device/pciexp_device: Add set_L1_ss_latency() for pciexp device
This patch performs below operations
1. Add new function to perform L1 latency programming for PCIE devices.
2. Remove duplicate implementaion of PCIE L1 latency programming and refer the same from common pciexp_device.c.
Change-Id: I3d14a40b4ed0dcc216dcac883e33749b7808f00d Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/pciexp_device.c M src/include/device/pciexp.h M src/soc/intel/broadwell/pcie.c M src/soc/intel/common/block/pcie/pcie.c 4 files changed, 17 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/31951/1
diff --git a/src/device/pciexp_device.c b/src/device/pciexp_device.c index 318f8cd..e4af9b2 100644 --- a/src/device/pciexp_device.c +++ b/src/device/pciexp_device.c @@ -463,6 +463,14 @@ pciexp_enable_ltr(dev); }
+void pciexp_set_L1_ss_max_latency(struct device *dev, unsigned int offset) +{ + /* Set max snoop and non-snoop latency for the SOC */ + pci_write_config32(dev, offset, + PCIE_LTR_MAX_NO_SNOOP_LATENCY_VALUE << 16 | + PCIE_LTR_MAX_SNOOP_LATENCY_VALUE); +} + void pciexp_set_subsystem(struct device *dev, unsigned int vendor, unsigned int device) { diff --git a/src/include/device/pciexp.h b/src/include/device/pciexp.h index 121b998..459eef5 100644 --- a/src/include/device/pciexp.h +++ b/src/include/device/pciexp.h @@ -5,6 +5,11 @@ /* PCI-E Sub-System ID */ #define PCIE_SUBSYSTEM_VENDOR_ID 0x94
+/* Latency tolerance reporting, max non-snoop latency value 3.14ms */ +#define PCIE_LTR_MAX_NO_SNOOP_LATENCY_VALUE 0x1003 +/* Latency tolerance reporting, max snoop latency value 3.14ms */ +#define PCIE_LTR_MAX_SNOOP_LATENCY_VALUE 0x1003 + enum aspm_type { PCIE_ASPM_NONE = 0, PCIE_ASPM_L0S = 1, @@ -29,4 +34,6 @@ void pciexp_set_subsystem(struct device *dev, unsigned int vendor, unsigned int device);
+void pciexp_set_L1_ss_max_latency(struct device *dev, unsigned int offset); + #endif /* DEVICE_PCIEXP_H */ diff --git a/src/soc/intel/broadwell/pcie.c b/src/soc/intel/broadwell/pcie.c index fdb8782..72e9884 100644 --- a/src/soc/intel/broadwell/pcie.c +++ b/src/soc/intel/broadwell/pcie.c @@ -649,15 +649,9 @@ root_port_commit_config(); }
-static void pcie_set_L1_ss_max_latency(struct device *dev, unsigned int off) -{ - /* Set max snoop and non-snoop latency for Broadwell */ - pci_write_config32(dev, off, 0x10031003); -} - static struct pci_operations pcie_ops = { .set_subsystem = pciexp_set_subsystem, - .set_L1_ss_latency = pcie_set_L1_ss_max_latency, + .set_L1_ss_latency = pciexp_set_L1_ss_max_latency, };
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 1c3114b..f02c38b 100644 --- a/src/soc/intel/common/block/pcie/pcie.c +++ b/src/soc/intel/common/block/pcie/pcie.c @@ -21,10 +21,6 @@ #include <device/pci_ops.h>
#define CACHE_LINE_SIZE 0x10 -/* Latency tolerance reporting, max non-snoop latency value 3.14ms */ -#define PCIE_LTR_MAX_NO_SNOOP_LATENCY_VALUE 0x1003 -/* Latency tolerance reporting, max snoop latency value 3.14ms */ -#define PCIE_LTR_MAX_SNOOP_LATENCY_VALUE 0x1003
static void pch_pcie_init(struct device *dev) { @@ -62,16 +58,8 @@ pci_write_config16(dev, PCI_SEC_STATUS, reg16); }
-static void pcie_set_L1_ss_max_latency(struct device *dev, unsigned int offset) -{ - /* Set max snoop and non-snoop latency for the SOC */ - pci_write_config32(dev, offset, - PCIE_LTR_MAX_NO_SNOOP_LATENCY_VALUE << 16 | - PCIE_LTR_MAX_SNOOP_LATENCY_VALUE); -} - static struct pci_operations pcie_ops = { - .set_L1_ss_latency = pcie_set_L1_ss_max_latency, + .set_L1_ss_latency = pciexp_set_L1_ss_max_latency, .set_subsystem = pciexp_set_subsystem, };
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31951 )
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/#/c/31951/1/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/#/c/31951/1/src/device/pciexp_device.c@470 PS1, Line 470: PCIE_LTR_MAX_NO_SNOOP_LATENCY_VALUE << 16 | The values you set should be parameters to this function, as you cannot assume them to be the same for all hardware. Thus, this function reduces to a single PCI configuration write, and basically just aliases pci_write_config32(), so little point defining it like this.
https://review.coreboot.org/#/c/31951/1/src/include/device/pciexp.h File src/include/device/pciexp.h:
https://review.coreboot.org/#/c/31951/1/src/include/device/pciexp.h@12 PS1, Line 12: Is there some relation between realtime and register value? Maybe change _VALUE to _3140US if not.
https://review.coreboot.org/#/c/31951/1/src/soc/intel/broadwell/pcie.c File src/soc/intel/broadwell/pcie.c:
https://review.coreboot.org/#/c/31951/1/src/soc/intel/broadwell/pcie.c@a655 PS1, Line 655: Just use the new _3140US defines here.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31951 )
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/31951/1/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/#/c/31951/1/src/device/pciexp_device.c@470 PS1, Line 470: PCIE_LTR_MAX_NO_SNOOP_LATENCY_VALUE << 16 |
The values you set should be parameters to this function, as you cannot assume them to be the same f […]
what i could see its more over intel specific hence its better to make a softlink and move actual implementation into intel/common someplace as applicable
https://review.coreboot.org/#/c/31951/1/src/include/device/pciexp.h File src/include/device/pciexp.h:
https://review.coreboot.org/#/c/31951/1/src/include/device/pciexp.h@12 PS1, Line 12:
Is there some relation between realtime and register value? […]
yes there is relation.
LTR snoop/non snoop field are like 16 bit registers
0-9: Latency value 10-12: Latency scale [000: Value times 1ns 001: Value times 32ns 010: Value times 1024ns 011 - Value time 32768ns 100 - value times 1048576ns 101 - Value times 33554432ns 110-111 reserved ]
13-15: reserved
so now if you decode 0x1003, latency value is 3 scale is 100 - 1.04ms so LTR value would be = 1.04*3 = 3.14ms
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Duncan Laurie, Philipp Deppenwiese, Lijian Zhao, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31951
to look at the new patch set (#2).
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
device/pciexp_device: Add set_L1_ss_latency() for pciexp device
This patch performs below operations
1. Add new function to perform L1 latency programming for PCIE devices.
2. Remove duplicate implementaion of PCIE L1 latency programming and refer the same from common pciexp_device.c.
Change-Id: I3d14a40b4ed0dcc216dcac883e33749b7808f00d Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/pciexp_device.c M src/include/device/pciexp.h M src/soc/intel/broadwell/pcie.c M src/soc/intel/common/block/pcie/pcie.c M src/soc/intel/common/pcie.c 5 files changed, 23 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/31951/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31951 )
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/31951/1/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/#/c/31951/1/src/device/pciexp_device.c@470 PS1, Line 470: PCIE_LTR_MAX_NO_SNOOP_LATENCY_VALUE << 16 |
what i could see its more over intel specific hence its better to make a softlink and move actual i […]
The 'offset' parameter is found by searching a standard PCIE capability structure, so this function _would_ be placed here in global PCI scope, if it was more complex and did more than just alias pci_write_config32().
https://review.coreboot.org/#/c/31951/1/src/include/device/pciexp.h File src/include/device/pciexp.h:
https://review.coreboot.org/#/c/31951/1/src/include/device/pciexp.h@12 PS1, Line 12:
yes there is relation.
LTR snoop/non snoop field are like 16 bit registers
0-9: Latency value 10-12: Latency scale [000: Value times 1ns 001: Value times 32ns 010: Value times 1024ns 011 - Value time 32768ns 100 - value times 1048576ns 101 - Value times 33554432ns 110-111 reserved ]
Just rename the #define from _VALUE to _3146US for now. Someone with deeper PCIe knowledge should revisit this, why only couple SOCs program this.
Looks like it is (value << (5 * scale)) in nanoseconds And the reverse function is something like:
scale = log2(latency_ns) / 5 value = latency_ns >> (scale * 5)
https://review.coreboot.org/#/c/31951/1/src/soc/intel/common/block/pcie/pcie... File src/soc/intel/common/block/pcie/pcie.c:
https://review.coreboot.org/#/c/31951/1/src/soc/intel/common/block/pcie/pcie... PS1, Line 71: No need to change anything here, fine as-is.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31951 )
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31951/1/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/#/c/31951/1/src/device/pciexp_device.c@470 PS1, Line 470: PCIE_LTR_MAX_NO_SNOOP_LATENCY_VALUE << 16 |
The 'offset' parameter is found by searching a standard PCIE capability structure, so this function […]
sorry i didn't get you. caller of this function passes offset value after reading capability ID register. this function is doing what it supposed to be, just program LTR snoop and non snoop value. what i could find my almost all code is that this value always remain same as 0x1003
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31951 )
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31951/1/src/soc/intel/common/block/pcie/pcie... File src/soc/intel/common/block/pcie/pcie.c:
https://review.coreboot.org/#/c/31951/1/src/soc/intel/common/block/pcie/pcie... PS1, Line 71:
No need to change anything here, fine as-is.
was moving the same code into intel/common to avoid duplication of same definitions
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31951 )
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/31951/1/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/#/c/31951/1/src/device/pciexp_device.c@470 PS1, Line 470: PCIE_LTR_MAX_NO_SNOOP_LATENCY_VALUE << 16 |
sorry i didn't get you. […]
Specially, if you would add this function in global space, it is a false assumption that every caller will want the same latency values programmed;
Thus this would become:
void pciex_set_L1_ss_max_latency(dev, offset, max_no_snoop, max_snoop) { pci_write_config32(dev, offset, (max_no_snoop << 16) | max_snoop); }
There is just no point in doing that... By aliasing I mean inventing a new function that does nothing else but calls an already existing function after combining two u16 to u32.
https://review.coreboot.org/#/c/31951/2/src/soc/intel/common/pcie.c File src/soc/intel/common/pcie.c:
https://review.coreboot.org/#/c/31951/2/src/soc/intel/common/pcie.c@26 PS2, Line 26: #define PCIE_LTR_MAX_SNOOP_LATENCY_3140US 0x1003 The defines are fine, but as I believe this is PCIe standard configuration register, these can go in device/pciexp.h. Comment above them which PCIe capability they are associated with.
To be precise, it is 3 x 1048576, so 3146 US rounded.
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Duncan Laurie, Philipp Deppenwiese, build bot (Jenkins), Lijian Zhao, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31951
to look at the new patch set (#3).
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
device/pciexp_device: Add set_L1_ss_latency() for pciexp device
This patch performs below operations
1. Add new function to perform L1 latency programming for PCIE devices.
2. Remove duplicate implementaion of PCIE L1 latency programming and refer the same from common pciexp_device.c.
Change-Id: I3d14a40b4ed0dcc216dcac883e33749b7808f00d Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/pciexp_device.c M src/include/device/pci.h M src/include/device/pciexp.h M src/soc/intel/broadwell/pcie.c M src/soc/intel/common/block/pcie/pcie.c 5 files changed, 52 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/31951/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31951 )
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31951/3/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/#/c/31951/3/src/device/pciexp_device.c@160 PS3, Line 160: if ((cap) && (root->ops->ops_pci != NULL) && suspect code indent for conditional statements (8, 24)
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Duncan Laurie, Philipp Deppenwiese, build bot (Jenkins), Lijian Zhao, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31951
to look at the new patch set (#4).
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
device/pciexp_device: Add set_L1_ss_latency() for pciexp device
This patch performs below operations
1. Add new function to perform L1 latency programming for PCIE devices.
2. Remove duplicate implementaion of PCIE L1 latency programming and refer the same from common pciexp_device.c.
Change-Id: I3d14a40b4ed0dcc216dcac883e33749b7808f00d Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/pciexp_device.c M src/include/device/pci.h M src/include/device/pciexp.h M src/soc/intel/broadwell/pcie.c M src/soc/intel/common/block/pcie/pcie.c 5 files changed, 53 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/31951/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31951 )
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/#/c/31951/4/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/#/c/31951/4/src/device/pciexp_device.c@160 PS4, Line 160: if ((cap) && (root->ops->ops_pci != NULL) && \ suspect code indent for conditional statements (8, 24)
https://review.coreboot.org/#/c/31951/4/src/device/pciexp_device.c@160 PS4, Line 160: if ((cap) && (root->ops->ops_pci != NULL) && \ Avoid unnecessary line continuations
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31951 )
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
Patch Set 4:
(2 comments)
Can you please review once again.
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Duncan Laurie, Philipp Deppenwiese, build bot (Jenkins), Lijian Zhao, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31951
to look at the new patch set (#5).
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
device/pciexp_device: Add set_L1_ss_latency() for pciexp device
This patch performs below operations
1. Add new function to perform L1 latency programming for PCIE devices.
2. Remove duplicate implementaion of PCIE L1 latency programming and refer the same from common pciexp_device.c.
Change-Id: I3d14a40b4ed0dcc216dcac883e33749b7808f00d Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/pciexp_device.c M src/include/device/pci.h M src/include/device/pciexp.h M src/soc/intel/broadwell/pcie.c M src/soc/intel/common/block/pcie/pcie.c 5 files changed, 52 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/31951/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31951 )
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/31951/5/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/#/c/31951/5/src/device/pciexp_device.c@160 PS5, Line 160: if ((cap) && (root->ops->ops_pci != NULL) && suspect code indent for conditional statements (8, 24)
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Duncan Laurie, Philipp Deppenwiese, build bot (Jenkins), Lijian Zhao, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31951
to look at the new patch set (#6).
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
device/pciexp_device: Add set_L1_ss_latency() for pciexp device
This patch performs below operations
1. Add new function to perform L1 latency programming for PCIE devices.
2. Remove duplicate implementaion of PCIE L1 latency programming and refer the same from common pciexp_device.c.
Change-Id: I3d14a40b4ed0dcc216dcac883e33749b7808f00d Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/pciexp_device.c M src/include/device/pci.h M src/include/device/pciexp.h M src/soc/intel/broadwell/pcie.c M src/soc/intel/common/block/pcie/pcie.c 5 files changed, 51 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/31951/6
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31951 )
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
Patch Set 6:
Can you please review ?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31951 )
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
Patch Set 6:
IMO this is going the wrong direction. Apologies if there was language barrier or confusion, I am not native English.
Now, all you should do is move that _3146US define into pciexp.h and use it instead of the literal 0x10041004. Nothing else for this commit.
I don't know why only one or two platforms set these values. Are we missing some PCIe compliancy here or this errata or tune-up?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31951 )
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
Patch Set 6:
IMO this is going the wrong direction. Apologies if there was language barrier or confusion, I am not native English.
Now, all you should do is move that _3146US define into pciexp.h and use it instead of the literal 0x10041004. Nothing else for this commit.
I don't know why only one or two platforms set these values. Are we missing some PCIe compliancy here or this errata or tune-up?
i believe default is proven working for other platform.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31951 )
Change subject: device/pciexp_device: Add set_L1_ss_latency() for pciexp device ......................................................................
Patch Set 6:
(1 comment)
Tested how?
https://review.coreboot.org/#/c/31951/6/src/include/device/pciexp.h File src/include/device/pciexp.h:
https://review.coreboot.org/#/c/31951/6/src/include/device/pciexp.h@36 PS6, Line 36: non snoop non-snoop
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Duncan Laurie, Philipp Deppenwiese, build bot (Jenkins), Lijian Zhao, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31951
to look at the new patch set (#7).
Change subject: device/pciexp_device: Convert LTR non-snoop/snoop value into common macro ......................................................................
device/pciexp_device: Convert LTR non-snoop/snoop value into common macro
Change-Id: I3d14a40b4ed0dcc216dcac883e33749b7808f00d Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/include/device/pciexp.h M src/soc/intel/broadwell/pcie.c M src/soc/intel/common/block/pcie/pcie.c 3 files changed, 10 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/31951/7
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Duncan Laurie, Philipp Deppenwiese, build bot (Jenkins), Lijian Zhao, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31951
to look at the new patch set (#8).
Change subject: device/pciexp_device: Convert LTR non-snoop/snoop value into common macro ......................................................................
device/pciexp_device: Convert LTR non-snoop/snoop value into common macro
Change-Id: I3d14a40b4ed0dcc216dcac883e33749b7808f00d Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/include/device/pciexp.h M src/soc/intel/broadwell/pcie.c M src/soc/intel/common/block/pcie/pcie.c 3 files changed, 10 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/31951/8
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31951 )
Change subject: device/pciexp_device: Convert LTR non-snoop/snoop value into common macro ......................................................................
Patch Set 8: Code-Review+2
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31951 )
Change subject: device/pciexp_device: Convert LTR non-snoop/snoop value into common macro ......................................................................
Patch Set 8: Code-Review+2
Subrata Banik has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31951 )
Change subject: device/pciexp_device: Convert LTR non-snoop/snoop value into common macro ......................................................................
device/pciexp_device: Convert LTR non-snoop/snoop value into common macro
Change-Id: I3d14a40b4ed0dcc216dcac883e33749b7808f00d Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31951 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-by: Lijian Zhao lijian.zhao@intel.com --- M src/include/device/pciexp.h M src/soc/intel/broadwell/pcie.c M src/soc/intel/common/block/pcie/pcie.c 3 files changed, 10 insertions(+), 7 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved Lijian Zhao: Looks good to me, approved
diff --git a/src/include/device/pciexp.h b/src/include/device/pciexp.h index 0f1420a..3a9825d 100644 --- a/src/include/device/pciexp.h +++ b/src/include/device/pciexp.h @@ -14,6 +14,11 @@ #define ASPM_LTR_L12_THRESHOLD_SCALE_OFFSET 29 #define ASPM_LTR_L12_THRESHOLD_SCALE_MASK (0x7 << ASPM_LTR_L12_THRESHOLD_SCALE_OFFSET)
+/* Latency tolerance reporting, max non-snoop latency value 3.14ms */ +#define PCIE_LTR_MAX_NO_SNOOP_LATENCY_3146US 0x1003 +/* Latency tolerance reporting, max snoop latency value 3.14ms */ +#define PCIE_LTR_MAX_SNOOP_LATENCY_3146US 0x1003 + void pciexp_scan_bus(struct bus *bus, unsigned int min_devfn, unsigned int max_devfn);
diff --git a/src/soc/intel/broadwell/pcie.c b/src/soc/intel/broadwell/pcie.c index 472e8da..dff4f81 100644 --- a/src/soc/intel/broadwell/pcie.c +++ b/src/soc/intel/broadwell/pcie.c @@ -652,7 +652,9 @@ static void pcie_set_L1_ss_max_latency(struct device *dev, unsigned int off) { /* Set max snoop and non-snoop latency for Broadwell */ - pci_write_config32(dev, off, 0x10031003); + pci_write_config32(dev, off, + PCIE_LTR_MAX_NO_SNOOP_LATENCY_3146US << 16 | + PCIE_LTR_MAX_SNOOP_LATENCY_3146US); }
static struct pci_operations pcie_ops = { diff --git a/src/soc/intel/common/block/pcie/pcie.c b/src/soc/intel/common/block/pcie/pcie.c index d74b098..e8b1050 100644 --- a/src/soc/intel/common/block/pcie/pcie.c +++ b/src/soc/intel/common/block/pcie/pcie.c @@ -21,10 +21,6 @@ #include <device/pci_ops.h>
#define CACHE_LINE_SIZE 0x10 -/* Latency tolerance reporting, max non-snoop latency value 3.14ms */ -#define PCIE_LTR_MAX_NO_SNOOP_LATENCY_VALUE 0x1003 -/* Latency tolerance reporting, max snoop latency value 3.14ms */ -#define PCIE_LTR_MAX_SNOOP_LATENCY_VALUE 0x1003
static void pch_pcie_init(struct device *dev) { @@ -66,8 +62,8 @@ { /* Set max snoop and non-snoop latency for the SOC */ pci_write_config32(dev, offset, - PCIE_LTR_MAX_NO_SNOOP_LATENCY_VALUE << 16 | - PCIE_LTR_MAX_SNOOP_LATENCY_VALUE); + PCIE_LTR_MAX_NO_SNOOP_LATENCY_3146US << 16 | + PCIE_LTR_MAX_SNOOP_LATENCY_3146US); }
static struct pci_operations pcie_ops = {
-
build bot (Jenkins) (Code Review) -
Kyösti Mälkki (Code Review)
-
Lijian Zhao (Code Review)
-
Paul Menzel (Code Review)
-
Subrata Banik (Code Review)