RE: [PATCH 2/3] PCI: imx: add err check to host init and fix regulator dump

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----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;
> 





[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux