RE: [PATCH v1] PCI: imx6: Keep the GPIO regulator always on

From: Hongxing Zhu
Date: Thu Nov 03 2022 - 21:22:19 EST

Next message: Lu Wei: "[patch net v5] tcp: prohibit TCP_REPAIR_OPTIONS if data was already sent"
Previous message: Mike Kravetz: "[PATCH] selftests/vm: update hugetlb madvise"
In reply to: Ahmad Fatoum: "Re: [PATCH v1] PCI: imx6: Keep the GPIO regulator always on"
Next in thread: Lucas Stach: "Re: [PATCH v1] PCI: imx6: Keep the GPIO regulator always on"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx>
> Sent: 2022年11月3日 18:30
> To: Hongxing Zhu <hongxing.zhu@xxxxxxx>; l.stach@xxxxxxxxxxxxxx;
> bhelgaas@xxxxxxxxxx; lorenzo.pieralisi@xxxxxxx
> Cc: linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>; kernel@xxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxx> 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@xxxxxxx>
> >>> ---
> >>> 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 |

Next message: Lu Wei: "[patch net v5] tcp: prohibit TCP_REPAIR_OPTIONS if data was already sent"
Previous message: Mike Kravetz: "[PATCH] selftests/vm: update hugetlb madvise"
In reply to: Ahmad Fatoum: "Re: [PATCH v1] PCI: imx6: Keep the GPIO regulator always on"
Next in thread: Lucas Stach: "Re: [PATCH v1] PCI: imx6: Keep the GPIO regulator always on"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]