[PATCH v9 0/8] PCI: imx6: refine codes and add compliance tests mode support

[PATCH v9 0/8] PCI: imx6: refine codes and add compliance tests mode support

[Richard Zhu]

This series patches refine pci-imx6 driver and do the following main changes.
- Encapsulate the clock enable into one standalone function
- Add the error propagation from host_init
- Disable the regulators and clocks when link never comes up
- Add the compliance tests mode support
- Move the phy driver callbacks to the proper places

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.

drivers/pci/controller/dwc/pci-imx6.c | 277 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------
1 file changed, 188 insertions(+), 89 deletions(-)

[PATCH v9 1/8] PCI: imx6: Encapsulate the clock enable into one
[PATCH v9 2/8] PCI: imx6: Add the error propagation from host_init
[PATCH v9 3/8] PCI: imx6: Move imx6_pcie_clk_disable() earlier
[PATCH v9 4/8] PCI: imx6: Disable iMX6QDL PCIe REF clock when disable
[PATCH v9 5/8] PCI: imx6: Refine the regulator usage
[PATCH v9 6/8] PCI: imx6: Disable clocks and regulators after link is
[PATCH v9 7/8] PCI: imx6: Move the phy driver callbacks to the proper
[PATCH v9 8/8] PCI: imx6: Add compliance tests mode support

[PATCH v9 1/8] PCI: imx6: Encapsulate the clock enable into one standalone function

[Richard Zhu]

No function changes, just encapsulate the i.MX PCIe clocks enable
operations into one standalone function

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/pci/controller/dwc/pci-imx6.c | 79 ++++++++++++++++-----------
 1 file changed, 48 insertions(+), 31 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 6619e3caffe2..d3ef0e94ab26 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -477,38 +477,16 @@ 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)
+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;
 
-	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
-		ret = regulator_enable(imx6_pcie->vpcie);
-		if (ret) {
-			dev_err(dev, "failed to enable vpcie regulator: %d\n",
-				ret);
-			return;
-		}
-	}
-
 	ret = clk_prepare_enable(imx6_pcie->pcie_phy);
 	if (ret) {
 		dev_err(dev, "unable to enable pcie_phy clock\n");
-		goto err_pcie_phy;
+		return ret;
 	}
 
 	ret = clk_prepare_enable(imx6_pcie->pcie_bus);
@@ -539,6 +517,51 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	}
 	/* 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 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;
+	struct device *dev = pci->dev;
+	int ret;
+
+	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
+		ret = regulator_enable(imx6_pcie->vpcie);
+		if (ret) {
+			dev_err(dev, "failed to enable vpcie regulator: %d\n",
+				ret);
+			return;
+		}
+	}
+
+	ret = imx6_pcie_clk_enable(imx6_pcie);
+	if (ret) {
+		dev_err(dev, "unable to enable pcie clocks\n");
+		goto err_clks;
+	}
 
 	/* Some boards don't have PCIe reset GPIO. */
 	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
@@ -597,13 +620,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 v9 2/8] PCI: imx6: Add the error propagation from host_init

[Richard Zhu]

Since there is error return check of the host_init callback, add error
check to imx6_pcie_deassert_core_reset() function, and change the
function type accordingly.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/pci/controller/dwc/pci-imx6.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index d3ef0e94ab26..1d3a8a7cafc2 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -542,24 +542,24 @@ static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
 		dev_err(dev, "PCIe PLL lock timeout\n");
 }
 
-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;
-	int ret;
+	int ret, err;
 
 	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
 		ret = regulator_enable(imx6_pcie->vpcie);
 		if (ret) {
 			dev_err(dev, "failed to enable vpcie regulator: %d\n",
 				ret);
-			return;
+			return ret;
 		}
 	}
 
-	ret = imx6_pcie_clk_enable(imx6_pcie);
-	if (ret) {
-		dev_err(dev, "unable to enable pcie clocks\n");
+	err = imx6_pcie_clk_enable(imx6_pcie);
+	if (err) {
+		dev_err(dev, "unable to enable pcie clocks: %d\n", err);
 		goto err_clks;
 	}
 
@@ -618,7 +618,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 		break;
 	}
 
-	return;
+	return 0;
 
 err_clks:
 	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
@@ -627,6 +627,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 void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
@@ -878,11 +879,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 v9 3/8] PCI: imx6: Move imx6_pcie_clk_disable() earlier

[Richard Zhu]

Just move the imx6_pcie_clk_disable() to an earlier place without function
changes, since it wouldn't be only used in imx6_pcie_suspend_noirq() later.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 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 1d3a8a7cafc2..9b0eac64badc 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -529,6 +529,30 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
 	return ret;
 }
 
+static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
+{
+	clk_disable_unprepare(imx6_pcie->pcie);
+	clk_disable_unprepare(imx6_pcie->pcie_phy);
+	clk_disable_unprepare(imx6_pcie->pcie_bus);
+
+	switch (imx6_pcie->drvdata->variant) {
+	case IMX6SX:
+		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
+		break;
+	case IMX7D:
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
+				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
+		break;
+	case IMX8MQ:
+	case IMX8MM:
+		clk_disable_unprepare(imx6_pcie->pcie_aux);
+		break;
+	default:
+		break;
+	}
+}
+
 static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
 {
 	u32 val;
@@ -961,30 +985,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 v9 4/8] PCI: imx6: Disable iMX6QDL PCIe REF clock when disable PCIe clocks

[Richard Zhu]

When disable PCIe clocks, disable i.MX6QDL PCIe REF clock too.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 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 9b0eac64badc..7005a7910003 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -536,6 +536,14 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
 	clk_disable_unprepare(imx6_pcie->pcie_bus);
 
 	switch (imx6_pcie->drvdata->variant) {
+	case IMX6Q:
+	case IMX6QP:
+		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 IMX6SX:
 		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
 		break;
-- 
2.25.1

[PATCH v9 5/8] PCI: imx6: Refine the regulator usage

[Richard Zhu]

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

To keep the balance of the regulator usage counter, disable the regulator
just behind of imx6_pcie_assert_core_reset() in resume and shutdown.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.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 7005a7910003..3ce3993d5797 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -369,8 +369,6 @@ static int imx6_pcie_attach_pd(struct device *dev)
 
 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:
@@ -400,14 +398,6 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 				   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);
-	}
 }
 
 static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
@@ -580,7 +570,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",
@@ -653,7 +643,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",
@@ -1026,6 +1016,9 @@ static int imx6_pcie_resume_noirq(struct device *dev)
 		return 0;
 
 	imx6_pcie_assert_core_reset(imx6_pcie);
+	if (imx6_pcie->vpcie)
+		regulator_disable(imx6_pcie->vpcie);
+
 	imx6_pcie_init_phy(imx6_pcie);
 	imx6_pcie_deassert_core_reset(imx6_pcie);
 	dw_pcie_setup_rc(pp);
@@ -1259,6 +1252,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 v9 6/8] PCI: imx6: Disable clocks and regulators after link is down

[Richard Zhu]

Since i.MX PCIe doesn't support hot-plug, reduce power consumption
as much as possible by disabling clocks and regulators and returning
error when the link is down.

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 3ce3993d5797..d122c12193a6 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -845,7 +845,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_out;
 
 	if (pci->link_gen == 2) {
 		/* Allow Gen2 mode after the link is up. */
@@ -876,12 +878,14 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 			ret = imx6_pcie_wait_for_speed_change(imx6_pcie);
 			if (ret) {
 				dev_err(dev, "Failed to bring link up!\n");
-				goto err_reset_phy;
+				goto err_out;
 			}
 		}
 
 		/* 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_out;
 	} else {
 		dev_info(dev, "Link: Gen2 disabled\n");
 	}
@@ -890,11 +894,18 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 	dev_info(dev, "Link up, Gen%i\n", tmp & PCI_EXP_LNKSTA_CLS);
 	return 0;
 
-err_reset_phy:
+err_out:
 	dev_dbg(dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n",
 		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
 		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
 	imx6_pcie_reset_phy(imx6_pcie);
+	imx6_pcie_clk_disable(imx6_pcie);
+	if (imx6_pcie->phy != NULL) {
+		phy_power_off(imx6_pcie->phy);
+		phy_exit(imx6_pcie->phy);
+	}
+	if (imx6_pcie->vpcie)
+		regulator_disable(imx6_pcie->vpcie);
 	return ret;
 }
 
-- 
2.25.1

[PATCH v9 7/8] PCI: imx6: Move the phy driver callbacks to the proper places

[Richard Zhu]

To make it more reasonable, move the phy_power_on/phy_init callbacks to
the proper places.
- move the phy_power_on() out of imx6_pcie_clk_enable().
- move the phy_init() out of imx6_pcie_deassert_core_reset().

In order to save power consumption, turn off the clocks and regulators when
the imx6_pcie_host_init() return error.

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index d122c12193a6..f0ffd9011975 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -497,14 +497,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;
@@ -597,10 +589,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	switch (imx6_pcie->drvdata->variant) {
 	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);
@@ -918,15 +907,38 @@ 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 != NULL) {
+		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_exit0;
+	} else if (imx6_pcie->phy != NULL) {
+		ret = phy_init(imx6_pcie->phy);
+		if (ret) {
+			dev_err(dev, "waiting for phy ready timeout!\n");
+			goto err_exit1;
+		}
 	}
 
 	imx6_setup_phy_mpll(imx6_pcie);
 
 	return 0;
+
+err_exit1:
+	imx6_pcie_clk_disable(imx6_pcie);
+	if (imx6_pcie->vpcie)
+		regulator_disable(imx6_pcie->vpcie);
+err_exit0:
+	if (imx6_pcie->phy != NULL)
+		phy_power_off(imx6_pcie->phy);
+	return ret;
 }
 
 static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
@@ -1031,14 +1043,47 @@ static int imx6_pcie_resume_noirq(struct device *dev)
 		regulator_disable(imx6_pcie->vpcie);
 
 	imx6_pcie_init_phy(imx6_pcie);
-	imx6_pcie_deassert_core_reset(imx6_pcie);
-	dw_pcie_setup_rc(pp);
+	if (imx6_pcie->phy != NULL) {
+		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_exit0;
+	} else if (imx6_pcie->phy != NULL) {
+		ret = phy_init(imx6_pcie->phy);
+		if (ret) {
+			dev_err(dev, "pcie phy init failed.\n");
+			goto err_exit1;
+		}
+	}
 
+	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 (ret < 0) {
+		/*
+		 * Return ret directly, since there are error exit
+		 * handle in imx6_pcie_start_link()
+		 */
+		dev_err(dev, "pcie link is down after resume.\n");
+		return ret;
+	}
 
 	return 0;
