> -----Original Message-----
> From: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> Sent: Wednesday, September 8, 2021 4:42 PM
> To: Richard Zhu <hongxing.zhu@xxxxxxx>; bhelgaas@xxxxxxxxxx;
> lorenzo.pieralisi@xxxxxxx
> Cc: linux-pci@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> kernel@xxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/3] PCI: imx: add err check to host init and fix regulator
> dump
>
> Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu:
> > Since there is error return check of the host_init callback, add error
> > check to imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> > function.
> >
> > Because that i.MX PCIe doesn't support the hot-plug feature. To save
> > power consumption as much as possible, turn off the clocks and power
> > supplies when the PCIe PHY link is never came up in probe procedure.
> >
> > When PCIe link is never came up and vpcie regulator is present, there
> > would be following dump when try to put the regulator.
> > Disable this regulator to fix this dump when link is never came up.
> >
> > [ 2.335880] imx6q-pcie 33800000.pcie: Phy link never came up
> > [ 2.341642] imx6q-pcie: probe of 33800000.pcie failed with error -110
> > [ 2.348160] ------------[ cut here ]------------
> > [ 2.352778] WARNING: CPU: 3 PID: 119 at
> drivers/regulator/core.c:2256 _regulator_put.part.0+0x14c/0x158
> > [ 2.362184] Modules linked in:
> > [ 2.365243] CPU: 3 PID: 119 Comm: kworker/u8:2 Not tainted
> 5.13.0-rc7-next-20210625-94710-ge4e92b2588a3 #10
> > [ 2.374987] Hardware name: FSL i.MX8MM EVK board (DT)
> > [ 2.380040] Workqueue: events_unbound async_run_entry_fn
> > [ 2.385359] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> > [ 2.391369] pc : _regulator_put.part.0+0x14c/0x158
> > [ 2.396163] lr : regulator_put+0x34/0x48
> > [ 2.400088] sp : ffff8000122ebb30
> > [ 2.403400] x29: ffff8000122ebb30 x28: ffff800011be7000 x27:
> 0000000000000000
> > [ 2.410546] x26: 0000000000000000 x25: 0000000000000000 x24:
> ffff00000025f2bc
> > [ 2.417689] x23: ffff00000025f2c0 x22: ffff00000025f010 x21:
> ffff8000122ebc18
> > [ 2.424834] x20: ffff800011e3fa60 x19: ffff00000375fd80 x18:
> 0000000000000010
> > [ 2.431979] x17: 000000040044ffff x16: 00400032b5503510 x15:
> 0000000000000108
> > [ 2.439124] x14: ffff0000003cc938 x13: 00000000ffffffea x12:
> 0000000000000000
> > [ 2.446267] x11: 0000000000000000 x10: ffff80001076ba88 x9 :
> ffff80001076a540
> > [ 2.453411] x8 : ffff00000025f2c0 x7 : ffff0000001f4450 x6 :
> ffff000000176cd8
> > [ 2.460556] x5 : ffff000003857880 x4 : 0000000000000000 x3 :
> ffff800011e3fe30
> > [ 2.467700] x2 : ffff0000003cc4c0 x1 : 0000000000000000 x0 :
> 0000000000000001
> > [ 2.474847] Call trace:
> > [ 2.477295] _regulator_put.part.0+0x14c/0x158
> > [ 2.481742] regulator_put+0x34/0x48
> > [ 2.485322] devm_regulator_release+0x10/0x18
> > [ 2.489681] release_nodes+0x38/0x60
> > [ 2.493262] devres_release_all+0x88/0xd0
> > [ 2.497276] really_probe+0xd0/0x2e8
> > [ 2.500858] __driver_probe_device+0x74/0xd8
> > [ 2.505137] driver_probe_device+0x7c/0x108
> > [ 2.509325] __device_attach_driver+0x8c/0xd0
> > [ 2.513685] bus_for_each_drv+0x74/0xc0
> > [ 2.517531] __device_attach_async_helper+0xb4/0xd8
> > [ 2.522419] async_run_entry_fn+0x30/0x100
> > [ 2.526521] process_one_work+0x19c/0x320
> > [ 2.530532] worker_thread+0x48/0x418
> > [ 2.534199] kthread+0x14c/0x158
> > [ 2.537432] ret_from_fork+0x10/0x18
> > [ 2.541013] ---[ end trace 3664ca4a50ce849b ]---
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@xxxxxxx>
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 28
> > +++++++++++++++++++--------
> > 1 file changed, 20 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 0264432e4c4a..129928e42f84 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -144,6 +144,7 @@ struct imx6_pcie {
> > #define PHY_RX_OVRD_IN_LO_RX_PLL_EN BIT(3)
> >
> > static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
> > +static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie);
> >
> > static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool
> > exp_val) { @@ -485,24 +486,24 @@ static void
> > imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
> > dev_err(dev, "PCIe PLL lock timeout\n"); }
> >
> > -static void imx6_pcie_deassert_core_reset(struct imx6_pcie
> > *imx6_pcie)
> > +static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> > {
> > struct dw_pcie *pci = imx6_pcie->pci;
> > struct device *dev = pci->dev;
> > - int ret;
> > + int ret, err;
>
> Why do we need a separate variable for the error code here? Why not just
> use the existing ret?
[Richard Zhu] Thanks for your review.
The "ret" variable might be assigned to one "0" by regulator_disable() in the error exit.
So, I use another one "err" variable to avoid this confliction.
How about do not make the err check of regulator_enable/disable, and just use the "ret" for the error code?
if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
ret = regulator_disable(imx6_pcie->vpcie);
if (ret)
dev_err(dev, "failed to disable vpcie regulator: %d\n",
ret);
}
>
> >
> > if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
> > ret = regulator_enable(imx6_pcie->vpcie);
> > if (ret) {
> > dev_err(dev, "failed to enable vpcie regulator: %d\n",
> > ret);
> > - return;
> > + return ret;
> > }
> > }
> >
> > - ret = imx6_pcie_clk_enable(imx6_pcie);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie clocks\n");
> > + err = imx6_pcie_clk_enable(imx6_pcie);
> > + if (err) {
> > + dev_err(dev, "unable to enable pcie clocks: %d\n", err);
> > goto err_clks;
> > }
> >
> > @@ -557,7 +558,7 @@ static void imx6_pcie_deassert_core_reset(struct
> imx6_pcie *imx6_pcie)
> > break;
> > }
> >
> > - return;
> > + return 0;
> >
> > err_clks:
> > if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> > { @@ -566,6 +567,7 @@ static void imx6_pcie_deassert_core_reset(struct
> imx6_pcie *imx6_pcie)
> > dev_err(dev, "failed to disable vpcie regulator: %d\n",
> > ret);
> > }
> > + return err;
> > }
> >
> > static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie) @@
> > -810,17 +812,27 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
> > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> > imx6_pcie_reset_phy(imx6_pcie);
> > + imx6_pcie_clk_disable(imx6_pcie);
>
> This is a separate fix for the clock enable counts, that isn't mentioned in the
> commit message.
>
> It seems like this patch fixes in fact 3 issues: error propagation from host_init,
> fix regulator reference handling, fix clk reference handling. Either split up the
> patch along those changes (if possible) or improve the commit message to
> explain those changes.
[Richard Zhu] My original purpose of this patch is to turn off the clocks
and power supplies( regulators and so on.) when link is down(for example, no EP device connected).
So, I combine the fix of the clock/regulator used counter balance together.
Would improve the commit message to explain these changes later.
You're right, the error propagation from host_init should be separated to another patch.
Thanks a lot 😊.
BR
Richard
>
> Regards,
> Lucas
>
> > + if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> > + regulator_disable(imx6_pcie->vpcie);
> > return ret;
> > }
> >
> > static int imx6_pcie_host_init(struct pcie_port *pp) {
> > struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct device *dev = pci->dev;
> > struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
> > + int ret;
> >
> > imx6_pcie_assert_core_reset(imx6_pcie);
> > imx6_pcie_init_phy(imx6_pcie);
> > - imx6_pcie_deassert_core_reset(imx6_pcie);
> > + ret = imx6_pcie_deassert_core_reset(imx6_pcie);
> > + if (ret < 0) {
> > + dev_err(dev, "pcie host init failed: %d.\n", ret);
> > + return ret;
> > + }
> > +
> > imx6_setup_phy_mpll(imx6_pcie);
> >
> > return 0;
>
