Change in ...coreboot[master]: device/pci_device: Add generic subsystem programming logic

[Subrata Banik (Code Review)]

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31983

Change subject: device/pci_device: Add generic subsystem programming logic
......................................................................

device/pci_device: Add generic subsystem programming logic

This patch adds generic log to perform subsystem programming
based on header type.

Type 0: subsystem offset 0x2C
Type 2: subsystem offset 0x40
Type 1: Read CAP ID 0xD to know cap offset start, offset 4 to locate
subsystem vendor id.

Change-Id: Id8aed6dac24517e93cd55d6bb3b254b7b4d950d3
Signed-off-by: Subrata Banik 
---
M src/device/pci_device.c
M src/include/device/pci_def.h
2 files changed, 21 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/31983/1

diff --git a/src/device/pci_device.c b/src/device/pci_device.c
index 86c72b8..a1a05a9 100644
--- a/src/device/pci_device.c
+++ b/src/device/pci_device.c
@@ -734,7 +734,26 @@
 void pci_dev_set_subsystem(struct device *dev, unsigned int vendor,
    		   unsigned int device)
 {
-	pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID,
+	uint8_t offset;
+
+	/* Header type */
+	switch (dev->hdr_type & 0x7f) {
+	case PCI_HEADER_TYPE_NORMAL:
+		offset = PCI_SUBSYSTEM_VENDOR_ID;
+		break;
+	case PCI_HEADER_TYPE_BRIDGE:
+		unsigned int cap_offset;
+		cap_offset = pci_find_capability(dev, PCI_CAP_ID_SSVID);
+		offset = cap_offset + 4; /* Vendor ID at offset 4 */
+		break;
+	case PCI_HEADER_TYPE_CARDBUS:
+		offset = PCI_CB_SUBSYSTEM_VENDOR_ID;
+		break;
+	default:
+		return;
+	}
+
+	pci_write_config32(dev, offset,
    		   ((device & 0xffff) << 16) | (vendor & 0xffff));
 }

diff --git a/src/include/device/pci_def.h b/src/include/device/pci_def.h
index f9ce1a6..bc5bc79 100644
--- a/src/include/device/pci_def.h
+++ b/src/include/device/pci_def.h
@@ -198,6 +198,7 @@
 #define  PCI_CAP_ID_HT          0x08	/* Hypertransport */
 #define  PCI_CAP_ID_EHCI_DEBUG	0x0A	/* EHCI debug port */
 #define  PCI_CAP_ID_SHPC	0x0C	/* PCI Standard Hot-Plug Controller */
+#define  PCI_CAP_ID_SSVID	0x0D	/* Bridge subsystem vendor/device ID */
 #define  PCI_CAP_ID_PCIE	0x10	/* PCI Express */
 #define  PCI_CAP_ID_MSIX	0x11	/* MSI-X */
 #define PCI_CAP_LIST_NEXT	1	/* Next capability in the list */

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31983 )

Change subject: device/pci_device: Add generic subsystem programming logic
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31983/1/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/#/c/31983/1/src/device/pci_device.c@746 
PS1, Line 746: 		cap_offset = pci_find_capability(dev, PCI_CAP_ID_SSVID);
It will return 0 if missing, should not happen but must skip the write in that case. Also, you can just reuse 'offset' here.

[Subrata Banik (Code Review)]

Hello Kyösti Mälkki, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31983

to look at the new patch set (#2).

Change subject: device/pci_device: Add generic subsystem programming logic
......................................................................

device/pci_device: Add generic subsystem programming logic

This patch adds generic log to perform subsystem programming
based on header type.

Type 0: subsystem offset 0x2C
Type 2: subsystem offset 0x40
Type 1: Read CAP ID 0xD to know cap offset start, offset 4 to locate
subsystem vendor id.

Change-Id: Id8aed6dac24517e93cd55d6bb3b254b7b4d950d3
Signed-off-by: Subrata Banik 
---
M src/device/pci_device.c
M src/include/device/pci_def.h
2 files changed, 20 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/31983/2

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31983 )

Change subject: device/pci_device: Add generic subsystem programming logic
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31983/1/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/#/c/31983/1/src/device/pci_device.c@746 
PS1, Line 746: 		cap_offset = pci_find_capability(dev, PCI_CAP_ID_SSVID);
> It will return 0 if missing, should not happen but must skip the write in that case. […]

offset one i have fixed.

Didn't get this one
> > It will return 0 if missing, should not happen but must skip the write in that case.

do you mean like below?
if (!offset)
  return;

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31983 )

