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

[PATCH v3 0/7] 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
- Add one new host_exit callback, used to balance the usage of the
  regulator and clocks when link never came up
- Add the compliance tests mode support

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

[PATCH v3 1/7] PCI: imx6: Encapsulate the clock enable into one
[PATCH v3 2/7] PCI: imx6: Add the error propagation from host_init
[PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came
[PATCH v3 4/7] PCI: imx6: move the clock disable function to a proper
[PATCH v3 5/7] PCI: dwc: add a new callback host exit function into
[PATCH v3 6/7] PCI: imx6: Fix the reference handling unbalance when
[PATCH v3 7/7] PCI: imx6: Add the compliance tests mode support

[PATCH v3 0/7] 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
- Add one new host_exit callback, used to balance the usage of the
  regulator and clocks when link never came up
- Add the compliance tests mode support

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

[PATCH v3 1/7] PCI: imx6: Encapsulate the clock enable into one
[PATCH v3 2/7] PCI: imx6: Add the error propagation from host_init
[PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came
[PATCH v3 4/7] PCI: imx6: move the clock disable function to a proper
[PATCH v3 5/7] PCI: dwc: add a new callback host exit function into
[PATCH v3 6/7] PCI: imx6: Fix the reference handling unbalance when
[PATCH v3 7/7] PCI: imx6: Add the compliance tests mode support

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 1/7] 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 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

[PATCH v3 1/7] 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 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 2/7] 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 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

[PATCH v3 2/7] 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 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 3/7] 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 ]---

Link: https://lore.kernel.org/r/20201105211159.1814485-11-robh@kernel.org
Fixes: 886a9c134755 ("PCI: dwc: Move link handling into common code")
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 3372775834a2..39a485bfc676 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1180,8 +1180,12 @@ 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->vpcie
+		    && regulator_is_enabled(imx6_pcie->vpcie) > 0)
+			regulator_disable(imx6_pcie->vpcie);
 		return ret;
+	}
 
 	if (pci_msi_enabled()) {
 		u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
-- 
2.25.1

[PATCH v3 3/7] 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 ]---

Link: https://lore.kernel.org/r/20201105211159.1814485-11-robh@kernel.org
Fixes: 886a9c134755 ("PCI: dwc: Move link handling into common code")
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 3372775834a2..39a485bfc676 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1180,8 +1180,12 @@ 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->vpcie
+		    && regulator_is_enabled(imx6_pcie->vpcie) > 0)
+			regulator_disable(imx6_pcie->vpcie);
 		return ret;
+	}
 
 	if (pci_msi_enabled()) {
 		u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Francesco Dolcini]

Hello Richard,
please see this comment from Mark,  https://lore.kernel.org/all/YXaGve1ZJq0DGZ9l@sirena.org.uk/.

Francesco


On Fri, Oct 22, 2021 at 03:12:26PM +0800, Richard Zhu 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 ]---
> 
> Link: https://lore.kernel.org/r/20201105211159.1814485-11-robh@kernel.org
> Fixes: 886a9c134755 ("PCI: dwc: Move link handling into common code")
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 3372775834a2..39a485bfc676 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1180,8 +1180,12 @@ 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->vpcie
> +		    && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> +			regulator_disable(imx6_pcie->vpcie);
>  		return ret;
> +	}
>  
>  	if (pci_msi_enabled()) {
>  		u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Francesco Dolcini]

Hello Richard,
please see this comment from Mark,  https://lore.kernel.org/all/YXaGve1ZJq0DGZ9l@sirena.org.uk/.

Francesco


On Fri, Oct 22, 2021 at 03:12:26PM +0800, Richard Zhu 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 ]---
> 
> Link: https://lore.kernel.org/r/20201105211159.1814485-11-robh@kernel.org
> Fixes: 886a9c134755 ("PCI: dwc: Move link handling into common code")
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 3372775834a2..39a485bfc676 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1180,8 +1180,12 @@ 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->vpcie
> +		    && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> +			regulator_disable(imx6_pcie->vpcie);
>  		return ret;
> +	}
>  
>  	if (pci_msi_enabled()) {
>  		u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 768 bytes --]

On Mon, Oct 25, 2021 at 01:13:12PM +0200, Francesco Dolcini wrote:

> Hello Richard,
> please see this comment from Mark,  https://lore.kernel.org/all/YXaGve1ZJq0DGZ9l@sirena.org.uk/.

