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

[PATCH v8 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 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

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             | 223 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------
drivers/pci/controller/dwc/pcie-designware-host.c |   5 ++-
drivers/pci/controller/dwc/pcie-designware.h      |   1 +
3 files changed, 150 insertions(+), 79 deletions(-)

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

[PATCH v8 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 5fd33e4536af..5582ee1b420a 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -481,38 +481,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);
@@ -543,6 +521,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)) {
@@ -601,13 +624,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 v8 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 5582ee1b420a..3ca2eef39617 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -546,24 +546,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;
 	}
 
@@ -622,7 +622,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) {
@@ -631,6 +631,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)
@@ -882,11 +883,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 v8 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 3ca2eef39617..99fc22d1d55e 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -533,6 +533,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;
@@ -965,30 +989,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 v8 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 99fc22d1d55e..0f1b8c873a4a 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -540,6 +540,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 v8 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 0f1b8c873a4a..160a0bd02098 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)
@@ -584,7 +574,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",
@@ -657,7 +647,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",
@@ -1029,6 +1019,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);
@@ -1262,6 +1255,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 v8 6/8] PCI: dwc: Add dw_pcie_host_ops.host_exit() callback

[Richard Zhu]

When the link never comes up after ->host_init(), some drivers,
especially those that don't support hotplug, want to turn off clocks
and power supplies.

Add a new ->host_exit() callback in dw_pcie_host_ops so these
drivers can clean up if ->host_init() fails.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 5 ++++-
 drivers/pci/controller/dwc/pcie-designware.h      | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index f4755f3a03be..461863bde3c9 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -405,7 +405,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	if (!dw_pcie_link_up(pci) && pci->ops && pci->ops->start_link) {
 		ret = pci->ops->start_link(pci);
 		if (ret)
-			goto err_free_msi;
+			goto err_host_init;
 	}
 
 	/* Ignore errors, the link may come up later */
@@ -417,6 +417,9 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	if (!ret)
 		return 0;
 
+err_host_init:
+	if (pp->ops->host_exit)
+		pp->ops->host_exit(pp);
 err_free_msi:
 	if (pp->has_msi_ctrl)
 		dw_pcie_free_msi(pp);
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 7d6e9b7576be..1153687ea9a6 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -174,6 +174,7 @@ enum dw_pcie_device_mode {
 
 struct dw_pcie_host_ops {
 	int (*host_init)(struct pcie_port *pp);
+	void (*host_exit)(struct pcie_port *pp);
 	int (*msi_host_init)(struct pcie_port *pp);
 };
 
-- 
2.25.1

[PATCH v8 7/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.

Add a new host_exit() callback for i.MX PCIe driver to disable the clocks,
regulators and so on in the error handling after host_init is finished.

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 160a0bd02098..f8b15f0bc9ce 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -849,7 +849,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. */
@@ -880,12 +882,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");
 	}
@@ -894,11 +898,10 @@ 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);
 	return ret;
 }
 
@@ -922,8 +925,29 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
 	return 0;
 }
 
+static void imx6_pcie_host_exit(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);
+
+	imx6_pcie_reset_phy(imx6_pcie);
+	imx6_pcie_clk_disable(imx6_pcie);
+	switch (imx6_pcie->drvdata->variant) {
+	case IMX8MM:
+		if (phy_power_off(imx6_pcie->phy))
+			dev_err(dev, "unable to power off phy\n");
+		break;
+	default:
+		break;
+	}
+	if (imx6_pcie->vpcie)
+		regulator_disable(imx6_pcie->vpcie);
+}
+
 static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
 	.host_init = imx6_pcie_host_init,
+	.host_exit = imx6_pcie_host_exit,
 };
 
 static const struct dw_pcie_ops dw_pcie_ops = {
-- 
2.25.1

[PATCH v8 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 | 47 ++++++++++++++++++---------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index f8b15f0bc9ce..b976a4802db0 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;
@@ -841,10 +845,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);
@@ -931,18 +937,20 @@ static void imx6_pcie_host_exit(struct pcie_port *pp)
 	struct device *dev = pci->dev;
 	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
 
-	imx6_pcie_reset_phy(imx6_pcie);
-	imx6_pcie_clk_disable(imx6_pcie);
-	switch (imx6_pcie->drvdata->variant) {
-	case IMX8MM:
-		if (phy_power_off(imx6_pcie->phy))
-			dev_err(dev, "unable to power off phy\n");
-		break;
-	default:
-		break;
+	if (!imx6_pcie_cmp_mode) {
+		imx6_pcie_reset_phy(imx6_pcie);
+		imx6_pcie_clk_disable(imx6_pcie);
+		switch (imx6_pcie->drvdata->variant) {
+		case IMX8MM:
+			if (phy_power_off(imx6_pcie->phy))
+				dev_err(dev, "unable to power off phy\n");
+			break;
+		default:
+			break;
+		}
+		if (imx6_pcie->vpcie)
+			regulator_disable(imx6_pcie->vpcie);
 	}
-	if (imx6_pcie->vpcie)
-		regulator_disable(imx6_pcie->vpcie);
 }
 
 static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
@@ -1260,8 +1268,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 v8 5/8] PCI: imx6: Refine the regulator usage

