Re: [PATCH V5] drivers/nvme: Add support for ACPI StorageD3Enable property

* Re: [PATCH V5] drivers/nvme: Add support for ACPI StorageD3Enable property
@ 2021-05-26 17:53 Raul Rangel
  2021-05-27  2:20 ` David E. Box
  0 siblings, 1 reply; 6+ messages in thread
From: Raul Rangel @ 2021-05-26 17:53 UTC (permalink / raw)
  To: David E. Box
  Cc: Rafael J. Wysocki, Len Brown, kbusch, axboe, hch, sagi,
	dan.j.williams, shyjumon.n, linux-acpi, linux-kernel, linux-nvme

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?

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;
> +}

Thanks,
Raul

^ permalink raw reply	[flat|nested] 6+ messages in thread