+
+err_exit1:
+	imx6_pcie_clk_disable(imx6_pcie);
+	if (imx6_pcie->vpcie)
+		regulator_disable(imx6_pcie->vpcie);
+err_exit0:
+	if (imx6_pcie->phy != NULL)
+		phy_power_off(imx6_pcie->phy);
+	return ret;
 }
 #endif
 
-- 
2.25.1

[PATCH v9 8/8] PCI: imx6: Add compliance tests mode support

[Richard Zhu]

Refer to the Chapter 3.2 System Board Signal Quality of PCI Express
Architecture PHY Test Specification Revision 2.0.

Signal quality tests (for example: jitter, differential eye opening and
so on) can be executed with devices in the polling.compliance state.

To let the device support polling.compliance state, the clocks and powers
shouldn't be turned off when the probe of device driver fails.

Based on CLB (Compliance Load Board) Test Fixture and so on test
equipments, the PHY link would be down during the compliance tests.
Refer to this scenario, add the i.MX PCIe compliance tests mode enable
support, and keep the clocks and powers on, and finish the driver probe
without error return.

Use the "pci_imx6.compliance=1" in kernel command line to enable the
compliance tests mode.

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index f0ffd9011975..f78b59822626 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -146,6 +146,10 @@ 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 bool imx6_pcie_cmp_mode;
+module_param_named(compliance, imx6_pcie_cmp_mode, bool, 0644);
+MODULE_PARM_DESC(compliance, "i.MX PCIe compliance test mode (1=compliance test mode enabled)");
+
 static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
 {
 	struct dw_pcie *pci = imx6_pcie->pci;
@@ -826,10 +830,12 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 	 * started in Gen2 mode, there is a possibility the devices on the
 	 * bus will not be detected at all.  This happens with PCIe switches.
 	 */
-	tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
-	tmp &= ~PCI_EXP_LNKCAP_SLS;
-	tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
-	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
+	if (!imx6_pcie_cmp_mode) {
+		tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
+		tmp &= ~PCI_EXP_LNKCAP_SLS;
+		tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
+		dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
+	}
 
 	/* Start LTSSM. */
 	imx6_pcie_ltssm_enable(dev);
@@ -887,14 +893,16 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 	dev_dbg(dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n",
 		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
 		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
-	imx6_pcie_reset_phy(imx6_pcie);
-	imx6_pcie_clk_disable(imx6_pcie);
-	if (imx6_pcie->phy != NULL) {
-		phy_power_off(imx6_pcie->phy);
-		phy_exit(imx6_pcie->phy);
+	if (!imx6_pcie_cmp_mode) {
+		imx6_pcie_reset_phy(imx6_pcie);
+		imx6_pcie_clk_disable(imx6_pcie);
+		if (imx6_pcie->phy != NULL) {
+			phy_power_off(imx6_pcie->phy);
+			phy_exit(imx6_pcie->phy);
+		}
+		if (imx6_pcie->vpcie)
+			regulator_disable(imx6_pcie->vpcie);
 	}
-	if (imx6_pcie->vpcie)
-		regulator_disable(imx6_pcie->vpcie);
 	return ret;
 }
 
@@ -1289,8 +1297,15 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 		return ret;
 
 	ret = dw_pcie_host_init(&pci->pp);
-	if (ret < 0)
+	if (ret < 0) {
+		if (imx6_pcie_cmp_mode) {
+			dev_info(dev, "driver loaded with compliance test mode enabled\n");
+			ret = 0;
+		} else {
+			dev_err(dev, "unable to add PCIe port\n");
+		}
 		return ret;
+	}
 
 	if (pci_msi_enabled()) {
 		u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
-- 
2.25.1

Re: [PATCH v9 3/8] PCI: imx6: Move imx6_pcie_clk_disable() earlier

[Lucas Stach]

Am Freitag, dem 06.05.2022 um 09:47 +0800 schrieb Richard Zhu:
> Just move the imx6_pcie_clk_disable() to an earlier place without function
> changes, since it wouldn't be only used in imx6_pcie_suspend_noirq() later.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.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 1d3a8a7cafc2..9b0eac64badc 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -529,6 +529,30 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
>  	return ret;
>  }
>  
> +static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
> +{
> +	clk_disable_unprepare(imx6_pcie->pcie);
> +	clk_disable_unprepare(imx6_pcie->pcie_phy);
> +	clk_disable_unprepare(imx6_pcie->pcie_bus);
> +
> +	switch (imx6_pcie->drvdata->variant) {
> +	case IMX6SX:
> +		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> +		break;
> +	case IMX7D:
> +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> +				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> +				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> +		break;
> +	case IMX8MQ:
> +	case IMX8MM:
> +		clk_disable_unprepare(imx6_pcie->pcie_aux);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
>  static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
>  {
>  	u32 val;
> @@ -961,30 +985,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);

Re: [PATCH v9 4/8] PCI: imx6: Disable iMX6QDL PCIe REF clock when disable PCIe clocks

[Lucas Stach]

Am Freitag, dem 06.05.2022 um 09:47 +0800 schrieb Richard Zhu:
> When disable PCIe clocks, disable i.MX6QDL PCIe REF clock too.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.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 9b0eac64badc..7005a7910003 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -536,6 +536,14 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
>  	clk_disable_unprepare(imx6_pcie->pcie_bus);
>  
>  	switch (imx6_pcie->drvdata->variant) {
> +	case IMX6Q:
> +	case IMX6QP:
> +		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 IMX6SX:
>  		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
>  		break;

Re: [PATCH v9 5/8] PCI: imx6: Refine the regulator usage

[Lucas Stach]

Am Freitag, dem 06.05.2022 um 09:47 +0800 schrieb Richard Zhu:
> The driver should undo any enables it did itself. The regulator disable
> shouldn't be basing decisions on regulator_is_enabled().
> 
> To keep the balance of the regulator usage counter, disable the regulator
> just behind of imx6_pcie_assert_core_reset() in resume and shutdown.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.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 7005a7910003..3ce3993d5797 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -369,8 +369,6 @@ static int imx6_pcie_attach_pd(struct device *dev)
>  
>  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:
> @@ -400,14 +398,6 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
>  				   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);
> -	}
>  }
>  
>  static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
> @@ -580,7 +570,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",
> @@ -653,7 +643,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",
> @@ -1026,6 +1016,9 @@ static int imx6_pcie_resume_noirq(struct device *dev)
>  		return 0;
>  
>  	imx6_pcie_assert_core_reset(imx6_pcie);
> +	if (imx6_pcie->vpcie)
> +		regulator_disable(imx6_pcie->vpcie);
> +
This one looks misplaced. Surely you want the regulator to be on when
resuming the PCIe subsystem. Isn't this just papering over a wrong
usage count here, because there is no regulator_disable in
imx6_pcie_suspend_noirq, where I would expect this to happen?

Regards,
Lucas

>  	imx6_pcie_init_phy(imx6_pcie);
>  	imx6_pcie_deassert_core_reset(imx6_pcie);
>  	dw_pcie_setup_rc(pp);
> @@ -1259,6 +1252,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[] = {

Re: [PATCH v9 6/8] PCI: imx6: Disable clocks and regulators after link is down

[Lucas Stach]

Am Freitag, dem 06.05.2022 um 09:47 +0800 schrieb Richard Zhu:
> Since i.MX PCIe doesn't support hot-plug, reduce power consumption
> as much as possible by disabling clocks and regulators and returning
> error when the link is down.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 3ce3993d5797..d122c12193a6 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -845,7 +845,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_out;

This adds back error handling that has been intentionally removed in
f81f095e8771 ("PCI: imx6: Allow to probe when dw_pcie_wait_for_link()
fails"). While I agree that disabling the clocks and regulators is the
right thing to do when we don't manage to get a link, we should still
allow the driver to probe, so please add a "ret = 0" to this newly
added non-fatal error paths.

>  
>  	if (pci->link_gen == 2) {
>  		/* Allow Gen2 mode after the link is up. */
> @@ -876,12 +878,14 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
>  			ret = imx6_pcie_wait_for_speed_change(imx6_pcie);
>  			if (ret) {
>  				dev_err(dev, "Failed to bring link up!\n");
> -				goto err_reset_phy;
> +				goto err_out;
>  			}
>  		}
>  
>  		/* 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_out;
>  	} else {
>  		dev_info(dev, "Link: Gen2 disabled\n");
>  	}
> @@ -890,11 +894,18 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
>  	dev_info(dev, "Link up, Gen%i\n", tmp & PCI_EXP_LNKSTA_CLS);
>  	return 0;
>  
> -err_reset_phy:
> +err_out:
>  	dev_dbg(dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n",
>  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
>  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
>  	imx6_pcie_reset_phy(imx6_pcie);
> +	imx6_pcie_clk_disable(imx6_pcie);
> +	if (imx6_pcie->phy != NULL) {

Please use the more common if (imx6_pcie->phy) here.

Regards,
Lucas

> +		phy_power_off(imx6_pcie->phy);
> +		phy_exit(imx6_pcie->phy);
> +	}
> +	if (imx6_pcie->vpcie)
> +		regulator_disable(imx6_pcie->vpcie);
>  	return ret;
>  }
>  

Re: [PATCH v9 7/8] PCI: imx6: Move the phy driver callbacks to the proper places

[Lucas Stach]

Am Freitag, dem 06.05.2022 um 09:47 +0800 schrieb Richard Zhu:
> To make it more reasonable, move the phy_power_on/phy_init callbacks to
> the proper places.
> - move the phy_power_on() out of imx6_pcie_clk_enable().
> - move the phy_init() out of imx6_pcie_deassert_core_reset().
> 
> In order to save power consumption, turn off the clocks and regulators when
> the imx6_pcie_host_init() return error.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 77 +++++++++++++++++++++------
>  1 file changed, 61 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index d122c12193a6..f0ffd9011975 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -497,14 +497,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;
> @@ -597,10 +589,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  	switch (imx6_pcie->drvdata->variant) {
>  	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");

This looks fishy. You now fall-through from the 8MQ case to the 8MM
case, just to break there. Please move the 8MM case down to the 6QP
case that also doesn't need to do anything here.

>  		break;
>  	case IMX7D:
>  		reset_control_deassert(imx6_pcie->pciephy_reset);
> @@ -918,15 +907,38 @@ 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 != NULL) {

Same comment as on the last patch: drop the "!= NULL".

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

This label is not very descriptive. Please rename to something like
err_phy_off or something like that.

> +	} else if (imx6_pcie->phy != NULL) {

The else path isn't needed here. If there was an error you already
moved the contol flow to the error label. So drop the else and again
have this block under a simple "if (imx6_pcie->phy)" statement.

> +		ret = phy_init(imx6_pcie->phy);
> +		if (ret) {
> +			dev_err(dev, "waiting for phy ready timeout!\n");
> +			goto err_exit1;

Again, please give those labels a more descriptive name.

All of the above comments also apply to the changes in
imx6_pcie_resume_noirq.

Regards,
Lucas

> +		}
>  	}
>  
>  	imx6_setup_phy_mpll(imx6_pcie);
>  
>  	return 0;
> +
> +err_exit1:
> +	imx6_pcie_clk_disable(imx6_pcie);
> +	if (imx6_pcie->vpcie)
> +		regulator_disable(imx6_pcie->vpcie);
> +err_exit0:
> +	if (imx6_pcie->phy != NULL)
> +		phy_power_off(imx6_pcie->phy);
> +	return ret;
>  }
>  
>  static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
> @@ -1031,14 +1043,47 @@ static int imx6_pcie_resume_noirq(struct device *dev)
>  		regulator_disable(imx6_pcie->vpcie);
>  
>  	imx6_pcie_init_phy(imx6_pcie);
> -	imx6_pcie_deassert_core_reset(imx6_pcie);
> -	dw_pcie_setup_rc(pp);
> +	if (imx6_pcie->phy != NULL) {
> +		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_exit0;
> +	} else if (imx6_pcie->phy != NULL) {
> +		ret = phy_init(imx6_pcie->phy);
> +		if (ret) {
> +			dev_err(dev, "pcie phy init failed.\n");
> +			goto err_exit1;
> +		}
> +	}
>  
> +	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 (ret < 0) {
> +		/*
> +		 * Return ret directly, since there are error exit
> +		 * handle in imx6_pcie_start_link()
> +		 */
> +		dev_err(dev, "pcie link is down after resume.\n");
> +		return ret;
> +	}
>  
>  	return 0;
> +
> +err_exit1:
> +	imx6_pcie_clk_disable(imx6_pcie);
> +	if (imx6_pcie->vpcie)
> +		regulator_disable(imx6_pcie->vpcie);
> +err_exit0:
> +	if (imx6_pcie->phy != NULL)
> +		phy_power_off(imx6_pcie->phy);
> +	return ret;
>  }
>  #endif
>  

Re: [PATCH v9 8/8] PCI: imx6: Add compliance tests mode support

[Lucas Stach]

Am Freitag, dem 06.05.2022 um 09:47 +0800 schrieb Richard Zhu:
> Refer to the Chapter 3.2 System Board Signal Quality of PCI Express
> Architecture PHY Test Specification Revision 2.0.
> 
> Signal quality tests (for example: jitter, differential eye opening and
> so on) can be executed with devices in the polling.compliance state.
> 
> To let the device support polling.compliance state, the clocks and powers
> shouldn't be turned off when the probe of device driver fails.
> 
> Based on CLB (Compliance Load Board) Test Fixture and so on test
> equipments, the PHY link would be down during the compliance tests.
> Refer to this scenario, add the i.MX PCIe compliance tests mode enable
> support, and keep the clocks and powers on, and finish the driver probe
> without error return.
> 
> Use the "pci_imx6.compliance=1" in kernel command line to enable the
> compliance tests mode.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 39 ++++++++++++++++++---------
>  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index f0ffd9011975..f78b59822626 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -146,6 +146,10 @@ 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 bool imx6_pcie_cmp_mode;
> +module_param_named(compliance, imx6_pcie_cmp_mode, bool, 0644);
> +MODULE_PARM_DESC(compliance, "i.MX PCIe compliance test mode (1=compliance test mode enabled)");
> +
>  static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
>  {
>  	struct dw_pcie *pci = imx6_pcie->pci;
> @@ -826,10 +830,12 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
>  	 * started in Gen2 mode, there is a possibility the devices on the
>  	 * bus will not be detected at all.  This happens with PCIe switches.
>  	 */
> -	tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> -	tmp &= ~PCI_EXP_LNKCAP_SLS;
> -	tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
> -	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
> +	if (!imx6_pcie_cmp_mode) {
> +		tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> +		tmp &= ~PCI_EXP_LNKCAP_SLS;
> +		tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
> +		dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
> +	}
>  
>  	/* Start LTSSM. */
>  	imx6_pcie_ltssm_enable(dev);
> @@ -887,14 +893,16 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
>  	dev_dbg(dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n",
>  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
>  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> -	imx6_pcie_reset_phy(imx6_pcie);
> -	imx6_pcie_clk_disable(imx6_pcie);
> -	if (imx6_pcie->phy != NULL) {
> -		phy_power_off(imx6_pcie->phy);
> -		phy_exit(imx6_pcie->phy);
> +	if (!imx6_pcie_cmp_mode) {
> +		imx6_pcie_reset_phy(imx6_pcie);
> +		imx6_pcie_clk_disable(imx6_pcie);
> +		if (imx6_pcie->phy != NULL) {
> +			phy_power_off(imx6_pcie->phy);
> +			phy_exit(imx6_pcie->phy);
> +		}
> +		if (imx6_pcie->vpcie)
> +			regulator_disable(imx6_pcie->vpcie);
>  	}
> -	if (imx6_pcie->vpcie)
> -		regulator_disable(imx6_pcie->vpcie);
>  	return ret;
>  }
>  
> @@ -1289,8 +1297,15 @@ static int imx6_pcie_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	ret = dw_pcie_host_init(&pci->pp);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		if (imx6_pcie_cmp_mode) {
> +			dev_info(dev, "driver loaded with compliance test mode enabled\n");
> +			ret = 0;
> +		} else {
> +			dev_err(dev, "unable to add PCIe port\n");
> +		}
>  		return ret;
> +	}

If you drop the error return from imx6_pcie_start_link, like I
suggested in patch 6/8, you don't need this block as dw_pcie_host_init
will succeed even if the link is down or in compliance test mode.

Regards,
Lucas

>  
>  	if (pci_msi_enabled()) {
>  		u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);

Re: [PATCH v9 1/8] PCI: imx6: Encapsulate the clock enable into one standalone function

[Bjorn Helgaas]

On Fri, May 06, 2022 at 09:47:02AM +0800, Richard Zhu wrote:
> No function changes, just encapsulate the i.MX PCIe clocks enable
> operations into one standalone function

Since there's another rev coming for other reasons, maybe mention the
name of the "one standalone function."

s/clocks enable/clock enable/ above

Re: [PATCH v9 2/8] PCI: imx6: Add the error propagation from host_init

[Bjorn Helgaas]

On Fri, May 06, 2022 at 09:47:03AM +0800, Richard Zhu wrote:
> Since there is error return check of the host_init callback, add error
> check to imx6_pcie_deassert_core_reset() function, and change the
> function type accordingly.

> @@ -878,11 +879,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);

Other messages from this driver do not include a trailing period.

Re: [PATCH v9 5/8] PCI: imx6: Refine the regulator usage

[Bjorn Helgaas]

On Fri, May 06, 2022 at 09:47:06AM +0800, Richard Zhu wrote:
> The driver should undo any enables it did itself. The regulator disable
> shouldn't be basing decisions on regulator_is_enabled().
> 
> To keep the balance of the regulator usage counter, disable the regulator
> just behind of imx6_pcie_assert_core_reset() in resume and shutdown.

In subject, "Refine" doesn't tell me anything about what's happening
here.

Re: [PATCH v9 7/8] PCI: imx6: Move the phy driver callbacks to the proper places

[Bjorn Helgaas]

On Fri, May 06, 2022 at 09:47:08AM +0800, Richard Zhu wrote:
> To make it more reasonable, move the phy_power_on/phy_init callbacks to
> the proper places.
> - move the phy_power_on() out of imx6_pcie_clk_enable().
> - move the phy_init() out of imx6_pcie_deassert_core_reset().

I'm not sure what "make it more reasonable" is telling me.  In subject
line and commit log, please say something more specific than "the
proper places."  

It's probably more important to say where they are moving *to* than
where they're moving *out of*.

> In order to save power consumption, turn off the clocks and regulators when
> the imx6_pcie_host_init() return error.

Is the power savings the *reason* for this change?  I can't tell from
the commit log.

RE: [PATCH v9 5/8] PCI: imx6: Refine the regulator usage

[Hongxing Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年6月8日 15:27
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> jingoohan1@gmail.com; festevam@gmail.com;
> francesco.dolcini@toradex.com
> Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v9 5/8] PCI: imx6: Refine the regulator usage
> 
> Am Freitag, dem 06.05.2022 um 09:47 +0800 schrieb Richard Zhu:
> > The driver should undo any enables it did itself. The regulator
> > disable shouldn't be basing decisions on regulator_is_enabled().
> >
> > To keep the balance of the regulator usage counter, disable the
> > regulator just behind of imx6_pcie_assert_core_reset() in resume and
> shutdown.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.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 7005a7910003..3ce3993d5797 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -369,8 +369,6 @@ static int imx6_pcie_attach_pd(struct device *dev)
> >
> >  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:
> > @@ -400,14 +398,6 @@ static void imx6_pcie_assert_core_reset(struct
> imx6_pcie *imx6_pcie)
> >  				   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);
> > -	}
> >  }
> >
> >  static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie
> > *imx6_pcie) @@ -580,7 +570,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", @@
> -653,7
> > +643,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", @@
> -1026,6
> > +1016,9 @@ static int imx6_pcie_resume_noirq(struct device *dev)
> >  		return 0;
> >
> >  	imx6_pcie_assert_core_reset(imx6_pcie);
> > +	if (imx6_pcie->vpcie)
> > +		regulator_disable(imx6_pcie->vpcie);
> > +
> This one looks misplaced. Surely you want the regulator to be on when
> resuming the PCIe subsystem. Isn't this just papering over a wrong usage count
> here, because there is no regulator_disable in imx6_pcie_suspend_noirq,
> where I would expect this to happen?
> 
Hi Lucas:
Thanks for your comments.
There was one regulator_disable() operation at the end of the
 imx6_pcie_assert_core_reset() function before.