[Francesco Dolcini]

On Fri, Feb 25, 2022 at 11:44:24AM +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.

While this patch does not look wrong and improving the existing code, I
do have a couple of concern.

1. It's weird that regulator enable/disable are really non symmetric in
the code, enable is in imx6_pcie_deassert_core_reset(), while disable is
not in imx6_pcie_assert_core_reset() but spread around.

2. We are playing around with vpcie during regulator during
suspend/resume, why? We currently have issue with suspend/resume when a
PCIe switch is there, with the final effect of PCIe being not usable
afterward. I fear like this is related to our issue.

What about vpcie enable in probe, and vpcie disable in shutdown and
that's all? 

Francesco

> 
> 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 0f1b8c873a4a..160a0bd02098 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)
> @@ -584,7 +574,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",
> @@ -657,7 +647,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",
> @@ -1029,6 +1019,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);
> @@ -1262,6 +1255,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
> 

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

[Hongxing Zhu]

> -----Original Message-----
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> Sent: 2022年2月25日 17:43
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; 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 v8 5/8] PCI: imx6: Refine the regulator usage
> 
> On Fri, Feb 25, 2022 at 11:44:24AM +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.
> 
> While this patch does not look wrong and improving the existing code, I do
> have a couple of concern.
> 
> 1. It's weird that regulator enable/disable are really non symmetric in the code,
> enable is in imx6_pcie_deassert_core_reset(), while disable is not in
> imx6_pcie_assert_core_reset() but spread around.
Hi Francesco:
This regulator disable was a part of imx6_pcie_assert_core_reset() before,
 just had been moved out of imx6_pcie_assert_core_reset() in this refine
 commit.

> 
> 2. We are playing around with vpcie during regulator during suspend/resume,
> why? We currently have issue with suspend/resume when a PCIe switch is
> there, with the final effect of PCIe being not usable afterward. I fear like this is
> related to our issue.
>
To my understands, the VPCIE is the abstract of the DIESABLE# signal.
Refer to the Chapter 3.1.12.3 W_DISABLE# Signal of PCI Express M.2 SPEC.
These signals are used to disable the Adapter's radio operation to meet public
Safety regulations or when otherwise desired.
It's reasonable to assert/de-assert W_DISABLE# signal during suspend/resume.

> What about vpcie enable in probe, and vpcie disable in shutdown and that's
> all?
The W_DISABLE# Signal is used to control the radios on the Adapters.
I think you can do that, if the radios are always on in your system.

Best Regards
Richard
> 
> Francesco
> 
> >
> > 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 0f1b8c873a4a..160a0bd02098 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) @@ -584,7 +574,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", @@
> -657,7
> > +647,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", @@
> -1029,6
> > +1019,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);
> > @@ -1262,6 +1255,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
> >

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

[Francesco Dolcini]

On Mon, Feb 28, 2022 at 03:43:03AM +0000, Hongxing Zhu wrote:
> > 2. We are playing around with vpcie during regulator during suspend/resume,
> > why? We currently have issue with suspend/resume when a PCIe switch is
> > there, with the final effect of PCIe being not usable afterward. I fear like this is
> > related to our issue.
> >
> To my understands, the VPCIE is the abstract of the DIESABLE# signal.
> Refer to the Chapter 3.1.12.3 W_DISABLE# Signal of PCI Express M.2 SPEC.
> These signals are used to disable the Adapter's radio operation to meet public
> Safety regulations or when otherwise desired.
> It's reasonable to assert/de-assert W_DISABLE# signal during suspend/resume.
> 
> > What about vpcie enable in probe, and vpcie disable in shutdown and that's
> > all?
> The W_DISABLE# Signal is used to control the radios on the Adapters.
> I think you can do that, if the radios are always on in your system.

