Received: by 2002:a05:6358:16cc:b0:ea:6187:17c9 with SMTP id r12csp2798587rwl; Fri, 6 Jan 2023 10:58:14 -0800 (PST) X-Google-Smtp-Source: AMrXdXsv3e/KvsGKK/8zmEV16NLYitdPC8GHy1ud1llot5FphXSrgNBgULsci3TAMG31eaHsT+Qu X-Received: by 2002:a50:cc4c:0:b0:479:8313:2fdd with SMTP id n12-20020a50cc4c000000b0047983132fddmr51694536edi.10.1673031494079; Fri, 06 Jan 2023 10:58:14 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1673031494; cv=none; d=google.com; s=arc-20160816; b=VzTRClSuJUyJNJOJQkYSN0c8+uUcJWlq1FJ0/W/rkiO0JfFHv2bKbeKPVCJD0mpoIx By/OsKGrfuhKMVIHVvzpshDZfO8GgfDjKj3Ehs7IpaeVIK6Kv2zoSvvbuupljABo28dl I8YfYvwpR6w733xxIIar0bGCkdfaO3JjmoEA6wu6oKJCRcENO3GtzefBgNiVytp2rzZ5 CODb3GokgZ9f1WpN/vIQpi5lamNDkkj5TvcswGnSsiqKnCxWEqRsMWVNkhIgvmDGO88O rVUDQnWxIlN20xSuXv+bTlwg1x6Lk0eDlgFaJ7bEsZTAuWUD05IZ/r13rxnI3qUDaH6v a/oQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=1nWrzelfArIcGuHXeTwgFSuDsGz4bBzAEBvOTmzqK8s=; b=mQLNu3ne8LMnVFcJaQE7zBW9uj618+xFO8xKYGvbeOj9L1X2YvOubuS0Imsy1nj40q Z95Q6gkowaF8fE81BWdst2T4W599FgxKGmkVZ1NAfHV2ZRo3G8Li2MBc8thvMNiCwNKz GrR3dtGcqlays6o75+YNwXKT4+6bAyKL/9PL3FCTfPbSZvstGd9Ij7Zazno5/222mkfE R9Fd0Xb1T1jWXB/TDWSkc9bDES11YPp1KMnK0nTQWCjh/5p5TITtiF9wr5IYkNILpPfX 0Na121NPunWN/5/OHctE+Tgdfec6xm515Z9Gz5Vb8Xv6tgLe0dizqHOJ74E5lGBnOayG r8pg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=GviCqBDM; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id f24-20020a50d558000000b0047039339346si2296552edj.550.2023.01.06.10.58.01; Fri, 06 Jan 2023 10:58:14 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=GviCqBDM; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229547AbjAFSRY (ORCPT + 54 others); Fri, 6 Jan 2023 13:17:24 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54216 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234793AbjAFSRV (ORCPT ); Fri, 6 Jan 2023 13:17:21 -0500 Received: from mail-il1-x131.google.com (mail-il1-x131.google.com [IPv6:2607:f8b0:4864:20::131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 74D022DF3 for ; Fri, 6 Jan 2023 10:17:20 -0800 (PST) Received: by mail-il1-x131.google.com with SMTP id c20so1365749ilj.10 for ; Fri, 06 Jan 2023 10:17:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=1nWrzelfArIcGuHXeTwgFSuDsGz4bBzAEBvOTmzqK8s=; b=GviCqBDMsbHSqEZ7te60Ivt8W0zr+ldNmIFdvuQj2jKROT6ilJCUd55JsiRXMYZnVi gKLQdPUy8q5dcS0O1bQCRPcFwP7IKlqOp8c5FbQo7te+CvLleeMbj9tvUnhyNPn3L3Gl Zi+XcXixkDZgVK4U2+Ghv/jQGC+5iPObuxOLQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=1nWrzelfArIcGuHXeTwgFSuDsGz4bBzAEBvOTmzqK8s=; b=ZeUUYEdQ1aPPJs4jEn0BBsCLYSQHCfvRo6KVI/HzFpt2c2J0JVNWSSdG0stGQvIybO Ie+d1piEciMnN7QyP8D82TH2i6iPzf84dC4jAiDRPD/RZ+BXfnsu7g2AUbGJZ5NwYA6x s/1OHn6ml6USrk32/YcuAo0WJEjxP9VPLsmIwWV4H66VjRWNndXR1PEQZHCpOeMWbNB8 7Iva/zu2CGihKK52wiFScmCQ5bf3x+qy3/mZ7GVqno8p9bzDI8+eGN5iQHCKhAnx4Q4r 0sz5WBIgWWInF6BrNmQYOPvwX0u3Hau1y6JZpY8snRKJ5weg+L09IGd6tDM88ejqVhwM HFJA== X-Gm-Message-State: AFqh2kpFclaYWS6kEKRhN+Qyyf7sUnZOdVzs7mPPFo72y4H+uSwgPhGw ggBRaF1PkbCF7Gkm7mVRY5qtFA== X-Received: by 2002:a92:3012:0:b0:30b:b34b:c3bd with SMTP id x18-20020a923012000000b0030bb34bc3bdmr32701767ile.21.1673029039802; Fri, 06 Jan 2023 10:17:19 -0800 (PST) Received: from localhost (30.23.70.34.bc.googleusercontent.com. [34.70.23.30]) by smtp.gmail.com with UTF8SMTPSA id y88-20020a029561000000b0039e2e4c82c8sm502250jah.123.2023.01.06.10.17.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 06 Jan 2023 10:17:19 -0800 (PST) Date: Fri, 6 Jan 2023 18:17:19 +0000 From: Matthias Kaehlcke To: Manivannan Sadhasivam Cc: Dhruva Gole , lpieralisi@kernel.org, robh@kernel.org, andersson@kernel.org, konrad.dybcio@linaro.org, kw@linux.com, bhelgaas@google.com, linux-pci@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, quic_krichai@quicinc.com, johan+linaro@kernel.org, steev@kali.org Subject: Re: [PATCH 1/1] PCI: qcom: Add support for system suspend and resume Message-ID: References: <20230103074907.12784-1-manivannan.sadhasivam@linaro.org> <20230103074907.12784-2-manivannan.sadhasivam@linaro.org> <20230105133639.GC4463@thinkpad> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20230105133639.GC4463@thinkpad> X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 05, 2023 at 07:06:39PM +0530, Manivannan Sadhasivam wrote: > On Tue, Jan 03, 2023 at 04:46:11PM +0530, Dhruva Gole wrote: > > > > > > On 03/01/23 13:19, Manivannan Sadhasivam wrote: > > > During the system suspend, vote for minimal interconnect bandwidth and > > > also turn OFF the resources like clock and PHY if there are no active > > > devices connected to the controller. For the controllers with active > > > devices, the resources are kept ON as removing the resources will > > > trigger access violation during the late end of suspend cycle as kernel > > > tries to access the config space of PCIe devices to mask the MSIs. > > > > > > Also, it is not desirable to put the link into L2/L3 state as that > > > implies VDD supply will be removed and the devices may go into powerdown > > > state. This will affect the lifetime of storage devices like NVMe. > > > > > > And finally, during resume, turn ON the resources if the controller was > > > truly suspended (resources OFF) and update the interconnect bandwidth > > > based on PCIe Gen speed. > > > > > > Suggested-by: Krishna chaitanya chundru > > > Signed-off-by: Manivannan Sadhasivam > > > --- > > > > Nice to have another driver added to the list of system suspend > > support! > > > > Acked-by: Dhruva Gole > > > > > drivers/pci/controller/dwc/pcie-qcom.c | 52 ++++++++++++++++++++++++++ > > > 1 file changed, 52 insertions(+) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > > > index 5696e327795b..48810f1f2dba 100644 > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > > > @@ -227,6 +227,7 @@ struct qcom_pcie { > > > struct gpio_desc *reset; > > > struct icc_path *icc_mem; > > > const struct qcom_pcie_cfg *cfg;qcom_pcie_icc_update > > > + bool suspended; > > > }; > > > #define to_qcom_pcie(x) dev_get_drvdata((x)->dev) > > > @@ -1835,6 +1836,52 @@ static int qcom_pcie_remove(struct platform_device *pdev) > > > return 0; > > > } > > > +static int qcom_pcie_suspend_noirq(struct device *dev) > > > +{ > > > + struct qcom_pcie *pcie = dev_get_drvdata(dev); > > > + int ret; > > > + > > > + ret = icc_set_bw(pcie->icc_mem, 0, 0); > > > + if (ret) { > > > + dev_err(pcie->pci->dev, "Failed to set interconnect bandwidth: %d\n", ret); > > > + return ret; > > > + } > > > + > > > + /* > > > + * Turn OFF the resources only for controllers without active PCIe devices. For controllers > > > + * with active devices, the resources are kept ON and the link is expected to be in L0/L1 > > > + * (sub)states. > > > + * > > > + * Turning OFF the resources for controllers with active PCIe devices will trigger access > > > + * violation during the end of the suspend cycle, as kernel tries to access the PCIe devices > > > + * config space for masking MSIs. > > > + * > > > + * Also, it is not desirable to put the link into L2/L3 state as that implies VDD supply > > > + * will be removed and the devices may go into powerdown state. This will affect the > > > + * lifetime of the storage devices like NVMe. > > > + */ > > > + if (!dw_pcie_link_up(pcie->pci)) { > > > + qcom_pcie_host_deinit(&pcie->pci->pp); > > > + pcie->suspended = true; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int qcom_pcie_resume_noirq(struct device *dev) > > > +{ > > > + struct qcom_pcie *pcie = dev_get_drvdata(dev); > > > + > > > + if (pcie->suspended) { > > > + qcom_pcie_host_init(&pcie->pci->pp); > > > + pcie->suspended = false; > > > + } > > > + > > > + qcom_pcie_icc_update(pcie); > > > + > > > + return 0; > > > +} > > > + > > > static const struct of_device_id qcom_pcie_match[] = { > > > { .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 }, > > > { .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 }, > > > @@ -1870,12 +1917,17 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x0302, qcom_fixup_class); > > > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1000, qcom_fixup_class); > > > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1001, qcom_fixup_class); > > > +static const struct dev_pm_ops qcom_pcie_pm_ops = { > > > + NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_suspend_noirq, qcom_pcie_resume_noirq) > > > +}; > > > + > > > static struct platform_driver qcom_pcie_driver = { > > > .probe = qcom_pcie_probe, > > > .remove = qcom_pcie_remove, > > > .driver = { > > > .name = "qcom-pcie", > > > .of_match_table = qcom_pcie_match, > > > + .pm = &qcom_pcie_pm_ops, > > > }, > > > }; > > > module_platform_driver(qcom_pcie_driver); > > > > Out of curiosity, were you able to measure how much power you were able > > to save after adding suspend support for PCIe? I don't know if clock > > gating really saves much amount of power, but yeah its true that we > > can't really cut off the power domain entirely in this case. > > > > I did not measure the power consumption and I agree that we won't save much > power with setting icc bandwidth to 0. But it is better to have something > than nothing. And in the coming days, I have plans to look into other power > saving measures also. On a sc7280 system I see a reduction of ~30mW with this patch when no PCI card is plugged in. The reduction seems to come from powering the PHY down. Interestingly on that system power consumption during suspend (without this patch) is ~30mW higher *without* a PCI card vs. with a card. Maybe the PHY doesn't enter a low power mode when no card is plugged in?