On Tue, Apr 13, 2021 at 06:44:08AM +0000, Liang, Prike wrote:
> [AMD Public Use]
>
> >
> > On Tue, Apr 13, 2021 at 02:10:21PM +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@xxxxxxx>
> > > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
> > > Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@xxxxxxx>
> > > Cc: <stable@xxxxxxxxxxxxxxx> # 5.11+
> > > ---
> > > drivers/nvme/host/pci.c | 5 +++++
> > > drivers/pci/quirks.c | 11 +++++++++++
> > > include/linux/pci.h | 2 ++
> > > include/linux/pci_ids.h | 2 ++
> > > 4 files changed, 20 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..b7e19bb 100644
> > > --- a/drivers/pci/quirks.c
> > > +++ b/drivers/pci/quirks.c
> > > @@ -312,6 +312,17 @@ 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);
> > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD,
> > > +PCI_DEVICE_ID_AMD_RN_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..7f6340c 100644
> > > --- a/include/linux/pci_ids.h
> > > +++ b/include/linux/pci_ids.h
> > > @@ -602,6 +602,8 @@
> > > #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
> > > +#define PCI_DEVICE_ID_AMD_RN_RP
> > PCI_DEVICE_ID_AMD_CZN_RP
> >
> > If the pci ids are identical, why do you need different entries for it?
> > Haven't you above just listed the same thing twice in the quirk entry?
> >
> > thanks,
> >
> > greg k-h
> [Prike] Use the different entries for identifying the RN/CZN respectively and that will clearly imply which ASIC need this quirk. Anyway we can use the one DID for RN/CZN to avoid the PCI ID retrieved twice.
But look at this patch, you list the same device ids in a quirk entry
twice, why???
PCI device ids should be unique per-device, and not shared with
different names like this. Also, why even add them to the .h file, you
did read the top of it, right?
thanks,
greg k-h
