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

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

[Prike Liang]

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 seting to resume from RTD3 for s2idle.

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
---
Changes in v2:
 Fix the patch format and check chip root complex DID instead of PCIe RP
  to avoid the storage device plugged in internal PCIe RP by USB adaptor.
---
 drivers/nvme/host/pci.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6bad4d4..1952da1 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -42,6 +42,9 @@
 #define NVME_MAX_KB_SZ	4096
 #define NVME_MAX_SEGS	127
 
+#define AMD_PCIE_ID_CZN 0x1630
+#define AMD_PCIE_ID_RN AMD_PCIE_ID_CZN
+
 static int use_threaded_interrupts;
 module_param(use_threaded_interrupts, int, 0);
 
@@ -70,6 +73,12 @@ static unsigned int io_queue_depth = 1024;
 module_param_cb(io_queue_depth, &io_queue_depth_ops, &io_queue_depth, 0644);
 MODULE_PARM_DESC(io_queue_depth, "set io queue depth, should >= 2");
 
+static const struct pci_device_id pcie_ids[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_RN) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_CZN) },
+	{ }
+};
+
 static int io_queue_count_set(const char *val, const struct kernel_param *kp)
 {
 	unsigned int n;
@@ -2832,6 +2841,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 +2855,15 @@ 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 && pci_match_id(pcie_ids, rdev)) {
+	/* Detect NVME device which pluged in the Renoir PCIE root port
+	 * then force use quick suspend/resume path for handling the NVME
+	 * timeout during s2idle resume.
+	 */
+		return NVME_QUIRK_SIMPLE_SUSPEND;
+	}
+
 	adev = ACPI_COMPANION(&root->dev);
 	if (!adev)
 		return false;
-- 
2.7.4


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

[Chaitanya Kulkarni]

> try do the NVME shutdown save and restore in the BIOS and still need PCIe
> power seting to resume from RTD3 for s2idle.

s/seting/setting/ ? idk.

> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> ---
> Changes in v2:
>  Fix the patch format and check chip root complex DID instead of PCIe RP
>   to avoid the storage device plugged in internal PCIe RP by USB adaptor.
> ---
>  drivers/nvme/host/pci.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 6bad4d4..1952da1 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -42,6 +42,9 @@
>  #define NVME_MAX_KB_SZ	4096
>  #define NVME_MAX_SEGS	127
>  
> +#define AMD_PCIE_ID_CZN 0x1630
> +#define AMD_PCIE_ID_RN AMD_PCIE_ID_CZN
> +
>  static int use_threaded_interrupts;
>  module_param(use_threaded_interrupts, int, 0);
>  
> @@ -70,6 +73,12 @@ static unsigned int io_queue_depth = 1024;
>  module_param_cb(io_queue_depth, &io_queue_depth_ops, &io_queue_depth, 0644);
>  MODULE_PARM_DESC(io_queue_depth, "set io queue depth, should >= 2");
>  
> +static const struct pci_device_id pcie_ids[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_RN) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_CZN) },
> +	{ }
> +};
> +

why new global pci_device_id array is needed ?

>  static int io_queue_count_set(const char *val, const struct kernel_param *kp)
>  {
>  	unsigned int n;
> @@ -2832,6 +2841,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 +2855,15 @@ 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 && pci_match_id(pcie_ids, rdev)) {
> +	/* Detect NVME device which pluged in the Renoir PCIE root port
> +	 * then force use quick suspend/resume path for handling the NVME
> +	 * timeout during s2idle resume.
> +	 */

The comment should something like this :-

              if (rdev && pci_match_id(pcie_ids, rdev)) {

		/* 
		 * Detect NVMe device which pluged in the Renoir PCIe root port
		 * then force use quick suspend/resume path for handling the NVMe
		 * timeout during s2idle resume.
		 */return NVME_QUIRK_SIMPLE_SUSPEND;

          }
> +		return NVME_QUIRK_SIMPLE_SUSPEND;
> +	}
> +
>  	adev = ACPI_COMPANION(&root->dev);
>  	if (!adev)
>  		return false;


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

[Liang, Prike]

[AMD Official Use Only - Internal Distribution Only]

