[PATCH] nvme-pci: set some AMD PCIe downstream storage device to D3 for s2idle

[Limonciello, Mario]

[Public]

> > Any ideas on how to handle this at that platform level?  We need to select the
> NVME_QUIRK_SIMPLE_SUSPEND flag on certain AMD platforms.  This is a
> platform firmware requirement.  It's not an NVME specific requirement, it's not
> a PCIe specific requirement, it's a platform specific requirement.  DMI matching
> doesn't really make sense because it affects all AMD platforms of a certain
> generation.
> 
> Honestly I can understand that Christoph is a bit unhappy about this, but
> the nvme driver seems like the right place for this to me and it is
> already doing DMI based quirking for 1 specific Lenovo model, I don't
> see why that would be ok, but a CPU-id based check would not be ok.
> 

Yes - that was my point mentioned in the previous thread that led to this 
patch.

> Both are ugly, yet both are unfortunately sometimes necessary.
> 
> Reading Christoph's reply a second time though, I believe that what
> Christoph is trying to say, that this seems to be related to some
> special suspend demands stemming from using the pci-e root ports
> from the CPU, if instead the NVME device where situated behind
> some pci-e switch, then the link to that switch would need to
> power-down for the CPU's deep-sleep demands to be met, so this
> really should be a (probably new?) flag on the pci-e parent of the
> NVME device and then the nvme/host/pci.c code would set the
> NVME_QUIRK_SIMPLE_SUSPEND flag based on the flag on its pci-e
> parent, rather then having the CPU-id check inside the nvme code.
> 
> IOW I believe what Christoph is suggesting is the following:
> 
> 1. Add some new flag (or some-such) to pci-e ports/links inside
> Linux to signal the requirement for the link to be turned off
> during suspend (or whatever the requirement actually is).
> 
> 2. Make the drivers/pci code set that flag on the pci-e root
> ports of the AMD CPUs which need this (based on the CPU-id as
> done in the original patch).
> 
> 3. Have the nvme/host/pci.c code would set the
> NVME_QUIRK_SIMPLE_SUSPEND flag based on the new pci-e port/link
> flag.

Actually that's what was proposed in earlier patch series, but Bjorn
wasn't happy with introducing a new quirk just for it, especially with it
being an issue stemming from a particular silicon generation only and was
pushing that it should be included in NVME driver directly.

> 
> Christoph have I understood correctly that this is more or less
> what you are asking for ?
> 
> Note I don't know it this actually makes sense, because I don't
> know all the details here. I believe that AMD is in the best
> position to decide if this makes sense or not.
> 

The back story here is that it's nothing to do with PCIe or NVME, but
an issue with what happens with the SMU in the SOC.

Quoting an earlier version commit message:

"Then the NVMe device will be shutdown by SMU firmware in the s2idle entry
and then will lost the NVMe power context during s2idle resume. Finally,
the NVMe command queue request will be processed abnormally and result
in access timeout"

Which I think begs the question - how about if we keep the quirks list and logic
outside of NVME and also outside of PCI but instead in an AMD owned platform
driver?  Something like this:

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index a29b170701fc..ea3c8772cb11 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2837,6 +2837,12 @@ static bool nvme_acpi_storage_d3(struct pci_dev *dev)
        acpi_status status;
        u8 val;

+       /* Some AMD platforms need to use D3 due to SMU behavior */
+#if IS_REACHABLE(CONFIG_AMD_PMC)
+       if (amd_pmc_should_use_d3())
+               return true;
+#endif
+
        /*
         * Look for _DSD property specifying that the storage device on the port
         * must use D3 to support deep platform power savings during