From: Richard Zhu <[email protected]>
The driver should undo any enables it did itself. The regulator disable
shouldn't be basing decisions on regulator_is_enabled().
Move the regulator_disable to the suspend function, turn off regulator when
the system is in suspend mode.
To keep the balance of the regulator usage counter, disable the regulator
in shutdown.
Link: https://lore.kernel.org/r/1655189942-12678-6-git-send-email-hongxing.z
[email protected]
Signed-off-by: Richard Zhu <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 6eddd0b5f628..537b8a2e0e3b 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -670,8 +670,6 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
{
- struct device *dev = imx6_pcie->pci->dev;
-
switch (imx6_pcie->drvdata->variant) {
case IMX7D:
case IMX8MQ:
@@ -702,14 +700,6 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
break;
}
- if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
- int ret = regulator_disable(imx6_pcie->vpcie);
-
- if (ret)
- dev_err(dev, "failed to disable vpcie regulator: %d\n",
- ret);
- }
-
/* Some boards don't have PCIe reset GPIO. */
if (gpio_is_valid(imx6_pcie->reset_gpio))
gpio_set_value_cansleep(imx6_pcie->reset_gpio,
@@ -722,7 +712,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
struct device *dev = pci->dev;
int ret, err;
- if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
+ if (imx6_pcie->vpcie) {
ret = regulator_enable(imx6_pcie->vpcie);
if (ret) {
dev_err(dev, "failed to enable vpcie regulator: %d\n",
@@ -796,7 +786,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
return 0;
err_clks:
- if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
+ if (imx6_pcie->vpcie) {
ret = regulator_disable(imx6_pcie->vpcie);
if (ret)
dev_err(dev, "failed to disable vpcie regulator: %d\n",
@@ -1023,6 +1013,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
break;
}
+ if (imx6_pcie->vpcie)
+ regulator_disable(imx6_pcie->vpcie);
+
return 0;
}
@@ -1269,6 +1262,8 @@ static void imx6_pcie_shutdown(struct platform_device *pdev)
/* bring down link, so bootloader gets clean state in case of reboot */
imx6_pcie_assert_core_reset(imx6_pcie);
+ if (imx6_pcie->vpcie)
+ regulator_disable(imx6_pcie->vpcie);
}
static const struct imx6_pcie_drvdata drvdata[] = {
--
2.25.1
On Wed, Jun 15, 2022 at 06:15:47PM -0500, Bjorn Helgaas wrote:
> From: Richard Zhu <[email protected]>
>
> The driver should undo any enables it did itself. The regulator disable
> shouldn't be basing decisions on regulator_is_enabled().
>
> Move the regulator_disable to the suspend function, turn off regulator when
> the system is in suspend mode.
>
> To keep the balance of the regulator usage counter, disable the regulator
> in shutdown.
>
> Link: https://lore.kernel.org/r/1655189942-12678-6-git-send-email-hongxing.z
> [email protected]
> Signed-off-by: Richard Zhu <[email protected]>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++------------
> 1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 6eddd0b5f628..537b8a2e0e3b 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -670,8 +670,6 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
>
> static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> {
> - struct device *dev = imx6_pcie->pci->dev;
> -
> switch (imx6_pcie->drvdata->variant) {
> case IMX7D:
> case IMX8MQ:
> @@ -702,14 +700,6 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> break;
> }
>
> - if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> - int ret = regulator_disable(imx6_pcie->vpcie);
> -
> - if (ret)
> - dev_err(dev, "failed to disable vpcie regulator: %d\n",
> - ret);
> - }
> -
> /* Some boards don't have PCIe reset GPIO. */
> if (gpio_is_valid(imx6_pcie->reset_gpio))
> gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> @@ -722,7 +712,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> struct device *dev = pci->dev;
> int ret, err;
>
> - if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
> + if (imx6_pcie->vpcie) {
> ret = regulator_enable(imx6_pcie->vpcie);
> if (ret) {
> dev_err(dev, "failed to enable vpcie regulator: %d\n",
> @@ -796,7 +786,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> return 0;
>
> err_clks:
> - if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> + if (imx6_pcie->vpcie) {
> ret = regulator_disable(imx6_pcie->vpcie);
> if (ret)
> dev_err(dev, "failed to disable vpcie regulator: %d\n",
> @@ -1023,6 +1013,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> break;
> }
>
> + if (imx6_pcie->vpcie)
> + regulator_disable(imx6_pcie->vpcie);
This is a little bit ugly because imx6_pcie_suspend_noirq() and
imx6_pcie_resume_noirq() are not symmetric.
We call regulator_disable() directly here in
imx6_pcie_suspend_noirq(), but the corresponding regulator_enable() is
buried in imx6_pcie_deassert_core_reset().
It would be nicer if the suspend and resume paths looked more similar,
as rockchip_pcie_suspend_noirq() and rockchip_pcie_resume_noirq() do,
for example.
> return 0;
> }
>
> @@ -1269,6 +1262,8 @@ static void imx6_pcie_shutdown(struct platform_device *pdev)
>
> /* bring down link, so bootloader gets clean state in case of reboot */
> imx6_pcie_assert_core_reset(imx6_pcie);
> + if (imx6_pcie->vpcie)
> + regulator_disable(imx6_pcie->vpcie);
> }
>
> static const struct imx6_pcie_drvdata drvdata[] = {
> --
> 2.25.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> -----Original Message-----
> From: Bjorn Helgaas <[email protected]>
> Sent: 2022??6??16?? 7:31
> To: Hongxing Zhu <[email protected]>; Lucas Stach
> <[email protected]>; Rob Herring <[email protected]>; Mark Brown
> <[email protected]>; Lorenzo Pieralisi <[email protected]>; Fabio
> Estevam <[email protected]>; Francesco Dolcini
> <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; dl-linux-imx
> <[email protected]>; Bjorn Helgaas <[email protected]>
> Subject: Re: [PATCH v12 09/13] PCI: imx6: Turn off regulator when system is in
> suspend mode
>
> On Wed, Jun 15, 2022 at 06:15:47PM -0500, Bjorn Helgaas wrote:
> > From: Richard Zhu <[email protected]>
> >
> > The driver should undo any enables it did itself. The regulator
> > disable shouldn't be basing decisions on regulator_is_enabled().
> >
> > Move the regulator_disable to the suspend function, turn off regulator
> > when the system is in suspend mode.
> >
> > To keep the balance of the regulator usage counter, disable the
> > regulator in shutdown.
> >
> > Link:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Fr%2F1655189942-12678-6-git-send-email-hongxing.z&d
> at
> >
> a=05%7C01%7Chongxing.zhu%40nxp.com%7C8dea46d908d34cb4825808da4
> f270eaf%
> >
> 7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379093264156415
> 35%7CUnkn
> >
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> haWwi
> >
> LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=tqzlcCmtSkUNxNzJ0Fr5o
> CR3X2CTH
> > 4dwUJS4NXt5KRc%3D&reserved=0
> > [email protected]
> > Signed-off-by: Richard Zhu <[email protected]>
> > Signed-off-by: Bjorn Helgaas <[email protected]>
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++------------
> > 1 file changed, 7 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 6eddd0b5f628..537b8a2e0e3b 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -670,8 +670,6 @@ static void imx6_pcie_clk_disable(struct imx6_pcie
> > *imx6_pcie)
> >
> > static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> > {
> > - struct device *dev = imx6_pcie->pci->dev;
> > -
> > switch (imx6_pcie->drvdata->variant) {
> > case IMX7D:
> > case IMX8MQ:
> > @@ -702,14 +700,6 @@ static void imx6_pcie_assert_core_reset(struct
> imx6_pcie *imx6_pcie)
> > break;
> > }
> >
> > - if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> > - int ret = regulator_disable(imx6_pcie->vpcie);
> > -
> > - if (ret)
> > - dev_err(dev, "failed to disable vpcie regulator: %d\n",
> > - ret);
> > - }
> > -
> > /* Some boards don't have PCIe reset GPIO. */
> > if (gpio_is_valid(imx6_pcie->reset_gpio))
> > gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> > @@ -722,7 +712,7 @@ static int imx6_pcie_deassert_core_reset(struct
> imx6_pcie *imx6_pcie)
> > struct device *dev = pci->dev;
> > int ret, err;
> >
> > - if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
> > + if (imx6_pcie->vpcie) {
> > ret = regulator_enable(imx6_pcie->vpcie);
> > if (ret) {
> > dev_err(dev, "failed to enable vpcie regulator: %d\n", @@
> -796,7
> > +786,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie
> *imx6_pcie)
> > return 0;
> >
> > err_clks:
> > - if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> > + if (imx6_pcie->vpcie) {
> > ret = regulator_disable(imx6_pcie->vpcie);
> > if (ret)
> > dev_err(dev, "failed to disable vpcie regulator: %d\n", @@
> -1023,6
> > +1013,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> > break;
> > }
> >
> > + if (imx6_pcie->vpcie)
> > + regulator_disable(imx6_pcie->vpcie);
>
> This is a little bit ugly because imx6_pcie_suspend_noirq() and
> imx6_pcie_resume_noirq() are not symmetric.
>
> We call regulator_disable() directly here in imx6_pcie_suspend_noirq(), but the
> corresponding regulator_enable() is buried in imx6_pcie_deassert_core_reset().
>
> It would be nicer if the suspend and resume paths looked more similar, as
> rockchip_pcie_suspend_noirq() and rockchip_pcie_resume_noirq() do, for
> example.
Yes, it is. The regulator_disable()/regulator_enable() are not invoked
symmetrically.
In the original codes, the regulator_disable() is contained in
imx6_pcie_assert_core_reset() and with _eanbled() check.
And regulator_enable() is embedded in imx6_pcie_deassert_core_reset().
Both the regulator disable and enable are invoked in resume before.
I move the regulator_disable to suspend mode refer to Lucas' comments.
Refer to the current situation, how about to move the regulator_disable()
out of imx6_pcie_deassert_core_reset() too?
Best Regards
Richard Zhu
>
> > return 0;
> > }
> >
> > @@ -1269,6 +1262,8 @@ static void imx6_pcie_shutdown(struct
> > platform_device *pdev)
> >
> > /* bring down link, so bootloader gets clean state in case of reboot */
> > imx6_pcie_assert_core_reset(imx6_pcie);
> > + if (imx6_pcie->vpcie)
> > + regulator_disable(imx6_pcie->vpcie);
> > }
> >
> > static const struct imx6_pcie_drvdata drvdata[] = {
> > --
> > 2.25.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [email protected]
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists
> > .infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&data=05%7
> C0
> >
> 1%7Chongxing.zhu%40nxp.com%7C8dea46d908d34cb4825808da4f270eaf%7
> C686ea1
> >
> d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637909326415641535%7CUnk
> nown%7CTW
> >
> FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV
> CI6
> >
> Mn0%3D%7C3000%7C%7C%7C&sdata=xjn3jX7ub9%2BX%2BAJQ7YPecV5
> ujkzLAPXkY
> > hcieKs%2BKqg%3D&reserved=0
On Thu, Jun 16, 2022 at 02:23:29AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <[email protected]>
> > Sent: 2022年6月16日 7:31
> > To: Hongxing Zhu <[email protected]>; Lucas Stach
> > <[email protected]>; Rob Herring <[email protected]>; Mark Brown
> > <[email protected]>; Lorenzo Pieralisi <[email protected]>; Fabio
> > Estevam <[email protected]>; Francesco Dolcini
> > <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]; dl-linux-imx
> > <[email protected]>; Bjorn Helgaas <[email protected]>
> > Subject: Re: [PATCH v12 09/13] PCI: imx6: Turn off regulator when system is in
> > suspend mode
> >
> > On Wed, Jun 15, 2022 at 06:15:47PM -0500, Bjorn Helgaas wrote:
> > > From: Richard Zhu <[email protected]>
> > >
> > > The driver should undo any enables it did itself. The regulator
> > > disable shouldn't be basing decisions on regulator_is_enabled().
> > >
> > > Move the regulator_disable to the suspend function, turn off regulator
> > > when the system is in suspend mode.
> > >
> > > To keep the balance of the regulator usage counter, disable the
> > > regulator in shutdown.
> > >
> > > Link:
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > > .kernel.org%2Fr%2F1655189942-12678-6-git-send-email-hongxing.z&d
> > at
> > >
> > a=05%7C01%7Chongxing.zhu%40nxp.com%7C8dea46d908d34cb4825808da4
> > f270eaf%
> > >
> > 7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379093264156415
> > 35%7CUnkn
> > >
> > own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> > haWwi
> > >
> > LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=tqzlcCmtSkUNxNzJ0Fr5o
> > CR3X2CTH
> > > 4dwUJS4NXt5KRc%3D&reserved=0
> > > [email protected]
> > > Signed-off-by: Richard Zhu <[email protected]>
> > > Signed-off-by: Bjorn Helgaas <[email protected]>
> > > ---
> > > drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++------------
> > > 1 file changed, 7 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 6eddd0b5f628..537b8a2e0e3b 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -670,8 +670,6 @@ static void imx6_pcie_clk_disable(struct imx6_pcie
> > > *imx6_pcie)
> > >
> > > static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> > > {
> > > - struct device *dev = imx6_pcie->pci->dev;
> > > -
> > > switch (imx6_pcie->drvdata->variant) {
> > > case IMX7D:
> > > case IMX8MQ:
> > > @@ -702,14 +700,6 @@ static void imx6_pcie_assert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> > > break;
> > > }
> > >
> > > - if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> > > - int ret = regulator_disable(imx6_pcie->vpcie);
> > > -
> > > - if (ret)
> > > - dev_err(dev, "failed to disable vpcie regulator: %d\n",
> > > - ret);
> > > - }
> > > -
> > > /* Some boards don't have PCIe reset GPIO. */
> > > if (gpio_is_valid(imx6_pcie->reset_gpio))
> > > gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> > > @@ -722,7 +712,7 @@ static int imx6_pcie_deassert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> > > struct device *dev = pci->dev;
> > > int ret, err;
> > >
> > > - if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
> > > + if (imx6_pcie->vpcie) {
> > > ret = regulator_enable(imx6_pcie->vpcie);
> > > if (ret) {
> > > dev_err(dev, "failed to enable vpcie regulator: %d\n", @@
> > -796,7
> > > +786,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie
> > *imx6_pcie)
> > > return 0;
> > >
> > > err_clks:
> > > - if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> > > + if (imx6_pcie->vpcie) {
> > > ret = regulator_disable(imx6_pcie->vpcie);
> > > if (ret)
> > > dev_err(dev, "failed to disable vpcie regulator: %d\n", @@
> > -1023,6
> > > +1013,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> > > break;
> > > }
> > >
> > > + if (imx6_pcie->vpcie)
> > > + regulator_disable(imx6_pcie->vpcie);
> >
> > This is a little bit ugly because imx6_pcie_suspend_noirq() and
> > imx6_pcie_resume_noirq() are not symmetric.
> >
> > We call regulator_disable() directly here in imx6_pcie_suspend_noirq(), but the
> > corresponding regulator_enable() is buried in imx6_pcie_deassert_core_reset().
> >
> > It would be nicer if the suspend and resume paths looked more similar, as
> > rockchip_pcie_suspend_noirq() and rockchip_pcie_resume_noirq() do, for
> > example.
> Yes, it is. The regulator_disable()/regulator_enable() are not invoked
> symmetrically.
>
> In the original codes, the regulator_disable() is contained in
> imx6_pcie_assert_core_reset() and with _eanbled() check.
> And regulator_enable() is embedded in imx6_pcie_deassert_core_reset().
> Both the regulator disable and enable are invoked in resume before.
> I move the regulator_disable to suspend mode refer to Lucas' comments.
>
> Refer to the current situation, how about to move the regulator_disable()
> out of imx6_pcie_deassert_core_reset() too?
I think it's important to tweak this closer to the pattern of other
drivers like exynos and rockchip.
exynos_pcie_resume_noirq() calls exynos_pcie_host_init().
imx6_pcie_resume_noirq() is VERY similar to imx6_pcie_host_init(), and
I think it should call imx6_pcie_host_init() instead of duplicating
that code. The only real difference is that imx6_pcie_host_init()
calls imx6_setup_phy_mpll() while imx6_pcie_resume_noirq() does not.
I suspect that's a bug.
I don't think "regulator_enable(imx6_pcie->vpcie)" really has anything
to do with imx6_pcie_deassert_core_reset() and it should be moved out.
The typical pattern is to enable regulators in *_probe() and
*_resume_noirq() and disable them in *_remove() and *_suspend_noirq().
I think you should do the same unless there's some reason you can't.
Can you give this a try, starting with this v12 posting?
Bjorn
> -----Original Message-----
> From: Bjorn Helgaas <[email protected]>
> Sent: 2022??6??16?? 23:56
> To: Hongxing Zhu <[email protected]>
> Cc: Lucas Stach <[email protected]>; Rob Herring <[email protected]>;
> Mark Brown <[email protected]>; Lorenzo Pieralisi
> <[email protected]>; Fabio Estevam <[email protected]>;
> Francesco Dolcini <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>; Bjorn Helgaas
> <[email protected]>
> Subject: Re: [PATCH v12 09/13] PCI: imx6: Turn off regulator when system is in
> suspend mode
>
> On Thu, Jun 16, 2022 at 02:23:29AM +0000, Hongxing Zhu wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <[email protected]>
> > > Sent: 2022??6??16?? 7:31
> > > To: Hongxing Zhu <[email protected]>; Lucas Stach
> > > <[email protected]>; Rob Herring <[email protected]>; Mark
> > > Brown <[email protected]>; Lorenzo Pieralisi
> > > <[email protected]>; Fabio Estevam <[email protected]>;
> > > Francesco Dolcini <[email protected]>
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected]; dl-linux-imx
> > > <[email protected]>; Bjorn Helgaas <[email protected]>
> > > Subject: Re: [PATCH v12 09/13] PCI: imx6: Turn off regulator when
> > > system is in suspend mode
> > >
> > > On Wed, Jun 15, 2022 at 06:15:47PM -0500, Bjorn Helgaas wrote:
> > > > From: Richard Zhu <[email protected]>
> > > >
> > > > The driver should undo any enables it did itself. The regulator
> > > > disable shouldn't be basing decisions on regulator_is_enabled().
> > > >
> > > > Move the regulator_disable to the suspend function, turn off
> > > > regulator when the system is in suspend mode.
> > > >
> > > > To keep the balance of the regulator usage counter, disable the
> > > > regulator in shutdown.
> > > >
> > > > Link:
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > > lore
> > > > .kernel.org%2Fr%2F1655189942-12678-6-git-send-email-hongxing.z&am
> p
> > > > ;d
> > > at
> > > >
> > >
> a=05%7C01%7Chongxing.zhu%40nxp.com%7C8dea46d908d34cb4825808da4
> > > f270eaf%
> > > >
> > >
> 7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379093264156415
> > > 35%7CUnkn
> > > >
> > >
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> > > haWwi
> > > >
> > >
> LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=tqzlcCmtSkUNxNzJ0Fr5o
> > > CR3X2CTH
> > > > 4dwUJS4NXt5KRc%3D&reserved=0
> > > > [email protected]
> > > > Signed-off-by: Richard Zhu <[email protected]>
> > > > Signed-off-by: Bjorn Helgaas <[email protected]>
> > > > ---
> > > > drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++------------
> > > > 1 file changed, 7 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > > index 6eddd0b5f628..537b8a2e0e3b 100644
> > > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > > @@ -670,8 +670,6 @@ static void imx6_pcie_clk_disable(struct
> > > > imx6_pcie
> > > > *imx6_pcie)
> > > >
> > > > static void imx6_pcie_assert_core_reset(struct imx6_pcie
> > > > *imx6_pcie) {
> > > > - struct device *dev = imx6_pcie->pci->dev;
> > > > -
> > > > switch (imx6_pcie->drvdata->variant) {
> > > > case IMX7D:
> > > > case IMX8MQ:
> > > > @@ -702,14 +700,6 @@ static void
> > > > imx6_pcie_assert_core_reset(struct
> > > imx6_pcie *imx6_pcie)
> > > > break;
> > > > }
> > > >
> > > > - if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> {
> > > > - int ret = regulator_disable(imx6_pcie->vpcie);
> > > > -
> > > > - if (ret)
> > > > - dev_err(dev, "failed to disable vpcie regulator: %d\n",
> > > > - ret);
> > > > - }
> > > > -
> > > > /* Some boards don't have PCIe reset GPIO. */
> > > > if (gpio_is_valid(imx6_pcie->reset_gpio))
> > > > gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> > > > @@ -722,7 +712,7 @@ static int
> > > > imx6_pcie_deassert_core_reset(struct
> > > imx6_pcie *imx6_pcie)
> > > > struct device *dev = pci->dev;
> > > > int ret, err;
> > > >
> > > > - if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
> > > > + if (imx6_pcie->vpcie) {
> > > > ret = regulator_enable(imx6_pcie->vpcie);
> > > > if (ret) {
> > > > dev_err(dev, "failed to enable vpcie regulator: %d\n", @@
> > > -796,7
> > > > +786,7 @@ static int imx6_pcie_deassert_core_reset(struct
> > > > +imx6_pcie
> > > *imx6_pcie)
> > > > return 0;
> > > >
> > > > err_clks:
> > > > - if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> {
> > > > + if (imx6_pcie->vpcie) {
> > > > ret = regulator_disable(imx6_pcie->vpcie);
> > > > if (ret)
> > > > dev_err(dev, "failed to disable vpcie regulator: %d\n", @@
> > > -1023,6
> > > > +1013,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> > > > break;
> > > > }
> > > >
> > > > + if (imx6_pcie->vpcie)
> > > > + regulator_disable(imx6_pcie->vpcie);
> > >
> > > This is a little bit ugly because imx6_pcie_suspend_noirq() and
> > > imx6_pcie_resume_noirq() are not symmetric.
> > >
> > > We call regulator_disable() directly here in
> > > imx6_pcie_suspend_noirq(), but the corresponding regulator_enable() is
> buried in imx6_pcie_deassert_core_reset().
> > >
> > > It would be nicer if the suspend and resume paths looked more
> > > similar, as
> > > rockchip_pcie_suspend_noirq() and rockchip_pcie_resume_noirq() do,
> > > for example.
> > Yes, it is. The regulator_disable()/regulator_enable() are not invoked
> > symmetrically.
> >
> > In the original codes, the regulator_disable() is contained in
> > imx6_pcie_assert_core_reset() and with _eanbled() check.
> > And regulator_enable() is embedded in imx6_pcie_deassert_core_reset().
> > Both the regulator disable and enable are invoked in resume before.
> > I move the regulator_disable to suspend mode refer to Lucas' comments.
> >
> > Refer to the current situation, how about to move the
> > regulator_disable() out of imx6_pcie_deassert_core_reset() too?
>
> I think it's important to tweak this closer to the pattern of other drivers like
> exynos and rockchip.
>
> exynos_pcie_resume_noirq() calls exynos_pcie_host_init().
> imx6_pcie_resume_noirq() is VERY similar to imx6_pcie_host_init(), and I think
> it should call imx6_pcie_host_init() instead of duplicating that code. The only
> real difference is that imx6_pcie_host_init() calls imx6_setup_phy_mpll() while
> imx6_pcie_resume_noirq() does not.
> I suspect that's a bug.
>
> I don't think "regulator_enable(imx6_pcie->vpcie)" really has anything to do
> with imx6_pcie_deassert_core_reset() and it should be moved out.
> The typical pattern is to enable regulators in *_probe() and
> *_resume_noirq() and disable them in *_remove() and *_suspend_noirq().
> I think you should do the same unless there's some reason you can't.
>
> Can you give this a try, starting with this v12 posting?
Thanks a lot for your inspiration.
Exactly, these codes are almost same. The host_init can be invoked directly
in resume. I would re-change the #12/13 of v12, and send the v13 later.
Best Regards
Richard Zhu
>
> Bjorn