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

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

[Richard Zhu]

This series patches refine pci-imx6 driver and do the following changes.
- Encapsulate the clock enable into one standalone function
- Add the error propagation from host_init
- Balance the usage of the regulator and clocks when link never came up
- Add the compliance tests mode support

Main changes from v5 to v6:
- Refer to the following discussion with Fabio, fix the dump by his patch.
  https://patchwork.kernel.org/project/linux-pci/patch/1641368602-20401-6-git-send-email-hongxing.zhu@nxp.com/
  Refine and rebase this patch-set after Fabio' dump fix patch is merged.
- Add one new #4 patch to disable i.MX6QDL REF clock too when disable clocks
- Split the regulator refine codes into one standalone patch #5 in this version.

Main changes from v4 to v5:
- Since i.MX8MM PCIe support had been merged. Based on Lorenzo's git repos,
  resend the patch-set after rebase.

Main changes from v3 to v4:
- Regarding Mark's comments, delete the regulator_is_enabled() check.
- Squash #3 and #6 of v3 patch into #5 patch of v4 set.

Main changes from v2 to v3:
- Add "Reviewed-by: Lucas Stach <l.stach@pengutronix.de>" tag into
  first two patches.
- Add a Fixes tag into #3 patch.
- Split the #4 of v2 to two patches, one is clock disable codes move,
  the other one is the acutal clock unbalance fix.
- Add a new host_exit() callback into dw_pcie_host_ops, then it could be
  invoked to handle the unbalance issue in the error handling after
  host_init() function when link is down.
- Add a new host_exit() callback for i.MX PCIe driver to handle this case
  in the error handling after host_init.

Main changes from v1 to v2:
Regarding Lucas' comments.
  - Move the placement of the new imx6_pcie_clk_enable() to avoid the
    forward declarition.
  - Seperate the second patch of v1 patch-set to three patches.
  - Use the module_param to replace the kernel command line.
Regarding Bjorn's comments:
  - Use the cover-letter for a multi-patch series.
  - Correct the subject line, and refine the commit logs. For example,
    remove the timestamp of the logs.

drivers/pci/controller/dwc/pci-imx6.c             | 213 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------
drivers/pci/controller/dwc/pcie-designware-host.c |   5 ++-
drivers/pci/controller/dwc/pcie-designware.h      |   1 +
3 files changed, 142 insertions(+), 77 deletions(-)

[PATCH v6 1/8] PCI: imx6: Encapsulate the clock enable into one
[PATCH v6 2/8] PCI: imx6: Add the error propagation from host_init
[PATCH v6 3/8] PCI: imx6: Move imx6_pcie_clk_disable() earlier
[PATCH v6 4/8] PCI: imx6: Disable imx6qdl pcie ref clk
[PATCH v6 5/8] PCI: imx6: Refine the regulator usage
[PATCH v6 6/8] PCI: dwc: Add dw_pcie_host_ops.host_exit() callback
[PATCH v6 7/8] PCI: imx6: Disable enabled clocks and regulators after
[PATCH v6 8/8] 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 v6 0/8] PCI: imx6: refine codes and add compliance tests mode support

[Richard Zhu]

This series patches refine pci-imx6 driver and do the following changes.
- Encapsulate the clock enable into one standalone function
- Add the error propagation from host_init
- Balance the usage of the regulator and clocks when link never came up
- Add the compliance tests mode support

Main changes from v5 to v6:
- Refer to the following discussion with Fabio, fix the dump by his patch.
  https://patchwork.kernel.org/project/linux-pci/patch/1641368602-20401-6-git-send-email-hongxing.zhu@nxp.com/
  Refine and rebase this patch-set after Fabio' dump fix patch is merged.
- Add one new #4 patch to disable i.MX6QDL REF clock too when disable clocks
- Split the regulator refine codes into one standalone patch #5 in this version.

Main changes from v4 to v5:
- Since i.MX8MM PCIe support had been merged. Based on Lorenzo's git repos,
  resend the patch-set after rebase.

Main changes from v3 to v4:
- Regarding Mark's comments, delete the regulator_is_enabled() check.
- Squash #3 and #6 of v3 patch into #5 patch of v4 set.

Main changes from v2 to v3:
- Add "Reviewed-by: Lucas Stach <l.stach@pengutronix.de>" tag into
  first two patches.
- Add a Fixes tag into #3 patch.
- Split the #4 of v2 to two patches, one is clock disable codes move,
  the other one is the acutal clock unbalance fix.
- Add a new host_exit() callback into dw_pcie_host_ops, then it could be
  invoked to handle the unbalance issue in the error handling after
  host_init() function when link is down.
- Add a new host_exit() callback for i.MX PCIe driver to handle this case
  in the error handling after host_init.

Main changes from v1 to v2:
Regarding Lucas' comments.
  - Move the placement of the new imx6_pcie_clk_enable() to avoid the
    forward declarition.
  - Seperate the second patch of v1 patch-set to three patches.
  - Use the module_param to replace the kernel command line.
Regarding Bjorn's comments:
  - Use the cover-letter for a multi-patch series.
  - Correct the subject line, and refine the commit logs. For example,
    remove the timestamp of the logs.

drivers/pci/controller/dwc/pci-imx6.c             | 213 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------
drivers/pci/controller/dwc/pcie-designware-host.c |   5 ++-
drivers/pci/controller/dwc/pcie-designware.h      |   1 +
3 files changed, 142 insertions(+), 77 deletions(-)

[PATCH v6 1/8] PCI: imx6: Encapsulate the clock enable into one
[PATCH v6 2/8] PCI: imx6: Add the error propagation from host_init
[PATCH v6 3/8] PCI: imx6: Move imx6_pcie_clk_disable() earlier
[PATCH v6 4/8] PCI: imx6: Disable imx6qdl pcie ref clk
[PATCH v6 5/8] PCI: imx6: Refine the regulator usage
[PATCH v6 6/8] PCI: dwc: Add dw_pcie_host_ops.host_exit() callback
[PATCH v6 7/8] PCI: imx6: Disable enabled clocks and regulators after
[PATCH v6 8/8] PCI: imx6: Add the compliance tests mode support

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

[Richard Zhu]

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

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 5fd33e4536af..5582ee1b420a 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -481,38 +481,16 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
 	return ret;
 }
 
-static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
-{
-	u32 val;
-	struct device *dev = imx6_pcie->pci->dev;
-
-	if (regmap_read_poll_timeout(imx6_pcie->iomuxc_gpr,
-				     IOMUXC_GPR22, val,
-				     val & IMX7D_GPR22_PCIE_PHY_PLL_LOCKED,
-				     PHY_PLL_LOCK_WAIT_USLEEP_MAX,
-				     PHY_PLL_LOCK_WAIT_TIMEOUT))
-		dev_err(dev, "PCIe PLL lock timeout\n");
-}
-
-static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
+static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
 {
 	struct dw_pcie *pci = imx6_pcie->pci;
 	struct device *dev = pci->dev;
 	int ret;
 
-	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
-		ret = regulator_enable(imx6_pcie->vpcie);
-		if (ret) {
-			dev_err(dev, "failed to enable vpcie regulator: %d\n",
-				ret);
-			return;
-		}
-	}
-
 	ret = clk_prepare_enable(imx6_pcie->pcie_phy);
 	if (ret) {
 		dev_err(dev, "unable to enable pcie_phy clock\n");
-		goto err_pcie_phy;
+		return ret;
 	}
 
 	ret = clk_prepare_enable(imx6_pcie->pcie_bus);
@@ -543,6 +521,51 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	}
 	/* allow the clocks to stabilize */
 	usleep_range(200, 500);
+	return 0;
+
+err_ref_clk:
+	clk_disable_unprepare(imx6_pcie->pcie);
+err_pcie:
+	clk_disable_unprepare(imx6_pcie->pcie_bus);
+err_pcie_bus:
+	clk_disable_unprepare(imx6_pcie->pcie_phy);
+
+	return ret;
+}
+
+static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
+{
+	u32 val;
+	struct device *dev = imx6_pcie->pci->dev;
+
+	if (regmap_read_poll_timeout(imx6_pcie->iomuxc_gpr,
+				     IOMUXC_GPR22, val,
+				     val & IMX7D_GPR22_PCIE_PHY_PLL_LOCKED,
+				     PHY_PLL_LOCK_WAIT_USLEEP_MAX,
+				     PHY_PLL_LOCK_WAIT_TIMEOUT))
+		dev_err(dev, "PCIe PLL lock timeout\n");
+}
+
+static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
+{
+	struct dw_pcie *pci = imx6_pcie->pci;
+	struct device *dev = pci->dev;
+	int ret;
+
+	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
+		ret = regulator_enable(imx6_pcie->vpcie);
+		if (ret) {
+			dev_err(dev, "failed to enable vpcie regulator: %d\n",
+				ret);
+			return;
+		}
+	}
+
+	ret = imx6_pcie_clk_enable(imx6_pcie);
+	if (ret) {
+		dev_err(dev, "unable to enable pcie clocks\n");
+		goto err_clks;
+	}
 
 	/* Some boards don't have PCIe reset GPIO. */
 	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
