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

[David E. Box]

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