When create the 5/8 patch, I follow the same behavior to disable the
regulator just behind the imx6_pcie_assert_core_reset() function.

Yes, it is. Imx6_pcie_suspend_noirq doesn't have the regulator_disable.
The regulaor_enable is contained in imx6_pcie_deassert_core_reset().
Both of the regulator_disable and regulator_enabe are invoked once in
 imx6_pcie_resume_noirq.
So, the regulator is on and has a balanced usage count after resume.
Best Regards
Richard Zhu

> Regards,
> Lucas
> 
> >  	imx6_pcie_init_phy(imx6_pcie);
> >  	imx6_pcie_deassert_core_reset(imx6_pcie);
> >  	dw_pcie_setup_rc(pp);
> > @@ -1259,6 +1252,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[] = {
> 

RE: [PATCH v9 6/8] PCI: imx6: Disable clocks and regulators after link is down

[Hongxing Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年6月8日 15:35
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> jingoohan1@gmail.com; festevam@gmail.com;
> francesco.dolcini@toradex.com
> Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v9 6/8] PCI: imx6: Disable clocks and regulators after link is
> down
> 
> Am Freitag, dem 06.05.2022 um 09:47 +0800 schrieb Richard Zhu:
> > Since i.MX PCIe doesn't support hot-plug, reduce power consumption as
> > much as possible by disabling clocks and regulators and returning
> > error when the link is down.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 3ce3993d5797..d122c12193a6 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -845,7 +845,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_out;
> 
> This adds back error handling that has been intentionally removed in
> f81f095e8771 ("PCI: imx6: Allow to probe when dw_pcie_wait_for_link() fails").
> While I agree that disabling the clocks and regulators is the right thing to do
> when we don't manage to get a link, we should still allow the driver to probe,
> so please add a "ret = 0" to this newly added non-fatal error paths.
> 
Thanks for your review comments.
There would be a long latency if the link is down and probe is finished
 successfully.
Since the dw_pcie_wait_for_link() would be invoked twice in every driver probe
 and resume operation later. Each dw_pcie_wait_for_link() would consume about
 90,000*10 ~ 100,000*10 u-seconds. I'm afraid that such a long latency would
 bring bad user experience.

Here are the logs when probe is allowed when PCIe link is down:
[   55.045954][ T1835] imx6q-pcie 5f000000.pcie: PM: calling imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x8 @ 1835, parent: bus@5f000000
...
[   56.074566][ T1835] imx6q-pcie 5f000000.pcie: Phy link never came up
[   57.074816][ T1835] imx6q-pcie 5f000000.pcie: Phy link never came up
...
[   57.182300][ T1835] imx6q-pcie 5f000000.pcie: PM: imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x8 returned 0 after 2136334 usecs

[   57.182347][ T1835] imx6q-pcie 5f010000.pcie: PM: calling imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x8 @ 1835, parent: bus@5f000000
...
[   58.210584][ T1835] imx6q-pcie 5f010000.pcie: Phy link never came up
[   59.210831][ T1835] imx6q-pcie 5f010000.pcie: Phy link never came up
...
[   59.318313][ T1835] imx6q-pcie 5f010000.pcie: PM: imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x8 returned 0 after 2135949 usecs

So, I'm prefer that it's better to let the probe failed when link is down. 
How do you think about that?

> >
> >  	if (pci->link_gen == 2) {
> >  		/* Allow Gen2 mode after the link is up. */ @@ -876,12 +878,14
> @@
> > static int imx6_pcie_start_link(struct dw_pcie *pci)
> >  			ret = imx6_pcie_wait_for_speed_change(imx6_pcie);
> >  			if (ret) {
> >  				dev_err(dev, "Failed to bring link up!\n");
> > -				goto err_reset_phy;
> > +				goto err_out;
> >  			}
> >  		}
> >
> >  		/* 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_out;
> >  	} else {
> >  		dev_info(dev, "Link: Gen2 disabled\n");
> >  	}
> > @@ -890,11 +894,18 @@ static int imx6_pcie_start_link(struct dw_pcie
> *pci)
> >  	dev_info(dev, "Link up, Gen%i\n", tmp & PCI_EXP_LNKSTA_CLS);
> >  	return 0;
> >
> > -err_reset_phy:
> > +err_out:
> >  	dev_dbg(dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n",
> >  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> >  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> >  	imx6_pcie_reset_phy(imx6_pcie);
> > +	imx6_pcie_clk_disable(imx6_pcie);
> > +	if (imx6_pcie->phy != NULL) {
> 
> Please use the more common if (imx6_pcie->phy) here.
> 
Okay. Thanks.

Best Regards
Richard Zhu

> Regards,
> Lucas
> 
> > +		phy_power_off(imx6_pcie->phy);
> > +		phy_exit(imx6_pcie->phy);
> > +	}
> > +	if (imx6_pcie->vpcie)
> > +		regulator_disable(imx6_pcie->vpcie);
> >  	return ret;
> >  }
> >
> 

RE: [PATCH v9 7/8] PCI: imx6: Move the phy driver callbacks to the proper places

[Hongxing Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年6月8日 15:45
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> jingoohan1@gmail.com; festevam@gmail.com;
> francesco.dolcini@toradex.com
> Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v9 7/8] PCI: imx6: Move the phy driver callbacks to the
> proper places
> 
> Am Freitag, dem 06.05.2022 um 09:47 +0800 schrieb Richard Zhu:
> > To make it more reasonable, move the phy_power_on/phy_init callbacks
> > to the proper places.
> > - move the phy_power_on() out of imx6_pcie_clk_enable().
> > - move the phy_init() out of imx6_pcie_deassert_core_reset().
> >
> > In order to save power consumption, turn off the clocks and regulators
> > when the imx6_pcie_host_init() return error.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 77
> > +++++++++++++++++++++------
> >  1 file changed, 61 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index d122c12193a6..f0ffd9011975 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -497,14 +497,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;
> > @@ -597,10 +589,7 @@ static int imx6_pcie_deassert_core_reset(struct
> imx6_pcie *imx6_pcie)
> >  	switch (imx6_pcie->drvdata->variant) {
> >  	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");
> 
> This looks fishy. You now fall-through from the 8MQ case to the 8MM case,
> just to break there. Please move the 8MM case down to the 6QP case that also
> doesn't need to do anything here.
> 
Thanks for your comments.
Would be changed.

> >  		break;
> >  	case IMX7D:
> >  		reset_control_deassert(imx6_pcie->pciephy_reset);
> > @@ -918,15 +907,38 @@ 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 != NULL) {
> 
> Same comment as on the last patch: drop the "!= NULL".
Okay, got that, all of them would be changed.
Thanks.

> 
> > +		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_exit0;
> 
> This label is not very descriptive. Please rename to something like err_phy_off
> or something like that.
Okay. Thanks.

> 
> > +	} else if (imx6_pcie->phy != NULL) {
> 
> The else path isn't needed here. If there was an error you already moved the
> contol flow to the error label. So drop the else and again have this block under
> a simple "if (imx6_pcie->phy)" statement.
Okay. Thanks.

> 
> > +		ret = phy_init(imx6_pcie->phy);
> > +		if (ret) {
> > +			dev_err(dev, "waiting for phy ready timeout!\n");
> > +			goto err_exit1;
> 
> Again, please give those labels a more descriptive name.
> 
> All of the above comments also apply to the changes in
> imx6_pcie_resume_noirq.
Okay, thanks. Would be changed later.

Best Regards
Richard Zhu

> 
> Regards,
> Lucas
> 
> > +		}
> >  	}
> >
> >  	imx6_setup_phy_mpll(imx6_pcie);
> >
> >  	return 0;
> > +
> > +err_exit1:
> > +	imx6_pcie_clk_disable(imx6_pcie);
> > +	if (imx6_pcie->vpcie)
> > +		regulator_disable(imx6_pcie->vpcie);
> > +err_exit0:
> > +	if (imx6_pcie->phy != NULL)
> > +		phy_power_off(imx6_pcie->phy);
> > +	return ret;
> >  }
> >
> >  static const struct dw_pcie_host_ops imx6_pcie_host_ops = { @@
> > -1031,14 +1043,47 @@ static int imx6_pcie_resume_noirq(struct device
> *dev)
> >  		regulator_disable(imx6_pcie->vpcie);
> >
> >  	imx6_pcie_init_phy(imx6_pcie);
> > -	imx6_pcie_deassert_core_reset(imx6_pcie);
> > -	dw_pcie_setup_rc(pp);
> > +	if (imx6_pcie->phy != NULL) {
> > +		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_exit0;
> > +	} else if (imx6_pcie->phy != NULL) {
> > +		ret = phy_init(imx6_pcie->phy);
> > +		if (ret) {
> > +			dev_err(dev, "pcie phy init failed.\n");
> > +			goto err_exit1;
> > +		}
> > +	}
> >
> > +	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 (ret < 0) {
> > +		/*
> > +		 * Return ret directly, since there are error exit
> > +		 * handle in imx6_pcie_start_link()
> > +		 */
> > +		dev_err(dev, "pcie link is down after resume.\n");
> > +		return ret;
> > +	}
> >
> >  	return 0;
> > +
> > +err_exit1:
> > +	imx6_pcie_clk_disable(imx6_pcie);
> > +	if (imx6_pcie->vpcie)
> > +		regulator_disable(imx6_pcie->vpcie);
> > +err_exit0:
> > +	if (imx6_pcie->phy != NULL)
> > +		phy_power_off(imx6_pcie->phy);
> > +	return ret;
> >  }
> >  #endif
> >
> 

