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 <[email protected]>
---
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");
--
2.25.1
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 <[email protected]>
> ---
> 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
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Ahmad
Thanks for your review comments.
> -----Original Message-----
> From: Ahmad Fatoum <[email protected]>
> Sent: 2022??11??3?? 15:05
> To: Hongxing Zhu <[email protected]>; [email protected];
> [email protected]; [email protected]
> Cc: [email protected]; [email protected]; dl-linux-imx
> <[email protected]>; [email protected];
> [email protected]
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> ---
> 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 <[email protected]>
>> 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 <[email protected]>
>>> ---
>>> 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 |
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
> -----Original Message-----
> From: Ahmad Fatoum <[email protected]>
> Sent: 2022??11??3?? 18:30
> To: Hongxing Zhu <[email protected]>; [email protected];
> [email protected]; [email protected]
> Cc: [email protected]; [email protected]; dl-linux-imx
> <[email protected]>; [email protected];
> [email protected]
> 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 <[email protected]> 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 <[email protected]>
> >>> ---
> >>> 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 |
> -----Original Message-----
> From: Lucas Stach <[email protected]>
> Sent: 2022年11月3日 17:12
> To: Hongxing Zhu <[email protected]>; [email protected];
> [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; dl-linux-imx
> <[email protected]>
> 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 <[email protected]>
> > ---
> > 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");
>