[RESEND v2 0/5] PCI: imx6: refine codes and add compliance tests mode support

[RESEND v2 0/5] 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
- Balance the usage of the regulator and clocks when link never came up
- Add the compliance tests mode support

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 | 166 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------
1 file changed, 105 insertions(+), 61 deletions(-)

[RESEND v2 1/5] PCI: imx6: Encapsulate the clock enable into one
[RESEND v2 2/5] PCI: imx6: Add the error propagation from host_init
[RESEND v2 3/5] PCI: imx6: Fix the regulator dump when link never
[RESEND v2 4/5] PCI: imx6: Fix the clock reference handling unbalance
[RESEND v2 5/5] PCI: imx6: Add the compliance tests mode support

[RESEND v2 1/5] 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>
---
 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 26f49f797b0f..1fa1dba6da81 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -470,38 +470,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);
@@ -524,6 +502,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)) {
@@ -578,13 +601,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

[RESEND v2 2/5] 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>
---
 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 1fa1dba6da81..3372775834a2 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -527,24 +527,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;
 	}
 
@@ -599,7 +599,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) {
@@ -608,6 +608,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)
@@ -858,11 +859,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

[RESEND v2 3/5] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

When PCIe PHY link never came up and vpcie regulator is present, there
would be following dump when try to put the regulator.
Disable this regulator to fix this dump when link never came up.

  imx6q-pcie 33800000.pcie: Phy link never came up
  imx6q-pcie: probe of 33800000.pcie failed with error -110
  ------------[ cut here ]------------
  WARNING: CPU: 3 PID: 119 at drivers/regulator/core.c:2256 _regulator_put.part.0+0x14c/0x158
  Modules linked in:
  CPU: 3 PID: 119 Comm: kworker/u8:2 Not tainted 5.13.0-rc7-next-20210625-94710-ge4e92b2588a3 #10
  Hardware name: FSL i.MX8MM EVK board (DT)
  Workqueue: events_unbound async_run_entry_fn
  pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
  pc : _regulator_put.part.0+0x14c/0x158
  lr : regulator_put+0x34/0x48
  sp : ffff8000122ebb30
  x29: ffff8000122ebb30 x28: ffff800011be7000 x27: 0000000000000000
  x26: 0000000000000000 x25: 0000000000000000 x24: ffff00000025f2bc
  x23: ffff00000025f2c0 x22: ffff00000025f010 x21: ffff8000122ebc18
  x20: ffff800011e3fa60 x19: ffff00000375fd80 x18: 0000000000000010
  x17: 000000040044ffff x16: 00400032b5503510 x15: 0000000000000108
  x14: ffff0000003cc938 x13: 00000000ffffffea x12: 0000000000000000
  x11: 0000000000000000 x10: ffff80001076ba88 x9 : ffff80001076a540
  x8 : ffff00000025f2c0 x7 : ffff0000001f4450 x6 : ffff000000176cd8
  x5 : ffff000003857880 x4 : 0000000000000000 x3 : ffff800011e3fe30
  x2 : ffff0000003cc4c0 x1 : 0000000000000000 x0 : 0000000000000001
  Call trace:
   _regulator_put.part.0+0x14c/0x158
   regulator_put+0x34/0x48
   devm_regulator_release+0x10/0x18
   release_nodes+0x38/0x60
   devres_release_all+0x88/0xd0
   really_probe+0xd0/0x2e8
   __driver_probe_device+0x74/0xd8
   driver_probe_device+0x7c/0x108
   __device_attach_driver+0x8c/0xd0
   bus_for_each_drv+0x74/0xc0
   __device_attach_async_helper+0xb4/0xd8
   async_run_entry_fn+0x30/0x100
   process_one_work+0x19c/0x320
   worker_thread+0x48/0x418
   kthread+0x14c/0x158
   ret_from_fork+0x10/0x18
  ---[ end trace 3664ca4a50ce849b ]---

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 3372775834a2..cc837f8bf6d4 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -853,6 +853,8 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
 		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
 	imx6_pcie_reset_phy(imx6_pcie);
+	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
+		regulator_disable(imx6_pcie->vpcie);
 	return ret;
 }
 
-- 
2.25.1

[RESEND v2 4/5] PCI: imx6: Fix the clock reference handling unbalance when link never came up

[Richard Zhu]

When link never came up, driver probe would be failed with error -110.
To keep usage counter balance of the clocks, disable the previous
enabled clocks when link is down.
Move definitions of the imx6_pcie_clk_disable() function to the proper
place. Because it wouldn't be used in imx6_pcie_suspend_noirq() only.

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index cc837f8bf6d4..d6a5d99ffa52 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -514,6 +514,29 @@ 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:
+		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;
@@ -853,6 +876,7 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
 		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
 	imx6_pcie_reset_phy(imx6_pcie);
+	imx6_pcie_clk_disable(imx6_pcie);
 	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
 		regulator_disable(imx6_pcie->vpcie);
 	return ret;
@@ -941,29 +965,6 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
 	usleep_range(1000, 10000);
 }
 
-static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
-{
-	clk_disable_unprepare(imx6_pcie->pcie);
-	clk_disable_unprepare(imx6_pcie->pcie_phy);
-	clk_disable_unprepare(imx6_pcie->pcie_bus);
-
-	switch (imx6_pcie->drvdata->variant) {
-	case IMX6SX:
-		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
-		break;
-	case IMX7D:
-		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
-				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
-				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
-		break;
-	case IMX8MQ:
-		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

[RESEND v2 5/5] PCI: imx6: Add the compliance tests mode support

[Richard Zhu]

Refer to the system board signal Quality of PCIe archiecture PHY test
specification. Signal quality tests(for example: jitters,  differential
eye opening and so on ) can be executed with devices in the
polling.compliance state.

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

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 | 32 ++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index d6a5d99ffa52..e861a516d517 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -143,6 +143,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;
@@ -812,10 +816,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);
@@ -876,9 +882,12 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
 		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
 	imx6_pcie_reset_phy(imx6_pcie);
-	imx6_pcie_clk_disable(imx6_pcie);
-	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
-		regulator_disable(imx6_pcie->vpcie);
+	if (!imx6_pcie_cmp_mode) {
+		imx6_pcie_clk_disable(imx6_pcie);
+		if (imx6_pcie->vpcie
+		    && regulator_is_enabled(imx6_pcie->vpcie) > 0)
+			regulator_disable(imx6_pcie->vpcie);
+	}
 	return ret;
 }
 
@@ -1183,8 +1192,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: [RESEND v2 1/5] PCI: imx6: Encapsulate the clock enable into one standalone function