According to the binding documentation (imx6q-pcie.yaml)

  vpcie-supply:
    description: Should specify the regulator in charge of PCIe port power.
      The regulator will be enabled when initializing the PCIe host and
      disabled either as part of the init process or when shutting down
      the host (optional required).

This is how it's documented to be used, if this is used according to the
documentation (and this is happening) what you are already doing (not
changed in this patch, you are right) is not correct.

Abusing this for the W_DISABLE# disable is new to me and does not look
legit.

Francesco

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

[Hongxing Zhu]

> -----Original Message-----
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> Sent: 2022年3月4日 15:46
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: Francesco Dolcini <francesco.dolcini@toradex.com>;
> l.stach@pengutronix.de; bhelgaas@google.com; broonie@kernel.org;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.com; festevam@gmail.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 v8 5/8] PCI: imx6: Refine the regulator usage
> 
> On Mon, Feb 28, 2022 at 03:43:03AM +0000, Hongxing Zhu wrote:
> > > 2. We are playing around with vpcie during regulator during
> > > suspend/resume, why? We currently have issue with suspend/resume
> > > when a PCIe switch is there, with the final effect of PCIe being not
> > > usable afterward. I fear like this is related to our issue.
> > >
> > To my understands, the VPCIE is the abstract of the DIESABLE# signal.
> > Refer to the Chapter 3.1.12.3 W_DISABLE# Signal of PCI Express M.2 SPEC.
> > These signals are used to disable the Adapter's radio operation to
> > meet public Safety regulations or when otherwise desired.
> > It's reasonable to assert/de-assert W_DISABLE# signal during
> suspend/resume.
> >
> > > What about vpcie enable in probe, and vpcie disable in shutdown and
> > > that's all?
> > The W_DISABLE# Signal is used to control the radios on the Adapters.
> > I think you can do that, if the radios are always on in your system.
> 
> According to the binding documentation (imx6q-pcie.yaml)
> 
>   vpcie-supply:
>     description: Should specify the regulator in charge of PCIe port power.
>       The regulator will be enabled when initializing the PCIe host and
>       disabled either as part of the init process or when shutting down
>       the host (optional required).
> 
> This is how it's documented to be used, if this is used according to the
> documentation (and this is happening) what you are already doing (not
> changed in this patch, you are right) is not correct.
> 
> Abusing this for the W_DISABLE# disable is new to me and does not look legit.
The vpcie-supply is one gpio regulator actually, used to control remote Endpoint
 device, through the DISABLE# signal (defined in Mini CEM/M.2 SPEC) of the port.

Best Regards
Richard Zhu

> 
> Francesco

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

[Hongxing Zhu]

Hi Lucas:
Since this v8 series patches are pending for a while, and no review comments anymore.
Can you help to take look at the other patches of this set?

Best Regards
Richard Zhu

> -----Original Message-----
> From: Richard Zhu <hongxing.zhu@nxp.com>
> Sent: 2022年2月25日 11:44
> To: l.stach@pengutronix.de; bhelgaas@google.com; broonie@kernel.org;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.com; festevam@gmail.com;
> francesco.dolcini@toradex.com
> Cc: Hongxing Zhu <hongxing.zhu@nxp.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: [PATCH v8 0/8] PCI: imx6: refine codes and add compliance tests
> mode support
> 
> This series patches refine pci-imx6 driver and do the following 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
> 
> 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchw
> ork.kernel.org%2Fproject%2Flinux-pci%2Fpatch%2F1641368602-20401-6-git-s
> end-email-hongxing.zhu%40nxp.com%2F&amp;data=04%7C01%7Chongxing.z
> hu%40nxp.com%7C100772798bed44ec362f08d9f8124d9c%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C0%7C637813579753331370%7CUnknown%
> 7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwi
> LCJXVCI6Mn0%3D%7C3000&amp;sdata=m4XhLgO0e7H1MzS1KPskWhK1Lc1g
> uUqzYpudrL03pCQ%3D&amp;reserved=0
>   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             | 223
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> -------------------------------
> drivers/pci/controller/dwc/pcie-designware-host.c |   5 ++-
> drivers/pci/controller/dwc/pcie-designware.h      |   1 +
> 3 files changed, 150 insertions(+), 79 deletions(-)
> 
> [PATCH v8 1/8] PCI: imx6: Encapsulate the clock enable into one [PATCH v8 2/8]
> PCI: imx6: Add the error propagation from host_init [PATCH v8 3/8] PCI: imx6:
> Move imx6_pcie_clk_disable() earlier [PATCH v8 4/8] PCI: imx6: Disable
> iMX6QDL PCIe REF clock when disable [PATCH v8 5/8] PCI: imx6: Refine the
> regulator usage [PATCH v8 6/8] PCI: dwc: Add dw_pcie_host_ops.host_exit()
> callback [PATCH v8 7/8] PCI: imx6: Disable clocks and regulators after link is
> [PATCH v8 8/8] PCI: imx6: Add compliance tests mode support

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

