[PATCH v14 0/17] PCI: imx6: refine codes and add the error propagation

[PATCH v14 0/17] PCI: imx6: refine codes and add the error propagation

[Richard Zhu]

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 <l.stach@pengutronix.de>" 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/1641368602-20401-6-git-send-email-hongxing.zhu@nxp.com/
  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 <l.stach@pengutronix.de>" 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(-)

[PATCH v14 01/17] PCI: imx6: Move imx6_pcie_grp_offset(), imx6_pcie_configure_type() earlier

[Richard Zhu]

From: Bjorn Helgaas <bhelgaas@google.com>

Move imx6_pcie_grp_offset() and imx6_pcie_configure_type() earlier in the
file since they depend on nothing and are used by several other functions
that will be moved earlier.  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 50 +++++++++++++--------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 7a285fb0f619..8653ca8cbfb9 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -146,6 +146,31 @@ struct imx6_pcie {
 #define PHY_RX_OVRD_IN_LO_RX_DATA_EN		BIT(5)
 #define PHY_RX_OVRD_IN_LO_RX_PLL_EN		BIT(3)
 
+static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
+{
+	WARN_ON(imx6_pcie->drvdata->variant != IMX8MQ &&
+		imx6_pcie->drvdata->variant != IMX8MM);
+	return imx6_pcie->controller_id == 1 ? IOMUXC_GPR16 : IOMUXC_GPR14;
+}
+
+static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
+{
+	unsigned int mask, val;
+
+	if (imx6_pcie->drvdata->variant == IMX8MQ &&
+	    imx6_pcie->controller_id == 1) {
+		mask = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE;
+		val  = FIELD_PREP(IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
+				  PCI_EXP_TYPE_ROOT_PORT);
+	} else {
+		mask = IMX6Q_GPR12_DEVICE_TYPE;
+		val  = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE,
+				  PCI_EXP_TYPE_ROOT_PORT);
+	}
+
+	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, mask, val);
+}
+
 static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
 {
 	struct dw_pcie *pci = imx6_pcie->pci;
@@ -415,13 +440,6 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 					imx6_pcie->gpio_active_high);
 }
 
-static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
-{
-	WARN_ON(imx6_pcie->drvdata->variant != IMX8MQ &&
-		imx6_pcie->drvdata->variant != IMX8MM);
-	return imx6_pcie->controller_id == 1 ? IOMUXC_GPR16 : IOMUXC_GPR14;
-}
-
 static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
 {
 	struct dw_pcie *pci = imx6_pcie->pci;
@@ -617,24 +635,6 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	}
 }
 
-static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
-{
-	unsigned int mask, val;
-
-	if (imx6_pcie->drvdata->variant == IMX8MQ &&
-	    imx6_pcie->controller_id == 1) {
-		mask   = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE;
-		val    = FIELD_PREP(IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
-				    PCI_EXP_TYPE_ROOT_PORT);
-	} else {
-		mask = IMX6Q_GPR12_DEVICE_TYPE;
-		val  = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE,
-				  PCI_EXP_TYPE_ROOT_PORT);
-	}
-
-	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, mask, val);
-}
-
 static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
 {
 	switch (imx6_pcie->drvdata->variant) {
-- 
2.25.1

[PATCH v14 02/17] PCI: imx6: Move PHY management functions together

[Richard Zhu]

From: Bjorn Helgaas <bhelgaas@google.com>

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 <bhelgaas@google.com>
Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 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

[PATCH v14 03/17] PCI: imx6: Move imx6_pcie_enable_ref_clk() earlier

[Richard Zhu]

From: Bjorn Helgaas <bhelgaas@google.com>

Move imx6_pcie_enable_ref_clk() earlier so it's not in the middle between
imx6_pcie_assert_core_reset() and imx6_pcie_deassert_core_reset().  No
functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 96 +++++++++++++--------------
 1 file changed, 48 insertions(+), 48 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index e63eb6380020..a6d2b907d42b 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -520,54 +520,6 @@ static int imx6_pcie_attach_pd(struct device *dev)
 	return 0;
 }
 
-static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
-{
-	struct device *dev = imx6_pcie->pci->dev;
-
-	switch (imx6_pcie->drvdata->variant) {
-	case IMX7D:
-	case IMX8MQ:
-		reset_control_assert(imx6_pcie->pciephy_reset);
-		fallthrough;
-	case IMX8MM:
-		reset_control_assert(imx6_pcie->apps_reset);
-		break;
-	case IMX6SX:
-		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
-				   IMX6SX_GPR12_PCIE_TEST_POWERDOWN,
-				   IMX6SX_GPR12_PCIE_TEST_POWERDOWN);
-		/* Force PCIe PHY reset */
-		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR5,
-				   IMX6SX_GPR5_PCIE_BTNRST_RESET,
-				   IMX6SX_GPR5_PCIE_BTNRST_RESET);
-		break;
-	case IMX6QP:
-		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
-				   IMX6Q_GPR1_PCIE_SW_RST,
-				   IMX6Q_GPR1_PCIE_SW_RST);
-		break;
-	case IMX6Q:
-		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
-				   IMX6Q_GPR1_PCIE_TEST_PD, 1 << 18);
-		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
-				   IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
-		break;
-	}
-
-	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
-		int ret = regulator_disable(imx6_pcie->vpcie);
-
-		if (ret)
-			dev_err(dev, "failed to disable vpcie regulator: %d\n",
-				ret);
-	}
-
-	/* Some boards don't have PCIe reset GPIO. */
-	if (gpio_is_valid(imx6_pcie->reset_gpio))
-		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
-					imx6_pcie->gpio_active_high);
-}
-
 static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
 {
 	struct dw_pcie *pci = imx6_pcie->pci;
@@ -628,6 +580,54 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
 	return ret;
 }
 
+static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
+{
+	struct device *dev = imx6_pcie->pci->dev;
+
+	switch (imx6_pcie->drvdata->variant) {
+	case IMX7D:
+	case IMX8MQ:
+		reset_control_assert(imx6_pcie->pciephy_reset);
+		fallthrough;
+	case IMX8MM:
+		reset_control_assert(imx6_pcie->apps_reset);
+		break;
+	case IMX6SX:
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+				   IMX6SX_GPR12_PCIE_TEST_POWERDOWN,
+				   IMX6SX_GPR12_PCIE_TEST_POWERDOWN);
+		/* Force PCIe PHY reset */
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR5,
+				   IMX6SX_GPR5_PCIE_BTNRST_RESET,
+				   IMX6SX_GPR5_PCIE_BTNRST_RESET);
+		break;
+	case IMX6QP:
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
+				   IMX6Q_GPR1_PCIE_SW_RST,
+				   IMX6Q_GPR1_PCIE_SW_RST);
+		break;
+	case IMX6Q:
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
+				   IMX6Q_GPR1_PCIE_TEST_PD, 1 << 18);
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
+				   IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
+		break;
+	}
+
+	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
+		int ret = regulator_disable(imx6_pcie->vpcie);
+
+		if (ret)
+			dev_err(dev, "failed to disable vpcie regulator: %d\n",
+				ret);
+	}
+
+	/* Some boards don't have PCIe reset GPIO. */
+	if (gpio_is_valid(imx6_pcie->reset_gpio))
+		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
+					imx6_pcie->gpio_active_high);
+}
+
 static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 {
 	struct dw_pcie *pci = imx6_pcie->pci;
-- 
2.25.1

[PATCH v14 04/17] PCI: imx6: Move imx6_pcie_clk_disable() earlier

[Richard Zhu]

Move imx6_pcie_clk_disable() earlier to be near other clock-related
functions.  No functional change intended.

[bhelgaas: reorder patch so pure moves are earlier]
Link: https://lore.kernel.org/r/1655189942-12678-4-git-send-email-hongxing.z
hu@nxp.com
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/pci/controller/dwc/pci-imx6.c | 48 +++++++++++++--------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index a6d2b907d42b..38f208eea2d7 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -580,6 +580,30 @@ static int imx6_pcie_enable_ref_clk(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:
+	case IMX8MM:
+		clk_disable_unprepare(imx6_pcie->pcie_aux);
+		break;
+	default:
+		break;
+	}
+}
+
 static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 {
 	struct device *dev = imx6_pcie->pci->dev;
@@ -941,30 +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:
-	case IMX8MM:
-		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

[PATCH v14 05/17] PCI: imx6: Factor out ref clock disable to match enable

[Richard Zhu]

From: Bjorn Helgaas <bhelgaas@google.com>

The PCIe ref clocks are specific to different variants.  The enables are
already split out into imx6_pcie_enable_ref_clk(), but the disables were
combined with the more generic bus/phy/pcie clock disables in
imx6_pcie_clk_disable().

Split out the variant-specific disables into imx6_pcie_disable_ref_clk() to
match imx6_pcie_enable_ref_clk().

No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 38f208eea2d7..f458461880dc 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -580,12 +580,8 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
 	return ret;
 }
 
-static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
+static void imx6_pcie_disable_ref_clk(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);
@@ -595,8 +591,8 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
 				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
 				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
 		break;
-	case IMX8MQ:
 	case IMX8MM:
+	case IMX8MQ:
 		clk_disable_unprepare(imx6_pcie->pcie_aux);
 		break;
 	default:
@@ -604,6 +600,14 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
 	}
 }
 
+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);
+	imx6_pcie_disable_ref_clk(imx6_pcie);
+}
+
 static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 {
 	struct device *dev = imx6_pcie->pci->dev;
-- 
2.25.1

[PATCH v14 06/17] PCI: imx6: Collect clock enables in imx6_pcie_clk_enable()

[Richard Zhu]

Encapsulate the i.MX PCIe clock enable operations into one standalone
function, imx6_pcie_clk_enable().  No functional change intended.

[bhelgaas: split pure code moves into separate patches]
Link: https://lore.kernel.org/r/1655189942-12678-2-git-send-email-hongxing.zhu@nxp.com
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/pci/controller/dwc/pci-imx6.c | 95 ++++++++++++++++-----------
 1 file changed, 56 insertions(+), 39 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index f458461880dc..ff5829e42ea7 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -600,6 +600,58 @@ static void imx6_pcie_disable_ref_clk(struct imx6_pcie *imx6_pcie)
 	}
 }
 
+static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
+{
+	struct dw_pcie *pci = imx6_pcie->pci;
+	struct device *dev = pci->dev;
+	int ret;
+
+	ret = clk_prepare_enable(imx6_pcie->pcie_phy);
+	if (ret) {
+		dev_err(dev, "unable to enable pcie_phy clock\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(imx6_pcie->pcie_bus);
+	if (ret) {
+		dev_err(dev, "unable to enable pcie_bus clock\n");
+		goto err_pcie_bus;
+	}
+
+	ret = clk_prepare_enable(imx6_pcie->pcie);
+	if (ret) {
+		dev_err(dev, "unable to enable pcie clock\n");
+		goto err_pcie;
+	}
+
+	ret = imx6_pcie_enable_ref_clk(imx6_pcie);
+	if (ret) {
+		dev_err(dev, "unable to enable pcie ref clock\n");
+		goto err_ref_clk;
+	}
+
+	switch (imx6_pcie->drvdata->variant) {
+	case IMX8MM:
+		if (phy_power_on(imx6_pcie->phy))
+			dev_err(dev, "unable to power on PHY\n");
+		break;
+	default:
+		break;
+	}
+	/* allow the clocks to stabilize */
+	usleep_range(200, 500);
+	return 0;
+
+err_ref_clk:
+	clk_disable_unprepare(imx6_pcie->pcie);
+err_pcie:
+	clk_disable_unprepare(imx6_pcie->pcie_bus);
+err_pcie_bus:
+	clk_disable_unprepare(imx6_pcie->pcie_phy);
+
+	return ret;
+}
+
 static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
 {
 	clk_disable_unprepare(imx6_pcie->pcie);
@@ -671,40 +723,11 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 		}
 	}
 
-	ret = clk_prepare_enable(imx6_pcie->pcie_phy);
-	if (ret) {
-		dev_err(dev, "unable to enable pcie_phy clock\n");
-		goto err_pcie_phy;
-	}
-
-	ret = clk_prepare_enable(imx6_pcie->pcie_bus);
-	if (ret) {
-		dev_err(dev, "unable to enable pcie_bus clock\n");
-		goto err_pcie_bus;
-	}
-
-	ret = clk_prepare_enable(imx6_pcie->pcie);
-	if (ret) {
-		dev_err(dev, "unable to enable pcie clock\n");
-		goto err_pcie;
-	}
-
-	ret = imx6_pcie_enable_ref_clk(imx6_pcie);
+	ret = imx6_pcie_clk_enable(imx6_pcie);
 	if (ret) {
-		dev_err(dev, "unable to enable pcie ref clock\n");
-		goto err_ref_clk;
-	}
-
-	switch (imx6_pcie->drvdata->variant) {
-	case IMX8MM:
-		if (phy_power_on(imx6_pcie->phy))
-			dev_err(dev, "unable to power on PHY\n");
-		break;
-	default:
-		break;
+		dev_err(dev, "unable to enable pcie clocks: %d\n", ret);
+		goto err_clks;
 	}
-	/* allow the clocks to stabilize */
-	usleep_range(200, 500);
 
 	switch (imx6_pcie->drvdata->variant) {
 	case IMX8MQ:
@@ -763,13 +786,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 
 	return;
 
-err_ref_clk:
-	clk_disable_unprepare(imx6_pcie->pcie);
-err_pcie:
-	clk_disable_unprepare(imx6_pcie->pcie_bus);
-err_pcie_bus:
-	clk_disable_unprepare(imx6_pcie->pcie_phy);
-err_pcie_phy:
+err_clks:
 	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
 		ret = regulator_disable(imx6_pcie->vpcie);
 		if (ret)
-- 
2.25.1

[PATCH v14 07/17] PCI: imx6: Propagate .host_init() errors to caller

[Richard Zhu]

Since dw_pcie_host_init() checks for errors from ops->host_init(),
check for errors when enabling power regulators and clocks and return them.

[bhelgaas: commit log]
Link: https://lore.kernel.org/r/1655189942-12678-3-git-send-email-hongxing.zhu@nxp.com
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/pci/controller/dwc/pci-imx6.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index ff5829e42ea7..78b839e92620 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -708,7 +708,7 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 					imx6_pcie->gpio_active_high);
 }
 
