From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

The Qualcomm SC8280XP SoC requires that all PCIe hosts are powered down
before the platform can reach S3-like sleep states. This is very much
similar in nature to the issue described in [1].

Other Qualcomm platforms we support upstream require more complex code
additions across both the PCIe RC driver and other platform-specific
ones, before the link can be sustained in suspend. Hence, they
effectively need the same treatment for now.

Force NVME_QUIRK_SIMPLE_SUSPEND on all Qualcomm platforms (as
identified by the upstream bridge having a Qualcomm VID) to address
that. Once the aforementioned issues on non-SC8280XP platforms are
addressed, the condition will be made more specific, with a PID check
limiting it to only the platform(s) that require it due to HW design.

[1] Commit df4f9bc4fb9c ("nvme-pci: add support for ACPI StorageD3Enable property")

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 drivers/nvme/host/pci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 4b9fda0b1d9a33af4d7030b72532835b205e9cbb..007b28a03cf2c7b3a90f7b7faa1c4c1950e25b6e 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2999,6 +2999,14 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev)
 	if (dmi_match(DMI_BOARD_NAME, "LXKT-ZXEG-N6"))
 		return NVME_QUIRK_NO_APST;
 
+	/*
+	 * Qualcomm SC8280XP must shut down the PCIe host to enter S3.
+	 * Other Qualcomm platforms require more driver changes to restore the
+	 * link state after wakeup.
+	 */
+	if (pci_upstream_bridge(pdev)->vendor == PCI_VENDOR_ID_QCOM)
+		return NVME_QUIRK_SIMPLE_SUSPEND;
+
 	return 0;
 }
 

---
base-commit: fd21fa4a912ebbf8a6a341c31d8456f61e7d4170
change-id: 20241024-topic-nvmequirk-10e70e6b13ba

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

On Thu, Oct 24, 2024 at 07:33:07PM +0200, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> The Qualcomm SC8280XP SoC requires that all PCIe hosts are powered down
> before the platform can reach S3-like sleep states. This is very much
> similar in nature to the issue described in [1].

The "SIMPLE" quirk is only supposed to affect kernel managed runtime
suspend states, s2idle or s0ix. Shouldn't s3 already be using the simple
suspend?

On 25.10.2024 6:12 PM, Keith Busch wrote:
> On Thu, Oct 24, 2024 at 07:33:07PM +0200, Konrad Dybcio wrote:
>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>
>> The Qualcomm SC8280XP SoC requires that all PCIe hosts are powered down
>> before the platform can reach S3-like sleep states. This is very much
>> similar in nature to the issue described in [1].
> 
> The "SIMPLE" quirk is only supposed to affect kernel managed runtime
> suspend states, s2idle or s0ix. Shouldn't s3 already be using the simple
> suspend?

So on these platforms, all system sleep states (incl. S3) are entered
through what Linux sees as s2idle, with a separate MCU doing a lot
behind the scenes. s2idle of course also covers the runtime cpuidle
cases.

All but the deepest state (which Linux doesn't differentiate as of
today) are effectively somewhat like s0ix.
It's a bit hard to draw accurate lines between Intel terminology and
what we have here, as there's way more things onboard than just the CPU
cluster that may be operating independently..

Konrad

On Fri, Oct 25, 2024 at 06:40:23PM +0200, Konrad Dybcio wrote:
> On 25.10.2024 6:12 PM, Keith Busch wrote:
> > On Thu, Oct 24, 2024 at 07:33:07PM +0200, Konrad Dybcio wrote:
> >> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>
> >> The Qualcomm SC8280XP SoC requires that all PCIe hosts are powered down
> >> before the platform can reach S3-like sleep states. This is very much
> >> similar in nature to the issue described in [1].
> > 
> > The "SIMPLE" quirk is only supposed to affect kernel managed runtime
> > suspend states, s2idle or s0ix. Shouldn't s3 already be using the simple
> > suspend?
> 
> So on these platforms, all system sleep states (incl. S3) are entered
> through what Linux sees as s2idle, with a separate MCU doing a lot
> behind the scenes. s2idle of course also covers the runtime cpuidle
> cases.
> 
> All but the deepest state (which Linux doesn't differentiate as of
> today) are effectively somewhat like s0ix.
> It's a bit hard to draw accurate lines between Intel terminology and
> what we have here, as there's way more things onboard than just the CPU
> cluster that may be operating independently..

Gotcha.

Is there any sleep state on this where using the nvme managed power is
advantageous, or is the simple suspend preferred in every scenario for
this platform?

On 25.10.2024 6:57 PM, Keith Busch wrote:
> On Fri, Oct 25, 2024 at 06:40:23PM +0200, Konrad Dybcio wrote:
>> On 25.10.2024 6:12 PM, Keith Busch wrote:
>>> On Thu, Oct 24, 2024 at 07:33:07PM +0200, Konrad Dybcio wrote:
>>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>
>>>> The Qualcomm SC8280XP SoC requires that all PCIe hosts are powered down
>>>> before the platform can reach S3-like sleep states. This is very much
>>>> similar in nature to the issue described in [1].
>>>
>>> The "SIMPLE" quirk is only supposed to affect kernel managed runtime
>>> suspend states, s2idle or s0ix. Shouldn't s3 already be using the simple
>>> suspend?
>>
>> So on these platforms, all system sleep states (incl. S3) are entered
>> through what Linux sees as s2idle, with a separate MCU doing a lot
>> behind the scenes. s2idle of course also covers the runtime cpuidle
>> cases.
>>
>> All but the deepest state (which Linux doesn't differentiate as of
>> today) are effectively somewhat like s0ix.
>> It's a bit hard to draw accurate lines between Intel terminology and
>> what we have here, as there's way more things onboard than just the CPU
>> cluster that may be operating independently..
> 
> Gotcha.
> 
> Is there any sleep state on this where using the nvme managed power is
> advantageous, or is the simple suspend preferred in every scenario for
> this platform?