[Rob Herring]

On Fri, Feb 25, 2022 at 11:44:27AM +0800, Richard Zhu wrote:
> 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.

How is this specific to i.MX? If there's a need for compliance test 
mode, then shouldn't it be common? However, while your usecase is 
compliance testing, there doesn't seem to be any need for the kernel to 
know what the usecase is.

In the same series you've argued both ways. You need the driver to probe 
successfully with no link and you need it to shutdown on no link.

> 
> 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 | 47 ++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 16 deletions(-)

Re: [PATCH v8 6/8] PCI: dwc: Add dw_pcie_host_ops.host_exit() callback

[Rob Herring]

On Fri, Feb 25, 2022 at 11:44:25AM +0800, Richard Zhu wrote:
> When the link never comes up after ->host_init(), some drivers,
> especially those that don't support hotplug, want to turn off clocks
> and power supplies.

Isn't supporting hotplug or not a board level decision? And hotplug 
doesn't have to mean physical plug/unplug. For example, you could have a 
soldered down PCIe device which needs regulators, resets, clocks, etc. 
for that device to be initialized before the link comes up. If that 
device is handled by a module loaded some time later, then the link may 
be down when you probe.

I think the way this all needs to work is with runtime PM. If that's all 
in place, then either you shutdown clocks/power on timeout or via sysfs 
suspend. If there's a child device, then that should prevent suspending.

> Add a new ->host_exit() callback in dw_pcie_host_ops so these
> drivers can clean up if ->host_init() fails.

I'm not really a fan of adding more ops nor the ops which aren't too 
specific about what they do. 'init' and 'exit' can be anything. I'd 
rather see more specific ops with the DWC core driver in charge of 
sequence of operations and the state. 

Rob

RE: [PATCH v8 6/8] PCI: dwc: Add dw_pcie_host_ops.host_exit() callback

[Hongxing Zhu]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: 2022年4月2日 4:44
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; 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 v8 6/8] PCI: dwc: Add dw_pcie_host_ops.host_exit()
> callback
> 
> On Fri, Feb 25, 2022 at 11:44:25AM +0800, Richard Zhu wrote:
> > When the link never comes up after ->host_init(), some drivers,
> > especially those that don't support hotplug, want to turn off clocks
> > and power supplies.
> 
> Isn't supporting hotplug or not a board level decision? And hotplug doesn't
> have to mean physical plug/unplug. For example, you could have a soldered
> down PCIe device which needs regulators, resets, clocks, etc.
> for that device to be initialized before the link comes up. If that device is
> handled by a module loaded some time later, then the link may be down when
> you probe.
> 
> I think the way this all needs to work is with runtime PM. If that's all in place,
> then either you shutdown clocks/power on timeout or via sysfs suspend. If
> there's a child device, then that should prevent suspending.
Hi Rob:
Thanks a lot for your review comments.
Understand what you mean.
i.MX PCIe doesn't support hot-plug from chip design view.

The ops.host_exit() callback is invoked only when the iMX PCIe driver hooked
 callback ops->start_link return an error.
For the platforms, that support the hot-plug feature, they can just return one
zero from their own ops->start_link.
In the current situation, i.MX PCIe does just return one zero when probe failed.
See the discussion and commit issued by Fabio below.
https://patchwork.kernel.org/project/linux-pci/patch/1641368602-20401-6-git-send-email-hongxing.zhu@nxp.com/
https://patchwork.ozlabs.org/project/linux-pci/patch/20220106103645.2790803-1-festevam@gmail.com/

> 
> > Add a new ->host_exit() callback in dw_pcie_host_ops so these drivers
> > can clean up if ->host_init() fails.
> 
> I'm not really a fan of adding more ops nor the ops which aren't too specific
> about what they do. 'init' and 'exit' can be anything. I'd rather see more
> specific ops with the DWC core driver in charge of sequence of operations and
> the state.
Understand. 
i.MX PCIe can't handle the error exit properly in this case by itself. So I
 add one more ops.host_exit() in this series.

