[PATCH v12 00/13] PCI: imx6: refine codes and add the error propagation

[PATCH v12 00/13] PCI: imx6: refine codes and add the error propagation

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

This is a v12 of Richard's series with this description:

> 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.
> BTW, this series are verified on i.MX8MM EVK board when one NVME is used.

Main changes from v11 to v12:
  - 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 (8):
  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: Turn off regulator when system is in suspend mode
  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

 drivers/pci/controller/dwc/pci-imx6.c | 606 +++++++++++++++-----------
 1 file changed, 342 insertions(+), 264 deletions(-)

-- 
2.25.1

[PATCH v12 01/13] PCI: imx6: Move imx6_pcie_grp_offset(), imx6_pcie_configure_type() earlier

[Bjorn Helgaas]

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>
---
 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 v12 02/13] PCI: imx6: Move PHY management functions together

[Bjorn Helgaas]

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>
---
 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 v12 03/13] PCI: imx6: Move imx6_pcie_enable_ref_clk() earlier

[Bjorn Helgaas]

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>
---
 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 v12 04/13] PCI: imx6: Move imx6_pcie_clk_disable() earlier

[Bjorn Helgaas]

From: Richard Zhu <hongxing.zhu@nxp.com>

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 v12 05/13] PCI: imx6: Factor out ref clock disable to match enable

[Bjorn Helgaas]

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>
---
 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 v12 06/13] PCI: imx6: Collect clock enables in imx6_pcie_clk_enable()

[Bjorn Helgaas]

From: Richard Zhu <hongxing.zhu@nxp.com>

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 | 98 ++++++++++++++++-----------
 1 file changed, 58 insertions(+), 40 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index f458461880dc..e36ca08a7c51 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);
@@ -660,7 +712,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 {
 	struct dw_pcie *pci = imx6_pcie->pci;
 	struct device *dev = pci->dev;
-	int ret;
+	int ret, err;
 
 	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
 		ret = regulator_enable(imx6_pcie->vpcie);
@@ -671,40 +723,12 @@ 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;
+	err = imx6_pcie_clk_enable(imx6_pcie);
+	if (err) {
+		dev_err(dev, "unable to enable pcie clocks: %d\n", err);
+		goto err_clks;
 	}
 
-	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);
 
 	switch (imx6_pcie->drvdata->variant) {
 	case IMX8MQ:
@@ -763,13 +787,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 v12 07/13] PCI: imx6: Propagate .host_init() errors to caller

[Bjorn Helgaas]

From: Richard Zhu <hongxing.zhu@nxp.com>

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 e36ca08a7c51..90ab9397a935 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;
 		}
 	}
 
@@ -785,7 +785,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) {
@@ -794,6 +794,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 err;
 }
 
 static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie)
@@ -912,11 +913,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 host init failed: %d\n", ret);
+		return ret;
+	}
+
 	imx6_setup_phy_mpll(imx6_pcie);
 
 	return 0;
-- 
2.25.1

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

[Bjorn Helgaas]

From: Richard Zhu <hongxing.zhu@nxp.com>

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 90ab9397a935..6eddd0b5f628 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 v12 09/13] PCI: imx6: Turn off regulator when system is in suspend mode

[Bjorn Helgaas]

From: Richard Zhu <hongxing.zhu@nxp.com>

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 6eddd0b5f628..537b8a2e0e3b 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, err;
 
-	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",
@@ -796,7 +786,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",
@@ -1023,6 +1013,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
 		break;
 	}
 
+	if (imx6_pcie->vpcie)
+		regulator_disable(imx6_pcie->vpcie);
+
 	return 0;
 }
 
@@ -1269,6 +1262,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 v12 10/13] PCI: imx6: Mark the link down as non-fatal error

[Bjorn Helgaas]

From: Richard Zhu <hongxing.zhu@nxp.com>

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 537b8a2e0e3b..7d3592540b8a 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -855,7 +855,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. */
@@ -891,7 +893,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");
 	}
@@ -905,7 +909,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 v12 11/13] PCI: imx6: Reduce resume time by only starting link if it was up before suspend

[Bjorn Helgaas]

From: Richard Zhu <hongxing.zhu@nxp.com>

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.

