Re: [PATCH] nvme-pci: Use NVME_QUIRK_SIMPLE_SUSPEND for Qualcomm Snapdragon 8cx Gen 3 platforms

[Ulf Hansson]

On Sun, 26 Jan 2025 at 06:03, Manivannan Sadhasivam
<manivannan.sadhasivam@xxxxxxxxxx> wrote:
>
> On these platforms, power to the PCIe bus is not retained if the SoC enters
> its own deep low power state called, CX power collapse state during system
> suspend. Once the SoC resumes after going through CX power collapse, all
> the PCIe bus state will be lost. So the NVMe devices on these platforms
> won't resume properly, rendering the machines useless until forcefully
> restarted by the users.
>
> To workaround this issue, the PCIe controller driver is keeping a minimal
> vote on the PCIe-MEM interconnect path in its system suspend callback
> current currently. While this gives a working system suspend, it also
> results in increased power consumption during suspend as the SoC never
> enters its low power state. So with this change, the workaround can finally
> be removed.
>
> Also it should be noted that the actual fix to this issue lies in the
> PCI/PM core. But currently there is no such fix exist as of now and the
> consensus to reach it is also taking a lot of time.
>
> So use this quirk to allow users to save battery life on these platforms
> until the PCI/PM core comes up with an API that tells the PCI client
> drivers when to turn off the devices.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>

FWIW, feel free to add:

Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>

> ---
>
> Hi,
>
> I'm sending this patch as there seems to be no consensus so far (as been
> discussed in this thread [1]) in coming up with a PCI/PM API that tells the
> client drivers when to safely turn off the devices. Since coming up with these
> kind of generic APIs are bound to take time (I do intend to pursue), it makes
> sense to allow these platforms to make use of the existing

In whatever form you intend to continue to try to move this forward,
feel free to keep me in the loop. I am interested and will try to
provide as much feedback as I possibly can.

I can also keep an eye for a conference where we can bring this up
again, last time we discussed this was in LPC in Richmond 2023, I
think. It looks like we have some more data for discussion now, but
let's see how far we can get on LKML.

> NVME_QUIRK_SIMPLE_SUSPEND quirk in the meantime. Once such API is upstreamed and
> used in this driver, this quirk can be reverted.
>
> It should be noted that this CX power collapse issue only exist on the platforms
> based on the Qcom Snapdragon 8cx Gen 3 SoC. On other platforms, it is a more of
> a policy decision to decide whether to shutdown the devices or not. And these
> other platforms can wait for the generic API to be made available by the PCI/PM
> core.
>
> [1] https://lore.kernel.org/linux-pci/20241118082344.8146-1-manivannan.sadhasivam@xxxxxxxxxx
>
>  drivers/nvme/host/pci.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index e2634f437f33..c52dced952a1 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -3162,6 +3162,16 @@ 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 Snapdragon 8cx Gen 3 (SC8280XP) platforms doesn't retain
> +        * power to the PCIe bus after entering low power CX power collapse
> +        * state during system suspend. So shutdown the NVMe devices to have a
> +        * working system suspend on these platforms.
> +        */
> +       if (dmi_match(DMI_PRODUCT_FAMILY, "SCP_MAKENA") ||
> +           dmi_match(DMI_PRODUCT_FAMILY, "ThinkPad X13s Gen 1"))
> +               return NVME_QUIRK_SIMPLE_SUSPEND;
> +
>         return 0;
>  }
>
> --
> 2.25.1
>

Kind regards
Uffe