@@ -601,13 +624,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 
 	return;
 
-err_ref_clk:
-	clk_disable_unprepare(imx6_pcie->pcie);
-err_pcie:
-	clk_disable_unprepare(imx6_pcie->pcie_bus);
-err_pcie_bus:
-	clk_disable_unprepare(imx6_pcie->pcie_phy);
-err_pcie_phy:
+err_clks:
 	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
 		ret = regulator_disable(imx6_pcie->vpcie);
 		if (ret)
-- 
2.25.1


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

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

[Richard Zhu]

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

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 5fd33e4536af..5582ee1b420a 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -481,38 +481,16 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
 	return ret;
 }
 
-static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
-{
-	u32 val;
-	struct device *dev = imx6_pcie->pci->dev;
-
-	if (regmap_read_poll_timeout(imx6_pcie->iomuxc_gpr,
-				     IOMUXC_GPR22, val,
-				     val & IMX7D_GPR22_PCIE_PHY_PLL_LOCKED,
-				     PHY_PLL_LOCK_WAIT_USLEEP_MAX,
-				     PHY_PLL_LOCK_WAIT_TIMEOUT))
-		dev_err(dev, "PCIe PLL lock timeout\n");
-}
-
-static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
+static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
 {
 	struct dw_pcie *pci = imx6_pcie->pci;
 	struct device *dev = pci->dev;
 	int ret;
 
-	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
-		ret = regulator_enable(imx6_pcie->vpcie);
-		if (ret) {
-			dev_err(dev, "failed to enable vpcie regulator: %d\n",
-				ret);
-			return;
-		}
-	}
-
 	ret = clk_prepare_enable(imx6_pcie->pcie_phy);
 	if (ret) {
 		dev_err(dev, "unable to enable pcie_phy clock\n");
-		goto err_pcie_phy;
+		return ret;
 	}
 
 	ret = clk_prepare_enable(imx6_pcie->pcie_bus);
@@ -543,6 +521,51 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	}
 	/* allow the clocks to stabilize */
 	usleep_range(200, 500);
+	return 0;
+
+err_ref_clk:
+	clk_disable_unprepare(imx6_pcie->pcie);
+err_pcie:
+	clk_disable_unprepare(imx6_pcie->pcie_bus);
+err_pcie_bus:
+	clk_disable_unprepare(imx6_pcie->pcie_phy);
+
+	return ret;
+}
+
+static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
+{
+	u32 val;
+	struct device *dev = imx6_pcie->pci->dev;
+
+	if (regmap_read_poll_timeout(imx6_pcie->iomuxc_gpr,
+				     IOMUXC_GPR22, val,
+				     val & IMX7D_GPR22_PCIE_PHY_PLL_LOCKED,
+				     PHY_PLL_LOCK_WAIT_USLEEP_MAX,
+				     PHY_PLL_LOCK_WAIT_TIMEOUT))
+		dev_err(dev, "PCIe PLL lock timeout\n");
+}
+
+static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
+{
+	struct dw_pcie *pci = imx6_pcie->pci;
+	struct device *dev = pci->dev;
+	int ret;
+
+	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
+		ret = regulator_enable(imx6_pcie->vpcie);
+		if (ret) {
+			dev_err(dev, "failed to enable vpcie regulator: %d\n",
+				ret);
+			return;
+		}
+	}
+
+	ret = imx6_pcie_clk_enable(imx6_pcie);
+	if (ret) {
+		dev_err(dev, "unable to enable pcie clocks\n");
+		goto err_clks;
+	}
 
 	/* Some boards don't have PCIe reset GPIO. */
 	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
@@ -601,13 +624,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 
 	return;
 
-err_ref_clk:
-	clk_disable_unprepare(imx6_pcie->pcie);
-err_pcie:
-	clk_disable_unprepare(imx6_pcie->pcie_bus);
-err_pcie_bus:
-	clk_disable_unprepare(imx6_pcie->pcie_phy);
-err_pcie_phy:
+err_clks:
 	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
 		ret = regulator_disable(imx6_pcie->vpcie);
 		if (ret)
-- 
2.25.1

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

[Richard Zhu]

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

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 5582ee1b420a..3ca2eef39617 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -546,24 +546,24 @@ static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
 		dev_err(dev, "PCIe PLL lock timeout\n");
 }
 
-static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
+static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 {
 	struct dw_pcie *pci = imx6_pcie->pci;
 	struct device *dev = pci->dev;
-	int ret;
+	int ret, err;
 
 	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
 		ret = regulator_enable(imx6_pcie->vpcie);
 		if (ret) {
 			dev_err(dev, "failed to enable vpcie regulator: %d\n",
 				ret);
-			return;
+			return ret;
 		}
 	}
 
-	ret = imx6_pcie_clk_enable(imx6_pcie);
-	if (ret) {
-		dev_err(dev, "unable to enable pcie clocks\n");
+	err = imx6_pcie_clk_enable(imx6_pcie);
+	if (err) {
+		dev_err(dev, "unable to enable pcie clocks: %d\n", err);
 		goto err_clks;
 	}
 
@@ -622,7 +622,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 		break;
 	}
 
-	return;
+	return 0;
 
 err_clks:
 	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
@@ -631,6 +631,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 			dev_err(dev, "failed to disable vpcie regulator: %d\n",
 				ret);
 	}
+	return err;
 }
 
 static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
@@ -882,11 +883,18 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 static int imx6_pcie_host_init(struct pcie_port *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct device *dev = pci->dev;
 	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
+	int ret;
 
 	imx6_pcie_assert_core_reset(imx6_pcie);
 	imx6_pcie_init_phy(imx6_pcie);
-	imx6_pcie_deassert_core_reset(imx6_pcie);
+	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
+	if (ret < 0) {
+		dev_err(dev, "pcie host init failed: %d.\n", ret);
+		return ret;
+	}
+
 	imx6_setup_phy_mpll(imx6_pcie);
 
 	return 0;
-- 
2.25.1


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

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

[Richard Zhu]

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

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 5582ee1b420a..3ca2eef39617 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -546,24 +546,24 @@ static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
 		dev_err(dev, "PCIe PLL lock timeout\n");
 }
 
-static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
+static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 {
 	struct dw_pcie *pci = imx6_pcie->pci;
 	struct device *dev = pci->dev;
-	int ret;
+	int ret, err;
 
 	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
 		ret = regulator_enable(imx6_pcie->vpcie);
 		if (ret) {
 			dev_err(dev, "failed to enable vpcie regulator: %d\n",
 				ret);
-			return;
+			return ret;
 		}
 	}
 
-	ret = imx6_pcie_clk_enable(imx6_pcie);
-	if (ret) {
-		dev_err(dev, "unable to enable pcie clocks\n");
+	err = imx6_pcie_clk_enable(imx6_pcie);
+	if (err) {
+		dev_err(dev, "unable to enable pcie clocks: %d\n", err);
 		goto err_clks;
 	}
 
@@ -622,7 +622,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 		break;
 	}
 
-	return;
+	return 0;
 
 err_clks:
 	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
@@ -631,6 +631,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 			dev_err(dev, "failed to disable vpcie regulator: %d\n",
 				ret);
 	}
+	return err;
 }
 
 static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
@@ -882,11 +883,18 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 static int imx6_pcie_host_init(struct pcie_port *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct device *dev = pci->dev;
 	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
+	int ret;
 
 	imx6_pcie_assert_core_reset(imx6_pcie);
 	imx6_pcie_init_phy(imx6_pcie);
-	imx6_pcie_deassert_core_reset(imx6_pcie);
+	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
+	if (ret < 0) {
+		dev_err(dev, "pcie host init failed: %d.\n", ret);
+		return ret;
+	}
+
 	imx6_setup_phy_mpll(imx6_pcie);
 
 	return 0;
-- 
2.25.1

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

[Richard Zhu]

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

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 3ca2eef39617..e157f0bec37e 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -533,6 +533,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;
@@ -965,30 +988,6 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
 	usleep_range(1000, 10000);
 }
 
-static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
-{
-	clk_disable_unprepare(imx6_pcie->pcie);
-	clk_disable_unprepare(imx6_pcie->pcie_phy);
-	clk_disable_unprepare(imx6_pcie->pcie_bus);
-
-	switch (imx6_pcie->drvdata->variant) {
-	case IMX6SX:
-		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
-		break;
-	case IMX7D:
-		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
-				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
-				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
-		break;
-	case IMX8MQ:
-	case IMX8MM:
-		clk_disable_unprepare(imx6_pcie->pcie_aux);
-		break;
-	default:
-		break;
-	}
-}
-
 static int imx6_pcie_suspend_noirq(struct device *dev)
 {
 	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
-- 
2.25.1


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

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

[Richard Zhu]

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

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 3ca2eef39617..e157f0bec37e 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -533,6 +533,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;
@@ -965,30 +988,6 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
 	usleep_range(1000, 10000);
 }
 