> > +		if (imx6_pcie->vpcie
> > +		    && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> > +			regulator_disable(imx6_pcie->vpcie);
> >  		return ret;

I should probably also say that the check for the regulator looks buggy
as well, regulators should only be optional if they can be physically
absent which does not seem likely for PCI devices.  If the driver is not
doing something to reconfigure the hardware to account for a missing
supply this is generally a big warning sign.

I really don't understand why regulator support is so frequently
problematic for PCI controllers.  :(

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 768 bytes --]

On Mon, Oct 25, 2021 at 01:13:12PM +0200, Francesco Dolcini wrote:

> Hello Richard,
> please see this comment from Mark,  https://lore.kernel.org/all/YXaGve1ZJq0DGZ9l@sirena.org.uk/.

> > +		if (imx6_pcie->vpcie
> > +		    && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> > +			regulator_disable(imx6_pcie->vpcie);
> >  		return ret;

I should probably also say that the check for the regulator looks buggy
as well, regulators should only be optional if they can be physically
absent which does not seem likely for PCI devices.  If the driver is not
doing something to reconfigure the hardware to account for a missing
supply this is generally a big warning sign.

I really don't understand why regulator support is so frequently
problematic for PCI controllers.  :(

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Monday, October 25, 2021 7:24 PM
> To: Francesco Dolcini <francesco.dolcini@toradex.com>
> Cc: Richard Zhu <hongxing.zhu@nxp.com>; l.stach@pengutronix.de;
> bhelgaas@google.com; lorenzo.pieralisi@arm.com; jingoohan1@gmail.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: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never
> came up
> 
> On Mon, Oct 25, 2021 at 01:13:12PM +0200, Francesco Dolcini wrote:
> 
> > Hello Richard,
> > please see this comment from Mark,
> https://lore.kernel.org/all/YXaGve1ZJq0DGZ9l@sirena.org.uk/.
> 
> > > +		if (imx6_pcie->vpcie
> > > +		    && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> > > +			regulator_disable(imx6_pcie->vpcie);
> > >  		return ret;
> 
> I should probably also say that the check for the regulator looks buggy as well,
> regulators should only be optional if they can be physically absent which does
> not seem likely for PCI devices.  If the driver is not doing something to
> reconfigure the hardware to account for a missing supply this is generally a big
> warning sign.
> 
> I really don't understand why regulator support is so frequently problematic
> for PCI controllers.  :(
[Richard Zhu] Hi Mark:
The _enabled check is used because that this regulator is optional in the HW design.
To make the codes clean and aligned on different HW boards, the _enabled check is added.

The root cause is that the error return is not handled properly by the controller driver.
I.MX PCIe controller doesn't support the Hot-Plug, and it would return -110 error
when PCIe link never came up. Thus, the _probe would be failed in the end.
The clocks/regulator usage balance should be considered by i.MX PCIe controller, that's all.
It's not a general case, and the problem is not caused by the regulator support.

Best Regards
Richard Zhu

RE: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Monday, October 25, 2021 7:24 PM
> To: Francesco Dolcini <francesco.dolcini@toradex.com>
> Cc: Richard Zhu <hongxing.zhu@nxp.com>; l.stach@pengutronix.de;
> bhelgaas@google.com; lorenzo.pieralisi@arm.com; jingoohan1@gmail.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: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never
> came up
> 
> On Mon, Oct 25, 2021 at 01:13:12PM +0200, Francesco Dolcini wrote:
> 
> > Hello Richard,
> > please see this comment from Mark,
> https://lore.kernel.org/all/YXaGve1ZJq0DGZ9l@sirena.org.uk/.
> 
> > > +		if (imx6_pcie->vpcie
> > > +		    && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> > > +			regulator_disable(imx6_pcie->vpcie);
> > >  		return ret;
> 
> I should probably also say that the check for the regulator looks buggy as well,
> regulators should only be optional if they can be physically absent which does
> not seem likely for PCI devices.  If the driver is not doing something to
> reconfigure the hardware to account for a missing supply this is generally a big
> warning sign.
> 
> I really don't understand why regulator support is so frequently problematic
> for PCI controllers.  :(
[Richard Zhu] Hi Mark:
The _enabled check is used because that this regulator is optional in the HW design.
To make the codes clean and aligned on different HW boards, the _enabled check is added.

The root cause is that the error return is not handled properly by the controller driver.
I.MX PCIe controller doesn't support the Hot-Plug, and it would return -110 error
when PCIe link never came up. Thus, the _probe would be failed in the end.
The clocks/regulator usage balance should be considered by i.MX PCIe controller, that's all.
It's not a general case, and the problem is not caused by the regulator support.

Best Regards
Richard Zhu


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Francesco Dolcini]

On Tue, Oct 26, 2021 at 02:18:18AM +0000, Richard Zhu wrote:
> The _enabled check is used because that this regulator is optional in the HW design.
> To make the codes clean and aligned on different HW boards, the _enabled check is added.
> 
> The root cause is that the error return is not handled properly by the controller driver.
> I.MX PCIe controller doesn't support the Hot-Plug, and it would return -110 error
> when PCIe link never came up. Thus, the _probe would be failed in the end.
> The clocks/regulator usage balance should be considered by i.MX PCIe controller, that's all.
> It's not a general case, and the problem is not caused by the regulator support.

Hello Richard,
I have one curiosity on this topic. Does this works well in case the regulator
is shared? I just want to be sure that if the regulator is shared everything is working fine
even if the PCI-E link is not used or not coming up for any reason.

Francesco

Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Francesco Dolcini]

On Tue, Oct 26, 2021 at 02:18:18AM +0000, Richard Zhu wrote:
> The _enabled check is used because that this regulator is optional in the HW design.
> To make the codes clean and aligned on different HW boards, the _enabled check is added.
> 
> The root cause is that the error return is not handled properly by the controller driver.
> I.MX PCIe controller doesn't support the Hot-Plug, and it would return -110 error
> when PCIe link never came up. Thus, the _probe would be failed in the end.
> The clocks/regulator usage balance should be considered by i.MX PCIe controller, that's all.
> It's not a general case, and the problem is not caused by the regulator support.

Hello Richard,
I have one curiosity on this topic. Does this works well in case the regulator
is shared? I just want to be sure that if the regulator is shared everything is working fine
even if the PCI-E link is not used or not coming up for any reason.

Francesco


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> Sent: Tuesday, October 26, 2021 4:52 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Mark Brown <broonie@kernel.org>; Francesco Dolcini
> <francesco.dolcini@toradex.com>; l.stach@pengutronix.de;
> bhelgaas@google.com; lorenzo.pieralisi@arm.com; jingoohan1@gmail.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: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never
> came up
> 
> On Tue, Oct 26, 2021 at 02:18:18AM +0000, Richard Zhu wrote:
> > The _enabled check is used because that this regulator is optional in the HW
> design.
> > To make the codes clean and aligned on different HW boards, the _enabled
> check is added.
> >
> > The root cause is that the error return is not handled properly by the
> controller driver.
> > I.MX PCIe controller doesn't support the Hot-Plug, and it would return
> > -110 error when PCIe link never came up. Thus, the _probe would be failed
> in the end.
> > The clocks/regulator usage balance should be considered by i.MX PCIe
> controller, that's all.
> > It's not a general case, and the problem is not caused by the regulator
> support.
> 
> Hello Richard,
> I have one curiosity on this topic. Does this works well in case the regulator is
> shared? I just want to be sure that if the regulator is shared everything is
> working fine even if the PCI-E link is not used or not coming up for any reason.
> 
[Richard Zhu] Hi Francesco:
Actually, this regulator used by i.MX PCIe controller driver is one fixed gpio regulator.
Refer to my understand, this regulator is not shared by different devices.

Up to now, it works fine to power up the PCIe slot populated on the HW board.

BR
Richard

> Francesco

RE: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> Sent: Tuesday, October 26, 2021 4:52 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Mark Brown <broonie@kernel.org>; Francesco Dolcini
> <francesco.dolcini@toradex.com>; l.stach@pengutronix.de;
> bhelgaas@google.com; lorenzo.pieralisi@arm.com; jingoohan1@gmail.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: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never
> came up
> 
> On Tue, Oct 26, 2021 at 02:18:18AM +0000, Richard Zhu wrote:
> > The _enabled check is used because that this regulator is optional in the HW
> design.
> > To make the codes clean and aligned on different HW boards, the _enabled
> check is added.
> >
> > The root cause is that the error return is not handled properly by the
> controller driver.
> > I.MX PCIe controller doesn't support the Hot-Plug, and it would return
> > -110 error when PCIe link never came up. Thus, the _probe would be failed
> in the end.
> > The clocks/regulator usage balance should be considered by i.MX PCIe
> controller, that's all.
> > It's not a general case, and the problem is not caused by the regulator
> support.
> 
> Hello Richard,
> I have one curiosity on this topic. Does this works well in case the regulator is
> shared? I just want to be sure that if the regulator is shared everything is
> working fine even if the PCI-E link is not used or not coming up for any reason.
> 
[Richard Zhu] Hi Francesco:
Actually, this regulator used by i.MX PCIe controller driver is one fixed gpio regulator.
Refer to my understand, this regulator is not shared by different devices.

Up to now, it works fine to power up the PCIe slot populated on the HW board.

BR
Richard

> Francesco


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Francesco Dolcini]

On Tue, Oct 26, 2021 at 09:06:56AM +0000, Richard Zhu wrote:
> > -----Original Message-----
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > Sent: Tuesday, October 26, 2021 4:52 PM
> > To: Richard Zhu <hongxing.zhu@nxp.com>
> > Cc: Mark Brown <broonie@kernel.org>; Francesco Dolcini
> > <francesco.dolcini@toradex.com>; l.stach@pengutronix.de;
> > bhelgaas@google.com; lorenzo.pieralisi@arm.com; jingoohan1@gmail.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: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never
> > came up
> > 
> > On Tue, Oct 26, 2021 at 02:18:18AM +0000, Richard Zhu wrote:
> > > The _enabled check is used because that this regulator is optional in the HW
> > design.
> > > To make the codes clean and aligned on different HW boards, the _enabled
> > check is added.
> > >
> > > The root cause is that the error return is not handled properly by the
> > controller driver.
> > > I.MX PCIe controller doesn't support the Hot-Plug, and it would return
> > > -110 error when PCIe link never came up. Thus, the _probe would be failed
> > in the end.
> > > The clocks/regulator usage balance should be considered by i.MX PCIe
> > controller, that's all.
> > > It's not a general case, and the problem is not caused by the regulator
> > support.
> > 
> > Hello Richard,
> > I have one curiosity on this topic. Does this works well in case the regulator is
> > shared? I just want to be sure that if the regulator is shared everything is
> > working fine even if the PCI-E link is not used or not coming up for any reason.
> > 
> [Richard Zhu] Hi Francesco:
> Actually, this regulator used by i.MX PCIe controller driver is one fixed gpio regulator.
> Refer to my understand, this regulator is not shared by different devices.
> 
> Up to now, it works fine to power up the PCIe slot populated on the HW board.

Isn't this something that depend on the actual board design? From the driver point of view
you should not silently enforce such design requirement on the board.
Am I missing something here? Would be glad to you if you can clarify in case.

Francesco

Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Francesco Dolcini]

On Tue, Oct 26, 2021 at 09:06:56AM +0000, Richard Zhu wrote:
> > -----Original Message-----
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > Sent: Tuesday, October 26, 2021 4:52 PM
> > To: Richard Zhu <hongxing.zhu@nxp.com>
> > Cc: Mark Brown <broonie@kernel.org>; Francesco Dolcini
> > <francesco.dolcini@toradex.com>; l.stach@pengutronix.de;
> > bhelgaas@google.com; lorenzo.pieralisi@arm.com; jingoohan1@gmail.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: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never
> > came up
> > 
> > On Tue, Oct 26, 2021 at 02:18:18AM +0000, Richard Zhu wrote:
> > > The _enabled check is used because that this regulator is optional in the HW
> > design.
> > > To make the codes clean and aligned on different HW boards, the _enabled
> > check is added.
> > >
> > > The root cause is that the error return is not handled properly by the
> > controller driver.
> > > I.MX PCIe controller doesn't support the Hot-Plug, and it would return
> > > -110 error when PCIe link never came up. Thus, the _probe would be failed
> > in the end.
> > > The clocks/regulator usage balance should be considered by i.MX PCIe
> > controller, that's all.
> > > It's not a general case, and the problem is not caused by the regulator
> > support.
> > 
> > Hello Richard,
> > I have one curiosity on this topic. Does this works well in case the regulator is
> > shared? I just want to be sure that if the regulator is shared everything is
> > working fine even if the PCI-E link is not used or not coming up for any reason.
> > 
> [Richard Zhu] Hi Francesco:
> Actually, this regulator used by i.MX PCIe controller driver is one fixed gpio regulator.
> Refer to my understand, this regulator is not shared by different devices.
> 
> Up to now, it works fine to power up the PCIe slot populated on the HW board.

Isn't this something that depend on the actual board design? From the driver point of view
you should not silently enforce such design requirement on the board.
Am I missing something here? Would be glad to you if you can clarify in case.

Francesco



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

Snipped...

> > >
> > > Hello Richard,
> > > I have one curiosity on this topic. Does this works well in case the
> > > regulator is shared? I just want to be sure that if the regulator is
> > > shared everything is working fine even if the PCI-E link is not used or not
> coming up for any reason.
> > >
> > [Richard Zhu] Hi Francesco:
> > Actually, this regulator used by i.MX PCIe controller driver is one fixed gpio
> regulator.
> > Refer to my understand, this regulator is not shared by different devices.
> >
> > Up to now, it works fine to power up the PCIe slot populated on the HW
> board.
> 
> Isn't this something that depend on the actual board design? From the driver
> point of view you should not silently enforce such design requirement on the
> board.
> Am I missing something here? Would be glad to you if you can clarify in case.
> 
[Richard Zhu] Yes, it is relied on the actual HW board design.
This regulator is one optional, not mandatory required for all the board designs.
So, there is one _enabled or not check before manipulate this regulator.

BR
Richard

> Francesco
> 

RE: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

Snipped...

> > >
> > > Hello Richard,
> > > I have one curiosity on this topic. Does this works well in case the
> > > regulator is shared? I just want to be sure that if the regulator is
> > > shared everything is working fine even if the PCI-E link is not used or not
> coming up for any reason.
> > >
> > [Richard Zhu] Hi Francesco:
> > Actually, this regulator used by i.MX PCIe controller driver is one fixed gpio
> regulator.
> > Refer to my understand, this regulator is not shared by different devices.
> >
> > Up to now, it works fine to power up the PCIe slot populated on the HW
> board.
> 
> Isn't this something that depend on the actual board design? From the driver
> point of view you should not silently enforce such design requirement on the
> board.
> Am I missing something here? Would be glad to you if you can clarify in case.
> 
[Richard Zhu] Yes, it is relied on the actual HW board design.
This regulator is one optional, not mandatory required for all the board designs.
So, there is one _enabled or not check before manipulate this regulator.

BR
Richard

> Francesco
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Francesco Dolcini]

On Tue, Oct 26, 2021 at 09:18:39AM +0000, Richard Zhu wrote:
> > Isn't this something that depend on the actual board design? From the driver
> > point of view you should not silently enforce such design requirement on the
> > board.
> > Am I missing something here? Would be glad to you if you can clarify in case.
> > 
> [Richard Zhu] Yes, it is relied on the actual HW board design.
> This regulator is one optional, not mandatory required for all the board designs.
> So, there is one _enabled or not check before manipulate this regulator.
I think I was not clear in my question.

I'm asking what's is going to happen if the vpci-e supply is used in the
actual board design AND the same regulator is shared with another device (to my
understanding this should be just fine from the regulator API
point of view, correct me if I'm wrong).

I'm not talking about board designed by NXP in which such use case might not
exist.

Francesco

Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Francesco Dolcini]

On Tue, Oct 26, 2021 at 09:18:39AM +0000, Richard Zhu wrote:
> > Isn't this something that depend on the actual board design? From the driver
> > point of view you should not silently enforce such design requirement on the
> > board.
> > Am I missing something here? Would be glad to you if you can clarify in case.
> > 
> [Richard Zhu] Yes, it is relied on the actual HW board design.
> This regulator is one optional, not mandatory required for all the board designs.
> So, there is one _enabled or not check before manipulate this regulator.
I think I was not clear in my question.

I'm asking what's is going to happen if the vpci-e supply is used in the
actual board design AND the same regulator is shared with another device (to my
understanding this should be just fine from the regulator API
point of view, correct me if I'm wrong).

I'm not talking about board designed by NXP in which such use case might not
exist.

Francesco


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> Sent: Tuesday, October 26, 2021 5:49 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Francesco Dolcini <francesco.dolcini@toradex.com>; Mark Brown
> <broonie@kernel.org>; l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.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: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link
> never came up
> 
> On Tue, Oct 26, 2021 at 09:18:39AM +0000, Richard Zhu wrote:
> > > Isn't this something that depend on the actual board design? From
> > > the driver point of view you should not silently enforce such design
> > > requirement on the board.
> > > Am I missing something here? Would be glad to you if you can clarify
> in case.
> > >
> > [Richard Zhu] Yes, it is relied on the actual HW board design.
> > This regulator is one optional, not mandatory required for all the board
> designs.
> > So, there is one _enabled or not check before manipulate this regulator.
> I think I was not clear in my question.
> 
> I'm asking what's is going to happen if the vpci-e supply is used in the
> actual board design AND the same regulator is shared with another
> device (to my understanding this should be just fine from the regulator API
> point of view, correct me if I'm wrong).
[Richard Zhu] Yes, agree with you. 
It should be fine from the regulator API point of view.
BR
Richard

> 
> I'm not talking about board designed by NXP in which such use case might
> not exist.
> 
> Francesco

RE: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> Sent: Tuesday, October 26, 2021 5:49 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Francesco Dolcini <francesco.dolcini@toradex.com>; Mark Brown
> <broonie@kernel.org>; l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.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: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link
> never came up
> 
> On Tue, Oct 26, 2021 at 09:18:39AM +0000, Richard Zhu wrote:
> > > Isn't this something that depend on the actual board design? From
> > > the driver point of view you should not silently enforce such design
> > > requirement on the board.
> > > Am I missing something here? Would be glad to you if you can clarify
> in case.
> > >
> > [Richard Zhu] Yes, it is relied on the actual HW board design.
> > This regulator is one optional, not mandatory required for all the board
> designs.
> > So, there is one _enabled or not check before manipulate this regulator.
> I think I was not clear in my question.
> 
> I'm asking what's is going to happen if the vpci-e supply is used in the
> actual board design AND the same regulator is shared with another
> device (to my understanding this should be just fine from the regulator API
> point of view, correct me if I'm wrong).
[Richard Zhu] Yes, agree with you. 
It should be fine from the regulator API point of view.
BR
Richard

> 
> I'm not talking about board designed by NXP in which such use case might
> not exist.
> 
> Francesco


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]

On Tue, Oct 26, 2021 at 02:18:18AM +0000, Richard Zhu wrote:

> > I should probably also say that the check for the regulator looks buggy as well,
> > regulators should only be optional if they can be physically absent which does
> > not seem likely for PCI devices.  If the driver is not doing something to
> > reconfigure the hardware to account for a missing supply this is generally a big
> > warning sign.
> > 
> > I really don't understand why regulator support is so frequently problematic
> > for PCI controllers.  :(

> [Richard Zhu] Hi Mark:
> The _enabled check is used because that this regulator is optional in the HW design.
> To make the codes clean and aligned on different HW boards, the _enabled check is added.

I would be really surprised to see PCI hardware that was able to support
a supply being physically absent, and this use of _is_enabled() is quite
simply not how any of this is supposed to work in the regulator API even
for regulators that can be optional.

> The root cause is that the error return is not handled properly by the controller driver.
> I.MX PCIe controller doesn't support the Hot-Plug, and it would return -110 error
> when PCIe link never came up. Thus, the _probe would be failed in the end.
> The clocks/regulator usage balance should be considered by i.MX PCIe controller, that's all.
> It's not a general case, and the problem is not caused by the regulator support.

Perhaps it's not causing problems in this design but if the supply is
ever shared with anything else then the software will run into trouble.
There will also be problems with the error handling on a system where
the regulator needs to be controlled.

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1879 bytes --]

On Tue, Oct 26, 2021 at 02:18:18AM +0000, Richard Zhu wrote:

> > I should probably also say that the check for the regulator looks buggy as well,
> > regulators should only be optional if they can be physically absent which does
> > not seem likely for PCI devices.  If the driver is not doing something to
> > reconfigure the hardware to account for a missing supply this is generally a big
> > warning sign.
> > 
> > I really don't understand why regulator support is so frequently problematic
> > for PCI controllers.  :(

> [Richard Zhu] Hi Mark:
> The _enabled check is used because that this regulator is optional in the HW design.
> To make the codes clean and aligned on different HW boards, the _enabled check is added.

I would be really surprised to see PCI hardware that was able to support
a supply being physically absent, and this use of _is_enabled() is quite
simply not how any of this is supposed to work in the regulator API even
for regulators that can be optional.

> The root cause is that the error return is not handled properly by the controller driver.
> I.MX PCIe controller doesn't support the Hot-Plug, and it would return -110 error
> when PCIe link never came up. Thus, the _probe would be failed in the end.
> The clocks/regulator usage balance should be considered by i.MX PCIe controller, that's all.
> It's not a general case, and the problem is not caused by the regulator support.

Perhaps it's not causing problems in this design but if the supply is
ever shared with anything else then the software will run into trouble.
There will also be problems with the error handling on a system where
the regulator needs to be controlled.

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Tuesday, October 26, 2021 6:58 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Francesco Dolcini <francesco.dolcini@toradex.com>;
> l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.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: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link
> never came up
> 
> On Tue, Oct 26, 2021 at 02:18:18AM +0000, Richard Zhu wrote:
> 
> > > I should probably also say that the check for the regulator looks
> > > buggy as well, regulators should only be optional if they can be
> > > physically absent which does not seem likely for PCI devices.  If
> > > the driver is not doing something to reconfigure the hardware to
> > > account for a missing supply this is generally a big warning sign.
> > >
> > > I really don't understand why regulator support is so frequently
> > > problematic for PCI controllers.  :(
> 
> > [Richard Zhu] Hi Mark:
> > The _enabled check is used because that this regulator is optional in the
> HW design.
> > To make the codes clean and aligned on different HW boards, the
> _enabled check is added.
> 
> I would be really surprised to see PCI hardware that was able to support a
> supply being physically absent, and this use of _is_enabled() is quite
> simply not how any of this is supposed to work in the regulator API even
> for regulators that can be optional.
[Richard Zhu] Actually, this regulator is one GPIO fixed regulator.
Controlled by SW to turn on (GPIO high) or turn off (GPIO low) the supply.
In some boards designs, this supply might be always on(GPIO high).
So, in point of SW driver view, this regulator is optional.

> 
> > The root cause is that the error return is not handled properly by the
> controller driver.
> > I.MX PCIe controller doesn't support the Hot-Plug, and it would return
> > -110 error when PCIe link never came up. Thus, the _probe would be
> failed in the end.
> > The clocks/regulator usage balance should be considered by i.MX PCIe
> controller, that's all.
> > It's not a general case, and the problem is not caused by the regulator
> support.
> 
> Perhaps it's not causing problems in this design but if the supply is ever
> shared with anything else then the software will run into trouble.
> There will also be problems with the error handling on a system where
> the regulator needs to be controlled.
[Richard Zhu] This GPIO fixed regulator is only used by controller driver.
It makes sense to disable the enabled regulator when driver probe is failed.

> 
> Please fix your mail client to word wrap within paragraphs at something
> substantially less than 80 columns.  Doing this makes your messages
> much easier to read and reply to.
[Richard] Sorry about that.
BR
Richard

RE: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Tuesday, October 26, 2021 6:58 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Francesco Dolcini <francesco.dolcini@toradex.com>;
> l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.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: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link
> never came up
> 
> On Tue, Oct 26, 2021 at 02:18:18AM +0000, Richard Zhu wrote:
> 
> > > I should probably also say that the check for the regulator looks
> > > buggy as well, regulators should only be optional if they can be
> > > physically absent which does not seem likely for PCI devices.  If
> > > the driver is not doing something to reconfigure the hardware to
> > > account for a missing supply this is generally a big warning sign.
> > >
> > > I really don't understand why regulator support is so frequently
> > > problematic for PCI controllers.  :(
> 
> > [Richard Zhu] Hi Mark:
> > The _enabled check is used because that this regulator is optional in the
> HW design.
> > To make the codes clean and aligned on different HW boards, the
> _enabled check is added.
> 
> I would be really surprised to see PCI hardware that was able to support a
> supply being physically absent, and this use of _is_enabled() is quite
> simply not how any of this is supposed to work in the regulator API even
> for regulators that can be optional.
[Richard Zhu] Actually, this regulator is one GPIO fixed regulator.
Controlled by SW to turn on (GPIO high) or turn off (GPIO low) the supply.
In some boards designs, this supply might be always on(GPIO high).
So, in point of SW driver view, this regulator is optional.

> 
> > The root cause is that the error return is not handled properly by the
> controller driver.
> > I.MX PCIe controller doesn't support the Hot-Plug, and it would return
> > -110 error when PCIe link never came up. Thus, the _probe would be
> failed in the end.
> > The clocks/regulator usage balance should be considered by i.MX PCIe
> controller, that's all.
> > It's not a general case, and the problem is not caused by the regulator
> support.
> 
> Perhaps it's not causing problems in this design but if the supply is ever
> shared with anything else then the software will run into trouble.
> There will also be problems with the error handling on a system where
> the regulator needs to be controlled.
[Richard Zhu] This GPIO fixed regulator is only used by controller driver.
It makes sense to disable the enabled regulator when driver probe is failed.

> 
> Please fix your mail client to word wrap within paragraphs at something
> substantially less than 80 columns.  Doing this makes your messages
> much easier to read and reply to.
[Richard] Sorry about that.
BR
Richard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1503 bytes --]

On Thu, Oct 28, 2021 at 06:50:58AM +0000, Richard Zhu wrote:

> > I would be really surprised to see PCI hardware that was able to support a
> > supply being physically absent, and this use of _is_enabled() is quite
> > simply not how any of this is supposed to work in the regulator API even
> > for regulators that can be optional.

> [Richard Zhu] Actually, this regulator is one GPIO fixed regulator.
> Controlled by SW to turn on (GPIO high) or turn off (GPIO low) the supply.
> In some boards designs, this supply might be always on(GPIO high).
> So, in point of SW driver view, this regulator is optional.

No, it's not.  The regulator API supports the systems where the
regualtor is always on perfectly well, the client driver should not need
to do anything to support them.

> > Perhaps it's not causing problems in this design but if the supply is ever
> > shared with anything else then the software will run into trouble.
> > There will also be problems with the error handling on a system where
> > the regulator needs to be controlled.

> [Richard Zhu] This GPIO fixed regulator is only used by controller driver.
> It makes sense to disable the enabled regulator when driver probe is failed.

The driver should undo any enables it did itself, it should not undo any
enables that anything else did which means it should never be basing
decisions on regulator_is_enabled().  While the regulator may not be
shared in the particular board you're looking at it may be shared in
other systems.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1503 bytes --]

On Thu, Oct 28, 2021 at 06:50:58AM +0000, Richard Zhu wrote:

> > I would be really surprised to see PCI hardware that was able to support a
> > supply being physically absent, and this use of _is_enabled() is quite
> > simply not how any of this is supposed to work in the regulator API even
> > for regulators that can be optional.

> [Richard Zhu] Actually, this regulator is one GPIO fixed regulator.
> Controlled by SW to turn on (GPIO high) or turn off (GPIO low) the supply.
> In some boards designs, this supply might be always on(GPIO high).
> So, in point of SW driver view, this regulator is optional.

No, it's not.  The regulator API supports the systems where the
regualtor is always on perfectly well, the client driver should not need
to do anything to support them.

> > Perhaps it's not causing problems in this design but if the supply is ever
> > shared with anything else then the software will run into trouble.
> > There will also be problems with the error handling on a system where
> > the regulator needs to be controlled.

> [Richard Zhu] This GPIO fixed regulator is only used by controller driver.
> It makes sense to disable the enabled regulator when driver probe is failed.

The driver should undo any enables it did itself, it should not undo any
enables that anything else did which means it should never be basing
decisions on regulator_is_enabled().  While the regulator may not be
shared in the particular board you're looking at it may be shared in
other systems.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Thursday, October 28, 2021 7:50 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Francesco Dolcini <francesco.dolcini@toradex.com>;
> l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.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: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link
> never came up
> 
> On Thu, Oct 28, 2021 at 06:50:58AM +0000, Richard Zhu wrote:
> 
> > > I would be really surprised to see PCI hardware that was able to
> > > support a supply being physically absent, and this use of
> > > _is_enabled() is quite simply not how any of this is supposed to
> > > work in the regulator API even for regulators that can be optional.
> 
> > [Richard Zhu] Actually, this regulator is one GPIO fixed regulator.
> > Controlled by SW to turn on (GPIO high) or turn off (GPIO low) the
> supply.
> > In some boards designs, this supply might be always on(GPIO high).
> > So, in point of SW driver view, this regulator is optional.
> 
> No, it's not.  The regulator API supports the systems where the regualtor
> is always on perfectly well, the client driver should not need to do
> anything to support them.
[Richard Zhu] Hi Mark: Thanks for your explains.
To disable the regulator explicitly, is a part of power save of i.MX PCIe port
 usage when link is down.
Because that this regulator might not be present at all on some boards
 (e.x: powered directly when board is powered up), so this regulator is
 optional from SW view.
> 
> > > Perhaps it's not causing problems in this design but if the supply
> > > is ever shared with anything else then the software will run into
> trouble.
> > > There will also be problems with the error handling on a system
> > > where the regulator needs to be controlled.
> 
> > [Richard Zhu] This GPIO fixed regulator is only used by controller driver.
> > It makes sense to disable the enabled regulator when driver probe is
> failed.
> 
> The driver should undo any enables it did itself, it should not undo any
> enables that anything else did which means it should never be basing
> decisions on regulator_is_enabled().  While the regulator may not be
> shared in the particular board you're looking at it may be shared in other
> systems.
[Richard Zhu] Understood. Thanks.
Can I disabled this regulator in PCIe probe failure handler without the
 regulator_is_enabled() check?

BR
Richard

RE: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Thursday, October 28, 2021 7:50 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Francesco Dolcini <francesco.dolcini@toradex.com>;
> l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.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: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link
> never came up
> 
> On Thu, Oct 28, 2021 at 06:50:58AM +0000, Richard Zhu wrote:
> 
> > > I would be really surprised to see PCI hardware that was able to
> > > support a supply being physically absent, and this use of
> > > _is_enabled() is quite simply not how any of this is supposed to
> > > work in the regulator API even for regulators that can be optional.
> 
> > [Richard Zhu] Actually, this regulator is one GPIO fixed regulator.
> > Controlled by SW to turn on (GPIO high) or turn off (GPIO low) the
> supply.
> > In some boards designs, this supply might be always on(GPIO high).
> > So, in point of SW driver view, this regulator is optional.
> 
> No, it's not.  The regulator API supports the systems where the regualtor
> is always on perfectly well, the client driver should not need to do
> anything to support them.
[Richard Zhu] Hi Mark: Thanks for your explains.
To disable the regulator explicitly, is a part of power save of i.MX PCIe port
 usage when link is down.
Because that this regulator might not be present at all on some boards
 (e.x: powered directly when board is powered up), so this regulator is
 optional from SW view.
> 
> > > Perhaps it's not causing problems in this design but if the supply
> > > is ever shared with anything else then the software will run into
> trouble.
> > > There will also be problems with the error handling on a system
> > > where the regulator needs to be controlled.
> 
> > [Richard Zhu] This GPIO fixed regulator is only used by controller driver.
> > It makes sense to disable the enabled regulator when driver probe is
> failed.
> 
> The driver should undo any enables it did itself, it should not undo any
> enables that anything else did which means it should never be basing
> decisions on regulator_is_enabled().  While the regulator may not be
> shared in the particular board you're looking at it may be shared in other
> systems.
[Richard Zhu] Understood. Thanks.
Can I disabled this regulator in PCIe probe failure handler without the
 regulator_is_enabled() check?

BR
Richard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]

On Fri, Oct 29, 2021 at 03:58:41AM +0000, Richard Zhu wrote:

> > The driver should undo any enables it did itself, it should not undo any
> > enables that anything else did which means it should never be basing
> > decisions on regulator_is_enabled().  While the regulator may not be
> > shared in the particular board you're looking at it may be shared in other
> > systems.

> [Richard Zhu] Understood. Thanks.
> Can I disabled this regulator in PCIe probe failure handler without the
>  regulator_is_enabled() check?

If the driver called regulator_enable() (and that didn't return an
error) it can always call regulator_disable() as many times as it called
regulator_enable(), no need to check if the regulator is still enabled.  
When the driver hasn't successfully called regulator_enable() it
shouldn't call regulator_disable() even if the regualtor is enabled.
This means that after your driver has enabled the regulator it can just
disable it but between the regulator_get() and regulator_enable() it
shouldn't do that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1050 bytes --]

On Fri, Oct 29, 2021 at 03:58:41AM +0000, Richard Zhu wrote:

> > The driver should undo any enables it did itself, it should not undo any
> > enables that anything else did which means it should never be basing
> > decisions on regulator_is_enabled().  While the regulator may not be
> > shared in the particular board you're looking at it may be shared in other
> > systems.

> [Richard Zhu] Understood. Thanks.
> Can I disabled this regulator in PCIe probe failure handler without the
>  regulator_is_enabled() check?

If the driver called regulator_enable() (and that didn't return an
error) it can always call regulator_disable() as many times as it called
regulator_enable(), no need to check if the regulator is still enabled.  
When the driver hasn't successfully called regulator_enable() it
shouldn't call regulator_disable() even if the regualtor is enabled.
This means that after your driver has enabled the regulator it can just
disable it but between the regulator_get() and regulator_enable() it
shouldn't do that.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Friday, October 29, 2021 7:46 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Francesco Dolcini <francesco.dolcini@toradex.com>;
> l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.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: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link
> never came up
> 
> On Fri, Oct 29, 2021 at 03:58:41AM +0000, Richard Zhu wrote:
> 
> > > The driver should undo any enables it did itself, it should not undo
> > > any enables that anything else did which means it should never be
> > > basing decisions on regulator_is_enabled().  While the regulator may
> > > not be shared in the particular board you're looking at it may be
> > > shared in other systems.
> 
> > [Richard Zhu] Understood. Thanks.
> > Can I disabled this regulator in PCIe probe failure handler without
> > the
> >  regulator_is_enabled() check?
> 
> If the driver called regulator_enable() (and that didn't return an
> error) it can always call regulator_disable() as many times as it called
> regulator_enable(), no need to check if the regulator is still enabled.
> When the driver hasn't successfully called regulator_enable() it shouldn't
> call regulator_disable() even if the regualtor is enabled.
> This means that after your driver has enabled the regulator it can just
> disable it but between the regulator_get() and regulator_enable() it
> shouldn't do that.
[Richard Zhu] Got that, thanks a lot.
BR
Richard

RE: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Friday, October 29, 2021 7:46 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Francesco Dolcini <francesco.dolcini@toradex.com>;
> l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.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: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link
> never came up
> 
> On Fri, Oct 29, 2021 at 03:58:41AM +0000, Richard Zhu wrote:
> 
> > > The driver should undo any enables it did itself, it should not undo
> > > any enables that anything else did which means it should never be
> > > basing decisions on regulator_is_enabled().  While the regulator may
> > > not be shared in the particular board you're looking at it may be
> > > shared in other systems.
> 
> > [Richard Zhu] Understood. Thanks.
> > Can I disabled this regulator in PCIe probe failure handler without
> > the
> >  regulator_is_enabled() check?
> 
> If the driver called regulator_enable() (and that didn't return an
> error) it can always call regulator_disable() as many times as it called
> regulator_enable(), no need to check if the regulator is still enabled.
> When the driver hasn't successfully called regulator_enable() it shouldn't
> call regulator_disable() even if the regualtor is enabled.
> This means that after your driver has enabled the regulator it can just
> disable it but between the regulator_get() and regulator_enable() it
> shouldn't do that.
[Richard Zhu] Got that, thanks a lot.
BR
Richard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> Sent: Monday, October 25, 2021 7:13 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.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; Mark Brown <broonie@kernel.org>
> Subject: Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never
> came up
> 
> Hello Richard,
> please see this comment from Mark,
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.ke
> rnel.org%2Fall%2FYXaGve1ZJq0DGZ9l%40sirena.org.uk%2F&amp;data=04%7
> C01%7Chongxing.zhu%40nxp.com%7C27ddf15a4ef34496eff708d997a87097
> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637707571965246
> 405%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=nLZcGoLwXWEr
> HR14ZLg8prtPuotNWHBrQb8J99HiNT0%3D&amp;reserved=0.
> 
> Francesco
[Richard Zhu] Got that, thanks for your reminder.

Best Regards
Richard Zhu
> 
> 
> On Fri, Oct 22, 2021 at 03:12:26PM +0800, Richard Zhu 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 ]---
> >
> > Link:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Fr%2F20201105211159.1814485-11-robh%40kernel.org&am
> p;data
> >
> =04%7C01%7Chongxing.zhu%40nxp.com%7C27ddf15a4ef34496eff708d997a
> 87097%7
> >
> C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63770757196524640
> 5%7CUnkno
> >
> wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> aWwiL
> >
> CJXVCI6Mn0%3D%7C1000&amp;sdata=mJbSEVYuoGCbCOqqXdcUG00JXu74l
> 5%2BYxpzCX
> > yK96pE%3D&amp;reserved=0
> > Fixes: 886a9c134755 ("PCI: dwc: Move link handling into common code")
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 3372775834a2..39a485bfc676 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -1180,8 +1180,12 @@ 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->vpcie
> > +		    && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> > +			regulator_disable(imx6_pcie->vpcie);
> >  		return ret;
> > +	}
> >
> >  	if (pci_msi_enabled()) {
> >  		u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> > --
> > 2.25.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists
> > .infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&amp;data=04%
> 7C0
> >
> 1%7Chongxing.zhu%40nxp.com%7C27ddf15a4ef34496eff708d997a87097%7
> C686ea1
> >
> d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637707571965246405%7CUn
> known%7CTW
> >
> FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6
> >
> Mn0%3D%7C1000&amp;sdata=SHLkdzTt2F1Nht9eoefHlEH9Klsnw3%2F7KOP
> uGGeI%2Ba
> > w%3D&amp;reserved=0

RE: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu]

> -----Original Message-----
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> Sent: Monday, October 25, 2021 7:13 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.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; Mark Brown <broonie@kernel.org>
> Subject: Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never
> came up
> 
> Hello Richard,
> please see this comment from Mark,
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.ke
> rnel.org%2Fall%2FYXaGve1ZJq0DGZ9l%40sirena.org.uk%2F&amp;data=04%7
> C01%7Chongxing.zhu%40nxp.com%7C27ddf15a4ef34496eff708d997a87097
> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637707571965246
> 405%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=nLZcGoLwXWEr
> HR14ZLg8prtPuotNWHBrQb8J99HiNT0%3D&amp;reserved=0.
> 
> Francesco
[Richard Zhu] Got that, thanks for your reminder.

Best Regards
Richard Zhu
> 
> 
> On Fri, Oct 22, 2021 at 03:12:26PM +0800, Richard Zhu 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 ]---
> >
> > Link:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Fr%2F20201105211159.1814485-11-robh%40kernel.org&am
> p;data
> >
> =04%7C01%7Chongxing.zhu%40nxp.com%7C27ddf15a4ef34496eff708d997a
> 87097%7
> >
> C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63770757196524640
> 5%7CUnkno
> >
> wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> aWwiL
> >
> CJXVCI6Mn0%3D%7C1000&amp;sdata=mJbSEVYuoGCbCOqqXdcUG00JXu74l
> 5%2BYxpzCX
> > yK96pE%3D&amp;reserved=0
> > Fixes: 886a9c134755 ("PCI: dwc: Move link handling into common code")
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 3372775834a2..39a485bfc676 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -1180,8 +1180,12 @@ 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->vpcie
> > +		    && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> > +			regulator_disable(imx6_pcie->vpcie);
> >  		return ret;
> > +	}
> >
> >  	if (pci_msi_enabled()) {
> >  		u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> > --
> > 2.25.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists
> > .infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&amp;data=04%
> 7C0
> >
> 1%7Chongxing.zhu%40nxp.com%7C27ddf15a4ef34496eff708d997a87097%7
> C686ea1
> >
> d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637707571965246405%7CUn
> known%7CTW
> >
> FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6
> >
> Mn0%3D%7C1000&amp;sdata=SHLkdzTt2F1Nht9eoefHlEH9Klsnw3%2F7KOP
> uGGeI%2Ba
> > w%3D&amp;reserved=0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 4/7] PCI: imx6: move the clock disable function to a proper place