[Lucas Stach]

Am Freitag, dem 15.10.2021 um 14:05 +0800 schrieb 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 26f49f797b0f..1fa1dba6da81 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -470,38 +470,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);
> @@ -524,6 +502,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)) {
> @@ -578,13 +601,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)

Re: [RESEND v2 2/5] PCI: imx6: Add the error propagation from host_init

[Lucas Stach]

Am Freitag, dem 15.10.2021 um 14:05 +0800 schrieb 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 1fa1dba6da81..3372775834a2 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -527,24 +527,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;
>  	}
>  
> @@ -599,7 +599,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) {
> @@ -608,6 +608,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)
> @@ -858,11 +859,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;

Re: [RESEND v2 3/5] PCI: imx6: Fix the regulator dump when link never came up

[Lucas Stach]

Am Freitag, dem 15.10.2021 um 14:05 +0800 schrieb Richard Zhu:
> When PCIe PHY link never came up and vpcie regulator is present, there
> would be following dump when try to put the regulator.
> Disable this regulator to fix this dump when link never came up.
> 
>   imx6q-pcie 33800000.pcie: Phy link never came up
>   imx6q-pcie: probe of 33800000.pcie failed with error -110
>   ------------[ cut here ]------------
>   WARNING: CPU: 3 PID: 119 at drivers/regulator/core.c:2256 _regulator_put.part.0+0x14c/0x158
>   Modules linked in:
>   CPU: 3 PID: 119 Comm: kworker/u8:2 Not tainted 5.13.0-rc7-next-20210625-94710-ge4e92b2588a3 #10
>   Hardware name: FSL i.MX8MM EVK board (DT)
>   Workqueue: events_unbound async_run_entry_fn
>   pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
>   pc : _regulator_put.part.0+0x14c/0x158
>   lr : regulator_put+0x34/0x48
>   sp : ffff8000122ebb30
>   x29: ffff8000122ebb30 x28: ffff800011be7000 x27: 0000000000000000
>   x26: 0000000000000000 x25: 0000000000000000 x24: ffff00000025f2bc
>   x23: ffff00000025f2c0 x22: ffff00000025f010 x21: ffff8000122ebc18
>   x20: ffff800011e3fa60 x19: ffff00000375fd80 x18: 0000000000000010
>   x17: 000000040044ffff x16: 00400032b5503510 x15: 0000000000000108
>   x14: ffff0000003cc938 x13: 00000000ffffffea x12: 0000000000000000
>   x11: 0000000000000000 x10: ffff80001076ba88 x9 : ffff80001076a540
>   x8 : ffff00000025f2c0 x7 : ffff0000001f4450 x6 : ffff000000176cd8
>   x5 : ffff000003857880 x4 : 0000000000000000 x3 : ffff800011e3fe30
>   x2 : ffff0000003cc4c0 x1 : 0000000000000000 x0 : 0000000000000001
>   Call trace:
>    _regulator_put.part.0+0x14c/0x158
>    regulator_put+0x34/0x48
>    devm_regulator_release+0x10/0x18
>    release_nodes+0x38/0x60
>    devres_release_all+0x88/0xd0
>    really_probe+0xd0/0x2e8
>    __driver_probe_device+0x74/0xd8
>    driver_probe_device+0x7c/0x108
>    __device_attach_driver+0x8c/0xd0
>    bus_for_each_drv+0x74/0xc0
>    __device_attach_async_helper+0xb4/0xd8
>    async_run_entry_fn+0x30/0x100
>    process_one_work+0x19c/0x320
>    worker_thread+0x48/0x418
>    kthread+0x14c/0x158
>    ret_from_fork+0x10/0x18
>   ---[ end trace 3664ca4a50ce849b ]---
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 3372775834a2..cc837f8bf6d4 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -853,6 +853,8 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
>  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
>  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
>  	imx6_pcie_reset_phy(imx6_pcie);
> +	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> +		regulator_disable(imx6_pcie->vpcie);

This doesn't seem like the right place to add this. I guess it would be
better to have proper error handling after dw_pcie_host_init to roll
back things like the clock and regulator enable.

Regards,
Lucas

>  	return ret;
>  }
>  

Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling unbalance when link never came up

[Lucas Stach]

Am Freitag, dem 15.10.2021 um 14:05 +0800 schrieb Richard Zhu:
> When link never came up, driver probe would be failed with error -110.
> To keep usage counter balance of the clocks, disable the previous
> enabled clocks when link is down.
> Move definitions of the imx6_pcie_clk_disable() function to the proper
> place. Because it wouldn't be used in imx6_pcie_suspend_noirq() only.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 47 ++++++++++++++-------------
>  1 file changed, 24 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index cc837f8bf6d4..d6a5d99ffa52 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -514,6 +514,29 @@ 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:
> +		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;
> @@ -853,6 +876,7 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
>  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
>  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
>  	imx6_pcie_reset_phy(imx6_pcie);
> +	imx6_pcie_clk_disable(imx6_pcie);

Same comment as with the previous patch. We should not cram in more
error handling in the imx6_pcie_start_link function, but rather move
out all the error handling to be after dw_pcie_host_init. Even the
already existing phy reset here seems misplaced and should be moved
out.

Regards,
Lucas

>  	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
>  		regulator_disable(imx6_pcie->vpcie);
>  	return ret;
> @@ -941,29 +965,6 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
>  	usleep_range(1000, 10000);
>  }
>  
> -static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
> -{
> -	clk_disable_unprepare(imx6_pcie->pcie);
> -	clk_disable_unprepare(imx6_pcie->pcie_phy);
> -	clk_disable_unprepare(imx6_pcie->pcie_bus);
> -
> -	switch (imx6_pcie->drvdata->variant) {
> -	case IMX6SX:
> -		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> -		break;
> -	case IMX7D:
> -		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> -				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> -				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> -		break;
> -	case IMX8MQ:
> -		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: [RESEND v2 5/5] PCI: imx6: Add the compliance tests mode support

[Lucas Stach]

Am Freitag, dem 15.10.2021 um 14:05 +0800 schrieb Richard Zhu:
> Refer to the system board signal Quality of PCIe archiecture PHY test
> specification. Signal quality tests(for example: jitters,  differential
> eye opening and so on ) can be executed with devices in the
> polling.compliance state.
> 
> To let the device support polling.compliance stat, the clocks and powers
> shouldn't be turned off when the probe of device driver is failed.
> 
> 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 | 32 ++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index d6a5d99ffa52..e861a516d517 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -143,6 +143,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;
> @@ -812,10 +816,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);
> @@ -876,9 +882,12 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
>  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
>  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
>  	imx6_pcie_reset_phy(imx6_pcie);

Is it correct to reset the PHY here when in compliance test mode?

> -	imx6_pcie_clk_disable(imx6_pcie);
> -	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> -		regulator_disable(imx6_pcie->vpcie);
> +	if (!imx6_pcie_cmp_mode) {
> +		imx6_pcie_clk_disable(imx6_pcie);
> +		if (imx6_pcie->vpcie
> +		    && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> +			regulator_disable(imx6_pcie->vpcie);
> +	}
>  	return ret;
>  }
>  
> @@ -1183,8 +1192,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);

