RE: [PATCH v13 10/15] PCI: imx6: Turn off regulator when system is in suspend mode

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: 2022年6月24日 6:20
> To: Hongxing Zhu <hongxing.zhu@xxxxxxx>
> Cc: l.stach@xxxxxxxxxxxxxx; bhelgaas@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
> broonie@xxxxxxxxxx; lorenzo.pieralisi@xxxxxxx; festevam@xxxxxxxxx;
> francesco.dolcini@xxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> kernel@xxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>
> Subject: Re: [PATCH v13 10/15] PCI: imx6: Turn off regulator when system is in
> suspend mode
> 
> On Fri, Jun 17, 2022 at 06:31:09PM +0800, Richard Zhu wrote:
> > 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&amp;d
> at
> >
> a=05%7C01%7Chongxing.zhu%40nxp.com%7C5633fa1bf3c443e203e108da55
> 667dc2%
> >
> 7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379161959277276
> 04%7CUnkn
> >
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> haWwi
> >
> LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=1Kbzn3XSVvt3gGPrEy%2
> BET8EZn4I
> > dwS%2BhUZ3AalZ2YZ0%3D&amp;reserved=0
> > hu@xxxxxxx
> > Signed-off-by: Richard Zhu <hongxing.zhu@xxxxxxx>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > ---
> >  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 2b42c37f1617..f72eb609769b 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;
> >
> > -	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", @@
> -795,7
> > +785,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", @@
> -1022,6
> > +1012,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> >  		break;
> >  	}
> >
> > +	if (imx6_pcie->vpcie)
> > +		regulator_disable(imx6_pcie->vpcie);
> > +
> >  	return 0;
> >  }
> 
> The suspend and resume methods should be symmetric, and they should
> *look* symmetric.
> 
> imx6_pcie_suspend_noirq() disables the regulator, so
> imx6_pcie_resume_noirq() should enable it.
> 
> imx6_pcie_suspend_noirq() calls imx6_pcie_clk_disable() to disable several
> clocks.  imx6_pcie_resume_noirq() should call
> imx6_pcie_clk_enable() to enable them.
> 
> imx6_pcie_clk_enable() *is* called in the resume path, but it's buried inside
> imx6_pcie_host_init() and imx6_pcie_deassert_core_reset().
> That makes it hard to analyze.
> 
> We should be able to look at imx6_pcie_suspend_noirq() and
> imx6_pcie_resume_noirq() and easily see that the resume path resumes
> everything that was suspended in the suspend path.
Hi Bjorn:
Thanks for your kindly help to review it.
Yes, it is. It's better to keep suspend/resume symmetric as much as possible.
In resume, the host_init is invoked, clocks, regulators and so on would be
initialized properly. 
Unfortunately, there is no according host_exit() that can be called to do the
reversed clocks, regulators disable operations in the suspend.
So, the clocks and regulator disable are explicitly invoked in suspend callback.

How about to do the incremental updates if the .host_exit can be added later?

Best Regards
Richard Zhu
> 
> Bjorn