Change subject: device/pci_device: Add generic subsystem programming logic
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31983/1/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/#/c/31983/1/src/device/pci_device.c@746 
PS1, Line 746: 		cap_offset = pci_find_capability(dev, PCI_CAP_ID_SSVID);
> offset one i have fixed.  […]

Exactly that.

[Subrata Banik (Code Review)]

Hello Kyösti Mälkki, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31983

to look at the new patch set (#3).

Change subject: device/pci_device: Add generic subsystem programming logic
......................................................................

device/pci_device: Add generic subsystem programming logic

This patch adds generic log to perform subsystem programming
based on header type.

Type 0: subsystem offset 0x2C
Type 2: subsystem offset 0x40
Type 1: Read CAP ID 0xD to know cap offset start, offset 4 to locate
subsystem vendor id.

Change-Id: Id8aed6dac24517e93cd55d6bb3b254b7b4d950d3
Signed-off-by: Subrata Banik 
---
M src/device/pci_device.c
M src/include/device/pci_def.h
2 files changed, 23 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/31983/3

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31983 )

Change subject: device/pci_device: Add generic subsystem programming logic
......................................................................

Patch Set 3: Code-Review+1

[David Guckian (Code Review)]

David Guckian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31983 )

Change subject: device/pci_device: Add generic subsystem programming logic
......................................................................

Patch Set 3: Code-Review+1

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31983 )

Change subject: device/pci_device: Add generic subsystem programming logic
......................................................................

Patch Set 3: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31983 )

Change subject: device/pci_device: Add generic subsystem programming logic
......................................................................

device/pci_device: Add generic subsystem programming logic

This patch adds generic log to perform subsystem programming
based on header type.

Type 0: subsystem offset 0x2C
Type 2: subsystem offset 0x40
Type 1: Read CAP ID 0xD to know cap offset start, offset 4 to locate
subsystem vendor id.

Change-Id: Id8aed6dac24517e93cd55d6bb3b254b7b4d950d3
Signed-off-by: Subrata Banik 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/31983
Tested-by: build bot (Jenkins) 
Reviewed-by: Kyösti Mälkki 
Reviewed-by: David Guckian
Reviewed-by: Furquan Shaikh 
---
M src/device/pci_device.c
M src/include/device/pci_def.h
2 files changed, 23 insertions(+), 2 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
  David Guckian: Looks good to me, but someone else must approve

diff --git a/src/device/pci_device.c b/src/device/pci_device.c
index fbe9d96..5f908d2 100644
--- a/src/device/pci_device.c
+++ b/src/device/pci_device.c
@@ -734,11 +734,31 @@
 void pci_dev_set_subsystem(struct device *dev, unsigned int vendor,
    		   unsigned int device)
 {
+	uint8_t offset;
+
+	/* Header type */
+	switch (dev->hdr_type & 0x7f) {
+	case PCI_HEADER_TYPE_NORMAL:
+		offset = PCI_SUBSYSTEM_VENDOR_ID;
+		break;
+	case PCI_HEADER_TYPE_BRIDGE:
+		offset = pci_find_capability(dev, PCI_CAP_ID_SSVID);
+		if (!offset)
+			return;
+		offset += 4; /* Vendor ID at offset 4 */
+		break;
+	case PCI_HEADER_TYPE_CARDBUS:
+		offset = PCI_CB_SUBSYSTEM_VENDOR_ID;
+		break;
+	default:
+		return;
+	}
+
    if (!vendor || !device) {
-		pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID,
+		pci_write_config32(dev, offset,
    		pci_read_config32(dev, PCI_VENDOR_ID));
    } else {
-		pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID,
+		pci_write_config32(dev, offset,
    		((device & 0xffff) << 16) | (vendor & 0xffff));
    }
 }
diff --git a/src/include/device/pci_def.h b/src/include/device/pci_def.h
index f9ce1a6..bc5bc79 100644
--- a/src/include/device/pci_def.h
+++ b/src/include/device/pci_def.h
@@ -198,6 +198,7 @@
 #define  PCI_CAP_ID_HT          0x08	/* Hypertransport */
 #define  PCI_CAP_ID_EHCI_DEBUG	0x0A	/* EHCI debug port */
 #define  PCI_CAP_ID_SHPC	0x0C	/* PCI Standard Hot-Plug Controller */
+#define  PCI_CAP_ID_SSVID	0x0D	/* Bridge subsystem vendor/device ID */
 #define  PCI_CAP_ID_PCIE	0x10	/* PCI Express */
 #define  PCI_CAP_ID_MSIX	0x11	/* MSI-X */
 #define PCI_CAP_LIST_NEXT	1	/* Next capability in the list */