Re: [RESEND v2 3/5] PCI: imx6: Fix the regulator dump when link never came up

[Fabio Estevam]

Hi Richard,

On Fri, Oct 15, 2021 at 3:32 AM Richard Zhu <hongxing.zhu@nxp.com> wrote:
>
> When PCIe PHY link never came up and vpcie regulator is present, there
> would be following dump when try to put the regulator.
> Disable this regulator to fix this dump when link never came up.
>
>   imx6q-pcie 33800000.pcie: Phy link never came up
>   imx6q-pcie: probe of 33800000.pcie failed with error -110
>   ------------[ cut here ]------------
>   WARNING: CPU: 3 PID: 119 at drivers/regulator/core.c:2256 _regulator_put.part.0+0x14c/0x158
>   Modules linked in:
>   CPU: 3 PID: 119 Comm: kworker/u8:2 Not tainted 5.13.0-rc7-next-20210625-94710-ge4e92b2588a3 #10
>   Hardware name: FSL i.MX8MM EVK board (DT)
>   Workqueue: events_unbound async_run_entry_fn
>   pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
>   pc : _regulator_put.part.0+0x14c/0x158
>   lr : regulator_put+0x34/0x48
>   sp : ffff8000122ebb30
>   x29: ffff8000122ebb30 x28: ffff800011be7000 x27: 0000000000000000
>   x26: 0000000000000000 x25: 0000000000000000 x24: ffff00000025f2bc
>   x23: ffff00000025f2c0 x22: ffff00000025f010 x21: ffff8000122ebc18
>   x20: ffff800011e3fa60 x19: ffff00000375fd80 x18: 0000000000000010
>   x17: 000000040044ffff x16: 00400032b5503510 x15: 0000000000000108
>   x14: ffff0000003cc938 x13: 00000000ffffffea x12: 0000000000000000
>   x11: 0000000000000000 x10: ffff80001076ba88 x9 : ffff80001076a540
>   x8 : ffff00000025f2c0 x7 : ffff0000001f4450 x6 : ffff000000176cd8
>   x5 : ffff000003857880 x4 : 0000000000000000 x3 : ffff800011e3fe30
>   x2 : ffff0000003cc4c0 x1 : 0000000000000000 x0 : 0000000000000001
>   Call trace:
>    _regulator_put.part.0+0x14c/0x158
>    regulator_put+0x34/0x48
>    devm_regulator_release+0x10/0x18
>    release_nodes+0x38/0x60
>    devres_release_all+0x88/0xd0
>    really_probe+0xd0/0x2e8
>    __driver_probe_device+0x74/0xd8
>    driver_probe_device+0x7c/0x108
>    __device_attach_driver+0x8c/0xd0
>    bus_for_each_drv+0x74/0xc0
>    __device_attach_async_helper+0xb4/0xd8
>    async_run_entry_fn+0x30/0x100
>    process_one_work+0x19c/0x320
>    worker_thread+0x48/0x418
>    kthread+0x14c/0x158
>    ret_from_fork+0x10/0x18
>   ---[ end trace 3664ca4a50ce849b ]---
>
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>

I am seeing this on imx6 too. When you send a v2, after addressing
Lucas' comments, please add a Fixes tag/

Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling unbalance when link never came up

[Bjorn Helgaas]

On Fri, Oct 15, 2021 at 02:05:40PM +0800, Richard Zhu wrote:
> When link never came up, driver probe would be failed with error -110.
> To keep usage counter balance of the clocks, disable the previous
> enabled clocks when link is down.
> Move definitions of the imx6_pcie_clk_disable() function to the proper
> place. Because it wouldn't be used in imx6_pcie_suspend_noirq() only.

Add blank line between paragraphs.

Can you please split this into two patches:

  1) the imx6_pcie_clk_disable() move
  2) the actual fix

It's hard to tell exactly where the fix is when things are mixed
together.

> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 47 ++++++++++++++-------------
>  1 file changed, 24 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index cc837f8bf6d4..d6a5d99ffa52 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -514,6 +514,29 @@ 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:
> +		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;
> @@ -853,6 +876,7 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
>  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
>  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
>  	imx6_pcie_reset_phy(imx6_pcie);
> +	imx6_pcie_clk_disable(imx6_pcie);
>  	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
>  		regulator_disable(imx6_pcie->vpcie);
>  	return ret;
> @@ -941,29 +965,6 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
>  	usleep_range(1000, 10000);
>  }
>  
> -static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
> -{
> -	clk_disable_unprepare(imx6_pcie->pcie);
> -	clk_disable_unprepare(imx6_pcie->pcie_phy);
> -	clk_disable_unprepare(imx6_pcie->pcie_bus);
> -
> -	switch (imx6_pcie->drvdata->variant) {
> -	case IMX6SX:
> -		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> -		break;
> -	case IMX7D:
> -		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> -				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> -				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> -		break;
> -	case IMX8MQ:
> -		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
> 

Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling unbalance when link never came up

[Bjorn Helgaas]

On Fri, Oct 15, 2021 at 01:49:45PM -0500, Bjorn Helgaas wrote:
> On Fri, Oct 15, 2021 at 02:05:40PM +0800, Richard Zhu wrote:
> > When link never came up, driver probe would be failed with error -110.
> > To keep usage counter balance of the clocks, disable the previous
> > enabled clocks when link is down.
> > Move definitions of the imx6_pcie_clk_disable() function to the proper
> > place. Because it wouldn't be used in imx6_pcie_suspend_noirq() only.

> > -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:
> > -		clk_disable_unprepare(imx6_pcie->pcie_aux);
> > -		break;
> > -	default:
> > -		break;

While you're at it, this "default: break;" thing is pointless.
Normally it's better to just *move* something without changing it at
all, but this is such a simple thing I think you could drop this at
the same time as the move.

> > -	}
> > -}

RE: [RESEND v2 1/5] PCI: imx6: Encapsulate the clock enable into one standalone function