[Richard Zhu]

Just move the imx6_pcie_clk_disable() to a proper 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 | 46 +++++++++++++--------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 39a485bfc676..b752a673e767 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;
@@ -939,29 +962,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

[PATCH v3 4/7] PCI: imx6: move the clock disable function to a proper place

[Richard Zhu]

Just move the imx6_pcie_clk_disable() to a proper 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 | 46 +++++++++++++--------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 39a485bfc676..b752a673e767 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;
@@ -939,29 +962,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 5/7] PCI: dwc: add a new callback host exit function into host ops

[Richard Zhu]

When link is never came up in the link training after host_init.
The clocks and power supplies usage counter balance should be handled
properly on some DWC platforms (for example, i.MX PCIe).

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.

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 d1d9b8344ec9..9d450e71b93b 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -404,7 +404,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 */
@@ -416,6 +416,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 v3 5/7] PCI: dwc: add a new callback host exit function into host ops

[Richard Zhu]

When link is never came up in the link training after host_init.
The clocks and power supplies usage counter balance should be handled
properly on some DWC platforms (for example, i.MX PCIe).

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.

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 d1d9b8344ec9..9d450e71b93b 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -404,7 +404,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 */
@@ -416,6 +416,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 6/7] PCI: imx6: Fix the 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, powers and so on.

