This series patches refine pci-imx6 driver and do the following main changes.
- Encapsulate the clock enable into one standalone function
- Add the error propagation from host_init and resume
- Turn off regulator when the system is in suspend mode
- Let the probe successfully when link never comes up
- Do not hide the phy driver callbacks in core reset and clk_enable.
- Keep symmetric as much as possible between suspend and resume callbacks.
BTW, this series are verified on i.MX8MM EVK board when one NVME is used.
Main changes from v13 to v14 refer to Bjorn's comments:
- To keep suspend/resume symmetric as much as possible. Create
imx6_pcie_stop_link() and imx6_pcie_host_exit() functions, and invoke
them in suspend callback.
- Since the imx6_pcie_clk_disable() is invoked by imx6_pcie_host_exit(),
to be symmetric with imx6_pcie_host_exit(), move imx6_pcie_clk_enable()
to imx6_pcie_host_init() from imx6_pcie_deassert_core_reset().
Main changes from v12 to v13:
- Call imx6_pcie_host_init() instead of duplicating codes in resume.
- Move the regulator enable out of imx6_pcie_deassert_core_reset().
Re-format the error handling in imx6_pcie_deassert_core_reset()
and imx6_pcie_host_init() accordingly.
Main changes from v11 to v12 issued by Bjorn:
- Add four intro patches to move code around to collect similar things
(PHY management, reset management) together. This makes the first
diff to collect clock enables simpler because it's not cluttered with
unrelated things that didn't actually change.
- Factor out ref clock disables so the disable function structure matches
the enable structure.
- Drop unused "ret" from "Reduce resume time ..." to avoid bisection
hole, then add it back in "Do not hide phy driver ..." where we start
using it again.
- Add patch to make imx6_pcie_clk_disable() symmetric with
imx6_pcie_clk_enable() in terms of enable/disable ordering.
Main changes from v10 to v11:
No code changes, just do the following operations refer to Bjorn's comments.
- Split #6 patch into two patches.
- Rebase to v5.19-rc1 based on for-next branch of Shawn's git.
Main changes from v9 to v10:
- Add the "Reviewed-by: Lucas Stach <[email protected]>" tag into #3
and #4 patches.
- Refer to Bjorn's comments:
- refine the commit of the first patch
- keep alignment of the message format in the second patch
- More specific commit and subject of the #5 and #7 patches.
- Move the regualtor_disable into suspend, turn off the regulator when bus
is powered off and system in suspend mode.
- Let the driver probe successfully, return zero in imx6_pcie_start_link()
when PCIe link is down.
In this link down scenario, only start the PCIe link training in resume
when the link is up before system suspend to avoid the long latency in
the link training period.
- Don't hide phy driver callbacks in core reset and clk_enable, and refine
the error handling accordingly.
- Drop the #8 patch of v9 series, since the clocks and powers are not gated
off anymore when link is down.
Main changes from v8 to v9:
- Don't change pcie-designware codes, and do the error exit process only in
pci-imx6 driver internally.
- Move the phy driver callbacks to the proper places
Main changes from v7 to v8:
Regarding Bjorn's review comments.
- Align the format of the dev_info message and refine commit log of
#6/7/8 patches.
- Rename the err_reset_phy label, since there is no PHY reset in the out
Main changes from v6 to v7:
- Keep the regulator usage counter balance in the #5 patch of v6 series.
Main changes from v5 to v6:
- Refer to the following discussion with Fabio, fix the dump by his patch.
https://patchwork.kernel.org/project/linux-pci/patch/[email protected]/
Refine and rebase this patch-set after Fabio' dump fix patch is merged.
- Add one new #4 patch to disable i.MX6QDL REF clock too when disable clocks
- Split the regulator refine codes into one standalone patch #5 in this version.
Main changes from v4 to v5:
- Since i.MX8MM PCIe support had been merged. Based on Lorenzo's git repos,
resend the patch-set after rebase.
Main changes from v3 to v4:
- Regarding Mark's comments, delete the regulator_is_enabled() check.
- Squash #3 and #6 of v3 patch into #5 patch of v4 set.
Main changes from v2 to v3:
- Add "Reviewed-by: Lucas Stach <[email protected]>" tag into
first two patches.
- Add a Fixes tag into #3 patch.
- Split the #4 of v2 to two patches, one is clock disable codes move,
the other one is the acutal clock unbalance fix.
- Add a new host_exit() callback into dw_pcie_host_ops, then it could be
invoked to handle the unbalance issue in the error handling after
host_init() function when link is down.
- Add a new host_exit() callback for i.MX PCIe driver to handle this case
in the error handling after host_init.
Main changes from v1 to v2:
Regarding Lucas' comments.
- Move the placement of the new imx6_pcie_clk_enable() to avoid the
forward declarition.
- Seperate the second patch of v1 patch-set to three patches.
- Use the module_param to replace the kernel command line.
Regarding Bjorn's comments:
- Use the cover-letter for a multi-patch series.
- Correct the subject line, and refine the commit logs. For example,
remove the timestamp of the logs.
Bjorn Helgaas (5):
PCI: imx6: Move imx6_pcie_grp_offset(), imx6_pcie_configure_type()
earlier
PCI: imx6: Move PHY management functions together
PCI: imx6: Move imx6_pcie_enable_ref_clk() earlier
PCI: imx6: Factor out ref clock disable to match enable
PCI: imx6: Disable clocks in reverse order of enable
Richard Zhu (12):
PCI: imx6: Move imx6_pcie_clk_disable() earlier
PCI: imx6: Collect clock enables in imx6_pcie_clk_enable()
PCI: imx6: Propagate .host_init() errors to caller
PCI: imx6: Disable i.MX6QDL clock when disabling ref clocks
PCI: imx6: Call host init function directly in resume
PCI: imx6: Turn off regulator when system is in suspend mode
PCI: imx6: Move regulator enable out of
imx6_pcie_deassert_core_reset()
PCI: imx6: Mark the link down as non-fatal error
PCI: imx6: Reduce resume time by only starting link if it was up
before suspend
PCI: imx6: Do not hide phy driver callbacks and refine the error
handling
PCI: imx6: Reformat suspend callback to keep symmetric with resume
PCI: imx6: Move the imx6_pcie_ltssm_disable() earlier
drivers/pci/controller/dwc/pci-imx6.c | 661 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------------------------------------------
1 file changed, 358 insertions(+), 303 deletions(-)
Because i.MX PCIe doesn't support hot-plug feature. In the link down
scenario, only start the PCIe link training in resume when the link is up
before system suspend to avoid the long latency in the link training
period.
Signed-off-by: Richard Zhu <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index e236f824c808..5a06fbca82d6 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -67,6 +67,7 @@ struct imx6_pcie {
struct dw_pcie *pci;
int reset_gpio;
bool gpio_active_high;
+ bool link_is_up;
struct clk *pcie_bus;
struct clk *pcie_phy;
struct clk *pcie_inbound_axi;
@@ -881,11 +882,13 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
dev_info(dev, "Link: Gen2 disabled\n");
}
+ imx6_pcie->link_is_up = true;
tmp = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
dev_info(dev, "Link up, Gen%i\n", tmp & PCI_EXP_LNKSTA_CLS);
return 0;
err_reset_phy:
+ imx6_pcie->link_is_up = false;
dev_dbg(dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n",
dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
@@ -1032,9 +1035,8 @@ static int imx6_pcie_resume_noirq(struct device *dev)
return ret;
dw_pcie_setup_rc(pp);
- ret = imx6_pcie_start_link(imx6_pcie->pci);
- if (ret < 0)
- dev_info(dev, "pcie link is down after resume.\n");
+ if (imx6_pcie->link_is_up)
+ imx6_pcie_start_link(imx6_pcie->pci);
return 0;
}
--
2.25.1
When disabling PCIe clocks, disable i.MX6QDL ref clock too.
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Richard Zhu <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
Reviewed-by: Lucas Stach <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 78b839e92620..eaae144db4f3 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -586,6 +586,14 @@ static void imx6_pcie_disable_ref_clk(struct imx6_pcie *imx6_pcie)
case IMX6SX:
clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
break;
+ case IMX6QP:
+ case IMX6Q:
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
+ IMX6Q_GPR1_PCIE_REF_CLK_EN, 0);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
+ IMX6Q_GPR1_PCIE_TEST_PD,
+ IMX6Q_GPR1_PCIE_TEST_PD);
+ break;
case IMX7D:
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
--
2.25.1
From: Bjorn Helgaas <[email protected]>
Collect imx6_pcie_init_phy(), imx7d_pcie_wait_for_phy_pll_lock(), and
imx6_setup_phy_mpll() earlier with other PHY-related code. No functional
change intended.
Signed-off-by: Bjorn Helgaas <[email protected]>
Acked-by: Richard Zhu <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 256 +++++++++++++-------------
1 file changed, 128 insertions(+), 128 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 8653ca8cbfb9..e63eb6380020 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -296,6 +296,134 @@ static int pcie_phy_write(struct imx6_pcie *imx6_pcie, int addr, u16 data)
return 0;
}
+static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
+{
+ switch (imx6_pcie->drvdata->variant) {
+ case IMX8MM:
+ /*
+ * The PHY initialization had been done in the PHY
+ * driver, break here directly.
+ */
+ break;
+ case IMX8MQ:
+ /*
+ * TODO: Currently this code assumes external
+ * oscillator is being used
+ */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ imx6_pcie_grp_offset(imx6_pcie),
+ IMX8MQ_GPR_PCIE_REF_USE_PAD,
+ IMX8MQ_GPR_PCIE_REF_USE_PAD);
+ /*
+ * Regarding the datasheet, the PCIE_VPH is suggested
+ * to be 1.8V. If the PCIE_VPH is supplied by 3.3V, the
+ * VREG_BYPASS should be cleared to zero.
+ */
+ if (imx6_pcie->vph &&
+ regulator_get_voltage(imx6_pcie->vph) > 3000000)
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ imx6_pcie_grp_offset(imx6_pcie),
+ IMX8MQ_GPR_PCIE_VREG_BYPASS,
+ 0);
+ break;
+ case IMX7D:
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, 0);
+ break;
+ case IMX6SX:
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX6SX_GPR12_PCIE_RX_EQ_MASK,
+ IMX6SX_GPR12_PCIE_RX_EQ_2);
+ fallthrough;
+ default:
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX6Q_GPR12_PCIE_CTL_2, 0 << 10);
+
+ /* configure constant input signal to the pcie ctrl and phy */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX6Q_GPR12_LOS_LEVEL, 9 << 4);
+
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
+ IMX6Q_GPR8_TX_DEEMPH_GEN1,
+ imx6_pcie->tx_deemph_gen1 << 0);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
+ IMX6Q_GPR8_TX_DEEMPH_GEN2_3P5DB,
+ imx6_pcie->tx_deemph_gen2_3p5db << 6);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
+ IMX6Q_GPR8_TX_DEEMPH_GEN2_6DB,
+ imx6_pcie->tx_deemph_gen2_6db << 12);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
+ IMX6Q_GPR8_TX_SWING_FULL,
+ imx6_pcie->tx_swing_full << 18);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
+ IMX6Q_GPR8_TX_SWING_LOW,
+ imx6_pcie->tx_swing_low << 25);
+ break;
+ }
+
+ imx6_pcie_configure_type(imx6_pcie);
+}
+
+static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
+{
+ u32 val;
+ struct device *dev = imx6_pcie->pci->dev;
+
+ if (regmap_read_poll_timeout(imx6_pcie->iomuxc_gpr,
+ IOMUXC_GPR22, val,
+ val & IMX7D_GPR22_PCIE_PHY_PLL_LOCKED,
+ PHY_PLL_LOCK_WAIT_USLEEP_MAX,
+ PHY_PLL_LOCK_WAIT_TIMEOUT))
+ dev_err(dev, "PCIe PLL lock timeout\n");
+}
+
+static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie)
+{
+ unsigned long phy_rate = clk_get_rate(imx6_pcie->pcie_phy);
+ int mult, div;
+ u16 val;
+
+ if (!(imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_IMX6_PHY))
+ return 0;
+
+ switch (phy_rate) {
+ case 125000000:
+ /*
+ * The default settings of the MPLL are for a 125MHz input
+ * clock, so no need to reconfigure anything in that case.
+ */
+ return 0;
+ case 100000000:
+ mult = 25;
+ div = 0;
+ break;
+ case 200000000:
+ mult = 25;
+ div = 1;
+ break;
+ default:
+ dev_err(imx6_pcie->pci->dev,
+ "Unsupported PHY reference clock rate %lu\n", phy_rate);
+ return -EINVAL;
+ }
+
+ pcie_phy_read(imx6_pcie, PCIE_PHY_MPLL_OVRD_IN_LO, &val);
+ val &= ~(PCIE_PHY_MPLL_MULTIPLIER_MASK <<
+ PCIE_PHY_MPLL_MULTIPLIER_SHIFT);
+ val |= mult << PCIE_PHY_MPLL_MULTIPLIER_SHIFT;
+ val |= PCIE_PHY_MPLL_MULTIPLIER_OVRD;
+ pcie_phy_write(imx6_pcie, PCIE_PHY_MPLL_OVRD_IN_LO, val);
+
+ pcie_phy_read(imx6_pcie, PCIE_PHY_ATEOVRD, &val);
+ val &= ~(PCIE_PHY_ATEOVRD_REF_CLKDIV_MASK <<
+ PCIE_PHY_ATEOVRD_REF_CLKDIV_SHIFT);
+ val |= div << PCIE_PHY_ATEOVRD_REF_CLKDIV_SHIFT;
+ val |= PCIE_PHY_ATEOVRD_EN;
+ pcie_phy_write(imx6_pcie, PCIE_PHY_ATEOVRD, val);
+
+ return 0;
+}
+
static void imx6_pcie_reset_phy(struct imx6_pcie *imx6_pcie)
{
u16 tmp;
@@ -500,19 +628,6 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
return ret;
}
-static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
-{
- u32 val;
- struct device *dev = imx6_pcie->pci->dev;
-
- if (regmap_read_poll_timeout(imx6_pcie->iomuxc_gpr,
- IOMUXC_GPR22, val,
- val & IMX7D_GPR22_PCIE_PHY_PLL_LOCKED,
- PHY_PLL_LOCK_WAIT_USLEEP_MAX,
- PHY_PLL_LOCK_WAIT_TIMEOUT))
- dev_err(dev, "PCIe PLL lock timeout\n");
-}
-
static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
{
struct dw_pcie *pci = imx6_pcie->pci;
@@ -635,121 +750,6 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
}
}
-static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
-{
- switch (imx6_pcie->drvdata->variant) {
- case IMX8MM:
- /*
- * The PHY initialization had been done in the PHY
- * driver, break here directly.
- */
- break;
- case IMX8MQ:
- /*
- * TODO: Currently this code assumes external
- * oscillator is being used
- */
- regmap_update_bits(imx6_pcie->iomuxc_gpr,
- imx6_pcie_grp_offset(imx6_pcie),
- IMX8MQ_GPR_PCIE_REF_USE_PAD,
- IMX8MQ_GPR_PCIE_REF_USE_PAD);
- /*
- * Regarding the datasheet, the PCIE_VPH is suggested
- * to be 1.8V. If the PCIE_VPH is supplied by 3.3V, the
- * VREG_BYPASS should be cleared to zero.
- */
- if (imx6_pcie->vph &&
- regulator_get_voltage(imx6_pcie->vph) > 3000000)
- regmap_update_bits(imx6_pcie->iomuxc_gpr,
- imx6_pcie_grp_offset(imx6_pcie),
- IMX8MQ_GPR_PCIE_VREG_BYPASS,
- 0);
- break;
- case IMX7D:
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, 0);
- break;
- case IMX6SX:
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6SX_GPR12_PCIE_RX_EQ_MASK,
- IMX6SX_GPR12_PCIE_RX_EQ_2);
- fallthrough;
- default:
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6Q_GPR12_PCIE_CTL_2, 0 << 10);
-
- /* configure constant input signal to the pcie ctrl and phy */
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6Q_GPR12_LOS_LEVEL, 9 << 4);
-
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
- IMX6Q_GPR8_TX_DEEMPH_GEN1,
- imx6_pcie->tx_deemph_gen1 << 0);
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
- IMX6Q_GPR8_TX_DEEMPH_GEN2_3P5DB,
- imx6_pcie->tx_deemph_gen2_3p5db << 6);
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
- IMX6Q_GPR8_TX_DEEMPH_GEN2_6DB,
- imx6_pcie->tx_deemph_gen2_6db << 12);
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
- IMX6Q_GPR8_TX_SWING_FULL,
- imx6_pcie->tx_swing_full << 18);
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
- IMX6Q_GPR8_TX_SWING_LOW,
- imx6_pcie->tx_swing_low << 25);
- break;
- }
-
- imx6_pcie_configure_type(imx6_pcie);
-}
-
-static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie)
-{
- unsigned long phy_rate = clk_get_rate(imx6_pcie->pcie_phy);
- int mult, div;
- u16 val;
-
- if (!(imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_IMX6_PHY))
- return 0;
-
- switch (phy_rate) {
- case 125000000:
- /*
- * The default settings of the MPLL are for a 125MHz input
- * clock, so no need to reconfigure anything in that case.
- */
- return 0;
- case 100000000:
- mult = 25;
- div = 0;
- break;
- case 200000000:
- mult = 25;
- div = 1;
- break;
- default:
- dev_err(imx6_pcie->pci->dev,
- "Unsupported PHY reference clock rate %lu\n", phy_rate);
- return -EINVAL;
- }
-
- pcie_phy_read(imx6_pcie, PCIE_PHY_MPLL_OVRD_IN_LO, &val);
- val &= ~(PCIE_PHY_MPLL_MULTIPLIER_MASK <<
- PCIE_PHY_MPLL_MULTIPLIER_SHIFT);
- val |= mult << PCIE_PHY_MPLL_MULTIPLIER_SHIFT;
- val |= PCIE_PHY_MPLL_MULTIPLIER_OVRD;
- pcie_phy_write(imx6_pcie, PCIE_PHY_MPLL_OVRD_IN_LO, val);
-
- pcie_phy_read(imx6_pcie, PCIE_PHY_ATEOVRD, &val);
- val &= ~(PCIE_PHY_ATEOVRD_REF_CLKDIV_MASK <<
- PCIE_PHY_ATEOVRD_REF_CLKDIV_SHIFT);
- val |= div << PCIE_PHY_ATEOVRD_REF_CLKDIV_SHIFT;
- val |= PCIE_PHY_ATEOVRD_EN;
- pcie_phy_write(imx6_pcie, PCIE_PHY_ATEOVRD, val);
-
- return 0;
-}
-
static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie)
{
struct dw_pcie *pci = imx6_pcie->pci;
--
2.25.1
Call imx6_pcie_host_init() instead of duplicating codes in resume.
Signed-off-by: Richard Zhu <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index eaae144db4f3..2b42c37f1617 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1034,9 +1034,9 @@ static int imx6_pcie_resume_noirq(struct device *dev)
if (!(imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_SUPPORTS_SUSPEND))
return 0;
- imx6_pcie_assert_core_reset(imx6_pcie);
- imx6_pcie_init_phy(imx6_pcie);
- imx6_pcie_deassert_core_reset(imx6_pcie);
+ ret = imx6_pcie_host_init(pp);
+ if (ret)
+ return ret;
dw_pcie_setup_rc(pp);
ret = imx6_pcie_start_link(imx6_pcie->pci);
--
2.25.1
On Fri, Jul 01, 2022 at 11:25:18AM +0800, Richard Zhu wrote:
> This series patches refine pci-imx6 driver and do the following main changes.
> - Encapsulate the clock enable into one standalone function
> - Add the error propagation from host_init and resume
> - Turn off regulator when the system is in suspend mode
> - Let the probe successfully when link never comes up
> - Do not hide the phy driver callbacks in core reset and clk_enable.
> - Keep symmetric as much as possible between suspend and resume callbacks.
> BTW, this series are verified on i.MX8MM EVK board when one NVME is used.
>
> Main changes from v13 to v14 refer to Bjorn's comments:
> - To keep suspend/resume symmetric as much as possible. Create
> imx6_pcie_stop_link() and imx6_pcie_host_exit() functions, and invoke
> them in suspend callback.
> - Since the imx6_pcie_clk_disable() is invoked by imx6_pcie_host_exit(),
> to be symmetric with imx6_pcie_host_exit(), move imx6_pcie_clk_enable()
> to imx6_pcie_host_init() from imx6_pcie_deassert_core_reset().
>
> Main changes from v12 to v13:
> - Call imx6_pcie_host_init() instead of duplicating codes in resume.
> - Move the regulator enable out of imx6_pcie_deassert_core_reset().
> Re-format the error handling in imx6_pcie_deassert_core_reset()
> and imx6_pcie_host_init() accordingly.
>
> Main changes from v11 to v12 issued by Bjorn:
> - Add four intro patches to move code around to collect similar things
> (PHY management, reset management) together. This makes the first
> diff to collect clock enables simpler because it's not cluttered with
> unrelated things that didn't actually change.
> - Factor out ref clock disables so the disable function structure matches
> the enable structure.
> - Drop unused "ret" from "Reduce resume time ..." to avoid bisection
> hole, then add it back in "Do not hide phy driver ..." where we start
> using it again.
> - Add patch to make imx6_pcie_clk_disable() symmetric with
> imx6_pcie_clk_enable() in terms of enable/disable ordering.
>
> Main changes from v10 to v11:
> No code changes, just do the following operations refer to Bjorn's comments.
> - Split #6 patch into two patches.
> - Rebase to v5.19-rc1 based on for-next branch of Shawn's git.
>
> Main changes from v9 to v10:
> - Add the "Reviewed-by: Lucas Stach <[email protected]>" tag into #3
> and #4 patches.
> - Refer to Bjorn's comments:
> - refine the commit of the first patch
> - keep alignment of the message format in the second patch
> - More specific commit and subject of the #5 and #7 patches.
> - Move the regualtor_disable into suspend, turn off the regulator when bus
> is powered off and system in suspend mode.
> - Let the driver probe successfully, return zero in imx6_pcie_start_link()
> when PCIe link is down.
> In this link down scenario, only start the PCIe link training in resume
> when the link is up before system suspend to avoid the long latency in
> the link training period.
> - Don't hide phy driver callbacks in core reset and clk_enable, and refine
> the error handling accordingly.
> - Drop the #8 patch of v9 series, since the clocks and powers are not gated
> off anymore when link is down.
>
> Main changes from v8 to v9:
> - Don't change pcie-designware codes, and do the error exit process only in
> pci-imx6 driver internally.
> - Move the phy driver callbacks to the proper places
>
> Main changes from v7 to v8:
> Regarding Bjorn's review comments.
> - Align the format of the dev_info message and refine commit log of
> #6/7/8 patches.
> - Rename the err_reset_phy label, since there is no PHY reset in the out
>
> Main changes from v6 to v7:
> - Keep the regulator usage counter balance in the #5 patch of v6 series.
>
> Main changes from v5 to v6:
> - Refer to the following discussion with Fabio, fix the dump by his patch.
> https://patchwork.kernel.org/project/linux-pci/patch/[email protected]/
> Refine and rebase this patch-set after Fabio' dump fix patch is merged.
> - Add one new #4 patch to disable i.MX6QDL REF clock too when disable clocks
> - Split the regulator refine codes into one standalone patch #5 in this version.
>
> Main changes from v4 to v5:
> - Since i.MX8MM PCIe support had been merged. Based on Lorenzo's git repos,
> resend the patch-set after rebase.
>
> Main changes from v3 to v4:
> - Regarding Mark's comments, delete the regulator_is_enabled() check.
> - Squash #3 and #6 of v3 patch into #5 patch of v4 set.
>
> Main changes from v2 to v3:
> - Add "Reviewed-by: Lucas Stach <[email protected]>" tag into
> first two patches.
> - Add a Fixes tag into #3 patch.
> - Split the #4 of v2 to two patches, one is clock disable codes move,
> the other one is the acutal clock unbalance fix.
> - Add a new host_exit() callback into dw_pcie_host_ops, then it could be
> invoked to handle the unbalance issue in the error handling after
> host_init() function when link is down.
> - Add a new host_exit() callback for i.MX PCIe driver to handle this case
> in the error handling after host_init.
>
> Main changes from v1 to v2:
> Regarding Lucas' comments.
> - Move the placement of the new imx6_pcie_clk_enable() to avoid the
> forward declarition.
> - Seperate the second patch of v1 patch-set to three patches.
> - Use the module_param to replace the kernel command line.
> Regarding Bjorn's comments:
> - Use the cover-letter for a multi-patch series.
> - Correct the subject line, and refine the commit logs. For example,
> remove the timestamp of the logs.
>
> Bjorn Helgaas (5):
> PCI: imx6: Move imx6_pcie_grp_offset(), imx6_pcie_configure_type()
> earlier
> PCI: imx6: Move PHY management functions together
> PCI: imx6: Move imx6_pcie_enable_ref_clk() earlier
> PCI: imx6: Factor out ref clock disable to match enable
> PCI: imx6: Disable clocks in reverse order of enable
>
> Richard Zhu (12):
> PCI: imx6: Move imx6_pcie_clk_disable() earlier
> PCI: imx6: Collect clock enables in imx6_pcie_clk_enable()
> PCI: imx6: Propagate .host_init() errors to caller
> PCI: imx6: Disable i.MX6QDL clock when disabling ref clocks
> PCI: imx6: Call host init function directly in resume
> PCI: imx6: Turn off regulator when system is in suspend mode
> PCI: imx6: Move regulator enable out of
> imx6_pcie_deassert_core_reset()
> PCI: imx6: Mark the link down as non-fatal error
> PCI: imx6: Reduce resume time by only starting link if it was up
> before suspend
> PCI: imx6: Do not hide phy driver callbacks and refine the error
> handling
> PCI: imx6: Reformat suspend callback to keep symmetric with resume
> PCI: imx6: Move the imx6_pcie_ltssm_disable() earlier
>
> drivers/pci/controller/dwc/pci-imx6.c | 661 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------------------------------------------
> 1 file changed, 358 insertions(+), 303 deletions(-)
Applied to pci/ctrl/imx6 for v5.20, thanks a lot for all your work
here!
Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> From: Bjorn Helgaas <[email protected]>
>
> Collect imx6_pcie_init_phy(), imx7d_pcie_wait_for_phy_pll_lock(), and
> imx6_setup_phy_mpll() earlier with other PHY-related code. No functional
> change intended.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> Acked-by: Richard Zhu <[email protected]>
Reviewed-by: Lucas Stach <[email protected]>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 256 +++++++++++++-------------
> 1 file changed, 128 insertions(+), 128 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 8653ca8cbfb9..e63eb6380020 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -296,6 +296,134 @@ static int pcie_phy_write(struct imx6_pcie *imx6_pcie, int addr, u16 data)
> return 0;
> }
>
> +static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
> +{
> + switch (imx6_pcie->drvdata->variant) {
> + case IMX8MM:
> + /*
> + * The PHY initialization had been done in the PHY
> + * driver, break here directly.
> + */
> + break;
> + case IMX8MQ:
> + /*
> + * TODO: Currently this code assumes external
> + * oscillator is being used
> + */
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + imx6_pcie_grp_offset(imx6_pcie),
> + IMX8MQ_GPR_PCIE_REF_USE_PAD,
> + IMX8MQ_GPR_PCIE_REF_USE_PAD);
> + /*
> + * Regarding the datasheet, the PCIE_VPH is suggested
> + * to be 1.8V. If the PCIE_VPH is supplied by 3.3V, the
> + * VREG_BYPASS should be cleared to zero.
> + */
> + if (imx6_pcie->vph &&
> + regulator_get_voltage(imx6_pcie->vph) > 3000000)
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> + imx6_pcie_grp_offset(imx6_pcie),
> + IMX8MQ_GPR_PCIE_VREG_BYPASS,
> + 0);
> + break;
> + case IMX7D:
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> + IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, 0);
> + break;
> + case IMX6SX:
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> + IMX6SX_GPR12_PCIE_RX_EQ_MASK,
> + IMX6SX_GPR12_PCIE_RX_EQ_2);
> + fallthrough;
> + default:
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> + IMX6Q_GPR12_PCIE_CTL_2, 0 << 10);
> +
> + /* configure constant input signal to the pcie ctrl and phy */
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> + IMX6Q_GPR12_LOS_LEVEL, 9 << 4);
> +
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
> + IMX6Q_GPR8_TX_DEEMPH_GEN1,
> + imx6_pcie->tx_deemph_gen1 << 0);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
> + IMX6Q_GPR8_TX_DEEMPH_GEN2_3P5DB,
> + imx6_pcie->tx_deemph_gen2_3p5db << 6);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
> + IMX6Q_GPR8_TX_DEEMPH_GEN2_6DB,
> + imx6_pcie->tx_deemph_gen2_6db << 12);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
> + IMX6Q_GPR8_TX_SWING_FULL,
> + imx6_pcie->tx_swing_full << 18);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
> + IMX6Q_GPR8_TX_SWING_LOW,
> + imx6_pcie->tx_swing_low << 25);
> + break;
> + }
> +
> + imx6_pcie_configure_type(imx6_pcie);
> +}
> +
> +static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
> +{
> + u32 val;
> + struct device *dev = imx6_pcie->pci->dev;
> +
> + if (regmap_read_poll_timeout(imx6_pcie->iomuxc_gpr,
> + IOMUXC_GPR22, val,
> + val & IMX7D_GPR22_PCIE_PHY_PLL_LOCKED,
> + PHY_PLL_LOCK_WAIT_USLEEP_MAX,
> + PHY_PLL_LOCK_WAIT_TIMEOUT))
> + dev_err(dev, "PCIe PLL lock timeout\n");
> +}
> +
> +static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie)
> +{
> + unsigned long phy_rate = clk_get_rate(imx6_pcie->pcie_phy);
> + int mult, div;
> + u16 val;
> +
> + if (!(imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_IMX6_PHY))
> + return 0;
> +
> + switch (phy_rate) {
> + case 125000000:
> + /*
> + * The default settings of the MPLL are for a 125MHz input
> + * clock, so no need to reconfigure anything in that case.
> + */
> + return 0;
> + case 100000000:
> + mult = 25;
> + div = 0;
> + break;
> + case 200000000:
> + mult = 25;
> + div = 1;
> + break;
> + default:
> + dev_err(imx6_pcie->pci->dev,
> + "Unsupported PHY reference clock rate %lu\n", phy_rate);
> + return -EINVAL;
> + }
> +
> + pcie_phy_read(imx6_pcie, PCIE_PHY_MPLL_OVRD_IN_LO, &val);
> + val &= ~(PCIE_PHY_MPLL_MULTIPLIER_MASK <<
> + PCIE_PHY_MPLL_MULTIPLIER_SHIFT);
> + val |= mult << PCIE_PHY_MPLL_MULTIPLIER_SHIFT;
> + val |= PCIE_PHY_MPLL_MULTIPLIER_OVRD;
> + pcie_phy_write(imx6_pcie, PCIE_PHY_MPLL_OVRD_IN_LO, val);
> +
> + pcie_phy_read(imx6_pcie, PCIE_PHY_ATEOVRD, &val);
> + val &= ~(PCIE_PHY_ATEOVRD_REF_CLKDIV_MASK <<
> + PCIE_PHY_ATEOVRD_REF_CLKDIV_SHIFT);
> + val |= div << PCIE_PHY_ATEOVRD_REF_CLKDIV_SHIFT;
> + val |= PCIE_PHY_ATEOVRD_EN;
> + pcie_phy_write(imx6_pcie, PCIE_PHY_ATEOVRD, val);
> +
> + return 0;
> +}
> +
> static void imx6_pcie_reset_phy(struct imx6_pcie *imx6_pcie)
> {
> u16 tmp;
> @@ -500,19 +628,6 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> return ret;
> }
>
> -static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
> -{
> - u32 val;
> - struct device *dev = imx6_pcie->pci->dev;
> -
> - if (regmap_read_poll_timeout(imx6_pcie->iomuxc_gpr,
> - IOMUXC_GPR22, val,
> - val & IMX7D_GPR22_PCIE_PHY_PLL_LOCKED,
> - PHY_PLL_LOCK_WAIT_USLEEP_MAX,
> - PHY_PLL_LOCK_WAIT_TIMEOUT))
> - dev_err(dev, "PCIe PLL lock timeout\n");
> -}
> -
> static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> {
> struct dw_pcie *pci = imx6_pcie->pci;
> @@ -635,121 +750,6 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> }
> }
>
> -static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
> -{
> - switch (imx6_pcie->drvdata->variant) {
> - case IMX8MM:
> - /*
> - * The PHY initialization had been done in the PHY
> - * driver, break here directly.
> - */
> - break;
> - case IMX8MQ:
> - /*
> - * TODO: Currently this code assumes external
> - * oscillator is being used
> - */
> - regmap_update_bits(imx6_pcie->iomuxc_gpr,
> - imx6_pcie_grp_offset(imx6_pcie),
> - IMX8MQ_GPR_PCIE_REF_USE_PAD,
> - IMX8MQ_GPR_PCIE_REF_USE_PAD);
> - /*
> - * Regarding the datasheet, the PCIE_VPH is suggested
> - * to be 1.8V. If the PCIE_VPH is supplied by 3.3V, the
> - * VREG_BYPASS should be cleared to zero.
> - */
> - if (imx6_pcie->vph &&
> - regulator_get_voltage(imx6_pcie->vph) > 3000000)
> - regmap_update_bits(imx6_pcie->iomuxc_gpr,
> - imx6_pcie_grp_offset(imx6_pcie),
> - IMX8MQ_GPR_PCIE_VREG_BYPASS,
> - 0);
> - break;
> - case IMX7D:
> - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> - IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, 0);
> - break;
> - case IMX6SX:
> - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> - IMX6SX_GPR12_PCIE_RX_EQ_MASK,
> - IMX6SX_GPR12_PCIE_RX_EQ_2);
> - fallthrough;
> - default:
> - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> - IMX6Q_GPR12_PCIE_CTL_2, 0 << 10);
> -
> - /* configure constant input signal to the pcie ctrl and phy */
> - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> - IMX6Q_GPR12_LOS_LEVEL, 9 << 4);
> -
> - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
> - IMX6Q_GPR8_TX_DEEMPH_GEN1,
> - imx6_pcie->tx_deemph_gen1 << 0);
> - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
> - IMX6Q_GPR8_TX_DEEMPH_GEN2_3P5DB,
> - imx6_pcie->tx_deemph_gen2_3p5db << 6);
> - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
> - IMX6Q_GPR8_TX_DEEMPH_GEN2_6DB,
> - imx6_pcie->tx_deemph_gen2_6db << 12);
> - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
> - IMX6Q_GPR8_TX_SWING_FULL,
> - imx6_pcie->tx_swing_full << 18);
> - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
> - IMX6Q_GPR8_TX_SWING_LOW,
> - imx6_pcie->tx_swing_low << 25);
> - break;
> - }
> -
> - imx6_pcie_configure_type(imx6_pcie);
> -}
> -
> -static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie)
> -{
> - unsigned long phy_rate = clk_get_rate(imx6_pcie->pcie_phy);
> - int mult, div;
> - u16 val;
> -
> - if (!(imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_IMX6_PHY))
> - return 0;
> -
> - switch (phy_rate) {
> - case 125000000:
> - /*
> - * The default settings of the MPLL are for a 125MHz input
> - * clock, so no need to reconfigure anything in that case.
> - */
> - return 0;
> - case 100000000:
> - mult = 25;
> - div = 0;
> - break;
> - case 200000000:
> - mult = 25;
> - div = 1;
> - break;
> - default:
> - dev_err(imx6_pcie->pci->dev,
> - "Unsupported PHY reference clock rate %lu\n", phy_rate);
> - return -EINVAL;
> - }
> -
> - pcie_phy_read(imx6_pcie, PCIE_PHY_MPLL_OVRD_IN_LO, &val);
> - val &= ~(PCIE_PHY_MPLL_MULTIPLIER_MASK <<
> - PCIE_PHY_MPLL_MULTIPLIER_SHIFT);
> - val |= mult << PCIE_PHY_MPLL_MULTIPLIER_SHIFT;
> - val |= PCIE_PHY_MPLL_MULTIPLIER_OVRD;
> - pcie_phy_write(imx6_pcie, PCIE_PHY_MPLL_OVRD_IN_LO, val);
> -
> - pcie_phy_read(imx6_pcie, PCIE_PHY_ATEOVRD, &val);
> - val &= ~(PCIE_PHY_ATEOVRD_REF_CLKDIV_MASK <<
> - PCIE_PHY_ATEOVRD_REF_CLKDIV_SHIFT);
> - val |= div << PCIE_PHY_ATEOVRD_REF_CLKDIV_SHIFT;
> - val |= PCIE_PHY_ATEOVRD_EN;
> - pcie_phy_write(imx6_pcie, PCIE_PHY_ATEOVRD, val);
> -
> - return 0;
> -}
> -
> static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie)
> {
> struct dw_pcie *pci = imx6_pcie->pci;
Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> Call imx6_pcie_host_init() instead of duplicating codes in resume.
>
> Signed-off-by: Richard Zhu <[email protected]>
So this isn't strictly a de-duplication, as imx6_pcie_host_init also
does the MPLL setup again on i.MX6SX. Which I believe is absolutely the
right thing to do in resume, even though I'm not aware of any system
that would be affected by this change.
Reviewed-by: Lucas Stach <[email protected]>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index eaae144db4f3..2b42c37f1617 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1034,9 +1034,9 @@ static int imx6_pcie_resume_noirq(struct device *dev)
> if (!(imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_SUPPORTS_SUSPEND))
> return 0;
>
> - imx6_pcie_assert_core_reset(imx6_pcie);
> - imx6_pcie_init_phy(imx6_pcie);
> - imx6_pcie_deassert_core_reset(imx6_pcie);
> + ret = imx6_pcie_host_init(pp);
> + if (ret)
> + return ret;
> dw_pcie_setup_rc(pp);
>
> ret = imx6_pcie_start_link(imx6_pcie->pci);
Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> Because i.MX PCIe doesn't support hot-plug feature. In the link down
> scenario, only start the PCIe link training in resume when the link is up
> before system suspend to avoid the long latency in the link training
> period.
>
> Signed-off-by: Richard Zhu <[email protected]>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index e236f824c808..5a06fbca82d6 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -67,6 +67,7 @@ struct imx6_pcie {
> struct dw_pcie *pci;
> int reset_gpio;
> bool gpio_active_high;
> + bool link_is_up;
> struct clk *pcie_bus;
> struct clk *pcie_phy;
> struct clk *pcie_inbound_axi;
> @@ -881,11 +882,13 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
> dev_info(dev, "Link: Gen2 disabled\n");
> }
>
> + imx6_pcie->link_is_up = true;
> tmp = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
> dev_info(dev, "Link up, Gen%i\n", tmp & PCI_EXP_LNKSTA_CLS);
> return 0;
>
> err_reset_phy:
> + imx6_pcie->link_is_up = false;
> dev_dbg(dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n",
> dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> @@ -1032,9 +1035,8 @@ static int imx6_pcie_resume_noirq(struct device *dev)
> return ret;
> dw_pcie_setup_rc(pp);
>
> - ret = imx6_pcie_start_link(imx6_pcie->pci);
> - if (ret < 0)
> - dev_info(dev, "pcie link is down after resume.\n");
> + if (imx6_pcie->link_is_up)
> + imx6_pcie_start_link(imx6_pcie->pci);
While the change itself is correct, the removal of the return value
check should be added to the previous patch, as that's the point where
you change this function to always return 0, rendering this check
pointless.
Regards,
Lucas
>
> return 0;
> }
> -----Original Message-----
> From: Lucas Stach <[email protected]>
> Sent: 2022年7月13日 16:48
> To: Hongxing Zhu <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; dl-linux-imx
> <[email protected]>
> Subject: Re: [PATCH v14 13/17] PCI: imx6: Reduce resume time by only starting
> link if it was up before suspend
>
> Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> > Because i.MX PCIe doesn't support hot-plug feature. In the link down
> > scenario, only start the PCIe link training in resume when the link is
> > up before system suspend to avoid the long latency in the link
> > training period.
> >
> > Signed-off-by: Richard Zhu <[email protected]>
> > Signed-off-by: Bjorn Helgaas <[email protected]>
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index e236f824c808..5a06fbca82d6 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -67,6 +67,7 @@ struct imx6_pcie {
> > struct dw_pcie *pci;
> > int reset_gpio;
> > bool gpio_active_high;
> > + bool link_is_up;
> > struct clk *pcie_bus;
> > struct clk *pcie_phy;
> > struct clk *pcie_inbound_axi;
> > @@ -881,11 +882,13 @@ static int imx6_pcie_start_link(struct dw_pcie
> *pci)
> > dev_info(dev, "Link: Gen2 disabled\n");
> > }
> >
> > + imx6_pcie->link_is_up = true;
> > tmp = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
> > dev_info(dev, "Link up, Gen%i\n", tmp & PCI_EXP_LNKSTA_CLS);
> > return 0;
> >
> > err_reset_phy:
> > + imx6_pcie->link_is_up = false;
> > dev_dbg(dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n",
> > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1)); @@ -1032,9 +1035,8
> @@
> > static int imx6_pcie_resume_noirq(struct device *dev)
> > return ret;
> > dw_pcie_setup_rc(pp);
> >
> > - ret = imx6_pcie_start_link(imx6_pcie->pci);
> > - if (ret < 0)
> > - dev_info(dev, "pcie link is down after resume.\n");
> > + if (imx6_pcie->link_is_up)
> > + imx6_pcie_start_link(imx6_pcie->pci);
>
> While the change itself is correct, the removal of the return value check should
> be added to the previous patch, as that's the point where you change this
> function to always return 0, rendering this check pointless.
Thanks for your comments.
Yes, it is. Indeed the return check should be cleaned up in the 12/17, since
imx6_pcie_start_link() always returns 0.
Best Regards
Richard Zhu
>
> Regards,
> Lucas
>
> >
> > return 0;
> > }
>
On Mon, Jul 11, 2022 at 05:29:59PM -0500, Bjorn Helgaas wrote:
> On Fri, Jul 01, 2022 at 11:25:18AM +0800, Richard Zhu wrote:
> > ...
> > Bjorn Helgaas (5):
> > PCI: imx6: Move imx6_pcie_grp_offset(), imx6_pcie_configure_type()
> > earlier
> > PCI: imx6: Move PHY management functions together
> > PCI: imx6: Move imx6_pcie_enable_ref_clk() earlier
> > PCI: imx6: Factor out ref clock disable to match enable
> > PCI: imx6: Disable clocks in reverse order of enable
> >
> > Richard Zhu (12):
> > PCI: imx6: Move imx6_pcie_clk_disable() earlier
> > PCI: imx6: Collect clock enables in imx6_pcie_clk_enable()
> > PCI: imx6: Propagate .host_init() errors to caller
> > PCI: imx6: Disable i.MX6QDL clock when disabling ref clocks
> > PCI: imx6: Call host init function directly in resume
> > PCI: imx6: Turn off regulator when system is in suspend mode
> > PCI: imx6: Move regulator enable out of
> > imx6_pcie_deassert_core_reset()
> > PCI: imx6: Mark the link down as non-fatal error
> > PCI: imx6: Reduce resume time by only starting link if it was up
> > before suspend
> > PCI: imx6: Do not hide phy driver callbacks and refine the error
> > handling
> > PCI: imx6: Reformat suspend callback to keep symmetric with resume
> > PCI: imx6: Move the imx6_pcie_ltssm_disable() earlier
> >
> > drivers/pci/controller/dwc/pci-imx6.c | 661 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------------------------------------------
> > 1 file changed, 358 insertions(+), 303 deletions(-)
>
> Applied to pci/ctrl/imx6 for v5.20, thanks a lot for all your work
> here!
I updated the branch with Lucas' acks and with the minor
imx6_pcie_start_link() return value check he suggested.
I did not do anything about the missing IMX8MQ case in
imx6_pcie_ltssm_disable() or the PHY init or regulator or shutdown
issues. If you want changes there, please make them starting with the
pci/ctrl/imx6 branch and I can just replace it:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/ctrl/imx6&id=7d652ce95e70
Bjorn
> -----Original Message-----
> From: Bjorn Helgaas <[email protected]>
> Sent: 2022??7??14?? 0:46
> To: Hongxing Zhu <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>
> Subject: Re: [PATCH v14 0/17] PCI: imx6: refine codes and add the error
> propagation
>
> On Mon, Jul 11, 2022 at 05:29:59PM -0500, Bjorn Helgaas wrote:
> > On Fri, Jul 01, 2022 at 11:25:18AM +0800, Richard Zhu wrote:
> > > ...
>
> > > Bjorn Helgaas (5):
> > > PCI: imx6: Move imx6_pcie_grp_offset(), imx6_pcie_configure_type()
> > > earlier
> > > PCI: imx6: Move PHY management functions together
> > > PCI: imx6: Move imx6_pcie_enable_ref_clk() earlier
> > > PCI: imx6: Factor out ref clock disable to match enable
> > > PCI: imx6: Disable clocks in reverse order of enable
> > >
> > > Richard Zhu (12):
> > > PCI: imx6: Move imx6_pcie_clk_disable() earlier
> > > PCI: imx6: Collect clock enables in imx6_pcie_clk_enable()
> > > PCI: imx6: Propagate .host_init() errors to caller
> > > PCI: imx6: Disable i.MX6QDL clock when disabling ref clocks
> > > PCI: imx6: Call host init function directly in resume
> > > PCI: imx6: Turn off regulator when system is in suspend mode
> > > PCI: imx6: Move regulator enable out of
> > > imx6_pcie_deassert_core_reset()
> > > PCI: imx6: Mark the link down as non-fatal error
> > > PCI: imx6: Reduce resume time by only starting link if it was up
> > > before suspend
> > > PCI: imx6: Do not hide phy driver callbacks and refine the error
> > > handling
> > > PCI: imx6: Reformat suspend callback to keep symmetric with resume
> > > PCI: imx6: Move the imx6_pcie_ltssm_disable() earlier
> > >
> > > drivers/pci/controller/dwc/pci-imx6.c | 661
> > >
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++
> > > +++++++++++++++++++++++++++++++++++++++++++++-----------------------
> > > --------------------------------------------------------------------
> > > ----
> > > 1 file changed, 358 insertions(+), 303 deletions(-)
> >
> > Applied to pci/ctrl/imx6 for v5.20, thanks a lot for all your work
> > here!
Hi Bjorn:
>
> I updated the branch with Lucas' acks and with the minor
> imx6_pcie_start_link() return value check he suggested.
>
Thanks a lot for your kindly help.
> I did not do anything about the missing IMX8MQ case in
> imx6_pcie_ltssm_disable() or the PHY init or regulator or shutdown issues. If
> you want changes there, please make them starting with the
> pci/ctrl/imx6 branch and I can just replace it:
>
>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern
> el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fhelgaas%2Fpci.git%2Fcom
> mit%2F%3Fh%3Dpci%2Fctrl%2Fimx6%26id%3D7d652ce95e70&data=05
> %7C01%7Chongxing.zhu%40nxp.com%7Cd0b3d82dae37495b2f7e08da64ef2a
> 3a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379332756103
> 31009%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu
> MzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Z8
> grlALYqs3ONJp1EQJsFJd8fD8XiE3zFXHuYlfE4TI%3D&reserved=0
>
The following changes are applied.
Should I re-send all 17 patches out, or just the following five patches?
10/17
- Remove regulator_disable() from imx6_pcie_shutdown() and update the commit
log accordingly refer to Lucas' comments.
11/17
- Move the regulator enable before the PHY init and core reset assert to
avoid introducing more failure cleanup paths refer to Lucas' comments.
14/17 has the codes conflictions.
- Rebase the 14/17 patch because of the codes conflictions introduced by
previous 11/17 new changes.
16/17
- Add the missing the IMX8MQ case and drop the default case in
imx6_pcie_ltssm_disable() refer to Lucas' comments.
17/17
- Rebase the codes and resolve the codes conflictions introduced by
previous 11/17 new changes.
- Correct one failure cleanup in imx6_pcie_host_init().
Best Regards
Richard Zhu
> Bjorn