-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;
@@ -719,7 +719,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 		if (ret) {
 			dev_err(dev, "failed to enable vpcie regulator: %d\n",
 				ret);
-			return;
+			return ret;
 		}
 	}
 
@@ -784,7 +784,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 		msleep(100);
 	}
 
-	return;
+	return 0;
 
 err_clks:
 	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
@@ -793,6 +793,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 ret;
 }
 
 static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie)
@@ -911,11 +912,18 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 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 deassert core reset failed: %d\n", ret);
+		return ret;
+	}
+
 	imx6_setup_phy_mpll(imx6_pcie);
 
 	return 0;
-- 
2.25.1

[PATCH v14 08/17] PCI: imx6: Disable i.MX6QDL clock when disabling ref clocks

[Richard Zhu]

When disabling PCIe clocks, disable i.MX6QDL ref clock too.

Link: https://lore.kernel.org/r/1655189942-12678-5-git-send-email-hongxing.zhu@nxp.com
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---
 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

[PATCH v14 09/17] PCI: imx6: Call host init function directly in resume

[Richard Zhu]

Call imx6_pcie_host_init() instead of duplicating codes in resume.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 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

[PATCH v14 10/17] PCI: imx6: Turn off regulator when system is in suspend mode

[Richard Zhu]

The driver should undo any enables it did itself. The regulator disable
shouldn't be basing decisions on regulator_is_enabled().

Move the regulator_disable to the suspend function, turn off regulator when
the system is in suspend mode.

To keep the balance of the regulator usage counter, disable the regulator
in shutdown.

Link: https://lore.kernel.org/r/1655189942-12678-6-git-send-email-hongxing.z
hu@nxp.com
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 2b42c37f1617..f72eb609769b 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -670,8 +670,6 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
 
 static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 {
-	struct device *dev = imx6_pcie->pci->dev;
-
 	switch (imx6_pcie->drvdata->variant) {
 	case IMX7D:
 	case IMX8MQ:
@@ -702,14 +700,6 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 		break;
 	}
 
-	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
-		int ret = regulator_disable(imx6_pcie->vpcie);
-
-		if (ret)
-			dev_err(dev, "failed to disable vpcie regulator: %d\n",
-				ret);
-	}
-
 	/* Some boards don't have PCIe reset GPIO. */
 	if (gpio_is_valid(imx6_pcie->reset_gpio))
 		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
@@ -722,7 +712,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	struct device *dev = pci->dev;
 	int ret;
 
-	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
+	if (imx6_pcie->vpcie) {
 		ret = regulator_enable(imx6_pcie->vpcie);
 		if (ret) {
 			dev_err(dev, "failed to enable vpcie regulator: %d\n",
@@ -795,7 +785,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	return 0;
 
 err_clks:
-	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
+	if (imx6_pcie->vpcie) {
 		ret = regulator_disable(imx6_pcie->vpcie);
 		if (ret)
 			dev_err(dev, "failed to disable vpcie regulator: %d\n",
@@ -1022,6 +1012,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
 		break;
 	}
 
+	if (imx6_pcie->vpcie)
+		regulator_disable(imx6_pcie->vpcie);
+
 	return 0;
 }
 
@@ -1268,6 +1261,8 @@ static void imx6_pcie_shutdown(struct platform_device *pdev)
 
 	/* bring down link, so bootloader gets clean state in case of reboot */
 	imx6_pcie_assert_core_reset(imx6_pcie);
+	if (imx6_pcie->vpcie)
+		regulator_disable(imx6_pcie->vpcie);
 }
 
 static const struct imx6_pcie_drvdata drvdata[] = {
-- 
2.25.1

[PATCH v14 11/17] PCI: imx6: Move regulator enable out of imx6_pcie_deassert_core_reset()

[Richard Zhu]

Move regulator enable out of imx6_pcie_deassert_core_reset(), since the
regulator_enable() has nothing to do in with
imx6_pcie_deassert_core_reset().

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 36 ++++++++++++---------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index f72eb609769b..0b168f0d57b8 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -712,19 +712,10 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	struct device *dev = pci->dev;
 	int ret;
 
-	if (imx6_pcie->vpcie) {
-		ret = regulator_enable(imx6_pcie->vpcie);
-		if (ret) {
-			dev_err(dev, "failed to enable vpcie regulator: %d\n",
-				ret);
-			return ret;
-		}
-	}
-
 	ret = imx6_pcie_clk_enable(imx6_pcie);
 	if (ret) {
 		dev_err(dev, "unable to enable pcie clocks: %d\n", ret);
-		goto err_clks;
+		return ret;
 	}
 
 	switch (imx6_pcie->drvdata->variant) {
@@ -783,15 +774,6 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	}
 
 	return 0;
-
-err_clks:
-	if (imx6_pcie->vpcie) {
-		ret = regulator_disable(imx6_pcie->vpcie);
-		if (ret)
-			dev_err(dev, "failed to disable vpcie regulator: %d\n",
-				ret);
-	}
-	return ret;
 }
 
 static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie)
@@ -916,15 +898,29 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
 
 	imx6_pcie_assert_core_reset(imx6_pcie);
 	imx6_pcie_init_phy(imx6_pcie);
+	if (imx6_pcie->vpcie) {
+		ret = regulator_enable(imx6_pcie->vpcie);
+		if (ret) {
+			dev_err(dev, "failed to enable vpcie regulator: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
 	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
 	if (ret < 0) {
 		dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
-		return ret;
+		goto err_reg_disable;
 	}
 
 	imx6_setup_phy_mpll(imx6_pcie);
 
 	return 0;
+
+err_reg_disable:
+	if (imx6_pcie->vpcie)
+		regulator_disable(imx6_pcie->vpcie);
+	return ret;
 }
 
 static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
-- 
2.25.1

[PATCH v14 12/17] PCI: imx6: Mark the link down as non-fatal error

[Richard Zhu]

Let the driver probe successfully, return zero in imx6_pcie_start_link()
when PCIe link is down.

Link: https://lore.kernel.org/r/1655189942-12678-7-git-send-email-hongxing.zhu@nxp.com
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 0b168f0d57b8..e236f824c808 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -836,7 +836,9 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 	/* Start LTSSM. */
 	imx6_pcie_ltssm_enable(dev);
 
-	dw_pcie_wait_for_link(pci);
+	ret = dw_pcie_wait_for_link(pci);
+	if (ret)
+		goto err_reset_phy;
 
 	if (pci->link_gen == 2) {
 		/* Allow Gen2 mode after the link is up. */
@@ -872,7 +874,9 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 		}
 
 		/* Make sure link training is finished as well! */
-		dw_pcie_wait_for_link(pci);
+		ret = dw_pcie_wait_for_link(pci);
+		if (ret)
+			goto err_reset_phy;
 	} else {
 		dev_info(dev, "Link: Gen2 disabled\n");
 	}
@@ -886,7 +890,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);
-	return ret;
+	return 0;
 }
 
 static int imx6_pcie_host_init(struct pcie_port *pp)
-- 
2.25.1

[PATCH v14 13/17] PCI: imx6: Reduce resume time by only starting link if it was up before suspend

[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 <hongxing.zhu@nxp.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 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

[PATCH v14 14/17] PCI: imx6: Do not hide phy driver callbacks and refine the error handling

[Richard Zhu]

- Move the phy_power_on() to host_init from imx6_pcie_clk_enable().
- Move the phy_init() to host_init from imx6_pcie_deassert_core_reset().

Refine the error handling in imx6_pcie_host_init() accordingly.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 34 +++++++++++++++++----------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 5a06fbca82d6..0b2a5256fb0d 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -639,14 +639,6 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
 		goto err_ref_clk;
 	}
 
-	switch (imx6_pcie->drvdata->variant) {
-	case IMX8MM:
-		if (phy_power_on(imx6_pcie->phy))
-			dev_err(dev, "unable to power on PHY\n");
-		break;
-	default:
-		break;
-	}
 	/* allow the clocks to stabilize */
 	usleep_range(200, 500);
 	return 0;
@@ -723,10 +715,6 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	case IMX8MQ:
 		reset_control_deassert(imx6_pcie->pciephy_reset);
 		break;
-	case IMX8MM:
-		if (phy_init(imx6_pcie->phy))
-			dev_err(dev, "waiting for phy ready timeout!\n");
-		break;
 	case IMX7D:
 		reset_control_deassert(imx6_pcie->pciephy_reset);
 
@@ -762,6 +750,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 		usleep_range(200, 500);
 		break;
 	case IMX6Q:		/* Nothing to do */
+	case IMX8MM:
 		break;
 	}
 
@@ -913,17 +902,36 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
 			return ret;
 		}
 	}
+	if (imx6_pcie->phy) {
+		ret = phy_power_on(imx6_pcie->phy);
+		if (ret) {
+			dev_err(dev, "pcie phy power up failed.\n");
+			goto err_reg_disable;
+		}
+	}
 
 	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
 	if (ret < 0) {
 		dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
-		goto err_reg_disable;
+		goto err_phy_off;
 	}
 
+	if (imx6_pcie->phy) {
+		ret = phy_init(imx6_pcie->phy);
+		if (ret) {
+			dev_err(dev, "waiting for phy ready timeout!\n");
+			goto err_clk_disable;
+		}
+	}
 	imx6_setup_phy_mpll(imx6_pcie);
 
 	return 0;
 
+err_clk_disable:
+	imx6_pcie_clk_disable(imx6_pcie);
+err_phy_off:
+	if (imx6_pcie->phy)
+		phy_power_off(imx6_pcie->phy);
 err_reg_disable:
 	if (imx6_pcie->vpcie)
 		regulator_disable(imx6_pcie->vpcie);
-- 
2.25.1

[PATCH v14 15/17] PCI: imx6: Disable clocks in reverse order of enable

[Richard Zhu]

From: Bjorn Helgaas <bhelgaas@google.com>

imx6_pcie_clk_enable() enables clocks in the order:

  pcie_phy
  pcie_bus
  pcie
  imx6_pcie_enable_ref_clk

Change imx6_pcie_clk_disable() to disable them in the reverse order.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 0b2a5256fb0d..79a05e190016 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -655,10 +655,10 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
 
 static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
 {
+	imx6_pcie_disable_ref_clk(imx6_pcie);
 	clk_disable_unprepare(imx6_pcie->pcie);
-	clk_disable_unprepare(imx6_pcie->pcie_phy);
 	clk_disable_unprepare(imx6_pcie->pcie_bus);
-	imx6_pcie_disable_ref_clk(imx6_pcie);
+	clk_disable_unprepare(imx6_pcie->pcie_phy);
 }
 
 static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