[Richard Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: Saturday, October 16, 2021 2:14 AM
> To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com
> Cc: linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de
> Subject: Re: [RESEND v2 1/5] PCI: imx6: Encapsulate the clock enable into
> one standalone function
> 
> Am Freitag, dem 15.10.2021 um 14:05 +0800 schrieb 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>
> 
[Richard Zhu] Thanks.

Best Regards
Richard Zhu
> > ---
> >  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 26f49f797b0f..1fa1dba6da81 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -470,38 +470,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);
> > @@ -524,6 +502,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)) { @@ -578,13 +601,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)
> 

RE: [RESEND v2 2/5] PCI: imx6: Add the error propagation from host_init

[Richard Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: Saturday, October 16, 2021 2:16 AM
> To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com
> Cc: linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de
> Subject: Re: [RESEND v2 2/5] PCI: imx6: Add the error propagation from
> host_init
> 
> Am Freitag, dem 15.10.2021 um 14:05 +0800 schrieb 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>
> 
[Richard Zhu] Thanks.

Best Regards
Richard Zhu
> > ---
> >  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 1fa1dba6da81..3372775834a2 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -527,24 +527,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;
> >  	}
> >
> > @@ -599,7 +599,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)
> > { @@ -608,6 +608,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) @@
> > -858,11 +859,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;
> 

RE: [RESEND v2 3/5] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: Saturday, October 16, 2021 2:23 AM
> To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com
> Cc: linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de
> Subject: Re: [RESEND v2 3/5] PCI: imx6: Fix the regulator dump when link
> never came up
> 
> Am Freitag, dem 15.10.2021 um 14:05 +0800 schrieb Richard Zhu:
> > When PCIe PHY link never came up and vpcie regulator is present, there
> > would be following dump when try to put the regulator.
> > Disable this regulator to fix this dump when link never came up.
> >
> >   imx6q-pcie 33800000.pcie: Phy link never came up
> >   imx6q-pcie: probe of 33800000.pcie failed with error -110
> >   ------------[ cut here ]------------
> >   WARNING: CPU: 3 PID: 119 at drivers/regulator/core.c:2256
> _regulator_put.part.0+0x14c/0x158
> >   Modules linked in:
> >   CPU: 3 PID: 119 Comm: kworker/u8:2 Not tainted
> 5.13.0-rc7-next-20210625-94710-ge4e92b2588a3 #10
> >   Hardware name: FSL i.MX8MM EVK board (DT)
> >   Workqueue: events_unbound async_run_entry_fn
> >   pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> >   pc : _regulator_put.part.0+0x14c/0x158
> >   lr : regulator_put+0x34/0x48
> >   sp : ffff8000122ebb30
> >   x29: ffff8000122ebb30 x28: ffff800011be7000 x27: 0000000000000000
> >   x26: 0000000000000000 x25: 0000000000000000 x24: ffff00000025f2bc
> >   x23: ffff00000025f2c0 x22: ffff00000025f010 x21: ffff8000122ebc18
> >   x20: ffff800011e3fa60 x19: ffff00000375fd80 x18: 0000000000000010
> >   x17: 000000040044ffff x16: 00400032b5503510 x15:
> 0000000000000108
> >   x14: ffff0000003cc938 x13: 00000000ffffffea x12: 0000000000000000
> >   x11: 0000000000000000 x10: ffff80001076ba88 x9 : ffff80001076a540
> >   x8 : ffff00000025f2c0 x7 : ffff0000001f4450 x6 : ffff000000176cd8
> >   x5 : ffff000003857880 x4 : 0000000000000000 x3 : ffff800011e3fe30
> >   x2 : ffff0000003cc4c0 x1 : 0000000000000000 x0 : 0000000000000001
> >   Call trace:
> >    _regulator_put.part.0+0x14c/0x158
> >    regulator_put+0x34/0x48
> >    devm_regulator_release+0x10/0x18
> >    release_nodes+0x38/0x60
> >    devres_release_all+0x88/0xd0
> >    really_probe+0xd0/0x2e8
> >    __driver_probe_device+0x74/0xd8
> >    driver_probe_device+0x7c/0x108
> >    __device_attach_driver+0x8c/0xd0
> >    bus_for_each_drv+0x74/0xc0
> >    __device_attach_async_helper+0xb4/0xd8
> >    async_run_entry_fn+0x30/0x100
> >    process_one_work+0x19c/0x320
> >    worker_thread+0x48/0x418
> >    kthread+0x14c/0x158
> >    ret_from_fork+0x10/0x18
> >   ---[ end trace 3664ca4a50ce849b ]---
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 3372775834a2..cc837f8bf6d4 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -853,6 +853,8 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
> >  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> >  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> >  	imx6_pcie_reset_phy(imx6_pcie);
> > +	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> > +		regulator_disable(imx6_pcie->vpcie);
> 
> This doesn't seem like the right place to add this. I guess it would be better to
> have proper error handling after dw_pcie_host_init to roll back things like the
> clock and regulator enable.
[Richard Zhu] This is good suggestion. It's make sense that move the regulator disable to the
dw_pcie_host_init error handling since the regulator have the enabled check, before to disable it.
But it seems that there are some potential problems when do the similar operations to the clocks disable here.
Because that the dw_pcie_host_init might have some error returns when the host_init is not invoked at all.
How to handle this situation although this is one corner case?

Best Regards
Richard Zhu
> 
> Regards,
> Lucas
> 
> >  	return ret;
> >  }
> >
> 

RE: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling unbalance when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: Saturday, October 16, 2021 2:25 AM
> To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com
> Cc: linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de
> Subject: Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling
> unbalance when link never came up
> 
> Am Freitag, dem 15.10.2021 um 14:05 +0800 schrieb Richard Zhu:
> > When link never came up, driver probe would be failed with error -110.
> > To keep usage counter balance of the clocks, disable the previous
> > enabled clocks when link is down.
> > Move definitions of the imx6_pcie_clk_disable() function to the proper
> > place. Because it wouldn't be used in imx6_pcie_suspend_noirq() only.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 47
> > ++++++++++++++-------------
> >  1 file changed, 24 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index cc837f8bf6d4..d6a5d99ffa52 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -514,6 +514,29 @@ 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:
> > +		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;
> > @@ -853,6 +876,7 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
> >  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> >  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> >  	imx6_pcie_reset_phy(imx6_pcie);
> > +	imx6_pcie_clk_disable(imx6_pcie);
> 
> Same comment as with the previous patch. We should not cram in more error
> handling in the imx6_pcie_start_link function, but rather move out all the
> error handling to be after dw_pcie_host_init. Even the already existing phy
> reset here seems misplaced and should be moved out.
[Richard Zhu] About the clk disabled, please see my explains under the addressed comments in the #3 patch.
Agree to move the existing phy reset to the error handling after dw_pcie_host_init.
Thanks.
BR
Richard