On the special snowflake SC8280XP, simple suspend is the only choice
that makes sense here. Otherwise, you'd need to keep the entire SoC
powered, which (even if most of it would be driver-suspended) ruins
battery life.

On other platforms (where we'll be able to sustain the PCIe link,
eventually), I can see both options being useful for different use
cases. But again, that's only for when we can actually get that working
on mainline.

Konrad

On Thu, Oct 24, 2024 at 07:33:07PM +0200, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> The Qualcomm SC8280XP SoC requires that all PCIe hosts are powered down
> before the platform can reach S3-like sleep states. This is very much
> similar in nature to the issue described in [1].
> 
> Other Qualcomm platforms we support upstream require more complex code
> additions across both the PCIe RC driver and other platform-specific
> ones, before the link can be sustained in suspend. Hence, they
> effectively need the same treatment for now.
> 
> Force NVME_QUIRK_SIMPLE_SUSPEND on all Qualcomm platforms (as
> identified by the upstream bridge having a Qualcomm VID) to address
> that. Once the aforementioned issues on non-SC8280XP platforms are
> addressed, the condition will be made more specific, with a PID check
> limiting it to only the platform(s) that require it due to HW design.

The NVMe driver is the wrong place for this, it needs to happen in the
core by making acpi_storage_d3() evaluate to true.  Preferably by
actually setting the right ACPI attributes because a check for 
PCI vendor ID absolutely will never do the right thing in the long run.

On 25.10.2024 1:35 PM, Christoph Hellwig wrote:
> On Thu, Oct 24, 2024 at 07:33:07PM +0200, Konrad Dybcio wrote:
>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>
>> The Qualcomm SC8280XP SoC requires that all PCIe hosts are powered down
>> before the platform can reach S3-like sleep states. This is very much
>> similar in nature to the issue described in [1].
>>
>> Other Qualcomm platforms we support upstream require more complex code
>> additions across both the PCIe RC driver and other platform-specific
>> ones, before the link can be sustained in suspend. Hence, they
>> effectively need the same treatment for now.
>>
>> Force NVME_QUIRK_SIMPLE_SUSPEND on all Qualcomm platforms (as
>> identified by the upstream bridge having a Qualcomm VID) to address
>> that. Once the aforementioned issues on non-SC8280XP platforms are
>> addressed, the condition will be made more specific, with a PID check
>> limiting it to only the platform(s) that require it due to HW design.
> 
> The NVMe driver is the wrong place for this, it needs to happen in the
> core by making acpi_storage_d3() evaluate to true.  Preferably by
> actually setting the right ACPI attributes because a check for 
> PCI vendor ID absolutely will never do the right thing in the long run.

(Un?)fortunately, said platforms use FDT, so we can't fix that in ACPI.

We also considered a DT property to indicate this, but:

a) PCIe devices are discoverable and it's really really really
   discouraged to hardcode devices that are likely to be present
   on the bus (and this wouldn't work if a NVMe device showed up
   on a different-than-usual RC)

b) Adding such a property to the PCIe host node sounds a bit
   saner, but the NVMe code isn't aware of the RC. We could add
   something like:


struct pci_bus *pbus = pdev->bus;

while (!(pci_is_root_bus(pbus)))
	pbus = pbus->parent;

if (of_property_present(pbus->dev.of_node, "broken-sleep-foo-bar"))
	return NVME_QUIRK_SIMPLE_SUSPEND;


..but that implies we have to set that quirk in DT on all platforms
which only require an equivalent workaround temporarily. That in turn
is later much harder to undo than a simple driver change (e.g. if your
FDT is provided by the firmware).

In general, I don't think we can at all rely on firmware updates for
devices that are already on the market.

Konrad

On Fri, Oct 25, 2024 at 05:30:05PM +0200, Konrad Dybcio wrote:
> (Un?)fortunately, said platforms use FDT, so we can't fix that in ACPI.

Then generalize the acpi helper to a generic one checking ACPI and
DT and specify a proper DT binding.

If th requirement to put all devices into D3 is a plaform propery
a specific driver is always the wrong place for it.

> b) Adding such a property to the PCIe host node sounds a bit
>    saner, but the NVMe code isn't aware of the RC. We could add
>    something like:

Again, this all belongs into core code.  The nvme driver is just
a consumer of this information, just like for example the AHCI
driver.

On 28.10.2024 10:19 AM, Christoph Hellwig wrote:
> On Fri, Oct 25, 2024 at 05:30:05PM +0200, Konrad Dybcio wrote:
>> (Un?)fortunately, said platforms use FDT, so we can't fix that in ACPI.
> 
> Then generalize the acpi helper to a generic one checking ACPI and
> DT and specify a proper DT binding.
> 
> If th requirement to put all devices into D3 is a plaform propery
> a specific driver is always the wrong place for it.
> 
>> b) Adding such a property to the PCIe host node sounds a bit
>>    saner, but the NVMe code isn't aware of the RC. We could add
>>    something like:
> 
> Again, this all belongs into core code.  The nvme driver is just
> a consumer of this information, just like for example the AHCI
> driver.

Alright, I'll try to pursue that angle instead,

Konrad