RE: [PATCH v9 8/8] PCI: imx6: Add compliance tests mode support

[Hongxing Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年6月8日 15:49
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> jingoohan1@gmail.com; festevam@gmail.com;
> francesco.dolcini@toradex.com
> Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v9 8/8] PCI: imx6: Add compliance tests mode support
> 
> Am Freitag, dem 06.05.2022 um 09:47 +0800 schrieb Richard Zhu:
> > Refer to the Chapter 3.2 System Board Signal Quality of PCI Express
> > Architecture PHY Test Specification Revision 2.0.
> >
> > Signal quality tests (for example: jitter, differential eye opening
> > and so on) can be executed with devices in the polling.compliance state.
> >
> > To let the device support polling.compliance state, the clocks and
> > powers shouldn't be turned off when the probe of device driver fails.
> >
> > Based on CLB (Compliance Load Board) Test Fixture and so on test
> > equipments, the PHY link would be down during the compliance tests.
> > Refer to this scenario, add the i.MX PCIe compliance tests mode enable
> > support, and keep the clocks and powers on, and finish the driver
> > probe without error return.
> >
> > Use the "pci_imx6.compliance=1" in kernel command line to enable the
> > compliance tests mode.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 39
> > ++++++++++++++++++---------
> >  1 file changed, 27 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index f0ffd9011975..f78b59822626 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -146,6 +146,10 @@ 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 bool imx6_pcie_cmp_mode;
> > +module_param_named(compliance, imx6_pcie_cmp_mode, bool, 0644);
> > +MODULE_PARM_DESC(compliance, "i.MX PCIe compliance test mode
> > +(1=compliance test mode enabled)");
> > +
> >  static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool
> > exp_val)  {
> >  	struct dw_pcie *pci = imx6_pcie->pci; @@ -826,10 +830,12 @@ static
> > int imx6_pcie_start_link(struct dw_pcie *pci)
> >  	 * started in Gen2 mode, there is a possibility the devices on the
> >  	 * bus will not be detected at all.  This happens with PCIe switches.
> >  	 */
> > -	tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> > -	tmp &= ~PCI_EXP_LNKCAP_SLS;
> > -	tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
> > -	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
> > +	if (!imx6_pcie_cmp_mode) {
> > +		tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> > +		tmp &= ~PCI_EXP_LNKCAP_SLS;
> > +		tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
> > +		dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
> > +	}
> >
> >  	/* Start LTSSM. */
> >  	imx6_pcie_ltssm_enable(dev);
> > @@ -887,14 +893,16 @@ static int imx6_pcie_start_link(struct dw_pcie
> *pci)
> >  	dev_dbg(dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n",
> >  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> >  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> > -	imx6_pcie_reset_phy(imx6_pcie);
> > -	imx6_pcie_clk_disable(imx6_pcie);
> > -	if (imx6_pcie->phy != NULL) {
> > -		phy_power_off(imx6_pcie->phy);
> > -		phy_exit(imx6_pcie->phy);
> > +	if (!imx6_pcie_cmp_mode) {
> > +		imx6_pcie_reset_phy(imx6_pcie);
> > +		imx6_pcie_clk_disable(imx6_pcie);
> > +		if (imx6_pcie->phy != NULL) {
> > +			phy_power_off(imx6_pcie->phy);
> > +			phy_exit(imx6_pcie->phy);
> > +		}
> > +		if (imx6_pcie->vpcie)
> > +			regulator_disable(imx6_pcie->vpcie);
> >  	}
> > -	if (imx6_pcie->vpcie)
> > -		regulator_disable(imx6_pcie->vpcie);
> >  	return ret;
> >  }
> >
> > @@ -1289,8 +1297,15 @@ static int imx6_pcie_probe(struct
> platform_device *pdev)
> >  		return ret;
> >
> >  	ret = dw_pcie_host_init(&pci->pp);
> > -	if (ret < 0)
> > +	if (ret < 0) {
> > +		if (imx6_pcie_cmp_mode) {
> > +			dev_info(dev, "driver loaded with compliance test mode
> enabled\n");
> > +			ret = 0;
> > +		} else {
> > +			dev_err(dev, "unable to add PCIe port\n");
> > +		}
> >  		return ret;
> > +	}
> 
> If you drop the error return from imx6_pcie_start_link, like I suggested in patch
> 6/8, you don't need this block as dw_pcie_host_init will succeed even if the
> link is down or in compliance test mode.
Thanks for your review comments.
As what I said in the 6/8 patch, there would be a long latency in the probe and
 every resume operation if the probe is finished successfully when link is down.
With this extreme long latency, I'm afraid that user would have a bad
experience and wouldn't accept such kind of long latency in every
suspend/resume operations.

Best Regards
Richard Zhu

> 
> Regards,
> Lucas
> 
> >
> >  	if (pci_msi_enabled()) {
> >  		u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> 

RE: [PATCH v9 1/8] PCI: imx6: Encapsulate the clock enable into one standalone function

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 2022年6月9日 2:52
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org;
> broonie@kernel.org; lorenzo.pieralisi@arm.com; jingoohan1@gmail.com;
> festevam@gmail.com; francesco.dolcini@toradex.com;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v9 1/8] PCI: imx6: Encapsulate the clock enable into one
> standalone function
> 
> On Fri, May 06, 2022 at 09:47:02AM +0800, Richard Zhu wrote:
> > No function changes, just encapsulate the i.MX PCIe clocks enable
> > operations into one standalone function
> 
> Since there's another rev coming for other reasons, maybe mention the name
> of the "one standalone function."
> 
> s/clocks enable/clock enable/ above
Okay, thanks.

Best Regards
Richard Zhu

RE: [PATCH v9 2/8] PCI: imx6: Add the error propagation from host_init

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 2022年6月9日 2:53
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org;
> broonie@kernel.org; lorenzo.pieralisi@arm.com; jingoohan1@gmail.com;
> festevam@gmail.com; francesco.dolcini@toradex.com;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v9 2/8] PCI: imx6: Add the error propagation from
> host_init
> 
> On Fri, May 06, 2022 at 09:47:03AM +0800, Richard Zhu wrote:
> > Since there is error return check of the host_init callback, add error
> > check to imx6_pcie_deassert_core_reset() function, and change the
> > function type accordingly.
> 
> > @@ -878,11 +879,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);
> 
> Other messages from this driver do not include a trailing period.
Okay, to keep alignment, would remove the trailing period.
Thanks.

Best Regards
Richard Zhu

RE: [PATCH v9 5/8] PCI: imx6: Refine the regulator usage

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 2022年6月9日 2:55
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org;
> broonie@kernel.org; lorenzo.pieralisi@arm.com; jingoohan1@gmail.com;
> festevam@gmail.com; francesco.dolcini@toradex.com;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v9 5/8] PCI: imx6: Refine the regulator usage
> 
> On Fri, May 06, 2022 at 09:47:06AM +0800, Richard Zhu wrote:
> > The driver should undo any enables it did itself. The regulator
> > disable shouldn't be basing decisions on regulator_is_enabled().
> >
> > To keep the balance of the regulator usage counter, disable the
> > regulator just behind of imx6_pcie_assert_core_reset() in resume and
> shutdown.
> 
> In subject, "Refine" doesn't tell me anything about what's happening here.
Thanks for your comments.
How about the following one?
PCI: imx6: Do regulator disable without the regulator_is_enabled check

Best Regards
Richard Zhu

RE: [PATCH v9 7/8] PCI: imx6: Move the phy driver callbacks to the proper places

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 2022年6月9日 2:58
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org;
> broonie@kernel.org; lorenzo.pieralisi@arm.com; jingoohan1@gmail.com;
> festevam@gmail.com; francesco.dolcini@toradex.com;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v9 7/8] PCI: imx6: Move the phy driver callbacks to the
> proper places
> 
> On Fri, May 06, 2022 at 09:47:08AM +0800, Richard Zhu wrote:
> > To make it more reasonable, move the phy_power_on/phy_init callbacks
> > to the proper places.
> > - move the phy_power_on() out of imx6_pcie_clk_enable().
> > - move the phy_init() out of imx6_pcie_deassert_core_reset().
> 
> I'm not sure what "make it more reasonable" is telling me.  In subject line and
> commit log, please say something more specific than "the proper places."
> 
> It's probably more important to say where they are moving *to* than where
> they're moving *out of*.
Thanks for your comments.
In another review loop listed below, Lucas used said that it's not good to hide
 PHY init in imx6_pcie_assert_core_reset()
So, I make a try to move the phy_init() out of imx6_pcie_assert_core_reset().
And move phy_power_on() out of imx6_pcie_clk_enable() accordingly.
https://patchwork.kernel.org/project/linux-pci/patch/1646289275-17813-1-git-send-email-hongxing.zhu@nxp.com/
Okay, I would specific that they are moving *to* later.

> 
> > In order to save power consumption, turn off the clocks and regulators
> > when the imx6_pcie_host_init() return error.
> 
> Is the power savings the *reason* for this change?  I can't tell from the
> commit log.
The error handling of the imx6_pcie_host_init() is not mentioned in the subject.
Should I split these changes into two patches?

Best Regards
Richard Zhu

Re: [PATCH v9 5/8] PCI: imx6: Refine the regulator usage

[Lucas Stach]

Am Donnerstag, dem 09.06.2022 um 06:17 +0000 schrieb Hongxing Zhu:
> > -----Original Message-----
> > From: Lucas Stach <l.stach@pengutronix.de>
> > Sent: 2022年6月8日 15:27
> > To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> > robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> > jingoohan1@gmail.com; festevam@gmail.com;
> > francesco.dolcini@toradex.com
> > Cc: linux-pci@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > <linux-imx@nxp.com>
> > Subject: Re: [PATCH v9 5/8] PCI: imx6: Refine the regulator usage
> > 
> > Am Freitag, dem 06.05.2022 um 09:47 +0800 schrieb Richard Zhu:
> > > The driver should undo any enables it did itself. The regulator
> > > disable shouldn't be basing decisions on regulator_is_enabled().
> > > 
> > > To keep the balance of the regulator usage counter, disable the
> > > regulator just behind of imx6_pcie_assert_core_reset() in resume
> > > and
> > shutdown.
> > > 
> > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.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 7005a7910003..3ce3993d5797 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -369,8 +369,6 @@ static int imx6_pcie_attach_pd(struct device
> > > *dev)
> > > 
> > >  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:
> > > @@ -400,14 +398,6 @@ static void
> > > imx6_pcie_assert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> > >  				   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);
> > > -	}
> > >  }
> > > 
> > >  static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie
> > > *imx6_pcie) @@ -580,7 +570,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", @@
> > -653,7
> > > +643,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", @@
> > -1026,6
> > > +1016,9 @@ static int imx6_pcie_resume_noirq(struct device *dev)
> > >  		return 0;
> > > 
> > >  	imx6_pcie_assert_core_reset(imx6_pcie);
> > > +	if (imx6_pcie->vpcie)
> > > +		regulator_disable(imx6_pcie->vpcie);
> > > +
> > This one looks misplaced. Surely you want the regulator to be on
> > when
> > resuming the PCIe subsystem. Isn't this just papering over a wrong
> > usage count
> > here, because there is no regulator_disable in
> > imx6_pcie_suspend_noirq,
> > where I would expect this to happen?
> > 
> Hi Lucas:
> Thanks for your comments.
> There was one regulator_disable() operation at the end of the
>  imx6_pcie_assert_core_reset() function before.
> When create the 5/8 patch, I follow the same behavior to disable the
> regulator just behind the imx6_pcie_assert_core_reset() function.
> 
> Yes, it is. Imx6_pcie_suspend_noirq doesn't have the
> regulator_disable.
> The regulaor_enable is contained in imx6_pcie_deassert_core_reset().
> Both of the regulator_disable and regulator_enabe are invoked once in
>  imx6_pcie_resume_noirq.
> So, the regulator is on and has a balanced usage count after resume.
> 

Yea, my argument is that when we are moving around the regulator
handling anyways, we should move the regulator_disable into the suspend
function. It's the right thing to do: we don't want the bus to be
powered when the system is in suspend and while the use-count is
correct, it's confusing to read the resume function otherwise.

Regards,
Lucas

Re: [PATCH v9 6/8] PCI: imx6: Disable clocks and regulators after link is down

[Francesco Dolcini]