[bhelgaas: drop now-unused "ret"]
Link: https://lore.kernel.org/r/1655189942-12678-8-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 | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 7d3592540b8a..b6e5420d67b6 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;
@@ -900,6 +901,7 @@ 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;
@@ -1025,7 +1027,6 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
 
 static int imx6_pcie_resume_noirq(struct device *dev)
 {
-	int ret;
 	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
 	struct pcie_port *pp = &imx6_pcie->pci->pp;
 
@@ -1036,10 +1037,8 @@ static int imx6_pcie_resume_noirq(struct device *dev)
 	imx6_pcie_init_phy(imx6_pcie);
 	imx6_pcie_deassert_core_reset(imx6_pcie);
 	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 v12 12/13] PCI: imx6: Do not hide phy driver callbacks and refine the error handling

[Bjorn Helgaas]

From: Richard Zhu <hongxing.zhu@nxp.com>

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

Refine the error handling in imx6_pcie_host_init() and
imx6_pcie_resume_noirq() functions accordingly.

[bhelgaas: add "ret" back since it's used again]
Link: https://lore.kernel.org/r/1655189942-12678-9-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 | 70 +++++++++++++++++++++------
 1 file changed, 56 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index b6e5420d67b6..bd736aff94a3 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;
@@ -733,10 +725,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);
 
@@ -772,6 +760,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;
 	}
 
@@ -923,15 +912,39 @@ 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->phy) {
+		ret = phy_power_on(imx6_pcie->phy);
+		if (ret) {
+			dev_err(dev, "pcie phy power up failed.\n");
+			return ret;
+		}
+	}
+
 	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
 	if (ret < 0) {
 		dev_err(dev, "pcie host init failed: %d\n", ret);
-		return ret;
+		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_phy_init;
+		}
 	}
 
 	imx6_setup_phy_mpll(imx6_pcie);
 
 	return 0;
+
+err_phy_init:
+	imx6_pcie_clk_disable(imx6_pcie);
+	if (imx6_pcie->vpcie)
+		regulator_disable(imx6_pcie->vpcie);
+err_phy_off:
+	if (imx6_pcie->phy)
+		phy_power_off(imx6_pcie->phy);
+	return ret;
 }
 
 static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
@@ -1029,18 +1042,47 @@ static int imx6_pcie_resume_noirq(struct device *dev)
 {
 	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
 	struct pcie_port *pp = &imx6_pcie->pci->pp;
+	int ret;
 
 	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);
+	if (imx6_pcie->phy) {
+		ret = phy_power_on(imx6_pcie->phy);
+		if (ret) {
+			dev_err(dev, "pcie phy power up failed.\n");
+			return ret;
+		}
+	}
+
+	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
+	if (ret < 0) {
+		dev_err(dev, "pcie deassert core reset failed: %d.\n", ret);
+		goto err_phy_off;
+	} else if (imx6_pcie->phy) {
+		ret = phy_init(imx6_pcie->phy);
+		if (ret) {
+			dev_err(dev, "pcie phy init failed.\n");
+			goto err_phy_init;
+		}
+	}
+
 	dw_pcie_setup_rc(pp);
 	if (imx6_pcie->link_is_up)
 		imx6_pcie_start_link(imx6_pcie->pci);
 
 	return 0;
+
+err_phy_init:
+	imx6_pcie_clk_disable(imx6_pcie);
+	if (imx6_pcie->vpcie)
+		regulator_disable(imx6_pcie->vpcie);
+err_phy_off:
+	if (imx6_pcie->phy)
+		phy_power_off(imx6_pcie->phy);
+	return ret;
 }
 #endif
 
-- 
2.25.1

[PATCH v12 13/13] PCI: imx6: Disable clocks in reverse order of enable

[Bjorn Helgaas]

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>
---
 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 bd736aff94a3..738b5a732cef 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)
 {
-	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);
+	clk_disable_unprepare(imx6_pcie->pcie_bus);
+	clk_disable_unprepare(imx6_pcie->pcie_phy);
 }
 
 static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
-- 
2.25.1

Re: [PATCH v12 13/13] PCI: imx6: Disable clocks in reverse order of enable

[Bjorn Helgaas]

On Wed, Jun 15, 2022 at 06:15:51PM -0500, Bjorn Helgaas wrote:
> 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>
> ---
>  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 bd736aff94a3..738b5a732cef 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)
>  {
> -	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);
> +	clk_disable_unprepare(imx6_pcie->pcie_bus);
> +	clk_disable_unprepare(imx6_pcie->pcie_phy);

Please comment on this.  I have no actual information that this is the
right thing, but normally we disable things in the reverse order that
we enabled them.

And the error path of imx6_pcie_deassert_core_reset() definitely
disables pcie, pcie_bus, pcie_phy in that order, so it seems
reasonable to do the same here.

>  }
>  
>  static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12 09/13] PCI: imx6: Turn off regulator when system is in suspend mode

[Bjorn Helgaas]

On Wed, Jun 15, 2022 at 06:15:47PM -0500, Bjorn Helgaas wrote:
> From: Richard Zhu <hongxing.zhu@nxp.com>
> 
> 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 6eddd0b5f628..537b8a2e0e3b 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, err;
>  
> -	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",
> @@ -796,7 +786,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",
> @@ -1023,6 +1013,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
>  		break;
>  	}
>  
> +	if (imx6_pcie->vpcie)
> +		regulator_disable(imx6_pcie->vpcie);

This is a little bit ugly because imx6_pcie_suspend_noirq() and
imx6_pcie_resume_noirq() are not symmetric.

We call regulator_disable() directly here in
imx6_pcie_suspend_noirq(), but the corresponding regulator_enable() is
buried in imx6_pcie_deassert_core_reset().

It would be nicer if the suspend and resume paths looked more similar,
as rockchip_pcie_suspend_noirq() and rockchip_pcie_resume_noirq() do,
for example.

>  	return 0;
>  }
>  
> @@ -1269,6 +1262,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
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v12 09/13] PCI: imx6: Turn off regulator when system is in suspend mode

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 2022年6月16日 7:31
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; Lucas Stach
> <l.stach@pengutronix.de>; Rob Herring <robh+dt@kernel.org>; Mark Brown
> <broonie@kernel.org>; Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Fabio
> Estevam <festevam@gmail.com>; Francesco Dolcini
> <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>; Bjorn Helgaas <bhelgaas@google.com>
> Subject: Re: [PATCH v12 09/13] PCI: imx6: Turn off regulator when system is in
> suspend mode
> 
> On Wed, Jun 15, 2022 at 06:15:47PM -0500, Bjorn Helgaas wrote:
> > From: Richard Zhu <hongxing.zhu@nxp.com>
> >
> > 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%7C8dea46d908d34cb4825808da4
> f270eaf%
> >
> 7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379093264156415
> 35%7CUnkn
> >
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> haWwi
> >
> LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=tqzlcCmtSkUNxNzJ0Fr5o
> CR3X2CTH
> > 4dwUJS4NXt5KRc%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 6eddd0b5f628..537b8a2e0e3b 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, err;
> >
> > -	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", @@
> -796,7
> > +786,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", @@
> -1023,6
> > +1013,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> >  		break;
> >  	}
> >
> > +	if (imx6_pcie->vpcie)
> > +		regulator_disable(imx6_pcie->vpcie);
> 
> This is a little bit ugly because imx6_pcie_suspend_noirq() and
> imx6_pcie_resume_noirq() are not symmetric.
> 
> We call regulator_disable() directly here in imx6_pcie_suspend_noirq(), but the
> corresponding regulator_enable() is buried in imx6_pcie_deassert_core_reset().
> 
> It would be nicer if the suspend and resume paths looked more similar, as
> rockchip_pcie_suspend_noirq() and rockchip_pcie_resume_noirq() do, for
> example.
Yes, it is. The regulator_disable()/regulator_enable() are not invoked
 symmetrically.

In the original codes, the regulator_disable() is contained in
imx6_pcie_assert_core_reset() and with _eanbled() check.
And regulator_enable() is embedded in imx6_pcie_deassert_core_reset().
Both the regulator disable and enable are invoked in resume before.
I move the regulator_disable to suspend mode refer to Lucas' comments.

Refer to the current situation, how about to move the regulator_disable()
 out of imx6_pcie_deassert_core_reset() too?

Best Regards
Richard Zhu

> 
> >  	return 0;
> >  }
> >
> > @@ -1269,6 +1262,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
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists
> > .infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&amp;data=05%7
> C0
> >
> 1%7Chongxing.zhu%40nxp.com%7C8dea46d908d34cb4825808da4f270eaf%7
> C686ea1
> >
> d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637909326415641535%7CUnk
> nown%7CTW
> >
> FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV
> CI6
> >
> Mn0%3D%7C3000%7C%7C%7C&amp;sdata=xjn3jX7ub9%2BX%2BAJQ7YPecV5
> ujkzLAPXkY
> > hcieKs%2BKqg%3D&amp;reserved=0

RE: [PATCH v12 01/13] PCI: imx6: Move imx6_pcie_grp_offset(), imx6_pcie_configure_type() earlier

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 2022年6月16日 7:16
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; Lucas Stach
> <l.stach@pengutronix.de>; Rob Herring <robh+dt@kernel.org>; Mark Brown
> <broonie@kernel.org>; Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Fabio
> Estevam <festevam@gmail.com>; Francesco Dolcini
> <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>; Bjorn Helgaas <bhelgaas@google.com>
> Subject: [PATCH v12 01/13] PCI: imx6: Move imx6_pcie_grp_offset(),
> imx6_pcie_configure_type() earlier
> 
> 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>

Best Regards
Richard Zhu

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

RE: [PATCH v12 02/13] PCI: imx6: Move PHY management functions together

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 2022年6月16日 7:16
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; Lucas Stach
> <l.stach@pengutronix.de>; Rob Herring <robh+dt@kernel.org>; Mark Brown
> <broonie@kernel.org>; Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Fabio
> Estevam <festevam@gmail.com>; Francesco Dolcini
> <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>; Bjorn Helgaas <bhelgaas@google.com>
> Subject: [PATCH v12 02/13] PCI: imx6: Move PHY management functions
> together
> 
> 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>

Best Regards
Richard Zhu
> ---
>  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

RE: [PATCH v12 03/13] PCI: imx6: Move imx6_pcie_enable_ref_clk() earlier

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 2022年6月16日 7:16
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; Lucas Stach
> <l.stach@pengutronix.de>; Rob Herring <robh+dt@kernel.org>; Mark Brown
> <broonie@kernel.org>; Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Fabio
> Estevam <festevam@gmail.com>; Francesco Dolcini
> <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>; Bjorn Helgaas <bhelgaas@google.com>
> Subject: [PATCH v12 03/13] PCI: imx6: Move imx6_pcie_enable_ref_clk()
> earlier
> 
> 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>

Best Regards
Richard Zhu
> ---
>  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 v12 05/13] PCI: imx6: Factor out ref clock disable to match enable

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 2022年6月16日 7:16
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; Lucas Stach
> <l.stach@pengutronix.de>; Rob Herring <robh+dt@kernel.org>; Mark Brown
> <broonie@kernel.org>; Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Fabio
> Estevam <festevam@gmail.com>; Francesco Dolcini
> <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>; Bjorn Helgaas <bhelgaas@google.com>
> Subject: [PATCH v12 05/13] PCI: imx6: Factor out ref clock disable to match
> enable
> 
> 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>

Best Regards
Richard Zhu
> ---
>  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

RE: [PATCH v12 13/13] PCI: imx6: Disable clocks in reverse order of enable

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 2022年6月16日 7:16
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; Lucas Stach
> <l.stach@pengutronix.de>; Rob Herring <robh+dt@kernel.org>; Mark Brown
> <broonie@kernel.org>; Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Fabio
> Estevam <festevam@gmail.com>; Francesco Dolcini
> <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>; Bjorn Helgaas <bhelgaas@google.com>
> Subject: [PATCH v12 13/13] PCI: imx6: Disable clocks in reverse order of enable
> 
> 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>

Best Regards
Richard Zhu
> ---
>  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 bd736aff94a3..738b5a732cef 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)  {
> -	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);
> +	clk_disable_unprepare(imx6_pcie->pcie_bus);
> +	clk_disable_unprepare(imx6_pcie->pcie_phy);
>  }
> 
>  static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> --
> 2.25.1

