[PATCH] nvme: put some AMD PCIE downstream NVME device to simple suspend/resume path

[Liang, Prike]

[AMD Public Use]

> From: Greg KH <gregkh at linuxfoundation.org>
> Sent: Tuesday, April 13, 2021 5:12 PM
> To: Liang, Prike <Prike.Liang at amd.com>
> Cc: linux-nvme at lists.infradead.org; kbusch at kernel.org;
> Chaitanya.Kulkarni at wdc.com; stable at vger.kernel.org; S-k, Shyam-sundar
> <Shyam-sundar.S-k at amd.com>; Deucher, Alexander
> <Alexander.Deucher at amd.com>
> Subject: Re: [PATCH] nvme: put some AMD PCIE downstream NVME device to
> simple suspend/resume path
>
> On Tue, Apr 13, 2021 at 05:00:41PM +0800, Prike Liang wrote:
> > The NVME device pluged in some AMD PCIE root port will resume timeout
> > from s2idle which caused by NVME power CFG lost in the SMU FW restore.
> > This issue can be workaround by using PCIe power set with simple
> > suspend/resume process path instead of APST. In the onwards ASIC will
> > try do the NVME shutdown save and restore in the BIOS and still need
> > PCIe power setting to resume from RTD3 for s2idle.
> >
> > Signed-off-by: Prike Liang <Prike.Liang at amd.com>
> > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k at amd.com>
> > Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
> > Cc: <stable at vger.kernel.org> # 5.11+
> > ---
> >  drivers/nvme/host/pci.c |  5 +++++
> >  drivers/pci/quirks.c    | 10 ++++++++++
> >  include/linux/pci.h     |  2 ++
> >  include/linux/pci_ids.h |  1 +
> >  4 files changed, 18 insertions(+)
> >
> > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index
> > 6bad4d4..dd46d9e 100644
> > --- a/drivers/nvme/host/pci.c
> > +++ b/drivers/nvme/host/pci.c
> > @@ -2832,6 +2832,7 @@ static bool nvme_acpi_storage_d3(struct pci_dev
> > *dev)  {
> >  struct acpi_device *adev;
> >  struct pci_dev *root;
> > +struct pci_dev *rdev;
> >  acpi_handle handle;
> >  acpi_status status;
> >  u8 val;
> > @@ -2845,6 +2846,10 @@ static bool nvme_acpi_storage_d3(struct
> pci_dev *dev)
> >  if (!root)
> >  return false;
> >
> > +rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0));
> > +if (rdev->dev_flags &
> PCI_DEV_FLAGS_AMD_NVME_SIMPLE_SUSPEND)
> > +return NVME_QUIRK_SIMPLE_SUSPEND;
> > +
> >  adev = ACPI_COMPANION(&root->dev);
> >  if (!adev)
> >  return false;
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index
> > 653660e3..0b175ce 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -312,6 +312,16 @@ static void quirk_nopciamd(struct pci_dev *dev)
> > }
> >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD,
> PCI_DEVICE_ID_AMD_8151_0,quirk_nopciamd);
> >
> > +static void quirk_amd_nvme_fixup(struct pci_dev *dev) {
> > +struct pci_dev *rdev;
> > +
> > +dev->dev_flags |= PCI_DEV_FLAGS_AMD_NVME_SIMPLE_SUSPEND;
> > +pci_info(dev, "AMD simple suspend opt enabled\n");
> > +
> > +}
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD,
> PCI_DEVICE_ID_AMD_CZN_RP,
> > +quirk_amd_nvme_fixup);
> > +
> >  /* Triton requires workarounds to be used by the drivers */  static
> > void quirk_triton(struct pci_dev *dev)  { diff --git
> > a/include/linux/pci.h b/include/linux/pci.h index 53f4904..a6e1b1b
> > 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -227,6 +227,8 @@ enum pci_dev_flags {
> >  PCI_DEV_FLAGS_NO_FLR_RESET = (__force pci_dev_flags_t) (1 << 10),
> >  /* Don't use Relaxed Ordering for TLPs directed at this device */
> >  PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t)
> (1 <<
> > 11),
> > +/* AMD simple suspend opt quirk */
> > +PCI_DEV_FLAGS_AMD_NVME_SIMPLE_SUSPEND = (__force
> pci_dev_flags_t) (1
> > +<< 12),
> >  };
> >
> >  enum pci_irq_reroute_variant {
> > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index
> > d8156a5..a82443f 100644
> > --- a/include/linux/pci_ids.h
> > +++ b/include/linux/pci_ids.h
> > @@ -602,6 +602,7 @@
> >  #define PCI_DEVICE_ID_AMD_HUDSON2_SMBUS0x780b
> >  #define PCI_DEVICE_ID_AMD_HUDSON2_IDE0x780c
> >  #define PCI_DEVICE_ID_AMD_KERNCZ_SMBUS  0x790b
> > +#define PCI_DEVICE_ID_AMD_CZN_RP0x1630
>
> Why add this here when it is not needed in this file?  Please read the top of
> the file...
>
[Prike]  I'm sorry can't get you meaning before. Do you mean the pci_id header used only for the global PCI ID definition and in this case only need put the 0x1630 DID in the quirk entry directly?

> thanks,
>
> greg k-h