| Message ID | 1655461874-16908-11-git-send-email-hongxing.zhu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: imx6: refine codes and add the error propagation | expand |
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://lore.kernel.org/r/1655189942-12678-6-git-send-email-hongxing.z > hu@nxp.com > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > 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. Bjorn
> -----Original Message----- > From: Bjorn Helgaas <helgaas@kernel.org> > Sent: 2022年6月24日 6:20 > To: Hongxing Zhu <hongxing.zhu@nxp.com> > Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org; > broonie@kernel.org; lorenzo.pieralisi@arm.com; festevam@gmail.com; > francesco.dolcini@toradex.com; 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 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&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&sdata=1Kbzn3XSVvt3gGPrEy%2 > BET8EZn4I > > dwS%2BhUZ3AalZ2YZ0%3D&reserved=0 > > hu@nxp.com > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > --- > > 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
On Fri, Jun 24, 2022 at 05:05:00AM +0000, Hongxing Zhu wrote: > > -----Original Message----- > > From: Bjorn Helgaas <helgaas@kernel.org> > > Sent: 2022年6月24日 6:20 > > To: Hongxing Zhu <hongxing.zhu@nxp.com> > > Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org; > > broonie@kernel.org; lorenzo.pieralisi@arm.com; festevam@gmail.com; > > francesco.dolcini@toradex.com; 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 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&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&sdata=1Kbzn3XSVvt3gGPrEy%2 > > BET8EZn4I > > > dwS%2BhUZ3AalZ2YZ0%3D&reserved=0 > > > hu@nxp.com > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > > --- > > > 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. > > 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? This doesn't seem very convincing because everything here is in the imx6 domain. The only DWC core thing here is the dw_pcie_setup_rc() called in imx6_pcie_resume_noirq(), and it doesn't call back to any imx6 code. So you should be able to make an imx6_pcie_host_exit() or whatever that corresponds to imx6_pcie_host_init().
> -----Original Message----- > From: Bjorn Helgaas <helgaas@kernel.org> > Sent: 2022年6月28日 3:52 > To: Hongxing Zhu <hongxing.zhu@nxp.com> > Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org; > broonie@kernel.org; lorenzo.pieralisi@arm.com; festevam@gmail.com; > francesco.dolcini@toradex.com; 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 v13 10/15] PCI: imx6: Turn off regulator when system is in > suspend mode > > On Fri, Jun 24, 2022 at 05:05:00AM +0000, Hongxing Zhu wrote: > > > -----Original Message----- > > > From: Bjorn Helgaas <helgaas@kernel.org> > > > Sent: 2022年6月24日 6:20 > > > To: Hongxing Zhu <hongxing.zhu@nxp.com> > > > Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org; > > > broonie@kernel.org; lorenzo.pieralisi@arm.com; festevam@gmail.com; > > > francesco.dolcini@toradex.com; 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 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%2F > > > > lore > > > > .kernel.org%2Fr%2F1655189942-12678-6-git-send-email-hongxing.z&am > p > > > > ;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&sdata=1Kbzn3XSVvt3gGPrEy%2 > > > BET8EZn4I > > > > dwS%2BhUZ3AalZ2YZ0%3D&reserved=0 > > > > hu@nxp.com > > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > > > --- > > > > 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. > > > > 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? > > This doesn't seem very convincing because everything here is in the > imx6 domain. The only DWC core thing here is the dw_pcie_setup_rc() called > in imx6_pcie_resume_noirq(), and it doesn't call back to any > imx6 code. > > So you should be able to make an imx6_pcie_host_exit() or whatever that > corresponds to imx6_pcie_host_init(). Hi Bjorn: Thanks for your kindly help to review it. That's reasonable. So, to make it symmetric with imx6_pcie_host_init() and imx6_pcie_start_link(). The according local functions imx6_pcie_host_exit() and imx6_pcie_stop_link() would be created. BTW, to be symmetric with imx6_pcie_host_init(), the parameter of imx6_pcie_host_exit() is same to the parameter of imx6_pcie_host_init(). So do imx6_pcie_stop_link() and imx6_pcie_start_link(). Are you satisfied with the following functions? static void imx6_pcie_stop_link(struct dw_pcie *pci) { struct device *dev = pci->dev; /* Turn off PCIe LTSSM */ imx6_pcie_ltssm_disable(dev); } static void imx6_pcie_host_exit(struct pcie_port *pp) { struct dw_pcie *pci = to_dw_pcie_from_pp(pp); struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci); imx6_pcie_clk_disable(imx6_pcie); if (imx6_pcie->phy) { if (phy_power_off(imx6_pcie->phy)) dev_err(pci->dev, "unable to power off PHY\n"); phy_exit(imx6_pcie->phy); } if (imx6_pcie->vpcie) regulator_disable(imx6_pcie->vpcie); } Best Regards Richard Zhu
On Tue, Jun 28, 2022 at 03:48:01AM +0000, Hongxing Zhu wrote: > > -----Original Message----- > > From: Bjorn Helgaas <helgaas@kernel.org> > > Sent: 2022年6月28日 3:52 > > To: Hongxing Zhu <hongxing.zhu@nxp.com> > > Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org; > > broonie@kernel.org; lorenzo.pieralisi@arm.com; festevam@gmail.com; > > francesco.dolcini@toradex.com; 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 v13 10/15] PCI: imx6: Turn off regulator when system is in > > suspend mode > > > > On Fri, Jun 24, 2022 at 05:05:00AM +0000, Hongxing Zhu wrote: > > > > -----Original Message----- > > > > From: Bjorn Helgaas <helgaas@kernel.org> > > > > Sent: 2022年6月24日 6:20 > > > > To: Hongxing Zhu <hongxing.zhu@nxp.com> > > > > Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org; > > > > broonie@kernel.org; lorenzo.pieralisi@arm.com; festevam@gmail.com; > > > > francesco.dolcini@toradex.com; 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 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%2F > > > > > lore > > > > > .kernel.org%2Fr%2F1655189942-12678-6-git-send-email-hongxing.z&am > > p > > > > > ;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&sdata=1Kbzn3XSVvt3gGPrEy%2 > > > > BET8EZn4I > > > > > dwS%2BhUZ3AalZ2YZ0%3D&reserved=0 > > > > > hu@nxp.com > > > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > > > > --- > > > > > 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. > > > > > > 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? > > > > This doesn't seem very convincing because everything here is in the > > imx6 domain. The only DWC core thing here is the dw_pcie_setup_rc() called > > in imx6_pcie_resume_noirq(), and it doesn't call back to any > > imx6 code. > > > > So you should be able to make an imx6_pcie_host_exit() or whatever that > > corresponds to imx6_pcie_host_init(). > > Thanks for your kindly help to review it. That's reasonable. > > So, to make it symmetric with imx6_pcie_host_init() and > imx6_pcie_start_link(). The according local functions > imx6_pcie_host_exit() and imx6_pcie_stop_link() would be created. > > BTW, to be symmetric with imx6_pcie_host_init(), the parameter of > imx6_pcie_host_exit() is same to the parameter of > imx6_pcie_host_init(). So do imx6_pcie_stop_link() and > imx6_pcie_start_link(). Are you satisfied with the following > functions? > > static void imx6_pcie_stop_link(struct dw_pcie *pci) > { > struct device *dev = pci->dev; > > /* Turn off PCIe LTSSM */ > imx6_pcie_ltssm_disable(dev); > } > > static void imx6_pcie_host_exit(struct pcie_port *pp) > { > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci); > > imx6_pcie_clk_disable(imx6_pcie); > if (imx6_pcie->phy) { > if (phy_power_off(imx6_pcie->phy)) > dev_err(pci->dev, "unable to power off PHY\n"); > phy_exit(imx6_pcie->phy); > } > > if (imx6_pcie->vpcie) > regulator_disable(imx6_pcie->vpcie); > } After the current series, imx6_pcie_host_init() looks like: imx6_pcie_host_init phy_power_on regulator_enable imx6_pcie_deassert_core_reset imx6_pcie_clk_enable phy_init and you propose: imx6_pcie_host_exit imx6_pcie_clk_disable phy_power_off phy_exit regulator_disable Generally they should do things in the reverse order. imx6_pcie_host_init() does phy_power_on(), regulator_enable(), imx6_pcie_clk_enable(). imx6_pcie_host_exit() should do imx6_pcie_clk_disable(), regulator_disable(), phy_power_off(). (It looks like imx6_pcie_host_init() calls phy_power_on() and phy_init() in the wrong order [1].) IMO the imx6_pcie_clk_enable() should not be hidden inside imx6_pcie_deassert_core_reset(). [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/phy-core.c?id=v5.19-rc1#n233
> -----Original Message----- > From: Bjorn Helgaas <helgaas@kernel.org> > Sent: 2022年6月28日 23:51 > To: Hongxing Zhu <hongxing.zhu@nxp.com> > Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org; > broonie@kernel.org; lorenzo.pieralisi@arm.com; festevam@gmail.com; > francesco.dolcini@toradex.com; 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 v13 10/15] PCI: imx6: Turn off regulator when system is in > suspend mode > > On Tue, Jun 28, 2022 at 03:48:01AM +0000, Hongxing Zhu wrote: > > > -----Original Message----- > > > From: Bjorn Helgaas <helgaas@kernel.org> > > > Sent: 2022年6月28日 3:52 > > > To: Hongxing Zhu <hongxing.zhu@nxp.com> > > > Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org; > > > broonie@kernel.org; lorenzo.pieralisi@arm.com; festevam@gmail.com; > > > francesco.dolcini@toradex.com; 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 v13 10/15] PCI: imx6: Turn off regulator when > > > system is in suspend mode > > > > > > On Fri, Jun 24, 2022 at 05:05:00AM +0000, Hongxing Zhu wrote: > > > > > -----Original Message----- > > > > > From: Bjorn Helgaas <helgaas@kernel.org> > > > > > Sent: 2022年6月24日 6:20 > > > > > To: Hongxing Zhu <hongxing.zhu@nxp.com> > > > > > Cc: l.stach@pengutronix.de; bhelgaas@google.com; > > > > > robh+dt@kernel.org; broonie@kernel.org; > > > > > lorenzo.pieralisi@arm.com; festevam@gmail.com; > > > > > francesco.dolcini@toradex.com; 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 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%2 > > > > > > F%2F > > > > > > lore > > > > > > .kernel.org%2Fr%2F1655189942-12678-6-git-send-email-hongxing.z > > > > > > &am > > > p > > > > > > ;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&sdata=1Kbzn3XSVvt3gGPrEy%2 > > > > > BET8EZn4I > > > > > > dwS%2BhUZ3AalZ2YZ0%3D&reserved=0 > > > > > > hu@nxp.com > > > > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > > > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > > > > > --- > > > > > > 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. > > > > > > > > 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? > > > > > > This doesn't seem very convincing because everything here is in the > > > imx6 domain. The only DWC core thing here is the dw_pcie_setup_rc() > > > called in imx6_pcie_resume_noirq(), and it doesn't call back to any > > > imx6 code. > > > > > > So you should be able to make an imx6_pcie_host_exit() or whatever > > > that corresponds to imx6_pcie_host_init(). > > > > Thanks for your kindly help to review it. That's reasonable. > > > > So, to make it symmetric with imx6_pcie_host_init() and > > imx6_pcie_start_link(). The according local functions > > imx6_pcie_host_exit() and imx6_pcie_stop_link() would be created. > > > > BTW, to be symmetric with imx6_pcie_host_init(), the parameter of > > imx6_pcie_host_exit() is same to the parameter of > > imx6_pcie_host_init(). So do imx6_pcie_stop_link() and > > imx6_pcie_start_link(). Are you satisfied with the following > > functions? > > > > static void imx6_pcie_stop_link(struct dw_pcie *pci) { > > struct device *dev = pci->dev; > > > > /* Turn off PCIe LTSSM */ > > imx6_pcie_ltssm_disable(dev); > > } > > > > static void imx6_pcie_host_exit(struct pcie_port *pp) { > > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > > struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci); > > > > imx6_pcie_clk_disable(imx6_pcie); > > if (imx6_pcie->phy) { > > if (phy_power_off(imx6_pcie->phy)) > > dev_err(pci->dev, "unable to power off PHY\n"); > > phy_exit(imx6_pcie->phy); > > } > > > > if (imx6_pcie->vpcie) > > regulator_disable(imx6_pcie->vpcie); > > } > > After the current series, imx6_pcie_host_init() looks like: > > imx6_pcie_host_init > phy_power_on > regulator_enable > imx6_pcie_deassert_core_reset > imx6_pcie_clk_enable > phy_init > > and you propose: > > imx6_pcie_host_exit > imx6_pcie_clk_disable > phy_power_off > phy_exit > regulator_disable > > Generally they should do things in the reverse order. > Hi Bjorn: Thanks a lot for your review. > imx6_pcie_host_init() does phy_power_on(), regulator_enable(), > imx6_pcie_clk_enable(). > > imx6_pcie_host_exit() should do imx6_pcie_clk_disable(), regulator_disable(), > phy_power_off(). > > (It looks like imx6_pcie_host_init() calls phy_power_on() and > phy_init() in the wrong order [1].) Yes, it is . I notice the warning too in my local tests. I made a mistake that I assumed the PHY should be powered on firstly, then be initialized. Since it is bug fix, how about to issue another fix commit after this series? > > IMO the imx6_pcie_clk_enable() should not be hidden inside > imx6_pcie_deassert_core_reset(). Okay, would move the imx6_pcie_clk_enable() from imx6_pcie_deassert_core_reset() to imx6_pcie_host_init(). Since the 6/15 of v13 has already Lucas' reviewed-by tag. Can I combine these changes with the creation of the imx6_pcie_host_exit() and imx6_pcie_host_stop_link() into one patch? > > [1] > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern > el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftr > ee%2Fdrivers%2Fphy%2Fphy-core.c%3Fid%3Dv5.19-rc1%23n233&data= > 05%7C01%7Chongxing.zhu%40nxp.com%7C0e7ce8e53ee0478aabd508da591 > e06f7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379202827 > 66167735%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi > V2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdat > a=BQQnn1ju47AGwfQW48%2Ba%2BnlLszha8P0QAynr4G3qeLM%3D&res > erved=0 Thanks a lot for your kindly reminder. Best Regards Richard
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; } @@ -1268,6 +1261,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[] = {