> -----Original Message-----
> From: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
>
> > try do the NVME shutdown save and restore in the BIOS and still need
> > PCIe power seting to resume from RTD3 for s2idle.
>
> s/seting/setting/ ? idk.
>
[Prike]  It's should be a typo and will correct it.
> > Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> > ---
> > Changes in v2:
> >  Fix the patch format and check chip root complex DID instead of PCIe RP
> >   to avoid the storage device plugged in internal PCIe RP by USB adaptor.
> > ---
> >  drivers/nvme/host/pci.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index
> > 6bad4d4..1952da1 100644
> > --- a/drivers/nvme/host/pci.c
> > +++ b/drivers/nvme/host/pci.c
> > @@ -42,6 +42,9 @@
> >  #define NVME_MAX_KB_SZ4096
> >  #define NVME_MAX_SEGS127
> >
> > +#define AMD_PCIE_ID_CZN 0x1630
> > +#define AMD_PCIE_ID_RN AMD_PCIE_ID_CZN
> > +
> >  static int use_threaded_interrupts;
> >  module_param(use_threaded_interrupts, int, 0);
> >
> > @@ -70,6 +73,12 @@ static unsigned int io_queue_depth = 1024;
> > module_param_cb(io_queue_depth, &io_queue_depth_ops,
> &io_queue_depth,
> > 0644);  MODULE_PARM_DESC(io_queue_depth, "set io queue depth,
> should
> > >= 2");
> >
> > +static const struct pci_device_id pcie_ids[] = {
> > +{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_RN) },
> > +{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_CZN) },
> > +{ }
> > +};
> > +
>
> why new global pci_device_id array is needed ?
>
[Prike] This pci_device_id  array contains the platform root complex DID which need shutdown the NVME in this armed platform for s2idle.

> >  static int io_queue_count_set(const char *val, const struct
> > kernel_param *kp)  {
> >  unsigned int n;
> > @@ -2832,6 +2841,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 +2855,15 @@ 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 && pci_match_id(pcie_ids, rdev)) {
> > +/* Detect NVME device which pluged in the Renoir PCIE root port
> > + * then force use quick suspend/resume path for handling the NVME
> > + * timeout during s2idle resume.
> > + */
>
> The comment should something like this :-
[Prike]  Thanks point out and miss check this during this quick send.
>
>               if (rdev && pci_match_id(pcie_ids, rdev)) {
>
> /*
>  * Detect NVMe device which pluged in the Renoir PCIe root
> port
>  * then force use quick suspend/resume path for handling
> the NVMe
>  * timeout during s2idle resume.
>  */return NVME_QUIRK_SIMPLE_SUSPEND;
>
>           }
> > +return NVME_QUIRK_SIMPLE_SUSPEND;
> > +}
> > +
> >  adev = ACPI_COMPANION(&root->dev);
> >  if (!adev)
> >  return false;


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

[Keith Busch]

On Thu, Apr 01, 2021 at 01:04:43AM +0000, Liang, Prike wrote:
> [AMD Official Use Only - Internal Distribution Only]

This header is not appropriate for public list communication.

> > > +static const struct pci_device_id pcie_ids[] = {
> > > +{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_RN) },
> > > +{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_CZN) },
> > > +{ }
> > > +};
> > > +
> >
> > why new global pci_device_id array is needed ?
> >
> [Prike] This pci_device_id  array contains the platform root complex DID which need shutdown the NVME in this armed platform for s2idle.

I think Chaitanya is just suggesting you move this declaration to the
scope that uses it. 

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

[Chaitanya Kulkarni]

On 3/31/21 18:51, Keith Busch wrote:
> On Thu, Apr 01, 2021 at 01:04:43AM +0000, Liang, Prike wrote:
>> [AMD Official Use Only - Internal Distribution Only]
> This header is not appropriate for public list communication.

Yes please remove this from next patches.

>>>> +static const struct pci_device_id pcie_ids[] = {
>>>> +{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_RN) },
>>>> +{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_CZN) },
>>>> +{ }
>>>> +};
>>>> +
>>> why new global pci_device_id array is needed ?
>>>
>> [Prike] This pci_device_id  array contains the platform root complex DID which need shutdown the NVME in this armed platform for s2idle.
> I think Chaitanya is just suggesting you move this declaration to the
> scope that uses it. 
>

Yes, something like this on the top of your patch totally untested :-

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index fa81cea41f48..8b2f619eabab 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -42,9 +42,6 @@
 #define NVME_MAX_KB_SZ 4096
 #define NVME_MAX_SEGS  127
 
-#define AMD_PCIE_ID_CZN 0x1630
-#define AMD_PCIE_ID_RN AMD_PCIE_ID_CZN
-
 static int use_threaded_interrupts;
 module_param(use_threaded_interrupts, int, 0);
 
@@ -73,12 +70,6 @@ static unsigned int io_queue_depth = 1024;
 module_param_cb(io_queue_depth, &io_queue_depth_ops, &io_queue_depth,
0644);
 MODULE_PARM_DESC(io_queue_depth, "set io queue depth, should >= 2");
 