-- 
2.25.1

[PATCH v14 16/17] PCI: imx6: Move the imx6_pcie_ltssm_disable() earlier

[Richard Zhu]

Move the imx6_pcie_ltssm_disable() earlier and place it just behind the
imx6_pcie_ltssm_enable(), since it might not be only used by suspend
callback directly.
To be symmetric with imx6_pcie_ltssm_enable(), add the IMX6Q switch
case in the imx6_pcie_ltssm_disable().

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 39 ++++++++++++++-------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 79a05e190016..1cf8bf9035f2 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -805,6 +805,26 @@ static void imx6_pcie_ltssm_enable(struct device *dev)
 	}
 }
 
+static void imx6_pcie_ltssm_disable(struct device *dev)
+{
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+
+	switch (imx6_pcie->drvdata->variant) {
+	case IMX6Q:
+	case IMX6SX:
+	case IMX6QP:
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+				   IMX6Q_GPR12_PCIE_CTL_2, 0);
+		break;
+	case IMX7D:
+	case IMX8MM:
+		reset_control_assert(imx6_pcie->apps_reset);
+		break;
+	default:
+		dev_err(dev, "ltssm_disable not supported\n");
+	}
+}
+
 static int imx6_pcie_start_link(struct dw_pcie *pci)
 {
 	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
@@ -947,25 +967,6 @@ static const struct dw_pcie_ops dw_pcie_ops = {
 };
 
 #ifdef CONFIG_PM_SLEEP
-static void imx6_pcie_ltssm_disable(struct device *dev)
-{
-	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
-
-	switch (imx6_pcie->drvdata->variant) {
-	case IMX6SX:
-	case IMX6QP:
-		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
-				   IMX6Q_GPR12_PCIE_CTL_2, 0);
-		break;
-	case IMX7D:
-	case IMX8MM:
-		reset_control_assert(imx6_pcie->apps_reset);
-		break;
-	default:
-		dev_err(dev, "ltssm_disable not supported\n");
-	}
-}
-
 static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
 {
 	struct device *dev = imx6_pcie->pci->dev;
-- 
2.25.1

[PATCH v14 17/17] PCI: imx6: Reformat suspend callback to keep symmetric with resume

[Richard Zhu]

Create imx6_pcie_stop_link() and imx6_pcie_host_exit() functions.
Encapsulate clocks, regulators disables and PHY uninitialization into
imx6_pcie_host_exit().
To keep suspend/resume symmetric as much as possible, invoke these two
new created functions in suspend callback.

To be symmetric with imx6_pcie_host_exit(), move imx6_pcie_clk_enable()
to imx6_pcie_host_init() from imx6_pcie_deassert_core_reset().

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 60 ++++++++++++++++-----------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 1cf8bf9035f2..bf8992a6c238 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -703,13 +703,6 @@ 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;
-
-	ret = imx6_pcie_clk_enable(imx6_pcie);
-	if (ret) {
-		dev_err(dev, "unable to enable pcie clocks: %d\n", ret);
-		return ret;
-	}
 
 	switch (imx6_pcie->drvdata->variant) {
 	case IMX8MQ:
@@ -905,6 +898,14 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 	return 0;
 }
 
+static void imx6_pcie_stop_link(struct dw_pcie *pci)
+{
+	struct device *dev = pci->dev;
+
+	/* Turn off PCIe LTSSM */
+	imx6_pcie_ltssm_disable(dev);
+}
+
 static int imx6_pcie_host_init(struct pcie_port *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -922,11 +923,17 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
 			return ret;
 		}
 	}
+	ret = imx6_pcie_clk_enable(imx6_pcie);
+	if (ret) {
+		dev_err(dev, "unable to enable pcie clocks: %d\n", ret);
+		goto err_reg_disable;
+	}
+
 	if (imx6_pcie->phy) {
 		ret = phy_power_on(imx6_pcie->phy);
 		if (ret) {
 			dev_err(dev, "pcie phy power up failed.\n");
-			goto err_reg_disable;
+			goto err_clk_disable;
 		}
 	}
 
@@ -947,17 +954,33 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
 
 	return 0;
 
-err_clk_disable:
-	imx6_pcie_clk_disable(imx6_pcie);
 err_phy_off:
 	if (imx6_pcie->phy)
 		phy_power_off(imx6_pcie->phy);
+err_clk_disable:
+	imx6_pcie_clk_disable(imx6_pcie);
 err_reg_disable:
 	if (imx6_pcie->vpcie)
 		regulator_disable(imx6_pcie->vpcie);
 	return ret;
 }
 
+static void imx6_pcie_host_exit(struct pcie_port *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
+
+	if (imx6_pcie->phy) {
+		if (phy_power_off(imx6_pcie->phy))
+			dev_err(pci->dev, "unable to power off PHY\n");
+		phy_exit(imx6_pcie->phy);
+	}
+	imx6_pcie_clk_disable(imx6_pcie);
+
+	if (imx6_pcie->vpcie)
+		regulator_disable(imx6_pcie->vpcie);
+}
+
 static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
 	.host_init = imx6_pcie_host_init,
 };
@@ -1007,25 +1030,14 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
 static int imx6_pcie_suspend_noirq(struct device *dev)
 {
 	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+	struct pcie_port *pp = &imx6_pcie->pci->pp;
 
 	if (!(imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_SUPPORTS_SUSPEND))
 		return 0;
 
 	imx6_pcie_pm_turnoff(imx6_pcie);
-	imx6_pcie_ltssm_disable(dev);
-	imx6_pcie_clk_disable(imx6_pcie);
-	switch (imx6_pcie->drvdata->variant) {
-	case IMX8MM:
-		if (phy_power_off(imx6_pcie->phy))
-			dev_err(dev, "unable to power off PHY\n");
-		phy_exit(imx6_pcie->phy);
-		break;
-	default:
-		break;
-	}
-
-	if (imx6_pcie->vpcie)
-		regulator_disable(imx6_pcie->vpcie);
+	imx6_pcie_stop_link(imx6_pcie->pci);
+	imx6_pcie_host_exit(pp);
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH v14 0/17] PCI: imx6: refine codes and add the error propagation

[Bjorn Helgaas]

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 <l.stach@pengutronix.de>" 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/1641368602-20401-6-git-send-email-hongxing.zhu@nxp.com/
>   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 <l.stach@pengutronix.de>" 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!

Re: [PATCH v14 01/17] PCI: imx6: Move imx6_pcie_grp_offset(), imx6_pcie_configure_type() earlier

