LinuxLists.cc - [PATCH 0/2] Add support for StorageD3Enable

2020-04-28 00:34:34

by David E. Box

[permalink] [raw]

Subject: [PATCH 0/2] Add support for StorageD3Enable _DSD property

2020-04-28 00:34:40

by David E. Box

[permalink] [raw]

On Tuesday, April 28, 2020 2:32:13 AM CEST David E. Box wrote:
> NVMe storage power management during suspend-to-idle, particularly on
> laptops, has been inconsistent with some devices working with D3 while
> others must rely on NVMe APST in order for power savings to be realized.
> Currently the default is to use APST unless quirked to do otherwise.
> However newer platforms, like Intel Comet Lake systems, may require NVMe
> drives to use D3 in order for the PCIe ports to be properly power managed.
> To make it easier for drivers to choose, these platforms may supply a
> special "StorageD3Enable" _DSD property under the root port that the device
> is attached to. If supplied, the driver must use D3 in order for the
> platform to realize the deepest power savings in suspend-to-idle.
>
> Adds support for the _DSD to the pci/acpi layer.
>
> Acked-by: Dan Williams <[email protected]>
> Link: https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro
> Signed-off-by: David E. Box <[email protected]>
> ---
> drivers/acpi/property.c | 3 +++
> drivers/pci/pci-acpi.c | 47 +++++++++++++++++++++++++++++++++++++++++
> drivers/pci/pci.c | 6 ++++++
> drivers/pci/pci.h | 4 ++++
> include/linux/pci.h | 1 +
> 5 files changed, 61 insertions(+)
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index e601c4511a8b..f09375ab40e4 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -45,6 +45,9 @@ static const guid_t prp_guids[] = {
> /* Thunderbolt GUID for WAKE_SUPPORTED: 6c501103-c189-4296-ba72-9bf5a26ebe5d */
> GUID_INIT(0x6c501103, 0xc189, 0x4296,
> 0xba, 0x72, 0x9b, 0xf5, 0xa2, 0x6e, 0xbe, 0x5d),
> + /* D3 Support for storage devivce: 5025030f-842f-4ab4-a561-99a5189762d0 */
> + GUID_INIT(0x5025030f, 0x842f, 0x4ab4,
> + 0xa5, 0x61, 0x99, 0xa5, 0x18, 0x97, 0x62, 0xd0),
> };
>
> /* ACPI _DSD data subnodes GUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index d21969fba6ab..5df249ebf022 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -972,6 +972,52 @@ static bool acpi_pci_bridge_d3(struct pci_dev *dev)
> return val == 1;
> }
>
> +static bool acpi_pci_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 = pci_find_pcie_root_port(dev);
> + if (!root)
> + return false;
> +
> + adev = ACPI_COMPANION(&root->dev);
> + if (root == dev) {
> + /*
> + * It is possible that the ACPI companion is not yet bound
> + * for the root port so look it up manually here.
> + */
> + if (!adev && !pci_dev_is_added(root))
> + adev = acpi_pci_find_companion(&root->dev);
> + }
> +
> + if (!adev)
> + return false;
> +
> + status = acpi_get_handle(adev->handle, "PXSX", &handle);
> + if (ACPI_FAILURE(status))
> + return false;
> +
> + adev = acpi_bus_get_acpi_device(handle);
> + if (!adev)
> + return false;
> +
> + fwnode = acpi_fwnode_handle(adev);
> + if (!fwnode_property_read_u8(fwnode, "StorageD3Enable", &val))
> + return val == 1;
> +
> + return false;
> +}

Kind of orthogonal to what happens to the second patch in this series, I don't
think that the PCI changes below are all needed.

IMO it would be sufficient to export the function above, maybe as
pci_acpi_storage_d3(), to drivers, so that they can call it directly as
desired.

Since _DSD return data are not allowed by the spec to change between
subsequent invocations of it, the interested driver may call this function
once at the device init time and quirk it accordingly if needed.

Cheers!

2020-05-18 13:56:09

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