Re: [PATCH v8 5/8] PCI: imx6: Refine the regulator usage
From: Francesco Dolcini <hidden>
Date: 2022-02-25 09:43:11
Also in:
linux-pci, lkml
On Fri, Feb 25, 2022 at 11:44:24AM +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(). To keep the balance of the regulator usage counter, disable the regulator just behind of imx6_pcie_assert_core_reset() in resume and shutdown.
While this patch does not look wrong and improving the existing code, I do have a couple of concern. 1. It's weird that regulator enable/disable are really non symmetric in the code, enable is in imx6_pcie_deassert_core_reset(), while disable is not in imx6_pcie_assert_core_reset() but spread around. 2. We are playing around with vpcie during regulator during suspend/resume, why? We currently have issue with suspend/resume when a PCIe switch is there, with the final effect of PCIe being not usable afterward. I fear like this is related to our issue. What about vpcie enable in probe, and vpcie disable in shutdown and that's all? Francesco
quoted hunk ↗ jump to hunk
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.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 0f1b8c873a4a..160a0bd02098 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/dwc/pci-imx6.c@@ -369,8 +369,6 @@ static int imx6_pcie_attach_pd(struct device *dev) 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:@@ -400,14 +398,6 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie) IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16); 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); - } } static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)@@ -584,7 +574,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",@@ -657,7 +647,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",@@ -1029,6 +1019,9 @@ static int imx6_pcie_resume_noirq(struct device *dev) return 0; imx6_pcie_assert_core_reset(imx6_pcie); + if (imx6_pcie->vpcie) + regulator_disable(imx6_pcie->vpcie); + imx6_pcie_init_phy(imx6_pcie); imx6_pcie_deassert_core_reset(imx6_pcie); dw_pcie_setup_rc(pp);@@ -1262,6 +1255,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 linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel