[PATCH v3] nvme/pci: Add quick suspend quirk for Sc7280 Platform

[PATCH v3] nvme/pci: Add quick suspend quirk for Sc7280 Platform

[Nitin Rawat]

Enable quick suspend quirks for Sc7280 platform, where power
to nvme device is removed during suspend-resume process. This
is done to avoid the failure dring resume.

This enables simple suspend path for this platform.

Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com>
---

Change from v2-v3:
* changed if/else condition

---

 drivers/nvme/host/pci.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6a99ed6..1dff749 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3034,6 +3034,15 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev)
 			return NVME_QUIRK_SIMPLE_SUSPEND;
 	}

+	if (of_machine_is_compatible("qcom,sc7280")) {
+		/*
+		 * Append quick suspend quirks for sc7280 platforms
+		 * so that simple suspend path is executed for this
+		 * platform to avoid any resume failure.
+		 */
+		return NVME_QUIRK_SIMPLE_SUSPEND;
+	}
+
 	return 0;
 }

--
2.7.4

Re: [PATCH v3] nvme/pci: Add quick suspend quirk for Sc7280 Platform

[Keith Busch]

On Fri, Feb 11, 2022 at 02:23:28AM +0530, Nitin Rawat wrote:
> Enable quick suspend quirks for Sc7280 platform, where power
> to nvme device is removed during suspend-resume process. This
> is done to avoid the failure dring resume.
> 
> This enables simple suspend path for this platform.

Looks good to me.

Reviewed-by: Keith Busch <kbusch@kernel.org>

RE: [PATCH v3] nvme/pci: Add quick suspend quirk for Sc7280 Platform

[Nitin Rawat (QUIC)]

Hi Keith,
My Bad..Uploaded wrong patch from my local. I have uploaded the tested patch now.

Regards,
Nitin



-----Original Message-----
From: Nitin Rawat <quic_nitirawa@quicinc.com> 
Sent: Friday, February 11, 2022 2:23 AM
To: Keith Busch <kbusch@kernel.org>; Jens Axboe <axboe@fb.com>; Sagi Grimberg <sagi@grimberg.me>; linux-nvme@lists.infradead.org; linux-kernel@vger.kernel.org
Cc: Nitin Rawat (QUIC) <quic_nitirawa@quicinc.com>; Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com>
Subject: [PATCH v3] nvme/pci: Add quick suspend quirk for Sc7280 Platform

Enable quick suspend quirks for Sc7280 platform, where power to nvme device is removed during suspend-resume process. This is done to avoid the failure dring resume.

This enables simple suspend path for this platform.

Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com>
---

Change from v2-v3:
* changed if/else condition

---

 drivers/nvme/host/pci.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 6a99ed6..1dff749 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3034,6 +3034,15 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev)
 			return NVME_QUIRK_SIMPLE_SUSPEND;
 	}

+	if (of_machine_is_compatible("qcom,sc7280")) {
+		/*
+		 * Append quick suspend quirks for sc7280 platforms
+		 * so that simple suspend path is executed for this
+		 * platform to avoid any resume failure.
+		 */
+		return NVME_QUIRK_SIMPLE_SUSPEND;
+	}
+
 	return 0;
 }

--
2.7.4

Re: [PATCH v3] nvme/pci: Add quick suspend quirk for Sc7280 Platform

[Christoph Hellwig]

On Fri, Feb 11, 2022 at 02:23:28AM +0530, Nitin Rawat wrote:
> Enable quick suspend quirks for Sc7280 platform, where power
> to nvme device is removed during suspend-resume process. This
> is done to avoid the failure dring resume.

You need to sort this out in the PCI and PM layers.  The broken platform
will also affect all other PCIe drivers and not just nvme.

RE: [PATCH v3] nvme/pci: Add quick suspend quirk for Sc7280 Platform

[Nitin Rawat (QUIC)]

Hi Christoph/Keith/Rafael

Since we are giving control to PCIe (NVMe power rails control), it can vary from platform to platform.
More over, PCIe driver doesn't control these power rails directly from PCIe driver, they tie nvme supply with one of the pcie supply and control them together. 
So i think it would be better to either have quirk based on platform or always setting simple suspend and platform which needs full suspend can update it through some means. 

Based on below link, Looks like this can be across platform ...vidya also mention similar concern for tegra platform.  
https://lore.kernel.org/lkml/20220209202639.GB1616420@dhcp-10-100-145-180.wdc.com/T/#md10b0108b3ccbb1254fe0ad8dbb6e98044a8820b

Please let me know your opinions.

Thanks,
Nitin
-----Original Message-----
From: Christoph Hellwig <hch@infradead.org> 
Sent: Friday, February 11, 2022 11:53 AM
To: Nitin Rawat (QUIC) <quic_nitirawa@quicinc.com>
Cc: Keith Busch <kbusch@kernel.org>; Jens Axboe <axboe@fb.com>; Sagi Grimberg <sagi@grimberg.me>; linux-nvme@lists.infradead.org; linux-kernel@vger.kernel.org; Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com>
Subject: Re: [PATCH v3] nvme/pci: Add quick suspend quirk for Sc7280 Platform

On Fri, Feb 11, 2022 at 02:23:28AM +0530, Nitin Rawat wrote:
> Enable quick suspend quirks for Sc7280 platform, where power to nvme 
> device is removed during suspend-resume process. This is done to avoid 
> the failure dring resume.

You need to sort this out in the PCI and PM layers.  The broken platform will also affect all other PCIe drivers and not just nvme.

RE: [PATCH v3] nvme/pci: Add quick suspend quirk for Sc7280 Platform

[Nitin Rawat (QUIC)]

Hi Christoph/Keith/Rafael