-static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
-{
-	clk_disable_unprepare(imx6_pcie->pcie);
-	clk_disable_unprepare(imx6_pcie->pcie_phy);
-	clk_disable_unprepare(imx6_pcie->pcie_bus);
-
-	switch (imx6_pcie->drvdata->variant) {
-	case IMX6SX:
-		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
-		break;
-	case IMX7D:
-		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
-				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
-				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
-		break;
-	case IMX8MQ:
-	case IMX8MM:
-		clk_disable_unprepare(imx6_pcie->pcie_aux);
-		break;
-	default:
-		break;
-	}
-}
-
 static int imx6_pcie_suspend_noirq(struct device *dev)
 {
 	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
-- 
2.25.1

[PATCH v6 4/8] PCI: imx6: Disable imx6qdl pcie ref clk

[Richard Zhu]

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

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index e157f0bec37e..0aca762d88a3 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -540,6 +540,14 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
 	clk_disable_unprepare(imx6_pcie->pcie_bus);
 
 	switch (imx6_pcie->drvdata->variant) {
+	case IMX6Q:
+	case IMX6QP:
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
+				IMX6Q_GPR1_PCIE_REF_CLK_EN, 0);
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
+				IMX6Q_GPR1_PCIE_TEST_PD,
+				IMX6Q_GPR1_PCIE_TEST_PD);
+		break;
 	case IMX6SX:
 		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
 		break;
-- 
2.25.1


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

[PATCH v6 4/8] PCI: imx6: Disable imx6qdl pcie ref clk

[Richard Zhu]

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

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index e157f0bec37e..0aca762d88a3 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -540,6 +540,14 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
 	clk_disable_unprepare(imx6_pcie->pcie_bus);
 
 	switch (imx6_pcie->drvdata->variant) {
+	case IMX6Q:
+	case IMX6QP:
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
+				IMX6Q_GPR1_PCIE_REF_CLK_EN, 0);
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
+				IMX6Q_GPR1_PCIE_TEST_PD,
+				IMX6Q_GPR1_PCIE_TEST_PD);
+		break;
 	case IMX6SX:
 		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
 		break;
-- 
2.25.1

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

[Richard Zhu]

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

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 0aca762d88a3..e165ad00989c 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -369,8 +369,6 @@ static int imx6_pcie_attach_pd(struct device *dev)
 
 static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 {
-	struct device *dev = imx6_pcie->pci->dev;
-
 	switch (imx6_pcie->drvdata->variant) {
 	case IMX7D:
 	case IMX8MQ:
@@ -400,14 +398,6 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 				   IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
 		break;
 	}
-
-	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
-		int ret = regulator_disable(imx6_pcie->vpcie);
-
-		if (ret)
-			dev_err(dev, "failed to disable vpcie regulator: %d\n",
-				ret);
-	}
 }
 
 static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
@@ -583,7 +573,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	struct device *dev = pci->dev;
 	int ret, err;
 
