Change in ...coreboot[master]: device/pciexp_device: Add set_subsystem() for pciexp device
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31950
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
device/pciexp_device: Add set_subsystem() for pciexp device
This patch performs below operations
1. Add new function to perform subsystem programming for PCIE devices.
2. Remove duplicate implementaion of PCIE subsystem programming and refer the same from common pciexp_device.c.
Change-Id: I5fbed39ed448baf11f0e0786ce0ee94741d57237 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/pciexp_device.c M src/include/device/pciexp.h M src/northbridge/intel/sandybridge/pcie.c M src/soc/intel/broadwell/pcie.c M src/soc/intel/common/block/pcie/pcie.c M src/southbridge/intel/bd82x6x/pcie.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801ix/pcie.c M src/southbridge/intel/i82801jx/pcie.c M src/southbridge/intel/lynxpoint/pcie.c 10 files changed, 23 insertions(+), 106 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/31950/1
diff --git a/src/device/pciexp_device.c b/src/device/pciexp_device.c index c209816..318f8cd 100644 --- a/src/device/pciexp_device.c +++ b/src/device/pciexp_device.c @@ -463,9 +463,16 @@ pciexp_enable_ltr(dev); }
+void pciexp_set_subsystem(struct device *dev, unsigned int vendor, + unsigned int device) +{ + pci_write_config32(dev, PCIE_SUBSYSTEM_VENDOR_ID, + ((device & 0xffff) << 16) | (vendor & 0xffff)); +} + /** Default device operations for PCI Express bridges */ static struct pci_operations pciexp_bus_ops_pci = { - .set_subsystem = 0, + .set_subsystem = pciexp_set_subsystem, };
struct device_operations default_pciexp_ops_bus = { diff --git a/src/include/device/pciexp.h b/src/include/device/pciexp.h index 0f1420a..121b998 100644 --- a/src/include/device/pciexp.h +++ b/src/include/device/pciexp.h @@ -2,6 +2,9 @@ #define DEVICE_PCIEXP_H /* (c) 2005 Linux Networx GPL see COPYING for details */
+/* PCI-E Sub-System ID */ +#define PCIE_SUBSYSTEM_VENDOR_ID 0x94 + enum aspm_type { PCIE_ASPM_NONE = 0, PCIE_ASPM_L0S = 1, @@ -22,4 +25,8 @@ extern struct device_operations default_pciexp_ops_bus;
unsigned int pciexp_find_extended_cap(struct device *dev, unsigned int cap); + +void pciexp_set_subsystem(struct device *dev, unsigned int vendor, + unsigned int device); + #endif /* DEVICE_PCIEXP_H */ diff --git a/src/northbridge/intel/sandybridge/pcie.c b/src/northbridge/intel/sandybridge/pcie.c index 16bc314..6ffe2d3 100644 --- a/src/northbridge/intel/sandybridge/pcie.c +++ b/src/northbridge/intel/sandybridge/pcie.c @@ -65,20 +65,8 @@ } #endif
-static void -pcie_set_subsystem(struct device *dev, unsigned int ven, unsigned int device) -{ - /* NOTE: This is not the default position! */ - if (!ven || !device) - pci_write_config32(dev, 0x94, - pci_read_config32(dev, 0)); - else - pci_write_config32(dev, 0x94, - ((device & 0xffff) << 16) | (ven & 0xffff)); -} - static struct pci_operations pci_ops = { - .set_subsystem = pcie_set_subsystem, + .set_subsystem = pciexp_set_subsystem, };
static struct device_operations device_ops = { diff --git a/src/soc/intel/broadwell/pcie.c b/src/soc/intel/broadwell/pcie.c index 32135ee..fdb8782 100644 --- a/src/soc/intel/broadwell/pcie.c +++ b/src/soc/intel/broadwell/pcie.c @@ -649,16 +649,6 @@ root_port_commit_config(); }
-static void pcie_set_subsystem(struct device *dev, unsigned int vendor, - unsigned int device) -{ - /* NOTE: This is not the default position! */ - if (!vendor || !device) - pci_write_config32(dev, 0x94, pci_read_config32(dev, 0)); - else - pci_write_config32(dev, 0x94, (device << 16) | vendor); -} - static void pcie_set_L1_ss_max_latency(struct device *dev, unsigned int off) { /* Set max snoop and non-snoop latency for Broadwell */ @@ -666,7 +656,7 @@ }
static struct pci_operations pcie_ops = { - .set_subsystem = pcie_set_subsystem, + .set_subsystem = pciexp_set_subsystem, .set_L1_ss_latency = pcie_set_L1_ss_max_latency, };
diff --git a/src/soc/intel/common/block/pcie/pcie.c b/src/soc/intel/common/block/pcie/pcie.c index 0a5e1bf..1c3114b 100644 --- a/src/soc/intel/common/block/pcie/pcie.c +++ b/src/soc/intel/common/block/pcie/pcie.c @@ -25,8 +25,6 @@ #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 -/* PCI-E Sub-System ID */ -#define PCIE_SUBSYSTEM_VENDOR_ID 0x94
static void pch_pcie_init(struct device *dev) { @@ -72,16 +70,9 @@ PCIE_LTR_MAX_SNOOP_LATENCY_VALUE); }
-static void pcie_dev_set_subsystem(struct device *dev, - unsigned vendor, unsigned device) -{ - pci_write_config32(dev, PCIE_SUBSYSTEM_VENDOR_ID, - ((device & 0xffff) << 16) | (vendor & 0xffff)); -} - static struct pci_operations pcie_ops = { .set_L1_ss_latency = pcie_set_L1_ss_max_latency, - .set_subsystem = pcie_dev_set_subsystem, + .set_subsystem = pciexp_set_subsystem, };
static struct device_operations device_ops = { diff --git a/src/southbridge/intel/bd82x6x/pcie.c b/src/southbridge/intel/bd82x6x/pcie.c index 39c53e8..3e2d5fb 100644 --- a/src/southbridge/intel/bd82x6x/pcie.c +++ b/src/southbridge/intel/bd82x6x/pcie.c @@ -306,21 +306,8 @@ return NULL; }
-static void pcie_set_subsystem(struct device *dev, unsigned vendor, - unsigned device) -{ - /* NOTE: This is not the default position! */ - if (!vendor || !device) { - pci_write_config32(dev, 0x94, - pci_read_config32(dev, 0)); - } else { - pci_write_config32(dev, 0x94, - ((device & 0xffff) << 16) | (vendor & 0xffff)); - } -} - static struct pci_operations pci_ops = { - .set_subsystem = pcie_set_subsystem, + .set_subsystem = pciexp_set_subsystem, };
static struct device_operations device_ops = { diff --git a/src/southbridge/intel/i82801gx/pcie.c b/src/southbridge/intel/i82801gx/pcie.c index 4679ee5..c412927 100644 --- a/src/southbridge/intel/i82801gx/pcie.c +++ b/src/southbridge/intel/i82801gx/pcie.c @@ -252,22 +252,8 @@ root_port_commit_config(dev); }
- -static void pcie_set_subsystem(struct device *dev, unsigned int vendor, - unsigned int device) -{ - /* NOTE: This is not the default position! */ - if (!vendor || !device) { - pci_write_config32(dev, 0x94, - pci_read_config32(dev, 0)); - } else { - pci_write_config32(dev, 0x94, - ((device & 0xffff) << 16) | (vendor & 0xffff)); - } -} - static struct pci_operations pci_ops = { - .set_subsystem = pcie_set_subsystem, + .set_subsystem = pciexp_set_subsystem, };
static struct device_operations device_ops = { diff --git a/src/southbridge/intel/i82801ix/pcie.c b/src/southbridge/intel/i82801ix/pcie.c index a36fdc6..fee77ee 100644 --- a/src/southbridge/intel/i82801ix/pcie.c +++ b/src/southbridge/intel/i82801ix/pcie.c @@ -95,19 +95,6 @@ } }
-static void pcie_set_subsystem(struct device *dev, unsigned vendor, - unsigned device) -{ - /* NOTE: 0x94 is not the default position! */ - if (!vendor || !device) { - pci_write_config32(dev, 0x94, - pci_read_config32(dev, 0)); - } else { - pci_write_config32(dev, 0x94, - ((device & 0xffff) << 16) | (vendor & 0xffff)); - } -} - static void pch_pciexp_scan_bridge(struct device *dev) { struct southbridge_intel_i82801ix_config *config = dev->chip_info; @@ -121,7 +108,7 @@ }
static struct pci_operations pci_ops = { - .set_subsystem = pcie_set_subsystem, + .set_subsystem = pciexp_set_subsystem, };
static struct device_operations device_ops = { diff --git a/src/southbridge/intel/i82801jx/pcie.c b/src/southbridge/intel/i82801jx/pcie.c index fb90cd9..1f0038f 100644 --- a/src/southbridge/intel/i82801jx/pcie.c +++ b/src/southbridge/intel/i82801jx/pcie.c @@ -95,19 +95,6 @@ } }
-static void pcie_set_subsystem(struct device *dev, unsigned vendor, - unsigned device) -{ - /* NOTE: 0x94 is not the default position! */ - if (!vendor || !device) { - pci_write_config32(dev, 0x94, - pci_read_config32(dev, 0)); - } else { - pci_write_config32(dev, 0x94, - ((device & 0xffff) << 16) | (vendor & 0xffff)); - } -} - static void pch_pciexp_scan_bridge(struct device *dev) { struct southbridge_intel_i82801jx_config *config = dev->chip_info; @@ -121,7 +108,7 @@ }
static struct pci_operations pci_ops = { - .set_subsystem = pcie_set_subsystem, + .set_subsystem = pciexp_set_subsystem, };
static struct device_operations device_ops = { diff --git a/src/southbridge/intel/lynxpoint/pcie.c b/src/southbridge/intel/lynxpoint/pcie.c index 695abf2..9479a37 100644 --- a/src/southbridge/intel/lynxpoint/pcie.c +++ b/src/southbridge/intel/lynxpoint/pcie.c @@ -727,21 +727,8 @@ root_port_commit_config(); }
-static void pcie_set_subsystem(struct device *dev, unsigned vendor, - unsigned device) -{ - /* NOTE: This is not the default position! */ - if (!vendor || !device) { - pci_write_config32(dev, 0x94, - pci_read_config32(dev, 0)); - } else { - pci_write_config32(dev, 0x94, - ((device & 0xffff) << 16) | (vendor & 0xffff)); - } -} - static struct pci_operations pci_ops = { - .set_subsystem = pcie_set_subsystem, + .set_subsystem = pciexp_set_subsystem, };
static struct device_operations device_ops = {
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31950
to look at the new patch set (#2).
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
device/pciexp_device: Add set_subsystem() for pciexp device
This patch performs below operations
1. Add new function to perform subsystem programming for PCIE devices.
2. Remove duplicate implementaion of PCIE subsystem programming and refer the same from common pciexp_device.c.
Change-Id: I5fbed39ed448baf11f0e0786ce0ee94741d57237 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/pciexp_device.c M src/include/device/pciexp.h M src/northbridge/intel/sandybridge/pcie.c M src/soc/intel/broadwell/pcie.c M src/soc/intel/common/block/pcie/pcie.c M src/southbridge/intel/bd82x6x/pcie.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801ix/pcie.c M src/southbridge/intel/i82801jx/pcie.c M src/southbridge/intel/lynxpoint/pcie.c 10 files changed, 24 insertions(+), 106 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/31950/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
Patch Set 2:
(1 comment)
I was originally referring to eg soc/baytrail/chip.c where these functions are implemented using writes to config register (already defined) PCI_SUBSYSTEM_VENDOR_ID 0x2C. IMHO You can promote that to global space, like in device/pci.c.
It's not really tied to PCIe (or pciexp) thus keep the name as-is.
This should give you quite accurate list:
git grep --name-only "PCI_SUBSYSTEM_VENDOR_ID," -- src/ > files-to-fix sed -i -e "/PCI_SUBSYSTEM/ ,+2d" `cat files-to-fix` git add -e
With last one, tidy up the errors sed has left behind.
https://review.coreboot.org/#/c/31950/2/src/include/device/pciexp.h File src/include/device/pciexp.h:
https://review.coreboot.org/#/c/31950/2/src/include/device/pciexp.h@6 PS2, Line 6: #define PCIE_SUBSYSTEM_VENDOR_ID 0x94 Ah.. this register 0x94 seems to be specific to Intel, (unless of course you find it in the PCIe specifications defined like this for this purpose).
I like the work though, just that if this 0x94 is Intel specific, reflect it somehow in the names and put code under sb/intel/common instead of global space.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31950/2/src/include/device/pciexp.h File src/include/device/pciexp.h:
https://review.coreboot.org/#/c/31950/2/src/include/device/pciexp.h@6 PS2, Line 6: #define PCIE_SUBSYSTEM_VENDOR_ID 0x94
Ah.. […]
yes, subsystem programming register is eventually different between PCI and PCIE. And i could see PCI devices are still cover using common name but i thought you are explicit about PCIE as i knew in past that there are no any common PCIE subsystem programming, hence thought of creating dedicated one for PCIE and refer it across.
Actually i was thinking i don't even need to declare PCIE_SUBSYSTEM_VENDOR_ID in .h as no one going to use the same from .h explicitly
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31950/2/src/include/device/pciexp.h File src/include/device/pciexp.h:
https://review.coreboot.org/#/c/31950/2/src/include/device/pciexp.h@6 PS2, Line 6: #define PCIE_SUBSYSTEM_VENDOR_ID 0x94
yes, subsystem programming register is eventually different between PCI and PCIE. […]
Please reference the PCIe specs for register 0x94.
I very much believe the case is Intel implemented register 0x2C as read-only, and you will not find any mentions of register 0x94 in either of PCI or PCIe specifications. PCI silicon vendors were once told anything above 0x80 (or even 0x40) is free "namespace" for vendors to use so standards committee at least should not have chosen any new "standard" register from the region 0x80 to 0xFF.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31950/2/src/include/device/pciexp.h File src/include/device/pciexp.h:
https://review.coreboot.org/#/c/31950/2/src/include/device/pciexp.h@6 PS2, Line 6: #define PCIE_SUBSYSTEM_VENDOR_ID 0x94
Please reference the PCIe specs for register 0x94. […]
can you please access link https://www.intel.com/content/www/in/en/design/products-and-solutions/proces...
to download product specification
i can see those 2 registers in PCI and PCIE config space details as below
0x2c: in general PCI controller like PMC, SMBUS etc.
Bit Range Default & Access Field Name (ID): Description 31:16 0h RW/O Subsystem ID (SSID): Written by BIOS. Not used by hardware. This field is reset by PLTRST# assertion. 15:0 0h RW/O Subsystem Vendor ID (SSVID): Written by BIOS. Not used by hardware. This field is reset by PLTRST# assertion
0x94: for PCIE controller
Bit Range Default & Access Field Name (ID): Description 31:16 0h RW/O Subsystem Identifier (SID): Indicates the subsystem as identified by the vendor. This field is write once and is locked down until a bridge reset occurs (not the PCI bus reset). 15:0 0h RW/O Subsystem Vendor Identifier (SVID): Indicates the manufacturer of the subsystem. This field is write once and is locked down until a bridge reset occurs (not the PCI bus reset).
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
Patch Set 2:
(1 comment)
Agree that 0x94 is not generic but Intel's own implementation. Anything over 0x3f will need to be based on capability but not defined by pci-sig spec.
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Duncan Laurie, Lijian Zhao, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31950
to look at the new patch set (#3).
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
device/pciexp_device: Add set_subsystem() for pciexp device
This patch performs below operations
1. Add new function to perform subsystem programming for PCIE devices.
2. Remove duplicate implementaion of PCIE subsystem programming and refer the same from common pciexp_device.c.
Change-Id: I5fbed39ed448baf11f0e0786ce0ee94741d57237 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/pciexp_device.c M src/include/device/pciexp.h M src/northbridge/intel/sandybridge/pcie.c M src/soc/intel/broadwell/pcie.c M src/soc/intel/common/block/pcie/pcie.c M src/southbridge/intel/bd82x6x/pcie.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801ix/pcie.c M src/southbridge/intel/i82801jx/pcie.c M src/southbridge/intel/lynxpoint/pcie.c 10 files changed, 24 insertions(+), 106 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/31950/3
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
Patch Set 3:
Patch Set 2:
(1 comment)
Agree that 0x94 is not generic but Intel's own implementation. Anything over 0x3f will need to be based on capability but not defined by pci-sig spec.
That's why a function writing directly on 0x94 does not belong in device/pciexp.c but sb/intel/common, same for the register #define.
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Duncan Laurie, build bot (Jenkins), Lijian Zhao, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31950
to look at the new patch set (#4).
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
device/pciexp_device: Add set_subsystem() for pciexp device
This patch performs below operations
1. Add new function to perform subsystem programming for PCIE devices.
2. Remove duplicate implementaion of PCIE subsystem programming and refer the same from common pciexp_device.c.
Change-Id: I5fbed39ed448baf11f0e0786ce0ee94741d57237 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/pciexp_device.c M src/include/device/pciexp.h M src/northbridge/intel/sandybridge/pcie.c M src/soc/intel/broadwell/pcie.c M src/soc/intel/common/Makefile.inc M src/soc/intel/common/block/pcie/pcie.c A src/soc/intel/common/pcie.c M src/southbridge/intel/bd82x6x/pcie.c M src/southbridge/intel/common/Makefile.inc A src/southbridge/intel/common/pcie.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801ix/pcie.c M src/southbridge/intel/i82801jx/pcie.c M src/southbridge/intel/lynxpoint/pcie.c 14 files changed, 89 insertions(+), 103 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/31950/4
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
Patch Set 4:
Patch Set 2:
(1 comment)
Agree that 0x94 is not generic but Intel's own implementation.
Anything over 0x3f will need to be based on capability but not defined by pci-sig spec.
That's why a function writing directly on 0x94 does not belong in device/pciexp.c but sb/intel/common, same for the register #define.
can you please check latest patch set
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Duncan Laurie, build bot (Jenkins), Lijian Zhao, Patrick Georgi, Martin Roth, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31950
to look at the new patch set (#5).
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
device/pciexp_device: Add set_subsystem() for pciexp device
This patch performs below operations
1. Add new function to perform subsystem programming for PCIE devices.
2. Remove duplicate implementaion of PCIE subsystem programming and refer the same from common pciexp_device.c.
Change-Id: I5fbed39ed448baf11f0e0786ce0ee94741d57237 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/pciexp_device.c M src/include/device/pciexp.h M src/northbridge/intel/sandybridge/pcie.c M src/soc/intel/broadwell/pcie.c M src/soc/intel/common/Makefile.inc M src/soc/intel/common/block/pcie/pcie.c A src/soc/intel/common/pcie.c M src/southbridge/intel/bd82x6x/pcie.c M src/southbridge/intel/common/Makefile.inc A src/southbridge/intel/common/pcie.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801ix/pcie.c M src/southbridge/intel/i82801jx/pcie.c M src/southbridge/intel/lynxpoint/pcie.c 14 files changed, 82 insertions(+), 106 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/31950/5
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/31950/5/src/soc/intel/common/pcie.c File src/soc/intel/common/pcie.c:
https://review.coreboot.org/#/c/31950/5/src/soc/intel/common/pcie.c@22 PS5, Line 22: #define PCIE_SUBSYSTEM_VENDOR_ID 0x94 I would drop this define, it confuses one to believe this is standard register while it is not.
https://review.coreboot.org/#/c/31950/5/src/soc/intel/common/pcie.c@24 PS5, Line 24: void pciexp_set_subsystem(struct device *dev, unsigned int vendor, Same with this name, add _intel somewhere in there, if this register is specific to intel.
Now, the function declaration does not really belong there (because it is specific to Intel), but I will accept it in device/pci.h (not pciexp.h).
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/31950/5/src/soc/intel/common/pcie.c File src/soc/intel/common/pcie.c:
https://review.coreboot.org/#/c/31950/5/src/soc/intel/common/pcie.c@24 PS5, Line 24: void pciexp_set_subsystem(struct device *dev, unsigned int vendor,
Same with this name, add _intel somewhere in there, if this register is specific to intel. […]
its related to PCIEXP hence pciexp.h should be more applicable?
I agree with _intel name.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
Patch Set 5:
Subrata, apologies, I may have done a bad review here, wasn't staring at the specs from the right angle.
So, PCI standard has register 0x0e, PCI header type. Based on the bottom 7 bits of that register, the layout of _standard_ PCI registers change! The subsystem ID we discuss here will have a _standard_ location of either 0x2c or 0x94.
Based on this information, I would now suggest a function named pci_bridge_set_subsystem() to be located in device/pci.c. It's not about PCIe, it's about the function exposing its configuration layout with PCI bridge layout.
You may want to wait for second opinions here...
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
Patch Set 5:
One more thing, MPC configuration bit BT. These PCIe root ports may, in some configurations, appear with Type 0 header. The way I see it, datasheets only describe the register layout for the typical case of Type 1 header, and forget to mention it does not apply when this MPC.BT is set. Do you agree? I don't have a convenient way to test this.
As a conclusion: we should not hard-wire these PCIe root ports to use register 0x94 for SSID programming, but sometimes need to use 0x2c instead. I'll do a followup once we have this merged.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
Patch Set 5:
Patch Set 5:
Subrata, apologies, I may have done a bad review here, wasn't staring at the specs from the right angle.
So, PCI standard has register 0x0e, PCI header type. Based on the bottom 7 bits of that register, the layout of _standard_ PCI registers change! The subsystem ID we discuss here will have a _standard_ location of either 0x2c or 0x94.
Based on this information, I would now suggest a function named pci_bridge_set_subsystem() to be located in device/pci.c. It's not about PCIe, it's about the function exposing its configuration layout with PCI bridge layout.
You may want to wait for second opinions here...
Isn't the 0x94 address a very Intel implementation specific thing? i.e. according to PCI spec, there can be three header types: 0 - Normal Non-bridge devices 1 - PCI-PCI bridge 2 - PCI-Cardbus bridge
For type 0, standard location is 0x2c. For type 2, standard location is 0x40. For type 1, it is decided by the Capabilities pointer. Based on the offset of capability ID 0xD (bridge subsystem vendor/device ID capability), vendor id is located at offset 0x4 within that capability. Depending upon the implementation, capability 0xD can be at 0x90 or some other offset. I couldn't find the text where it states that the vendor id for bridge has to be at 0x94.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
Patch Set 5:
Isn't the 0x94 address a very Intel implementation specific thing? i.e. according to PCI spec, there can be three header types:
Ahh.. right. So even for Intel hardware, the more correct solution would have been to follow the capabilities list, instead of just hardcoding 0x94 there. It would have been nice to see that as a comment in the original code but hey..
0 - Normal Non-bridge devices 1 - PCI-PCI bridge 2 - PCI-Cardbus bridge
For type 0, standard location is 0x2c. For type 2, standard location is 0x40. For type 1, it is decided by the Capabilities pointer. Based on the offset of capability ID 0xD (bridge subsystem vendor/device ID capability), vendor id is located at offset 0x4 within that capability. Depending upon the implementation, capability 0xD can be at 0x90 or some other offset. I couldn't find the text where it states that the vendor id for bridge has to be at 0x94.
So.. I think we should implement one generic pci_set_subsystem(dev), that checks dev->hdr_type, and supports all the three types above in one function. For type 1, it needs to do pci_find_capability() with ID 0x0D.
For some hardware this may still not be right, so enter this via dev->ops?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
Patch Set 5:
Isn't the 0x94 address a very Intel implementation specific
thing? i.e. according to PCI spec, there can be three header types:
Ahh.. right. So even for Intel hardware, the more correct solution would have been to follow the capabilities list, instead of just hardcoding 0x94 there. It would have been nice to see that as a comment in the original code but hey..
0 - Normal Non-bridge devices 1 - PCI-PCI bridge 2 - PCI-Cardbus bridge
For type 0, standard location is 0x2c. For type 2, standard location is 0x40. For type 1, it is decided by the Capabilities pointer. Based on
the offset of capability ID 0xD (bridge subsystem vendor/device ID capability), vendor id is located at offset 0x4 within that capability. Depending upon the implementation, capability 0xD can be at 0x90 or some other offset. I couldn't find the text where it states that the vendor id for bridge has to be at 0x94.
So.. I think we should implement one generic pci_set_subsystem(dev), that checks dev->hdr_type, and supports all the three types above in one function. For type 1, it needs to do pci_find_capability() with ID 0x0D.
For some hardware this may still not be right, so enter this via dev->ops?
valid argument and i'm working on that now.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
Patch Set 5:
For some hardware this may still not be right, so enter this via dev->ops?
valid argument and i'm working on that now.
Thanks! I would do this two patches:
All the sites that only write on 0x2c (PCI_SUBSYSTEM_VENDOR_ID) can call the already existing pci_dev_set_subsystem() function. I believe PCI specification does not allow SSID of 0000:0000 and this function can be adjusted to copy PCI_VENDOR_ID pair in that case, like done in lynxpoint/lpc.c and various other files.
Second patch would add evaluating dev->hdr_type in this function. Once that is done, all the callsites that previously referred 0x94 can also call this. I can got through AMD hardware once this is submitted. Sounds like a plan?
Also, sb/ti/pci1x2x() has a sample of cardbus with an additional lock bit required. Just to remind us that one generic function will not work for all.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: device/pciexp_device: Add set_subsystem() for pciexp device ......................................................................
Patch Set 5:
For some hardware this may still not be right, so enter this
via
dev->ops?
valid argument and i'm working on that now.
Thanks! I would do this two patches:
All the sites that only write on 0x2c (PCI_SUBSYSTEM_VENDOR_ID) can call the already existing pci_dev_set_subsystem() function. I believe PCI specification does not allow SSID of 0000:0000 and this function can be adjusted to copy PCI_VENDOR_ID pair in that case, like done in lynxpoint/lpc.c and various other files.
Second patch would add evaluating dev->hdr_type in this function. Once that is done, all the callsites that previously referred 0x94 can also call this. I can got through AMD hardware once this is submitted. Sounds like a plan?
Also, sb/ti/pci1x2x() has a sample of cardbus with an additional lock bit required. Just to remind us that one generic function will not work for all.
cool, on it.
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Duncan Laurie, build bot (Jenkins), Lijian Zhao, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31950
to look at the new patch set (#6).
Change subject: {northbridge, soc, southbridge}/intel: Make use of generic set_subsystem() ......................................................................
{northbridge, soc, southbridge}/intel: Make use of generic set_subsystem()
This patch removes all local definitions of sub_system functions and make use of common generic pci_dev_set_subsystem() from PCI bridge and Cardbus devices as well.
Change-Id: I5fbed39ed448baf11f0e0786ce0ee94741d57237 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/northbridge/intel/sandybridge/pcie.c M src/soc/intel/broadwell/pcie.c M src/soc/intel/common/block/pcie/pcie.c M src/southbridge/intel/bd82x6x/pcie.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801ix/pcie.c M src/southbridge/intel/i82801jx/pcie.c M src/southbridge/intel/lynxpoint/pcie.c M src/southbridge/ricoh/rl5c476/rl5c476.c M src/southbridge/ti/pci1x2x/pci1x2x.c 10 files changed, 11 insertions(+), 109 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/31950/6
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: {northbridge, soc, southbridge}/intel: Make use of generic set_subsystem() ......................................................................
Patch Set 6:
There are some more PCI bridge functions in sb/intel/pci.c that write register 0x54.
git grep 0x54 -- src/southbridge/intel/*pci.c
We can deal with them separately. There is some added complexity in some of them.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: {northbridge, soc, southbridge}/intel: Make use of generic set_subsystem() ......................................................................
Patch Set 6: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: {northbridge, soc, southbridge}/intel: Make use of generic set_subsystem() ......................................................................
Patch Set 6: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31950 )
Change subject: {northbridge, soc, southbridge}/intel: Make use of generic set_subsystem() ......................................................................
{northbridge, soc, southbridge}/intel: Make use of generic set_subsystem()
This patch removes all local definitions of sub_system functions and make use of common generic pci_dev_set_subsystem() from PCI bridge and Cardbus devices as well.
Change-Id: I5fbed39ed448baf11f0e0786ce0ee94741d57237 Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31950 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-by: Furquan Shaikh furquan@google.com --- M src/northbridge/intel/sandybridge/pcie.c M src/soc/intel/broadwell/pcie.c M src/soc/intel/common/block/pcie/pcie.c M src/southbridge/intel/bd82x6x/pcie.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801ix/pcie.c M src/southbridge/intel/i82801jx/pcie.c M src/southbridge/intel/lynxpoint/pcie.c M src/southbridge/ricoh/rl5c476/rl5c476.c M src/southbridge/ti/pci1x2x/pci1x2x.c 10 files changed, 11 insertions(+), 109 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, but someone else must approve Furquan Shaikh: Looks good to me, approved
diff --git a/src/northbridge/intel/sandybridge/pcie.c b/src/northbridge/intel/sandybridge/pcie.c index 16bc314..344cd80 100644 --- a/src/northbridge/intel/sandybridge/pcie.c +++ b/src/northbridge/intel/sandybridge/pcie.c @@ -65,20 +65,8 @@ } #endif
-static void -pcie_set_subsystem(struct device *dev, unsigned int ven, unsigned int device) -{ - /* NOTE: This is not the default position! */ - if (!ven || !device) - pci_write_config32(dev, 0x94, - pci_read_config32(dev, 0)); - else - pci_write_config32(dev, 0x94, - ((device & 0xffff) << 16) | (ven & 0xffff)); -} - static struct pci_operations pci_ops = { - .set_subsystem = pcie_set_subsystem, + .set_subsystem = pci_dev_set_subsystem, };
static struct device_operations device_ops = { diff --git a/src/soc/intel/broadwell/pcie.c b/src/soc/intel/broadwell/pcie.c index 32135ee..472e8da 100644 --- a/src/soc/intel/broadwell/pcie.c +++ b/src/soc/intel/broadwell/pcie.c @@ -649,16 +649,6 @@ root_port_commit_config(); }
-static void pcie_set_subsystem(struct device *dev, unsigned int vendor, - unsigned int device) -{ - /* NOTE: This is not the default position! */ - if (!vendor || !device) - pci_write_config32(dev, 0x94, pci_read_config32(dev, 0)); - else - pci_write_config32(dev, 0x94, (device << 16) | vendor); -} - static void pcie_set_L1_ss_max_latency(struct device *dev, unsigned int off) { /* Set max snoop and non-snoop latency for Broadwell */ @@ -666,7 +656,7 @@ }
static struct pci_operations pcie_ops = { - .set_subsystem = pcie_set_subsystem, + .set_subsystem = pci_dev_set_subsystem, .set_L1_ss_latency = pcie_set_L1_ss_max_latency, };
diff --git a/src/soc/intel/common/block/pcie/pcie.c b/src/soc/intel/common/block/pcie/pcie.c index 8295765..d74b098 100644 --- a/src/soc/intel/common/block/pcie/pcie.c +++ b/src/soc/intel/common/block/pcie/pcie.c @@ -25,8 +25,6 @@ #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 -/* PCI-E Sub-System ID */ -#define PCIE_SUBSYSTEM_VENDOR_ID 0x94
static void pch_pcie_init(struct device *dev) { @@ -72,16 +70,9 @@ PCIE_LTR_MAX_SNOOP_LATENCY_VALUE); }
-static void pcie_dev_set_subsystem(struct device *dev, - unsigned int vendor, unsigned int device) -{ - pci_write_config32(dev, PCIE_SUBSYSTEM_VENDOR_ID, - ((device & 0xffff) << 16) | (vendor & 0xffff)); -} - static struct pci_operations pcie_ops = { .set_L1_ss_latency = pcie_set_L1_ss_max_latency, - .set_subsystem = pcie_dev_set_subsystem, + .set_subsystem = pci_dev_set_subsystem, };
static struct device_operations device_ops = { diff --git a/src/southbridge/intel/bd82x6x/pcie.c b/src/southbridge/intel/bd82x6x/pcie.c index 39c53e8..0bc75b5 100644 --- a/src/southbridge/intel/bd82x6x/pcie.c +++ b/src/southbridge/intel/bd82x6x/pcie.c @@ -306,21 +306,8 @@ return NULL; }
-static void pcie_set_subsystem(struct device *dev, unsigned vendor, - unsigned device) -{ - /* NOTE: This is not the default position! */ - if (!vendor || !device) { - pci_write_config32(dev, 0x94, - pci_read_config32(dev, 0)); - } else { - pci_write_config32(dev, 0x94, - ((device & 0xffff) << 16) | (vendor & 0xffff)); - } -} - static struct pci_operations pci_ops = { - .set_subsystem = pcie_set_subsystem, + .set_subsystem = pci_dev_set_subsystem, };
static struct device_operations device_ops = { diff --git a/src/southbridge/intel/i82801gx/pcie.c b/src/southbridge/intel/i82801gx/pcie.c index 9446527..3e5dbc3 100644 --- a/src/southbridge/intel/i82801gx/pcie.c +++ b/src/southbridge/intel/i82801gx/pcie.c @@ -252,22 +252,8 @@ root_port_commit_config(dev); }
- -static void pcie_set_subsystem(struct device *dev, unsigned int vendor, - unsigned int device) -{ - /* NOTE: This is not the default position! */ - if (!vendor || !device) { - pci_write_config32(dev, 0x94, - pci_read_config32(dev, 0)); - } else { - pci_write_config32(dev, 0x94, - ((device & 0xffff) << 16) | (vendor & 0xffff)); - } -} - static struct pci_operations pci_ops = { - .set_subsystem = pcie_set_subsystem, + .set_subsystem = pci_dev_set_subsystem, };
static struct device_operations device_ops = { diff --git a/src/southbridge/intel/i82801ix/pcie.c b/src/southbridge/intel/i82801ix/pcie.c index a36fdc6..3b90ce6 100644 --- a/src/southbridge/intel/i82801ix/pcie.c +++ b/src/southbridge/intel/i82801ix/pcie.c @@ -95,19 +95,6 @@ } }
-static void pcie_set_subsystem(struct device *dev, unsigned vendor, - unsigned device) -{ - /* NOTE: 0x94 is not the default position! */ - if (!vendor || !device) { - pci_write_config32(dev, 0x94, - pci_read_config32(dev, 0)); - } else { - pci_write_config32(dev, 0x94, - ((device & 0xffff) << 16) | (vendor & 0xffff)); - } -} - static void pch_pciexp_scan_bridge(struct device *dev) { struct southbridge_intel_i82801ix_config *config = dev->chip_info; @@ -121,7 +108,7 @@ }
static struct pci_operations pci_ops = { - .set_subsystem = pcie_set_subsystem, + .set_subsystem = pci_dev_set_subsystem, };
static struct device_operations device_ops = { diff --git a/src/southbridge/intel/i82801jx/pcie.c b/src/southbridge/intel/i82801jx/pcie.c index fb90cd9..84b2b6a 100644 --- a/src/southbridge/intel/i82801jx/pcie.c +++ b/src/southbridge/intel/i82801jx/pcie.c @@ -95,19 +95,6 @@ } }
-static void pcie_set_subsystem(struct device *dev, unsigned vendor, - unsigned device) -{ - /* NOTE: 0x94 is not the default position! */ - if (!vendor || !device) { - pci_write_config32(dev, 0x94, - pci_read_config32(dev, 0)); - } else { - pci_write_config32(dev, 0x94, - ((device & 0xffff) << 16) | (vendor & 0xffff)); - } -} - static void pch_pciexp_scan_bridge(struct device *dev) { struct southbridge_intel_i82801jx_config *config = dev->chip_info; @@ -121,7 +108,7 @@ }
static struct pci_operations pci_ops = { - .set_subsystem = pcie_set_subsystem, + .set_subsystem = pci_dev_set_subsystem, };
static struct device_operations device_ops = { diff --git a/src/southbridge/intel/lynxpoint/pcie.c b/src/southbridge/intel/lynxpoint/pcie.c index 695abf2..2a8b44e 100644 --- a/src/southbridge/intel/lynxpoint/pcie.c +++ b/src/southbridge/intel/lynxpoint/pcie.c @@ -727,21 +727,8 @@ root_port_commit_config(); }
-static void pcie_set_subsystem(struct device *dev, unsigned vendor, - unsigned device) -{ - /* NOTE: This is not the default position! */ - if (!vendor || !device) { - pci_write_config32(dev, 0x94, - pci_read_config32(dev, 0)); - } else { - pci_write_config32(dev, 0x94, - ((device & 0xffff) << 16) | (vendor & 0xffff)); - } -} - static struct pci_operations pci_ops = { - .set_subsystem = pcie_set_subsystem, + .set_subsystem = pci_dev_set_subsystem, };
static struct device_operations device_ops = { diff --git a/src/southbridge/ricoh/rl5c476/rl5c476.c b/src/southbridge/ricoh/rl5c476/rl5c476.c index 4d8b6e6..c94722c 100644 --- a/src/southbridge/ricoh/rl5c476/rl5c476.c +++ b/src/southbridge/ricoh/rl5c476/rl5c476.c @@ -200,8 +200,8 @@ /* Enable subsystem id register writes */ pci_write_config16(dev, 0x82, miscreg | 0x40);
- pci_write_config16(dev, 0x40, vendor); - pci_write_config16(dev, 0x42, device); + pci_dev_set_subsystem(dev, vendor, device); + /* restore original contents */ pci_write_config16(dev, 0x82, miscreg); } diff --git a/src/southbridge/ti/pci1x2x/pci1x2x.c b/src/southbridge/ti/pci1x2x/pci1x2x.c index f84d866..bfb5ab9 100644 --- a/src/southbridge/ti/pci1x2x/pci1x2x.c +++ b/src/southbridge/ti/pci1x2x/pci1x2x.c @@ -46,8 +46,7 @@ * to the sub-vendor/device ids at 40 and 42. */ pci_write_config32(dev, 0x80, pci_read_config32(dev, 0x080) & ~0x10); - pci_write_config16(dev, 0x40, vendor); - pci_write_config16(dev, 0x42, device); + pci_dev_set_subsystem(dev, vendor, device); pci_write_config32(dev, 0x80, pci_read_config32(dev, 0x80) | 0x10); }
-
Furquan Shaikh (Code Review) -
Kyösti Mälkki (Code Review)
-
Lijian Zhao (Code Review)
-
Patrick Georgi (Code Review)
-
Subrata Banik (Code Review)