-static const struct pci_device_id pcie_ids[] = {
-       { PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_RN) },
-       { PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_CZN) },
-       { }
-};
-
 static int io_queue_count_set(const char *val, const struct
kernel_param *kp)
 {
        unsigned int n;
@@ -2835,6 +2826,10 @@ static unsigned long
check_vendor_combination_bug(struct pci_dev *pdev)
 }
 
 #ifdef CONFIG_ACPI
+
+#define AMD_PCIE_ID_CZN 0x1630
+#define AMD_PCIE_ID_RN AMD_PCIE_ID_CZN
+
 static bool nvme_acpi_storage_d3(struct pci_dev *dev)
 {
        struct acpi_device *adev;
@@ -2843,6 +2838,12 @@ static bool nvme_acpi_storage_d3(struct pci_dev *dev)
        acpi_handle handle;
        acpi_status status;
        u8 val;
+       const struct pci_device_id pcie_ids[] = {
+               { PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_RN) },
+               { PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_CZN) },
+               { }
+       };
+
 
        /*
         * Look for _DSD property specifying that the storage device on
the port


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

[Liang, Prike]

[AMD Public Use]

> > This header is not appropriate for public list communication.
>
> Yes please remove this from next patches.
[Prike]  Yeah, the header should be replaced to public info.
>
> >>>> +static const struct pci_device_id pcie_ids[] = { {
> >>>> +PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_RN) }, {
> >>>> +PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_CZN) }, { } };
> >>>> +
> >>> why new global pci_device_id array is needed ?
> >>>
> >> [Prike] This pci_device_id  array contains the platform root complex DID
> which need shutdown the NVME in this armed platform for s2idle.
> > I think Chaitanya is just suggesting you move this declaration to the
> > scope that uses it.
> >
>
[Prike]  Thanks proposal and that's sound good to move the reference PCI ID table into the caller scope directly.

> Yes, something like this on the top of your patch totally untested :-
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index
> fa81cea41f48..8b2f619eabab 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -42,9 +42,6 @@
>  #define NVME_MAX_KB_SZ 4096
>  #define NVME_MAX_SEGS  127
>
> -#define AMD_PCIE_ID_CZN 0x1630
> -#define AMD_PCIE_ID_RN AMD_PCIE_ID_CZN
> -
>  static int use_threaded_interrupts;
>  module_param(use_threaded_interrupts, int, 0);
>
> @@ -73,12 +70,6 @@ static unsigned int io_queue_depth = 1024;
> module_param_cb(io_queue_depth, &io_queue_depth_ops,
> &io_queue_depth, 0644);  MODULE_PARM_DESC(io_queue_depth, "set io
> queue depth, should >= 2");
>
> -static const struct pci_device_id pcie_ids[] = {
> -       { PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_RN) },
> -       { PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_CZN) },
> -       { }
> -};
> -
>  static int io_queue_count_set(const char *val, const struct kernel_param *kp)
> {
>         unsigned int n;
> @@ -2835,6 +2826,10 @@ static unsigned long
> check_vendor_combination_bug(struct pci_dev *pdev)  }
>
>  #ifdef CONFIG_ACPI
> +
> +#define AMD_PCIE_ID_CZN 0x1630
> +#define AMD_PCIE_ID_RN AMD_PCIE_ID_CZN
> +
>  static bool nvme_acpi_storage_d3(struct pci_dev *dev)  {
>         struct acpi_device *adev;
> @@ -2843,6 +2838,12 @@ static bool nvme_acpi_storage_d3(struct pci_dev
> *dev)
>         acpi_handle handle;
>         acpi_status status;
>         u8 val;
> +       const struct pci_device_id pcie_ids[] = {
> +               { PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_RN) },
> +               { PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_PCIE_ID_CZN) },
> +               { }
> +       };
> +
>
>         /*
>          * Look for _DSD property specifying that the storage device on the port


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

[Christoph Hellwig]

On Wed, Mar 31, 2021 at 08:48:05PM +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 seting to resume from RTD3 for s2idle.

Looks like this is a generic PCIe-level quirk and should go into the
core drivers/pci/ quirk table, with NVMe just looking at the
quirk flag.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

[Liang, Prike]

[AMD Public Use]

>
> On Wed, Mar 31, 2021 at 08:48:05PM +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 seting to resume from RTD3 for s2idle.
>
> Looks like this is a generic PCIe-level quirk and should go into the core
> drivers/pci/ quirk table, with NVMe just looking at the quirk flag.
[Prike]  Thanks proposal, put the PCI identify to PCIe quirk out of NVMe seems more modularization but seems may not simplify the logic. Will update the v3 patch according.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme