Message ID | 1667455698-14578-1-git-send-email-hongxing.zhu@nxp.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp354033wru; Wed, 2 Nov 2022 23:40:45 -0700 (PDT) X-Google-Smtp-Source: AMsMyM4h4QRpNqojc9gZ4q5mbImwgy2KGomfeoxIKMAYeLCsfAxdThC5hgLIE6rTDj9FRB+xiPST X-Received: by 2002:a63:6a87:0:b0:46f:8fcc:de1a with SMTP id f129-20020a636a87000000b0046f8fccde1amr21370490pgc.429.1667457645148; Wed, 02 Nov 2022 23:40:45 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1667457645; cv=none; d=google.com; s=arc-20160816; b=XNlPV/l5hdR6oUm4nXW4pw81tUVkmb/+cXO3l2Yv+X6Fs7RxgPuF8S1yPWlSMa9d8T N70fVBq/kSErN46kzjsYWQDVBcPTrOZdSB7DwunbUZtqmFqchEHSPOpG5OHxG2h6gzbd FdYSk1NlW3ZxqxuO8p++kQmfBS/FGqisjRmKPnHLO80Z6EHqGLQOFaE1rtqtrhzS94JM tuY2PoMNqaGwcQ1At7Oe2AjKREgfW8iZ4bnlGfNZ1FHhKVa3U1uFs4XKiNhoQZCHHXkO NsAPET/G0LBYhN1mGJpA/JECvk7JnMRxzDdczuaspjeoGPp9spNWBmJbP9rHcCKWAlo0 bXBQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:message-id:date:subject:cc:to:from; bh=VaTzJqbJGq1qXGJoZAOPXMm/FaryqYdaiYgtbReNqw0=; b=xiT5R41coezEoQNxlyex58SFUe7zbskvRJjFm4ciuU/RJPN8zbaAvvH2x7+Lh4+6rB 86AhGMk+TTLD3RtqctnvmzW+yTxJeUgh51ogPcIi+hCMcHVY4hENvXu7sKb+aoyXgPZh lUvV5joVqlWaAw/9J1MHgdDbHyvmf5XHW66Bq7gneCoI6jjr2q/OXjq6pkJUoc0MefaS SsbNvqTwPzdt3gGTz4BfXm36hPIYhitKG54ST4usId8bhbMynRKV+ABj4ymhuOo7LrZn gsHvHclgX7+Mo7vbkT2vyuHLAqEiwrF7i90HMz1nYjj1s61tiLPjxBv7gfjIP56kHG4n NKqA== ARC-Authentication-Results: i=1; mx.google.com; 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=fail (p=NONE sp=NONE dis=NONE) header.from=nxp.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id oa14-20020a17090b1bce00b00212d47deabesi260603pjb.60.2022.11.02.23.40.31; Wed, 02 Nov 2022 23:40:45 -0700 (PDT) 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; 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=fail (p=NONE sp=NONE dis=NONE) header.from=nxp.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230451AbiKCG3X (ORCPT <rfc822;yves.mi.zy@gmail.com> + 99 others); Thu, 3 Nov 2022 02:29:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54226 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229531AbiKCG3V (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 3 Nov 2022 02:29:21 -0400 Received: from inva021.nxp.com (inva021.nxp.com [92.121.34.21]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id F2AF4BE12; Wed, 2 Nov 2022 23:29:20 -0700 (PDT) Received: from inva021.nxp.com (localhost [127.0.0.1]) by inva021.eu-rdc02.nxp.com (Postfix) with ESMTP id 3FB8A201444; Thu, 3 Nov 2022 07:29:19 +0100 (CET) Received: from aprdc01srsp001v.ap-rdc01.nxp.com (aprdc01srsp001v.ap-rdc01.nxp.com [165.114.16.16]) by inva021.eu-rdc02.nxp.com (Postfix) with ESMTP id 091C4201440; Thu, 3 Nov 2022 07:29:19 +0100 (CET) Received: from localhost.localdomain (shlinux2.ap.freescale.net [10.192.224.44]) by aprdc01srsp001v.ap-rdc01.nxp.com (Postfix) with ESMTP id ADCA51802201; Thu, 3 Nov 2022 14:29:17 +0800 (+08) From: Richard Zhu <hongxing.zhu@nxp.com> To: l.stach@pengutronix.de, bhelgaas@google.com, lorenzo.pieralisi@arm.com Cc: hongxing.zhu@nxp.com, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kernel@pengutronix.de, linux-imx@nxp.com Subject: [PATCH v1] PCI: imx6: Keep the GPIO regulator always on Date: Thu, 3 Nov 2022 14:08:18 +0800 Message-Id: <1667455698-14578-1-git-send-email-hongxing.zhu@nxp.com> X-Mailer: git-send-email 2.7.4 X-Virus-Scanned: ClamAV using ClamSMTP X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, 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: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1748456067704513569?= X-GMAIL-MSGID: =?utf-8?q?1748456067704513569?= |
Series |
[v1] PCI: imx6: Keep the GPIO regulator always on
|
|
Commit Message
Richard Zhu
Nov. 3, 2022, 6:08 a.m. UTC
Since vpcie regulator is one GPIO regulator, used to control the
VPCIe_3V3 and power up remote PCIe EP device.
Some WIFI modules load their firmware once in probe, and can't be
powered off during suspend. Otherwise, these WIFI modules wouldn't be
functional anymore after resume back.
So, keep this regulator always on in the probe.
Fixes: a4bb720eeb1e ("PCI: imx6: Turn off regulator when system is in suspend mode")
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
drivers/pci/controller/dwc/pci-imx6.c | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
Comments
Hello Richard, On 03.11.22 07:08, Richard Zhu wrote: > Since vpcie regulator is one GPIO regulator, used to control the > VPCIe_3V3 and power up remote PCIe EP device. Regulator need not be a GPIO regulator. I'd drop this from the title. > Some WIFI modules load their firmware once in probe, and can't be > powered off during suspend. Otherwise, these WIFI modules wouldn't be > functional anymore after resume back. The brcmfmac OTOH, reprobes when resuming from suspend. Before this patch, AFAIU, it should've been possible for the EP to go into D3cold during suspend. This may no longer be possible when we keep vpcie powered. > So, keep this regulator always on in the probe. > > Fixes: a4bb720eeb1e ("PCI: imx6: Turn off regulator when system is in suspend mode") Prior to a4bb720eeb1e, vpcie was briefly toggled during PCIe core reset sequence, so aforementioned WiFi modules that don't reprobe over resume should've been broken by that too? If so, I don't see how it fixes that commit as everything that is broken now was broken before that commit as well. After this patch however, modules that can accept vpcie being toggled can't benefit from some of the power saving. Why can't users with this issue use a regulator-always-on regulator instead? > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > --- > drivers/pci/controller/dwc/pci-imx6.c | 24 ++++++++---------------- > 1 file changed, 8 insertions(+), 16 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 2616585ca5f8..94a89bbf381d 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -926,22 +926,13 @@ static int imx6_pcie_host_init(struct dw_pcie_rp *pp) > struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci); > int ret; > > - if (imx6_pcie->vpcie) { > - ret = regulator_enable(imx6_pcie->vpcie); > - if (ret) { > - dev_err(dev, "failed to enable vpcie regulator: %d\n", > - ret); > - return ret; > - } > - } > - > imx6_pcie_assert_core_reset(imx6_pcie); > imx6_pcie_init_phy(imx6_pcie); > > ret = imx6_pcie_clk_enable(imx6_pcie); > if (ret) { > dev_err(dev, "unable to enable pcie clocks: %d\n", ret); > - goto err_reg_disable; > + return ret; > } > > if (imx6_pcie->phy) { > @@ -974,9 +965,6 @@ static int imx6_pcie_host_init(struct dw_pcie_rp *pp) > phy_exit(imx6_pcie->phy); > err_clk_disable: > imx6_pcie_clk_disable(imx6_pcie); > -err_reg_disable: > - if (imx6_pcie->vpcie) > - regulator_disable(imx6_pcie->vpcie); > return ret; > } > > @@ -991,9 +979,6 @@ static void imx6_pcie_host_exit(struct dw_pcie_rp *pp) > phy_exit(imx6_pcie->phy); > } > imx6_pcie_clk_disable(imx6_pcie); > - > - if (imx6_pcie->vpcie) > - regulator_disable(imx6_pcie->vpcie); > } > > static const struct dw_pcie_host_ops imx6_pcie_host_ops = { > @@ -1263,6 +1248,13 @@ static int imx6_pcie_probe(struct platform_device *pdev) > if (PTR_ERR(imx6_pcie->vpcie) != -ENODEV) > return PTR_ERR(imx6_pcie->vpcie); > imx6_pcie->vpcie = NULL; > + } else { > + ret = regulator_enable(imx6_pcie->vpcie); > + if (ret) { > + dev_err(dev, "failed to enable vpcie regulator: %d\n", > + ret); > + return ret; > + } Shouldn't the regulator enable be undone if the probe later fails? Cheers, Ahmad
Hi Ahmad Thanks for your review comments. > -----Original Message----- > From: Ahmad Fatoum <a.fatoum@pengutronix.de> > Sent: 2022年11月3日 15:05 > To: Hongxing Zhu <hongxing.zhu@nxp.com>; l.stach@pengutronix.de; > bhelgaas@google.com; lorenzo.pieralisi@arm.com > Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com>; kernel@pengutronix.de; > linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH v1] PCI: imx6: Keep the GPIO regulator always on > > Hello Richard, > > On 03.11.22 07:08, Richard Zhu wrote: > > Since vpcie regulator is one GPIO regulator, used to control the > > VPCIe_3V3 and power up remote PCIe EP device. > > Regulator need not be a GPIO regulator. I'd drop this from the title. > Got that, thanks. > > Some WIFI modules load their firmware once in probe, and can't be > > powered off during suspend. Otherwise, these WIFI modules wouldn't be > > functional anymore after resume back. > > The brcmfmac OTOH, reprobes when resuming from suspend. Before this patch, > AFAIU, it should've been possible for the EP to go into D3cold during suspend. > This may no longer be possible when we keep vpcie powered. > Oh, understood. In the other words, the EP wouldn't be in D3 mode when vpcie is always powered on, right? Thanks for your detailed explains. > > So, keep this regulator always on in the probe. > > > > Fixes: a4bb720eeb1e ("PCI: imx6: Turn off regulator when system is in > > suspend mode") > > Prior to a4bb720eeb1e, vpcie was briefly toggled during PCIe core reset > sequence, so aforementioned WiFi modules that don't reprobe over resume > should've been broken by that too? If so, I don't see how it fixes that commit > as everything that is broken now was broken before that commit as well. After > this patch however, modules that can accept vpcie being toggled can't benefit > from some of the power saving. The WIFI modules that don't re-probe over resume are always broken, if the vpcie is toggled during suspend/resume, I think. BTW, is the re-probe over resume mandatory requirements for EP devices (for example, WIFI modules)? I'm curious that how the WIFI remote wake up going on if the WIFI module is totally powered off. > > Why can't users with this issue use a regulator-always-on regulator instead? Yes, you're right. One regulator-always-on regulator is a good idea. > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > > --- > > drivers/pci/controller/dwc/pci-imx6.c | 24 ++++++++---------------- > > 1 file changed, 8 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c > > b/drivers/pci/controller/dwc/pci-imx6.c > > index 2616585ca5f8..94a89bbf381d 100644 > > --- a/drivers/pci/controller/dwc/pci-imx6.c > > +++ b/drivers/pci/controller/dwc/pci-imx6.c > > @@ -926,22 +926,13 @@ static int imx6_pcie_host_init(struct dw_pcie_rp > *pp) > > struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci); > > int ret; > > > > - if (imx6_pcie->vpcie) { > > - ret = regulator_enable(imx6_pcie->vpcie); > > - if (ret) { > > - dev_err(dev, "failed to enable vpcie regulator: %d\n", > > - ret); > > - return ret; > > - } > > - } > > - > > imx6_pcie_assert_core_reset(imx6_pcie); > > imx6_pcie_init_phy(imx6_pcie); > > > > ret = imx6_pcie_clk_enable(imx6_pcie); > > if (ret) { > > dev_err(dev, "unable to enable pcie clocks: %d\n", ret); > > - goto err_reg_disable; > > + return ret; > > } > > > > if (imx6_pcie->phy) { > > @@ -974,9 +965,6 @@ static int imx6_pcie_host_init(struct dw_pcie_rp > *pp) > > phy_exit(imx6_pcie->phy); > > err_clk_disable: > > imx6_pcie_clk_disable(imx6_pcie); > > -err_reg_disable: > > - if (imx6_pcie->vpcie) > > - regulator_disable(imx6_pcie->vpcie); > > return ret; > > } > > > > @@ -991,9 +979,6 @@ static void imx6_pcie_host_exit(struct dw_pcie_rp > *pp) > > phy_exit(imx6_pcie->phy); > > } > > imx6_pcie_clk_disable(imx6_pcie); > > - > > - if (imx6_pcie->vpcie) > > - regulator_disable(imx6_pcie->vpcie); > > } > > > > static const struct dw_pcie_host_ops imx6_pcie_host_ops = { @@ > > -1263,6 +1248,13 @@ static int imx6_pcie_probe(struct platform_device > *pdev) > > if (PTR_ERR(imx6_pcie->vpcie) != -ENODEV) > > return PTR_ERR(imx6_pcie->vpcie); > > imx6_pcie->vpcie = NULL; > > + } else { > > + ret = regulator_enable(imx6_pcie->vpcie); > > + if (ret) { > > + dev_err(dev, "failed to enable vpcie regulator: %d\n", > > + ret); > > + return ret; > > + } > > Shouldn't the regulator enable be undone if the probe later fails? > Yes, it's required. Thanks a lot for your comments. Richard Zhu Best Regards > Cheers, > Ahmad > > -- > Pengutronix e.K. | > | > Steuerwalder Str. 21 | > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pe > ngutronix.de%2F&data=05%7C01%7Chongxing.zhu%40nxp.com%7C06f5 > 363342c9464bca5a08dabd69bdb5%7C686ea1d3bc2b4c6fa92cd99c5c301635 > %7C0%7C0%7C638030559094875195%7CUnknown%7CTWFpbGZsb3d8eyJW > IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3 > 000%7C%7C%7C&sdata=36fpveVBgKraYIeEJjMSiPA10azBfhhHrNVYTaocN > nQ%3D&reserved=0 | > 31137 Hildesheim, Germany | Phone: > +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: > +49-5121-206917-5555 |
Am Donnerstag, dem 03.11.2022 um 14:08 +0800 schrieb Richard Zhu: > Since vpcie regulator is one GPIO regulator, used to control the > VPCIe_3V3 and power up remote PCIe EP device. > > Some WIFI modules load their firmware once in probe, and can't be > powered off during suspend. Otherwise, these WIFI modules wouldn't be > functional anymore after resume back. I would call this a bug in the WiFi driver. I think we need to walk down the PCIe hierarchy to see if it is safe to disable the PCIe regulator. When all devices in the hierarchy are in D3hot state, we can safely put the whole hierarchy into D3cold by removing power. When any of the devices connected to the RC are in a state other than D3hot, we need to keep the regulator enabled, as those devices may need power in suspend to implement wakeups or other functionality that should be available during suspend. Regards, Lucas > > So, keep this regulator always on in the probe. > > Fixes: a4bb720eeb1e ("PCI: imx6: Turn off regulator when system is in suspend mode") > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > --- > drivers/pci/controller/dwc/pci-imx6.c | 24 ++++++++---------------- > 1 file changed, 8 insertions(+), 16 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 2616585ca5f8..94a89bbf381d 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -926,22 +926,13 @@ static int imx6_pcie_host_init(struct dw_pcie_rp *pp) > struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci); > int ret; > > - if (imx6_pcie->vpcie) { > - ret = regulator_enable(imx6_pcie->vpcie); > - if (ret) { > - dev_err(dev, "failed to enable vpcie regulator: %d\n", > - ret); > - return ret; > - } > - } > - > imx6_pcie_assert_core_reset(imx6_pcie); > imx6_pcie_init_phy(imx6_pcie); > > ret = imx6_pcie_clk_enable(imx6_pcie); > if (ret) { > dev_err(dev, "unable to enable pcie clocks: %d\n", ret); > - goto err_reg_disable; > + return ret; > } > > if (imx6_pcie->phy) { > @@ -974,9 +965,6 @@ static int imx6_pcie_host_init(struct dw_pcie_rp *pp) > phy_exit(imx6_pcie->phy); > err_clk_disable: > imx6_pcie_clk_disable(imx6_pcie); > -err_reg_disable: > - if (imx6_pcie->vpcie) > - regulator_disable(imx6_pcie->vpcie); > return ret; > } > > @@ -991,9 +979,6 @@ static void imx6_pcie_host_exit(struct dw_pcie_rp *pp) > phy_exit(imx6_pcie->phy); > } > imx6_pcie_clk_disable(imx6_pcie); > - > - if (imx6_pcie->vpcie) > - regulator_disable(imx6_pcie->vpcie); > } > > static const struct dw_pcie_host_ops imx6_pcie_host_ops = { > @@ -1263,6 +1248,13 @@ static int imx6_pcie_probe(struct platform_device *pdev) > if (PTR_ERR(imx6_pcie->vpcie) != -ENODEV) > return PTR_ERR(imx6_pcie->vpcie); > imx6_pcie->vpcie = NULL; > + } else { > + ret = regulator_enable(imx6_pcie->vpcie); > + if (ret) { > + dev_err(dev, "failed to enable vpcie regulator: %d\n", > + ret); > + return ret; > + } > } > > imx6_pcie->vph = devm_regulator_get_optional(&pdev->dev, "vph");
Hello Richard, On 03.11.22 09:05, Hongxing Zhu wrote: >> -----Original Message----- >> From: Ahmad Fatoum <a.fatoum@pengutronix.de> >> On 03.11.22 07:08, Richard Zhu wrote: >>> Some WIFI modules load their firmware once in probe, and can't be >>> powered off during suspend. Otherwise, these WIFI modules wouldn't be >>> functional anymore after resume back. >> >> The brcmfmac OTOH, reprobes when resuming from suspend. Before this patch, >> AFAIU, it should've been possible for the EP to go into D3cold during suspend. >> This may no longer be possible when we keep vpcie powered. >> > Oh, understood. In the other words, the EP wouldn't be in D3 mode when vpcie > is always powered on, right? > Thanks for your detailed explains. D3cold specifically, which is the state the device enters when supply voltage is cut. Devices enter D3hot programmatically and in this case device supply voltage remains powered. >> Prior to a4bb720eeb1e, vpcie was briefly toggled during PCIe core reset >> sequence, so aforementioned WiFi modules that don't reprobe over resume >> should've been broken by that too? If so, I don't see how it fixes that commit >> as everything that is broken now was broken before that commit as well. After >> this patch however, modules that can accept vpcie being toggled can't benefit >> from some of the power saving. > The WIFI modules that don't re-probe over resume are always broken, if the > vpcie is toggled during suspend/resume, I think. > > BTW, is the re-probe over resume mandatory requirements for EP devices > (for example, WIFI modules)? I only looked at brcmfmac. > I'm curious that how the WIFI remote wake up going on if the WIFI module > is totally powered off. Device may be in D3cold, but link is at L2, so there's still auxiliary power for device to support wake on (wired) LAN. No idea how prevalent this is for Wake on Wireless LAN. >> Why can't users with this issue use a regulator-always-on regulator instead? > Yes, you're right. > One regulator-always-on regulator is a good idea. That's what I do on my side as well, because we didn't want the interface to briefly disappear and reappear during suspend. Cheers, Ahmad > >> >>> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> >>> --- >>> drivers/pci/controller/dwc/pci-imx6.c | 24 ++++++++---------------- >>> 1 file changed, 8 insertions(+), 16 deletions(-) >>> >>> diff --git a/drivers/pci/controller/dwc/pci-imx6.c >>> b/drivers/pci/controller/dwc/pci-imx6.c >>> index 2616585ca5f8..94a89bbf381d 100644 >>> --- a/drivers/pci/controller/dwc/pci-imx6.c >>> +++ b/drivers/pci/controller/dwc/pci-imx6.c >>> @@ -926,22 +926,13 @@ static int imx6_pcie_host_init(struct dw_pcie_rp >> *pp) >>> struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci); >>> int ret; >>> >>> - if (imx6_pcie->vpcie) { >>> - ret = regulator_enable(imx6_pcie->vpcie); >>> - if (ret) { >>> - dev_err(dev, "failed to enable vpcie regulator: %d\n", >>> - ret); >>> - return ret; >>> - } >>> - } >>> - >>> imx6_pcie_assert_core_reset(imx6_pcie); >>> imx6_pcie_init_phy(imx6_pcie); >>> >>> ret = imx6_pcie_clk_enable(imx6_pcie); >>> if (ret) { >>> dev_err(dev, "unable to enable pcie clocks: %d\n", ret); >>> - goto err_reg_disable; >>> + return ret; >>> } >>> >>> if (imx6_pcie->phy) { >>> @@ -974,9 +965,6 @@ static int imx6_pcie_host_init(struct dw_pcie_rp >> *pp) >>> phy_exit(imx6_pcie->phy); >>> err_clk_disable: >>> imx6_pcie_clk_disable(imx6_pcie); >>> -err_reg_disable: >>> - if (imx6_pcie->vpcie) >>> - regulator_disable(imx6_pcie->vpcie); >>> return ret; >>> } >>> >>> @@ -991,9 +979,6 @@ static void imx6_pcie_host_exit(struct dw_pcie_rp >> *pp) >>> phy_exit(imx6_pcie->phy); >>> } >>> imx6_pcie_clk_disable(imx6_pcie); >>> - >>> - if (imx6_pcie->vpcie) >>> - regulator_disable(imx6_pcie->vpcie); >>> } >>> >>> static const struct dw_pcie_host_ops imx6_pcie_host_ops = { @@ >>> -1263,6 +1248,13 @@ static int imx6_pcie_probe(struct platform_device >> *pdev) >>> if (PTR_ERR(imx6_pcie->vpcie) != -ENODEV) >>> return PTR_ERR(imx6_pcie->vpcie); >>> imx6_pcie->vpcie = NULL; >>> + } else { >>> + ret = regulator_enable(imx6_pcie->vpcie); >>> + if (ret) { >>> + dev_err(dev, "failed to enable vpcie regulator: %d\n", >>> + ret); >>> + return ret; >>> + } >> >> Shouldn't the regulator enable be undone if the probe later fails? >> > Yes, it's required. > Thanks a lot for your comments. > > Richard Zhu > Best Regards >> Cheers, >> Ahmad >> >> -- >> Pengutronix e.K. | >> | >> Steuerwalder Str. 21 | >> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pe >> ngutronix.de%2F&data=05%7C01%7Chongxing.zhu%40nxp.com%7C06f5 >> 363342c9464bca5a08dabd69bdb5%7C686ea1d3bc2b4c6fa92cd99c5c301635 >> %7C0%7C0%7C638030559094875195%7CUnknown%7CTWFpbGZsb3d8eyJW >> IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3 >> 000%7C%7C%7C&sdata=36fpveVBgKraYIeEJjMSiPA10azBfhhHrNVYTaocN >> nQ%3D&reserved=0 | >> 31137 Hildesheim, Germany | Phone: >> +49-5121-206917-0 | >> Amtsgericht Hildesheim, HRA 2686 | Fax: >> +49-5121-206917-5555 | >
> -----Original Message----- > From: Lucas Stach <l.stach@pengutronix.de> > Sent: 2022年11月3日 17:12 > To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com; > lorenzo.pieralisi@arm.com > Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx > <linux-imx@nxp.com> > Subject: Re: [PATCH v1] PCI: imx6: Keep the GPIO regulator always on > > Am Donnerstag, dem 03.11.2022 um 14:08 +0800 schrieb Richard Zhu: > > Since vpcie regulator is one GPIO regulator, used to control the > > VPCIe_3V3 and power up remote PCIe EP device. > > > > Some WIFI modules load their firmware once in probe, and can't be > > powered off during suspend. Otherwise, these WIFI modules wouldn't be > > functional anymore after resume back. > > I would call this a bug in the WiFi driver. > > I think we need to walk down the PCIe hierarchy to see if it is safe to disable > the PCIe regulator. When all devices in the hierarchy are in D3hot state, we can > safely put the whole hierarchy into D3cold by removing power. When any of > the devices connected to the RC are in a state other than D3hot, we need to > keep the regulator enabled, as those devices may need power in suspend to > implement wakeups or other functionality that should be available during > suspend. > Understood, thanks a lot. Best Regards Richard Zhu > Regards, > Lucas > > > > > So, keep this regulator always on in the probe. > > > > Fixes: a4bb720eeb1e ("PCI: imx6: Turn off regulator when system is in > > suspend mode") > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > > --- > > drivers/pci/controller/dwc/pci-imx6.c | 24 ++++++++---------------- > > 1 file changed, 8 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c > > b/drivers/pci/controller/dwc/pci-imx6.c > > index 2616585ca5f8..94a89bbf381d 100644 > > --- a/drivers/pci/controller/dwc/pci-imx6.c > > +++ b/drivers/pci/controller/dwc/pci-imx6.c > > @@ -926,22 +926,13 @@ static int imx6_pcie_host_init(struct dw_pcie_rp > *pp) > > struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci); > > int ret; > > > > - if (imx6_pcie->vpcie) { > > - ret = regulator_enable(imx6_pcie->vpcie); > > - if (ret) { > > - dev_err(dev, "failed to enable vpcie regulator: %d\n", > > - ret); > > - return ret; > > - } > > - } > > - > > imx6_pcie_assert_core_reset(imx6_pcie); > > imx6_pcie_init_phy(imx6_pcie); > > > > ret = imx6_pcie_clk_enable(imx6_pcie); > > if (ret) { > > dev_err(dev, "unable to enable pcie clocks: %d\n", ret); > > - goto err_reg_disable; > > + return ret; > > } > > > > if (imx6_pcie->phy) { > > @@ -974,9 +965,6 @@ static int imx6_pcie_host_init(struct dw_pcie_rp > *pp) > > phy_exit(imx6_pcie->phy); > > err_clk_disable: > > imx6_pcie_clk_disable(imx6_pcie); > > -err_reg_disable: > > - if (imx6_pcie->vpcie) > > - regulator_disable(imx6_pcie->vpcie); > > return ret; > > } > > > > @@ -991,9 +979,6 @@ static void imx6_pcie_host_exit(struct dw_pcie_rp > *pp) > > phy_exit(imx6_pcie->phy); > > } > > imx6_pcie_clk_disable(imx6_pcie); > > - > > - if (imx6_pcie->vpcie) > > - regulator_disable(imx6_pcie->vpcie); > > } > > > > static const struct dw_pcie_host_ops imx6_pcie_host_ops = { @@ > > -1263,6 +1248,13 @@ static int imx6_pcie_probe(struct platform_device > *pdev) > > if (PTR_ERR(imx6_pcie->vpcie) != -ENODEV) > > return PTR_ERR(imx6_pcie->vpcie); > > imx6_pcie->vpcie = NULL; > > + } else { > > + ret = regulator_enable(imx6_pcie->vpcie); > > + if (ret) { > > + dev_err(dev, "failed to enable vpcie regulator: %d\n", > > + ret); > > + return ret; > > + } > > } > > > > imx6_pcie->vph = devm_regulator_get_optional(&pdev->dev, "vph"); >
> -----Original Message----- > From: Ahmad Fatoum <a.fatoum@pengutronix.de> > Sent: 2022年11月3日 18:30 > To: Hongxing Zhu <hongxing.zhu@nxp.com>; l.stach@pengutronix.de; > bhelgaas@google.com; lorenzo.pieralisi@arm.com > Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com>; kernel@pengutronix.de; > linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH v1] PCI: imx6: Keep the GPIO regulator always on > > Hello Richard, > > On 03.11.22 09:05, Hongxing Zhu wrote: > >> -----Original Message----- > >> From: Ahmad Fatoum <a.fatoum@pengutronix.de> On 03.11.22 07:08, > >> Richard Zhu wrote: > >>> Some WIFI modules load their firmware once in probe, and can't be > >>> powered off during suspend. Otherwise, these WIFI modules wouldn't > >>> be functional anymore after resume back. > >> > >> The brcmfmac OTOH, reprobes when resuming from suspend. Before this > >> patch, AFAIU, it should've been possible for the EP to go into D3cold during > suspend. > >> This may no longer be possible when we keep vpcie powered. > >> > > Oh, understood. In the other words, the EP wouldn't be in D3 mode when > > vpcie is always powered on, right? > > Thanks for your detailed explains. > > D3cold specifically, which is the state the device enters when supply voltage is > cut. Devices enter D3hot programmatically and in this case device supply > voltage remains powered. Got that, thanks. > > >> Prior to a4bb720eeb1e, vpcie was briefly toggled during PCIe core > >> reset sequence, so aforementioned WiFi modules that don't reprobe > >> over resume should've been broken by that too? If so, I don't see how > >> it fixes that commit as everything that is broken now was broken > >> before that commit as well. After this patch however, modules that > >> can accept vpcie being toggled can't benefit from some of the power saving. > > The WIFI modules that don't re-probe over resume are always broken, if > > the vpcie is toggled during suspend/resume, I think. > > > > BTW, is the re-probe over resume mandatory requirements for EP devices > > (for example, WIFI modules)? > > I only looked at brcmfmac. Got that. > > > I'm curious that how the WIFI remote wake up going on if the WIFI > > module is totally powered off. > > Device may be in D3cold, but link is at L2, so there's still auxiliary power for > device to support wake on (wired) LAN. No idea how prevalent this is for Wake > on Wireless LAN. > Thanks. > >> Why can't users with this issue use a regulator-always-on regulator instead? > > Yes, you're right. > > One regulator-always-on regulator is a good idea. > > That's what I do on my side as well, because we didn't want the interface to > briefly disappear and reappear during suspend. Understood, I prefer to use the similar method on the EVK boards. Thanks a lot for your great help. Best Regards Richard Zhu > > Cheers, > Ahmad > > > > >> > >>> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > >>> --- > >>> drivers/pci/controller/dwc/pci-imx6.c | 24 ++++++++---------------- > >>> 1 file changed, 8 insertions(+), 16 deletions(-) > >>> > >>> diff --git a/drivers/pci/controller/dwc/pci-imx6.c > >>> b/drivers/pci/controller/dwc/pci-imx6.c > >>> index 2616585ca5f8..94a89bbf381d 100644 > >>> --- a/drivers/pci/controller/dwc/pci-imx6.c > >>> +++ b/drivers/pci/controller/dwc/pci-imx6.c > >>> @@ -926,22 +926,13 @@ static int imx6_pcie_host_init(struct > >>> dw_pcie_rp > >> *pp) > >>> struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci); > >>> int ret; > >>> > >>> - if (imx6_pcie->vpcie) { > >>> - ret = regulator_enable(imx6_pcie->vpcie); > >>> - if (ret) { > >>> - dev_err(dev, "failed to enable vpcie regulator: %d\n", > >>> - ret); > >>> - return ret; > >>> - } > >>> - } > >>> - > >>> imx6_pcie_assert_core_reset(imx6_pcie); > >>> imx6_pcie_init_phy(imx6_pcie); > >>> > >>> ret = imx6_pcie_clk_enable(imx6_pcie); > >>> if (ret) { > >>> dev_err(dev, "unable to enable pcie clocks: %d\n", ret); > >>> - goto err_reg_disable; > >>> + return ret; > >>> } > >>> > >>> if (imx6_pcie->phy) { > >>> @@ -974,9 +965,6 @@ static int imx6_pcie_host_init(struct dw_pcie_rp > >> *pp) > >>> phy_exit(imx6_pcie->phy); > >>> err_clk_disable: > >>> imx6_pcie_clk_disable(imx6_pcie); > >>> -err_reg_disable: > >>> - if (imx6_pcie->vpcie) > >>> - regulator_disable(imx6_pcie->vpcie); > >>> return ret; > >>> } > >>> > >>> @@ -991,9 +979,6 @@ static void imx6_pcie_host_exit(struct > >>> dw_pcie_rp > >> *pp) > >>> phy_exit(imx6_pcie->phy); > >>> } > >>> imx6_pcie_clk_disable(imx6_pcie); > >>> - > >>> - if (imx6_pcie->vpcie) > >>> - regulator_disable(imx6_pcie->vpcie); > >>> } > >>> > >>> static const struct dw_pcie_host_ops imx6_pcie_host_ops = { @@ > >>> -1263,6 +1248,13 @@ static int imx6_pcie_probe(struct > >>> platform_device > >> *pdev) > >>> if (PTR_ERR(imx6_pcie->vpcie) != -ENODEV) > >>> return PTR_ERR(imx6_pcie->vpcie); > >>> imx6_pcie->vpcie = NULL; > >>> + } else { > >>> + ret = regulator_enable(imx6_pcie->vpcie); > >>> + if (ret) { > >>> + dev_err(dev, "failed to enable vpcie regulator: %d\n", > >>> + ret); > >>> + return ret; > >>> + } > >> > >> Shouldn't the regulator enable be undone if the probe later fails? > >> > > Yes, it's required. > > Thanks a lot for your comments. > > > > Richard Zhu > > Best Regards > >> Cheers, > >> Ahmad > >> > >> -- > >> Pengutronix e.K. | > >> | > >> Steuerwalder Str. 21 | > >> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww. > >> > pe%2F&data=05%7C01%7Chongxing.zhu%40nxp.com%7C274e02514229 > 42cf8cb > >> > 108dabd865a38%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63 > 80306819 > >> > 77779878%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi > V2luMzIi > >> > LCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=j3q7D > pydoaQc > >> pyqJL6u4179YCOkD5FpzQdbK3MUE%2BlA%3D&reserved=0 > >> > ngutronix.de%2F&data=05%7C01%7Chongxing.zhu%40nxp.com%7C06f5 > >> > 363342c9464bca5a08dabd69bdb5%7C686ea1d3bc2b4c6fa92cd99c5c301635 > >> %7C0%7C0%7C638030559094875195%7CUnknown%7CTWFpbGZsb3d8ey > JW > >> > IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3 > >> > 000%7C%7C%7C&sdata=36fpveVBgKraYIeEJjMSiPA10azBfhhHrNVYTaocN > >> nQ%3D&reserved=0 | > >> 31137 Hildesheim, Germany | Phone: > >> +49-5121-206917-0 | > >> Amtsgericht Hildesheim, HRA 2686 | Fax: > >> +49-5121-206917-5555 | > > > > -- > Pengutronix e.K. | > | > Steuerwalder Str. 21 | > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pe > ngutronix.de%2F&data=05%7C01%7Chongxing.zhu%40nxp.com%7C274e > 0251422942cf8cb108dabd865a38%7C686ea1d3bc2b4c6fa92cd99c5c301635 > %7C0%7C0%7C638030681977779878%7CUnknown%7CTWFpbGZsb3d8eyJW > IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3 > 000%7C%7C%7C&sdata=TZubKn0%2BjcwEyDh1r0WXhRy%2FTM%2FeOA > eAufGNLtMtCEg%3D&reserved=0 | > 31137 Hildesheim, Germany | Phone: > +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: > +49-5121-206917-5555 |
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index 2616585ca5f8..94a89bbf381d 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/dwc/pci-imx6.c @@ -926,22 +926,13 @@ static int imx6_pcie_host_init(struct dw_pcie_rp *pp) struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci); int ret; - if (imx6_pcie->vpcie) { - ret = regulator_enable(imx6_pcie->vpcie); - if (ret) { - dev_err(dev, "failed to enable vpcie regulator: %d\n", - ret); - return ret; - } - } - imx6_pcie_assert_core_reset(imx6_pcie); imx6_pcie_init_phy(imx6_pcie); ret = imx6_pcie_clk_enable(imx6_pcie); if (ret) { dev_err(dev, "unable to enable pcie clocks: %d\n", ret); - goto err_reg_disable; + return ret; } if (imx6_pcie->phy) { @@ -974,9 +965,6 @@ static int imx6_pcie_host_init(struct dw_pcie_rp *pp) phy_exit(imx6_pcie->phy); err_clk_disable: imx6_pcie_clk_disable(imx6_pcie); -err_reg_disable: - if (imx6_pcie->vpcie) - regulator_disable(imx6_pcie->vpcie); return ret; } @@ -991,9 +979,6 @@ static void imx6_pcie_host_exit(struct dw_pcie_rp *pp) phy_exit(imx6_pcie->phy); } imx6_pcie_clk_disable(imx6_pcie); - - if (imx6_pcie->vpcie) - regulator_disable(imx6_pcie->vpcie); } static const struct dw_pcie_host_ops imx6_pcie_host_ops = { @@ -1263,6 +1248,13 @@ static int imx6_pcie_probe(struct platform_device *pdev) if (PTR_ERR(imx6_pcie->vpcie) != -ENODEV) return PTR_ERR(imx6_pcie->vpcie); imx6_pcie->vpcie = NULL; + } else { + ret = regulator_enable(imx6_pcie->vpcie); + if (ret) { + dev_err(dev, "failed to enable vpcie regulator: %d\n", + ret); + return ret; + } } imx6_pcie->vph = devm_regulator_get_optional(&pdev->dev, "vph");