-	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
+	if (imx6_pcie->vpcie) {
 		ret = regulator_enable(imx6_pcie->vpcie);
 		if (ret) {
 			dev_err(dev, "failed to enable vpcie regulator: %d\n",
@@ -656,7 +646,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	return 0;
 
 err_clks:
-	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
+	if (imx6_pcie->vpcie) {
 		ret = regulator_disable(imx6_pcie->vpcie);
 		if (ret)
 			dev_err(dev, "failed to disable vpcie regulator: %d\n",
-- 
2.25.1


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

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

[Richard Zhu]

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

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 0aca762d88a3..e165ad00989c 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -369,8 +369,6 @@ static int imx6_pcie_attach_pd(struct device *dev)
 
 static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 {
-	struct device *dev = imx6_pcie->pci->dev;
-
 	switch (imx6_pcie->drvdata->variant) {
 	case IMX7D:
 	case IMX8MQ:
@@ -400,14 +398,6 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 				   IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
 		break;
 	}
-
-	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
-		int ret = regulator_disable(imx6_pcie->vpcie);
-
-		if (ret)
-			dev_err(dev, "failed to disable vpcie regulator: %d\n",
-				ret);
-	}
 }
 
 static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
@@ -583,7 +573,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	struct device *dev = pci->dev;
 	int ret, err;
 
-	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
+	if (imx6_pcie->vpcie) {
 		ret = regulator_enable(imx6_pcie->vpcie);
 		if (ret) {
 			dev_err(dev, "failed to enable vpcie regulator: %d\n",
@@ -656,7 +646,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	return 0;
 
 err_clks:
-	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
+	if (imx6_pcie->vpcie) {
 		ret = regulator_disable(imx6_pcie->vpcie);
 		if (ret)
 			dev_err(dev, "failed to disable vpcie regulator: %d\n",
-- 
2.25.1

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

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


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

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

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

[PATCH v6 7/8] PCI: imx6: Disable enabled clocks and regulators after link is down

[Richard Zhu]

Since i.MX PCIe doesn't support the hot-plug, and to save power
consumption as much as possible. Return error and disable the enabled
clocks and regulators when link is down,.

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

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index e165ad00989c..7a7d9204c6bc 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -848,7 +848,9 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 	/* Start LTSSM. */
 	imx6_pcie_ltssm_enable(dev);
 
-	dw_pcie_wait_for_link(pci);
+	ret = dw_pcie_wait_for_link(pci);
+	if (ret)
+		goto err_reset_phy;
 
 	if (pci->link_gen == 2) {
 		/* Allow Gen2 mode after the link is up. */
@@ -884,7 +886,9 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 		}
 
 		/* Make sure link training is finished as well! */
-		dw_pcie_wait_for_link(pci);
+		ret = dw_pcie_wait_for_link(pci);
+		if (ret)
+			goto err_reset_phy;
 	} else {
 		dev_info(dev, "Link: Gen2 disabled\n");
 	}
@@ -897,7 +901,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;
 }
 
@@ -921,8 +924,29 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
 	return 0;
 }
 
+static void imx6_pcie_host_exit(struct pcie_port *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct device *dev = pci->dev;
+	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
+
+	imx6_pcie_reset_phy(imx6_pcie);
+	imx6_pcie_clk_disable(imx6_pcie);
+	switch (imx6_pcie->drvdata->variant) {
+	case IMX8MM:
+		if (phy_power_off(imx6_pcie->phy))
+			dev_err(dev, "unable to power off phy\n");
+		break;
+	default:
+		break;
+	}
+	if (imx6_pcie->vpcie)
+		regulator_disable(imx6_pcie->vpcie);
+}
+
 static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
 	.host_init = imx6_pcie_host_init,
+	.host_exit = imx6_pcie_host_exit,
 };
 
 static const struct dw_pcie_ops dw_pcie_ops = {
-- 
2.25.1


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

[PATCH v6 7/8] PCI: imx6: Disable enabled clocks and regulators after link is down

[Richard Zhu]

Since i.MX PCIe doesn't support the hot-plug, and to save power
consumption as much as possible. Return error and disable the enabled
clocks and regulators when link is down,.

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

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index e165ad00989c..7a7d9204c6bc 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -848,7 +848,9 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 	/* Start LTSSM. */
 	imx6_pcie_ltssm_enable(dev);
 
-	dw_pcie_wait_for_link(pci);
+	ret = dw_pcie_wait_for_link(pci);
+	if (ret)
+		goto err_reset_phy;
 
 	if (pci->link_gen == 2) {
 		/* Allow Gen2 mode after the link is up. */
@@ -884,7 +886,9 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 		}
 
 		/* Make sure link training is finished as well! */
-		dw_pcie_wait_for_link(pci);
+		ret = dw_pcie_wait_for_link(pci);
+		if (ret)
+			goto err_reset_phy;
 	} else {
 		dev_info(dev, "Link: Gen2 disabled\n");
 	}
@@ -897,7 +901,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;
 }
 
@@ -921,8 +924,29 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
 	return 0;
 }
 
+static void imx6_pcie_host_exit(struct pcie_port *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct device *dev = pci->dev;
+	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
+
+	imx6_pcie_reset_phy(imx6_pcie);
+	imx6_pcie_clk_disable(imx6_pcie);
+	switch (imx6_pcie->drvdata->variant) {
+	case IMX8MM:
+		if (phy_power_off(imx6_pcie->phy))
+			dev_err(dev, "unable to power off phy\n");
+		break;
+	default:
+		break;
+	}
+	if (imx6_pcie->vpcie)
+		regulator_disable(imx6_pcie->vpcie);
+}
+
 static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
 	.host_init = imx6_pcie_host_init,
+	.host_exit = imx6_pcie_host_exit,
 };
 
 static const struct dw_pcie_ops dw_pcie_ops = {
-- 
2.25.1

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 7a7d9204c6bc..62262483470a 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -146,6 +146,10 @@ struct imx6_pcie {
 #define PHY_RX_OVRD_IN_LO_RX_DATA_EN		BIT(5)
 #define PHY_RX_OVRD_IN_LO_RX_PLL_EN		BIT(3)
 
+static bool imx6_pcie_cmp_mode;
+module_param_named(compliance, imx6_pcie_cmp_mode, bool, 0644);
+MODULE_PARM_DESC(compliance, "i.MX PCIe compliance test mode (1=compliance test mode enabled)");
+
 static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
 {
 	struct dw_pcie *pci = imx6_pcie->pci;
@@ -840,10 +844,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);
@@ -930,18 +936,20 @@ static void imx6_pcie_host_exit(struct pcie_port *pp)
 	struct device *dev = pci->dev;
 	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
 
-	imx6_pcie_reset_phy(imx6_pcie);
-	imx6_pcie_clk_disable(imx6_pcie);
-	switch (imx6_pcie->drvdata->variant) {
-	case IMX8MM:
-		if (phy_power_off(imx6_pcie->phy))
-			dev_err(dev, "unable to power off phy\n");
-		break;
-	default:
-		break;
+	if (!imx6_pcie_cmp_mode) {
+		imx6_pcie_reset_phy(imx6_pcie);
+		imx6_pcie_clk_disable(imx6_pcie);
+		switch (imx6_pcie->drvdata->variant) {
+		case IMX8MM:
+			if (phy_power_off(imx6_pcie->phy))
+				dev_err(dev, "unable to power off phy\n");
+			break;
+		default:
+			break;
+		}
+		if (imx6_pcie->vpcie)
+			regulator_disable(imx6_pcie->vpcie);
 	}
-	if (imx6_pcie->vpcie)
-		regulator_disable(imx6_pcie->vpcie);
 }
 
 static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
@@ -1256,8 +1264,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

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 7a7d9204c6bc..62262483470a 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -146,6 +146,10 @@ struct imx6_pcie {
 #define PHY_RX_OVRD_IN_LO_RX_DATA_EN		BIT(5)
 #define PHY_RX_OVRD_IN_LO_RX_PLL_EN		BIT(3)
 
+static bool imx6_pcie_cmp_mode;
+module_param_named(compliance, imx6_pcie_cmp_mode, bool, 0644);
+MODULE_PARM_DESC(compliance, "i.MX PCIe compliance test mode (1=compliance test mode enabled)");
+
 static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
 {
 	struct dw_pcie *pci = imx6_pcie->pci;
@@ -840,10 +844,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);
@@ -930,18 +936,20 @@ static void imx6_pcie_host_exit(struct pcie_port *pp)
 	struct device *dev = pci->dev;
 	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
 
-	imx6_pcie_reset_phy(imx6_pcie);
-	imx6_pcie_clk_disable(imx6_pcie);
-	switch (imx6_pcie->drvdata->variant) {
-	case IMX8MM:
-		if (phy_power_off(imx6_pcie->phy))
-			dev_err(dev, "unable to power off phy\n");
-		break;
-	default:
-		break;
+	if (!imx6_pcie_cmp_mode) {
+		imx6_pcie_reset_phy(imx6_pcie);
+		imx6_pcie_clk_disable(imx6_pcie);
+		switch (imx6_pcie->drvdata->variant) {
+		case IMX8MM:
+			if (phy_power_off(imx6_pcie->phy))
+				dev_err(dev, "unable to power off phy\n");
+			break;
+		default:
+			break;
+		}
+		if (imx6_pcie->vpcie)
+			regulator_disable(imx6_pcie->vpcie);
 	}
-	if (imx6_pcie->vpcie)
-		regulator_disable(imx6_pcie->vpcie);
 }
 
 static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
@@ -1256,8 +1264,15 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 		return ret;
 
 	ret = dw_pcie_host_init(&pci->pp);
-	if (ret < 0)
+	if (ret < 0) {
+		if (imx6_pcie_cmp_mode) {
+			dev_info(dev, "Driver loaded with compliance test mode enabled.\n");
+			ret = 0;
+		} else {
+			dev_err(dev, "Unable to add pcie port.\n");
+		}
 		return ret;
+	}
 
 	if (pci_msi_enabled()) {
 		u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
-- 
2.25.1

Re: [PATCH v6 7/8] PCI: imx6: Disable enabled clocks and regulators after link is down

[Fabio Estevam]

Hi Richard,

On Tue, Feb 8, 2022 at 12:57 AM Richard Zhu <hongxing.zhu@nxp.com> wrote:
>
> Since i.MX PCIe doesn't support the hot-plug, and to save power
> consumption as much as possible. Return error and disable the enabled
> clocks and regulators when link is down,.

It is OK to disable clocks and regulators, but I don't think we should
return an error on dw_pcie_wait_for_link() failure.

Please check:

https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/commit/?h=pci/imx6&id=f81f095e87715e198471f4653952fe5e3f824874

and

https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/commit/?h=pci/imx6&id=886a9c134755

as to why all the dwc PCI drivers should treat dw_pcie_wait_for_link()
uniformly.






>
> Add a new host_exit() callback for i.MX PCIe driver to disable the
> enabled clocks, regulators and so on in the error handling after
> host_init is finished.
>
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 30 ++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index e165ad00989c..7a7d9204c6bc 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -848,7 +848,9 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
>         /* Start LTSSM. */
>         imx6_pcie_ltssm_enable(dev);
>
> -       dw_pcie_wait_for_link(pci);
> +       ret = dw_pcie_wait_for_link(pci);
> +       if (ret)
> +               goto err_reset_phy;
>
>         if (pci->link_gen == 2) {
>                 /* Allow Gen2 mode after the link is up. */
> @@ -884,7 +886,9 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
>                 }
>
>                 /* Make sure link training is finished as well! */
> -               dw_pcie_wait_for_link(pci);
> +               ret = dw_pcie_wait_for_link(pci);
> +               if (ret)
> +                       goto err_reset_phy;
>         } else {
>                 dev_info(dev, "Link: Gen2 disabled\n");
>         }
> @@ -897,7 +901,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;
>  }
>
> @@ -921,8 +924,29 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
>         return 0;
>  }
>
> +static void imx6_pcie_host_exit(struct pcie_port *pp)
> +{
> +       struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +       struct device *dev = pci->dev;
> +       struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
> +
> +       imx6_pcie_reset_phy(imx6_pcie);
> +       imx6_pcie_clk_disable(imx6_pcie);
> +       switch (imx6_pcie->drvdata->variant) {
> +       case IMX8MM:
> +               if (phy_power_off(imx6_pcie->phy))
> +                       dev_err(dev, "unable to power off phy\n");
> +               break;
> +       default:
> +               break;
> +       }
> +       if (imx6_pcie->vpcie)
> +               regulator_disable(imx6_pcie->vpcie);
> +}
> +
>  static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
>         .host_init = imx6_pcie_host_init,
> +       .host_exit = imx6_pcie_host_exit,
>  };
>
>  static const struct dw_pcie_ops dw_pcie_ops = {
> --
> 2.25.1
>

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

Re: [PATCH v6 7/8] PCI: imx6: Disable enabled clocks and regulators after link is down

[Fabio Estevam]

Hi Richard,

On Tue, Feb 8, 2022 at 12:57 AM Richard Zhu <hongxing.zhu@nxp.com> wrote:
>
> Since i.MX PCIe doesn't support the hot-plug, and to save power
> consumption as much as possible. Return error and disable the enabled
> clocks and regulators when link is down,.

It is OK to disable clocks and regulators, but I don't think we should
return an error on dw_pcie_wait_for_link() failure.

Please check:

https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/commit/?h=pci/imx6&id=f81f095e87715e198471f4653952fe5e3f824874

and

https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/commit/?h=pci/imx6&id=886a9c134755

as to why all the dwc PCI drivers should treat dw_pcie_wait_for_link()
uniformly.






>
> Add a new host_exit() callback for i.MX PCIe driver to disable the
> enabled clocks, regulators and so on in the error handling after
> host_init is finished.
>
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 30 ++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index e165ad00989c..7a7d9204c6bc 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -848,7 +848,9 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
>         /* Start LTSSM. */
>         imx6_pcie_ltssm_enable(dev);
>
> -       dw_pcie_wait_for_link(pci);
> +       ret = dw_pcie_wait_for_link(pci);
> +       if (ret)
> +               goto err_reset_phy;
>
>         if (pci->link_gen == 2) {
>                 /* Allow Gen2 mode after the link is up. */
> @@ -884,7 +886,9 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
>                 }
>
>                 /* Make sure link training is finished as well! */
> -               dw_pcie_wait_for_link(pci);
> +               ret = dw_pcie_wait_for_link(pci);
> +               if (ret)
> +                       goto err_reset_phy;
>         } else {
>                 dev_info(dev, "Link: Gen2 disabled\n");
>         }
> @@ -897,7 +901,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;
>  }
>
> @@ -921,8 +924,29 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
>         return 0;
>  }
>
> +static void imx6_pcie_host_exit(struct pcie_port *pp)
> +{
> +       struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +       struct device *dev = pci->dev;
> +       struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
> +
> +       imx6_pcie_reset_phy(imx6_pcie);
> +       imx6_pcie_clk_disable(imx6_pcie);
> +       switch (imx6_pcie->drvdata->variant) {
> +       case IMX8MM:
> +               if (phy_power_off(imx6_pcie->phy))
> +                       dev_err(dev, "unable to power off phy\n");
> +               break;
> +       default:
> +               break;
> +       }
> +       if (imx6_pcie->vpcie)
> +               regulator_disable(imx6_pcie->vpcie);
> +}
> +
>  static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
>         .host_init = imx6_pcie_host_init,
> +       .host_exit = imx6_pcie_host_exit,
>  };
>
>  static const struct dw_pcie_ops dw_pcie_ops = {
> --
> 2.25.1
>

RE: [PATCH v6 7/8] PCI: imx6: Disable enabled clocks and regulators after link is down

[Hongxing Zhu]

> -----Original Message-----
> From: Fabio Estevam <festevam@gmail.com>
> Sent: 2022年2月8日 18:10
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>; Bjorn Helgaas
> <bhelgaas@google.com>; Mark Brown <broonie@kernel.org>; Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com>; Jingoo Han <jingoohan1@gmail.com>;
> linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; moderated
> list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> <linux-arm-kernel@lists.infradead.org>; linux-kernel
> <linux-kernel@vger.kernel.org>; Sascha Hauer <kernel@pengutronix.de>
> Subject: Re: [PATCH v6 7/8] PCI: imx6: Disable enabled clocks and regulators
> after link is down
> 
> Hi Richard,
> 
> On Tue, Feb 8, 2022 at 12:57 AM Richard Zhu <hongxing.zhu@nxp.com>
> wrote:
> >
> > Since i.MX PCIe doesn't support the hot-plug, and to save power
> > consumption as much as possible. Return error and disable the enabled
> > clocks and regulators when link is down,.
> 
> It is OK to disable clocks and regulators, but I don't think we should return an
> error on dw_pcie_wait_for_link() failure.
Hi Fabio:
Thanks for your review.
How do we know there is a link down, if we don't return an error on the
 dw_pcie_wait_for_link() after host_init callback.

Do you mean that we should ignore the return of dw_pcie_host_init() and
finish the _probe without an error return?
Best Regards
Richard
> 
> Please check:
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern
> el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Flpieralisi%2Fpci.git%2Fco
> mmit%2F%3Fh%3Dpci%2Fimx6%26id%3Df81f095e87715e198471f4653952fe
> 5e3f824874&amp;data=04%7C01%7Chongxing.zhu%40nxp.com%7C3630184
> 5833b41f1f70a08d9eaeb2519%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
> 0%7C0%7C637799117928160088%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C300
> 0&amp;sdata=UUF%2FbZNUy7T4d89sJKnPvN6kNxEB%2FhnSUbIze7ilUOA%3D
> &amp;reserved=0
> 
> and
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern
> el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Flpieralisi%2Fpci.git%2Fco
> mmit%2F%3Fh%3Dpci%2Fimx6%26id%3D886a9c134755&amp;data=04%7C0
> 1%7Chongxing.zhu%40nxp.com%7C36301845833b41f1f70a08d9eaeb2519%7
> C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63779911792816008
> 8%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiL
> CJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=FO0wr4IE%2BA6YCu
> f0Lal2az8FRkK8ZvHkmLM%2F5zlosZY%3D&amp;reserved=0
> 
> as to why all the dwc PCI drivers should treat dw_pcie_wait_for_link()
> uniformly.
> 
> 
> 
> 
> 
> 
> >
> > Add a new host_exit() callback for i.MX PCIe driver to disable the
> > enabled clocks, regulators and so on in the error handling after
> > host_init is finished.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 30
> > ++++++++++++++++++++++++---
> >  1 file changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index e165ad00989c..7a7d9204c6bc 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -848,7 +848,9 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
> >         /* Start LTSSM. */
> >         imx6_pcie_ltssm_enable(dev);
> >
> > -       dw_pcie_wait_for_link(pci);
> > +       ret = dw_pcie_wait_for_link(pci);
> > +       if (ret)
> > +               goto err_reset_phy;
> >
> >         if (pci->link_gen == 2) {
> >                 /* Allow Gen2 mode after the link is up. */ @@ -884,7
> > +886,9 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
> >                 }
> >
> >                 /* Make sure link training is finished as well! */
> > -               dw_pcie_wait_for_link(pci);
> > +               ret = dw_pcie_wait_for_link(pci);
> > +               if (ret)
> > +                       goto err_reset_phy;
> >         } else {
> >                 dev_info(dev, "Link: Gen2 disabled\n");
> >         }
> > @@ -897,7 +901,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;
> >  }
> >
> > @@ -921,8 +924,29 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
> >         return 0;
> >  }
> >
> > +static void imx6_pcie_host_exit(struct pcie_port *pp) {
> > +       struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +       struct device *dev = pci->dev;
> > +       struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
> > +
> > +       imx6_pcie_reset_phy(imx6_pcie);
> > +       imx6_pcie_clk_disable(imx6_pcie);
> > +       switch (imx6_pcie->drvdata->variant) {
> > +       case IMX8MM:
> > +               if (phy_power_off(imx6_pcie->phy))
> > +                       dev_err(dev, "unable to power off phy\n");
> > +               break;
> > +       default:
> > +               break;
> > +       }
> > +       if (imx6_pcie->vpcie)
> > +               regulator_disable(imx6_pcie->vpcie);
> > +}
> > +
> >  static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
> >         .host_init = imx6_pcie_host_init,
> > +       .host_exit = imx6_pcie_host_exit,
> >  };
> >
> >  static const struct dw_pcie_ops dw_pcie_ops = {
> > --
> > 2.25.1
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v6 7/8] PCI: imx6: Disable enabled clocks and regulators after link is down