[Lucas Stach]

Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Move imx6_pcie_grp_offset() and imx6_pcie_configure_type() earlier in the
> file since they depend on nothing and are used by several other functions
> that will be moved earlier.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Acked-by: Richard Zhu <hongxing.zhu@nxp.com>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 50 +++++++++++++--------------
>  1 file changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 7a285fb0f619..8653ca8cbfb9 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -146,6 +146,31 @@ struct imx6_pcie {
>  #define PHY_RX_OVRD_IN_LO_RX_DATA_EN		BIT(5)
>  #define PHY_RX_OVRD_IN_LO_RX_PLL_EN		BIT(3)
>  
> +static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
> +{
> +	WARN_ON(imx6_pcie->drvdata->variant != IMX8MQ &&
> +		imx6_pcie->drvdata->variant != IMX8MM);
> +	return imx6_pcie->controller_id == 1 ? IOMUXC_GPR16 : IOMUXC_GPR14;
> +}
> +
> +static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
> +{
> +	unsigned int mask, val;
> +
> +	if (imx6_pcie->drvdata->variant == IMX8MQ &&
> +	    imx6_pcie->controller_id == 1) {
> +		mask = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE;
> +		val  = FIELD_PREP(IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
> +				  PCI_EXP_TYPE_ROOT_PORT);
> +	} else {
> +		mask = IMX6Q_GPR12_DEVICE_TYPE;
> +		val  = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE,
> +				  PCI_EXP_TYPE_ROOT_PORT);
> +	}
> +
> +	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, mask, val);
> +}
> +
>  static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
>  {
>  	struct dw_pcie *pci = imx6_pcie->pci;
> @@ -415,13 +440,6 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
>  					imx6_pcie->gpio_active_high);
>  }
>  
> -static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
> -{
> -	WARN_ON(imx6_pcie->drvdata->variant != IMX8MQ &&
> -		imx6_pcie->drvdata->variant != IMX8MM);
> -	return imx6_pcie->controller_id == 1 ? IOMUXC_GPR16 : IOMUXC_GPR14;
> -}
> -
>  static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
>  {
>  	struct dw_pcie *pci = imx6_pcie->pci;
> @@ -617,24 +635,6 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  	}
>  }
>  
> -static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
> -{
> -	unsigned int mask, val;
> -
> -	if (imx6_pcie->drvdata->variant == IMX8MQ &&
> -	    imx6_pcie->controller_id == 1) {
> -		mask   = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE;
> -		val    = FIELD_PREP(IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
> -				    PCI_EXP_TYPE_ROOT_PORT);
> -	} else {
> -		mask = IMX6Q_GPR12_DEVICE_TYPE;
> -		val  = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE,
> -				  PCI_EXP_TYPE_ROOT_PORT);
> -	}
> -
> -	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, mask, val);
> -}
> -
>  static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
>  {
>  	switch (imx6_pcie->drvdata->variant) {

Re: [PATCH v14 02/17] PCI: imx6: Move PHY management functions together

[Lucas Stach]

Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> 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 <bhelgaas@google.com>
> Acked-by: Richard Zhu <hongxing.zhu@nxp.com>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

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

Re: [PATCH v14 03/17] PCI: imx6: Move imx6_pcie_enable_ref_clk() earlier

[Lucas Stach]

Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Move imx6_pcie_enable_ref_clk() earlier so it's not in the middle between
> imx6_pcie_assert_core_reset() and imx6_pcie_deassert_core_reset().  No
> functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Acked-by: Richard Zhu <hongxing.zhu@nxp.com>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 96 +++++++++++++--------------
>  1 file changed, 48 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index e63eb6380020..a6d2b907d42b 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -520,54 +520,6 @@ static int imx6_pcie_attach_pd(struct device *dev)
>  	return 0;
>  }
>  
> -static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> -{
> -	struct device *dev = imx6_pcie->pci->dev;
> -
> -	switch (imx6_pcie->drvdata->variant) {
> -	case IMX7D:
> -	case IMX8MQ:
> -		reset_control_assert(imx6_pcie->pciephy_reset);
> -		fallthrough;
> -	case IMX8MM:
> -		reset_control_assert(imx6_pcie->apps_reset);
> -		break;
> -	case IMX6SX:
> -		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> -				   IMX6SX_GPR12_PCIE_TEST_POWERDOWN,
> -				   IMX6SX_GPR12_PCIE_TEST_POWERDOWN);
> -		/* Force PCIe PHY reset */
> -		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR5,
> -				   IMX6SX_GPR5_PCIE_BTNRST_RESET,
> -				   IMX6SX_GPR5_PCIE_BTNRST_RESET);
> -		break;
> -	case IMX6QP:
> -		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> -				   IMX6Q_GPR1_PCIE_SW_RST,
> -				   IMX6Q_GPR1_PCIE_SW_RST);
> -		break;
> -	case IMX6Q:
> -		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> -				   IMX6Q_GPR1_PCIE_TEST_PD, 1 << 18);
> -		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> -				   IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
> -		break;
> -	}
> -
> -	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> -		int ret = regulator_disable(imx6_pcie->vpcie);
> -
> -		if (ret)
> -			dev_err(dev, "failed to disable vpcie regulator: %d\n",
> -				ret);
> -	}
> -
> -	/* Some boards don't have PCIe reset GPIO. */
> -	if (gpio_is_valid(imx6_pcie->reset_gpio))
> -		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> -					imx6_pcie->gpio_active_high);
> -}
> -
>  static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
>  {
>  	struct dw_pcie *pci = imx6_pcie->pci;
> @@ -628,6 +580,54 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
>  	return ret;
>  }
>  
> +static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> +{
> +	struct device *dev = imx6_pcie->pci->dev;
> +
> +	switch (imx6_pcie->drvdata->variant) {
> +	case IMX7D:
> +	case IMX8MQ:
> +		reset_control_assert(imx6_pcie->pciephy_reset);
> +		fallthrough;
> +	case IMX8MM:
> +		reset_control_assert(imx6_pcie->apps_reset);
> +		break;
> +	case IMX6SX:
> +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> +				   IMX6SX_GPR12_PCIE_TEST_POWERDOWN,
> +				   IMX6SX_GPR12_PCIE_TEST_POWERDOWN);
> +		/* Force PCIe PHY reset */
> +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR5,
> +				   IMX6SX_GPR5_PCIE_BTNRST_RESET,
> +				   IMX6SX_GPR5_PCIE_BTNRST_RESET);
> +		break;
> +	case IMX6QP:
> +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> +				   IMX6Q_GPR1_PCIE_SW_RST,
> +				   IMX6Q_GPR1_PCIE_SW_RST);
> +		break;
> +	case IMX6Q:
> +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> +				   IMX6Q_GPR1_PCIE_TEST_PD, 1 << 18);
> +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> +				   IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
> +		break;
> +	}
> +
> +	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> +		int ret = regulator_disable(imx6_pcie->vpcie);
> +
> +		if (ret)
> +			dev_err(dev, "failed to disable vpcie regulator: %d\n",
> +				ret);
> +	}
> +
> +	/* Some boards don't have PCIe reset GPIO. */
> +	if (gpio_is_valid(imx6_pcie->reset_gpio))
> +		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> +					imx6_pcie->gpio_active_high);
> +}
> +
>  static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  {
>  	struct dw_pcie *pci = imx6_pcie->pci;
> -- 
> 2.25.1
> 

Re: [PATCH v14 05/17] PCI: imx6: Factor out ref clock disable to match enable

[Lucas Stach]

Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> The PCIe ref clocks are specific to different variants.  The enables are
> already split out into imx6_pcie_enable_ref_clk(), but the disables were
> combined with the more generic bus/phy/pcie clock disables in
> imx6_pcie_clk_disable().
> 
> Split out the variant-specific disables into imx6_pcie_disable_ref_clk() to
> match imx6_pcie_enable_ref_clk().
> 
> No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Acked-by: Richard Zhu <hongxing.zhu@nxp.com>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 38f208eea2d7..f458461880dc 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -580,12 +580,8 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
>  	return ret;
>  }
>  
> -static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
> +static void imx6_pcie_disable_ref_clk(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);
> @@ -595,8 +591,8 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
>  				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
>  				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
>  		break;
> -	case IMX8MQ:
>  	case IMX8MM:
> +	case IMX8MQ:
>  		clk_disable_unprepare(imx6_pcie->pcie_aux);
>  		break;
>  	default:
> @@ -604,6 +600,14 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
>  	}
>  }
>  
> +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);
> +	imx6_pcie_disable_ref_clk(imx6_pcie);
> +}
> +
>  static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
>  {
>  	struct device *dev = imx6_pcie->pci->dev;

Re: [PATCH v14 09/17] PCI: imx6: Call host init function directly in resume

[Lucas Stach]

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 <hongxing.zhu@nxp.com>

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 <l.stach@pengutronix.de>

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

Re: [PATCH v14 10/17] PCI: imx6: Turn off regulator when system is in suspend mode

[Lucas Stach]

Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> The driver should undo any enables it did itself. The regulator disable
> shouldn't be basing decisions on regulator_is_enabled().
> 
> Move the regulator_disable to the suspend function, turn off regulator when
> the system is in suspend mode.
> 
> To keep the balance of the regulator usage counter, disable the regulator
> in shutdown.
> 
> Link: https://lore.kernel.org/r/1655189942-12678-6-git-send-email-hongxing.z
> hu@nxp.com
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 2b42c37f1617..f72eb609769b 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -670,8 +670,6 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
>  
>  static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
>  {
> -	struct device *dev = imx6_pcie->pci->dev;
> -
>  	switch (imx6_pcie->drvdata->variant) {
>  	case IMX7D:
>  	case IMX8MQ:
> @@ -702,14 +700,6 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
>  		break;
>  	}
>  
> -	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> -		int ret = regulator_disable(imx6_pcie->vpcie);
> -
> -		if (ret)
> -			dev_err(dev, "failed to disable vpcie regulator: %d\n",
> -				ret);
> -	}
> -
>  	/* Some boards don't have PCIe reset GPIO. */
>  	if (gpio_is_valid(imx6_pcie->reset_gpio))
>  		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> @@ -722,7 +712,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  	struct device *dev = pci->dev;
>  	int ret;
>  
> -	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
> +	if (imx6_pcie->vpcie) {
>  		ret = regulator_enable(imx6_pcie->vpcie);
>  		if (ret) {
>  			dev_err(dev, "failed to enable vpcie regulator: %d\n",

The regulator really has nothing to do with the core reset. Please move
this regulator enable into imx6_pcie_host_init().

> @@ -795,7 +785,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  	return 0;
>  
>  err_clks:
> -	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> +	if (imx6_pcie->vpcie) {
>  		ret = regulator_disable(imx6_pcie->vpcie);
>  		if (ret)
>  			dev_err(dev, "failed to disable vpcie regulator: %d\n",
> @@ -1022,6 +1012,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
>  		break;
>  	}
>  
> +	if (imx6_pcie->vpcie)
> +		regulator_disable(imx6_pcie->vpcie);
> +
>  	return 0;
>  }
>  
> @@ -1268,6 +1261,8 @@ static void imx6_pcie_shutdown(struct platform_device *pdev)
>  
>  	/* bring down link, so bootloader gets clean state in case of reboot */
>  	imx6_pcie_assert_core_reset(imx6_pcie);
> +	if (imx6_pcie->vpcie)
> +		regulator_disable(imx6_pcie->vpcie);

This looks like a separate change, not mentioned in the commit message.
I'm not sure if we should do this. Shutdown is supposed to just stop
the device, which is already achieved by
imx6_pcie_assert_core_reset(). 

If we would want to do a full cleanup here we would also need to
disable clocks and get the reset GPIO into asserted state. I don't
think we want to do all of this here.

Regards,
Lucas
>  }
>  
>  static const struct imx6_pcie_drvdata drvdata[] = {

Re: [PATCH v14 11/17] PCI: imx6: Move regulator enable out of imx6_pcie_deassert_core_reset()

[Lucas Stach]

Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> Move regulator enable out of imx6_pcie_deassert_core_reset(), since the
> regulator_enable() has nothing to do in with
> imx6_pcie_deassert_core_reset().
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>

Ah, so you are doing things in two steps. Disregard my first comment on
the last patch then.

> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 36 ++++++++++++---------------
>  1 file changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index f72eb609769b..0b168f0d57b8 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -712,19 +712,10 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  	struct device *dev = pci->dev;
>  	int ret;
>  
> -	if (imx6_pcie->vpcie) {
> -		ret = regulator_enable(imx6_pcie->vpcie);
> -		if (ret) {
> -			dev_err(dev, "failed to enable vpcie regulator: %d\n",
> -				ret);
> -			return ret;
> -		}
> -	}
> -
>  	ret = imx6_pcie_clk_enable(imx6_pcie);
>  	if (ret) {
>  		dev_err(dev, "unable to enable pcie clocks: %d\n", ret);
> -		goto err_clks;
> +		return ret;
>  	}
>  
>  	switch (imx6_pcie->drvdata->variant) {
> @@ -783,15 +774,6 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  	}
>  
>  	return 0;
> -
> -err_clks:
> -	if (imx6_pcie->vpcie) {
> -		ret = regulator_disable(imx6_pcie->vpcie);
> -		if (ret)
> -			dev_err(dev, "failed to disable vpcie regulator: %d\n",
> -				ret);
> -	}
> -	return ret;
>  }
>  
>  static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie)
> @@ -916,15 +898,29 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
>  
>  	imx6_pcie_assert_core_reset(imx6_pcie);
>  	imx6_pcie_init_phy(imx6_pcie);
> +	if (imx6_pcie->vpcie) {
> +		ret = regulator_enable(imx6_pcie->vpcie);
> +		if (ret) {
> +			dev_err(dev, "failed to enable vpcie regulator: %d\n",
> +				ret);
> +			return ret;

If the regulator enable fails, you don't roll back the PHY init and
core reset. This seems harmless now, but might have unintended
consequences if the PHY code changes. I think it should be safe to move
the regulator enable before the PHY init and core reset assert to avoid
introducing more failure cleanup paths here.

Regards,
Lucas

> +		}
> +	}
> +
>  	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
>  	if (ret < 0) {
>  		dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
> -		return ret;
> +		goto err_reg_disable;
>  	}
>  
>  	imx6_setup_phy_mpll(imx6_pcie);
>  
>  	return 0;
> +
> +err_reg_disable:
> +	if (imx6_pcie->vpcie)
> +		regulator_disable(imx6_pcie->vpcie);
> +	return ret;
>  }
>  
>  static const struct dw_pcie_host_ops imx6_pcie_host_ops = {

Re: [PATCH v14 12/17] PCI: imx6: Mark the link down as non-fatal error

[Lucas Stach]

Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> Let the driver probe successfully, return zero in imx6_pcie_start_link()
> when PCIe link is down.
> 
> Link: https://lore.kernel.org/r/1655189942-12678-7-git-send-email-hongxing.zhu@nxp.com
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 0b168f0d57b8..e236f824c808 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -836,7 +836,9 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
>  	/* Start LTSSM. */
>  	imx6_pcie_ltssm_enable(dev);
>  
> -	dw_pcie_wait_for_link(pci);
> +	ret = dw_pcie_wait_for_link(pci);
> +	if (ret)
> +		goto err_reset_phy;
>  
>  	if (pci->link_gen == 2) {
>  		/* Allow Gen2 mode after the link is up. */
> @@ -872,7 +874,9 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
>  		}
>  
>  		/* Make sure link training is finished as well! */
> -		dw_pcie_wait_for_link(pci);
> +		ret = dw_pcie_wait_for_link(pci);
> +		if (ret)
> +			goto err_reset_phy;
>  	} else {
>  		dev_info(dev, "Link: Gen2 disabled\n");
>  	}
> @@ -886,7 +890,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);
> -	return ret;
> +	return 0;
>  }
>  
>  static int imx6_pcie_host_init(struct pcie_port *pp)

Re: [PATCH v14 13/17] PCI: imx6: Reduce resume time by only starting link if it was up before suspend

[Lucas Stach]

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 <hongxing.zhu@nxp.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  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;
>  }

Re: [PATCH v14 14/17] PCI: imx6: Do not hide phy driver callbacks and refine the error handling

[Lucas Stach]

Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> - Move the phy_power_on() to host_init from imx6_pcie_clk_enable().
> - Move the phy_init() to host_init from imx6_pcie_deassert_core_reset().
> 
> Refine the error handling in imx6_pcie_host_init() accordingly.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 34 +++++++++++++++++----------
>  1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 5a06fbca82d6..0b2a5256fb0d 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -639,14 +639,6 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
>  		goto err_ref_clk;
>  	}
>  
> -	switch (imx6_pcie->drvdata->variant) {
> -	case IMX8MM:
> -		if (phy_power_on(imx6_pcie->phy))
> -			dev_err(dev, "unable to power on PHY\n");
> -		break;
> -	default:
> -		break;
> -	}
>  	/* allow the clocks to stabilize */
>  	usleep_range(200, 500);
>  	return 0;
> @@ -723,10 +715,6 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  	case IMX8MQ:
>  		reset_control_deassert(imx6_pcie->pciephy_reset);
>  		break;
> -	case IMX8MM:
> -		if (phy_init(imx6_pcie->phy))
> -			dev_err(dev, "waiting for phy ready timeout!\n");
> -		break;
>  	case IMX7D:
>  		reset_control_deassert(imx6_pcie->pciephy_reset);
>  
> @@ -762,6 +750,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  		usleep_range(200, 500);
>  		break;
>  	case IMX6Q:		/* Nothing to do */
> +	case IMX8MM:
>  		break;
>  	}
>  
> @@ -913,17 +902,36 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
>  			return ret;
>  		}
>  	}
> +	if (imx6_pcie->phy) {
> +		ret = phy_power_on(imx6_pcie->phy);
> +		if (ret) {
> +			dev_err(dev, "pcie phy power up failed.\n");
> +			goto err_reg_disable;
> +		}
> +	}
>  
>  	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
>  	if (ret < 0) {
>  		dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
> -		goto err_reg_disable;
> +		goto err_phy_off;
>  	}
>  
> +	if (imx6_pcie->phy) {
> +		ret = phy_init(imx6_pcie->phy);
> +		if (ret) {
> +			dev_err(dev, "waiting for phy ready timeout!\n");
> +			goto err_clk_disable;
> +		}
> +	}

