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

[Richard Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Saturday, October 16, 2021 2:50 AM
> To: Richard Zhu <hongxing.zhu@xxxxxxx>
> Cc: l.stach@xxxxxxxxxxxxxx; bhelgaas@xxxxxxxxxx;
> lorenzo.pieralisi@xxxxxxx; 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
> 
> On Fri, Oct 15, 2021 at 02:05:40PM +0800, Richard Zhu wrote:
> > 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.
> 
> Add blank line between paragraphs.
> 
> Can you please split this into two patches:
> 
>   1) the imx6_pcie_clk_disable() move
>   2) the actual fix
> 
> It's hard to tell exactly where the fix is when things are mixed together.
> 
[Richard Zhu] Okay, would split this patch into two patches later.
One is used for imx6_pcie_clk_disable() move, and no function changes.
The other one is the actual fix. Thanks.

Best Regards
Richard Zhu

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