Re: [PATCH v12 09/13] PCI: imx6: Turn off regulator when system is in suspend mode

[Bjorn Helgaas]

On Thu, Jun 16, 2022 at 02:23:29AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > Sent: 2022年6月16日 7:31
> > To: Hongxing Zhu <hongxing.zhu@nxp.com>; Lucas Stach
> > <l.stach@pengutronix.de>; Rob Herring <robh+dt@kernel.org>; Mark Brown
> > <broonie@kernel.org>; Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Fabio
> > Estevam <festevam@gmail.com>; Francesco Dolcini
> > <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>; Bjorn Helgaas <bhelgaas@google.com>
> > Subject: Re: [PATCH v12 09/13] PCI: imx6: Turn off regulator when system is in
> > suspend mode
> > 
> > On Wed, Jun 15, 2022 at 06:15:47PM -0500, Bjorn Helgaas wrote:
> > > From: Richard Zhu <hongxing.zhu@nxp.com>
> > >
> > > 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%7C8dea46d908d34cb4825808da4
> > f270eaf%
> > >
> > 7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379093264156415
> > 35%7CUnkn
> > >
> > own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> > haWwi
> > >
> > LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=tqzlcCmtSkUNxNzJ0Fr5o
> > CR3X2CTH
> > > 4dwUJS4NXt5KRc%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 6eddd0b5f628..537b8a2e0e3b 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, err;
> > >
> > > -	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", @@
> > -796,7
> > > +786,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", @@
> > -1023,6
> > > +1013,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> > >  		break;
> > >  	}
> > >
> > > +	if (imx6_pcie->vpcie)
> > > +		regulator_disable(imx6_pcie->vpcie);
> > 
> > This is a little bit ugly because imx6_pcie_suspend_noirq() and
> > imx6_pcie_resume_noirq() are not symmetric.
> > 
> > We call regulator_disable() directly here in imx6_pcie_suspend_noirq(), but the
> > corresponding regulator_enable() is buried in imx6_pcie_deassert_core_reset().
> > 
> > It would be nicer if the suspend and resume paths looked more similar, as
> > rockchip_pcie_suspend_noirq() and rockchip_pcie_resume_noirq() do, for
> > example.
> Yes, it is. The regulator_disable()/regulator_enable() are not invoked
>  symmetrically.
> 
> In the original codes, the regulator_disable() is contained in
> imx6_pcie_assert_core_reset() and with _eanbled() check.
> And regulator_enable() is embedded in imx6_pcie_deassert_core_reset().
> Both the regulator disable and enable are invoked in resume before.
> I move the regulator_disable to suspend mode refer to Lucas' comments.
> 
> Refer to the current situation, how about to move the regulator_disable()
>  out of imx6_pcie_deassert_core_reset() too?

I think it's important to tweak this closer to the pattern of other
drivers like exynos and rockchip.

exynos_pcie_resume_noirq() calls exynos_pcie_host_init().
imx6_pcie_resume_noirq() is VERY similar to imx6_pcie_host_init(), and
I think it should call imx6_pcie_host_init() instead of duplicating
that code.  The only real difference is that imx6_pcie_host_init()
calls imx6_setup_phy_mpll() while imx6_pcie_resume_noirq() does not.
I suspect that's a bug.

I don't think "regulator_enable(imx6_pcie->vpcie)" really has anything
to do with imx6_pcie_deassert_core_reset() and it should be moved out.
The typical pattern is to enable regulators in *_probe() and
*_resume_noirq() and disable them in *_remove() and *_suspend_noirq().
I think you should do the same unless there's some reason you can't.

Can you give this a try, starting with this v12 posting?

Bjorn

RE: [PATCH v12 09/13] PCI: imx6: Turn off regulator when system is in suspend mode

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 2022年6月16日 23:56
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>; Rob Herring <robh+dt@kernel.org>;
> Mark Brown <broonie@kernel.org>; Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com>; Fabio Estevam <festevam@gmail.com>;
> Francesco Dolcini <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>; Bjorn Helgaas
> <bhelgaas@google.com>
> Subject: Re: [PATCH v12 09/13] PCI: imx6: Turn off regulator when system is in
> suspend mode
> 
> On Thu, Jun 16, 2022 at 02:23:29AM +0000, Hongxing Zhu wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > Sent: 2022年6月16日 7:31
> > > To: Hongxing Zhu <hongxing.zhu@nxp.com>; Lucas Stach
> > > <l.stach@pengutronix.de>; Rob Herring <robh+dt@kernel.org>; Mark
> > > Brown <broonie@kernel.org>; Lorenzo Pieralisi
> > > <lorenzo.pieralisi@arm.com>; Fabio Estevam <festevam@gmail.com>;
> > > Francesco Dolcini <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>; Bjorn Helgaas <bhelgaas@google.com>
> > > Subject: Re: [PATCH v12 09/13] PCI: imx6: Turn off regulator when
> > > system is in suspend mode
> > >
> > > On Wed, Jun 15, 2022 at 06:15:47PM -0500, Bjorn Helgaas wrote:
> > > > From: Richard Zhu <hongxing.zhu@nxp.com>
> > > >
> > > > 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&am
> p
> > > > ;d
> > > at
> > > >
> > >
> a=05%7C01%7Chongxing.zhu%40nxp.com%7C8dea46d908d34cb4825808da4
> > > f270eaf%
> > > >
> > >
> 7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379093264156415
> > > 35%7CUnkn
> > > >
> > >
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> > > haWwi
> > > >
> > >
> LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=tqzlcCmtSkUNxNzJ0Fr5o
> > > CR3X2CTH
> > > > 4dwUJS4NXt5KRc%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 6eddd0b5f628..537b8a2e0e3b 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, err;
> > > >
> > > > -	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", @@
> > > -796,7
> > > > +786,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", @@
> > > -1023,6
> > > > +1013,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> > > >  		break;
> > > >  	}
> > > >
> > > > +	if (imx6_pcie->vpcie)
> > > > +		regulator_disable(imx6_pcie->vpcie);
> > >
> > > This is a little bit ugly because imx6_pcie_suspend_noirq() and
> > > imx6_pcie_resume_noirq() are not symmetric.
> > >
> > > We call regulator_disable() directly here in
> > > imx6_pcie_suspend_noirq(), but the corresponding regulator_enable() is
> buried in imx6_pcie_deassert_core_reset().
> > >
> > > It would be nicer if the suspend and resume paths looked more
> > > similar, as
> > > rockchip_pcie_suspend_noirq() and rockchip_pcie_resume_noirq() do,
> > > for example.
> > Yes, it is. The regulator_disable()/regulator_enable() are not invoked
> > symmetrically.
> >
> > In the original codes, the regulator_disable() is contained in
> > imx6_pcie_assert_core_reset() and with _eanbled() check.
> > And regulator_enable() is embedded in imx6_pcie_deassert_core_reset().
> > Both the regulator disable and enable are invoked in resume before.
> > I move the regulator_disable to suspend mode refer to Lucas' comments.
> >
> > Refer to the current situation, how about to move the
> > regulator_disable()  out of imx6_pcie_deassert_core_reset() too?
> 
> I think it's important to tweak this closer to the pattern of other drivers like
> exynos and rockchip.
> 
> exynos_pcie_resume_noirq() calls exynos_pcie_host_init().
> imx6_pcie_resume_noirq() is VERY similar to imx6_pcie_host_init(), and I think
> it should call imx6_pcie_host_init() instead of duplicating that code.  The only
> real difference is that imx6_pcie_host_init() calls imx6_setup_phy_mpll() while
> imx6_pcie_resume_noirq() does not.
> I suspect that's a bug.
> 
> I don't think "regulator_enable(imx6_pcie->vpcie)" really has anything to do
> with imx6_pcie_deassert_core_reset() and it should be moved out.
> The typical pattern is to enable regulators in *_probe() and
> *_resume_noirq() and disable them in *_remove() and *_suspend_noirq().
> I think you should do the same unless there's some reason you can't.
> 
> Can you give this a try, starting with this v12 posting?
Thanks a lot for your inspiration.
Exactly, these codes are almost same. The host_init can be invoked directly
 in resume. I would re-change the #12/13 of v12, and send the v13 later.

Best Regards
Richard Zhu

> 
> Bjorn