[Hongxing Zhu]

> -----Original Message-----
> From: Fabio Estevam <festevam@gmail.com>
> Sent: 2022年2月8日 18:10
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>; Bjorn Helgaas
> <bhelgaas@google.com>; Mark Brown <broonie@kernel.org>; Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com>; Jingoo Han <jingoohan1@gmail.com>;
> linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; moderated
> list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> <linux-arm-kernel@lists.infradead.org>; linux-kernel
> <linux-kernel@vger.kernel.org>; Sascha Hauer <kernel@pengutronix.de>
> Subject: Re: [PATCH v6 7/8] PCI: imx6: Disable enabled clocks and regulators
> after link is down
> 
> Hi Richard,
> 
> On Tue, Feb 8, 2022 at 12:57 AM Richard Zhu <hongxing.zhu@nxp.com>
> wrote:
> >
> > Since i.MX PCIe doesn't support the hot-plug, and to save power
> > consumption as much as possible. Return error and disable the enabled
> > clocks and regulators when link is down,.
> 
> It is OK to disable clocks and regulators, but I don't think we should return an
> error on dw_pcie_wait_for_link() failure.
Hi Fabio:
Thanks for your review.
How do we know there is a link down, if we don't return an error on the
 dw_pcie_wait_for_link() after host_init callback.