Wouldn't it be more logical to put this into imx6_pcie_init_phy()?

Regards,
Lucas

>  	imx6_setup_phy_mpll(imx6_pcie);
>  
>  	return 0;
>  
> +err_clk_disable:
> +	imx6_pcie_clk_disable(imx6_pcie);
> +err_phy_off:
> +	if (imx6_pcie->phy)
> +		phy_power_off(imx6_pcie->phy);
>  err_reg_disable:
>  	if (imx6_pcie->vpcie)
>  		regulator_disable(imx6_pcie->vpcie);

Re: [PATCH v14 15/17] PCI: imx6: Disable clocks in reverse order of enable

[Lucas Stach]

Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> imx6_pcie_clk_enable() enables clocks in the order:
> 
>   pcie_phy
>   pcie_bus
>   pcie
>   imx6_pcie_enable_ref_clk
> 
> Change imx6_pcie_clk_disable() to disable them in the reverse order.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Acked-by: Richard Zhu <hongxing.zhu@nxp.com>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 0b2a5256fb0d..79a05e190016 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -655,10 +655,10 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
>  
>  static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
>  {
> +	imx6_pcie_disable_ref_clk(imx6_pcie);
>  	clk_disable_unprepare(imx6_pcie->pcie);
> -	clk_disable_unprepare(imx6_pcie->pcie_phy);
>  	clk_disable_unprepare(imx6_pcie->pcie_bus);
> -	imx6_pcie_disable_ref_clk(imx6_pcie);
> +	clk_disable_unprepare(imx6_pcie->pcie_phy);
>  }
>  
>  static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)

Re: [PATCH v14 16/17] PCI: imx6: Move the imx6_pcie_ltssm_disable() earlier

[Lucas Stach]

Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> Move the imx6_pcie_ltssm_disable() earlier and place it just behind the
> imx6_pcie_ltssm_enable(), since it might not be only used by suspend
> callback directly.
> To be symmetric with imx6_pcie_ltssm_enable(), add the IMX6Q switch
> case in the imx6_pcie_ltssm_disable().
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 39 ++++++++++++++-------------
>  1 file changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 79a05e190016..1cf8bf9035f2 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -805,6 +805,26 @@ static void imx6_pcie_ltssm_enable(struct device *dev)
>  	}
>  }
>  
> +static void imx6_pcie_ltssm_disable(struct device *dev)
> +{
> +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> +
> +	switch (imx6_pcie->drvdata->variant) {
> +	case IMX6Q:
> +	case IMX6SX:
> +	case IMX6QP:
> +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> +				   IMX6Q_GPR12_PCIE_CTL_2, 0);
> +		break;
> +	case IMX7D:
> +	case IMX8MM:
> +		reset_control_assert(imx6_pcie->apps_reset);
> +		break;
This is missing the IMX8MQ case.

> +	default:
> +		dev_err(dev, "ltssm_disable not supported\n");

Drop the default, we want a compile time warning if a variant isn't
covered by this switch statement.

Regards,
Lucas

> +	}
> +}
> +
>  static int imx6_pcie_start_link(struct dw_pcie *pci)
>  {
>  	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
> @@ -947,25 +967,6 @@ static const struct dw_pcie_ops dw_pcie_ops = {
>  };
>  
>  #ifdef CONFIG_PM_SLEEP
> -static void imx6_pcie_ltssm_disable(struct device *dev)
> -{
> -	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> -
> -	switch (imx6_pcie->drvdata->variant) {
> -	case IMX6SX:
> -	case IMX6QP:
> -		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> -				   IMX6Q_GPR12_PCIE_CTL_2, 0);
> -		break;
> -	case IMX7D:
> -	case IMX8MM:
> -		reset_control_assert(imx6_pcie->apps_reset);
> -		break;
> -	default:
> -		dev_err(dev, "ltssm_disable not supported\n");
> -	}
> -}
> -
>  static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
>  {
>  	struct device *dev = imx6_pcie->pci->dev;

Re: [PATCH v14 17/17] PCI: imx6: Reformat suspend callback to keep symmetric with resume

[Lucas Stach]

Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> Create imx6_pcie_stop_link() and imx6_pcie_host_exit() functions.
> Encapsulate clocks, regulators disables and PHY uninitialization into
> imx6_pcie_host_exit().
> To keep suspend/resume symmetric as much as possible, invoke these two
> new created functions in suspend callback.
> 
> To be symmetric with imx6_pcie_host_exit(), move imx6_pcie_clk_enable()
> to imx6_pcie_host_init() from imx6_pcie_deassert_core_reset().
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 60 ++++++++++++++++-----------
>  1 file changed, 36 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 1cf8bf9035f2..bf8992a6c238 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -703,13 +703,6 @@ 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;
> -
> -	ret = imx6_pcie_clk_enable(imx6_pcie);
> -	if (ret) {
> -		dev_err(dev, "unable to enable pcie clocks: %d\n", ret);
> -		return ret;
> -	}
>  
>  	switch (imx6_pcie->drvdata->variant) {
>  	case IMX8MQ:
> @@ -905,6 +898,14 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
>  	return 0;
>  }
>  
> +static void imx6_pcie_stop_link(struct dw_pcie *pci)
> +{
> +	struct device *dev = pci->dev;
> +
> +	/* Turn off PCIe LTSSM */
> +	imx6_pcie_ltssm_disable(dev);
> +}
> +
>  static int imx6_pcie_host_init(struct pcie_port *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> @@ -922,11 +923,17 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
>  			return ret;
>  		}
>  	}
> +	ret = imx6_pcie_clk_enable(imx6_pcie);
> +	if (ret) {
> +		dev_err(dev, "unable to enable pcie clocks: %d\n", ret);
> +		goto err_reg_disable;
> +	}
> +
>  	if (imx6_pcie->phy) {
>  		ret = phy_power_on(imx6_pcie->phy);
>  		if (ret) {
>  			dev_err(dev, "pcie phy power up failed.\n");
> -			goto err_reg_disable;
> +			goto err_clk_disable;
>  		}
>  	}
>  
> @@ -947,17 +954,33 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
>  
>  	return 0;
>  
> -err_clk_disable:
> -	imx6_pcie_clk_disable(imx6_pcie);
>  err_phy_off:
>  	if (imx6_pcie->phy)
>  		phy_power_off(imx6_pcie->phy);
> +err_clk_disable:
> +	imx6_pcie_clk_disable(imx6_pcie);
>  err_reg_disable:
>  	if (imx6_pcie->vpcie)
>  		regulator_disable(imx6_pcie->vpcie);
>  	return ret;
>  }
>  
> +static void imx6_pcie_host_exit(struct pcie_port *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
> +
> +	if (imx6_pcie->phy) {
> +		if (phy_power_off(imx6_pcie->phy))
> +			dev_err(pci->dev, "unable to power off PHY\n");
> +		phy_exit(imx6_pcie->phy);
> +	}
> +	imx6_pcie_clk_disable(imx6_pcie);
> +
> +	if (imx6_pcie->vpcie)
> +		regulator_disable(imx6_pcie->vpcie);
> +}
> +
>  static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
>  	.host_init = imx6_pcie_host_init,
>  };
> @@ -1007,25 +1030,14 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
>  static int imx6_pcie_suspend_noirq(struct device *dev)
>  {
>  	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> +	struct pcie_port *pp = &imx6_pcie->pci->pp;
>  
>  	if (!(imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_SUPPORTS_SUSPEND))
>  		return 0;
>  
>  	imx6_pcie_pm_turnoff(imx6_pcie);
> -	imx6_pcie_ltssm_disable(dev);
> -	imx6_pcie_clk_disable(imx6_pcie);
> -	switch (imx6_pcie->drvdata->variant) {
> -	case IMX8MM:
> -		if (phy_power_off(imx6_pcie->phy))
> -			dev_err(dev, "unable to power off PHY\n");
> -		phy_exit(imx6_pcie->phy);
> -		break;
> -	default:
> -		break;
> -	}
> -
> -	if (imx6_pcie->vpcie)
> -		regulator_disable(imx6_pcie->vpcie);
> +	imx6_pcie_stop_link(imx6_pcie->pci);
> +	imx6_pcie_host_exit(pp);
>  
>  	return 0;
>  }

RE: [PATCH v14 10/17] PCI: imx6: Turn off regulator when system is in suspend mode