> 
> Regards,
> Lucas
> 
> >  	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> >  		regulator_disable(imx6_pcie->vpcie);
> >  	return ret;
> > @@ -941,29 +965,6 @@ static void imx6_pcie_pm_turnoff(struct
> imx6_pcie *imx6_pcie)
> >  	usleep_range(1000, 10000);
> >  }
> >
> > -static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie) -{
> > -	clk_disable_unprepare(imx6_pcie->pcie);
> > -	clk_disable_unprepare(imx6_pcie->pcie_phy);
> > -	clk_disable_unprepare(imx6_pcie->pcie_bus);
> > -
> > -	switch (imx6_pcie->drvdata->variant) {
> > -	case IMX6SX:
> > -		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> > -		break;
> > -	case IMX7D:
> > -		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > -				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> > -				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> > -		break;
> > -	case IMX8MQ:
> > -		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: [RESEND v2 3/5] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Fabio Estevam <festevam@gmail.com>
> Sent: Saturday, October 16, 2021 2:35 AM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>; Bjorn Helgaas
> <bhelgaas@google.com>; Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>;
> linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; moderated
> list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> <linux-arm-kernel@lists.infradead.org>; linux-kernel
> <linux-kernel@vger.kernel.org>; Sascha Hauer <kernel@pengutronix.de>
> Subject: Re: [RESEND v2 3/5] PCI: imx6: Fix the regulator dump when link
> never came up
> 
> Hi Richard,
> 
> On Fri, Oct 15, 2021 at 3:32 AM Richard Zhu <hongxing.zhu@nxp.com> wrote:
> >
> > When PCIe PHY link never came up and vpcie regulator is present, there
> > would be following dump when try to put the regulator.
> > Disable this regulator to fix this dump when link never came up.
> >
> >   imx6q-pcie 33800000.pcie: Phy link never came up
> >   imx6q-pcie: probe of 33800000.pcie failed with error -110
> >   ------------[ cut here ]------------
> >   WARNING: CPU: 3 PID: 119 at drivers/regulator/core.c:2256
> _regulator_put.part.0+0x14c/0x158
> >   Modules linked in:
> >   CPU: 3 PID: 119 Comm: kworker/u8:2 Not tainted
> 5.13.0-rc7-next-20210625-94710-ge4e92b2588a3 #10
> >   Hardware name: FSL i.MX8MM EVK board (DT)
> >   Workqueue: events_unbound async_run_entry_fn
> >   pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> >   pc : _regulator_put.part.0+0x14c/0x158
> >   lr : regulator_put+0x34/0x48
> >   sp : ffff8000122ebb30
> >   x29: ffff8000122ebb30 x28: ffff800011be7000 x27: 0000000000000000
> >   x26: 0000000000000000 x25: 0000000000000000 x24: ffff00000025f2bc
> >   x23: ffff00000025f2c0 x22: ffff00000025f010 x21: ffff8000122ebc18
> >   x20: ffff800011e3fa60 x19: ffff00000375fd80 x18: 0000000000000010
> >   x17: 000000040044ffff x16: 00400032b5503510 x15:
> 0000000000000108
> >   x14: ffff0000003cc938 x13: 00000000ffffffea x12: 0000000000000000
> >   x11: 0000000000000000 x10: ffff80001076ba88 x9 : ffff80001076a540
> >   x8 : ffff00000025f2c0 x7 : ffff0000001f4450 x6 : ffff000000176cd8
> >   x5 : ffff000003857880 x4 : 0000000000000000 x3 : ffff800011e3fe30
> >   x2 : ffff0000003cc4c0 x1 : 0000000000000000 x0 : 0000000000000001
> >   Call trace:
> >    _regulator_put.part.0+0x14c/0x158
> >    regulator_put+0x34/0x48
> >    devm_regulator_release+0x10/0x18
> >    release_nodes+0x38/0x60
> >    devres_release_all+0x88/0xd0
> >    really_probe+0xd0/0x2e8
> >    __driver_probe_device+0x74/0xd8
> >    driver_probe_device+0x7c/0x108
> >    __device_attach_driver+0x8c/0xd0
> >    bus_for_each_drv+0x74/0xc0
> >    __device_attach_async_helper+0xb4/0xd8
> >    async_run_entry_fn+0x30/0x100
> >    process_one_work+0x19c/0x320
> >    worker_thread+0x48/0x418
> >    kthread+0x14c/0x158
> >    ret_from_fork+0x10/0x18
> >   ---[ end trace 3664ca4a50ce849b ]---
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> 
> I am seeing this on imx6 too. When you send a v2, after addressing Lucas'
> comments, please add a Fixes tag/
[Richard Zhu] Okay, no problem. Thanks.
BR
Richard

RE: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling unbalance when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Saturday, October 16, 2021 2:50 AM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de
> Subject: Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling
> unbalance when link never came up
> 
> On Fri, Oct 15, 2021 at 02:05:40PM +0800, Richard Zhu wrote:
> > When link never came up, driver probe would be failed with error -110.
> > To keep usage counter balance of the clocks, disable the previous
> > enabled clocks when link is down.
> > Move definitions of the imx6_pcie_clk_disable() function to the proper
> > place. Because it wouldn't be used in imx6_pcie_suspend_noirq() only.
> 
> Add blank line between paragraphs.
> 
> Can you please split this into two patches:
> 
>   1) the imx6_pcie_clk_disable() move
>   2) the actual fix
> 
> It's hard to tell exactly where the fix is when things are mixed together.
> 
[Richard Zhu] Okay, would split this patch into two patches later.
One is used for imx6_pcie_clk_disable() move, and no function changes.
The other one is the actual fix. Thanks.

Best Regards
Richard Zhu

> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 47
> > ++++++++++++++-------------
> >  1 file changed, 24 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index cc837f8bf6d4..d6a5d99ffa52 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -514,6 +514,29 @@ 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:
> > +		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;
> > @@ -853,6 +876,7 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
> >  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> >  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> >  	imx6_pcie_reset_phy(imx6_pcie);
> > +	imx6_pcie_clk_disable(imx6_pcie);
> >  	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> >  		regulator_disable(imx6_pcie->vpcie);
> >  	return ret;
> > @@ -941,29 +965,6 @@ static void imx6_pcie_pm_turnoff(struct
> imx6_pcie *imx6_pcie)
> >  	usleep_range(1000, 10000);
> >  }
> >
> > -static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie) -{
> > -	clk_disable_unprepare(imx6_pcie->pcie);
> > -	clk_disable_unprepare(imx6_pcie->pcie_phy);
> > -	clk_disable_unprepare(imx6_pcie->pcie_bus);
> > -
> > -	switch (imx6_pcie->drvdata->variant) {
> > -	case IMX6SX:
> > -		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> > -		break;
> > -	case IMX7D:
> > -		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > -				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> > -				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> > -		break;
> > -	case IMX8MQ:
> > -		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
> >

RE: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling unbalance when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Saturday, October 16, 2021 2:52 AM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de
> Subject: Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling
> unbalance when link never came up
> 
> On Fri, Oct 15, 2021 at 01:49:45PM -0500, Bjorn Helgaas wrote:
> > On Fri, Oct 15, 2021 at 02:05:40PM +0800, Richard Zhu wrote:
> > > When link never came up, driver probe would be failed with error -110.
> > > To keep usage counter balance of the clocks, disable the previous
> > > enabled clocks when link is down.
> > > Move definitions of the imx6_pcie_clk_disable() function to the
> > > proper place. Because it wouldn't be used in imx6_pcie_suspend_noirq()
> only.
> 
> > > -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:
> > > -		clk_disable_unprepare(imx6_pcie->pcie_aux);
> > > -		break;
> > > -	default:
> > > -		break;
> 
> While you're at it, this "default: break;" thing is pointless.
> Normally it's better to just *move* something without changing it at all, but
> this is such a simple thing I think you could drop this at the same time as the
> move.
> 
[Richard Zhu] Okay, got that. Would remove the "default:break" later. Thanks.

Best Regards
Richard Zhu

> > > -	}
> > > -}

RE: [RESEND v2 5/5] PCI: imx6: Add the compliance tests mode support

[Richard Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: Saturday, October 16, 2021 2:29 AM
> To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com
> Cc: linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de
> Subject: Re: [RESEND v2 5/5] PCI: imx6: Add the compliance tests mode
> support
> 
> Am Freitag, dem 15.10.2021 um 14:05 +0800 schrieb Richard Zhu:
> > Refer to the system board signal Quality of PCIe archiecture PHY test
> > specification. Signal quality tests(for example: jitters,
> > differential eye opening and so on ) can be executed with devices in
> > the polling.compliance state.
> >
> > To let the device support polling.compliance stat, the clocks and
> > powers shouldn't be turned off when the probe of device driver is failed.
> >
> > 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 | 32
> > ++++++++++++++++++++-------
> >  1 file changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index d6a5d99ffa52..e861a516d517 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -143,6 +143,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; @@ -812,10 +816,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);
> > @@ -876,9 +882,12 @@ static int imx6_pcie_start_link(struct dw_pcie
> *pci)
> >  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> >  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> >  	imx6_pcie_reset_phy(imx6_pcie);
> 
> Is it correct to reset the PHY here when in compliance test mode?
[Richard Zhu] It doesn't matter. PHY reset just let the PHY enter into a dedicated stat after link is down.
This wouldn't impact the compliance tests mode later.

Best Regards
Richard Zhu

> 
> > -	imx6_pcie_clk_disable(imx6_pcie);
> > -	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> > -		regulator_disable(imx6_pcie->vpcie);
> > +	if (!imx6_pcie_cmp_mode) {
> > +		imx6_pcie_clk_disable(imx6_pcie);
> > +		if (imx6_pcie->vpcie
> > +		    && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> > +			regulator_disable(imx6_pcie->vpcie);
> > +	}
> >  	return ret;
> >  }
> >
> > @@ -1183,8 +1192,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);
> 

RE: [RESEND v2 3/5] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Richard Zhu
> Sent: Tuesday, October 19, 2021 3:40 PM
> To: Lucas Stach <l.stach@pengutronix.de>; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com
> Cc: linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de
> Subject: RE: [RESEND v2 3/5] PCI: imx6: Fix the regulator dump when link
> never came up
> 
> > -----Original Message-----
> > From: Lucas Stach <l.stach@pengutronix.de>
> > Sent: Saturday, October 16, 2021 2:23 AM
> > To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> > lorenzo.pieralisi@arm.com
> > Cc: linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > kernel@pengutronix.de
> > Subject: Re: [RESEND v2 3/5] PCI: imx6: Fix the regulator dump when
> > link never came up
> >
> > Am Freitag, dem 15.10.2021 um 14:05 +0800 schrieb Richard Zhu:
> > > When PCIe PHY link never came up and vpcie regulator is present,
> > > there would be following dump when try to put the regulator.
> > > Disable this regulator to fix this dump when link never came up.
> > >
> > >   imx6q-pcie 33800000.pcie: Phy link never came up
> > >   imx6q-pcie: probe of 33800000.pcie failed with error -110
> > >   ------------[ cut here ]------------
> > >   WARNING: CPU: 3 PID: 119 at drivers/regulator/core.c:2256
> > _regulator_put.part.0+0x14c/0x158
> > >   Modules linked in:
> > >   CPU: 3 PID: 119 Comm: kworker/u8:2 Not tainted
> > 5.13.0-rc7-next-20210625-94710-ge4e92b2588a3 #10
> > >   Hardware name: FSL i.MX8MM EVK board (DT)
> > >   Workqueue: events_unbound async_run_entry_fn
> > >   pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> > >   pc : _regulator_put.part.0+0x14c/0x158
> > >   lr : regulator_put+0x34/0x48
> > >   sp : ffff8000122ebb30
> > >   x29: ffff8000122ebb30 x28: ffff800011be7000 x27:
> 0000000000000000
> > >   x26: 0000000000000000 x25: 0000000000000000 x24:
> ffff00000025f2bc
> > >   x23: ffff00000025f2c0 x22: ffff00000025f010 x21: ffff8000122ebc18
> > >   x20: ffff800011e3fa60 x19: ffff00000375fd80 x18: 0000000000000010
> > >   x17: 000000040044ffff x16: 00400032b5503510 x15:
> > 0000000000000108
> > >   x14: ffff0000003cc938 x13: 00000000ffffffea x12: 0000000000000000
> > >   x11: 0000000000000000 x10: ffff80001076ba88 x9 : ffff80001076a540
> > >   x8 : ffff00000025f2c0 x7 : ffff0000001f4450 x6 : ffff000000176cd8
> > >   x5 : ffff000003857880 x4 : 0000000000000000 x3 : ffff800011e3fe30
> > >   x2 : ffff0000003cc4c0 x1 : 0000000000000000 x0 : 0000000000000001
> > >   Call trace:
> > >    _regulator_put.part.0+0x14c/0x158
> > >    regulator_put+0x34/0x48
> > >    devm_regulator_release+0x10/0x18
> > >    release_nodes+0x38/0x60
> > >    devres_release_all+0x88/0xd0
> > >    really_probe+0xd0/0x2e8
> > >    __driver_probe_device+0x74/0xd8
> > >    driver_probe_device+0x7c/0x108
> > >    __device_attach_driver+0x8c/0xd0
> > >    bus_for_each_drv+0x74/0xc0
> > >    __device_attach_async_helper+0xb4/0xd8
> > >    async_run_entry_fn+0x30/0x100
> > >    process_one_work+0x19c/0x320
> > >    worker_thread+0x48/0x418
> > >    kthread+0x14c/0x158
> > >    ret_from_fork+0x10/0x18
> > >   ---[ end trace 3664ca4a50ce849b ]---
> > >
> > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > ---
> > >  drivers/pci/controller/dwc/pci-imx6.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 3372775834a2..cc837f8bf6d4 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -853,6 +853,8 @@ static int imx6_pcie_start_link(struct dw_pcie
> *pci)
> > >  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> > >  		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> > >  	imx6_pcie_reset_phy(imx6_pcie);
> > > +	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) >
> 0)
> > > +		regulator_disable(imx6_pcie->vpcie);
> >
> > This doesn't seem like the right place to add this. I guess it would
> > be better to have proper error handling after dw_pcie_host_init to
> > roll back things like the clock and regulator enable.
> [Richard Zhu] This is good suggestion. It's make sense that move the regulator
> disable to the dw_pcie_host_init error handling since the regulator have the
> enabled check, before to disable it.
> But it seems that there are some potential problems when do the similar
> operations to the clocks disable here.
> Because that the dw_pcie_host_init might have some error returns when the
> host_init is not invoked at all.
> How to handle this situation although this is one corner case?
> 
[Richard Zhu] I have an idea to handle this case.
Add one more callback(e.x: host_exit() ) into dw_pcie_host_ops struct, then do the host_exit() when there is a error return after host_init() is invoked.
How about this solution?
I would send the v3 patch-set later if you don't say no to this method.

