Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson, Tim Wawrzynczak, Karthik Ramasubramanian, Felix Held. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54966 )
Change subject: drivers/pcie/rtd3/device: Add PCIe RTD3 driver ......................................................................
Patch Set 4:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/54966/comment/36225f44_9f346535 PS1, Line 9: I decided : to copy and modify it because the Intel driver has a lot of Intel : specific code.
- So this is where I grew uncomfortable. I'm not sure of the sequence required to put a PCIe Root Port into D3. We also don't currently support hot plugging support while the root port is in D0. I'm not sure what it would take to support hot plugging while in D3, or if it's even supported by the hardware. The AMD CRB doesn't implement any of this.
I think it is the same for Intel platforms too. At least from a hardware design standpoint, we haven't designed these ports for being hot plug capable. But, looks like it was required for the GL9755 SD express device. We can skip this for AMD platforms if you want to wait until it is really required. It would be good to at least track this somehow in case AMD platforms use the same device and run into similar issues.
So I think the Intel driver conflates the two power resources.
Your explanation about the two power resources for the root port and the downstream device makes sense.
File src/soc/amd/common/block/rtd3/rtd3.c:
https://review.coreboot.org/c/coreboot/+/54966/comment/ddb5e287_ee5128cf PS1, Line 210: pci_dev_read_resources
I wasn't a fan of the fake device because it made it difficult to understand.
The reasons for the fake/generic device under SoC internal devices have been: 1. Reuse the device/pci* support, scan/enumeration and the device_operations without having to copy the device_operations structure and related function calls in each of these device drivers. 2. Keep the organization in devicetree consistent i.e. the SoC internal devices are never surrounded by any special chip driver. example: drivers/intel/ish, drivers/wifi/generic, soc/intel/common/block/pcie/rtd.
It was confusing why it was writing `acpigen_write_ADR(0);` for the `PXSX`, but then I realized it was the PCI Address of the first device/function on the root port.
I think that is primarily to satisfy the assumption in the kernel driver about having a PXSX device. It is not really anything to do with the fake/generic device in coreboot devicetree.
Thoughts?
You mentioned that the bridge case can be ignored for now. Though that is fine, I think the problem with duplicating PCI device_operations(i.e. duplicating default_pci_ops_dev) in different device drivers is that it scatters the PCI generic operation structures in different places which can make it difficult to track and perform common changes/fixes. Also, if the drivers/pcie/rtd3/ is supposed to be any kind of PCIe device, then it might have to support all the operations including pci_rom* operations too.
-
Furquan Shaikh (Code Review)