[Hongxing Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年7月13日 16:34
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> festevam@gmail.com; francesco.dolcini@toradex.com
> Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v14 10/17] PCI: imx6: Turn off regulator when system is in
> suspend mode
> 
> Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> > The driver should undo any enables it did itself. The regulator
> > disable shouldn't be basing decisions on regulator_is_enabled().
> >
> > Move the regulator_disable to the suspend function, turn off regulator
> > when the system is in suspend mode.
> >
> > To keep the balance of the regulator usage counter, disable the
> > regulator in shutdown.
> >
> > Link:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Fr%2F1655189942-12678-6-git-send-email-hongxing.z&amp;d
> at
> >
> a=05%7C01%7Chongxing.zhu%40nxp.com%7C361c9bf365b64895482808da6
> 4aa7cca%
> >
> 7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379329806385373
> 39%7CUnkn
> >
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> haWwi
> >
> LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=A%2Fw6AFCddC0T%2FA
> ocBT%2F7OJZ
> > ddAeYPTbgeAxpHpPubkw%3D&amp;reserved=0
> > hu@nxp.com
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++------------
> >  1 file changed, 7 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 2b42c37f1617..f72eb609769b 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -670,8 +670,6 @@ static void imx6_pcie_clk_disable(struct imx6_pcie
> > *imx6_pcie)
> >
> >  static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> > {
> > -	struct device *dev = imx6_pcie->pci->dev;
> > -
> >  	switch (imx6_pcie->drvdata->variant) {
> >  	case IMX7D:
> >  	case IMX8MQ:
> > @@ -702,14 +700,6 @@ static void imx6_pcie_assert_core_reset(struct
> imx6_pcie *imx6_pcie)
> >  		break;
> >  	}
> >
> > -	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> > -		int ret = regulator_disable(imx6_pcie->vpcie);
> > -
> > -		if (ret)
> > -			dev_err(dev, "failed to disable vpcie regulator: %d\n",
> > -				ret);
> > -	}
> > -
> >  	/* Some boards don't have PCIe reset GPIO. */
> >  	if (gpio_is_valid(imx6_pcie->reset_gpio))
> >  		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> > @@ -722,7 +712,7 @@ static int imx6_pcie_deassert_core_reset(struct
> imx6_pcie *imx6_pcie)
> >  	struct device *dev = pci->dev;
> >  	int ret;
> >
> > -	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
> > +	if (imx6_pcie->vpcie) {
> >  		ret = regulator_enable(imx6_pcie->vpcie);
> >  		if (ret) {
> >  			dev_err(dev, "failed to enable vpcie regulator: %d\n",
> 
> The regulator really has nothing to do with the core reset. Please move this
> regulator enable into imx6_pcie_host_init().
Hi Lucas:
Thanks for your comments.
Got that. Had done it in the 11/17 commit.

> 
> > @@ -795,7 +785,7 @@ static int imx6_pcie_deassert_core_reset(struct
> imx6_pcie *imx6_pcie)
> >  	return 0;
> >
> >  err_clks:
> > -	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> > +	if (imx6_pcie->vpcie) {
> >  		ret = regulator_disable(imx6_pcie->vpcie);
> >  		if (ret)
> >  			dev_err(dev, "failed to disable vpcie regulator: %d\n", @@
> -1022,6
> > +1012,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> >  		break;
> >  	}
> >
> > +	if (imx6_pcie->vpcie)
> > +		regulator_disable(imx6_pcie->vpcie);
> > +
> >  	return 0;
> >  }
> >
> > @@ -1268,6 +1261,8 @@ static void imx6_pcie_shutdown(struct
> > platform_device *pdev)
> >
> >  	/* bring down link, so bootloader gets clean state in case of reboot */
> >  	imx6_pcie_assert_core_reset(imx6_pcie);
> > +	if (imx6_pcie->vpcie)
> > +		regulator_disable(imx6_pcie->vpcie);
> 
> This looks like a separate change, not mentioned in the commit message.
> I'm not sure if we should do this. Shutdown is supposed to just stop the device,
> which is already achieved by imx6_pcie_assert_core_reset().
> 
> If we would want to do a full cleanup here we would also need to disable
> clocks and get the reset GPIO into asserted state. I don't think we want to do
> all of this here.
The regulator_disable() was contained in imx6_pcie_assert_core_reset() before.
When refine the regulator usage, the regulator_disable() is moved out of
 imx6_pcie_assert_core_reset().
Based on the discussion [1] with Francesco.
To keep the same behavior, the regulator_disable() is placed behind of
 imx6_pcie_assert_core_reset() in imx6_pcie_shutdown() here.
[1]https://patchwork.ozlabs.org/project/linux-pci/patch/1644290735-3797-6-git-send-email-hongxing.zhu@nxp.com/

Best Regards
Richard

> 
> Regards,
> Lucas
> >  }
> >
> >  static const struct imx6_pcie_drvdata drvdata[] = {
> 

RE: [PATCH v14 11/17] PCI: imx6: Move regulator enable out of imx6_pcie_deassert_core_reset()

[Hongxing Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年7月13日 16:41
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> festevam@gmail.com; francesco.dolcini@toradex.com
> Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v14 11/17] PCI: imx6: Move regulator enable out of
> imx6_pcie_deassert_core_reset()
> 
> Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> > Move regulator enable out of imx6_pcie_deassert_core_reset(), since
> > the
> > regulator_enable() has nothing to do in with
> > imx6_pcie_deassert_core_reset().
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> 
> Ah, so you are doing things in two steps. Disregard my first comment on the
> last patch then.
Thanks.

> 
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 36
> > ++++++++++++---------------
> >  1 file changed, 16 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index f72eb609769b..0b168f0d57b8 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -712,19 +712,10 @@ static int imx6_pcie_deassert_core_reset(struct
> imx6_pcie *imx6_pcie)
> >  	struct device *dev = pci->dev;
> >  	int ret;
> >
> > -	if (imx6_pcie->vpcie) {
> > -		ret = regulator_enable(imx6_pcie->vpcie);
> > -		if (ret) {
> > -			dev_err(dev, "failed to enable vpcie regulator: %d\n",
> > -				ret);
> > -			return ret;
> > -		}
> > -	}
> > -
> >  	ret = imx6_pcie_clk_enable(imx6_pcie);
> >  	if (ret) {
> >  		dev_err(dev, "unable to enable pcie clocks: %d\n", ret);
> > -		goto err_clks;
> > +		return ret;
> >  	}
> >
> >  	switch (imx6_pcie->drvdata->variant) { @@ -783,15 +774,6 @@ static
> > int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> >  	}
> >
> >  	return 0;
> > -
> > -err_clks:
> > -	if (imx6_pcie->vpcie) {
> > -		ret = regulator_disable(imx6_pcie->vpcie);
> > -		if (ret)
> > -			dev_err(dev, "failed to disable vpcie regulator: %d\n",
> > -				ret);
> > -	}
> > -	return ret;
> >  }
> >
> >  static int imx6_pcie_wait_for_speed_change(struct imx6_pcie
> > *imx6_pcie) @@ -916,15 +898,29 @@ static int
> > imx6_pcie_host_init(struct pcie_port *pp)
> >
> >  	imx6_pcie_assert_core_reset(imx6_pcie);
> >  	imx6_pcie_init_phy(imx6_pcie);
> > +	if (imx6_pcie->vpcie) {
> > +		ret = regulator_enable(imx6_pcie->vpcie);
> > +		if (ret) {
> > +			dev_err(dev, "failed to enable vpcie regulator: %d\n",
> > +				ret);
> > +			return ret;
> 
> If the regulator enable fails, you don't roll back the PHY init and core reset. This
> seems harmless now, but might have unintended consequences if the PHY
> code changes. I think it should be safe to move the regulator enable before the
> PHY init and core reset assert to avoid introducing more failure cleanup paths
> here.
To keep the same behavior, I just place the regulator_enable() in front of
 imx6_pcie_deassert_core_reset(). 
It makes sense to move it earlier to avoid the possible failure cleanup of
 imx6_pcie_assert_core_reset() and imx6_pcie_init_phy() in future.

Best Regards
Richard
> 
> Regards,
> Lucas
> 
> > +		}
> > +	}
> > +
> >  	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
> >  	if (ret < 0) {
> >  		dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
> > -		return ret;
> > +		goto err_reg_disable;
> >  	}
> >
> >  	imx6_setup_phy_mpll(imx6_pcie);
> >
> >  	return 0;
> > +
> > +err_reg_disable:
> > +	if (imx6_pcie->vpcie)
> > +		regulator_disable(imx6_pcie->vpcie);
> > +	return ret;
> >  }
> >
> >  static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
> 

RE: [PATCH v14 13/17] PCI: imx6: Reduce resume time by only starting link if it was up before suspend

[Hongxing Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年7月13日 16:48
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> festevam@gmail.com; francesco.dolcini@toradex.com
> Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> 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 <hongxing.zhu@nxp.com>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  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;
> >  }
> 

RE: [PATCH v14 14/17] PCI: imx6: Do not hide phy driver callbacks and refine the error handling

[Hongxing Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年7月13日 16:59
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> festevam@gmail.com; francesco.dolcini@toradex.com
> Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v14 14/17] PCI: imx6: Do not hide phy driver callbacks and
> refine the error handling
> 
> Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> > - Move the phy_power_on() to host_init from imx6_pcie_clk_enable().
> > - Move the phy_init() to host_init from imx6_pcie_deassert_core_reset().
> >
> > Refine the error handling in imx6_pcie_host_init() accordingly.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 34 +++++++++++++++++----------
> >  1 file changed, 21 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> b/drivers/pci/controller/dwc/pci-imx6.c
> > index 5a06fbca82d6..0b2a5256fb0d 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -639,14 +639,6 @@ static int imx6_pcie_clk_enable(struct imx6_pcie
> *imx6_pcie)
> >  		goto err_ref_clk;
> >  	}
> >
> > -	switch (imx6_pcie->drvdata->variant) {
> > -	case IMX8MM:
> > -		if (phy_power_on(imx6_pcie->phy))
> > -			dev_err(dev, "unable to power on PHY\n");
> > -		break;
> > -	default:
> > -		break;
> > -	}
> >  	/* allow the clocks to stabilize */
> >  	usleep_range(200, 500);
> >  	return 0;
> > @@ -723,10 +715,6 @@ static int imx6_pcie_deassert_core_reset(struct
> imx6_pcie *imx6_pcie)
> >  	case IMX8MQ:
> >  		reset_control_deassert(imx6_pcie->pciephy_reset);
> >  		break;
> > -	case IMX8MM:
> > -		if (phy_init(imx6_pcie->phy))
> > -			dev_err(dev, "waiting for phy ready timeout!\n");
> > -		break;
> >  	case IMX7D:
> >  		reset_control_deassert(imx6_pcie->pciephy_reset);
> >
> > @@ -762,6 +750,7 @@ static int imx6_pcie_deassert_core_reset(struct
> imx6_pcie *imx6_pcie)
> >  		usleep_range(200, 500);
> >  		break;
> >  	case IMX6Q:		/* Nothing to do */
> > +	case IMX8MM:
> >  		break;
> >  	}
> >
> > @@ -913,17 +902,36 @@ static int imx6_pcie_host_init(struct pcie_port
> *pp)
> >  			return ret;
> >  		}
> >  	}
> > +	if (imx6_pcie->phy) {
> > +		ret = phy_power_on(imx6_pcie->phy);
> > +		if (ret) {
> > +			dev_err(dev, "pcie phy power up failed.\n");
> > +			goto err_reg_disable;
> > +		}
> > +	}
> >
> >  	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
> >  	if (ret < 0) {
> >  		dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
> > -		goto err_reg_disable;
> > +		goto err_phy_off;
> >  	}
> >
> > +	if (imx6_pcie->phy) {
> > +		ret = phy_init(imx6_pcie->phy);
> > +		if (ret) {
> > +			dev_err(dev, "waiting for phy ready timeout!\n");
> > +			goto err_clk_disable;
> > +		}
> > +	}
> 
> Wouldn't it be more logical to put this into imx6_pcie_init_phy()?
> 
Before adding i.MX8MM PCIe support, the imx6_pcie_init_phy() only touches the
 GPR registers. PCIe clocks and so on are not required in this case.
But phy_init() used by i.MX8MM PCIe touches not only the GPR registers but
 also the PHY's registers.
The clocks should be on and resets of PHY should be configured properly when
 phy_init() is invoked.
So, phy_init() is placed behind of imx6_pcie_deassert_core_reset() here.

Best Regards
Richard Zhu

> Regards,
> Lucas
> 
> >  	imx6_setup_phy_mpll(imx6_pcie);
> >
> >  	return 0;
> >
> > +err_clk_disable:
> > +	imx6_pcie_clk_disable(imx6_pcie);
> > +err_phy_off:
> > +	if (imx6_pcie->phy)
> > +		phy_power_off(imx6_pcie->phy);
> >  err_reg_disable:
> >  	if (imx6_pcie->vpcie)
> >  		regulator_disable(imx6_pcie->vpcie);
> 

RE: [PATCH v14 16/17] PCI: imx6: Move the imx6_pcie_ltssm_disable() earlier