Since we are giving control to PCIe (NVMe power rails control), it can vary from platform to platform.
More over, PCIe driver doesn't control these power rails directly from PCIe driver, they tie nvme supply with one of the pcie supply and control them together. 
So i think it would be better to either have quirk based on platform or always setting simple suspend and platform which needs full suspend can update it through some means. 

Based on below link, Looks like this can be across platform ...vidya also mention similar concern for tegra platform.  
https://lore.kernel.org/lkml/20220209202639.GB1616420@dhcp-10-100-145-180.wdc.com/T/#md10b0108b3ccbb1254fe0ad8dbb6e98044a8820b

Please let me know your opinions.

Thanks,
Nitin
-----Original Message-----
From: Christoph Hellwig <hch@infradead.org>
Sent: Friday, February 11, 2022 11:53 AM
To: Nitin Rawat (QUIC) <quic_nitirawa@quicinc.com>
Cc: Keith Busch <kbusch@kernel.org>; Jens Axboe <axboe@fb.com>; Sagi Grimberg <sagi@grimberg.me>; linux-nvme@lists.infradead.org; linux-kernel@vger.kernel.org; Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com>
Subject: Re: [PATCH v3] nvme/pci: Add quick suspend quirk for Sc7280 Platform

On Fri, Feb 11, 2022 at 02:23:28AM +0530, Nitin Rawat wrote:
> Enable quick suspend quirks for Sc7280 platform, where power to nvme 
> device is removed during suspend-resume process. This is done to avoid 
> the failure dring resume.

You need to sort this out in the PCI and PM layers.  The broken platform will also affect all other PCIe drivers and not just nvme.

Re: [PATCH v3] nvme/pci: Add quick suspend quirk for Sc7280 Platform

[Vidya Sagar]

On 2/16/2022 9:39 PM, Nitin Rawat (QUIC) wrote:
> External email: Use caution opening links or attachments
> 
> 
> Hi Christoph/Keith/Rafael
> 
> Since we are giving control to PCIe (NVMe power rails control), it can vary from platform to platform.
> More over, PCIe driver doesn't control these power rails directly from PCIe driver, they tie nvme supply with one of the pcie supply and control them together.
> So i think it would be better to either have quirk based on platform or always setting simple suspend and platform which needs full suspend can update it through some means.
Agree with this comment.
I pushed a similar patch for Tegra platforms @ 
https://lore.kernel.org/linux-nvme/20220217023635.8963-1-vidyas@nvidia.com/T/#u

- Vidya Sagar
> 
> Based on below link, Looks like this can be across platform ...vidya also mention similar concern for tegra platform.
> https://lore.kernel.org/lkml/20220209202639.GB1616420@dhcp-10-100-145-180.wdc.com/T/#md10b0108b3ccbb1254fe0ad8dbb6e98044a8820b
> 
> Please let me know your opinions.
> 
> Thanks,
> Nitin
> -----Original Message-----
> From: Christoph Hellwig <hch@infradead.org>
> Sent: Friday, February 11, 2022 11:53 AM
> To: Nitin Rawat (QUIC) <quic_nitirawa@quicinc.com>
> Cc: Keith Busch <kbusch@kernel.org>; Jens Axboe <axboe@fb.com>; Sagi Grimberg <sagi@grimberg.me>; linux-nvme@lists.infradead.org; linux-kernel@vger.kernel.org; Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com>
> Subject: Re: [PATCH v3] nvme/pci: Add quick suspend quirk for Sc7280 Platform
> 
> On Fri, Feb 11, 2022 at 02:23:28AM +0530, Nitin Rawat wrote:
>> Enable quick suspend quirks for Sc7280 platform, where power to nvme
>> device is removed during suspend-resume process. This is done to avoid
>> the failure dring resume.
> 
> You need to sort this out in the PCI and PM layers.  The broken platform will also affect all other PCIe drivers and not just nvme.
> 

Re: [PATCH v3] nvme/pci: Add quick suspend quirk for Sc7280 Platform

[Christoph Hellwig]

On Wed, Feb 16, 2022 at 04:07:05PM +0000, Nitin Rawat (QUIC) wrote:
> Hi Christoph/Keith/Rafael
> 
> Since we are giving control to PCIe (NVMe power rails control), it can vary from platform to platform.
> More over, PCIe driver doesn't control these power rails directly from PCIe driver, they tie nvme supply with one of the pcie supply and control them together. 
> So i think it would be better to either have quirk based on platform or always setting simple suspend and platform which needs full suspend can update it through some means. 
> 
> Based on below link, Looks like this can be across platform ...vidya also mention similar concern for tegra platform.  

Here is the thing:  nothing here is really NVMe specific.  We do have
a bunch of drivers including nvme that would love to not do a full
firmware shutdown on a suspend.  For storage devices this is especially
important as each shutdown reduces media life time.  Until very recently
this has been perfectly fine, but now various platforms show up that
want to completely disable power to PCIe slots on a system suspend.

The drivers need information from the PCI / PM core when a ->suspend
call requires the device to got into D3 and when not, and we can't just
do that with quirks in the various drivers.

Re: [PATCH v3] nvme/pci: Add quick suspend quirk for Sc7280 Platform

[Christoph Hellwig]

On Fri, Apr 15, 2022 at 02:29:27PM +0530, Nitin Rawat wrote:
>  1. Adding a flag notifying nvme driver about the d3 support or not - Adding
> flag to PCI bus will make it applicable to all the child devices of the
> RC/bridge and that would cause problem as this behaviour is only applicable
> for nvme device for now. Any suggestion
> 
> to avoid this?

This seems like the only actual sensible option.  Why would you want
to avoid it?