| Message ID | 1634277941-6672-5-git-send-email-hongxing.zhu@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: imx6: refine codes and add compliance tests mode support | expand |
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@nxp.com> > --- > 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. 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);
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. > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > --- > 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 >
On Fri, Oct 15, 2021 at 01:49:45PM -0500, Bjorn Helgaas wrote: > 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. > > -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; While you're at it, this "default: break;" thing is pointless. Normally it's better to just *move* something without changing it at all, but this is such a simple thing I think you could drop this at the same time as the move. > > - } > > -}
> -----Original Message----- > From: Lucas Stach <l.stach@pengutronix.de> > Sent: Saturday, October 16, 2021 2:25 AM > To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com; > lorenzo.pieralisi@arm.com > Cc: linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > kernel@pengutronix.de > 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@nxp.com> > > --- > > 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); >
> -----Original Message----- > From: Bjorn Helgaas <helgaas@kernel.org> > Sent: Saturday, October 16, 2021 2:50 AM > To: Richard Zhu <hongxing.zhu@nxp.com> > Cc: l.stach@pengutronix.de; bhelgaas@google.com; > lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; kernel@pengutronix.de > 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@nxp.com> > > --- > > 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 > >
> -----Original Message----- > From: Bjorn Helgaas <helgaas@kernel.org> > Sent: Saturday, October 16, 2021 2:52 AM > To: Richard Zhu <hongxing.zhu@nxp.com> > Cc: l.stach@pengutronix.de; bhelgaas@google.com; > lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; kernel@pengutronix.de > 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 01:49:45PM -0500, Bjorn Helgaas wrote: > > 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. > > > > -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; > > While you're at it, this "default: break;" thing is pointless. > Normally it's better to just *move* something without changing it at all, but > this is such a simple thing I think you could drop this at the same time as the > move. > [Richard Zhu] Okay, got that. Would remove the "default:break" later. Thanks. Best Regards Richard Zhu > > > - } > > > -}
<snipped ...> > > > > > > -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; > > > > While you're at it, this "default: break;" thing is pointless. > > Normally it's better to just *move* something without changing it at > > all, but this is such a simple thing I think you could drop this at > > the same time as the move. > > > [Richard Zhu] Okay, got that. Would remove the "default:break" later. Thanks. [Richard Zhu] I figure out that the default:break is required by IMX6Q/IMX6QP. So I just don't drop them in v3 patch-set. > > Best Regards > Richard Zhu > > > > > - } > > > > -}
Hi, [...] > > > > > - default: > > > > > - break; > > > > > > While you're at it, this "default: break;" thing is pointless. > > > Normally it's better to just *move* something without changing it at > > > all, but this is such a simple thing I think you could drop this at > > > the same time as the move. > > > > > [Richard Zhu] Okay, got that. Would remove the "default:break" later. Thanks. > [Richard Zhu] I figure out that the default:break is required by IMX6Q/IMX6QP. > So I just don't drop them in v3 patch-set. I hope you don't mind me asking, but how is an empty default case in the switch statement helping IMX6Q and IMX6QP? What does it achieve for these two controllers specifically? Krzysztof
> -----Original Message----- > From: Krzysztof Wilczyński <kw@linux.com> > Sent: Saturday, October 23, 2021 5:54 PM > To: Richard Zhu <hongxing.zhu@nxp.com> > Cc: Bjorn Helgaas <helgaas@kernel.org>; l.stach@pengutronix.de; > bhelgaas@google.com; lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; kernel@pengutronix.de > Subject: Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling > unbalance when link never came up > > Hi, > > [...] > > > > > > - default: > > > > > > - break; > > > > > > > > While you're at it, this "default: break;" thing is pointless. > > > > Normally it's better to just *move* something without changing it > > > > at all, but this is such a simple thing I think you could drop > > > > this at the same time as the move. > > > > > > > [Richard Zhu] Okay, got that. Would remove the "default:break" later. > Thanks. > > [Richard Zhu] I figure out that the default:break is required by > IMX6Q/IMX6QP. > > So I just don't drop them in v3 patch-set. > > I hope you don't mind me asking, but how is an empty default case in the > switch statement helping IMX6Q and IMX6QP? What does it achieve for > these two controllers specifically? > [Richard Zhu] Never mind. 😊. There might be following building warning if the "default:break" is removed. " CC drivers/pci/controller/dwc/pci-imx6.o drivers/pci/controller/dwc/pci-imx6.c: In function ‘imx6_pcie_clk_disable’: drivers/pci/controller/dwc/pci-imx6.c:527:2: warning: enumeration value ‘IMX6Q’ not handled in switch [-Wswitch] 527 | switch (imx6_pcie->drvdata->variant) { | ^~~~~~ drivers/pci/controller/dwc/pci-imx6.c:527:2: warning: enumeration value ‘IMX6QP’ not handled in switch [-Wswitch]" Best Regards Richard Zhu > Krzysztof
On Fri, Oct 22, 2021 at 08:02:17AM +0000, Richard Zhu wrote: > <snipped ...> > > > > > > > > -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; > > > > > > While you're at it, this "default: break;" thing is pointless. > > > Normally it's better to just *move* something without changing it at > > > all, but this is such a simple thing I think you could drop this at > > > the same time as the move. > > > > > [Richard Zhu] Okay, got that. Would remove the "default:break" later. Thanks. > [Richard Zhu] I figure out that the default:break is required by > IMX6Q/IMX6QP. So I just don't drop them in v3 patch-set. That makes no sense. The code is: +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; + } +} Why do you think it makes a difference to remove the "default: break;"? There is no executable code after it. I don't see how IMX6Q/IMX6QP could depend on the default case. BTW, I feel like a broken record, but your v3 posting still has inconsistent subject line capitalization: PCI: imx6: move the clock disable function to a proper place PCI: dwc: add a new callback host exit function into host ops It would be nice if they were consistent and contained more specific information, e.g., PCI: imx6: Move imx6_pcie_clk_disable() earlier PCI: dwc: Add dw_pcie_host_ops.host_exit() callback Bjorn
On Mon, Oct 25, 2021 at 02:35:36AM +0000, Richard Zhu wrote: > > -----Original Message----- > > From: Krzysztof Wilczyński <kw@linux.com> > > Sent: Saturday, October 23, 2021 5:54 PM > > To: Richard Zhu <hongxing.zhu@nxp.com> > > Cc: Bjorn Helgaas <helgaas@kernel.org>; l.stach@pengutronix.de; > > bhelgaas@google.com; lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org; > > dl-linux-imx <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org; > > linux-kernel@vger.kernel.org; kernel@pengutronix.de > > Subject: Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling > > unbalance when link never came up > > I hope you don't mind me asking, but how is an empty default case in the > > switch statement helping IMX6Q and IMX6QP? What does it achieve for > > these two controllers specifically? > > > [Richard Zhu] Never mind. 😊. > There might be following building warning if the "default:break" is removed. > " CC drivers/pci/controller/dwc/pci-imx6.o > drivers/pci/controller/dwc/pci-imx6.c: In function ‘imx6_pcie_clk_disable’: > drivers/pci/controller/dwc/pci-imx6.c:527:2: warning: enumeration value ‘IMX6Q’ not handled in switch [-Wswitch] > 527 | switch (imx6_pcie->drvdata->variant) { > | ^~~~~~ > drivers/pci/controller/dwc/pci-imx6.c:527:2: warning: enumeration value ‘IMX6QP’ not handled in switch [-Wswitch]" Sorry, I didn't see this until after asking the same question as Krzysztof. Sigh. That's a really annoying gcc warning, but I guess I won't fight it ;) Bjorn
> -----Original Message----- > From: Bjorn Helgaas <helgaas@kernel.org> > Sent: Wednesday, October 27, 2021 12:39 AM > To: Richard Zhu <hongxing.zhu@nxp.com> > Cc: l.stach@pengutronix.de; bhelgaas@google.com; > lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; kernel@pengutronix.de > Subject: Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling > unbalance when link never came up > > On Fri, Oct 22, 2021 at 08:02:17AM +0000, Richard Zhu wrote: > > <snipped ...> > > > > > > > > > > -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; > > > > > > > > While you're at it, this "default: break;" thing is pointless. > > > > Normally it's better to just *move* something without changing it > > > > at all, but this is such a simple thing I think you could drop > > > > this at the same time as the move. > > > > > > > [Richard Zhu] Okay, got that. Would remove the "default:break" later. > Thanks. > > [Richard Zhu] I figure out that the default:break is required by > > IMX6Q/IMX6QP. So I just don't drop them in v3 patch-set. > > That makes no sense. The code is: > > +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; > + } > +} > > Why do you think it makes a difference to remove the > "default: break;"? There is no executable code after it. > I don't see how IMX6Q/IMX6QP could depend on the default case. > > BTW, I feel like a broken record, but your v3 posting still has inconsistent > subject line capitalization: > > PCI: imx6: move the clock disable function to a proper place > PCI: dwc: add a new callback host exit function into host ops > > It would be nice if they were consistent and contained more specific > information, e.g., > > PCI: imx6: Move imx6_pcie_clk_disable() earlier > PCI: dwc: Add dw_pcie_host_ops.host_exit() callback [Richard Zhu] Got that, would change to be consistent and more specific information. Thanks a lot. BR Richard > > Bjorn
> -----Original Message----- > From: Bjorn Helgaas <helgaas@kernel.org> > Sent: Wednesday, October 27, 2021 6:22 AM > To: Richard Zhu <hongxing.zhu@nxp.com> > Cc: Krzysztof Wilczyński <kw@linux.com>; l.stach@pengutronix.de; > bhelgaas@google.com; lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; kernel@pengutronix.de > Subject: Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling > unbalance when link never came up > > On Mon, Oct 25, 2021 at 02:35:36AM +0000, Richard Zhu wrote: > > > -----Original Message----- > > > From: Krzysztof Wilczyński <kw@linux.com> > > > Sent: Saturday, October 23, 2021 5:54 PM > > > To: Richard Zhu <hongxing.zhu@nxp.com> > > > Cc: Bjorn Helgaas <helgaas@kernel.org>; l.stach@pengutronix.de; > > > bhelgaas@google.com; lorenzo.pieralisi@arm.com; > > > linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > > > linux-arm-kernel@lists.infradead.org; > > > linux-kernel@vger.kernel.org; kernel@pengutronix.de > > > Subject: Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference > > > handling unbalance when link never came up > > > > I hope you don't mind me asking, but how is an empty default case in > > > the switch statement helping IMX6Q and IMX6QP? What does it achieve > > > for these two controllers specifically? > > > > > [Richard Zhu] Never mind. 😊. > > There might be following building warning if the "default:break" is removed. > > " CC drivers/pci/controller/dwc/pci-imx6.o > > drivers/pci/controller/dwc/pci-imx6.c: In function > ‘imx6_pcie_clk_disable’: > > drivers/pci/controller/dwc/pci-imx6.c:527:2: warning: enumeration value > ‘IMX6Q’ not handled in switch [-Wswitch] > > 527 | switch (imx6_pcie->drvdata->variant) { > > | ^~~~~~ > > drivers/pci/controller/dwc/pci-imx6.c:527:2: warning: enumeration value > ‘IMX6QP’ not handled in switch [-Wswitch]" > > Sorry, I didn't see this until after asking the same question as Krzysztof. > > Sigh. That's a really annoying gcc warning, but I guess I won't fight it ;) [Richard Zhu] Never mind, 😊. BR Richard > > Bjorn
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);
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@nxp.com> --- drivers/pci/controller/dwc/pci-imx6.c | 47 ++++++++++++++------------- 1 file changed, 24 insertions(+), 23 deletions(-)