Attention is currently required from: Jason Glenesk, Furquan Shaikh, Marshall Dawson, Tim Wawrzynczak, Karthik Ramasubramanian, Felix Held. Raul Rangel 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:
(5 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/54966/comment/9b4bea6d_375c1a53 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: […]
So I tried refactoring the code into common, but I grew uncomfortable at the number of changes required to isolate the Intel specific parts.
1. Agree that adding this is fine. 2. 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. 3. So I think the Intel driver conflates the two power resources. * There should be a power resource on the PCIe device that toggles the power/reset GPIOs. This power resource needs _PR3, otherwise the kernel will only put the device into D3Hot. HotPlugSupportInD3 is not required for this PowerResource to be used. We do need StorageD3Enabled for NVMe when ASPM is enabled. This will tell the kernel to power off the device instead of rely on ASPM + NVMe Power States. https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/thir...
* The second power resource is for the PCIe Root Port. This power resource handles the platform specific methods to enable/disable the root port. i.e., PMC communication, L23 entry/exit. It looks like the kernel doesn't even need HotPlugSupportInD3 to power down the port? https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/thir...
Patchset:
PS1: First off, sorry for the lack of detail in the commit message. I wanted to push it out before EOD for RFC but ran out of time to elaborate.
File src/soc/amd/common/block/rtd3/rtd3.c:
https://review.coreboot.org/c/coreboot/+/54966/comment/8c1260f1_0f74fadc PS1, Line 160: "_PR2", "_PR3"}
Duncan had added a comment about not adding _PR3 to this device: https://review.coreboot. […]
See comment #11 for a response.
https://review.coreboot.org/c/coreboot/+/54966/comment/4d2636fd_687e730f PS1, Line 206: PXSX So I shot a message to the LKML yesterday asking if we could remove this hardcoded constant: https://lore.kernel.org/linux-nvme/YK6gmAWqaRmvpJXb@google.com/T/#m900552229... And I think we can. This resulted in AMD pushing the following patch: https://lore.kernel.org/linux-nvme/20210527135941.7634-1-mario.limonciello@a...
This means that the StorageD3Enable _DSD should live on the PCI device's ACPI node and we can stop using this hardcoded string.
https://review.coreboot.org/c/coreboot/+/54966/comment/a6a8a822_918796bf PS1, Line 210: pci_dev_read_resources
This assumes that the PCI device under the GPP bridge is of type PCI_HEADER_TYPE_NORMAL. […]
Ah, are you envisioning a case where we have a bridge connected to the root port?
I wasn't a fan of the fake device because it made it difficult to understand. 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.
Thinking more about this now I think I want two drivers: * drivers/pcie/rtd3/device: This is an SoC agnostic driver that deals with adding a power resource onto the PCI device and setting the `StorageD3Enable` property. Essentially what this CL is, just in a different location. This is all that would be needed for AMD right now. If we needed to support a bridge device we can either create a new driver since it's a pretty trivial driver, or we can refactor set_pci_ops so we can get the default pci ops from a device. I'm not sure if an external PCIe Bridge will need a special power resource though, so it might be best to tackle that when we need it.
* soc/{amd,intel}/common/block/rtd3/gpp: This driver would contain all the SoC specific code to implement HotPlugSupportInD3, L23, and powering on/off the GPP. This wouldn't be required for AMD in the short term since it's unclear how to even accomplish this.
Thoughts?