Do you mean that we should ignore the return of dw_pcie_host_init() and
finish the _probe without an error return?
Best Regards
Richard
> 
> Please check:
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern
> el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Flpieralisi%2Fpci.git%2Fco
> mmit%2F%3Fh%3Dpci%2Fimx6%26id%3Df81f095e87715e198471f4653952fe
> 5e3f824874&amp;data=04%7C01%7Chongxing.zhu%40nxp.com%7C3630184
> 5833b41f1f70a08d9eaeb2519%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
> 0%7C0%7C637799117928160088%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C300
> 0&amp;sdata=UUF%2FbZNUy7T4d89sJKnPvN6kNxEB%2FhnSUbIze7ilUOA%3D
> &amp;reserved=0
> 
> and
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern
> el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Flpieralisi%2Fpci.git%2Fco
> mmit%2F%3Fh%3Dpci%2Fimx6%26id%3D886a9c134755&amp;data=04%7C0
> 1%7Chongxing.zhu%40nxp.com%7C36301845833b41f1f70a08d9eaeb2519%7
> C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63779911792816008
> 8%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiL
> CJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=FO0wr4IE%2BA6YCu
> f0Lal2az8FRkK8ZvHkmLM%2F5zlosZY%3D&amp;reserved=0
> 
> as to why all the dwc PCI drivers should treat dw_pcie_wait_for_link()
> uniformly.
> 
> 
> 
> 
> 
> 
> >
> > Add a new host_exit() callback for i.MX PCIe driver to disable the
> > enabled clocks, regulators and so on in the error handling after
> > host_init is finished.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 30
> > ++++++++++++++++++++++++---
> >  1 file changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index e165ad00989c..7a7d9204c6bc 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -848,7 +848,9 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
> >         /* Start LTSSM. */
> >         imx6_pcie_ltssm_enable(dev);
> >
> > -       dw_pcie_wait_for_link(pci);
> > +       ret = dw_pcie_wait_for_link(pci);
> > +       if (ret)
> > +               goto err_reset_phy;
> >
> >         if (pci->link_gen == 2) {
> >                 /* Allow Gen2 mode after the link is up. */ @@ -884,7
> > +886,9 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
> >                 }
> >
> >                 /* Make sure link training is finished as well! */
> > -               dw_pcie_wait_for_link(pci);
> > +               ret = dw_pcie_wait_for_link(pci);
> > +               if (ret)
> > +                       goto err_reset_phy;
> >         } else {
> >                 dev_info(dev, "Link: Gen2 disabled\n");
> >         }
> > @@ -897,7 +901,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;
> >  }
> >
> > @@ -921,8 +924,29 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
> >         return 0;
> >  }
> >
> > +static void imx6_pcie_host_exit(struct pcie_port *pp) {
> > +       struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +       struct device *dev = pci->dev;
> > +       struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
> > +
> > +       imx6_pcie_reset_phy(imx6_pcie);
> > +       imx6_pcie_clk_disable(imx6_pcie);
> > +       switch (imx6_pcie->drvdata->variant) {
> > +       case IMX8MM:
> > +               if (phy_power_off(imx6_pcie->phy))
> > +                       dev_err(dev, "unable to power off phy\n");
> > +               break;
> > +       default:
> > +               break;
> > +       }
> > +       if (imx6_pcie->vpcie)
> > +               regulator_disable(imx6_pcie->vpcie);
> > +}
> > +
> >  static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
> >         .host_init = imx6_pcie_host_init,
> > +       .host_exit = imx6_pcie_host_exit,
> >  };
> >
> >  static const struct dw_pcie_ops dw_pcie_ops = {
> > --
> > 2.25.1
> >

Re: [PATCH v6 7/8] PCI: imx6: Disable enabled clocks and regulators after link is down

[Fabio Estevam]

Hi Richard,

On Tue, Feb 8, 2022 at 10:56 PM Hongxing Zhu <hongxing.zhu@nxp.com> wrote:

> Do you mean that we should ignore the return of dw_pcie_host_init() and
> finish the _probe without an error return?

Yes, we should not return an error on probe if the link is down.

Thanks

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

Re: [PATCH v6 7/8] PCI: imx6: Disable enabled clocks and regulators after link is down

[Fabio Estevam]

Hi Richard,

On Tue, Feb 8, 2022 at 10:56 PM Hongxing Zhu <hongxing.zhu@nxp.com> wrote:

> Do you mean that we should ignore the return of dw_pcie_host_init() and
> finish the _probe without an error return?

Yes, we should not return an error on probe if the link is down.

Thanks

RE: [PATCH v6 7/8] PCI: imx6: Disable enabled clocks and regulators after link is down

[Hongxing Zhu]

> -----Original Message-----
> From: Fabio Estevam <festevam@gmail.com>
> Sent: 2022年2月9日 10:02
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>; Bjorn Helgaas
> <bhelgaas@google.com>; Mark Brown <broonie@kernel.org>; Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com>; Jingoo Han <jingoohan1@gmail.com>;
> linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; moderated
> list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> <linux-arm-kernel@lists.infradead.org>; linux-kernel
> <linux-kernel@vger.kernel.org>; Sascha Hauer <kernel@pengutronix.de>
> Subject: Re: [PATCH v6 7/8] PCI: imx6: Disable enabled clocks and regulators
> after link is down
> 
> Hi Richard,
> 
> On Tue, Feb 8, 2022 at 10:56 PM Hongxing Zhu <hongxing.zhu@nxp.com>
> wrote:
> 
> > Do you mean that we should ignore the return of dw_pcie_host_init()
> > and finish the _probe without an error return?
> 
> Yes, we should not return an error on probe if the link is down.
> 
> Thanks
Hi Fabio:
Regarding my understand, the suspend/resume callbacks would be hooked if the
 probe is complete successfully. There would be extra more than 100ms delay
 in every system resume operations.
I'm afraid that it would bring bad experience for the power management.

Is there a policy that we should obey, and let the probe complete without
an error return?
If yes, I can follow your suggestion, wouldn't return an error on probe when
link is down.

Thanks.
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 v6 7/8] PCI: imx6: Disable enabled clocks and regulators after link is down

[Hongxing Zhu]

> -----Original Message-----
> From: Fabio Estevam <festevam@gmail.com>
> Sent: 2022年2月9日 10:02
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>; Bjorn Helgaas
> <bhelgaas@google.com>; Mark Brown <broonie@kernel.org>; Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com>; Jingoo Han <jingoohan1@gmail.com>;
> linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; moderated
> list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> <linux-arm-kernel@lists.infradead.org>; linux-kernel
> <linux-kernel@vger.kernel.org>; Sascha Hauer <kernel@pengutronix.de>
> Subject: Re: [PATCH v6 7/8] PCI: imx6: Disable enabled clocks and regulators
> after link is down
> 
> Hi Richard,
> 
> On Tue, Feb 8, 2022 at 10:56 PM Hongxing Zhu <hongxing.zhu@nxp.com>
> wrote:
> 
> > Do you mean that we should ignore the return of dw_pcie_host_init()
> > and finish the _probe without an error return?
> 
> Yes, we should not return an error on probe if the link is down.
> 
> Thanks
Hi Fabio:
Regarding my understand, the suspend/resume callbacks would be hooked if the
 probe is complete successfully. There would be extra more than 100ms delay
 in every system resume operations.
I'm afraid that it would bring bad experience for the power management.

Is there a policy that we should obey, and let the probe complete without
an error return?
If yes, I can follow your suggestion, wouldn't return an error on probe when
link is down.

Thanks.
Best Regards
Richard Zhu

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

[Francesco Dolcini]

Hello Richard,

On Tue, Feb 08, 2022 at 11:25:32AM +0800, Richard Zhu wrote:
> The driver should undo any enables it did itself. The regulator disable
> shouldn't be basing decisions on regulator_is_enabled().
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 0aca762d88a3..e165ad00989c 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -369,8 +369,6 @@ static int imx6_pcie_attach_pd(struct device *dev)
>  
>  static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
>  {
> -	struct device *dev = imx6_pcie->pci->dev;
> -
>  	switch (imx6_pcie->drvdata->variant) {
>  	case IMX7D:
>  	case IMX8MQ:
> @@ -400,14 +398,6 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
>  				   IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
>  		break;
>  	}
> -
> -	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> -		int ret = regulator_disable(imx6_pcie->vpcie);
> -
> -		if (ret)
> -			dev_err(dev, "failed to disable vpcie regulator: %d\n",
> -				ret);
> -	}

This commit is not just cleaning up the regulator usage as you state in
the commit message, this is removing the vpcie regulator_disable
from imx6_pcie_assert_core_reset().

I would not do it, this is called for example on the shutdown callback
where it makes sense.

Francesco

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

[Francesco Dolcini]

Hello Richard,

On Tue, Feb 08, 2022 at 11:25:32AM +0800, Richard Zhu wrote:
> The driver should undo any enables it did itself. The regulator disable
> shouldn't be basing decisions on regulator_is_enabled().
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 0aca762d88a3..e165ad00989c 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -369,8 +369,6 @@ static int imx6_pcie_attach_pd(struct device *dev)
>  
>  static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
>  {
> -	struct device *dev = imx6_pcie->pci->dev;
> -
>  	switch (imx6_pcie->drvdata->variant) {
>  	case IMX7D:
>  	case IMX8MQ:
> @@ -400,14 +398,6 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
>  				   IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
>  		break;
>  	}
> -
> -	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> -		int ret = regulator_disable(imx6_pcie->vpcie);
> -
> -		if (ret)
> -			dev_err(dev, "failed to disable vpcie regulator: %d\n",
> -				ret);
> -	}

This commit is not just cleaning up the regulator usage as you state in
the commit message, this is removing the vpcie regulator_disable
from imx6_pcie_assert_core_reset().

I would not do it, this is called for example on the shutdown callback
where it makes sense.

Francesco


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

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

[Francesco Dolcini]

Hello Richard,

On Tue, Feb 08, 2022 at 11:25:30AM +0800, Richard Zhu wrote:
> Just move the imx6_pcie_clk_disable() to an earlier place without function
> changes, since it wouldn't be only used in imx6_pcie_suspend_noirq() later.

In which patch are you going to use imx6_pcie_clk_disable()? I could not
see it in this patch series.

Francesco

> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 47 +++++++++++++--------------
>  1 file changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 3ca2eef39617..e157f0bec37e 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -533,6 +533,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;
> @@ -965,30 +988,6 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
>  	usleep_range(1000, 10000);
>  }
>  
> -static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
> -{
> -	clk_disable_unprepare(imx6_pcie->pcie);
> -	clk_disable_unprepare(imx6_pcie->pcie_phy);
> -	clk_disable_unprepare(imx6_pcie->pcie_bus);
> -
> -	switch (imx6_pcie->drvdata->variant) {
> -	case IMX6SX:
> -		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> -		break;
> -	case IMX7D:
> -		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> -				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> -				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> -		break;
> -	case IMX8MQ:
> -	case IMX8MM:
> -		clk_disable_unprepare(imx6_pcie->pcie_aux);
> -		break;
> -	default:
> -		break;
> -	}
> -}
> -
>  static int imx6_pcie_suspend_noirq(struct device *dev)
>  {
>  	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> -- 
> 2.25.1
> 

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

[Francesco Dolcini]

Hello Richard,

On Tue, Feb 08, 2022 at 11:25:30AM +0800, Richard Zhu wrote:
> Just move the imx6_pcie_clk_disable() to an earlier place without function
> changes, since it wouldn't be only used in imx6_pcie_suspend_noirq() later.

In which patch are you going to use imx6_pcie_clk_disable()? I could not
see it in this patch series.

Francesco

> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 47 +++++++++++++--------------
>  1 file changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 3ca2eef39617..e157f0bec37e 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -533,6 +533,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;
> @@ -965,30 +988,6 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
>  	usleep_range(1000, 10000);
>  }
>  
> -static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
> -{
> -	clk_disable_unprepare(imx6_pcie->pcie);
> -	clk_disable_unprepare(imx6_pcie->pcie_phy);
> -	clk_disable_unprepare(imx6_pcie->pcie_bus);
> -
> -	switch (imx6_pcie->drvdata->variant) {
> -	case IMX6SX:
> -		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> -		break;
> -	case IMX7D:
> -		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> -				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> -				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> -		break;
> -	case IMX8MQ:
> -	case IMX8MM:
> -		clk_disable_unprepare(imx6_pcie->pcie_aux);
> -		break;
> -	default:
> -		break;
> -	}
> -}
> -
>  static int imx6_pcie_suspend_noirq(struct device *dev)
>  {
>  	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> -- 
> 2.25.1
> 

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

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

[Hongxing Zhu]

> -----Original Message-----
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> Sent: 2022年2月12日 0:28
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; broonie@kernel.org;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.com; festevam@gmail.com;
> 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 v6 5/8] PCI: imx6: Refine the regulator usage
> 
> Hello Richard,
> 
> On Tue, Feb 08, 2022 at 11:25:32AM +0800, Richard Zhu wrote:
> > The driver should undo any enables it did itself. The regulator
> > disable shouldn't be basing decisions on regulator_is_enabled().
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 14 ++------------
> >  1 file changed, 2 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 0aca762d88a3..e165ad00989c 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -369,8 +369,6 @@ static int imx6_pcie_attach_pd(struct device *dev)
> >
> >  static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> > {
> > -	struct device *dev = imx6_pcie->pci->dev;
> > -
> >  	switch (imx6_pcie->drvdata->variant) {
> >  	case IMX7D:
> >  	case IMX8MQ:
> > @@ -400,14 +398,6 @@ static void imx6_pcie_assert_core_reset(struct
> imx6_pcie *imx6_pcie)
> >  				   IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
> >  		break;
> >  	}
> > -
> > -	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> > -		int ret = regulator_disable(imx6_pcie->vpcie);
> > -
> > -		if (ret)
> > -			dev_err(dev, "failed to disable vpcie regulator: %d\n",
> > -				ret);
> > -	}
> 
> This commit is not just cleaning up the regulator usage as you state in the
> commit message, this is removing the vpcie regulator_disable from
> imx6_pcie_assert_core_reset().
> 
> I would not do it, this is called for example on the shutdown callback where it
> makes sense.
Hi Francesco:
Thanks for your review.
Do you means that we should keep regulator_disable() here?
Okay, I would change it later.

Best Regards
Richard Zhu

> 
> Francesco

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

[Hongxing Zhu]

> -----Original Message-----
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> Sent: 2022年2月12日 0:28
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; broonie@kernel.org;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.com; festevam@gmail.com;
> 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 v6 5/8] PCI: imx6: Refine the regulator usage
> 
> Hello Richard,
> 
> On Tue, Feb 08, 2022 at 11:25:32AM +0800, Richard Zhu wrote:
> > The driver should undo any enables it did itself. The regulator
> > disable shouldn't be basing decisions on regulator_is_enabled().
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 14 ++------------
> >  1 file changed, 2 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 0aca762d88a3..e165ad00989c 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -369,8 +369,6 @@ static int imx6_pcie_attach_pd(struct device *dev)
> >
> >  static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> > {
> > -	struct device *dev = imx6_pcie->pci->dev;
> > -
> >  	switch (imx6_pcie->drvdata->variant) {
> >  	case IMX7D:
> >  	case IMX8MQ:
> > @@ -400,14 +398,6 @@ static void imx6_pcie_assert_core_reset(struct
> imx6_pcie *imx6_pcie)
> >  				   IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
> >  		break;
> >  	}
> > -
> > -	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> > -		int ret = regulator_disable(imx6_pcie->vpcie);
> > -
> > -		if (ret)
> > -			dev_err(dev, "failed to disable vpcie regulator: %d\n",
> > -				ret);
> > -	}
> 
> This commit is not just cleaning up the regulator usage as you state in the
> commit message, this is removing the vpcie regulator_disable from
> imx6_pcie_assert_core_reset().
> 
> I would not do it, this is called for example on the shutdown callback where it
> makes sense.
Hi Francesco:
Thanks for your review.
Do you means that we should keep regulator_disable() here?
Okay, I would change it later.

Best Regards
Richard Zhu

> 
> Francesco

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

RE: [PATCH v6 3/8] PCI: imx6: Move imx6_pcie_clk_disable() earlier

[Hongxing Zhu]

> -----Original Message-----
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> Sent: 2022年2月12日 0:30
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; broonie@kernel.org;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.com; festevam@gmail.com;
> 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 v6 3/8] PCI: imx6: Move imx6_pcie_clk_disable() earlier
> 
> Hello Richard,
> 
> On Tue, Feb 08, 2022 at 11:25:30AM +0800, Richard Zhu wrote:
> > Just move the imx6_pcie_clk_disable() to an earlier place without
> > function changes, since it wouldn't be only used in
> imx6_pcie_suspend_noirq() later.
> 
> In which patch are you going to use imx6_pcie_clk_disable()? I could not see it
> in this patch series.
Hi Francesco:
Thanks for your review.
The imx6_pcie_clk_disable() would be invoked in imx6_pcie_host_exit() in #7
 patch later.

Best Regards
Richard Zhu

> 
> Francesco
> 
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 47
> > +++++++++++++--------------
> >  1 file changed, 23 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 3ca2eef39617..e157f0bec37e 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -533,6 +533,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;
> > @@ -965,30 +988,6 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie
> *imx6_pcie)
> >  	usleep_range(1000, 10000);
> >  }
> >
> > -static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie) -{
> > -	clk_disable_unprepare(imx6_pcie->pcie);
> > -	clk_disable_unprepare(imx6_pcie->pcie_phy);
> > -	clk_disable_unprepare(imx6_pcie->pcie_bus);
> > -
> > -	switch (imx6_pcie->drvdata->variant) {
> > -	case IMX6SX:
> > -		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> > -		break;
> > -	case IMX7D:
> > -		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > -				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> > -				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> > -		break;
> > -	case IMX8MQ:
> > -	case IMX8MM:
> > -		clk_disable_unprepare(imx6_pcie->pcie_aux);
> > -		break;
> > -	default:
> > -		break;
> > -	}
> > -}
> > -
> >  static int imx6_pcie_suspend_noirq(struct device *dev)  {
> >  	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > --
> > 2.25.1
> >

RE: [PATCH v6 3/8] PCI: imx6: Move imx6_pcie_clk_disable() earlier

[Hongxing Zhu]

> -----Original Message-----
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> Sent: 2022年2月12日 0:30
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; broonie@kernel.org;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.com; festevam@gmail.com;
> 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 v6 3/8] PCI: imx6: Move imx6_pcie_clk_disable() earlier
> 
> Hello Richard,
> 
> On Tue, Feb 08, 2022 at 11:25:30AM +0800, Richard Zhu wrote:
> > Just move the imx6_pcie_clk_disable() to an earlier place without
> > function changes, since it wouldn't be only used in
> imx6_pcie_suspend_noirq() later.
> 
> In which patch are you going to use imx6_pcie_clk_disable()? I could not see it
> in this patch series.
Hi Francesco:
Thanks for your review.
The imx6_pcie_clk_disable() would be invoked in imx6_pcie_host_exit() in #7
 patch later.

Best Regards
Richard Zhu