Best Regards
Richard Zhu
> 
> Rob

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

[Hongxing Zhu]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: 2022年4月2日 4:27
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; 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 v8 8/8] PCI: imx6: Add compliance tests mode support
> 
> On Fri, Feb 25, 2022 at 11:44:27AM +0800, Richard Zhu wrote:
> > 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.
> 
> How is this specific to i.MX? If there's a need for compliance test mode, then
> shouldn't it be common? However, while your usecase is compliance testing,
> there doesn't seem to be any need for the kernel to know what the usecase is.
> 
> In the same series you've argued both ways. You need the driver to probe
> successfully with no link and you need it to shutdown on no link.
> 
Hi Rob:
Thank a lot for your review comments.
Based on this series, to save power consumption, i.MX PCIe would turn off the
 power/clocks when probe failed. Thus, it's specific to i.MX to add one more
 option to keep power/clocks on when compliance tests mode is required.
The compliance test mode can be enabled dynamically through the
 module_para setting.

Best Regards
Richard Zhu

> >
> > 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 | 47
> > ++++++++++++++++++---------
> >  1 file changed, 31 insertions(+), 16 deletions(-)

Re: [PATCH v8 6/8] PCI: dwc: Add dw_pcie_host_ops.host_exit() callback

[Rob Herring]

On Sat, Apr 02, 2022 at 03:03:00AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Rob Herring <robh@kernel.org>
> > Sent: 2022年4月2日 4:44
> > To: Hongxing Zhu <hongxing.zhu@nxp.com>
> > Cc: l.stach@pengutronix.de; bhelgaas@google.com; 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 v8 6/8] PCI: dwc: Add dw_pcie_host_ops.host_exit()
> > callback
> > 
> > On Fri, Feb 25, 2022 at 11:44:25AM +0800, Richard Zhu wrote:
> > > When the link never comes up after ->host_init(), some drivers,
> > > especially those that don't support hotplug, want to turn off clocks
> > > and power supplies.
> > 
> > Isn't supporting hotplug or not a board level decision? And hotplug doesn't
> > have to mean physical plug/unplug. For example, you could have a soldered
> > down PCIe device which needs regulators, resets, clocks, etc.
> > for that device to be initialized before the link comes up. If that device is
> > handled by a module loaded some time later, then the link may be down when
> > you probe.
> > 
> > I think the way this all needs to work is with runtime PM. If that's all in place,
> > then either you shutdown clocks/power on timeout or via sysfs suspend. If
> > there's a child device, then that should prevent suspending.
> Hi Rob:
> Thanks a lot for your review comments.
> Understand what you mean.
> i.MX PCIe doesn't support hot-plug from chip design view.

The scenario I described is not hotplug.


> The ops.host_exit() callback is invoked only when the iMX PCIe driver hooked
>  callback ops->start_link return an error.
> For the platforms, that support the hot-plug feature, they can just return one
> zero from their own ops->start_link.

You cannot have a per board start_link().

> In the current situation, i.MX PCIe does just return one zero when probe failed.
> See the discussion and commit issued by Fabio below.
> https://patchwork.kernel.org/project/linux-pci/patch/1641368602-20401-6-git-send-email-hongxing.zhu@nxp.com/
> https://patchwork.ozlabs.org/project/linux-pci/patch/20220106103645.2790803-1-festevam@gmail.com/

That's a stable fix and different discussion.

> > > Add a new ->host_exit() callback in dw_pcie_host_ops so these drivers
> > > can clean up if ->host_init() fails.
> > 
> > I'm not really a fan of adding more ops nor the ops which aren't too specific
> > about what they do. 'init' and 'exit' can be anything. I'd rather see more
> > specific ops with the DWC core driver in charge of sequence of operations and
> > the state.
> Understand. 

You don't seem to...

> i.MX PCIe can't handle the error exit properly in this case by itself. So I
>  add one more ops.host_exit() in this series.
> 
> Best Regards
> Richard Zhu
> > 
> > Rob

RE: [PATCH v8 6/8] PCI: dwc: Add dw_pcie_host_ops.host_exit() callback