Add a new host_exit() callback for i.MX PCIe driver to handle this case
in the error handling after host_init.

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index b752a673e767..c723df053574 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -875,7 +875,6 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 	dev_dbg(dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n",
 		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
 		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
-	imx6_pcie_reset_phy(imx6_pcie);
 	return ret;
 }
 
@@ -899,8 +898,20 @@ 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 imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
+
+	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);
+}
+
 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 = {
@@ -1180,12 +1191,8 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 		return ret;
 
 	ret = dw_pcie_host_init(&pci->pp);
-	if (ret < 0) {
-		if (imx6_pcie->vpcie
-		    && regulator_is_enabled(imx6_pcie->vpcie) > 0)
-			regulator_disable(imx6_pcie->vpcie);
+	if (ret < 0)
 		return ret;
-	}
 
 	if (pci_msi_enabled()) {
 		u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
-- 
2.25.1

[PATCH v3 6/7] PCI: imx6: Fix the 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, powers and so on.

Add a new host_exit() callback for i.MX PCIe driver to handle this case
in the error handling after host_init.

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index b752a673e767..c723df053574 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -875,7 +875,6 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 	dev_dbg(dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n",
 		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
 		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
-	imx6_pcie_reset_phy(imx6_pcie);
 	return ret;
 }
 
@@ -899,8 +898,20 @@ 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 imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
+
+	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);
+}
+
 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 = {
@@ -1180,12 +1191,8 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 		return ret;
 
 	ret = dw_pcie_host_init(&pci->pp);
-	if (ret < 0) {
-		if (imx6_pcie->vpcie
-		    && regulator_is_enabled(imx6_pcie->vpcie) > 0)
-			regulator_disable(imx6_pcie->vpcie);
+	if (ret < 0)
 		return ret;
-	}
 
 	if (pci_msi_enabled()) {
 		u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 7/7] 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 | 34 ++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index c723df053574..0eb84fae817d 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);
@@ -903,10 +909,13 @@ static void imx6_pcie_host_exit(struct pcie_port *pp)
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
 
-	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_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);
+	}
 }
 
 static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
@@ -1191,8 +1200,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

[PATCH v3 7/7] 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 | 34 ++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index c723df053574..0eb84fae817d 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);
@@ -903,10 +909,13 @@ static void imx6_pcie_host_exit(struct pcie_port *pp)
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
 
-	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_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);
+	}
 }
 
 static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
@@ -1191,8 +1200,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel