drivers: pci: quirks: Add suspend fixup for SSD on sc7280

[PATCH v1] drivers: pci: quirks: Add suspend fixup for SSD on sc7280

Posted by Owen Yang 2 years, 8 months ago

Implement this workaround until Qualcomm fixed the
 correct NVMe suspend process.

Signed-off-by: Owen Yang <ecs.taipeikernel@gmail.com>
---

 drivers/pci/quirks.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index f4e2a88729fd..b57876dc2624 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -5945,6 +5945,16 @@ static void nvidia_ion_ahci_fixup(struct pci_dev *pdev)
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0ab8, nvidia_ion_ahci_fixup);
 
+/* In Qualcomm 7c gen 3 sc7280 platform. Some of the SSD won't enter
+ * the correct ASPM state properly. Therefore. Implement this workaround
+ * until Qualcomm fixed the correct NVMe suspend process*/
+static void phison_suspend_fixup(struct pci_dev *pdev)
+{
+	msleep(30);
+}
+DECLARE_PCI_FIXUP_SUSPEND(0x1987, 0x5013, phison_suspend_fixup);
+DECLARE_PCI_FIXUP_SUSPEND(0x1987, 0x5015, phison_suspend_fixup);
+
 static void rom_bar_overlap_defect(struct pci_dev *dev)
 {
 	pci_info(dev, "working around ROM BAR overlap defect\n");
-- 
2.17.1

Re: [PATCH v1] drivers: pci: quirks: Add suspend fixup for SSD on sc7280

Posted by Manivannan Sadhasivam 2 years, 8 months ago

On Thu, May 25, 2023 at 04:35:12PM +0800, Owen Yang wrote:
> Implement this workaround until Qualcomm fixed the
>  correct NVMe suspend process.
> 
> Signed-off-by: Owen Yang <ecs.taipeikernel@gmail.com>
> ---
> 
>  drivers/pci/quirks.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index f4e2a88729fd..b57876dc2624 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5945,6 +5945,16 @@ static void nvidia_ion_ahci_fixup(struct pci_dev *pdev)
>  }
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0ab8, nvidia_ion_ahci_fixup);
>  
> +/* In Qualcomm 7c gen 3 sc7280 platform. Some of the SSD won't enter
> + * the correct ASPM state properly. Therefore. Implement this workaround
> + * until Qualcomm fixed the correct NVMe suspend process*/

What is there to fix during suspend? Currently, Qcom PCIe driver just votes for
low interconnect bandwidth and keeps the resources (clocks, regulators) ON
during suspend. So there is no way the device would move to D3Cold.

Earlier Qcom reported that during suspend, link down event happens when the
resources are turned OFF without waiting for the link to enter L1ss. But as I
said above, we are _not_ turning OFF any resources.

I believe this patch is addressing an issue that is caused by an out-of-tree
patch.

- Mani

> +static void phison_suspend_fixup(struct pci_dev *pdev)
> +{
> +	msleep(30);
> +}
> +DECLARE_PCI_FIXUP_SUSPEND(0x1987, 0x5013, phison_suspend_fixup);
> +DECLARE_PCI_FIXUP_SUSPEND(0x1987, 0x5015, phison_suspend_fixup);
> +
>  static void rom_bar_overlap_defect(struct pci_dev *dev)
>  {
>  	pci_info(dev, "working around ROM BAR overlap defect\n");
> -- 
> 2.17.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v1] drivers: pci: quirks: Add suspend fixup for SSD on sc7280

Posted by Matthias Kaehlcke 2 years, 8 months ago

On Mon, May 29, 2023 at 10:18:56PM +0530, Manivannan Sadhasivam wrote:
> On Thu, May 25, 2023 at 04:35:12PM +0800, Owen Yang wrote:
> > Implement this workaround until Qualcomm fixed the
> >  correct NVMe suspend process.
> > 
> > Signed-off-by: Owen Yang <ecs.taipeikernel@gmail.com>
> > ---
> > 
> >  drivers/pci/quirks.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index f4e2a88729fd..b57876dc2624 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -5945,6 +5945,16 @@ static void nvidia_ion_ahci_fixup(struct pci_dev *pdev)
> >  }
> >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0ab8, nvidia_ion_ahci_fixup);
> >  
> > +/* In Qualcomm 7c gen 3 sc7280 platform. Some of the SSD won't enter
> > + * the correct ASPM state properly. Therefore. Implement this workaround
> > + * until Qualcomm fixed the correct NVMe suspend process*/
> 
> What is there to fix during suspend? Currently, Qcom PCIe driver just votes for
> low interconnect bandwidth and keeps the resources (clocks, regulators) ON
> during suspend. So there is no way the device would move to D3Cold.
> 
> Earlier Qcom reported that during suspend, link down event happens when the
> resources are turned OFF without waiting for the link to enter L1ss. But as I
> said above, we are _not_ turning OFF any resources.

Right, it makes little sense that the NVMe would move to D3Cold. And why does
the issue only reproduces sometimes (with certain NVMes) and not consistently?

> I believe this patch is addressing an issue that is caused by an out-of-tree
> patch.

I think ECS observed this with Chrome OS v5.15 kernel. On the PCI side this
kernel only has backported changes from upstream  (mostly clean picks), no
downstream patches, so it seems unlikely that the issue is caused by a
downstream patch.

Re: [PATCH v1] drivers: pci: quirks: Add suspend fixup for SSD on sc7280

Posted by Manivannan Sadhasivam 2 years, 8 months ago

On Tue, May 30, 2023 at 09:17:02PM +0000, Matthias Kaehlcke wrote:
> On Mon, May 29, 2023 at 10:18:56PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, May 25, 2023 at 04:35:12PM +0800, Owen Yang wrote:
> > > Implement this workaround until Qualcomm fixed the
> > >  correct NVMe suspend process.
> > > 
> > > Signed-off-by: Owen Yang <ecs.taipeikernel@gmail.com>
> > > ---
> > > 
> > >  drivers/pci/quirks.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > index f4e2a88729fd..b57876dc2624 100644
> > > --- a/drivers/pci/quirks.c
> > > +++ b/drivers/pci/quirks.c
> > > @@ -5945,6 +5945,16 @@ static void nvidia_ion_ahci_fixup(struct pci_dev *pdev)
> > >  }
> > >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0ab8, nvidia_ion_ahci_fixup);
> > >  
> > > +/* In Qualcomm 7c gen 3 sc7280 platform. Some of the SSD won't enter
> > > + * the correct ASPM state properly. Therefore. Implement this workaround
> > > + * until Qualcomm fixed the correct NVMe suspend process*/
> > 
> > What is there to fix during suspend? Currently, Qcom PCIe driver just votes for
> > low interconnect bandwidth and keeps the resources (clocks, regulators) ON
> > during suspend. So there is no way the device would move to D3Cold.
> > 
> > Earlier Qcom reported that during suspend, link down event happens when the
> > resources are turned OFF without waiting for the link to enter L1ss. But as I
> > said above, we are _not_ turning OFF any resources.
> 
> Right, it makes little sense that the NVMe would move to D3Cold. And why does
> the issue only reproduces sometimes (with certain NVMes) and not consistently?
> 

Honestly, I don't have any idea why it is happening. The link should transition
to L1ss during suspend and we keep all resources ON.

Did ECS only observe this issue when ASPM is enabled (powersupersave)? If so,
then it is a NVMe firmware issue not Qualcomm.

> > I believe this patch is addressing an issue that is caused by an out-of-tree
> > patch.
> 
> I think ECS observed this with Chrome OS v5.15 kernel. On the PCI side this
> kernel only has backported changes from upstream  (mostly clean picks), no
> downstream patches, so it seems unlikely that the issue is caused by a
> downstream patch.

Okay, thanks for the clarification. Is it possible to reproduce it on mainline?
Just to rule out the upstream vs downstream difference elsewhere.

That should also be the case to submit a patch against mainline.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v1] drivers: pci: quirks: Add suspend fixup for SSD on sc7280

Posted by Bjorn Helgaas 2 years, 8 months ago

On Thu, May 25, 2023 at 04:35:12PM +0800, Owen Yang wrote:
> Implement this workaround until Qualcomm fixed the
>  correct NVMe suspend process.

Thanks for the patch.  Before I can do anything, this needs:

  - Subject line in style of the file (use "git log --oneline
    drivers/pci/quirks.c").

  - Format commit log correctly (fill 75 columns, no leading spaces).

  - Description of incorrect behavior.  What does the user see?  If
    there's a bug report, include a link to it.

  - Multi-line code comments in style of the file (look at existing
    comments in the file).

  - Details of "the correct ASPM state".  ASPM may be enabled or
    disabled by the user, so you can't assume any particular ASPM
    configuration.

  - Details on the Qualcomm sc7280 connection.  This quirk would
    affect Phison SSDs on *all* platforms, not just sc7280.  I don't
    want to slow down suspend on all platforms just for a sc7280
    issue.

  - Drop the "until Qualcomm fixes NVMe suspend" text.  Even if
    Qualcomm fixes something, we can't just drop this quirk because
    there will be platforms in the field that don't have the Qualcomm
    fix.

Bjorn

> Signed-off-by: Owen Yang <ecs.taipeikernel@gmail.com>
> ---
> 
>  drivers/pci/quirks.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index f4e2a88729fd..b57876dc2624 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5945,6 +5945,16 @@ static void nvidia_ion_ahci_fixup(struct pci_dev *pdev)
>  }
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0ab8, nvidia_ion_ahci_fixup);
>  
> +/* In Qualcomm 7c gen 3 sc7280 platform. Some of the SSD won't enter
> + * the correct ASPM state properly. Therefore. Implement this workaround
> + * until Qualcomm fixed the correct NVMe suspend process*/
> +static void phison_suspend_fixup(struct pci_dev *pdev)
> +{
> +	msleep(30);
> +}
> +DECLARE_PCI_FIXUP_SUSPEND(0x1987, 0x5013, phison_suspend_fixup);
> +DECLARE_PCI_FIXUP_SUSPEND(0x1987, 0x5015, phison_suspend_fixup);
> +
>  static void rom_bar_overlap_defect(struct pci_dev *dev)
>  {
>  	pci_info(dev, "working around ROM BAR overlap defect\n");
> -- 
> 2.17.1
>