BR
Richard  

> Best Regards
> Richard Zhu
> >
> > Regards,
> > Lucas
> >
> > >  	return ret;
> > >  }
> > >
> >

RE: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling unbalance when link never came up

[Richard Zhu]

<snipped ...>
> >
> > > > -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:
> > > > -		clk_disable_unprepare(imx6_pcie->pcie_aux);
> > > > -		break;
> > > > -	default:
> > > > -		break;
> >
> > While you're at it, this "default: break;" thing is pointless.
> > Normally it's better to just *move* something without changing it at
> > all, but this is such a simple thing I think you could drop this at
> > the same time as the move.
> >
> [Richard Zhu] Okay, got that. Would remove the "default:break" later. Thanks.
[Richard Zhu] I figure out that the default:break is required by IMX6Q/IMX6QP.
 So I just don't drop them in v3 patch-set.

> 
> Best Regards
> Richard Zhu
> 
> > > > -	}
> > > > -}

Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling unbalance when link never came up

[Krzysztof Wilczyński]

Hi,

[...]
> > > > > -	default:
> > > > > -		break;
> > >
> > > While you're at it, this "default: break;" thing is pointless.
> > > Normally it's better to just *move* something without changing it at
> > > all, but this is such a simple thing I think you could drop this at
> > > the same time as the move.
> > >
> > [Richard Zhu] Okay, got that. Would remove the "default:break" later. Thanks.
> [Richard Zhu] I figure out that the default:break is required by IMX6Q/IMX6QP.
>  So I just don't drop them in v3 patch-set.

I hope you don't mind me asking, but how is an empty default case in the
switch statement helping IMX6Q and IMX6QP?  What does it achieve for these
two controllers specifically?

	Krzysztof

RE: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling unbalance when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Krzysztof Wilczyński <kw@linux.com>
> Sent: Saturday, October 23, 2021 5:54 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Bjorn Helgaas <helgaas@kernel.org>; l.stach@pengutronix.de;
> bhelgaas@google.com; lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de
> Subject: Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling
> unbalance when link never came up
> 
> Hi,
> 
> [...]
> > > > > > -	default:
> > > > > > -		break;
> > > >
> > > > While you're at it, this "default: break;" thing is pointless.
> > > > Normally it's better to just *move* something without changing it
> > > > at all, but this is such a simple thing I think you could drop
> > > > this at the same time as the move.
> > > >
> > > [Richard Zhu] Okay, got that. Would remove the "default:break" later.
> Thanks.
> > [Richard Zhu] I figure out that the default:break is required by
> IMX6Q/IMX6QP.
> >  So I just don't drop them in v3 patch-set.
> 
> I hope you don't mind me asking, but how is an empty default case in the
> switch statement helping IMX6Q and IMX6QP?  What does it achieve for
> these two controllers specifically?
> 
[Richard Zhu] Never mind. 😊.
There might be following building warning if the "default:break" is removed.
"  CC      drivers/pci/controller/dwc/pci-imx6.o
drivers/pci/controller/dwc/pci-imx6.c: In function ‘imx6_pcie_clk_disable’:
drivers/pci/controller/dwc/pci-imx6.c:527:2: warning: enumeration value ‘IMX6Q’ not handled in switch [-Wswitch]
  527 |  switch (imx6_pcie->drvdata->variant) {
      |  ^~~~~~
drivers/pci/controller/dwc/pci-imx6.c:527:2: warning: enumeration value ‘IMX6QP’ not handled in switch [-Wswitch]"

Best Regards
Richard Zhu

> 	Krzysztof

Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling unbalance when link never came up

[Bjorn Helgaas]

On Fri, Oct 22, 2021 at 08:02:17AM +0000, Richard Zhu wrote:
> <snipped ...>
> > >
> > > > > -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:
> > > > > -		clk_disable_unprepare(imx6_pcie->pcie_aux);
> > > > > -		break;
> > > > > -	default:
> > > > > -		break;
> > >
> > > While you're at it, this "default: break;" thing is pointless.
> > > Normally it's better to just *move* something without changing it at
> > > all, but this is such a simple thing I think you could drop this at
> > > the same time as the move.
> > >
> > [Richard Zhu] Okay, got that. Would remove the "default:break" later. Thanks.
> [Richard Zhu] I figure out that the default:break is required by
> IMX6Q/IMX6QP.  So I just don't drop them in v3 patch-set.

That makes no sense.  The code is:

  +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:
  +               clk_disable_unprepare(imx6_pcie->pcie_aux);
  +               break;
  +       default:
  +               break;
  +       }
  +}