[Hongxing Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年7月13日 17:04
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> festevam@gmail.com; francesco.dolcini@toradex.com
> Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v14 16/17] PCI: imx6: Move the imx6_pcie_ltssm_disable()
> earlier
> 
> Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> > Move the imx6_pcie_ltssm_disable() earlier and place it just behind
> > the imx6_pcie_ltssm_enable(), since it might not be only used by
> > suspend callback directly.
> > To be symmetric with imx6_pcie_ltssm_enable(), add the IMX6Q switch
> > case in the imx6_pcie_ltssm_disable().
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 39
> > ++++++++++++++-------------
> >  1 file changed, 20 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 79a05e190016..1cf8bf9035f2 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -805,6 +805,26 @@ static void imx6_pcie_ltssm_enable(struct device
> *dev)
> >  	}
> >  }
> >
> > +static void imx6_pcie_ltssm_disable(struct device *dev) {
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > +
> > +	switch (imx6_pcie->drvdata->variant) {
> > +	case IMX6Q:
> > +	case IMX6SX:
> > +	case IMX6QP:
> > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > +				   IMX6Q_GPR12_PCIE_CTL_2, 0);
> > +		break;
> > +	case IMX7D:
> > +	case IMX8MM:
> > +		reset_control_assert(imx6_pcie->apps_reset);
> > +		break;
> This is missing the IMX8MQ case.
Hi Lucas:
Good caught. I just move the function place, but forget to make a double check
on the contents accordingly.

> 
> > +	default:
> > +		dev_err(dev, "ltssm_disable not supported\n");
> 
> Drop the default, we want a compile time warning if a variant isn't covered by
> this switch statement.
> 
Got that. Thanks.

Best Regards
Richard Zhu

> Regards,
> Lucas
> 
> > +	}
> > +}
> > +
> >  static int imx6_pcie_start_link(struct dw_pcie *pci)  {
> >  	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci); @@ -947,25 +967,6
> > @@ static const struct dw_pcie_ops dw_pcie_ops = {  };
> >
> >  #ifdef CONFIG_PM_SLEEP
> > -static void imx6_pcie_ltssm_disable(struct device *dev) -{
> > -	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > -
> > -	switch (imx6_pcie->drvdata->variant) {
> > -	case IMX6SX:
> > -	case IMX6QP:
> > -		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > -				   IMX6Q_GPR12_PCIE_CTL_2, 0);
> > -		break;
> > -	case IMX7D:
> > -	case IMX8MM:
> > -		reset_control_assert(imx6_pcie->apps_reset);
> > -		break;
> > -	default:
> > -		dev_err(dev, "ltssm_disable not supported\n");
> > -	}
> > -}
> > -
> >  static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)  {
> >  	struct device *dev = imx6_pcie->pci->dev;
> 

Re: [PATCH v14 14/17] PCI: imx6: Do not hide phy driver callbacks and refine the error handling

[Lucas Stach]

Am Mittwoch, dem 13.07.2022 um 10:57 +0000 schrieb Hongxing Zhu:
> > -----Original Message-----
> > From: Lucas Stach <l.stach@pengutronix.de>
> > Sent: 2022年7月13日 16:59
> > To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> > robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> > festevam@gmail.com; francesco.dolcini@toradex.com
> > Cc: linux-pci@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > <linux-imx@nxp.com>
> > Subject: Re: [PATCH v14 14/17] PCI: imx6: Do not hide phy driver
> > callbacks and
> > refine the error handling
> > 
> > Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> > > - Move the phy_power_on() to host_init from
> > > imx6_pcie_clk_enable().
> > > - Move the phy_init() to host_init from
> > > imx6_pcie_deassert_core_reset().
> > > 
> > > Refine the error handling in imx6_pcie_host_init() accordingly.
> > > 
> > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > ---
> > >  drivers/pci/controller/dwc/pci-imx6.c | 34 +++++++++++++++++----
> > > ------
> > >  1 file changed, 21 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 5a06fbca82d6..0b2a5256fb0d 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -639,14 +639,6 @@ static int imx6_pcie_clk_enable(struct
> > > imx6_pcie
> > *imx6_pcie)
> > >  		goto err_ref_clk;
> > >  	}
> > > 
> > > -	switch (imx6_pcie->drvdata->variant) {
> > > -	case IMX8MM:
> > > -		if (phy_power_on(imx6_pcie->phy))
> > > -			dev_err(dev, "unable to power on
> > > PHY\n");
> > > -		break;
> > > -	default:
> > > -		break;
> > > -	}
> > >  	/* allow the clocks to stabilize */
> > >  	usleep_range(200, 500);
> > >  	return 0;
> > > @@ -723,10 +715,6 @@ static int
> > > imx6_pcie_deassert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> > >  	case IMX8MQ:
> > >  		reset_control_deassert(imx6_pcie-
> > > >pciephy_reset);
> > >  		break;
> > > -	case IMX8MM:
> > > -		if (phy_init(imx6_pcie->phy))
> > > -			dev_err(dev, "waiting for phy ready
> > > timeout!\n");
> > > -		break;
> > >  	case IMX7D:
> > >  		reset_control_deassert(imx6_pcie-
> > > >pciephy_reset);
> > > 
> > > @@ -762,6 +750,7 @@ static int
> > > imx6_pcie_deassert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> > >  		usleep_range(200, 500);
> > >  		break;
> > >  	case IMX6Q:		/* Nothing to do */
> > > +	case IMX8MM:
> > >  		break;
> > >  	}
> > > 
> > > @@ -913,17 +902,36 @@ static int imx6_pcie_host_init(struct
> > > pcie_port
> > *pp)
> > >  			return ret;
> > >  		}
> > >  	}
> > > +	if (imx6_pcie->phy) {
> > > +		ret = phy_power_on(imx6_pcie->phy);
> > > +		if (ret) {
> > > +			dev_err(dev, "pcie phy power up
> > > failed.\n");
> > > +			goto err_reg_disable;
> > > +		}
> > > +	}
> > > 
> > >  	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
> > >  	if (ret < 0) {
> > >  		dev_err(dev, "pcie deassert core reset failed:
> > > %d\n", ret);
> > > -		goto err_reg_disable;
> > > +		goto err_phy_off;
> > >  	}
> > > 
> > > +	if (imx6_pcie->phy) {
> > > +		ret = phy_init(imx6_pcie->phy);
> > > +		if (ret) {
> > > +			dev_err(dev, "waiting for phy ready
> > > timeout!\n");
> > > +			goto err_clk_disable;
> > > +		}
> > > +	}
> > 
> > Wouldn't it be more logical to put this into imx6_pcie_init_phy()?
> > 
> Before adding i.MX8MM PCIe support, the imx6_pcie_init_phy() only
> touches the
>  GPR registers. PCIe clocks and so on are not required in this case.
> But phy_init() used by i.MX8MM PCIe touches not only the GPR
> registers but
>  also the PHY's registers.
> The clocks should be on and resets of PHY should be configured
> properly when
>  phy_init() is invoked.
> So, phy_init() is placed behind of imx6_pcie_deassert_core_reset()
> here.

The PHY driver should be self-contained enough to not care about the
state of the controller here, no? It should set all the necessary GPRs
and enable clocks as needed on its own. Is this not the case with the
current code?

Also PHY init should be called before PHY power-on, to make things
symmetric with the shutdown paths which do phy_power_off() first, then
phy_exit().

Regards,
Lucas

Re: [PATCH v14 10/17] PCI: imx6: Turn off regulator when system is in suspend mode

[Lucas Stach]

Am Mittwoch, dem 13.07.2022 um 10:56 +0000 schrieb Hongxing Zhu:
> > -----Original Message-----
> > From: Lucas Stach <l.stach@pengutronix.de>
> > Sent: 2022年7月13日 16:34
> > To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> > robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> > festevam@gmail.com; francesco.dolcini@toradex.com
> > Cc: linux-pci@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > <linux-imx@nxp.com>
> > Subject: Re: [PATCH v14 10/17] PCI: imx6: Turn off regulator when
> > system is in
> > suspend mode
> > 
> > Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> > > The driver should undo any enables it did itself. The regulator
> > > disable shouldn't be basing decisions on regulator_is_enabled().
> > > 
> > > Move the regulator_disable to the suspend function, turn off
> > > regulator
> > > when the system is in suspend mode.
> > > 
> > > To keep the balance of the regulator usage counter, disable the
> > > regulator in shutdown.
> > > 
> > > Link:
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > > .kernel.org%2Fr%2F1655189942-12678-6-git-send-email-
> > > hongxing.z&amp;d
> > at
> > > 
> > a=05%7C01%7Chongxing.zhu%40nxp.com%7C361c9bf365b64895482808da6
> > 4aa7cca%
> > > 
> > 7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379329806385373
> > 39%7CUnkn
> > > 
> > own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> > haWwi
> > > 
> > LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=A%2Fw6AFCddC0T%2FA
> > ocBT%2F7OJZ
> > > ddAeYPTbgeAxpHpPubkw%3D&amp;reserved=0
> > > hu@nxp.com
> > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > ---
> > >  drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++------------
> > >  1 file changed, 7 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 2b42c37f1617..f72eb609769b 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -670,8 +670,6 @@ static void imx6_pcie_clk_disable(struct
> > > imx6_pcie
> > > *imx6_pcie)
> > > 
> > >  static void imx6_pcie_assert_core_reset(struct imx6_pcie
> > > *imx6_pcie)
> > > {
> > > -	struct device *dev = imx6_pcie->pci->dev;
> > > -
> > >  	switch (imx6_pcie->drvdata->variant) {
> > >  	case IMX7D:
> > >  	case IMX8MQ:
> > > @@ -702,14 +700,6 @@ static void
> > > imx6_pcie_assert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> > >  		break;
> > >  	}
> > > 
> > > -	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie-
> > > >vpcie) > 0) {
> > > -		int ret = regulator_disable(imx6_pcie->vpcie);
> > > -
> > > -		if (ret)
> > > -			dev_err(dev, "failed to disable vpcie
> > > regulator: %d\n",
> > > -				ret);
> > > -	}
> > > -
> > >  	/* Some boards don't have PCIe reset GPIO. */
> > >  	if (gpio_is_valid(imx6_pcie->reset_gpio))
> > >  		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> > > @@ -722,7 +712,7 @@ static int
> > > imx6_pcie_deassert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> > >  	struct device *dev = pci->dev;
> > >  	int ret;
> > > 
> > > -	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie-
> > > >vpcie)) {
> > > +	if (imx6_pcie->vpcie) {
> > >  		ret = regulator_enable(imx6_pcie->vpcie);
> > >  		if (ret) {
> > >  			dev_err(dev, "failed to enable vpcie
> > > regulator: %d\n",
> > 
> > The regulator really has nothing to do with the core reset. Please
> > move this
> > regulator enable into imx6_pcie_host_init().
> Hi Lucas:
> Thanks for your comments.
> Got that. Had done it in the 11/17 commit.
> 
> > 
> > > @@ -795,7 +785,7 @@ static int
> > > imx6_pcie_deassert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> > >  	return 0;
> > > 
> > >  err_clks:
> > > -	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie-
> > > >vpcie) > 0) {
> > > +	if (imx6_pcie->vpcie) {
> > >  		ret = regulator_disable(imx6_pcie->vpcie);
> > >  		if (ret)
> > >  			dev_err(dev, "failed to disable vpcie
> > > regulator: %d\n", @@
> > -1022,6
> > > +1012,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> > >  		break;
> > >  	}
> > > 
> > > +	if (imx6_pcie->vpcie)
> > > +		regulator_disable(imx6_pcie->vpcie);
> > > +
> > >  	return 0;
> > >  }
> > > 
> > > @@ -1268,6 +1261,8 @@ static void imx6_pcie_shutdown(struct
> > > platform_device *pdev)
> > > 
> > >  	/* bring down link, so bootloader gets clean state in
> > > case of reboot */
> > >  	imx6_pcie_assert_core_reset(imx6_pcie);
> > > +	if (imx6_pcie->vpcie)
> > > +		regulator_disable(imx6_pcie->vpcie);
> > 
> > This looks like a separate change, not mentioned in the commit
> > message.
> > I'm not sure if we should do this. Shutdown is supposed to just
> > stop the device,
> > which is already achieved by imx6_pcie_assert_core_reset().
> > 
> > If we would want to do a full cleanup here we would also need to
> > disable
> > clocks and get the reset GPIO into asserted state. I don't think we
> > want to do
> > all of this here.
> The regulator_disable() was contained in
> imx6_pcie_assert_core_reset() before.
> When refine the regulator usage, the regulator_disable() is moved out
> of
>  imx6_pcie_assert_core_reset().
> Based on the discussion [1] with Francesco.
> To keep the same behavior, the regulator_disable() is placed behind
> of
>  imx6_pcie_assert_core_reset() in imx6_pcie_shutdown() here.
> [1]
> https://patchwork.ozlabs.org/project/linux-pci/patch/1644290735-3797-6-git-send-email-hongxing.zhu@nxp.com/

I disagree with Francesco on this. This kind of half-done cleanup
doesn't help in any way. Either we care about doing a full cleanup
here, which would involve clocks and reset GPIOs, or we just care about
getting the core in a state where it survives a reboot, which frankly
was the original intention when I added the shutdown callback.

As the devices on the bus will see a full PERST reset via the reset
GPIO anyways, there is no harm in keeping them powered across the
reboot.

Regards,
Lucas

RE: [PATCH v14 14/17] PCI: imx6: Do not hide phy driver callbacks and refine the error handling

[Hongxing Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年7月13日 19:17
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> festevam@gmail.com; francesco.dolcini@toradex.com
> Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v14 14/17] PCI: imx6: Do not hide phy driver callbacks and
> refine the error handling
> 
> Am Mittwoch, dem 13.07.2022 um 10:57 +0000 schrieb Hongxing Zhu:
> > > -----Original Message-----
> > > From: Lucas Stach <l.stach@pengutronix.de>
> > > Sent: 2022年7月13日 16:59
> > > To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> > > robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> > > festevam@gmail.com; francesco.dolcini@toradex.com
> > > Cc: linux-pci@vger.kernel.org;
> > > linux-arm-kernel@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > > <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v14 14/17] PCI: imx6: Do not hide phy driver
> > > callbacks and refine the error handling
> > >
> > > Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> > > > - Move the phy_power_on() to host_init from
> > > > imx6_pcie_clk_enable().
> > > > - Move the phy_init() to host_init from
> > > > imx6_pcie_deassert_core_reset().
> > > >
> > > > Refine the error handling in imx6_pcie_host_init() accordingly.
> > > >
> > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > ---
> > > >  drivers/pci/controller/dwc/pci-imx6.c | 34 +++++++++++++++++----
> > > > ------
> > > >  1 file changed, 21 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > > index 5a06fbca82d6..0b2a5256fb0d 100644
> > > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > > @@ -639,14 +639,6 @@ static int imx6_pcie_clk_enable(struct
> > > > imx6_pcie
> > > *imx6_pcie)
> > > >  		goto err_ref_clk;
> > > >  	}
> > > >
> > > > -	switch (imx6_pcie->drvdata->variant) {
> > > > -	case IMX8MM:
> > > > -		if (phy_power_on(imx6_pcie->phy))
> > > > -			dev_err(dev, "unable to power on
> > > > PHY\n");
> > > > -		break;
> > > > -	default:
> > > > -		break;
> > > > -	}
> > > >  	/* allow the clocks to stabilize */
> > > >  	usleep_range(200, 500);
> > > >  	return 0;
> > > > @@ -723,10 +715,6 @@ static int
> > > > imx6_pcie_deassert_core_reset(struct
> > > imx6_pcie *imx6_pcie)
> > > >  	case IMX8MQ:
> > > >  		reset_control_deassert(imx6_pcie-
> > > > >pciephy_reset);
> > > >  		break;
> > > > -	case IMX8MM:
> > > > -		if (phy_init(imx6_pcie->phy))
> > > > -			dev_err(dev, "waiting for phy ready
> > > > timeout!\n");
> > > > -		break;
> > > >  	case IMX7D:
> > > >  		reset_control_deassert(imx6_pcie-
> > > > >pciephy_reset);
> > > >
> > > > @@ -762,6 +750,7 @@ static int
> > > > imx6_pcie_deassert_core_reset(struct
> > > imx6_pcie *imx6_pcie)
> > > >  		usleep_range(200, 500);
> > > >  		break;
> > > >  	case IMX6Q:		/* Nothing to do */
> > > > +	case IMX8MM:
> > > >  		break;
> > > >  	}
> > > >
> > > > @@ -913,17 +902,36 @@ static int imx6_pcie_host_init(struct
> > > > pcie_port
> > > *pp)
> > > >  			return ret;
> > > >  		}
> > > >  	}
> > > > +	if (imx6_pcie->phy) {
> > > > +		ret = phy_power_on(imx6_pcie->phy);
> > > > +		if (ret) {
> > > > +			dev_err(dev, "pcie phy power up
> > > > failed.\n");
> > > > +			goto err_reg_disable;
> > > > +		}
> > > > +	}
> > > >
> > > >  	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
> > > >  	if (ret < 0) {
> > > >  		dev_err(dev, "pcie deassert core reset failed:
> > > > %d\n", ret);
> > > > -		goto err_reg_disable;
> > > > +		goto err_phy_off;
> > > >  	}
> > > >
> > > > +	if (imx6_pcie->phy) {
> > > > +		ret = phy_init(imx6_pcie->phy);
> > > > +		if (ret) {
> > > > +			dev_err(dev, "waiting for phy ready
> > > > timeout!\n");
> > > > +			goto err_clk_disable;
> > > > +		}
> > > > +	}
> > >
> > > Wouldn't it be more logical to put this into imx6_pcie_init_phy()?
> > >
> > Before adding i.MX8MM PCIe support, the imx6_pcie_init_phy() only
> > touches the
> >  GPR registers. PCIe clocks and so on are not required in this case.
> > But phy_init() used by i.MX8MM PCIe touches not only the GPR registers
> > but
> >  also the PHY's registers.
> > The clocks should be on and resets of PHY should be configured
> > properly when
> >  phy_init() is invoked.
> > So, phy_init() is placed behind of imx6_pcie_deassert_core_reset()
> > here.
> 
> The PHY driver should be self-contained enough to not care about the state of
> the controller here, no? It should set all the necessary GPRs and enable clocks
> as needed on its own. Is this not the case with the current code?
> 
> Also PHY init should be called before PHY power-on, to make things symmetric
> with the shutdown paths which do phy_power_off() first, then phy_exit().
> 
Thanks for your comments.
Yes, agree with that PHY driver should be self-contained enough.
I mis-understood the sequence of phy_init() and phy_power_on() when I add
 the PHY driver. The phy_init() is relied on the phy_power_on() here.
So, I have to place the phy_init() behind phy_power_on() currently.
I think the phy_init() can be placed in imx6_pcie_init_phy() when the order
 issue is fixed.

BTW, Bjorn had noticed me this issue too [1]. Since it's a bug, I would issue
 another standalone bug fix commits to resolve this issue later.
[1] https://patchwork.ozlabs.org/project/linux-pci/patch/1655461874-16908-11-git-send-email-hongxing.zhu@nxp.com/

Best Regards
Richard Zhu

> Regards,
> Lucas

RE: [PATCH v14 10/17] PCI: imx6: Turn off regulator when system is in suspend mode

[Hongxing Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年7月13日 19:23
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> festevam@gmail.com; francesco.dolcini@toradex.com
> Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v14 10/17] PCI: imx6: Turn off regulator when system is in
> suspend mode
> 
> Am Mittwoch, dem 13.07.2022 um 10:56 +0000 schrieb Hongxing Zhu:
> > > -----Original Message-----
> > > From: Lucas Stach <l.stach@pengutronix.de>
> > > Sent: 2022年7月13日 16:34
> > > To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> > > robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> > > festevam@gmail.com; francesco.dolcini@toradex.com
> > > Cc: linux-pci@vger.kernel.org;
> > > linux-arm-kernel@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > > <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v14 10/17] PCI: imx6: Turn off regulator when
> > > system is in suspend mode
> > >
> > > Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> > > > The driver should undo any enables it did itself. The regulator
> > > > disable shouldn't be basing decisions on regulator_is_enabled().
> > > >
> > > > Move the regulator_disable to the suspend function, turn off
> > > > regulator when the system is in suspend mode.
> > > >
> > > > To keep the balance of the regulator usage counter, disable the
> > > > regulator in shutdown.
> > > >
> > > > Link:
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > > lore
> > > > .kernel.org%2Fr%2F1655189942-12678-6-git-send-email-
> > > > hongxing.z&amp;d
> > > at
> > > >
> > >
> a=05%7C01%7Chongxing.zhu%40nxp.com%7C361c9bf365b64895482808da6
> > > 4aa7cca%
> > > >
> > >
> 7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379329806385373
> > > 39%7CUnkn
> > > >
> > >
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> > > haWwi
> > > >
> > >
> LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=A%2Fw6AFCddC0T%2FA
> > > ocBT%2F7OJZ
> > > > ddAeYPTbgeAxpHpPubkw%3D&amp;reserved=0
> > > > hu@nxp.com
> > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > ---
> > > >  drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++------------
> > > >  1 file changed, 7 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > > index 2b42c37f1617..f72eb609769b 100644
> > > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > > @@ -670,8 +670,6 @@ static void imx6_pcie_clk_disable(struct
> > > > imx6_pcie
> > > > *imx6_pcie)
> > > >
> > > >  static void imx6_pcie_assert_core_reset(struct imx6_pcie
> > > > *imx6_pcie)
> > > > {
> > > > -	struct device *dev = imx6_pcie->pci->dev;
> > > > -
> > > >  	switch (imx6_pcie->drvdata->variant) {
> > > >  	case IMX7D:
> > > >  	case IMX8MQ:
> > > > @@ -702,14 +700,6 @@ static void
> > > > imx6_pcie_assert_core_reset(struct
> > > imx6_pcie *imx6_pcie)
> > > >  		break;
> > > >  	}
> > > >
> > > > -	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie-
> > > > >vpcie) > 0) {
> > > > -		int ret = regulator_disable(imx6_pcie->vpcie);
> > > > -
> > > > -		if (ret)
> > > > -			dev_err(dev, "failed to disable vpcie
> > > > regulator: %d\n",
> > > > -				ret);
> > > > -	}
> > > > -
> > > >  	/* Some boards don't have PCIe reset GPIO. */
> > > >  	if (gpio_is_valid(imx6_pcie->reset_gpio))
> > > >  		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> > > > @@ -722,7 +712,7 @@ static int
> > > > imx6_pcie_deassert_core_reset(struct
> > > imx6_pcie *imx6_pcie)
> > > >  	struct device *dev = pci->dev;
> > > >  	int ret;
> > > >
> > > > -	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie-
> > > > >vpcie)) {
> > > > +	if (imx6_pcie->vpcie) {
> > > >  		ret = regulator_enable(imx6_pcie->vpcie);
> > > >  		if (ret) {
> > > >  			dev_err(dev, "failed to enable vpcie
> > > > regulator: %d\n",
> > >
> > > The regulator really has nothing to do with the core reset. Please
> > > move this regulator enable into imx6_pcie_host_init().
> > Hi Lucas:
> > Thanks for your comments.
> > Got that. Had done it in the 11/17 commit.
> >
> > >
> > > > @@ -795,7 +785,7 @@ static int
> > > > imx6_pcie_deassert_core_reset(struct
> > > imx6_pcie *imx6_pcie)
> > > >  	return 0;
> > > >
> > > >  err_clks:
> > > > -	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie-
> > > > >vpcie) > 0) {
> > > > +	if (imx6_pcie->vpcie) {
> > > >  		ret = regulator_disable(imx6_pcie->vpcie);
> > > >  		if (ret)
> > > >  			dev_err(dev, "failed to disable vpcie
> > > > regulator: %d\n", @@
> > > -1022,6
> > > > +1012,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> > > >  		break;
> > > >  	}
> > > >
> > > > +	if (imx6_pcie->vpcie)
> > > > +		regulator_disable(imx6_pcie->vpcie);
> > > > +
> > > >  	return 0;
> > > >  }
> > > >
> > > > @@ -1268,6 +1261,8 @@ static void imx6_pcie_shutdown(struct
> > > > platform_device *pdev)
> > > >
> > > >  	/* bring down link, so bootloader gets clean state in case of
> > > > reboot */
> > > >  	imx6_pcie_assert_core_reset(imx6_pcie);
> > > > +	if (imx6_pcie->vpcie)
> > > > +		regulator_disable(imx6_pcie->vpcie);
> > >
> > > This looks like a separate change, not mentioned in the commit
> > > message.
> > > I'm not sure if we should do this. Shutdown is supposed to just stop
> > > the device, which is already achieved by
> > > imx6_pcie_assert_core_reset().
> > >
> > > If we would want to do a full cleanup here we would also need to
> > > disable clocks and get the reset GPIO into asserted state. I don't
> > > think we want to do all of this here.
> > The regulator_disable() was contained in
> > imx6_pcie_assert_core_reset() before.
> > When refine the regulator usage, the regulator_disable() is moved out
> > of
> >  imx6_pcie_assert_core_reset().
> > Based on the discussion [1] with Francesco.
> > To keep the same behavior, the regulator_disable() is placed behind of
> >  imx6_pcie_assert_core_reset() in imx6_pcie_shutdown() here.
> > [1]
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> >
> hwork.ozlabs.org%2Fproject%2Flinux-pci%2Fpatch%2F1644290735-3797-6-gi
> t
> >
> -send-email-hongxing.zhu%40nxp.com%2F&amp;data=05%7C01%7Chongxing.
> zhu%
> >
> 40nxp.com%7C88b4bedd17e84c440c5608da64c20698%7C686ea1d3bc2b4c6
> fa92cd99
> >
> c5c301635%7C0%7C0%7C637933081743458707%7CUnknown%7CTWFpbGZ
> sb3d8eyJWIjo
> >
> iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C300
> 0%7C%
> >
> 7C%7C&amp;sdata=NHl4prCMrfR4KAZgIaCuJX%2FOrMJW3Up31NX%2FzMMY
> pag%3D&amp
> > ;reserved=0
> 
> I disagree with Francesco on this. This kind of half-done cleanup doesn't help
> in any way. Either we care about doing a full cleanup here, which would
> involve clocks and reset GPIOs, or we just care about getting the core in a state
> where it survives a reboot, which frankly was the original intention when I
> added the shutdown callback.
> 
> As the devices on the bus will see a full PERST reset via the reset GPIO anyways,
> there is no harm in keeping them powered across the reboot.
Okay, I see.
Thanks for your comment.
I can remove it if you insist.

Best Regards
Richard Zhu

> 
> Regards,
> Lucas

Re: [PATCH v14 0/17] PCI: imx6: refine codes and add the error propagation

[Bjorn Helgaas]

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

RE: [PATCH v14 0/17] PCI: imx6: refine codes and add the error propagation

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 2022年7月14日 0:46
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org;
> broonie@kernel.org; lorenzo.pieralisi@arm.com; festevam@gmail.com;
> francesco.dolcini@toradex.com; linux-pci@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> 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&amp;data=05
> %7C01%7Chongxing.zhu%40nxp.com%7Cd0b3d82dae37495b2f7e08da64ef2a
> 3a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379332756103
> 31009%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu
> MzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=Z8
> grlALYqs3ONJp1EQJsFJd8fD8XiE3zFXHuYlfE4TI%3D&amp;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