RE: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling unbalance when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> Sent: Saturday, October 16, 2021 2:25 AM
> 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: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling
> unbalance when link never came up
> 
> Am Freitag, dem 15.10.2021 um 14:05 +0800 schrieb Richard Zhu:
> > When link never came up, driver probe would be failed with error -110.
> > To keep usage counter balance of the clocks, disable the previous
> > enabled clocks when link is down.
> > Move definitions of the imx6_pcie_clk_disable() function to the proper
> > place. Because it wouldn't be used in imx6_pcie_suspend_noirq() only.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@xxxxxxx>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 47
> > ++++++++++++++-------------
> >  1 file changed, 24 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index cc837f8bf6d4..d6a5d99ffa52 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -514,6 +514,29 @@ static int imx6_pcie_clk_enable(struct imx6_pcie
> *imx6_pcie)
> >  	return ret;
> >  }
> >
> > +static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie) {
> > +	clk_disable_unprepare(imx6_pcie->pcie);
> > +	clk_disable_unprepare(imx6_pcie->pcie_phy);
> > +	clk_disable_unprepare(imx6_pcie->pcie_bus);
> > +
> > +	switch (imx6_pcie->drvdata->variant) {
> > +	case IMX6SX:
> > +		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> > +		break;
> > +	case IMX7D:
> > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > +				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> > +				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> > +		break;
> > +	case IMX8MQ:
> > +		clk_disable_unprepare(imx6_pcie->pcie_aux);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +}
> > +
> >  static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie
> > *imx6_pcie)  {
> >  	u32 val;
> > @@ -853,6 +876,7 @@ 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);
> 
> Same comment as with the previous patch. We should not cram in more error
> handling in the imx6_pcie_start_link function, but rather move out all the
> error handling to be after dw_pcie_host_init. Even the already existing phy
> reset here seems misplaced and should be moved out.
[Richard Zhu] About the clk disabled, please see my explains under the addressed comments in the #3 patch.
Agree to move the existing phy reset to the error handling after dw_pcie_host_init.
Thanks.
BR
Richard

> 
> Regards,
> Lucas
> 
> >  	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> >  		regulator_disable(imx6_pcie->vpcie);
> >  	return ret;
> > @@ -941,29 +965,6 @@ static void imx6_pcie_pm_turnoff(struct
> imx6_pcie *imx6_pcie)
> >  	usleep_range(1000, 10000);
> >  }
> >
> > -static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie) -{
> > -	clk_disable_unprepare(imx6_pcie->pcie);
> > -	clk_disable_unprepare(imx6_pcie->pcie_phy);
> > -	clk_disable_unprepare(imx6_pcie->pcie_bus);
> > -
> > -	switch (imx6_pcie->drvdata->variant) {
> > -	case IMX6SX:
> > -		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> > -		break;
> > -	case IMX7D:
> > -		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > -				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> > -				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> > -		break;
> > -	case IMX8MQ:
> > -		clk_disable_unprepare(imx6_pcie->pcie_aux);
> > -		break;
> > -	default:
> > -		break;
> > -	}
> > -}
> > -
> >  static int imx6_pcie_suspend_noirq(struct device *dev)  {
> >  	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
>