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: soc/amd/common/block/rtd3: Add AMD specific RTD3 driver ......................................................................
Patch Set 1:
(3 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/54966/comment/593faf6e_f17830c5 PS1, Line 9: I decided : to copy and modify it because the Intel driver has a lot of Intel : specific code. From a quick look, I see that the following things were omitted:
1. ExternalFacingPort: Even though we don't need it right now, I think this might be needed on AMD platforms too at some point. So, it shouldn't hurt to have this.
2. HotPlugSupportInD3: See Duncan's comment here: https://review.coreboot.org/c/coreboot/+/46260/comment/f1e1d4b1_64e48a36/. I think we might need this for AMD platforms too.
3. L23 entry/exit and PMC communication in _ON/_OFF: PMC communication is definitely Intel-specific. But, L23 entry/exit might apply to AMD platforms too.
Overall, I think we can still have a common driver if we provide enough flexibility for platforms. Given that the flexibility is mostly required in _ON/_OFF routines, would it make sense to move the driver to common location and have SoC callbacks for pcie_rtd3_acpi_method_off/pcie_rtd3_acpi_method_on?
File src/soc/amd/common/block/rtd3/rtd3.c:
https://review.coreboot.org/c/coreboot/+/54966/comment/f6e2b7af_17ceb425 PS1, Line 160: "_PR2", "_PR3"} Duncan had added a comment about not adding _PR3 to this device: https://review.coreboot.org/c/coreboot/+/46260/comment/1aab6aea_41d9a99a/
https://review.coreboot.org/c/coreboot/+/54966/comment/5888c075_c832b2a2 PS1, Line 210: pci_dev_read_resources This assumes that the PCI device under the GPP bridge is of type PCI_HEADER_TYPE_NORMAL. I think one of the reasons why a generic device was used to emit the required SSDT _DSD in Intel driver was to let the pci_device.c logic take care of identifying the right set of device_operations for the downstream device. Instead a fake generic device was added under the root port which allows adding of the required properties to the upstream bridge device. I think we should use a similar logic here?