On Thu, Jun 09, 2022 at 06:17:46AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Lucas Stach <l.stach@pengutronix.de>
> > Sent: 2022年6月8日 15:35
> > To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> > robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> > jingoohan1@gmail.com; festevam@gmail.com;
> > francesco.dolcini@toradex.com
> > Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > <linux-imx@nxp.com>
> > Subject: Re: [PATCH v9 6/8] PCI: imx6: Disable clocks and regulators after link is
> > down
> > 
> > Am Freitag, dem 06.05.2022 um 09:47 +0800 schrieb Richard Zhu:
> > > Since i.MX PCIe doesn't support hot-plug, reduce power consumption as
> > > much as possible by disabling clocks and regulators and returning
> > > error when the link is down.
> > >
> > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > ---
> > >  drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++++++++++----
> > >  1 file changed, 15 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 3ce3993d5797..d122c12193a6 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -845,7 +845,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_out;
> > 
> > This adds back error handling that has been intentionally removed in
> > f81f095e8771 ("PCI: imx6: Allow to probe when dw_pcie_wait_for_link() fails").
> > While I agree that disabling the clocks and regulators is the right thing to do
> > when we don't manage to get a link, we should still allow the driver to probe,
> > so please add a "ret = 0" to this newly added non-fatal error paths.
> > 
> Thanks for your review comments.
> There would be a long latency if the link is down and probe is finished
>  successfully.
> Since the dw_pcie_wait_for_link() would be invoked twice in every driver probe
>  and resume operation later. Each dw_pcie_wait_for_link() would consume about
>  90,000*10 ~ 100,000*10 u-seconds. I'm afraid that such a long latency would
>  bring bad user experience.
> 
> Here are the logs when probe is allowed when PCIe link is down:
> [   55.045954][ T1835] imx6q-pcie 5f000000.pcie: PM: calling imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x8 @ 1835, parent: bus@5f000000
> ...
> [   56.074566][ T1835] imx6q-pcie 5f000000.pcie: Phy link never came up
> [   57.074816][ T1835] imx6q-pcie 5f000000.pcie: Phy link never came up
> ...
> [   57.182300][ T1835] imx6q-pcie 5f000000.pcie: PM: imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x8 returned 0 after 2136334 usecs
> 
> [   57.182347][ T1835] imx6q-pcie 5f010000.pcie: PM: calling imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x8 @ 1835, parent: bus@5f000000
> ...
> [   58.210584][ T1835] imx6q-pcie 5f010000.pcie: Phy link never came up
> [   59.210831][ T1835] imx6q-pcie 5f010000.pcie: Phy link never came up
> ...
> [   59.318313][ T1835] imx6q-pcie 5f010000.pcie: PM: imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x8 returned 0 after 2135949 usecs
> 
> So, I'm prefer that it's better to let the probe failed when link is down. 
> How do you think about that?

I think that recently Bjorn mentioned some concern with this approach,
and I agree with him.
I think that the probe of the PCIe root port should not fail if the link
is down.

What is the reason for such a long wait in dw_pcie_wait_for_link()? Is
this slowing down the resume process as a whole? Why called twice? (I'm
not familiar with that part of the code)

Francesco

RE: [PATCH v9 5/8] PCI: imx6: Refine the regulator usage

[Hongxing Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年6月9日 15:47
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> jingoohan1@gmail.com; festevam@gmail.com;
> francesco.dolcini@toradex.com
> Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v9 5/8] PCI: imx6: Refine the regulator usage
> 
> Am Donnerstag, dem 09.06.2022 um 06:17 +0000 schrieb Hongxing Zhu:
> > > -----Original Message-----
> > > From: Lucas Stach <l.stach@pengutronix.de>
> > > Sent: 2022年6月8日 15:27
> > > To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> > > robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> > > jingoohan1@gmail.com; festevam@gmail.com;
> > > francesco.dolcini@toradex.com
> > > Cc: linux-pci@vger.kernel.org;
> > > linux-arm-kernel@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > > <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v9 5/8] PCI: imx6: Refine the regulator usage
> > >
> > > Am Freitag, dem 06.05.2022 um 09:47 +0800 schrieb Richard Zhu:
> > > > The driver should undo any enables it did itself. The regulator
> > > > disable shouldn't be basing decisions on regulator_is_enabled().
> > > >
> > > > To keep the balance of the regulator usage counter, disable the
> > > > regulator just behind of imx6_pcie_assert_core_reset() in resume
> > > > and
> > > shutdown.
> > > >
> > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.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 7005a7910003..3ce3993d5797 100644
> > > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > > @@ -369,8 +369,6 @@ static int imx6_pcie_attach_pd(struct device
> > > > *dev)
> > > >
> > > >  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:
> > > > @@ -400,14 +398,6 @@ static void
> > > > imx6_pcie_assert_core_reset(struct
> > > imx6_pcie *imx6_pcie)
> > > >  				   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);
> > > > -	}
> > > >  }
> > > >
> > > >  static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie
> > > > *imx6_pcie) @@ -580,7 +570,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", @@
> > > -653,7
> > > > +643,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", @@
> > > -1026,6
> > > > +1016,9 @@ static int imx6_pcie_resume_noirq(struct device *dev)
> > > >  		return 0;
> > > >
> > > >  	imx6_pcie_assert_core_reset(imx6_pcie);
> > > > +	if (imx6_pcie->vpcie)
> > > > +		regulator_disable(imx6_pcie->vpcie);
> > > > +
> > > This one looks misplaced. Surely you want the regulator to be on
> > > when resuming the PCIe subsystem. Isn't this just papering over a
> > > wrong usage count here, because there is no regulator_disable in
> > > imx6_pcie_suspend_noirq, where I would expect this to happen?
> > >
> > Hi Lucas:
> > Thanks for your comments.
> > There was one regulator_disable() operation at the end of the
> >  imx6_pcie_assert_core_reset() function before.
> > When create the 5/8 patch, I follow the same behavior to disable the
> > regulator just behind the imx6_pcie_assert_core_reset() function.
> >
> > Yes, it is. Imx6_pcie_suspend_noirq doesn't have the
> > regulator_disable.
> > The regulaor_enable is contained in imx6_pcie_deassert_core_reset().
> > Both of the regulator_disable and regulator_enabe are invoked once in
> >  imx6_pcie_resume_noirq.
> > So, the regulator is on and has a balanced usage count after resume.
> >
> 
> Yea, my argument is that when we are moving around the regulator handling
> anyways, we should move the regulator_disable into the suspend function. It's
> the right thing to do: we don't want the bus to be powered when the system is
> in suspend and while the use-count is correct, it's confusing to read the resume
> function otherwise.
> 
Thanks for your quick reply.
Understand. I would move the regulator_disable at the end of the suspend
 function if you're fine with it.

Best Regards
Richard Zhu

> Regards,
> Lucas

Re: [PATCH v9 6/8] PCI: imx6: Disable clocks and regulators after link is down

[Lucas Stach]

