Hi Raul,
On Wed, 2021-05-26 at 11:53 -0600, Raul Rangel wrote:
> On Thu, Jul 09, 2020 at 11:43:33AM -0700, David E. Box wrote:
> > +#ifdef CONFIG_ACPI
> > +static bool nvme_acpi_storage_d3(struct pci_dev *dev)
> > +{
> > + const struct fwnode_handle *fwnode;
> > + struct acpi_device *adev;
> > + struct pci_dev *root;
> > + acpi_handle handle;
> > + acpi_status status;
> > + u8 val;
> > +
> > + /*
> > + * Look for _DSD property specifying that the storage device
> > on
> > + * the port must use D3 to support deep platform power
> > savings during
> > + * suspend-to-idle
> > + */
> > + root = pcie_find_root_port(dev);
> > + if (!root)
> > + return false;
> > +
> > + adev = ACPI_COMPANION(&root->dev);
> > + if (!adev)
> > + return false;
> > +
> > + /*
> > + * The property is defined in the PXSX device for South
> > complex ports
> > + * and in the PEGP device for North complex ports.
> > + */
> > + status = acpi_get_handle(adev->handle, "PXSX", &handle);
> So I'm curious why we need to directly look at the PXSX and PEGP
> devices instead of the ACPI_COMPANION node attached to the pci
> device?
>
> I've looked around and I can't find any documentation that defines
> the PXSX and PEGP device names.
>
> I've dumped some ACPI from a system that uses the PXSX name and
> StorageD3Cold attribute:
>
> Scope (\_SB.PCI0.GP14)
> {
> Device (PXSX)
> {
> Name (_ADR, 0x0000000000000000) // _ADR: Address
> Method (_STA, 0, NotSerialized) // _STA: Status
> {
> Return (0x0F)
> }
>
> Name (_DSD, Package (0x02) // _DSD: Device-Specific Data
> {
> ToUUID ("5025030f-842f-4ab4-a561-99a5189762d0"),
> Package (0x01)
> {
> Package (0x02)
> {
> "StorageD3Enable",
> One
> }
> }
> })
> }
> }
>
> It looks to me like it's just the firmware node for the NVMe device
> attached to the root port. Is that the correct assumption?
>
> I'm wondering if we can simplify the look up logic to look at the
> ACPI_COMPANION of the pci device?
I believe so, but I'd need to confirm on our systems that it will work.
I recall trying to use the companion device and not being able to
locate the _DSD. But that was on a preproduction platform at the time.
>
> The reason I ask is that I'm working on enabling S0i3 on an AMD
> device.
> This device also defines the StorageD3Enable property, but it don't
> use
> the PXSX name:
>
> Scope (GPP6) {
> Device (NVME)
> {
> Name (_ADR, Zero) // _ADR: Address
>
> Name (_DSD, Package (0x02) // _DSD: Device-Specific Data
> {
> ToUUID ("5025030f-842f-4ab4-a561-99a5189762d0"),
> Package (0x01)
> {
> Package (0x02)
> {
> "StorageD3Enable",
> One
> }
> }
> })
> }
> }
>
> The Windows
> [documentation](
> https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro#d3-support
> )
> makes it sound like the _DSD should be defined on the PCI device.
>
> I can send one of the following patches depending on the feedback:
> 1) Additionally check the pci device's ACPI_COMPANION for the _DSD.
> 2) Delete the PXSX and PEGP lookups and only look at the pci device's
> ACPI_COMPANION.
>
> > + if (ACPI_FAILURE(status)) {
> > + status = acpi_get_handle(adev->handle, "PEGP",
> > &handle);
> > + if (ACPI_FAILURE(status))
> > + return false;
> > + }
> > +
> > + if (acpi_bus_get_device(handle, &adev))
> > + return false;
> > +
> > + fwnode = acpi_fwnode_handle(adev);
> > +
> > + return fwnode_property_read_u8(fwnode, "StorageD3Enable",
> > &val) ?
> > + false : val == 1;
> > +}
Go for 2 first. I will check on those systems again with our latest
BIOS to ensure it works.
David
>
> Thanks,
> Raul
