| Message ID | 1656645935-1370-12-git-send-email-hongxing.zhu@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: imx6: refine codes and add the error propagation | expand |
Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu: > Move regulator enable out of imx6_pcie_deassert_core_reset(), since the > regulator_enable() has nothing to do in with > imx6_pcie_deassert_core_reset(). > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> Ah, so you are doing things in two steps. Disregard my first comment on the last patch then. > --- > drivers/pci/controller/dwc/pci-imx6.c | 36 ++++++++++++--------------- > 1 file changed, 16 insertions(+), 20 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index f72eb609769b..0b168f0d57b8 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -712,19 +712,10 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) > struct device *dev = pci->dev; > 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; > - } > - } > - > ret = imx6_pcie_clk_enable(imx6_pcie); > if (ret) { > dev_err(dev, "unable to enable pcie clocks: %d\n", ret); > - goto err_clks; > + return ret; > } > > switch (imx6_pcie->drvdata->variant) { > @@ -783,15 +774,6 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) > } > > return 0; > - > -err_clks: > - if (imx6_pcie->vpcie) { > - ret = regulator_disable(imx6_pcie->vpcie); > - if (ret) > - dev_err(dev, "failed to disable vpcie regulator: %d\n", > - ret); > - } > - return ret; > } > > static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie) > @@ -916,15 +898,29 @@ static int imx6_pcie_host_init(struct pcie_port *pp) > > imx6_pcie_assert_core_reset(imx6_pcie); > imx6_pcie_init_phy(imx6_pcie); > + 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; If the regulator enable fails, you don't roll back the PHY init and core reset. This seems harmless now, but might have unintended consequences if the PHY code changes. I think it should be safe to move the regulator enable before the PHY init and core reset assert to avoid introducing more failure cleanup paths here. Regards, Lucas > + } > + } > + > ret = imx6_pcie_deassert_core_reset(imx6_pcie); > if (ret < 0) { > dev_err(dev, "pcie deassert core reset failed: %d\n", ret); > - return ret; > + goto err_reg_disable; > } > > imx6_setup_phy_mpll(imx6_pcie); > > return 0; > + > +err_reg_disable: > + if (imx6_pcie->vpcie) > + regulator_disable(imx6_pcie->vpcie); > + return ret; > } > > static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
> -----Original Message----- > From: Lucas Stach <l.stach@pengutronix.de> > Sent: 2022年7月13日 16:41 > To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com; > robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com; > festevam@gmail.com; francesco.dolcini@toradex.com > Cc: 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 v14 11/17] PCI: imx6: Move regulator enable out of > imx6_pcie_deassert_core_reset() > > Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu: > > Move regulator enable out of imx6_pcie_deassert_core_reset(), since > > the > > regulator_enable() has nothing to do in with > > imx6_pcie_deassert_core_reset(). > > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > > Ah, so you are doing things in two steps. Disregard my first comment on the > last patch then. Thanks. > > > --- > > drivers/pci/controller/dwc/pci-imx6.c | 36 > > ++++++++++++--------------- > > 1 file changed, 16 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c > > b/drivers/pci/controller/dwc/pci-imx6.c > > index f72eb609769b..0b168f0d57b8 100644 > > --- a/drivers/pci/controller/dwc/pci-imx6.c > > +++ b/drivers/pci/controller/dwc/pci-imx6.c > > @@ -712,19 +712,10 @@ static int imx6_pcie_deassert_core_reset(struct > imx6_pcie *imx6_pcie) > > struct device *dev = pci->dev; > > 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; > > - } > > - } > > - > > ret = imx6_pcie_clk_enable(imx6_pcie); > > if (ret) { > > dev_err(dev, "unable to enable pcie clocks: %d\n", ret); > > - goto err_clks; > > + return ret; > > } > > > > switch (imx6_pcie->drvdata->variant) { @@ -783,15 +774,6 @@ static > > int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) > > } > > > > return 0; > > - > > -err_clks: > > - if (imx6_pcie->vpcie) { > > - ret = regulator_disable(imx6_pcie->vpcie); > > - if (ret) > > - dev_err(dev, "failed to disable vpcie regulator: %d\n", > > - ret); > > - } > > - return ret; > > } > > > > static int imx6_pcie_wait_for_speed_change(struct imx6_pcie > > *imx6_pcie) @@ -916,15 +898,29 @@ static int > > imx6_pcie_host_init(struct pcie_port *pp) > > > > imx6_pcie_assert_core_reset(imx6_pcie); > > imx6_pcie_init_phy(imx6_pcie); > > + 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; > > If the regulator enable fails, you don't roll back the PHY init and core reset. This > seems harmless now, but might have unintended consequences if the PHY > code changes. I think it should be safe to move the regulator enable before the > PHY init and core reset assert to avoid introducing more failure cleanup paths > here. To keep the same behavior, I just place the regulator_enable() in front of imx6_pcie_deassert_core_reset(). It makes sense to move it earlier to avoid the possible failure cleanup of imx6_pcie_assert_core_reset() and imx6_pcie_init_phy() in future. Best Regards Richard > > Regards, > Lucas > > > + } > > + } > > + > > ret = imx6_pcie_deassert_core_reset(imx6_pcie); > > if (ret < 0) { > > dev_err(dev, "pcie deassert core reset failed: %d\n", ret); > > - return ret; > > + goto err_reg_disable; > > } > > > > imx6_setup_phy_mpll(imx6_pcie); > > > > return 0; > > + > > +err_reg_disable: > > + if (imx6_pcie->vpcie) > > + regulator_disable(imx6_pcie->vpcie); > > + return ret; > > } > > > > static const struct dw_pcie_host_ops imx6_pcie_host_ops = { >
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index f72eb609769b..0b168f0d57b8 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/dwc/pci-imx6.c @@ -712,19 +712,10 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) struct device *dev = pci->dev; 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; - } - } - ret = imx6_pcie_clk_enable(imx6_pcie); if (ret) { dev_err(dev, "unable to enable pcie clocks: %d\n", ret); - goto err_clks; + return ret; } switch (imx6_pcie->drvdata->variant) { @@ -783,15 +774,6 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) } return 0; - -err_clks: - if (imx6_pcie->vpcie) { - ret = regulator_disable(imx6_pcie->vpcie); - if (ret) - dev_err(dev, "failed to disable vpcie regulator: %d\n", - ret); - } - return ret; } static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie) @@ -916,15 +898,29 @@ static int imx6_pcie_host_init(struct pcie_port *pp) imx6_pcie_assert_core_reset(imx6_pcie); imx6_pcie_init_phy(imx6_pcie); + 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; + } + } + ret = imx6_pcie_deassert_core_reset(imx6_pcie); if (ret < 0) { dev_err(dev, "pcie deassert core reset failed: %d\n", ret); - return ret; + goto err_reg_disable; } imx6_setup_phy_mpll(imx6_pcie); return 0; + +err_reg_disable: + if (imx6_pcie->vpcie) + regulator_disable(imx6_pcie->vpcie); + return ret; } static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
Move regulator enable out of imx6_pcie_deassert_core_reset(), since the regulator_enable() has nothing to do in with imx6_pcie_deassert_core_reset(). Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> --- drivers/pci/controller/dwc/pci-imx6.c | 36 ++++++++++++--------------- 1 file changed, 16 insertions(+), 20 deletions(-)