Am Donnerstag, dem 09.06.2022 um 06:17 +0000 schrieb Hongxing Zhu:
> > -----Original Message-----
> > From: Lucas Stach <l.stach@pengutronix.de>
> > Sent: 2022年6月8日 15:35
> > To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> > robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> > jingoohan1@gmail.com; festevam@gmail.com;
> > francesco.dolcini@toradex.com
> > Cc: linux-pci@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > <linux-imx@nxp.com>
> > Subject: Re: [PATCH v9 6/8] PCI: imx6: Disable clocks and
> > regulators after link is
> > down
> > 
> > Am Freitag, dem 06.05.2022 um 09:47 +0800 schrieb Richard Zhu:
> > > Since i.MX PCIe doesn't support hot-plug, reduce power
> > > consumption as
> > > much as possible by disabling clocks and regulators and returning
> > > error when the link is down.
> > > 
> > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > ---
> > >  drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++++++++++----
> > >  1 file changed, 15 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 3ce3993d5797..d122c12193a6 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -845,7 +845,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_out;
> > 
> > This adds back error handling that has been intentionally removed
> > in
> > f81f095e8771 ("PCI: imx6: Allow to probe when
> > dw_pcie_wait_for_link() fails").
> > While I agree that disabling the clocks and regulators is the right
> > thing to do
> > when we don't manage to get a link, we should still allow the
> > driver to probe,
> > so please add a "ret = 0" to this newly added non-fatal error
> > paths.
> > 
> Thanks for your review comments.
> There would be a long latency if the link is down and probe is
> finished
>  successfully.
> Since the dw_pcie_wait_for_link() would be invoked twice in every
> driver probe
>  and resume operation later. Each dw_pcie_wait_for_link() would
> consume about
>  90,000*10 ~ 100,000*10 u-seconds. I'm afraid that such a long
> latency would
>  bring bad user experience.
> 
> Here are the logs when probe is allowed when PCIe link is down:
> [   55.045954][ T1835] imx6q-pcie 5f000000.pcie: PM: calling
> imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x
> 8 @ 1835, parent: bus@5f000000
> ...
> [   56.074566][ T1835] imx6q-pcie 5f000000.pcie: Phy link never came
> up
> [   57.074816][ T1835] imx6q-pcie 5f000000.pcie: Phy link never came
> up
> ...
> [   57.182300][ T1835] imx6q-pcie 5f000000.pcie: PM:
> imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x
> 8 returned 0 after 2136334 usecs
> 
> [   57.182347][ T1835] imx6q-pcie 5f010000.pcie: PM: calling
> imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x
> 8 @ 1835, parent: bus@5f000000
> ...
> [   58.210584][ T1835] imx6q-pcie 5f010000.pcie: Phy link never came
> up
> [   59.210831][ T1835] imx6q-pcie 5f010000.pcie: Phy link never came
> up
> ...
> [   59.318313][ T1835] imx6q-pcie 5f010000.pcie: PM:
> imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x
> 8 returned 0 after 2135949 usecs
> 
> So, I'm prefer that it's better to let the probe failed when link is
> down. 
> How do you think about that?

It different from the behavior of other platforms, that still show the
root bridge in lspci, even if the link is down. I've seen people
confused by this behavior. Come to think about it: does lspci work when
all the clocks are disabled?

The latency in the probe path is not that relevant, as it is done
asynchronous, so it doesn't stall the boot process. You have a point on
the suspend/resume path however. Maybe we need to remember the link
state in suspend to know if resume should even do anything useful?

Regards,
Lucas

> 
> > > 
> > >  	if (pci->link_gen == 2) {
> > >  		/* Allow Gen2 mode after the link is up. */ @@ -
> > > 876,12 +878,14
> > @@
> > > static int imx6_pcie_start_link(struct dw_pcie *pci)
> > >  			ret =
> > > imx6_pcie_wait_for_speed_change(imx6_pcie);
> > >  			if (ret) {
> > >  				dev_err(dev, "Failed to bring
> > > link up!\n");
> > > -				goto err_reset_phy;
> > > +				goto err_out;
> > >  			}
> > >  		}
> > > 
> > >  		/* 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_out;
> > >  	} else {
> > >  		dev_info(dev, "Link: Gen2 disabled\n");
> > >  	}
> > > @@ -890,11 +894,18 @@ static int imx6_pcie_start_link(struct
> > > dw_pcie
> > *pci)
> > >  	dev_info(dev, "Link up, Gen%i\n", tmp &
> > > PCI_EXP_LNKSTA_CLS);
> > >  	return 0;
> > > 
> > > -err_reset_phy:
> > > +err_out:
> > >  	dev_dbg(dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n",
> > >  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> > >  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> > >  	imx6_pcie_reset_phy(imx6_pcie);
> > > +	imx6_pcie_clk_disable(imx6_pcie);
> > > +	if (imx6_pcie->phy != NULL) {
> > 
> > Please use the more common if (imx6_pcie->phy) here.
> > 
> Okay. Thanks.
> 
> Best Regards
> Richard Zhu
> 
> > Regards,
> > Lucas
> > 
> > > +		phy_power_off(imx6_pcie->phy);
> > > +		phy_exit(imx6_pcie->phy);
> > > +	}
> > > +	if (imx6_pcie->vpcie)
> > > +		regulator_disable(imx6_pcie->vpcie);
> > >  	return ret;
> > >  }
> > > 
> > 
> 

RE: [PATCH v9 6/8] PCI: imx6: Disable clocks and regulators after link is down

[Hongxing Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年6月9日 15:55
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> jingoohan1@gmail.com; festevam@gmail.com;
> francesco.dolcini@toradex.com
> Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v9 6/8] PCI: imx6: Disable clocks and regulators after link is
> down
> 
> Am Donnerstag, dem 09.06.2022 um 06:17 +0000 schrieb Hongxing Zhu:
> > > -----Original Message-----
> > > From: Lucas Stach <l.stach@pengutronix.de>
> > > Sent: 2022年6月8日 15:35
> > > To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> > > robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> > > jingoohan1@gmail.com; festevam@gmail.com;
> > > francesco.dolcini@toradex.com
> > > Cc: linux-pci@vger.kernel.org;
> > > linux-arm-kernel@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > > <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v9 6/8] PCI: imx6: Disable clocks and regulators
> > > after link is down
> > >
> > > Am Freitag, dem 06.05.2022 um 09:47 +0800 schrieb Richard Zhu:
> > > > Since i.MX PCIe doesn't support hot-plug, reduce power consumption
> > > > as much as possible by disabling clocks and regulators and
> > > > returning error when the link is down.
> > > >
> > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > ---
> > > >  drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++++++++++----
> > > >  1 file changed, 15 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > > index 3ce3993d5797..d122c12193a6 100644
> > > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > > @@ -845,7 +845,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_out;
> > >
> > > This adds back error handling that has been intentionally removed in
> > > f81f095e8771 ("PCI: imx6: Allow to probe when
> > > dw_pcie_wait_for_link() fails").
> > > While I agree that disabling the clocks and regulators is the right
> > > thing to do when we don't manage to get a link, we should still
> > > allow the driver to probe, so please add a "ret = 0" to this newly
> > > added non-fatal error paths.
> > >
> > Thanks for your review comments.
> > There would be a long latency if the link is down and probe is
> > finished
> >  successfully.
> > Since the dw_pcie_wait_for_link() would be invoked twice in every
> > driver probe
> >  and resume operation later. Each dw_pcie_wait_for_link() would
> > consume about
> >  90,000*10 ~ 100,000*10 u-seconds. I'm afraid that such a long latency
> > would
> >  bring bad user experience.
> >
> > Here are the logs when probe is allowed when PCIe link is down:
> > [   55.045954][ T1835] imx6q-pcie 5f000000.pcie: PM: calling
> >
> imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x
> > 8 @ 1835, parent: bus@5f000000
> > ...
> > [   56.074566][ T1835] imx6q-pcie 5f000000.pcie: Phy link never came
> > up
> > [   57.074816][ T1835] imx6q-pcie 5f000000.pcie: Phy link never came
> > up
> > ...
> > [   57.182300][ T1835] imx6q-pcie 5f000000.pcie: PM:
> >
> imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x
> > 8 returned 0 after 2136334 usecs
> >
> > [   57.182347][ T1835] imx6q-pcie 5f010000.pcie: PM: calling
> >
> imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x
> > 8 @ 1835, parent: bus@5f000000
> > ...
> > [   58.210584][ T1835] imx6q-pcie 5f010000.pcie: Phy link never came
> > up
> > [   59.210831][ T1835] imx6q-pcie 5f010000.pcie: Phy link never came
> > up
> > ...
> > [   59.318313][ T1835] imx6q-pcie 5f010000.pcie: PM:
> >
> imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x
> > 8 returned 0 after 2135949 usecs
> >
> > So, I'm prefer that it's better to let the probe failed when link is
> > down.
> > How do you think about that?
> 
> It different from the behavior of other platforms, that still show the root bridge
> in lspci, even if the link is down. I've seen people confused by this behavior.
> Come to think about it: does lspci work when all the clocks are disabled?
> 
> The latency in the probe path is not that relevant, as it is done asynchronous,
> so it doesn't stall the boot process. You have a point on the suspend/resume
> path however. Maybe we need to remember the link state in suspend to know
> if resume should even do anything useful?
> 
This sounds good. Thus, one flag is required to specify the link is up or not
before suspend. If the link is down, the imx6_pcie_start_link() shouldn't
invoked at all when resume back.
Then we can avoid the long latency and keep probe finished when link is down.

In this scenario, we can let probe successfully, although the link is down.
And the last patch can be dropped, since the clocks/regulators are always on
in this case.

Thanks for your suggestion and inspiration.

Best Regards
Richard Zhu

> Regards,
> Lucas
> 
> >
> > > >
> > > >  	if (pci->link_gen == 2) {
> > > >  		/* Allow Gen2 mode after the link is up. */ @@ -
> > > > 876,12 +878,14
> > > @@
> > > > static int imx6_pcie_start_link(struct dw_pcie *pci)
> > > >  			ret =
> > > > imx6_pcie_wait_for_speed_change(imx6_pcie);
> > > >  			if (ret) {
> > > >  				dev_err(dev, "Failed to bring link up!\n");
> > > > -				goto err_reset_phy;
> > > > +				goto err_out;
> > > >  			}
> > > >  		}
> > > >
> > > >  		/* 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_out;
> > > >  	} else {
> > > >  		dev_info(dev, "Link: Gen2 disabled\n");
> > > >  	}
> > > > @@ -890,11 +894,18 @@ static int imx6_pcie_start_link(struct
> > > > dw_pcie
> > > *pci)
> > > >  	dev_info(dev, "Link up, Gen%i\n", tmp & PCI_EXP_LNKSTA_CLS);
> > > >  	return 0;
> > > >
> > > > -err_reset_phy:
> > > > +err_out:
> > > >  	dev_dbg(dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n",
> > > >  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> > > >  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> > > >  	imx6_pcie_reset_phy(imx6_pcie);
> > > > +	imx6_pcie_clk_disable(imx6_pcie);
> > > > +	if (imx6_pcie->phy != NULL) {
> > >
> > > Please use the more common if (imx6_pcie->phy) here.
> > >
> > Okay. Thanks.
> >
> > Best Regards
> > Richard Zhu
> >
> > > Regards,
> > > Lucas
> > >
> > > > +		phy_power_off(imx6_pcie->phy);
> > > > +		phy_exit(imx6_pcie->phy);
> > > > +	}
> > > > +	if (imx6_pcie->vpcie)
> > > > +		regulator_disable(imx6_pcie->vpcie);
> > > >  	return ret;
> > > >  }
> > > >
> > >
> >
> 

RE: [PATCH v9 6/8] PCI: imx6: Disable clocks and regulators after link is down

[Hongxing Zhu]

> -----Original Message-----
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> Sent: 2022年6月9日 15:53
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>; bhelgaas@google.com;
> robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> jingoohan1@gmail.com; festevam@gmail.com;
> francesco.dolcini@toradex.com; linux-pci@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v9 6/8] PCI: imx6: Disable clocks and regulators after link is
> down
> 
> On Thu, Jun 09, 2022 at 06:17:46AM +0000, Hongxing Zhu wrote:
> > > -----Original Message-----
> > > From: Lucas Stach <l.stach@pengutronix.de>
> > > Sent: 2022年6月8日 15:35
> > > To: Hongxing Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> > > robh+dt@kernel.org; broonie@kernel.org; lorenzo.pieralisi@arm.com;
> > > jingoohan1@gmail.com; festevam@gmail.com;
> > > francesco.dolcini@toradex.com
> > > Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > > <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v9 6/8] PCI: imx6: Disable clocks and regulators
> > > after link is down
> > >
> > > Am Freitag, dem 06.05.2022 um 09:47 +0800 schrieb Richard Zhu:
> > > > Since i.MX PCIe doesn't support hot-plug, reduce power consumption
> > > > as much as possible by disabling clocks and regulators and
> > > > returning error when the link is down.
> > > >
> > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > ---
> > > >  drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++++++++++----
> > > >  1 file changed, 15 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > > index 3ce3993d5797..d122c12193a6 100644
> > > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > > @@ -845,7 +845,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_out;
> > >
> > > This adds back error handling that has been intentionally removed in
> > > f81f095e8771 ("PCI: imx6: Allow to probe when dw_pcie_wait_for_link()
> fails").
> > > While I agree that disabling the clocks and regulators is the right
> > > thing to do when we don't manage to get a link, we should still
> > > allow the driver to probe, so please add a "ret = 0" to this newly added
> non-fatal error paths.
> > >
> > Thanks for your review comments.
> > There would be a long latency if the link is down and probe is
> > finished  successfully.
> > Since the dw_pcie_wait_for_link() would be invoked twice in every
> > driver probe  and resume operation later. Each dw_pcie_wait_for_link()
> > would consume about
> >  90,000*10 ~ 100,000*10 u-seconds. I'm afraid that such a long latency
> > would  bring bad user experience.
> >
> > Here are the logs when probe is allowed when PCIe link is down:
> > [   55.045954][ T1835] imx6q-pcie 5f000000.pcie: PM: calling
> imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x
> 8 @ 1835, parent: bus@5f000000
> > ...
> > [   56.074566][ T1835] imx6q-pcie 5f000000.pcie: Phy link never came up
> > [   57.074816][ T1835] imx6q-pcie 5f000000.pcie: Phy link never came up
> > ...
> > [   57.182300][ T1835] imx6q-pcie 5f000000.pcie: PM:
> imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x
> 8 returned 0 after 2136334 usecs
> >
> > [   57.182347][ T1835] imx6q-pcie 5f010000.pcie: PM: calling
> imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x
> 8 @ 1835, parent: bus@5f000000
> > ...
> > [   58.210584][ T1835] imx6q-pcie 5f010000.pcie: Phy link never came up
> > [   59.210831][ T1835] imx6q-pcie 5f010000.pcie: Phy link never came up
> > ...
> > [   59.318313][ T1835] imx6q-pcie 5f010000.pcie: PM:
> imx6_pcie_resume_noirq.742dfa074b40dca7ca925f0c49c905ec.cfi_jt+0x0/0x
> 8 returned 0 after 2135949 usecs
> >
> > So, I'm prefer that it's better to let the probe failed when link is down.
> > How do you think about that?
> 
> I think that recently Bjorn mentioned some concern with this approach, and I
> agree with him.
> I think that the probe of the PCIe root port should not fail if the link is down.
> 
> What is the reason for such a long wait in dw_pcie_wait_for_link()? Is this
> slowing down the resume process as a whole? Why called twice? (I'm not
> familiar with that part of the code)
> 
Thanks for your concerns.
To avoid a corner link down issue, iMX PCIe driver force link up GEN1 speed
firstly, then try to link up the highest speed later with one speed exchange.
So, the dw_pcie_wait_for_link() would be invoked twice in iMX PCIe driver.

Lucas and I try to figure out one method to avoid the long latency when resume
back from suspend mode when link is down and probe is succeed.

Best Regards
Richard Zhu

> Francesco

Re: [PATCH v9 7/8] PCI: imx6: Move the phy driver callbacks to the proper places

[Bjorn Helgaas]

On Thu, Jun 09, 2022 at 06:20:16AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > Sent: 2022年6月9日 2:58
> > To: Hongxing Zhu <hongxing.zhu@nxp.com>
> > Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org;
> > broonie@kernel.org; lorenzo.pieralisi@arm.com; jingoohan1@gmail.com;
> > festevam@gmail.com; francesco.dolcini@toradex.com;
> > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > <linux-imx@nxp.com>
> > Subject: Re: [PATCH v9 7/8] PCI: imx6: Move the phy driver callbacks to the
> > proper places
> > 
> > On Fri, May 06, 2022 at 09:47:08AM +0800, Richard Zhu wrote:
> > > To make it more reasonable, move the phy_power_on/phy_init callbacks
> > > to the proper places.
> > > - move the phy_power_on() out of imx6_pcie_clk_enable().
> > > - move the phy_init() out of imx6_pcie_deassert_core_reset().
> > 
> > I'm not sure what "make it more reasonable" is telling me.  In
> > subject line and commit log, please say something more specific
> > than "the proper places."
> > 
> > It's probably more important to say where they are moving *to*
> > than where they're moving *out of*.

> Thanks for your comments.
> In another review loop listed below, Lucas used said that it's not
> good to hide PHY init in imx6_pcie_assert_core_reset() So, I make a
> try to move the phy_init() out of imx6_pcie_assert_core_reset().
> And move phy_power_on() out of imx6_pcie_clk_enable() accordingly.
> https://patchwork.kernel.org/project/linux-pci/patch/1646289275-17813-1-git-send-email-hongxing.zhu@nxp.com/
> Okay, I would specific that they are moving *to* later.
> 
> > > In order to save power consumption, turn off the clocks and
> > > regulators when the imx6_pcie_host_init() return error.
> > 
> > Is the power savings the *reason* for this change?  I can't tell
> > from the commit log.
>
> The error handling of the imx6_pcie_host_init() is not mentioned in
> the subject.  Should I split these changes into two patches?

If they can be split, they probably should be split.

Re: [PATCH v9 5/8] PCI: imx6: Refine the regulator usage

[Bjorn Helgaas]

On Thu, Jun 09, 2022 at 06:19:47AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > Sent: 2022年6月9日 2:55
> > To: Hongxing Zhu <hongxing.zhu@nxp.com>
> > Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org;
> > broonie@kernel.org; lorenzo.pieralisi@arm.com; jingoohan1@gmail.com;
> > festevam@gmail.com; francesco.dolcini@toradex.com;
> > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > <linux-imx@nxp.com>
> > Subject: Re: [PATCH v9 5/8] PCI: imx6: Refine the regulator usage
> > 
> > On Fri, May 06, 2022 at 09:47:06AM +0800, Richard Zhu wrote:
> > > The driver should undo any enables it did itself. The regulator
> > > disable shouldn't be basing decisions on regulator_is_enabled().

The driver should disable things if an error occurs after it has
enabled something, or if it enabled something during probe and we're
now detaching the driver.  That doesn't look like the case here.

> > > To keep the balance of the regulator usage counter, disable the
> > > regulator just behind of imx6_pcie_assert_core_reset() in resume and
> > > shutdown.
> > 
> > In subject, "Refine" doesn't tell me anything about what's happening here.
>
> Thanks for your comments.
> How about the following one?
> PCI: imx6: Do regulator disable without the regulator_is_enabled check

That's too low-level, like describing the C code line by line.
I'm hoping for something about the purpose for the patch so
"git log --oneline" can tell a coherent story.

Apparently this is about disabling the power regulator when the slot
isn't being used, so maybe it could say something about that.

  $ git grep -Ep "regulator_(en|dis)able" drivers/pci/controller/

shows that in other drivers, this being done in
probe/remove/suspend/resume-type paths.  imx6 should be similar.

Bjorn

RE: [PATCH v9 7/8] PCI: imx6: Move the phy driver callbacks to the proper places

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 2022年6月10日 0:26
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org;
> broonie@kernel.org; lorenzo.pieralisi@arm.com; jingoohan1@gmail.com;
> festevam@gmail.com; francesco.dolcini@toradex.com;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v9 7/8] PCI: imx6: Move the phy driver callbacks to the
> proper places
> 
> On Thu, Jun 09, 2022 at 06:20:16AM +0000, Hongxing Zhu wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > Sent: 2022年6月9日 2:58
> > > To: Hongxing Zhu <hongxing.zhu@nxp.com>
> > > Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org;
> > > broonie@kernel.org; lorenzo.pieralisi@arm.com; jingoohan1@gmail.com;
> > > festevam@gmail.com; francesco.dolcini@toradex.com;
> > > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > > <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v9 7/8] PCI: imx6: Move the phy driver callbacks
> > > to the proper places
> > >
> > > On Fri, May 06, 2022 at 09:47:08AM +0800, Richard Zhu wrote:
> > > > To make it more reasonable, move the phy_power_on/phy_init
> > > > callbacks to the proper places.
> > > > - move the phy_power_on() out of imx6_pcie_clk_enable().
> > > > - move the phy_init() out of imx6_pcie_deassert_core_reset().
> > >
> > > I'm not sure what "make it more reasonable" is telling me.  In
> > > subject line and commit log, please say something more specific than
> > > "the proper places."
> > >
> > > It's probably more important to say where they are moving *to* than
> > > where they're moving *out of*.
> 
> > Thanks for your comments.
> > In another review loop listed below, Lucas used said that it's not
> > good to hide PHY init in imx6_pcie_assert_core_reset() So, I make a
> > try to move the phy_init() out of imx6_pcie_assert_core_reset().
> > And move phy_power_on() out of imx6_pcie_clk_enable() accordingly.
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> >
> hwork.kernel.org%2Fproject%2Flinux-pci%2Fpatch%2F1646289275-17813-1-g
> i
> >
> t-send-email-hongxing.zhu%40nxp.com%2F&amp;data=05%7C01%7Chongxing
> .zhu
> > %40nxp.com%7C0094cb503bd64d9d8c0008da4a34bbc8%7C686ea1d3bc2b
> 4c6fa92cd9
> >
> 9c5c301635%7C0%7C0%7C637903887598125451%7CUnknown%7CTWFpbG
> Zsb3d8eyJWIj
> >
> oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C30
> 00%7C
> > %7C%7C&amp;sdata=ntbjAv6s3M0mUI8uCAUX23HiPNBlgvWKlAaw7lAh%2F
> pE%3D&amp;
> > reserved=0 Okay, I would specific that they are moving *to* later.
> >
> > > > In order to save power consumption, turn off the clocks and
> > > > regulators when the imx6_pcie_host_init() return error.
> > >
> > > Is the power savings the *reason* for this change?  I can't tell
> > > from the commit log.
> >
> > The error handling of the imx6_pcie_host_init() is not mentioned in
> > the subject.  Should I split these changes into two patches?
> 
> If they can be split, they probably should be split.
Got that, thanks a lot.

Best Regards
Richard Zhu

RE: [PATCH v9 5/8] PCI: imx6: Refine the regulator usage

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 2022年6月10日 1:20
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org;
> broonie@kernel.org; lorenzo.pieralisi@arm.com; jingoohan1@gmail.com;
> festevam@gmail.com; francesco.dolcini@toradex.com;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v9 5/8] PCI: imx6: Refine the regulator usage
> 
> On Thu, Jun 09, 2022 at 06:19:47AM +0000, Hongxing Zhu wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > Sent: 2022年6月9日 2:55
> > > To: Hongxing Zhu <hongxing.zhu@nxp.com>
> > > Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org;
> > > broonie@kernel.org; lorenzo.pieralisi@arm.com; jingoohan1@gmail.com;
> > > festevam@gmail.com; francesco.dolcini@toradex.com;
> > > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > > <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v9 5/8] PCI: imx6: Refine the regulator usage
> > >
> > > On Fri, May 06, 2022 at 09:47:06AM +0800, Richard Zhu wrote:
> > > > The driver should undo any enables it did itself. The regulator
> > > > disable shouldn't be basing decisions on regulator_is_enabled().
> 
> The driver should disable things if an error occurs after it has enabled
> something, or if it enabled something during probe and we're now detaching
> the driver.  That doesn't look like the case here.
> 
> > > > To keep the balance of the regulator usage counter, disable the
> > > > regulator just behind of imx6_pcie_assert_core_reset() in resume
> > > > and shutdown.
> > >
> > > In subject, "Refine" doesn't tell me anything about what's happening here.
> >
> > Thanks for your comments.
> > How about the following one?
> > PCI: imx6: Do regulator disable without the regulator_is_enabled check
> 
> That's too low-level, like describing the C code line by line.
> I'm hoping for something about the purpose for the patch so "git log --oneline"
> can tell a coherent story.
> 
> Apparently this is about disabling the power regulator when the slot isn't being
> used, so maybe it could say something about that.
> 
>   $ git grep -Ep "regulator_(en|dis)able" drivers/pci/controller/
> 
> shows that in other drivers, this being done in
> probe/remove/suspend/resume-type paths.  imx6 should be similar.
> 
Got that, thanks a lot.

Best Regards
Richard Zhu

> Bjorn