Received: by 2002:a05:6358:16cc:b0:ea:6187:17c9 with SMTP id r12csp6905846rwl; Mon, 9 Jan 2023 14:54:08 -0800 (PST) X-Google-Smtp-Source: AMrXdXuC1145/ovbmRSoYM2/pDu0675SAOYUnlYpKlWSI0q4hsY0LMDDHnouYFuDrgEdlHXt/Tgk X-Received: by 2002:a17:907:8c0c:b0:7c1:1901:a445 with SMTP id ta12-20020a1709078c0c00b007c11901a445mr59255243ejc.20.1673304848378; Mon, 09 Jan 2023 14:54:08 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1673304848; cv=none; d=google.com; s=arc-20160816; b=fFtpdtWoYKRm0V8W+Pi4I38dKDL4v/Ru3nOz+rCh1ferEYsotJ9oncqnqdOjetTLuc 0w91/3Xtk1WWxONltCcQZXOaszwfmy0KZltDDGHl0oo7Xa3S3pi6P2LkttKDjNiSmvC7 zLu9JZ9VoHLYV6TPnXyM5Vo70DbSEr8sUQkN4fXssSgpOCD6duAh8dMNdkujZcNwIA4y PlDRgccC+UTC//oNUD8Q/uYagp3BSGbHaaQ/HUmXxdmVWU1bSGzQwTlkQCwQdxvKLlx/ n6jG6+11SYCsVOENxdMgvtRMI+f4cf51W9eO8EEhnblpm4G+0tZP6/60sNlswgJx1nuv NSSw== 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=TErLXK6Xw8NHk2rMZzxz1f1Mf6NRGAvKbG0LeKvkd80=; b=fNhPxDCPzxGVhkT9kMuUtP9tY0Kb9HOGFdDNjbzSQdJQ688MxASV7B4vf1xCNaIYqo WK2ZknEd0y1gkjQB4XV1WEhRfNMNUVzAVsaX60PPEWhNRpDZRrtyFTFYzGZpM0l8urhc DBfWSgVEKOReCwO/iF4gazvlpaizPFRAXy9YsB2w9eoAZVECZgtcBp67m/5JONSfYoye z5F2w+MxbpmYzKtvVxG58frhoYLuYarMnszK8BPNs7deXy+qQvZx5LhcyKb/D5pD9DWi 2VqxudGXsqKPMernjxLS5QafqSw3m/+V5h0ExNK+pmZS9fi271L/UpnX14scRnQ8pFgK uMDA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=HBfGUR2B; 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 ht21-20020a170907609500b007c167cdc18esi11165673ejc.747.2023.01.09.14.53.55; Mon, 09 Jan 2023 14:54:08 -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=HBfGUR2B; 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 S237523AbjAIWnR (ORCPT + 53 others); Mon, 9 Jan 2023 17:43:17 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58858 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237809AbjAIWnA (ORCPT ); Mon, 9 Jan 2023 17:43:00 -0500 Received: from mail-il1-x132.google.com (mail-il1-x132.google.com [IPv6:2607:f8b0:4864:20::132]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 976BF34764 for ; Mon, 9 Jan 2023 14:42:01 -0800 (PST) Received: by mail-il1-x132.google.com with SMTP id bp26so5635034ilb.3 for ; Mon, 09 Jan 2023 14:42:01 -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=TErLXK6Xw8NHk2rMZzxz1f1Mf6NRGAvKbG0LeKvkd80=; b=HBfGUR2BKxXIVeDbpudbJRq9L23fs0sHUreqrbBXmmUUZbfH6VqskmyOg1iWjFi/rW S9b+M0aSingPAzsb+yhQFYUUaRtGiL+OWRSYNpv/R+1hTEWJT5RJ8/+sRh9OgS3o6eCO r/jHmUeGmniwWQx8K/O0M/odoYUr+ZbHaEC6g= 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=TErLXK6Xw8NHk2rMZzxz1f1Mf6NRGAvKbG0LeKvkd80=; b=mC0MMrXQ6n2ajOrJS+vIoiqMZHx6ZZ/vCNPpfVdkX97mYUnUtLxplx0ke2fcx9Bc4M 0SssxmVS+bawcRwFMzz+80GwaxBwzg2sadkDy1Ome23rmwT/kEqvHk4tcuk5UZON5fki vHo4O4LLh4OpaJtB3IX5gG3VHjENZqD8MC5snYWJj6V0/Zu3kfUDzXG8VwS9gITa/v/1 Hb3Ysqls5LV2xN0IxvvwXt+477uR7LVkJjoXdeaTerTu9qyut72E9eWmpQvBoXTSnuKW WxyYBKjqZZmZFjRiIW1jMlxVRs3lcYY7rYKzKt7NoVeKY0KTGHaebNgK5wiBxOreqHg1 +QXw== X-Gm-Message-State: AFqh2ko1/UAR0DM67m4d/301fRcMQ6c8LULtt7asfdKWsmGWZ1Hxk9xP c+CEOhhqAk1Rx7/HKnoHCQifZQ== X-Received: by 2002:a92:c744:0:b0:30c:41cc:6978 with SMTP id y4-20020a92c744000000b0030c41cc6978mr22570638ilp.1.1673304120899; Mon, 09 Jan 2023 14:42:00 -0800 (PST) Received: from localhost (30.23.70.34.bc.googleusercontent.com. [34.70.23.30]) by smtp.gmail.com with UTF8SMTPSA id 191-20020a021dc8000000b0039e07ca9ae5sm3109938jaj.113.2023.01.09.14.42.00 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 09 Jan 2023 14:42:00 -0800 (PST) Date: Mon, 9 Jan 2023 22:41:59 +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> <20230106190252.GA485076@thinkpad> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20230106190252.GA485076@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=unavailable 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 Sat, Jan 07, 2023 at 12:32:52AM +0530, Manivannan Sadhasivam wrote: > On Fri, Jan 06, 2023 at 06:17:19PM +0000, Matthias Kaehlcke wrote: > > 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. > > > > Thanks a lot for testing! > > > 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? > > Yeah, both PHY and controllers are never put into low power mode even if there > are no devices connected. I don't know if the low power mode is possible at > all with PHY. It's still interesting that the PHY apparently at least enters a *lower* power mode when a card is plugged in, the extra 30mW are only seen without a card.