Why do you think it makes a difference to remove the
"default: break;"?  There is no executable code after it.
I don't see how IMX6Q/IMX6QP could depend on the default
case.

BTW, I feel like a broken record, but your v3 posting still has
inconsistent subject line capitalization:

  PCI: imx6: move the clock disable function to a proper place
  PCI: dwc: add a new callback host exit function into host ops

It would be nice if they were consistent and contained more specific
information, e.g.,

  PCI: imx6: Move imx6_pcie_clk_disable() earlier
  PCI: dwc: Add dw_pcie_host_ops.host_exit() callback

Bjorn

Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling unbalance when link never came up

[Bjorn Helgaas]

On Mon, Oct 25, 2021 at 02:35:36AM +0000, Richard Zhu wrote:
> > -----Original Message-----
> > From: Krzysztof Wilczyński <kw@linux.com>
> > Sent: Saturday, October 23, 2021 5:54 PM
> > To: Richard Zhu <hongxing.zhu@nxp.com>
> > Cc: Bjorn Helgaas <helgaas@kernel.org>; l.stach@pengutronix.de;
> > bhelgaas@google.com; lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org;
> > dl-linux-imx <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; kernel@pengutronix.de
> > Subject: Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling
> > unbalance when link never came up

> > I hope you don't mind me asking, but how is an empty default case in the
> > switch statement helping IMX6Q and IMX6QP?  What does it achieve for
> > these two controllers specifically?
> > 
> [Richard Zhu] Never mind. 😊.
> There might be following building warning if the "default:break" is removed.
> "  CC      drivers/pci/controller/dwc/pci-imx6.o
> drivers/pci/controller/dwc/pci-imx6.c: In function ‘imx6_pcie_clk_disable’:
> drivers/pci/controller/dwc/pci-imx6.c:527:2: warning: enumeration value ‘IMX6Q’ not handled in switch [-Wswitch]
>   527 |  switch (imx6_pcie->drvdata->variant) {
>       |  ^~~~~~
> drivers/pci/controller/dwc/pci-imx6.c:527:2: warning: enumeration value ‘IMX6QP’ not handled in switch [-Wswitch]"

Sorry, I didn't see this until after asking the same question as
Krzysztof.

Sigh.  That's a really annoying gcc warning, but I guess I won't fight
it ;)

Bjorn

RE: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling unbalance when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Wednesday, October 27, 2021 12:39 AM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de
> Subject: Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling
> unbalance when link never came up
> 
> On Fri, Oct 22, 2021 at 08:02:17AM +0000, Richard Zhu wrote:
> > <snipped ...>
> > > >
> > > > > > -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:
> > > > > > -		clk_disable_unprepare(imx6_pcie->pcie_aux);
> > > > > > -		break;
> > > > > > -	default:
> > > > > > -		break;
> > > >
> > > > While you're at it, this "default: break;" thing is pointless.
> > > > Normally it's better to just *move* something without changing it
> > > > at all, but this is such a simple thing I think you could drop
> > > > this at the same time as the move.
> > > >
> > > [Richard Zhu] Okay, got that. Would remove the "default:break" later.
> Thanks.
> > [Richard Zhu] I figure out that the default:break is required by
> > IMX6Q/IMX6QP.  So I just don't drop them in v3 patch-set.
> 
> That makes no sense.  The code is:
> 
>   +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:
>   +               clk_disable_unprepare(imx6_pcie->pcie_aux);
>   +               break;
>   +       default:
>   +               break;
>   +       }
>   +}
> 
> Why do you think it makes a difference to remove the
> "default: break;"?  There is no executable code after it.
> I don't see how IMX6Q/IMX6QP could depend on the default case.
> 
> BTW, I feel like a broken record, but your v3 posting still has inconsistent
> subject line capitalization:
> 
>   PCI: imx6: move the clock disable function to a proper place
>   PCI: dwc: add a new callback host exit function into host ops
> 
> It would be nice if they were consistent and contained more specific
> information, e.g.,
> 
>   PCI: imx6: Move imx6_pcie_clk_disable() earlier
>   PCI: dwc: Add dw_pcie_host_ops.host_exit() callback
[Richard Zhu] Got that, would change to be consistent and more specific information. Thanks a lot.

BR
Richard
> 
> Bjorn

RE: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling unbalance when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Wednesday, October 27, 2021 6:22 AM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Krzysztof Wilczyński <kw@linux.com>; l.stach@pengutronix.de;
> bhelgaas@google.com; lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de
> Subject: Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling
> unbalance when link never came up
> 
> On Mon, Oct 25, 2021 at 02:35:36AM +0000, Richard Zhu wrote:
> > > -----Original Message-----
> > > From: Krzysztof Wilczyński <kw@linux.com>
> > > Sent: Saturday, October 23, 2021 5:54 PM
> > > To: Richard Zhu <hongxing.zhu@nxp.com>
> > > Cc: Bjorn Helgaas <helgaas@kernel.org>; l.stach@pengutronix.de;
> > > bhelgaas@google.com; lorenzo.pieralisi@arm.com;
> > > linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> > > linux-arm-kernel@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; kernel@pengutronix.de
> > > Subject: Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference
> > > handling unbalance when link never came up
> 
> > > I hope you don't mind me asking, but how is an empty default case in
> > > the switch statement helping IMX6Q and IMX6QP?  What does it achieve
> > > for these two controllers specifically?
> > >
> > [Richard Zhu] Never mind. 😊.
> > There might be following building warning if the "default:break" is removed.
> > "  CC      drivers/pci/controller/dwc/pci-imx6.o
> > drivers/pci/controller/dwc/pci-imx6.c: In function
> ‘imx6_pcie_clk_disable’:
> > drivers/pci/controller/dwc/pci-imx6.c:527:2: warning: enumeration value
> ‘IMX6Q’ not handled in switch [-Wswitch]
> >   527 |  switch (imx6_pcie->drvdata->variant) {
> >       |  ^~~~~~
> > drivers/pci/controller/dwc/pci-imx6.c:527:2: warning: enumeration value
> ‘IMX6QP’ not handled in switch [-Wswitch]"
> 
> Sorry, I didn't see this until after asking the same question as Krzysztof.
> 
> Sigh.  That's a really annoying gcc warning, but I guess I won't fight it ;)
[Richard Zhu] Never mind, 😊.

BR
Richard
> 
> Bjorn