[Hongxing Zhu]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: 2022年4月4日 22:40
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; 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 v8 6/8] PCI: dwc: Add dw_pcie_host_ops.host_exit()
> callback
> 
> On Sat, Apr 02, 2022 at 03:03:00AM +0000, Hongxing Zhu wrote:
> > > -----Original Message-----
> > > From: Rob Herring <robh@kernel.org>
> > > Sent: 2022年4月2日 4:44
> > > To: Hongxing Zhu <hongxing.zhu@nxp.com>
> > > Cc: l.stach@pengutronix.de; bhelgaas@google.com; 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 v8 6/8] PCI: dwc: Add
> > > dw_pcie_host_ops.host_exit() callback
> > >
> > > On Fri, Feb 25, 2022 at 11:44:25AM +0800, Richard Zhu wrote:
> > > > When the link never comes up after ->host_init(), some drivers,
> > > > especially those that don't support hotplug, want to turn off
> > > > clocks and power supplies.
> > >
> > > Isn't supporting hotplug or not a board level decision? And hotplug
> > > doesn't have to mean physical plug/unplug. For example, you could
> > > have a soldered down PCIe device which needs regulators, resets, clocks,
> etc.
> > > for that device to be initialized before the link comes up. If that
> > > device is handled by a module loaded some time later, then the link
> > > may be down when you probe.
> > >
> > > I think the way this all needs to work is with runtime PM. If that's
> > > all in place, then either you shutdown clocks/power on timeout or
> > > via sysfs suspend. If there's a child device, then that should prevent
> suspending.
> > Hi Rob:
> > Thanks a lot for your review comments.
> > Understand what you mean.
> > i.MX PCIe doesn't support hot-plug from chip design view.
> 
> The scenario I described is not hotplug.
Hi Rob:
Sorry, I didn't get what you means before.
In the hotplug is not supported scenario.
Refer to my understand, both pci_host_probe->pci_scan_root_bus_bridge and
LTSSM training enable are one shot callback during i.MX PCIe probe procedure.
How the example you described in the previous emails can work well?
Please correct me if I'm wrong.

Best Regards
Richard Zhu

> 
> 
> > The ops.host_exit() callback is invoked only when the iMX PCIe driver
> > hooked  callback ops->start_link return an error.
> > For the platforms, that support the hot-plug feature, they can just
> > return one zero from their own ops->start_link.
> 
> You cannot have a per board start_link().
> 
> > In the current situation, i.MX PCIe does just return one zero when probe
> failed.
> > See the discussion and commit issued by Fabio below.
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> >
> hwork.kernel.org%2Fproject%2Flinux-pci%2Fpatch%2F1641368602-20401-6-g
> i
> >
> t-send-email-hongxing.zhu%40nxp.com%2F&amp;data=04%7C01%7Chongxing
> .zhu
> > %40nxp.com%7Cb23fff6b2c264410da0a08da1648f105%7C686ea1d3bc2b4
> c6fa92cd9
> >
> 9c5c301635%7C0%7C1%7C637846799780646784%7CUnknown%7CTWFpbG
> Zsb3d8eyJWIj
> >
> oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C30
> 00&am
> >
> p;sdata=fE9UxCq4S5K%2BUTPWfFR5h5r56%2B26g7anlPKvklo9sKw%3D&amp;
> reserve
> > d=0
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> >
> hwork.ozlabs.org%2Fproject%2Flinux-pci%2Fpatch%2F20220106103645.2790
> 80
> >
> 3-1-festevam%40gmail.com%2F&amp;data=04%7C01%7Chongxing.zhu%40nx
> p.com%
> >
> 7Cb23fff6b2c264410da0a08da1648f105%7C686ea1d3bc2b4c6fa92cd99c5c3
> 01635%
> >
> 7C0%7C1%7C637846799780646784%7CUnknown%7CTWFpbGZsb3d8eyJWIj
> oiMC4wLjAwM
> >
> DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata
> =OG4
> >
> 5XQBOdqkH6B9KYmqU83mNs1wuHiOCb%2B3oL9ad6SQ%3D&amp;reserved=
> 0
> 
> That's a stable fix and different discussion.
> 
> > > > Add a new ->host_exit() callback in dw_pcie_host_ops so these
> > > > drivers can clean up if ->host_init() fails.
> > >
> > > I'm not really a fan of adding more ops nor the ops which aren't too
> > > specific about what they do. 'init' and 'exit' can be anything. I'd
> > > rather see more specific ops with the DWC core driver in charge of
> > > sequence of operations and the state.
> > Understand.
> 
> You don't seem to...
> 
> > i.MX PCIe can't handle the error exit properly in this case by itself.
> > So I  add one more ops.host_exit() in this series.
> >
> > Best Regards
> > Richard Zhu
> > >
> > > Rob