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?