> 
> Francesco
> 
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 47
> > +++++++++++++--------------
> >  1 file changed, 23 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 3ca2eef39617..e157f0bec37e 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -533,6 +533,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;
> > @@ -965,30 +988,6 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie
> *imx6_pcie)
> >  	usleep_range(1000, 10000);
> >  }
> >
> > -static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie) -{
> > -	clk_disable_unprepare(imx6_pcie->pcie);
> > -	clk_disable_unprepare(imx6_pcie->pcie_phy);
> > -	clk_disable_unprepare(imx6_pcie->pcie_bus);
> > -
> > -	switch (imx6_pcie->drvdata->variant) {
> > -	case IMX6SX:
> > -		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> > -		break;
> > -	case IMX7D:
> > -		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > -				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> > -				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> > -		break;
> > -	case IMX8MQ:
> > -	case IMX8MM:
> > -		clk_disable_unprepare(imx6_pcie->pcie_aux);
> > -		break;
> > -	default:
> > -		break;
> > -	}
> > -}
> > -
> >  static int imx6_pcie_suspend_noirq(struct device *dev)  {
> >  	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > --
> > 2.25.1
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Hongxing Zhu]

> -----Original Message-----
> From: Hongxing Zhu
> Sent: 2022年2月14日 11:08
> To: Francesco Dolcini <francesco.dolcini@toradex.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; broonie@kernel.org;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.com; festevam@gmail.com;
> linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de
> Subject: RE: [PATCH v6 5/8] PCI: imx6: Refine the regulator usage
> 
> > -----Original Message-----
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > Sent: 2022年2月12日 0:28
> > To: Hongxing Zhu <hongxing.zhu@nxp.com>
> > Cc: l.stach@pengutronix.de; bhelgaas@google.com; broonie@kernel.org;
> > lorenzo.pieralisi@arm.com; jingoohan1@gmail.com; festevam@gmail.com;
> > 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 v6 5/8] PCI: imx6: Refine the regulator usage
> >
> > Hello Richard,
> >
> > On Tue, Feb 08, 2022 at 11:25:32AM +0800, Richard Zhu wrote:
> > > The driver should undo any enables it did itself. The regulator
> > > disable shouldn't be basing decisions on regulator_is_enabled().
> > >
> > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > ---
> > >  drivers/pci/controller/dwc/pci-imx6.c | 14 ++------------
> > >  1 file changed, 2 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 0aca762d88a3..e165ad00989c 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -369,8 +369,6 @@ static int imx6_pcie_attach_pd(struct device
> > > *dev)
> > >
> > >  static void imx6_pcie_assert_core_reset(struct imx6_pcie
> > > *imx6_pcie) {
> > > -	struct device *dev = imx6_pcie->pci->dev;
> > > -
> > >  	switch (imx6_pcie->drvdata->variant) {
> > >  	case IMX7D:
> > >  	case IMX8MQ:
> > > @@ -400,14 +398,6 @@ static void imx6_pcie_assert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> > >  				   IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
> > >  		break;
> > >  	}
> > > -
> > > -	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> > > -		int ret = regulator_disable(imx6_pcie->vpcie);
> > > -
> > > -		if (ret)
> > > -			dev_err(dev, "failed to disable vpcie regulator: %d\n",
> > > -				ret);
> > > -	}
> >
> > This commit is not just cleaning up the regulator usage as you state
> > in the commit message, this is removing the vpcie regulator_disable
> > from imx6_pcie_assert_core_reset().
> >
> > I would not do it, this is called for example on the shutdown callback
> > where it makes sense.
> Hi Francesco:
> Thanks for your review.
> Do you means that we should keep regulator_disable() here?
> Okay, I would change it later.
Hi Francesco:
One more complementary that we can't disable this regulator here, because
 that the regulator might not be enabled at all.

But in the case of suspend/resume operations, the regulator_disable() should
 be invoked behind of imx6_pcie_assert_core_reset () in resume callback to
 balance the enable/disable usage counter.
I would add this change later.
Thanks again for your review comments.

Best Regards
Richard
> 
> Best Regards
> Richard Zhu
> 
> >
> > Francesco

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

[Hongxing Zhu]

> -----Original Message-----
> From: Hongxing Zhu
> Sent: 2022年2月14日 11:08
> To: Francesco Dolcini <francesco.dolcini@toradex.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; broonie@kernel.org;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.com; festevam@gmail.com;
> linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de
> Subject: RE: [PATCH v6 5/8] PCI: imx6: Refine the regulator usage
> 
> > -----Original Message-----
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > Sent: 2022年2月12日 0:28
> > To: Hongxing Zhu <hongxing.zhu@nxp.com>
> > Cc: l.stach@pengutronix.de; bhelgaas@google.com; broonie@kernel.org;
> > lorenzo.pieralisi@arm.com; jingoohan1@gmail.com; festevam@gmail.com;
> > 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 v6 5/8] PCI: imx6: Refine the regulator usage
> >
> > Hello Richard,
> >
> > On Tue, Feb 08, 2022 at 11:25:32AM +0800, Richard Zhu wrote:
> > > The driver should undo any enables it did itself. The regulator
> > > disable shouldn't be basing decisions on regulator_is_enabled().
> > >
> > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > ---
> > >  drivers/pci/controller/dwc/pci-imx6.c | 14 ++------------
> > >  1 file changed, 2 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 0aca762d88a3..e165ad00989c 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -369,8 +369,6 @@ static int imx6_pcie_attach_pd(struct device
> > > *dev)
> > >
> > >  static void imx6_pcie_assert_core_reset(struct imx6_pcie
> > > *imx6_pcie) {
> > > -	struct device *dev = imx6_pcie->pci->dev;
> > > -
> > >  	switch (imx6_pcie->drvdata->variant) {
> > >  	case IMX7D:
> > >  	case IMX8MQ:
> > > @@ -400,14 +398,6 @@ static void imx6_pcie_assert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> > >  				   IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
> > >  		break;
> > >  	}
> > > -
> > > -	if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> > > -		int ret = regulator_disable(imx6_pcie->vpcie);
> > > -
> > > -		if (ret)
> > > -			dev_err(dev, "failed to disable vpcie regulator: %d\n",
> > > -				ret);
> > > -	}
> >
> > This commit is not just cleaning up the regulator usage as you state
> > in the commit message, this is removing the vpcie regulator_disable
> > from imx6_pcie_assert_core_reset().
> >
> > I would not do it, this is called for example on the shutdown callback
> > where it makes sense.
> Hi Francesco:
> Thanks for your review.
> Do you means that we should keep regulator_disable() here?
> Okay, I would change it later.
Hi Francesco:
One more complementary that we can't disable this regulator here, because
 that the regulator might not be enabled at all.

But in the case of suspend/resume operations, the regulator_disable() should
 be invoked behind of imx6_pcie_assert_core_reset () in resume callback to
 balance the enable/disable usage counter.
I would add this change later.
Thanks again for your review comments.

Best Regards
Richard
> 
> Best Regards
> Richard Zhu
> 
> >
> > Francesco

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

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

[Francesco Dolcini]

On Mon, Feb 14, 2022 at 04:52:25AM +0000, Hongxing Zhu wrote:
> > > This commit is not just cleaning up the regulator usage as you state
> > > in the commit message, this is removing the vpcie regulator_disable
> > > from imx6_pcie_assert_core_reset().
> > >
> > > I would not do it, this is called for example on the shutdown callback
> > > where it makes sense.
> > Hi Francesco:
> > Thanks for your review.
> > Do you means that we should keep regulator_disable() here?
> > Okay, I would change it later.
> Hi Francesco:
> One more complementary that we can't disable this regulator here, because
>  that the regulator might not be enabled at all.
> 
> But in the case of suspend/resume operations, the regulator_disable() should
>  be invoked behind of imx6_pcie_assert_core_reset () in resume callback to
>  balance the enable/disable usage counter.

Understood, please do not forget about the imx6_pcie_shutdown() path,
having this regulator switched off there is important IMO.

A small side comment on the topic, at the moment suspend/resume is
not working correctly for me when the PCIe port is connected to a
switch, after resume only the upstream port is working correctly.
The issue is present on the current mainline driver, but also on
the downstream NXP kernel. Not sure what's the problem and at the
moment is not a priority for me to investigate.

Francesco

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

[Francesco Dolcini]

On Mon, Feb 14, 2022 at 04:52:25AM +0000, Hongxing Zhu wrote:
> > > This commit is not just cleaning up the regulator usage as you state
> > > in the commit message, this is removing the vpcie regulator_disable
> > > from imx6_pcie_assert_core_reset().
> > >
> > > I would not do it, this is called for example on the shutdown callback
> > > where it makes sense.
> > Hi Francesco:
> > Thanks for your review.
> > Do you means that we should keep regulator_disable() here?
> > Okay, I would change it later.
> Hi Francesco:
> One more complementary that we can't disable this regulator here, because
>  that the regulator might not be enabled at all.
> 
> But in the case of suspend/resume operations, the regulator_disable() should
>  be invoked behind of imx6_pcie_assert_core_reset () in resume callback to
>  balance the enable/disable usage counter.

Understood, please do not forget about the imx6_pcie_shutdown() path,
having this regulator switched off there is important IMO.

A small side comment on the topic, at the moment suspend/resume is
not working correctly for me when the PCIe port is connected to a
switch, after resume only the upstream port is working correctly.
The issue is present on the current mainline driver, but also on
the downstream NXP kernel. Not sure what's the problem and at the
moment is not a priority for me to investigate.

Francesco


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