[PATCH v2 0/3] PCI: imx: Initial imx7d pm support

[PATCH v2 0/3] PCI: imx: Initial imx7d pm support

[Leonard Crestez]

Changes since v1:
 * Indentation fixes
 * Use imx6_pcie_establish_link instead of imx6_pcie_wait_for_link
 * Move imx6_pcie_ltssm_disable to suspend
 * Make imx6_pcie_clk_disable a separate function
 * Revert PCI irq mapping (also useful separately)

Link to v1: https://lkml.org/lkml/2018/5/29/1042

Leonard Crestez (3):
  Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"
  reset: imx7: Fix always writing bits as 0
  PCI: imx: Initial imx7d pm support

 arch/arm/boot/dts/imx7d.dtsi          |  8 +--
 drivers/pci/controller/dwc/pci-imx6.c | 95 +++++++++++++++++++++++++--
 drivers/reset/reset-imx7.c            |  2 +-
 3 files changed, 95 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH v2 0/3] PCI: imx: Initial imx7d pm support

[Leonard Crestez]

Changes since v1:
 * Indentation fixes
 * Use imx6_pcie_establish_link instead of imx6_pcie_wait_for_link
 * Move imx6_pcie_ltssm_disable to suspend
 * Make imx6_pcie_clk_disable a separate function
 * Revert PCI irq mapping (also useful separately)

Link to v1: https://lkml.org/lkml/2018/5/29/1042

Leonard Crestez (3):
  Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"
  reset: imx7: Fix always writing bits as 0
  PCI: imx: Initial imx7d pm support

 arch/arm/boot/dts/imx7d.dtsi          |  8 +--
 drivers/pci/controller/dwc/pci-imx6.c | 95 +++++++++++++++++++++++++--
 drivers/reset/reset-imx7.c            |  2 +-
 3 files changed, 95 insertions(+), 10 deletions(-)

-- 
2.17.1


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

[PATCH v2 0/3] PCI: imx: Initial imx7d pm support

[Leonard Crestez]

Changes since v1:
 * Indentation fixes
 * Use imx6_pcie_establish_link instead of imx6_pcie_wait_for_link
 * Move imx6_pcie_ltssm_disable to suspend
 * Make imx6_pcie_clk_disable a separate function
 * Revert PCI irq mapping (also useful separately)

Link to v1: https://lkml.org/lkml/2018/5/29/1042

Leonard Crestez (3):
  Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"
  reset: imx7: Fix always writing bits as 0
  PCI: imx: Initial imx7d pm support

 arch/arm/boot/dts/imx7d.dtsi          |  8 +--
 drivers/pci/controller/dwc/pci-imx6.c | 95 +++++++++++++++++++++++++--
 drivers/reset/reset-imx7.c            |  2 +-
 3 files changed, 95 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"

[Leonard Crestez]

This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d.

That commit followed the reference manual but unfortunately the imx7d
manual is incorrect.

Tested with ath9k pcie card and confirmed internally.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 arch/arm/boot/dts/imx7d.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi
index 8d3d123d0a5c..12c5ba7e9f1e 100644
--- a/arch/arm/boot/dts/imx7d.dtsi
+++ b/arch/arm/boot/dts/imx7d.dtsi
@@ -123,14 +123,14 @@
 		num-lanes = <1>;
 		interrupts = <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>;
 		interrupt-names = "msi";
 		#interrupt-cells = <1>;
 		interrupt-map-mask = <0 0 0 0x7>;
-		interrupt-map = <0 0 0 1 &intc GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
-				<0 0 0 2 &intc GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
-				<0 0 0 3 &intc GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>,
-				<0 0 0 4 &intc GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-map = <0 0 0 1 &intc GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>,
+				<0 0 0 2 &intc GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>,
+				<0 0 0 3 &intc GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
+				<0 0 0 4 &intc GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&clks IMX7D_PCIE_CTRL_ROOT_CLK>,
 			 <&clks IMX7D_PLL_ENET_MAIN_100M_CLK>,
 			 <&clks IMX7D_PCIE_PHY_ROOT_CLK>;
 		clock-names = "pcie", "pcie_bus", "pcie_phy";
 		assigned-clocks = <&clks IMX7D_PCIE_CTRL_ROOT_SRC>,
-- 
2.17.1

[PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"

[Leonard Crestez]

This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d.

That commit followed the reference manual but unfortunately the imx7d
manual is incorrect.

Tested with ath9k pcie card and confirmed internally.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 arch/arm/boot/dts/imx7d.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi
index 8d3d123d0a5c..12c5ba7e9f1e 100644
--- a/arch/arm/boot/dts/imx7d.dtsi
+++ b/arch/arm/boot/dts/imx7d.dtsi
@@ -123,14 +123,14 @@
 		num-lanes = <1>;
 		interrupts = <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>;
 		interrupt-names = "msi";
 		#interrupt-cells = <1>;
 		interrupt-map-mask = <0 0 0 0x7>;
-		interrupt-map = <0 0 0 1 &intc GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
-				<0 0 0 2 &intc GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
-				<0 0 0 3 &intc GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>,
-				<0 0 0 4 &intc GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-map = <0 0 0 1 &intc GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>,
+				<0 0 0 2 &intc GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>,
+				<0 0 0 3 &intc GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
+				<0 0 0 4 &intc GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&clks IMX7D_PCIE_CTRL_ROOT_CLK>,
 			 <&clks IMX7D_PLL_ENET_MAIN_100M_CLK>,
 			 <&clks IMX7D_PCIE_PHY_ROOT_CLK>;
 		clock-names = "pcie", "pcie_bus", "pcie_phy";
 		assigned-clocks = <&clks IMX7D_PCIE_CTRL_ROOT_SRC>,
-- 
2.17.1


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

[PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"

[Leonard Crestez]

This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d.

That commit followed the reference manual but unfortunately the imx7d
manual is incorrect.

Tested with ath9k pcie card and confirmed internally.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 arch/arm/boot/dts/imx7d.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi
index 8d3d123d0a5c..12c5ba7e9f1e 100644
--- a/arch/arm/boot/dts/imx7d.dtsi
+++ b/arch/arm/boot/dts/imx7d.dtsi
@@ -123,14 +123,14 @@
 		num-lanes = <1>;
 		interrupts = <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>;
 		interrupt-names = "msi";
 		#interrupt-cells = <1>;
 		interrupt-map-mask = <0 0 0 0x7>;
-		interrupt-map = <0 0 0 1 &intc GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
-				<0 0 0 2 &intc GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
-				<0 0 0 3 &intc GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>,
-				<0 0 0 4 &intc GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-map = <0 0 0 1 &intc GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>,
+				<0 0 0 2 &intc GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>,
+				<0 0 0 3 &intc GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
+				<0 0 0 4 &intc GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&clks IMX7D_PCIE_CTRL_ROOT_CLK>,
 			 <&clks IMX7D_PLL_ENET_MAIN_100M_CLK>,
 			 <&clks IMX7D_PCIE_PHY_ROOT_CLK>;
 		clock-names = "pcie", "pcie_bus", "pcie_phy";
 		assigned-clocks = <&clks IMX7D_PCIE_CTRL_ROOT_SRC>,
-- 
2.17.1

[PATCH v2 2/3] reset: imx7: Fix always writing bits as 0

[Leonard Crestez]

Right now the only user of reset-imx7 is pci-imx6 and the
reset_control_assert and deassert calls on pciephy_reset don't toggle
the PCIEPHY_BTN and PCIEPHY_G_RST bits as expected. Fix this by writing
1 or 0 respectively.

The reference manual is not very clear regarding SRC_PCIEPHY_RCR but for
other registers like MIPIPHY and HSICPHY the bits are explicitly
documented as "1 means assert, 0 means deassert".

The values are still reversed for IMX7_RESET_PCIE_CTRL_APPS_EN.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/reset/reset-imx7.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
index 4db177bc89bc..fdeac1946429 100644
--- a/drivers/reset/reset-imx7.c
+++ b/drivers/reset/reset-imx7.c
@@ -78,11 +78,11 @@ static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
 static int imx7_reset_set(struct reset_controller_dev *rcdev,
 			  unsigned long id, bool assert)
 {
 	struct imx7_src *imx7src = to_imx7_src(rcdev);
 	const struct imx7_src_signal *signal = &imx7_src_signals[id];
-	unsigned int value = 0;
+	unsigned int value = assert ? signal->bit : 0;
 
 	switch (id) {
 	case IMX7_RESET_PCIEPHY:
 		/*
 		 * wait for more than 10us to release phy g_rst and
-- 
2.17.1

[PATCH v2 2/3] reset: imx7: Fix always writing bits as 0

[Leonard Crestez]

Right now the only user of reset-imx7 is pci-imx6 and the
reset_control_assert and deassert calls on pciephy_reset don't toggle
the PCIEPHY_BTN and PCIEPHY_G_RST bits as expected. Fix this by writing
1 or 0 respectively.

The reference manual is not very clear regarding SRC_PCIEPHY_RCR but for
other registers like MIPIPHY and HSICPHY the bits are explicitly
documented as "1 means assert, 0 means deassert".

The values are still reversed for IMX7_RESET_PCIE_CTRL_APPS_EN.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/reset/reset-imx7.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
index 4db177bc89bc..fdeac1946429 100644
--- a/drivers/reset/reset-imx7.c
+++ b/drivers/reset/reset-imx7.c
@@ -78,11 +78,11 @@ static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
 static int imx7_reset_set(struct reset_controller_dev *rcdev,
 			  unsigned long id, bool assert)
 {
 	struct imx7_src *imx7src = to_imx7_src(rcdev);
 	const struct imx7_src_signal *signal = &imx7_src_signals[id];
-	unsigned int value = 0;
+	unsigned int value = assert ? signal->bit : 0;
 
 	switch (id) {
 	case IMX7_RESET_PCIEPHY:
 		/*
 		 * wait for more than 10us to release phy g_rst and
-- 
2.17.1


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

[PATCH v2 2/3] reset: imx7: Fix always writing bits as 0

[Leonard Crestez]

Right now the only user of reset-imx7 is pci-imx6 and the
reset_control_assert and deassert calls on pciephy_reset don't toggle
the PCIEPHY_BTN and PCIEPHY_G_RST bits as expected. Fix this by writing
1 or 0 respectively.

The reference manual is not very clear regarding SRC_PCIEPHY_RCR but for
other registers like MIPIPHY and HSICPHY the bits are explicitly
documented as "1 means assert, 0 means deassert".

The values are still reversed for IMX7_RESET_PCIE_CTRL_APPS_EN.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/reset/reset-imx7.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
index 4db177bc89bc..fdeac1946429 100644
--- a/drivers/reset/reset-imx7.c
+++ b/drivers/reset/reset-imx7.c
@@ -78,11 +78,11 @@ static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
 static int imx7_reset_set(struct reset_controller_dev *rcdev,
 			  unsigned long id, bool assert)
 {
 	struct imx7_src *imx7src = to_imx7_src(rcdev);
 	const struct imx7_src_signal *signal = &imx7_src_signals[id];
-	unsigned int value = 0;
+	unsigned int value = assert ? signal->bit : 0;
 
 	switch (id) {
 	case IMX7_RESET_PCIEPHY:
 		/*
 		 * wait for more than 10us to release phy g_rst and
-- 
2.17.1

[PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Leonard Crestez]

On imx7d the pcie-phy power domain is turned off in suspend and this can
make the system hang after resume when attempting any read from PCI.

Fix this by adding minimal suspend/resume code from the nxp internal
tree. This will prepare for powering down on suspend and reset the block
on resume.

Code is only for imx7d but a very similar sequence can be used for
other socs.

The original author is mostly Richard Zhu <hongxing.zhu@nxp.com>, this
patch adjusts the code to the upstream imx7d implemention using reset
controls and power domains.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 95 +++++++++++++++++++++++++--
 1 file changed, 90 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 80f604602783..daebee905108 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -540,10 +540,27 @@ static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie)
 
 	dev_err(dev, "Speed change timeout\n");
 	return -EINVAL;
 }
 
+static void imx6_pcie_ltssm_enable(struct device *dev)
+{
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+
+	switch (imx6_pcie->variant) {
+	case IMX6Q:
+	case IMX6SX:
+	case IMX6QP:
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+				   IMX6Q_GPR12_PCIE_CTL_2,
+				   IMX6Q_GPR12_PCIE_CTL_2);
+		break;
+	case IMX7D:
+		reset_control_deassert(imx6_pcie->apps_reset);
+	}
+}
+
 static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie)
 {
 	struct dw_pcie *pci = imx6_pcie->pci;
 	struct device *dev = pci->dev;
 	u32 tmp;
@@ -558,15 +575,11 @@ static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie)
 	tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK;
 	tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1;
 	dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp);
 
 	/* Start LTSSM. */
-	if (imx6_pcie->variant == IMX7D)
-		reset_control_deassert(imx6_pcie->apps_reset);
-	else
-		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
-				   IMX6Q_GPR12_PCIE_CTL_2, 1 << 10);
+	imx6_pcie_ltssm_enable(dev);
 
 	ret = imx6_pcie_wait_for_link(imx6_pcie);
 	if (ret)
 		goto err_reset_phy;
 
@@ -681,10 +694,81 @@ static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie,
 
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.link_up = imx6_pcie_link_up,
 };
 
+#ifdef CONFIG_PM_SLEEP
+static void imx6_pcie_ltssm_disable(struct device *dev)
+{
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+
+	switch (imx6_pcie->variant) {
+	case IMX6Q:
+	case IMX6SX:
+	case IMX6QP:
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+				   IMX6Q_GPR12_PCIE_CTL_2, 0);
+		break;
+	case IMX7D:
+		reset_control_assert(imx6_pcie->apps_reset);
+		break;
+	}
+}
+
+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);
+
+	if (imx6_pcie->variant == IMX7D) {
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
+				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
+	}
+}
+
+static int imx6_pcie_suspend_noirq(struct device *dev)
+{
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+
+	if (imx6_pcie->variant != IMX7D)
+		return 0;
+
+	imx6_pcie_clk_disable(imx6_pcie);
+	imx6_pcie_ltssm_disable(dev);
+
+	return 0;
+}
+
+static int imx6_pcie_resume_noirq(struct device *dev)
+{
+	int ret = 0;
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+	struct pcie_port *pp = &imx6_pcie->pci->pp;
+
+	if (imx6_pcie->variant != IMX7D)
+		return 0;
+
+	imx6_pcie_assert_core_reset(imx6_pcie);
+	imx6_pcie_init_phy(imx6_pcie);
+	imx6_pcie_deassert_core_reset(imx6_pcie);
+	dw_pcie_setup_rc(pp);
+
+	ret = imx6_pcie_establish_link(imx6_pcie);
+	if (ret < 0)
+		pr_err("pcie link is down after resume.\n");
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops imx6_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx6_pcie_suspend_noirq,
+				      imx6_pcie_resume_noirq)
+};
+
 static int imx6_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct dw_pcie *pci;
 	struct imx6_pcie *imx6_pcie;
@@ -847,10 +931,11 @@ static const struct of_device_id imx6_pcie_of_match[] = {
 static struct platform_driver imx6_pcie_driver = {
 	.driver = {
 		.name	= "imx6q-pcie",
 		.of_match_table = imx6_pcie_of_match,
 		.suppress_bind_attrs = true,
+		.pm = &imx6_pcie_pm_ops,
 	},
 	.probe    = imx6_pcie_probe,
 	.shutdown = imx6_pcie_shutdown,
 };
 
-- 
2.17.1

[PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Leonard Crestez]

On imx7d the pcie-phy power domain is turned off in suspend and this can
make the system hang after resume when attempting any read from PCI.

Fix this by adding minimal suspend/resume code from the nxp internal
tree. This will prepare for powering down on suspend and reset the block
on resume.

Code is only for imx7d but a very similar sequence can be used for
other socs.

The original author is mostly Richard Zhu <hongxing.zhu@nxp.com>, this
patch adjusts the code to the upstream imx7d implemention using reset
controls and power domains.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 95 +++++++++++++++++++++++++--
 1 file changed, 90 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 80f604602783..daebee905108 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -540,10 +540,27 @@ static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie)
 
 	dev_err(dev, "Speed change timeout\n");
 	return -EINVAL;
 }
 
+static void imx6_pcie_ltssm_enable(struct device *dev)
+{
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+
+	switch (imx6_pcie->variant) {
+	case IMX6Q:
+	case IMX6SX:
+	case IMX6QP:
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+				   IMX6Q_GPR12_PCIE_CTL_2,
+				   IMX6Q_GPR12_PCIE_CTL_2);
+		break;
+	case IMX7D:
+		reset_control_deassert(imx6_pcie->apps_reset);
+	}
+}
+
 static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie)
 {
 	struct dw_pcie *pci = imx6_pcie->pci;
 	struct device *dev = pci->dev;
 	u32 tmp;
@@ -558,15 +575,11 @@ static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie)
 	tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK;
 	tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1;
 	dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp);
 
 	/* Start LTSSM. */
-	if (imx6_pcie->variant == IMX7D)
-		reset_control_deassert(imx6_pcie->apps_reset);
-	else
-		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
-				   IMX6Q_GPR12_PCIE_CTL_2, 1 << 10);
+	imx6_pcie_ltssm_enable(dev);
 
 	ret = imx6_pcie_wait_for_link(imx6_pcie);
 	if (ret)
 		goto err_reset_phy;
 
@@ -681,10 +694,81 @@ static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie,
 
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.link_up = imx6_pcie_link_up,
 };
 
+#ifdef CONFIG_PM_SLEEP
+static void imx6_pcie_ltssm_disable(struct device *dev)
+{
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+
+	switch (imx6_pcie->variant) {
+	case IMX6Q:
+	case IMX6SX:
+	case IMX6QP:
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+				   IMX6Q_GPR12_PCIE_CTL_2, 0);
+		break;
+	case IMX7D:
+		reset_control_assert(imx6_pcie->apps_reset);
+		break;
+	}
+}
+
+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);
+
+	if (imx6_pcie->variant == IMX7D) {
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
+				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
+	}
+}
+
+static int imx6_pcie_suspend_noirq(struct device *dev)
+{
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+
+	if (imx6_pcie->variant != IMX7D)
+		return 0;
+
+	imx6_pcie_clk_disable(imx6_pcie);
+	imx6_pcie_ltssm_disable(dev);
+
+	return 0;
+}
+
+static int imx6_pcie_resume_noirq(struct device *dev)
+{
+	int ret = 0;
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+	struct pcie_port *pp = &imx6_pcie->pci->pp;
+
+	if (imx6_pcie->variant != IMX7D)
+		return 0;
+
+	imx6_pcie_assert_core_reset(imx6_pcie);
+	imx6_pcie_init_phy(imx6_pcie);
+	imx6_pcie_deassert_core_reset(imx6_pcie);
+	dw_pcie_setup_rc(pp);
+
+	ret = imx6_pcie_establish_link(imx6_pcie);
+	if (ret < 0)
+		pr_err("pcie link is down after resume.\n");
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops imx6_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx6_pcie_suspend_noirq,
+				      imx6_pcie_resume_noirq)
+};
+
 static int imx6_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct dw_pcie *pci;
 	struct imx6_pcie *imx6_pcie;
@@ -847,10 +931,11 @@ static const struct of_device_id imx6_pcie_of_match[] = {
 static struct platform_driver imx6_pcie_driver = {
 	.driver = {
 		.name	= "imx6q-pcie",
 		.of_match_table = imx6_pcie_of_match,
 		.suppress_bind_attrs = true,
+		.pm = &imx6_pcie_pm_ops,
 	},
 	.probe    = imx6_pcie_probe,
 	.shutdown = imx6_pcie_shutdown,
 };
 
-- 
2.17.1


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

[PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Leonard Crestez]

On imx7d the pcie-phy power domain is turned off in suspend and this can
make the system hang after resume when attempting any read from PCI.

Fix this by adding minimal suspend/resume code from the nxp internal
tree. This will prepare for powering down on suspend and reset the block
on resume.

Code is only for imx7d but a very similar sequence can be used for
other socs.

The original author is mostly Richard Zhu <hongxing.zhu@nxp.com>, this
patch adjusts the code to the upstream imx7d implemention using reset
controls and power domains.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 95 +++++++++++++++++++++++++--
 1 file changed, 90 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 80f604602783..daebee905108 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -540,10 +540,27 @@ static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie)
 
 	dev_err(dev, "Speed change timeout\n");
 	return -EINVAL;
 }
 
+static void imx6_pcie_ltssm_enable(struct device *dev)
+{
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+
+	switch (imx6_pcie->variant) {
+	case IMX6Q:
+	case IMX6SX:
+	case IMX6QP:
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+				   IMX6Q_GPR12_PCIE_CTL_2,
+				   IMX6Q_GPR12_PCIE_CTL_2);
+		break;
+	case IMX7D:
+		reset_control_deassert(imx6_pcie->apps_reset);
+	}
+}
+
 static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie)
 {
 	struct dw_pcie *pci = imx6_pcie->pci;
 	struct device *dev = pci->dev;
 	u32 tmp;
@@ -558,15 +575,11 @@ static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie)
 	tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK;
 	tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1;
 	dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp);
 
 	/* Start LTSSM. */
-	if (imx6_pcie->variant == IMX7D)
-		reset_control_deassert(imx6_pcie->apps_reset);
-	else
-		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
-				   IMX6Q_GPR12_PCIE_CTL_2, 1 << 10);
+	imx6_pcie_ltssm_enable(dev);
 
 	ret = imx6_pcie_wait_for_link(imx6_pcie);
 	if (ret)
 		goto err_reset_phy;
 
@@ -681,10 +694,81 @@ static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie,
 
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.link_up = imx6_pcie_link_up,
 };
 
+#ifdef CONFIG_PM_SLEEP
+static void imx6_pcie_ltssm_disable(struct device *dev)
+{
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+
+	switch (imx6_pcie->variant) {
+	case IMX6Q:
+	case IMX6SX:
+	case IMX6QP:
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+				   IMX6Q_GPR12_PCIE_CTL_2, 0);
+		break;
+	case IMX7D:
+		reset_control_assert(imx6_pcie->apps_reset);
+		break;
+	}
+}
+
+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);
+
+	if (imx6_pcie->variant == IMX7D) {
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
+				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
+	}
+}
+
+static int imx6_pcie_suspend_noirq(struct device *dev)
+{
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+
+	if (imx6_pcie->variant != IMX7D)
+		return 0;
+
+	imx6_pcie_clk_disable(imx6_pcie);
+	imx6_pcie_ltssm_disable(dev);
+
+	return 0;
+}
+
+static int imx6_pcie_resume_noirq(struct device *dev)
+{
+	int ret = 0;
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+	struct pcie_port *pp = &imx6_pcie->pci->pp;
+
+	if (imx6_pcie->variant != IMX7D)
+		return 0;
+
+	imx6_pcie_assert_core_reset(imx6_pcie);
+	imx6_pcie_init_phy(imx6_pcie);
+	imx6_pcie_deassert_core_reset(imx6_pcie);
+	dw_pcie_setup_rc(pp);
+
+	ret = imx6_pcie_establish_link(imx6_pcie);
+	if (ret < 0)
+		pr_err("pcie link is down after resume.\n");
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops imx6_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx6_pcie_suspend_noirq,
+				      imx6_pcie_resume_noirq)
+};
+
 static int imx6_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct dw_pcie *pci;
 	struct imx6_pcie *imx6_pcie;
@@ -847,10 +931,11 @@ static const struct of_device_id imx6_pcie_of_match[] = {
 static struct platform_driver imx6_pcie_driver = {
 	.driver = {
 		.name	= "imx6q-pcie",
 		.of_match_table = imx6_pcie_of_match,
 		.suppress_bind_attrs = true,
+		.pm = &imx6_pcie_pm_ops,
 	},
 	.probe    = imx6_pcie_probe,
 	.shutdown = imx6_pcie_shutdown,
 };
 
-- 
2.17.1

Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Fabio Estevam]

Hi Leonard,

On Fri, Jul 20, 2018 at 9:47 AM, Leonard Crestez
<leonard.crestez@nxp.com> wrote:

> +static int imx6_pcie_resume_noirq(struct device *dev)
> +{
> +       int ret = 0;

There is no need for initializing 'ret' here.

> +       struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> +       struct pcie_port *pp = &imx6_pcie->pci->pp;
> +
> +       if (imx6_pcie->variant != IMX7D)
> +               return 0;
> +
> +       imx6_pcie_assert_core_reset(imx6_pcie);
> +       imx6_pcie_init_phy(imx6_pcie);
> +       imx6_pcie_deassert_core_reset(imx6_pcie);
> +       dw_pcie_setup_rc(pp);
> +
> +       ret = imx6_pcie_establish_link(imx6_pcie);
> +       if (ret < 0)
> +               pr_err("pcie link is down after resume.\n");

Shouldn't the error be propagated?

Also, dev_err() is preferred instead of pr_err().

Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Fabio Estevam]

Hi Leonard,

On Fri, Jul 20, 2018 at 9:47 AM, Leonard Crestez
<leonard.crestez@nxp.com> wrote:

> +static int imx6_pcie_resume_noirq(struct device *dev)
> +{
> +       int ret = 0;

There is no need for initializing 'ret' here.

> +       struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> +       struct pcie_port *pp = &imx6_pcie->pci->pp;
> +
> +       if (imx6_pcie->variant != IMX7D)
> +               return 0;
> +
> +       imx6_pcie_assert_core_reset(imx6_pcie);
> +       imx6_pcie_init_phy(imx6_pcie);
> +       imx6_pcie_deassert_core_reset(imx6_pcie);
> +       dw_pcie_setup_rc(pp);
> +
> +       ret = imx6_pcie_establish_link(imx6_pcie);
> +       if (ret < 0)
> +               pr_err("pcie link is down after resume.\n");

Shouldn't the error be propagated?

Also, dev_err() is preferred instead of pr_err().

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

Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Fabio Estevam]

Hi Leonard,

On Fri, Jul 20, 2018 at 9:47 AM, Leonard Crestez
<leonard.crestez@nxp.com> wrote:

> +static int imx6_pcie_resume_noirq(struct device *dev)
> +{
> +       int ret = 0;

There is no need for initializing 'ret' here.

> +       struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> +       struct pcie_port *pp = &imx6_pcie->pci->pp;
> +
> +       if (imx6_pcie->variant != IMX7D)
> +               return 0;
> +
> +       imx6_pcie_assert_core_reset(imx6_pcie);
> +       imx6_pcie_init_phy(imx6_pcie);
> +       imx6_pcie_deassert_core_reset(imx6_pcie);
> +       dw_pcie_setup_rc(pp);
> +
> +       ret = imx6_pcie_establish_link(imx6_pcie);
> +       if (ret < 0)
> +               pr_err("pcie link is down after resume.\n");

Shouldn't the error be propagated?

Also, dev_err() is preferred instead of pr_err().

[PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Fabio Estevam]

Hi Leonard,

On Fri, Jul 20, 2018 at 9:47 AM, Leonard Crestez
<leonard.crestez@nxp.com> wrote:

> +static int imx6_pcie_resume_noirq(struct device *dev)
> +{
> +       int ret = 0;

There is no need for initializing 'ret' here.

> +       struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> +       struct pcie_port *pp = &imx6_pcie->pci->pp;
> +
> +       if (imx6_pcie->variant != IMX7D)
> +               return 0;
> +
> +       imx6_pcie_assert_core_reset(imx6_pcie);
> +       imx6_pcie_init_phy(imx6_pcie);
> +       imx6_pcie_deassert_core_reset(imx6_pcie);
> +       dw_pcie_setup_rc(pp);
> +
> +       ret = imx6_pcie_establish_link(imx6_pcie);
> +       if (ret < 0)
> +               pr_err("pcie link is down after resume.\n");

Shouldn't the error be propagated?

Also, dev_err() is preferred instead of pr_err().

Re: [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"

[Andrey Smirnov]

On Fri, Jul 20, 2018 at 5:48 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d.
>
> That commit followed the reference manual but unfortunately the imx7d
> manual is incorrect.
>

I'd also add similar comment to DT file to prevent people from trying
to "fix" this in the future.

Also, this change is going to break QEMU's mapping found here:

https://git.qemu.org/?p=qemu.git;a=blob;f=hw/arm/fsl-imx7.c;h=d5e26855a552e2d52b91f0435a607fae2f88456b;hb=HEAD#l464

any change you are planning to make the change there as well?

Thanks,
Andrey Smirnov

Re: [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"

[Andrey Smirnov]

On Fri, Jul 20, 2018 at 5:48 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d.
>
> That commit followed the reference manual but unfortunately the imx7d
> manual is incorrect.
>

I'd also add similar comment to DT file to prevent people from trying
to "fix" this in the future.

Also, this change is going to break QEMU's mapping found here:

https://git.qemu.org/?p=qemu.git;a=blob;f=hw/arm/fsl-imx7.c;h=d5e26855a552e2d52b91f0435a607fae2f88456b;hb=HEAD#l464

any change you are planning to make the change there as well?

Thanks,
Andrey Smirnov

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

[PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"

[Andrey Smirnov]

On Fri, Jul 20, 2018 at 5:48 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d.
>
> That commit followed the reference manual but unfortunately the imx7d
> manual is incorrect.
>

I'd also add similar comment to DT file to prevent people from trying
to "fix" this in the future.

Also, this change is going to break QEMU's mapping found here:

https://git.qemu.org/?p=qemu.git;a=blob;f=hw/arm/fsl-imx7.c;h=d5e26855a552e2d52b91f0435a607fae2f88456b;hb=HEAD#l464

any change you are planning to make the change there as well?

Thanks,
Andrey Smirnov

Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Lucas Stach]

Hi Leonard,

Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> On imx7d the pcie-phy power domain is turned off in suspend and this can
> make the system hang after resume when attempting any read from PCI.
> 
> Fix this by adding minimal suspend/resume code from the nxp internal
> tree. This will prepare for powering down on suspend and reset the block
> on resume.
> 
> Code is only for imx7d but a very similar sequence can be used for
> other socs.
> 
> > The original author is mostly Richard Zhu <hongxing.zhu@nxp.com>, this
> patch adjusts the code to the upstream imx7d implemention using reset
> controls and power domains.
> 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 95 +++++++++++++++++++++++++--
>  1 file changed, 90 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 80f604602783..daebee905108 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -540,10 +540,27 @@ static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie)
>  
> >  	dev_err(dev, "Speed change timeout\n");
> >  	return -EINVAL;
>  }
>  
> +static void imx6_pcie_ltssm_enable(struct device *dev)
> +{
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> +
> > +	switch (imx6_pcie->variant) {
> > +	case IMX6Q:
> > +	case IMX6SX:
> > +	case IMX6QP:
> > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > +				   IMX6Q_GPR12_PCIE_CTL_2,
> > +				   IMX6Q_GPR12_PCIE_CTL_2);
> > +		break;
> > +	case IMX7D:
> > +		reset_control_deassert(imx6_pcie->apps_reset);
> > +	}
> +}
> +
>  static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie)
>  {
> >  	struct dw_pcie *pci = imx6_pcie->pci;
> >  	struct device *dev = pci->dev;
> >  	u32 tmp;
> @@ -558,15 +575,11 @@ static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie)
> >  	tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK;
> >  	tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1;
> >  	dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp);
>  
> >  	/* Start LTSSM. */
> > -	if (imx6_pcie->variant == IMX7D)
> > -		reset_control_deassert(imx6_pcie->apps_reset);
> > -	else
> > -		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > -				   IMX6Q_GPR12_PCIE_CTL_2, 1 << 10);
> > +	imx6_pcie_ltssm_enable(dev);
>  
> >  	ret = imx6_pcie_wait_for_link(imx6_pcie);
> >  	if (ret)
> >  		goto err_reset_phy;
>  
> @@ -681,10 +694,81 @@ static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie,
>  
>  static const struct dw_pcie_ops dw_pcie_ops = {
> >  	.link_up = imx6_pcie_link_up,
>  };
>  
> +#ifdef CONFIG_PM_SLEEP
> +static void imx6_pcie_ltssm_disable(struct device *dev)
> +{
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> +
> > +	switch (imx6_pcie->variant) {
> > +	case IMX6Q:
> > +	case IMX6SX:
> > +	case IMX6QP:
> > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> +				   IMX6Q_GPR12_PCIE_CTL_2, 0);

Has this been tested on i.MX6? LTSSM disable requires a more complex
sequence on this SoC to avoid hanging the system. See commit
3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling
it".

Note that implementing the correct sequence requires the core clocks to
  still be on when disabling LTSSM, so would need to switch ordering of
the function calls in imx6_pcie_suspend_noirq.

> +		break;
> > +	case IMX7D:
> > +		reset_control_assert(imx6_pcie->apps_reset);
> > +		break;
> > +	}
> +}
> +
> +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);
> +
> > +	if (imx6_pcie->variant == IMX7D) {
> > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > +				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> > +				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> > +	}
> +}
> +
> +static int imx6_pcie_suspend_noirq(struct device *dev)
> +{
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> +
> > +	if (imx6_pcie->variant != IMX7D)
> > +		return 0;
> +
> > +	imx6_pcie_clk_disable(imx6_pcie);
> > +	imx6_pcie_ltssm_disable(dev);
> +
> > +	return 0;
> +}
> +
> +static int imx6_pcie_resume_noirq(struct device *dev)
> +{
> > +	int ret = 0;
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > +	struct pcie_port *pp = &imx6_pcie->pci->pp;
> +
> > +	if (imx6_pcie->variant != IMX7D)
> > +		return 0;
> +
> > +	imx6_pcie_assert_core_reset(imx6_pcie);
> > +	imx6_pcie_init_phy(imx6_pcie);
> > +	imx6_pcie_deassert_core_reset(imx6_pcie);
> > +	dw_pcie_setup_rc(pp);
> +
> > +	ret = imx6_pcie_establish_link(imx6_pcie);
> > +	if (ret < 0)
> +		pr_err("pcie link is down after resume.\n");

dev_err(), please.

Regards,
Lucas

> +
> > +	return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops imx6_pcie_pm_ops = {
> > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx6_pcie_suspend_noirq,
> > +				      imx6_pcie_resume_noirq)
> +};
> +
>  static int imx6_pcie_probe(struct platform_device *pdev)
>  {
> >  	struct device *dev = &pdev->dev;
> >  	struct dw_pcie *pci;
> >  	struct imx6_pcie *imx6_pcie;
> @@ -847,10 +931,11 @@ static const struct of_device_id imx6_pcie_of_match[] = {
>  static struct platform_driver imx6_pcie_driver = {
> >  	.driver = {
> > >  		.name	= "imx6q-pcie",
> >  		.of_match_table = imx6_pcie_of_match,
> >  		.suppress_bind_attrs = true,
> > +		.pm = &imx6_pcie_pm_ops,
> >  	},
> >  	.probe    = imx6_pcie_probe,
> >  	.shutdown = imx6_pcie_shutdown,
>  };
>  

Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Lucas Stach]

SGkgTGVvbmFyZCwKCkFtIEZyZWl0YWcsIGRlbiAyMC4wNy4yMDE4LCAxNTo0NyArMDMwMCBzY2hy
aWViIExlb25hcmQgQ3Jlc3RlejoKPiBPbiBpbXg3ZCB0aGUgcGNpZS1waHkgcG93ZXIgZG9tYWlu
IGlzIHR1cm5lZCBvZmYgaW4gc3VzcGVuZCBhbmQgdGhpcyBjYW4KPiBtYWtlIHRoZSBzeXN0ZW0g
aGFuZyBhZnRlciByZXN1bWUgd2hlbiBhdHRlbXB0aW5nIGFueSByZWFkIGZyb20gUENJLgo+IAo+
IEZpeCB0aGlzIGJ5IGFkZGluZyBtaW5pbWFsIHN1c3BlbmQvcmVzdW1lIGNvZGUgZnJvbSB0aGUg
bnhwIGludGVybmFsCj4gdHJlZS4gVGhpcyB3aWxsIHByZXBhcmUgZm9yIHBvd2VyaW5nIGRvd24g
b24gc3VzcGVuZCBhbmQgcmVzZXQgdGhlIGJsb2NrCj4gb24gcmVzdW1lLgo+IAo+IENvZGUgaXMg
b25seSBmb3IgaW14N2QgYnV0IGEgdmVyeSBzaW1pbGFyIHNlcXVlbmNlIGNhbiBiZSB1c2VkIGZv
cgo+IG90aGVyIHNvY3MuCj4gCj4gPiBUaGUgb3JpZ2luYWwgYXV0aG9yIGlzIG1vc3RseSBSaWNo
YXJkIFpodSA8aG9uZ3hpbmcuemh1QG54cC5jb20+LCB0aGlzCj4gcGF0Y2ggYWRqdXN0cyB0aGUg
Y29kZSB0byB0aGUgdXBzdHJlYW0gaW14N2QgaW1wbGVtZW50aW9uIHVzaW5nIHJlc2V0Cj4gY29u
dHJvbHMgYW5kIHBvd2VyIGRvbWFpbnMuCj4gCj4gPiBTaWduZWQtb2ZmLWJ5OiBMZW9uYXJkIENy
ZXN0ZXogPGxlb25hcmQuY3Jlc3RlekBueHAuY29tPgo+IC0tLQo+IMKgZHJpdmVycy9wY2kvY29u
dHJvbGxlci9kd2MvcGNpLWlteDYuYyB8IDk1ICsrKysrKysrKysrKysrKysrKysrKysrKystLQo+
IMKgMSBmaWxlIGNoYW5nZWQsIDkwIGluc2VydGlvbnMoKyksIDUgZGVsZXRpb25zKC0pCj4gCj4g
ZGlmZiAtLWdpdCBhL2RyaXZlcnMvcGNpL2NvbnRyb2xsZXIvZHdjL3BjaS1pbXg2LmMgYi9kcml2
ZXJzL3BjaS9jb250cm9sbGVyL2R3Yy9wY2ktaW14Ni5jCj4gaW5kZXggODBmNjA0NjAyNzgzLi5k
YWViZWU5MDUxMDggMTAwNjQ0Cj4gLS0tIGEvZHJpdmVycy9wY2kvY29udHJvbGxlci9kd2MvcGNp
LWlteDYuYwo+ICsrKyBiL2RyaXZlcnMvcGNpL2NvbnRyb2xsZXIvZHdjL3BjaS1pbXg2LmMKPiBA
QCAtNTQwLDEwICs1NDAsMjcgQEAgc3RhdGljIGludCBpbXg2X3BjaWVfd2FpdF9mb3Jfc3BlZWRf
Y2hhbmdlKHN0cnVjdCBpbXg2X3BjaWUgKmlteDZfcGNpZSkKPiDCoAo+ID4gwqAJZGV2X2Vycihk
ZXYsICJTcGVlZCBjaGFuZ2UgdGltZW91dFxuIik7Cj4gPiDCoAlyZXR1cm4gLUVJTlZBTDsKPiDC
oH0KPiDCoAo+ICtzdGF0aWMgdm9pZCBpbXg2X3BjaWVfbHRzc21fZW5hYmxlKHN0cnVjdCBkZXZp
Y2UgKmRldikKPiArewo+ID4gKwlzdHJ1Y3QgaW14Nl9wY2llICppbXg2X3BjaWUgPSBkZXZfZ2V0
X2RydmRhdGEoZGV2KTsKPiArCj4gPiArCXN3aXRjaCAoaW14Nl9wY2llLT52YXJpYW50KSB7Cj4g
PiArCWNhc2UgSU1YNlE6Cj4gPiArCWNhc2UgSU1YNlNYOgo+ID4gKwljYXNlIElNWDZRUDoKPiA+
ICsJCXJlZ21hcF91cGRhdGVfYml0cyhpbXg2X3BjaWUtPmlvbXV4Y19ncHIsIElPTVVYQ19HUFIx
MiwKPiA+ICsJCQkJwqDCoMKgSU1YNlFfR1BSMTJfUENJRV9DVExfMiwKPiA+ICsJCQkJwqDCoMKg
SU1YNlFfR1BSMTJfUENJRV9DVExfMik7Cj4gPiArCQlicmVhazsKPiA+ICsJY2FzZSBJTVg3RDoK
PiA+ICsJCXJlc2V0X2NvbnRyb2xfZGVhc3NlcnQoaW14Nl9wY2llLT5hcHBzX3Jlc2V0KTsKPiA+
ICsJfQo+ICt9Cj4gKwo+IMKgc3RhdGljIGludCBpbXg2X3BjaWVfZXN0YWJsaXNoX2xpbmsoc3Ry
dWN0IGlteDZfcGNpZSAqaW14Nl9wY2llKQo+IMKgewo+ID4gwqAJc3RydWN0IGR3X3BjaWUgKnBj
aSA9IGlteDZfcGNpZS0+cGNpOwo+ID4gwqAJc3RydWN0IGRldmljZSAqZGV2ID0gcGNpLT5kZXY7
Cj4gPiDCoAl1MzIgdG1wOwo+IEBAIC01NTgsMTUgKzU3NSwxMSBAQCBzdGF0aWMgaW50IGlteDZf
cGNpZV9lc3RhYmxpc2hfbGluayhzdHJ1Y3QgaW14Nl9wY2llICppbXg2X3BjaWUpCj4gPiDCoAl0
bXAgJj0gflBDSUVfUkNfTENSX01BWF9MSU5LX1NQRUVEU19NQVNLOwo+ID4gwqAJdG1wIHw9IFBD
SUVfUkNfTENSX01BWF9MSU5LX1NQRUVEU19HRU4xOwo+ID4gwqAJZHdfcGNpZV93cml0ZWxfZGJp
KHBjaSwgUENJRV9SQ19MQ1IsIHRtcCk7Cj4gwqAKPiA+IMKgCS8qIFN0YXJ0IExUU1NNLiAqLwo+
ID4gLQlpZiAoaW14Nl9wY2llLT52YXJpYW50ID09IElNWDdEKQo+ID4gLQkJcmVzZXRfY29udHJv
bF9kZWFzc2VydChpbXg2X3BjaWUtPmFwcHNfcmVzZXQpOwo+ID4gLQllbHNlCj4gPiAtCQlyZWdt
YXBfdXBkYXRlX2JpdHMoaW14Nl9wY2llLT5pb211eGNfZ3ByLCBJT01VWENfR1BSMTIsCj4gPiAt
CQkJCcKgwqDCoElNWDZRX0dQUjEyX1BDSUVfQ1RMXzIsIDEgPDwgMTApOwo+ID4gKwlpbXg2X3Bj
aWVfbHRzc21fZW5hYmxlKGRldik7Cj4gwqAKPiA+IMKgCXJldCA9IGlteDZfcGNpZV93YWl0X2Zv
cl9saW5rKGlteDZfcGNpZSk7Cj4gPiDCoAlpZiAocmV0KQo+ID4gwqAJCWdvdG8gZXJyX3Jlc2V0
X3BoeTsKPiDCoAo+IEBAIC02ODEsMTAgKzY5NCw4MSBAQCBzdGF0aWMgaW50IGlteDZfYWRkX3Bj
aWVfcG9ydChzdHJ1Y3QgaW14Nl9wY2llICppbXg2X3BjaWUsCj4gwqAKPiDCoHN0YXRpYyBjb25z
dCBzdHJ1Y3QgZHdfcGNpZV9vcHMgZHdfcGNpZV9vcHMgPSB7Cj4gPiDCoAkubGlua191cCA9IGlt
eDZfcGNpZV9saW5rX3VwLAo+IMKgfTsKPiDCoAo+ICsjaWZkZWYgQ09ORklHX1BNX1NMRUVQCj4g
K3N0YXRpYyB2b2lkIGlteDZfcGNpZV9sdHNzbV9kaXNhYmxlKHN0cnVjdCBkZXZpY2UgKmRldikK
PiArewo+ID4gKwlzdHJ1Y3QgaW14Nl9wY2llICppbXg2X3BjaWUgPSBkZXZfZ2V0X2RydmRhdGEo
ZGV2KTsKPiArCj4gPiArCXN3aXRjaCAoaW14Nl9wY2llLT52YXJpYW50KSB7Cj4gPiArCWNhc2Ug
SU1YNlE6Cj4gPiArCWNhc2UgSU1YNlNYOgo+ID4gKwljYXNlIElNWDZRUDoKPiA+ICsJCXJlZ21h
cF91cGRhdGVfYml0cyhpbXg2X3BjaWUtPmlvbXV4Y19ncHIsIElPTVVYQ19HUFIxMiwKPiArCQkJ
CcKgwqDCoElNWDZRX0dQUjEyX1BDSUVfQ1RMXzIsIDApOwoKSGFzIHRoaXMgYmVlbiB0ZXN0ZWQg
b24gaS5NWDY/IExUU1NNIGRpc2FibGUgcmVxdWlyZXMgYSBtb3JlIGNvbXBsZXgKc2VxdWVuY2Ug
b24gdGhpcyBTb0MgdG8gYXZvaWQgaGFuZ2luZyB0aGUgc3lzdGVtLiBTZWUgY29tbWl0CjNlM2U0
MDZlMzgwNyAiUENJOiBpbXg2OiBQdXQgTFRTU00gaW4gIkRldGVjdCIgc3RhdGUgYmVmb3JlIGRp
c2FibGluZwppdCIuCgpOb3RlIHRoYXQgaW1wbGVtZW50aW5nIHRoZSBjb3JyZWN0IHNlcXVlbmNl
IHJlcXVpcmVzIHRoZSBjb3JlIGNsb2NrcyB0bwogIHN0aWxsIGJlIG9uIHdoZW4gZGlzYWJsaW5n
IExUU1NNLCBzbyB3b3VsZCBuZWVkIHRvIHN3aXRjaCBvcmRlcmluZyBvZgp0aGUgZnVuY3Rpb24g
Y2FsbHMgaW7CoGlteDZfcGNpZV9zdXNwZW5kX25vaXJxLgoKPiArCQlicmVhazsKPiA+ICsJY2Fz
ZSBJTVg3RDoKPiA+ICsJCXJlc2V0X2NvbnRyb2xfYXNzZXJ0KGlteDZfcGNpZS0+YXBwc19yZXNl
dCk7Cj4gPiArCQlicmVhazsKPiA+ICsJfQo+ICt9Cj4gKwo+ICtzdGF0aWMgdm9pZCBpbXg2X3Bj
aWVfY2xrX2Rpc2FibGUoc3RydWN0IGlteDZfcGNpZSAqaW14Nl9wY2llKQo+ICt7Cj4gPiArCWNs
a19kaXNhYmxlX3VucHJlcGFyZShpbXg2X3BjaWUtPnBjaWUpOwo+ID4gKwljbGtfZGlzYWJsZV91
bnByZXBhcmUoaW14Nl9wY2llLT5wY2llX3BoeSk7Cj4gPiArCWNsa19kaXNhYmxlX3VucHJlcGFy
ZShpbXg2X3BjaWUtPnBjaWVfYnVzKTsKPiArCj4gPiArCWlmIChpbXg2X3BjaWUtPnZhcmlhbnQg
PT0gSU1YN0QpIHsKPiA+ICsJCXJlZ21hcF91cGRhdGVfYml0cyhpbXg2X3BjaWUtPmlvbXV4Y19n
cHIsIElPTVVYQ19HUFIxMiwKPiA+ICsJCQkJwqDCoMKgSU1YN0RfR1BSMTJfUENJRV9QSFlfUkVG
Q0xLX1NFTCwKPiA+ICsJCQkJwqDCoMKgSU1YN0RfR1BSMTJfUENJRV9QSFlfUkVGQ0xLX1NFTCk7
Cj4gPiArCX0KPiArfQo+ICsKPiArc3RhdGljIGludCBpbXg2X3BjaWVfc3VzcGVuZF9ub2lycShz
dHJ1Y3QgZGV2aWNlICpkZXYpCj4gK3sKPiA+ICsJc3RydWN0IGlteDZfcGNpZSAqaW14Nl9wY2ll
ID0gZGV2X2dldF9kcnZkYXRhKGRldik7Cj4gKwo+ID4gKwlpZiAoaW14Nl9wY2llLT52YXJpYW50
ICE9IElNWDdEKQo+ID4gKwkJcmV0dXJuIDA7Cj4gKwo+ID4gKwlpbXg2X3BjaWVfY2xrX2Rpc2Fi
bGUoaW14Nl9wY2llKTsKPiA+ICsJaW14Nl9wY2llX2x0c3NtX2Rpc2FibGUoZGV2KTsKPiArCj4g
PiArCXJldHVybiAwOwo+ICt9Cj4gKwo+ICtzdGF0aWMgaW50IGlteDZfcGNpZV9yZXN1bWVfbm9p
cnEoc3RydWN0IGRldmljZSAqZGV2KQo+ICt7Cj4gPiArCWludCByZXQgPSAwOwo+ID4gKwlzdHJ1
Y3QgaW14Nl9wY2llICppbXg2X3BjaWUgPSBkZXZfZ2V0X2RydmRhdGEoZGV2KTsKPiA+ICsJc3Ry
dWN0IHBjaWVfcG9ydCAqcHAgPSAmaW14Nl9wY2llLT5wY2ktPnBwOwo+ICsKPiA+ICsJaWYgKGlt
eDZfcGNpZS0+dmFyaWFudCAhPSBJTVg3RCkKPiA+ICsJCXJldHVybiAwOwo+ICsKPiA+ICsJaW14
Nl9wY2llX2Fzc2VydF9jb3JlX3Jlc2V0KGlteDZfcGNpZSk7Cj4gPiArCWlteDZfcGNpZV9pbml0
X3BoeShpbXg2X3BjaWUpOwo+ID4gKwlpbXg2X3BjaWVfZGVhc3NlcnRfY29yZV9yZXNldChpbXg2
X3BjaWUpOwo+ID4gKwlkd19wY2llX3NldHVwX3JjKHBwKTsKPiArCj4gPiArCXJldCA9IGlteDZf
cGNpZV9lc3RhYmxpc2hfbGluayhpbXg2X3BjaWUpOwo+ID4gKwlpZiAocmV0IDwgMCkKPiArCQlw
cl9lcnIoInBjaWUgbGluayBpcyBkb3duIGFmdGVyIHJlc3VtZS5cbiIpOwoKZGV2X2VycigpLCBw
bGVhc2UuCgpSZWdhcmRzLApMdWNhcwoKPiArCj4gPiArCXJldHVybiAwOwo+ICt9Cj4gKyNlbmRp
Zgo+ICsKPiArc3RhdGljIGNvbnN0IHN0cnVjdCBkZXZfcG1fb3BzIGlteDZfcGNpZV9wbV9vcHMg
PSB7Cj4gPiArCVNFVF9OT0lSUV9TWVNURU1fU0xFRVBfUE1fT1BTKGlteDZfcGNpZV9zdXNwZW5k
X25vaXJxLAo+ID4gKwkJCQnCoMKgwqDCoMKgwqBpbXg2X3BjaWVfcmVzdW1lX25vaXJxKQo+ICt9
Owo+ICsKPiDCoHN0YXRpYyBpbnQgaW14Nl9wY2llX3Byb2JlKHN0cnVjdCBwbGF0Zm9ybV9kZXZp
Y2UgKnBkZXYpCj4gwqB7Cj4gPiDCoAlzdHJ1Y3QgZGV2aWNlICpkZXYgPSAmcGRldi0+ZGV2Owo+
ID4gwqAJc3RydWN0IGR3X3BjaWUgKnBjaTsKPiA+IMKgCXN0cnVjdCBpbXg2X3BjaWUgKmlteDZf
cGNpZTsKPiBAQCAtODQ3LDEwICs5MzEsMTEgQEAgc3RhdGljIGNvbnN0IHN0cnVjdCBvZl9kZXZp
Y2VfaWQgaW14Nl9wY2llX29mX21hdGNoW10gPSB7Cj4gwqBzdGF0aWMgc3RydWN0IHBsYXRmb3Jt
X2RyaXZlciBpbXg2X3BjaWVfZHJpdmVyID0gewo+ID4gwqAJLmRyaXZlciA9IHsKPiA+ID4gwqAJ
CS5uYW1lCT0gImlteDZxLXBjaWUiLAo+ID4gwqAJCS5vZl9tYXRjaF90YWJsZSA9IGlteDZfcGNp
ZV9vZl9tYXRjaCwKPiA+IMKgCQkuc3VwcHJlc3NfYmluZF9hdHRycyA9IHRydWUsCj4gPiArCQku
cG0gPSAmaW14Nl9wY2llX3BtX29wcywKPiA+IMKgCX0sCj4gPiDCoAkucHJvYmXCoMKgwqDCoD0g
aW14Nl9wY2llX3Byb2JlLAo+ID4gwqAJLnNodXRkb3duID0gaW14Nl9wY2llX3NodXRkb3duLAo+
IMKgfTsKPiDCoAoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
X18KbGludXgtYXJtLWtlcm5lbCBtYWlsaW5nIGxpc3QKbGludXgtYXJtLWtlcm5lbEBsaXN0cy5p
bmZyYWRlYWQub3JnCmh0dHA6Ly9saXN0cy5pbmZyYWRlYWQub3JnL21haWxtYW4vbGlzdGluZm8v
bGludXgtYXJtLWtlcm5lbAo=

[PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Lucas Stach]

Hi Leonard,

Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> On imx7d the pcie-phy power domain is turned off in suspend and this can
> make the system hang after resume when attempting any read from PCI.
> 
> Fix this by adding minimal suspend/resume code from the nxp internal
> tree. This will prepare for powering down on suspend and reset the block
> on resume.
> 
> Code is only for imx7d but a very similar sequence can be used for
> other socs.
> 
> > The original author is mostly Richard Zhu <hongxing.zhu@nxp.com>, this
> patch adjusts the code to the upstream imx7d implemention using reset
> controls and power domains.
> 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
> ?drivers/pci/controller/dwc/pci-imx6.c | 95 +++++++++++++++++++++++++--
> ?1 file changed, 90 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 80f604602783..daebee905108 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -540,10 +540,27 @@ static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie)
> ?
> > ?	dev_err(dev, "Speed change timeout\n");
> > ?	return -EINVAL;
> ?}
> ?
> +static void imx6_pcie_ltssm_enable(struct device *dev)
> +{
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> +
> > +	switch (imx6_pcie->variant) {
> > +	case IMX6Q:
> > +	case IMX6SX:
> > +	case IMX6QP:
> > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > +				???IMX6Q_GPR12_PCIE_CTL_2,
> > +				???IMX6Q_GPR12_PCIE_CTL_2);
> > +		break;
> > +	case IMX7D:
> > +		reset_control_deassert(imx6_pcie->apps_reset);
> > +	}
> +}
> +
> ?static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie)
> ?{
> > ?	struct dw_pcie *pci = imx6_pcie->pci;
> > ?	struct device *dev = pci->dev;
> > ?	u32 tmp;
> @@ -558,15 +575,11 @@ static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie)
> > ?	tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK;
> > ?	tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1;
> > ?	dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp);
> ?
> > ?	/* Start LTSSM. */
> > -	if (imx6_pcie->variant == IMX7D)
> > -		reset_control_deassert(imx6_pcie->apps_reset);
> > -	else
> > -		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > -				???IMX6Q_GPR12_PCIE_CTL_2, 1 << 10);
> > +	imx6_pcie_ltssm_enable(dev);
> ?
> > ?	ret = imx6_pcie_wait_for_link(imx6_pcie);
> > ?	if (ret)
> > ?		goto err_reset_phy;
> ?
> @@ -681,10 +694,81 @@ static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie,
> ?
> ?static const struct dw_pcie_ops dw_pcie_ops = {
> > ?	.link_up = imx6_pcie_link_up,
> ?};
> ?
> +#ifdef CONFIG_PM_SLEEP
> +static void imx6_pcie_ltssm_disable(struct device *dev)
> +{
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> +
> > +	switch (imx6_pcie->variant) {
> > +	case IMX6Q:
> > +	case IMX6SX:
> > +	case IMX6QP:
> > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> +				???IMX6Q_GPR12_PCIE_CTL_2, 0);

Has this been tested on i.MX6? LTSSM disable requires a more complex
sequence on this SoC to avoid hanging the system. See commit
3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling
it".

Note that implementing the correct sequence requires the core clocks to
  still be on when disabling LTSSM, so would need to switch ordering of
the function calls in?imx6_pcie_suspend_noirq.

> +		break;
> > +	case IMX7D:
> > +		reset_control_assert(imx6_pcie->apps_reset);
> > +		break;
> > +	}
> +}
> +
> +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);
> +
> > +	if (imx6_pcie->variant == IMX7D) {
> > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > +				???IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> > +				???IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> > +	}
> +}
> +
> +static int imx6_pcie_suspend_noirq(struct device *dev)
> +{
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> +
> > +	if (imx6_pcie->variant != IMX7D)
> > +		return 0;
> +
> > +	imx6_pcie_clk_disable(imx6_pcie);
> > +	imx6_pcie_ltssm_disable(dev);
> +
> > +	return 0;
> +}
> +
> +static int imx6_pcie_resume_noirq(struct device *dev)
> +{
> > +	int ret = 0;
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > +	struct pcie_port *pp = &imx6_pcie->pci->pp;
> +
> > +	if (imx6_pcie->variant != IMX7D)
> > +		return 0;
> +
> > +	imx6_pcie_assert_core_reset(imx6_pcie);
> > +	imx6_pcie_init_phy(imx6_pcie);
> > +	imx6_pcie_deassert_core_reset(imx6_pcie);
> > +	dw_pcie_setup_rc(pp);
> +
> > +	ret = imx6_pcie_establish_link(imx6_pcie);
> > +	if (ret < 0)
> +		pr_err("pcie link is down after resume.\n");

dev_err(), please.

Regards,
Lucas

> +
> > +	return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops imx6_pcie_pm_ops = {
> > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx6_pcie_suspend_noirq,
> > +				??????imx6_pcie_resume_noirq)
> +};
> +
> ?static int imx6_pcie_probe(struct platform_device *pdev)
> ?{
> > ?	struct device *dev = &pdev->dev;
> > ?	struct dw_pcie *pci;
> > ?	struct imx6_pcie *imx6_pcie;
> @@ -847,10 +931,11 @@ static const struct of_device_id imx6_pcie_of_match[] = {
> ?static struct platform_driver imx6_pcie_driver = {
> > ?	.driver = {
> > > ?		.name	= "imx6q-pcie",
> > ?		.of_match_table = imx6_pcie_of_match,
> > ?		.suppress_bind_attrs = true,
> > +		.pm = &imx6_pcie_pm_ops,
> > ?	},
> > ?	.probe????= imx6_pcie_probe,
> > ?	.shutdown = imx6_pcie_shutdown,
> ?};
> ?

Re: [PATCH v2 2/3] reset: imx7: Fix always writing bits as 0

[Lucas Stach]

As this doesn't depend on any other patch in this series, I think it
would be fine if Philipp takes this patch through the reset tree.

Regards,
Lucas

Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> Right now the only user of reset-imx7 is pci-imx6 and the
> reset_control_assert and deassert calls on pciephy_reset don't toggle
> the PCIEPHY_BTN and PCIEPHY_G_RST bits as expected. Fix this by writing
> 1 or 0 respectively.
> 
> The reference manual is not very clear regarding SRC_PCIEPHY_RCR but for
> other registers like MIPIPHY and HSICPHY the bits are explicitly
> documented as "1 means assert, 0 means deassert".
> 
> The values are still reversed for IMX7_RESET_PCIE_CTRL_APPS_EN.
> 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/reset/reset-imx7.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> index 4db177bc89bc..fdeac1946429 100644
> --- a/drivers/reset/reset-imx7.c
> +++ b/drivers/reset/reset-imx7.c
> @@ -78,11 +78,11 @@ static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
>  static int imx7_reset_set(struct reset_controller_dev *rcdev,
> >  			  unsigned long id, bool assert)
>  {
> >  	struct imx7_src *imx7src = to_imx7_src(rcdev);
> >  	const struct imx7_src_signal *signal = &imx7_src_signals[id];
> > -	unsigned int value = 0;
> > +	unsigned int value = assert ? signal->bit : 0;
>  
> >  	switch (id) {
> >  	case IMX7_RESET_PCIEPHY:
> >  		/*
> >  		 * wait for more than 10us to release phy g_rst and

Re: [PATCH v2 2/3] reset: imx7: Fix always writing bits as 0

[Lucas Stach]

QXMgdGhpcyBkb2Vzbid0IGRlcGVuZCBvbiBhbnkgb3RoZXIgcGF0Y2ggaW4gdGhpcyBzZXJpZXMs
IEkgdGhpbmsgaXQKd291bGQgYmUgZmluZSBpZiBQaGlsaXBwwqB0YWtlcyB0aGlzIHBhdGNoIHRo
cm91Z2ggdGhlIHJlc2V0IHRyZWUuCgpSZWdhcmRzLApMdWNhcwoKQW0gRnJlaXRhZywgZGVuIDIw
LjA3LjIwMTgsIDE1OjQ3ICswMzAwIHNjaHJpZWIgTGVvbmFyZCBDcmVzdGV6Ogo+IFJpZ2h0IG5v
dyB0aGUgb25seSB1c2VyIG9mIHJlc2V0LWlteDcgaXMgcGNpLWlteDYgYW5kIHRoZQo+IHJlc2V0
X2NvbnRyb2xfYXNzZXJ0IGFuZCBkZWFzc2VydCBjYWxscyBvbiBwY2llcGh5X3Jlc2V0IGRvbid0
IHRvZ2dsZQo+IHRoZSBQQ0lFUEhZX0JUTiBhbmQgUENJRVBIWV9HX1JTVCBiaXRzIGFzIGV4cGVj
dGVkLiBGaXggdGhpcyBieSB3cml0aW5nCj4gMSBvciAwIHJlc3BlY3RpdmVseS4KPiAKPiBUaGUg
cmVmZXJlbmNlIG1hbnVhbCBpcyBub3QgdmVyeSBjbGVhciByZWdhcmRpbmcgU1JDX1BDSUVQSFlf
UkNSIGJ1dCBmb3IKPiBvdGhlciByZWdpc3RlcnMgbGlrZSBNSVBJUEhZIGFuZCBIU0lDUEhZIHRo
ZSBiaXRzIGFyZSBleHBsaWNpdGx5Cj4gZG9jdW1lbnRlZCBhcyAiMSBtZWFucyBhc3NlcnQsIDAg
bWVhbnMgZGVhc3NlcnQiLgo+IAo+IFRoZSB2YWx1ZXMgYXJlIHN0aWxsIHJldmVyc2VkIGZvciBJ
TVg3X1JFU0VUX1BDSUVfQ1RSTF9BUFBTX0VOLgo+IAo+ID4gU2lnbmVkLW9mZi1ieTogTGVvbmFy
ZCBDcmVzdGV6IDxsZW9uYXJkLmNyZXN0ZXpAbnhwLmNvbT4KPiA+IFJldmlld2VkLWJ5OiBMdWNh
cyBTdGFjaCA8bC5zdGFjaEBwZW5ndXRyb25peC5kZT4KPiAtLS0KPiDCoGRyaXZlcnMvcmVzZXQv
cmVzZXQtaW14Ny5jIHwgMiArLQo+IMKgMSBmaWxlIGNoYW5nZWQsIDEgaW5zZXJ0aW9uKCspLCAx
IGRlbGV0aW9uKC0pCj4gCj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvcmVzZXQvcmVzZXQtaW14Ny5j
IGIvZHJpdmVycy9yZXNldC9yZXNldC1pbXg3LmMKPiBpbmRleCA0ZGIxNzdiYzg5YmMuLmZkZWFj
MTk0NjQyOSAxMDA2NDQKPiAtLS0gYS9kcml2ZXJzL3Jlc2V0L3Jlc2V0LWlteDcuYwo+ICsrKyBi
L2RyaXZlcnMvcmVzZXQvcmVzZXQtaW14Ny5jCj4gQEAgLTc4LDExICs3OCwxMSBAQCBzdGF0aWMg
c3RydWN0IGlteDdfc3JjICp0b19pbXg3X3NyYyhzdHJ1Y3QgcmVzZXRfY29udHJvbGxlcl9kZXYg
KnJjZGV2KQo+IMKgc3RhdGljIGludCBpbXg3X3Jlc2V0X3NldChzdHJ1Y3QgcmVzZXRfY29udHJv
bGxlcl9kZXYgKnJjZGV2LAo+ID4gwqAJCQnCoMKgdW5zaWduZWQgbG9uZyBpZCwgYm9vbCBhc3Nl
cnQpCj4gwqB7Cj4gPiDCoAlzdHJ1Y3QgaW14N19zcmMgKmlteDdzcmMgPSB0b19pbXg3X3NyYyhy
Y2Rldik7Cj4gPiDCoAljb25zdCBzdHJ1Y3QgaW14N19zcmNfc2lnbmFsICpzaWduYWwgPSAmaW14
N19zcmNfc2lnbmFsc1tpZF07Cj4gPiAtCXVuc2lnbmVkIGludCB2YWx1ZSA9IDA7Cj4gPiArCXVu
c2lnbmVkIGludCB2YWx1ZSA9IGFzc2VydCA/IHNpZ25hbC0+Yml0IDogMDsKPiDCoAo+ID4gwqAJ
c3dpdGNoIChpZCkgewo+ID4gwqAJY2FzZSBJTVg3X1JFU0VUX1BDSUVQSFk6Cj4gPiDCoAkJLyoK
PiA+IMKgCQnCoCogd2FpdCBmb3IgbW9yZSB0aGFuIDEwdXMgdG8gcmVsZWFzZSBwaHkgZ19yc3Qg
YW5kCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpsaW51
eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdApsaW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJhZGVh
ZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5mby9saW51eC1h
cm0ta2VybmVsCg==

[PATCH v2 2/3] reset: imx7: Fix always writing bits as 0

[Lucas Stach]

As this doesn't depend on any other patch in this series, I think it
would be fine if Philipp?takes this patch through the reset tree.

Regards,
Lucas

Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> Right now the only user of reset-imx7 is pci-imx6 and the
> reset_control_assert and deassert calls on pciephy_reset don't toggle
> the PCIEPHY_BTN and PCIEPHY_G_RST bits as expected. Fix this by writing
> 1 or 0 respectively.
> 
> The reference manual is not very clear regarding SRC_PCIEPHY_RCR but for
> other registers like MIPIPHY and HSICPHY the bits are explicitly
> documented as "1 means assert, 0 means deassert".
> 
> The values are still reversed for IMX7_RESET_PCIE_CTRL_APPS_EN.
> 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> ?drivers/reset/reset-imx7.c | 2 +-
> ?1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> index 4db177bc89bc..fdeac1946429 100644
> --- a/drivers/reset/reset-imx7.c
> +++ b/drivers/reset/reset-imx7.c
> @@ -78,11 +78,11 @@ static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
> ?static int imx7_reset_set(struct reset_controller_dev *rcdev,
> > ?			??unsigned long id, bool assert)
> ?{
> > ?	struct imx7_src *imx7src = to_imx7_src(rcdev);
> > ?	const struct imx7_src_signal *signal = &imx7_src_signals[id];
> > -	unsigned int value = 0;
> > +	unsigned int value = assert ? signal->bit : 0;
> ?
> > ?	switch (id) {
> > ?	case IMX7_RESET_PCIEPHY:
> > ?		/*
> > ?		?* wait for more than 10us to release phy g_rst and

Re: [PATCH v2 2/3] reset: imx7: Fix always writing bits as 0

[Philipp Zabel]

On Mon, 2018-07-23 at 11:41 +0200, Lucas Stach wrote:
> As this doesn't depend on any other patch in this series, I think it
> would be fine if Philipp takes this patch through the reset tree.
> 
> Regards,
> Lucas
> 
> Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> > Right now the only user of reset-imx7 is pci-imx6 and the
> > reset_control_assert and deassert calls on pciephy_reset don't toggle
> > the PCIEPHY_BTN and PCIEPHY_G_RST bits as expected. Fix this by writing
> > 1 or 0 respectively.
> > 
> > The reference manual is not very clear regarding SRC_PCIEPHY_RCR but for
> > other registers like MIPIPHY and HSICPHY the bits are explicitly
> > documented as "1 means assert, 0 means deassert".
> > 
> > The values are still reversed for IMX7_RESET_PCIE_CTRL_APPS_EN.
> > 
> > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > > Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

Thank you, applied to reset/fixes.

regards
Philipp

Re: [PATCH v2 2/3] reset: imx7: Fix always writing bits as 0

[Philipp Zabel]

T24gTW9uLCAyMDE4LTA3LTIzIGF0IDExOjQxICswMjAwLCBMdWNhcyBTdGFjaCB3cm90ZToKPiBB
cyB0aGlzIGRvZXNuJ3QgZGVwZW5kIG9uIGFueSBvdGhlciBwYXRjaCBpbiB0aGlzIHNlcmllcywg
SSB0aGluayBpdAo+IHdvdWxkIGJlIGZpbmUgaWYgUGhpbGlwcMKgdGFrZXMgdGhpcyBwYXRjaCB0
aHJvdWdoIHRoZSByZXNldCB0cmVlLgo+IAo+IFJlZ2FyZHMsCj4gTHVjYXMKPiAKPiBBbSBGcmVp
dGFnLCBkZW4gMjAuMDcuMjAxOCwgMTU6NDcgKzAzMDAgc2NocmllYiBMZW9uYXJkIENyZXN0ZXo6
Cj4gPiBSaWdodCBub3cgdGhlIG9ubHkgdXNlciBvZiByZXNldC1pbXg3IGlzIHBjaS1pbXg2IGFu
ZCB0aGUKPiA+IHJlc2V0X2NvbnRyb2xfYXNzZXJ0IGFuZCBkZWFzc2VydCBjYWxscyBvbiBwY2ll
cGh5X3Jlc2V0IGRvbid0IHRvZ2dsZQo+ID4gdGhlIFBDSUVQSFlfQlROIGFuZCBQQ0lFUEhZX0df
UlNUIGJpdHMgYXMgZXhwZWN0ZWQuIEZpeCB0aGlzIGJ5IHdyaXRpbmcKPiA+IDEgb3IgMCByZXNw
ZWN0aXZlbHkuCj4gPiAKPiA+IFRoZSByZWZlcmVuY2UgbWFudWFsIGlzIG5vdCB2ZXJ5IGNsZWFy
IHJlZ2FyZGluZyBTUkNfUENJRVBIWV9SQ1IgYnV0IGZvcgo+ID4gb3RoZXIgcmVnaXN0ZXJzIGxp
a2UgTUlQSVBIWSBhbmQgSFNJQ1BIWSB0aGUgYml0cyBhcmUgZXhwbGljaXRseQo+ID4gZG9jdW1l
bnRlZCBhcyAiMSBtZWFucyBhc3NlcnQsIDAgbWVhbnMgZGVhc3NlcnQiLgo+ID4gCj4gPiBUaGUg
dmFsdWVzIGFyZSBzdGlsbCByZXZlcnNlZCBmb3IgSU1YN19SRVNFVF9QQ0lFX0NUUkxfQVBQU19F
Ti4KPiA+IAo+ID4gPiBTaWduZWQtb2ZmLWJ5OiBMZW9uYXJkIENyZXN0ZXogPGxlb25hcmQuY3Jl
c3RlekBueHAuY29tPgo+ID4gPiBSZXZpZXdlZC1ieTogTHVjYXMgU3RhY2ggPGwuc3RhY2hAcGVu
Z3V0cm9uaXguZGU+CgpUaGFuayB5b3UsIGFwcGxpZWQgdG8gcmVzZXQvZml4ZXMuCgpyZWdhcmRz
ClBoaWxpcHAKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
CmxpbnV4LWFybS1rZXJuZWwgbWFpbGluZyBsaXN0CmxpbnV4LWFybS1rZXJuZWxAbGlzdHMuaW5m
cmFkZWFkLm9yZwpodHRwOi8vbGlzdHMuaW5mcmFkZWFkLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2xp
bnV4LWFybS1rZXJuZWwK

[PATCH v2 2/3] reset: imx7: Fix always writing bits as 0

[Philipp Zabel]

On Mon, 2018-07-23 at 11:41 +0200, Lucas Stach wrote:
> As this doesn't depend on any other patch in this series, I think it
> would be fine if Philipp?takes this patch through the reset tree.
> 
> Regards,
> Lucas
> 
> Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> > Right now the only user of reset-imx7 is pci-imx6 and the
> > reset_control_assert and deassert calls on pciephy_reset don't toggle
> > the PCIEPHY_BTN and PCIEPHY_G_RST bits as expected. Fix this by writing
> > 1 or 0 respectively.
> > 
> > The reference manual is not very clear regarding SRC_PCIEPHY_RCR but for
> > other registers like MIPIPHY and HSICPHY the bits are explicitly
> > documented as "1 means assert, 0 means deassert".
> > 
> > The values are still reversed for IMX7_RESET_PCIE_CTRL_APPS_EN.
> > 
> > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > > Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

Thank you, applied to reset/fixes.

regards
Philipp

Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Leonard Crestez]

On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote:
> Hi Leonard,
> 
> Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> > On imx7d the pcie-phy power domain is turned off in suspend and this can
> > make the system hang after resume when attempting any read from PCI.
> > 
> > Fix this by adding minimal suspend/resume code from the nxp internal
> > tree. This will prepare for powering down on suspend and reset the block
> > on resume.
> > 
> > Code is only for imx7d but a very similar sequence can be used for
> > other socs.
> > 
> > +static void imx6_pcie_ltssm_disable(struct device *dev)
> > +{
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > +
> > +	switch (imx6_pcie->variant) {
> > +	case IMX6Q:
> > +	case IMX6SX:
> > +	case IMX6QP:
> > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > +				   IMX6Q_GPR12_PCIE_CTL_2, 0);
> 
> Has this been tested on i.MX6? LTSSM disable requires a more complex
> sequence on this SoC to avoid hanging the system. See commit
> 3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling
> it".

This patch only enables suspend/resume for imx7d with other SOCs to
follow later. The ltssm_disable function is just symmetric with
ltssm_enable.

The 6Q parts are affected by errata "ERR005723 PCIe: PCIe does not
support L2 power down [i.MX 6Dual/6Quad Only]".

This design error seems to have the same root cause as your problem (no
dedicated reset control) so this works out quite nicely: the solution
is to never power down pci on affected chips.

> > +static int imx6_pcie_resume_noirq(struct device *dev)
> > +{
> > +	int ret = 0;
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > +	struct pcie_port *pp = &imx6_pcie->pci->pp;
> > +
> > +	if (imx6_pcie->variant != IMX7D)
> > +		return 0;
> > +
> > +	imx6_pcie_assert_core_reset(imx6_pcie);
> > +	imx6_pcie_init_phy(imx6_pcie);
> > +	imx6_pcie_deassert_core_reset(imx6_pcie);
> > +	dw_pcie_setup_rc(pp);
> > +
> > +	ret = imx6_pcie_establish_link(imx6_pcie);
> > +	if (ret < 0)
> > +		pr_err("pcie link is down after resume.\n");
> 
> dev_err(), please.

The imx6_pcie_establish_link function already seems to link error
information so the message could be dropped. However it's still helpful
to know that those pci link errors are specifically related to resume.

Fabio suggested I propagate the return code but I'm not sure that's
helpful since "link down" is what happens when the slot is empty and
this is clearly not a "error" or "failure". It's not clear if "slot
empty" can be distinguished in some way.

I'll switch to dev_info and drop the error code, is this OK?


One aspect that I skipped is PME_Turn_Off support: The PCI standard
mandates that this is sent before entering L2/L3 and the designware
core supports this but it's not part of this patch.

Is it fine if I post this separately or should it be part of the same
series? The turnoff bit is in IOMUX gpr for imx6 but SRC for imx7 so it
would require additional patches in reset, dts and then pci.

Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Leonard Crestez]

On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote:
> Hi Leonard,
> 
> Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> > On imx7d the pcie-phy power domain is turned off in suspend and this can
> > make the system hang after resume when attempting any read from PCI.
> > 
> > Fix this by adding minimal suspend/resume code from the nxp internal
> > tree. This will prepare for powering down on suspend and reset the block
> > on resume.
> > 
> > Code is only for imx7d but a very similar sequence can be used for
> > other socs.
> > 
> > +static void imx6_pcie_ltssm_disable(struct device *dev)
> > +{
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > +
> > +	switch (imx6_pcie->variant) {
> > +	case IMX6Q:
> > +	case IMX6SX:
> > +	case IMX6QP:
> > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > +				   IMX6Q_GPR12_PCIE_CTL_2, 0);
> 
> Has this been tested on i.MX6? LTSSM disable requires a more complex
> sequence on this SoC to avoid hanging the system. See commit
> 3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling
> it".

This patch only enables suspend/resume for imx7d with other SOCs to
follow later. The ltssm_disable function is just symmetric with
ltssm_enable.

The 6Q parts are affected by errata "ERR005723 PCIe: PCIe does not
support L2 power down [i.MX 6Dual/6Quad Only]".

This design error seems to have the same root cause as your problem (no
dedicated reset control) so this works out quite nicely: the solution
is to never power down pci on affected chips.

> > +static int imx6_pcie_resume_noirq(struct device *dev)
> > +{
> > +	int ret = 0;
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > +	struct pcie_port *pp = &imx6_pcie->pci->pp;
> > +
> > +	if (imx6_pcie->variant != IMX7D)
> > +		return 0;
> > +
> > +	imx6_pcie_assert_core_reset(imx6_pcie);
> > +	imx6_pcie_init_phy(imx6_pcie);
> > +	imx6_pcie_deassert_core_reset(imx6_pcie);
> > +	dw_pcie_setup_rc(pp);
> > +
> > +	ret = imx6_pcie_establish_link(imx6_pcie);
> > +	if (ret < 0)
> > +		pr_err("pcie link is down after resume.\n");
> 
> dev_err(), please.

The imx6_pcie_establish_link function already seems to link error
information so the message could be dropped. However it's still helpful
to know that those pci link errors are specifically related to resume.

Fabio suggested I propagate the return code but I'm not sure that's
helpful since "link down" is what happens when the slot is empty and
this is clearly not a "error" or "failure". It's not clear if "slot
empty" can be distinguished in some way.

I'll switch to dev_info and drop the error code, is this OK?


One aspect that I skipped is PME_Turn_Off support: The PCI standard
mandates that this is sent before entering L2/L3 and the designware
core supports this but it's not part of this patch.

Is it fine if I post this separately or should it be part of the same
series? The turnoff bit is in IOMUX gpr for imx6 but SRC for imx7 so it
would require additional patches in reset, dts and then pci.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Leonard Crestez]

On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote:
> Hi Leonard,
> 
> Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> > On imx7d the pcie-phy power domain is turned off in suspend and this can
> > make the system hang after resume when attempting any read from PCI.
> > 
> > Fix this by adding minimal suspend/resume code from the nxp internal
> > tree. This will prepare for powering down on suspend and reset the block
> > on resume.
> > 
> > Code is only for imx7d but a very similar sequence can be used for
> > other socs.
> > 
> > +static void imx6_pcie_ltssm_disable(struct device *dev)
> > +{
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > +
> > +	switch (imx6_pcie->variant) {
> > +	case IMX6Q:
> > +	case IMX6SX:
> > +	case IMX6QP:
> > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > +				   IMX6Q_GPR12_PCIE_CTL_2, 0);
> 
> Has this been tested on i.MX6? LTSSM disable requires a more complex
> sequence on this SoC to avoid hanging the system. See commit
> 3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling
> it".

This patch only enables suspend/resume for imx7d with other SOCs to
follow later. The ltssm_disable function is just symmetric with
ltssm_enable.

The 6Q parts are affected by errata "ERR005723 PCIe: PCIe does not
support L2 power down [i.MX 6Dual/6Quad Only]".

This design error seems to have the same root cause as your problem (no
dedicated reset control) so this works out quite nicely: the solution
is to never power down pci on affected chips.

> > +static int imx6_pcie_resume_noirq(struct device *dev)
> > +{
> > +	int ret = 0;
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > +	struct pcie_port *pp = &imx6_pcie->pci->pp;
> > +
> > +	if (imx6_pcie->variant != IMX7D)
> > +		return 0;
> > +
> > +	imx6_pcie_assert_core_reset(imx6_pcie);
> > +	imx6_pcie_init_phy(imx6_pcie);
> > +	imx6_pcie_deassert_core_reset(imx6_pcie);
> > +	dw_pcie_setup_rc(pp);
> > +
> > +	ret = imx6_pcie_establish_link(imx6_pcie);
> > +	if (ret < 0)
> > +		pr_err("pcie link is down after resume.\n");
> 
> dev_err(), please.

The imx6_pcie_establish_link function already seems to link error
information so the message could be dropped. However it's still helpful
to know that those pci link errors are specifically related to resume.

Fabio suggested I propagate the return code but I'm not sure that's
helpful since "link down" is what happens when the slot is empty and
this is clearly not a "error" or "failure". It's not clear if "slot
empty" can be distinguished in some way.

I'll switch to dev_info and drop the error code, is this OK?


One aspect that I skipped is PME_Turn_Off support: The PCI standard
mandates that this is sent before entering L2/L3 and the designware
core supports this but it's not part of this patch.

Is it fine if I post this separately or should it be part of the same
series? The turnoff bit is in IOMUX gpr for imx6 but SRC for imx7 so it
would require additional patches in reset, dts and then pci.

[PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Leonard Crestez]

On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote:
> Hi Leonard,
> 
> Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> > On imx7d the pcie-phy power domain is turned off in suspend and this can
> > make the system hang after resume when attempting any read from PCI.
> > 
> > Fix this by adding minimal suspend/resume code from the nxp internal
> > tree. This will prepare for powering down on suspend and reset the block
> > on resume.
> > 
> > Code is only for imx7d but a very similar sequence can be used for
> > other socs.
> > 
> > +static void imx6_pcie_ltssm_disable(struct device *dev)
> > +{
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > +
> > +	switch (imx6_pcie->variant) {
> > +	case IMX6Q:
> > +	case IMX6SX:
> > +	case IMX6QP:
> > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > +				   IMX6Q_GPR12_PCIE_CTL_2, 0);
> 
> Has this been tested on i.MX6? LTSSM disable requires a more complex
> sequence on this SoC to avoid hanging the system. See commit
> 3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling
> it".

This patch only enables suspend/resume for imx7d with other SOCs to
follow later. The ltssm_disable function is just symmetric with
ltssm_enable.

The 6Q parts are affected by errata "ERR005723 PCIe: PCIe does not
support L2 power down [i.MX 6Dual/6Quad Only]".

This design error seems to have the same root cause as your problem (no
dedicated reset control) so this works out quite nicely: the solution
is to never power down pci on affected chips.

> > +static int imx6_pcie_resume_noirq(struct device *dev)
> > +{
> > +	int ret = 0;
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > +	struct pcie_port *pp = &imx6_pcie->pci->pp;
> > +
> > +	if (imx6_pcie->variant != IMX7D)
> > +		return 0;
> > +
> > +	imx6_pcie_assert_core_reset(imx6_pcie);
> > +	imx6_pcie_init_phy(imx6_pcie);
> > +	imx6_pcie_deassert_core_reset(imx6_pcie);
> > +	dw_pcie_setup_rc(pp);
> > +
> > +	ret = imx6_pcie_establish_link(imx6_pcie);
> > +	if (ret < 0)
> > +		pr_err("pcie link is down after resume.\n");
> 
> dev_err(), please.

The imx6_pcie_establish_link function already seems to link error
information so the message could be dropped. However it's still helpful
to know that those pci link errors are specifically related to resume.

Fabio suggested I propagate the return code but I'm not sure that's
helpful since "link down" is what happens when the slot is empty and
this is clearly not a "error" or "failure". It's not clear if "slot
empty" can be distinguished in some way.

I'll switch to dev_info and drop the error code, is this OK?


One aspect that I skipped is PME_Turn_Off support: The PCI standard
mandates that this is sent before entering L2/L3 and the designware
core supports this but it's not part of this patch.

Is it fine if I post this separately or should it be part of the same
series? The turnoff bit is in IOMUX gpr for imx6 but SRC for imx7 so it
would require additional patches in reset, dts and then pci.

Re: [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"

[Leonard Crestez]

On Fri, 2018-07-20 at 08:33 -0700, Andrey Smirnov wrote:
> On Fri, Jul 20, 2018 at 5:48 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > 
> > This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d.
> > 
> > That commit followed the reference manual but unfortunately the imx7d
> > manual is incorrect.
> 
> I'd also add similar comment to DT file to prevent people from trying
> to "fix" this in the future.

I'll try to see if I can follow up internally with docs team to get
this updated in the next revision of the reference manual.

> Also, this change is going to break QEMU's mapping found here:

I had no idea that existed, I guess somebody needs to fix that as well.

Do you have an imx7d board using pci or did you just test in emulation?

--
Regards,
Leonard

Re: [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"

[Leonard Crestez]

On Fri, 2018-07-20 at 08:33 -0700, Andrey Smirnov wrote:
> On Fri, Jul 20, 2018 at 5:48 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > 
> > This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d.
> > 
> > That commit followed the reference manual but unfortunately the imx7d
> > manual is incorrect.
> 
> I'd also add similar comment to DT file to prevent people from trying
> to "fix" this in the future.

I'll try to see if I can follow up internally with docs team to get
this updated in the next revision of the reference manual.

> Also, this change is going to break QEMU's mapping found here:

I had no idea that existed, I guess somebody needs to fix that as well.

Do you have an imx7d board using pci or did you just test in emulation?

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

Re: [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"

[Leonard Crestez]

On Fri, 2018-07-20 at 08:33 -0700, Andrey Smirnov wrote:
> On Fri, Jul 20, 2018 at 5:48 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > 
> > This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d.
> > 
> > That commit followed the reference manual but unfortunately the imx7d
> > manual is incorrect.
> 
> I'd also add similar comment to DT file to prevent people from trying
> to "fix" this in the future.

I'll try to see if I can follow up internally with docs team to get
this updated in the next revision of the reference manual.

> Also, this change is going to break QEMU's mapping found here:

I had no idea that existed, I guess somebody needs to fix that as well.

Do you have an imx7d board using pci or did you just test in emulation?

--
Regards,
Leonard

[PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"

[Leonard Crestez]

On Fri, 2018-07-20 at 08:33 -0700, Andrey Smirnov wrote:
> On Fri, Jul 20, 2018 at 5:48 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > 
> > This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d.
> > 
> > That commit followed the reference manual but unfortunately the imx7d
> > manual is incorrect.
> 
> I'd also add similar comment to DT file to prevent people from trying
> to "fix" this in the future.

I'll try to see if I can follow up internally with docs team to get
this updated in the next revision of the reference manual.

> Also, this change is going to break QEMU's mapping found here:

I had no idea that existed, I guess somebody needs to fix that as well.

Do you have an imx7d board using pci or did you just test in emulation?

--
Regards,
Leonard

Re: [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"

[Andrey Smirnov]

On Mon, Jul 23, 2018 at 5:41 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> On Fri, 2018-07-20 at 08:33 -0700, Andrey Smirnov wrote:
> > On Fri, Jul 20, 2018 at 5:48 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > >
> > > This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d.
> > >
> > > That commit followed the reference manual but unfortunately the imx7d
> > > manual is incorrect.
> >
> > I'd also add similar comment to DT file to prevent people from trying
> > to "fix" this in the future.
>
> I'll try to see if I can follow up internally with docs team to get
> this updated in the next revision of the reference manual.
>

Not sure if we are on the same page here, but what I meant is adding
the same explanation that is in your commit message to Device Tree
file as well so that if anyone looks at DT code and goes "Huh?" in the
future, they wouldn't have to research commit history to see the
reason why things the way they are.

> > Also, this change is going to break QEMU's mapping found here:
>
> I had no idea that existed, I guess somebody needs to fix that as well.
>

I take it it's a no to my "any chance you can fix that as well?" ;-).
I'll see if I can find time to do that this week.

> Do you have an imx7d board using pci or did you just test in emulation?
>

I used real i.MX7D Sabre board with i210 network card, when I was
porting i.MX7 PCIe patches. However, as per note I made in the
original submission:

https://lkml.org/lkml/2017/10/9/913

this IRQ swap patch came up as a result of "connecting" emulated ICH4
in QEMU which was using legacy interrupts.

Thanks,
Andrey Smirnov

Re: [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"

[Andrey Smirnov]

On Mon, Jul 23, 2018 at 5:41 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> On Fri, 2018-07-20 at 08:33 -0700, Andrey Smirnov wrote:
> > On Fri, Jul 20, 2018 at 5:48 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > >
> > > This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d.
> > >
> > > That commit followed the reference manual but unfortunately the imx7d
> > > manual is incorrect.
> >
> > I'd also add similar comment to DT file to prevent people from trying
> > to "fix" this in the future.
>
> I'll try to see if I can follow up internally with docs team to get
> this updated in the next revision of the reference manual.
>

Not sure if we are on the same page here, but what I meant is adding
the same explanation that is in your commit message to Device Tree
file as well so that if anyone looks at DT code and goes "Huh?" in the
future, they wouldn't have to research commit history to see the
reason why things the way they are.

> > Also, this change is going to break QEMU's mapping found here:
>
> I had no idea that existed, I guess somebody needs to fix that as well.
>

I take it it's a no to my "any chance you can fix that as well?" ;-).
I'll see if I can find time to do that this week.

> Do you have an imx7d board using pci or did you just test in emulation?
>

I used real i.MX7D Sabre board with i210 network card, when I was
porting i.MX7 PCIe patches. However, as per note I made in the
original submission:

https://lkml.org/lkml/2017/10/9/913

this IRQ swap patch came up as a result of "connecting" emulated ICH4
in QEMU which was using legacy interrupts.

Thanks,
Andrey Smirnov

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

[PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"

[Andrey Smirnov]

On Mon, Jul 23, 2018 at 5:41 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> On Fri, 2018-07-20 at 08:33 -0700, Andrey Smirnov wrote:
> > On Fri, Jul 20, 2018 at 5:48 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > >
> > > This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d.
> > >
> > > That commit followed the reference manual but unfortunately the imx7d
> > > manual is incorrect.
> >
> > I'd also add similar comment to DT file to prevent people from trying
> > to "fix" this in the future.
>
> I'll try to see if I can follow up internally with docs team to get
> this updated in the next revision of the reference manual.
>

Not sure if we are on the same page here, but what I meant is adding
the same explanation that is in your commit message to Device Tree
file as well so that if anyone looks at DT code and goes "Huh?" in the
future, they wouldn't have to research commit history to see the
reason why things the way they are.

> > Also, this change is going to break QEMU's mapping found here:
>
> I had no idea that existed, I guess somebody needs to fix that as well.
>

I take it it's a no to my "any chance you can fix that as well?" ;-).
I'll see if I can find time to do that this week.

> Do you have an imx7d board using pci or did you just test in emulation?
>

I used real i.MX7D Sabre board with i210 network card, when I was
porting i.MX7 PCIe patches. However, as per note I made in the
original submission:

https://lkml.org/lkml/2017/10/9/913

this IRQ swap patch came up as a result of "connecting" emulated ICH4
in QEMU which was using legacy interrupts.

Thanks,
Andrey Smirnov

Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Lucas Stach]

Am Montag, den 23.07.2018, 12:37 +0000 schrieb Leonard Crestez:
> On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote:
> > Hi Leonard,
> > 
> > Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> > > On imx7d the pcie-phy power domain is turned off in suspend and this can
> > > make the system hang after resume when attempting any read from PCI.
> > > 
> > > Fix this by adding minimal suspend/resume code from the nxp internal
> > > tree. This will prepare for powering down on suspend and reset the block
> > > on resume.
> > > 
> > > Code is only for imx7d but a very similar sequence can be used for
> > > other socs.
> > > 
> > > +static void imx6_pcie_ltssm_disable(struct device *dev)
> > > +{
> > > > > > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > > +
> > > > > > +	switch (imx6_pcie->variant) {
> > > > > > +	case IMX6Q:
> > > > > > +	case IMX6SX:
> > > > > > +	case IMX6QP:
> > > > > > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > > +				   IMX6Q_GPR12_PCIE_CTL_2, 0);
> > 
> > Has this been tested on i.MX6? LTSSM disable requires a more complex
> > sequence on this SoC to avoid hanging the system. See commit
> > 3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling
> > it".
> 
> This patch only enables suspend/resume for imx7d with other SOCs to
> follow later. The ltssm_disable function is just symmetric with
> ltssm_enable.
> 
> The 6Q parts are affected by errata "ERR005723 PCIe: PCIe does not
> support L2 power down [i.MX 6Dual/6Quad Only]".
> 
> This design error seems to have the same root cause as your problem (no
> dedicated reset control) so this works out quite nicely: the solution
> is to never power down pci on affected chips.

I don't quite like code that looks like it is doing the right thing,
but then doesn't. Can we at least emit a warning that there might be
dragons if anyone tries to call this on i.MX6?

> > > +static int imx6_pcie_resume_noirq(struct device *dev)
> > > +{
> > > > > > +	int ret = 0;
> > > > > > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > > > > > +	struct pcie_port *pp = &imx6_pcie->pci->pp;
> > > +
> > > > > > +	if (imx6_pcie->variant != IMX7D)
> > > > > > +		return 0;
> > > +
> > > > > > +	imx6_pcie_assert_core_reset(imx6_pcie);
> > > > > > +	imx6_pcie_init_phy(imx6_pcie);
> > > > > > +	imx6_pcie_deassert_core_reset(imx6_pcie);
> > > > > > +	dw_pcie_setup_rc(pp);
> > > +
> > > > > > +	ret = imx6_pcie_establish_link(imx6_pcie);
> > > > > > +	if (ret < 0)
> > > +		pr_err("pcie link is down after resume.\n");
> > 
> > dev_err(), please.
> 
> The imx6_pcie_establish_link function already seems to link error
> information so the message could be dropped. However it's still helpful
> to know that those pci link errors are specifically related to resume.
> 
> Fabio suggested I propagate the return code but I'm not sure that's
> helpful since "link down" is what happens when the slot is empty and
> this is clearly not a "error" or "failure". It's not clear if "slot
> empty" can be distinguished in some way.

I don't think we have a generic way to distinguish between the two
cases.

> I'll switch to dev_info and drop the error code, is this OK?

Yes, that's fine with me.

> 
> One aspect that I skipped is PME_Turn_Off support: The PCI standard
> mandates that this is sent before entering L2/L3 and the designware
> core supports this but it's not part of this patch.
> 
> Is it fine if I post this separately or should it be part of the same
> series? The turnoff bit is in IOMUX gpr for imx6 but SRC for imx7 so it
> would require additional patches in reset, dts and then pci.

IMO it is fine to have this as a follow on patchset.

Regards,
Lucas

Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Lucas Stach]

QW0gTW9udGFnLCBkZW4gMjMuMDcuMjAxOCwgMTI6MzcgKzAwMDAgc2NocmllYiBMZW9uYXJkIENy
ZXN0ZXo6Cj4gT24gTW9uLCAyMDE4LTA3LTIzIGF0IDExOjM4ICswMjAwLCBMdWNhcyBTdGFjaCB3
cm90ZToKPiA+IEhpIExlb25hcmQsCj4gPiAKPiA+IEFtIEZyZWl0YWcsIGRlbiAyMC4wNy4yMDE4
LCAxNTo0NyArMDMwMCBzY2hyaWViIExlb25hcmQgQ3Jlc3RlejoKPiA+ID4gT24gaW14N2QgdGhl
IHBjaWUtcGh5IHBvd2VyIGRvbWFpbiBpcyB0dXJuZWQgb2ZmIGluIHN1c3BlbmQgYW5kIHRoaXMg
Y2FuCj4gPiA+IG1ha2UgdGhlIHN5c3RlbSBoYW5nIGFmdGVyIHJlc3VtZSB3aGVuIGF0dGVtcHRp
bmcgYW55IHJlYWQgZnJvbSBQQ0kuCj4gPiA+IAo+ID4gPiBGaXggdGhpcyBieSBhZGRpbmcgbWlu
aW1hbCBzdXNwZW5kL3Jlc3VtZSBjb2RlIGZyb20gdGhlIG54cCBpbnRlcm5hbAo+ID4gPiB0cmVl
LiBUaGlzIHdpbGwgcHJlcGFyZSBmb3IgcG93ZXJpbmcgZG93biBvbiBzdXNwZW5kIGFuZCByZXNl
dCB0aGUgYmxvY2sKPiA+ID4gb24gcmVzdW1lLgo+ID4gPiAKPiA+ID4gQ29kZSBpcyBvbmx5IGZv
ciBpbXg3ZCBidXQgYSB2ZXJ5IHNpbWlsYXIgc2VxdWVuY2UgY2FuIGJlIHVzZWQgZm9yCj4gPiA+
IG90aGVyIHNvY3MuCj4gPiA+IAo+ID4gPiArc3RhdGljIHZvaWQgaW14Nl9wY2llX2x0c3NtX2Rp
c2FibGUoc3RydWN0IGRldmljZSAqZGV2KQo+ID4gPiArewo+ID4gPiA+ID4gPiArCXN0cnVjdCBp
bXg2X3BjaWUgKmlteDZfcGNpZSA9IGRldl9nZXRfZHJ2ZGF0YShkZXYpOwo+ID4gPiArCj4gPiA+
ID4gPiA+ICsJc3dpdGNoIChpbXg2X3BjaWUtPnZhcmlhbnQpIHsKPiA+ID4gPiA+ID4gKwljYXNl
IElNWDZROgo+ID4gPiA+ID4gPiArCWNhc2UgSU1YNlNYOgo+ID4gPiA+ID4gPiArCWNhc2UgSU1Y
NlFQOgo+ID4gPiA+ID4gPiArCQlyZWdtYXBfdXBkYXRlX2JpdHMoaW14Nl9wY2llLT5pb211eGNf
Z3ByLCBJT01VWENfR1BSMTIsCj4gPiA+ICsJCQkJwqDCoMKgSU1YNlFfR1BSMTJfUENJRV9DVExf
MiwgMCk7Cj4gPiAKPiA+IEhhcyB0aGlzIGJlZW4gdGVzdGVkIG9uIGkuTVg2PyBMVFNTTSBkaXNh
YmxlIHJlcXVpcmVzIGEgbW9yZSBjb21wbGV4Cj4gPiBzZXF1ZW5jZSBvbiB0aGlzIFNvQyB0byBh
dm9pZCBoYW5naW5nIHRoZSBzeXN0ZW0uIFNlZSBjb21taXQKPiA+IDNlM2U0MDZlMzgwNyAiUENJ
OiBpbXg2OiBQdXQgTFRTU00gaW4gIkRldGVjdCIgc3RhdGUgYmVmb3JlIGRpc2FibGluZwo+ID4g
aXQiLgo+IAo+IFRoaXMgcGF0Y2ggb25seSBlbmFibGVzIHN1c3BlbmQvcmVzdW1lIGZvciBpbXg3
ZCB3aXRoIG90aGVyIFNPQ3MgdG8KPiBmb2xsb3cgbGF0ZXIuIFRoZSBsdHNzbV9kaXNhYmxlIGZ1
bmN0aW9uIGlzIGp1c3Qgc3ltbWV0cmljIHdpdGgKPiBsdHNzbV9lbmFibGUuCj4gCj4gVGhlIDZR
IHBhcnRzIGFyZSBhZmZlY3RlZCBieSBlcnJhdGEgIkVSUjAwNTcyMyBQQ0llOiBQQ0llIGRvZXMg
bm90Cj4gc3VwcG9ydCBMMiBwb3dlciBkb3duIFtpLk1YIDZEdWFsLzZRdWFkIE9ubHldIi4KPiAK
PiBUaGlzIGRlc2lnbiBlcnJvciBzZWVtcyB0byBoYXZlIHRoZSBzYW1lIHJvb3QgY2F1c2UgYXMg
eW91ciBwcm9ibGVtIChubwo+IGRlZGljYXRlZCByZXNldCBjb250cm9sKSBzbyB0aGlzIHdvcmtz
IG91dCBxdWl0ZSBuaWNlbHk6IHRoZSBzb2x1dGlvbgo+IGlzIHRvIG5ldmVyIHBvd2VyIGRvd24g
cGNpIG9uIGFmZmVjdGVkIGNoaXBzLgoKSSBkb24ndCBxdWl0ZSBsaWtlIGNvZGUgdGhhdCBsb29r
cyBsaWtlIGl0IGlzIGRvaW5nIHRoZSByaWdodCB0aGluZywKYnV0IHRoZW4gZG9lc24ndC4gQ2Fu
IHdlIGF0IGxlYXN0IGVtaXQgYSB3YXJuaW5nIHRoYXQgdGhlcmUgbWlnaHQgYmUKZHJhZ29ucyBp
ZiBhbnlvbmUgdHJpZXMgdG8gY2FsbCB0aGlzIG9uIGkuTVg2PwoKPiA+ID4gK3N0YXRpYyBpbnQg
aW14Nl9wY2llX3Jlc3VtZV9ub2lycShzdHJ1Y3QgZGV2aWNlICpkZXYpCj4gPiA+ICt7Cj4gPiA+
ID4gPiA+ICsJaW50IHJldCA9IDA7Cj4gPiA+ID4gPiA+ICsJc3RydWN0IGlteDZfcGNpZSAqaW14
Nl9wY2llID0gZGV2X2dldF9kcnZkYXRhKGRldik7Cj4gPiA+ID4gPiA+ICsJc3RydWN0IHBjaWVf
cG9ydCAqcHAgPSAmaW14Nl9wY2llLT5wY2ktPnBwOwo+ID4gPiArCj4gPiA+ID4gPiA+ICsJaWYg
KGlteDZfcGNpZS0+dmFyaWFudCAhPSBJTVg3RCkKPiA+ID4gPiA+ID4gKwkJcmV0dXJuIDA7Cj4g
PiA+ICsKPiA+ID4gPiA+ID4gKwlpbXg2X3BjaWVfYXNzZXJ0X2NvcmVfcmVzZXQoaW14Nl9wY2ll
KTsKPiA+ID4gPiA+ID4gKwlpbXg2X3BjaWVfaW5pdF9waHkoaW14Nl9wY2llKTsKPiA+ID4gPiA+
ID4gKwlpbXg2X3BjaWVfZGVhc3NlcnRfY29yZV9yZXNldChpbXg2X3BjaWUpOwo+ID4gPiA+ID4g
PiArCWR3X3BjaWVfc2V0dXBfcmMocHApOwo+ID4gPiArCj4gPiA+ID4gPiA+ICsJcmV0ID0gaW14
Nl9wY2llX2VzdGFibGlzaF9saW5rKGlteDZfcGNpZSk7Cj4gPiA+ID4gPiA+ICsJaWYgKHJldCA8
IDApCj4gPiA+ICsJCXByX2VycigicGNpZSBsaW5rIGlzIGRvd24gYWZ0ZXIgcmVzdW1lLlxuIik7
Cj4gPiAKPiA+IGRldl9lcnIoKSwgcGxlYXNlLgo+IAo+IFRoZSBpbXg2X3BjaWVfZXN0YWJsaXNo
X2xpbmsgZnVuY3Rpb24gYWxyZWFkeSBzZWVtcyB0byBsaW5rIGVycm9yCj4gaW5mb3JtYXRpb24g
c28gdGhlIG1lc3NhZ2UgY291bGQgYmUgZHJvcHBlZC4gSG93ZXZlciBpdCdzIHN0aWxsIGhlbHBm
dWwKPiB0byBrbm93IHRoYXQgdGhvc2UgcGNpIGxpbmsgZXJyb3JzIGFyZSBzcGVjaWZpY2FsbHkg
cmVsYXRlZCB0byByZXN1bWUuCj4gCj4gRmFiaW8gc3VnZ2VzdGVkIEkgcHJvcGFnYXRlIHRoZSBy
ZXR1cm4gY29kZSBidXQgSSdtIG5vdCBzdXJlIHRoYXQncwo+IGhlbHBmdWwgc2luY2UgImxpbmsg
ZG93biIgaXMgd2hhdCBoYXBwZW5zIHdoZW4gdGhlIHNsb3QgaXMgZW1wdHkgYW5kCj4gdGhpcyBp
cyBjbGVhcmx5IG5vdCBhICJlcnJvciIgb3IgImZhaWx1cmUiLiBJdCdzIG5vdCBjbGVhciBpZiAi
c2xvdAo+IGVtcHR5IiBjYW4gYmUgZGlzdGluZ3Vpc2hlZCBpbiBzb21lIHdheS4KCkkgZG9uJ3Qg
dGhpbmsgd2UgaGF2ZSBhIGdlbmVyaWMgd2F5IHRvIGRpc3Rpbmd1aXNoIGJldHdlZW4gdGhlIHR3
bwpjYXNlcy4KCj4gSSdsbCBzd2l0Y2ggdG8gZGV2X2luZm8gYW5kIGRyb3AgdGhlIGVycm9yIGNv
ZGUsIGlzIHRoaXMgT0s/CgpZZXMsIHRoYXQncyBmaW5lIHdpdGggbWUuCgo+IAo+IE9uZSBhc3Bl
Y3QgdGhhdCBJIHNraXBwZWQgaXMgUE1FX1R1cm5fT2ZmIHN1cHBvcnQ6IFRoZSBQQ0kgc3RhbmRh
cmQKPiBtYW5kYXRlcyB0aGF0IHRoaXMgaXMgc2VudCBiZWZvcmUgZW50ZXJpbmcgTDIvTDMgYW5k
IHRoZSBkZXNpZ253YXJlCj4gY29yZSBzdXBwb3J0cyB0aGlzIGJ1dCBpdCdzIG5vdCBwYXJ0IG9m
IHRoaXMgcGF0Y2guCj4gCj4gSXMgaXQgZmluZSBpZiBJIHBvc3QgdGhpcyBzZXBhcmF0ZWx5IG9y
IHNob3VsZCBpdCBiZSBwYXJ0IG9mIHRoZSBzYW1lCj4gc2VyaWVzPyBUaGUgdHVybm9mZiBiaXQg
aXMgaW4gSU9NVVggZ3ByIGZvciBpbXg2IGJ1dCBTUkMgZm9yIGlteDcgc28gaXQKPiB3b3VsZCBy
ZXF1aXJlIGFkZGl0aW9uYWwgcGF0Y2hlcyBpbiByZXNldCwgZHRzIGFuZCB0aGVuIHBjaS4KCklN
TyBpdCBpcyBmaW5lIHRvIGhhdmUgdGhpcyBhcyBhIGZvbGxvdyBvbiBwYXRjaHNldC4KClJlZ2Fy
ZHMsCkx1Y2FzCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
XwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdApsaW51eC1hcm0ta2VybmVsQGxpc3RzLmlu
ZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5mby9s
aW51eC1hcm0ta2VybmVsCg==

Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Lucas Stach]

Am Montag, den 23.07.2018, 12:37 +0000 schrieb Leonard Crestez:
> On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote:
> > Hi Leonard,
> > 
> > Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> > > On imx7d the pcie-phy power domain is turned off in suspend and this can
> > > make the system hang after resume when attempting any read from PCI.
> > > 
> > > Fix this by adding minimal suspend/resume code from the nxp internal
> > > tree. This will prepare for powering down on suspend and reset the block
> > > on resume.
> > > 
> > > Code is only for imx7d but a very similar sequence can be used for
> > > other socs.
> > > 
> > > +static void imx6_pcie_ltssm_disable(struct device *dev)
> > > +{
> > > > > > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > > +
> > > > > > +	switch (imx6_pcie->variant) {
> > > > > > +	case IMX6Q:
> > > > > > +	case IMX6SX:
> > > > > > +	case IMX6QP:
> > > > > > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > > +				   IMX6Q_GPR12_PCIE_CTL_2, 0);
> > 
> > Has this been tested on i.MX6? LTSSM disable requires a more complex
> > sequence on this SoC to avoid hanging the system. See commit
> > 3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling
> > it".
> 
> This patch only enables suspend/resume for imx7d with other SOCs to
> follow later. The ltssm_disable function is just symmetric with
> ltssm_enable.
> 
> The 6Q parts are affected by errata "ERR005723 PCIe: PCIe does not
> support L2 power down [i.MX 6Dual/6Quad Only]".
> 
> This design error seems to have the same root cause as your problem (no
> dedicated reset control) so this works out quite nicely: the solution
> is to never power down pci on affected chips.

I don't quite like code that looks like it is doing the right thing,
but then doesn't. Can we at least emit a warning that there might be
dragons if anyone tries to call this on i.MX6?

> > > +static int imx6_pcie_resume_noirq(struct device *dev)
> > > +{
> > > > > > +	int ret = 0;
> > > > > > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > > > > > +	struct pcie_port *pp = &imx6_pcie->pci->pp;
> > > +
> > > > > > +	if (imx6_pcie->variant != IMX7D)
> > > > > > +		return 0;
> > > +
> > > > > > +	imx6_pcie_assert_core_reset(imx6_pcie);
> > > > > > +	imx6_pcie_init_phy(imx6_pcie);
> > > > > > +	imx6_pcie_deassert_core_reset(imx6_pcie);
> > > > > > +	dw_pcie_setup_rc(pp);
> > > +
> > > > > > +	ret = imx6_pcie_establish_link(imx6_pcie);
> > > > > > +	if (ret < 0)
> > > +		pr_err("pcie link is down after resume.\n");
> > 
> > dev_err(), please.
> 
> The imx6_pcie_establish_link function already seems to link error
> information so the message could be dropped. However it's still helpful
> to know that those pci link errors are specifically related to resume.
> 
> Fabio suggested I propagate the return code but I'm not sure that's
> helpful since "link down" is what happens when the slot is empty and
> this is clearly not a "error" or "failure". It's not clear if "slot
> empty" can be distinguished in some way.

I don't think we have a generic way to distinguish between the two
cases.

> I'll switch to dev_info and drop the error code, is this OK?

Yes, that's fine with me.

> 
> One aspect that I skipped is PME_Turn_Off support: The PCI standard
> mandates that this is sent before entering L2/L3 and the designware
> core supports this but it's not part of this patch.
> 
> Is it fine if I post this separately or should it be part of the same
> series? The turnoff bit is in IOMUX gpr for imx6 but SRC for imx7 so it
> would require additional patches in reset, dts and then pci.

IMO it is fine to have this as a follow on patchset.

Regards,
Lucas

[PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Lucas Stach]

Am Montag, den 23.07.2018, 12:37 +0000 schrieb Leonard Crestez:
> On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote:
> > Hi Leonard,
> > 
> > Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> > > On imx7d the pcie-phy power domain is turned off in suspend and this can
> > > make the system hang after resume when attempting any read from PCI.
> > > 
> > > Fix this by adding minimal suspend/resume code from the nxp internal
> > > tree. This will prepare for powering down on suspend and reset the block
> > > on resume.
> > > 
> > > Code is only for imx7d but a very similar sequence can be used for
> > > other socs.
> > > 
> > > +static void imx6_pcie_ltssm_disable(struct device *dev)
> > > +{
> > > > > > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > > +
> > > > > > +	switch (imx6_pcie->variant) {
> > > > > > +	case IMX6Q:
> > > > > > +	case IMX6SX:
> > > > > > +	case IMX6QP:
> > > > > > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > > +				???IMX6Q_GPR12_PCIE_CTL_2, 0);
> > 
> > Has this been tested on i.MX6? LTSSM disable requires a more complex
> > sequence on this SoC to avoid hanging the system. See commit
> > 3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling
> > it".
> 
> This patch only enables suspend/resume for imx7d with other SOCs to
> follow later. The ltssm_disable function is just symmetric with
> ltssm_enable.
> 
> The 6Q parts are affected by errata "ERR005723 PCIe: PCIe does not
> support L2 power down [i.MX 6Dual/6Quad Only]".
> 
> This design error seems to have the same root cause as your problem (no
> dedicated reset control) so this works out quite nicely: the solution
> is to never power down pci on affected chips.

I don't quite like code that looks like it is doing the right thing,
but then doesn't. Can we at least emit a warning that there might be
dragons if anyone tries to call this on i.MX6?

> > > +static int imx6_pcie_resume_noirq(struct device *dev)
> > > +{
> > > > > > +	int ret = 0;
> > > > > > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > > > > > +	struct pcie_port *pp = &imx6_pcie->pci->pp;
> > > +
> > > > > > +	if (imx6_pcie->variant != IMX7D)
> > > > > > +		return 0;
> > > +
> > > > > > +	imx6_pcie_assert_core_reset(imx6_pcie);
> > > > > > +	imx6_pcie_init_phy(imx6_pcie);
> > > > > > +	imx6_pcie_deassert_core_reset(imx6_pcie);
> > > > > > +	dw_pcie_setup_rc(pp);
> > > +
> > > > > > +	ret = imx6_pcie_establish_link(imx6_pcie);
> > > > > > +	if (ret < 0)
> > > +		pr_err("pcie link is down after resume.\n");
> > 
> > dev_err(), please.
> 
> The imx6_pcie_establish_link function already seems to link error
> information so the message could be dropped. However it's still helpful
> to know that those pci link errors are specifically related to resume.
> 
> Fabio suggested I propagate the return code but I'm not sure that's
> helpful since "link down" is what happens when the slot is empty and
> this is clearly not a "error" or "failure". It's not clear if "slot
> empty" can be distinguished in some way.

I don't think we have a generic way to distinguish between the two
cases.

> I'll switch to dev_info and drop the error code, is this OK?

Yes, that's fine with me.

> 
> One aspect that I skipped is PME_Turn_Off support: The PCI standard
> mandates that this is sent before entering L2/L3 and the designware
> core supports this but it's not part of this patch.
> 
> Is it fine if I post this separately or should it be part of the same
> series? The turnoff bit is in IOMUX gpr for imx6 but SRC for imx7 so it
> would require additional patches in reset, dts and then pci.

IMO it is fine to have this as a follow on patchset.

Regards,
Lucas

Re: [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"

[Leonard Crestez]

On Mon, 2018-07-23 at 11:38 -0700, Andrey Smirnov wrote:
> On Mon, Jul 23, 2018 at 5:41 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > On Fri, 2018-07-20 at 08:33 -0700, Andrey Smirnov wrote:
> > > On Fri, Jul 20, 2018 at 5:48 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > > > 
> > > > This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d.
> > > > 
> > > > That commit followed the reference manual but unfortunately the imx7d
> > > > manual is incorrect.
> > > 
> > > I'd also add similar comment to DT file to prevent people from trying
> > > to "fix" this in the future.
> > 
> > I'll try to see if I can follow up internally with docs team to get
> > this updated in the next revision of the reference manual.
> 
> Not sure if we are on the same page here, but what I meant is adding
> the same explanation that is in your commit message to Device Tree
> file as well so that if anyone looks at DT code and goes "Huh?" in the
> future, they wouldn't have to research commit history to see the
> reason why things the way they are.

OK, I will add a comment on irq mappings in the next version explaining
that the reference manual is incorrect.

> > > Also, this change is going to break QEMU's mapping found here:
> > 
> > I had no idea that existed, I guess somebody needs to fix that as well.
> 
> I take it it's a no to my "any chance you can fix that as well?" ;-).
> I'll see if I can find time to do that this week.

Sorry, I'm not familiar with qemu source at all.

Re: [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"

[Leonard Crestez]

On Mon, 2018-07-23 at 11:38 -0700, Andrey Smirnov wrote:
> On Mon, Jul 23, 2018 at 5:41 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > On Fri, 2018-07-20 at 08:33 -0700, Andrey Smirnov wrote:
> > > On Fri, Jul 20, 2018 at 5:48 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > > > 
> > > > This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d.
> > > > 
> > > > That commit followed the reference manual but unfortunately the imx7d
> > > > manual is incorrect.
> > > 
> > > I'd also add similar comment to DT file to prevent people from trying
> > > to "fix" this in the future.
> > 
> > I'll try to see if I can follow up internally with docs team to get
> > this updated in the next revision of the reference manual.
> 
> Not sure if we are on the same page here, but what I meant is adding
> the same explanation that is in your commit message to Device Tree
> file as well so that if anyone looks at DT code and goes "Huh?" in the
> future, they wouldn't have to research commit history to see the
> reason why things the way they are.

OK, I will add a comment on irq mappings in the next version explaining
that the reference manual is incorrect.

> > > Also, this change is going to break QEMU's mapping found here:
> > 
> > I had no idea that existed, I guess somebody needs to fix that as well.
> 
> I take it it's a no to my "any chance you can fix that as well?" ;-).
> I'll see if I can find time to do that this week.

Sorry, I'm not familiar with qemu source at all.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"

[Leonard Crestez]

On Mon, 2018-07-23 at 11:38 -0700, Andrey Smirnov wrote:
> On Mon, Jul 23, 2018 at 5:41 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > On Fri, 2018-07-20 at 08:33 -0700, Andrey Smirnov wrote:
> > > On Fri, Jul 20, 2018 at 5:48 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > > > 
> > > > This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d.
> > > > 
> > > > That commit followed the reference manual but unfortunately the imx7d
> > > > manual is incorrect.
> > > 
> > > I'd also add similar comment to DT file to prevent people from trying
> > > to "fix" this in the future.
> > 
> > I'll try to see if I can follow up internally with docs team to get
> > this updated in the next revision of the reference manual.
> 
> Not sure if we are on the same page here, but what I meant is adding
> the same explanation that is in your commit message to Device Tree
> file as well so that if anyone looks at DT code and goes "Huh?" in the
> future, they wouldn't have to research commit history to see the
> reason why things the way they are.

OK, I will add a comment on irq mappings in the next version explaining
that the reference manual is incorrect.

> > > Also, this change is going to break QEMU's mapping found here:
> > 
> > I had no idea that existed, I guess somebody needs to fix that as well.
> 
> I take it it's a no to my "any chance you can fix that as well?" ;-).
> I'll see if I can find time to do that this week.

Sorry, I'm not familiar with qemu source at all.

[PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"

[Leonard Crestez]

On Mon, 2018-07-23 at 11:38 -0700, Andrey Smirnov wrote:
> On Mon, Jul 23, 2018 at 5:41 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > On Fri, 2018-07-20 at 08:33 -0700, Andrey Smirnov wrote:
> > > On Fri, Jul 20, 2018 at 5:48 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > > > 
> > > > This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d.
> > > > 
> > > > That commit followed the reference manual but unfortunately the imx7d
> > > > manual is incorrect.
> > > 
> > > I'd also add similar comment to DT file to prevent people from trying
> > > to "fix" this in the future.
> > 
> > I'll try to see if I can follow up internally with docs team to get
> > this updated in the next revision of the reference manual.
> 
> Not sure if we are on the same page here, but what I meant is adding
> the same explanation that is in your commit message to Device Tree
> file as well so that if anyone looks at DT code and goes "Huh?" in the
> future, they wouldn't have to research commit history to see the
> reason why things the way they are.

OK, I will add a comment on irq mappings in the next version explaining
that the reference manual is incorrect.

> > > Also, this change is going to break QEMU's mapping found here:
> > 
> > I had no idea that existed, I guess somebody needs to fix that as well.
> 
> I take it it's a no to my "any chance you can fix that as well?" ;-).
> I'll see if I can find time to do that this week.

Sorry, I'm not familiar with qemu source at all.

Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Leonard Crestez]

On Tue, 2018-07-24 at 12:09 +0200, Lucas Stach wrote:
> Am Montag, den 23.07.2018, 12:37 +0000 schrieb Leonard Crestez:
> > On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote:
> > > Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> > > > On imx7d the pcie-phy power domain is turned off in suspend and this can
> > > > make the system hang after resume when attempting any read from PCI.
> > > > 
> > > > Fix this by adding minimal suspend/resume code from the nxp internal
> > > > tree. This will prepare for powering down on suspend and reset the block
> > > > on resume.
> > > > 
> > > > Code is only for imx7d but a very similar sequence can be used for
> > > > other socs.
> > > > 
> > > > +static void imx6_pcie_ltssm_disable(struct device *dev)
> > > > +{
> > > > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > > > +
> > > > +	switch (imx6_pcie->variant) {
> > > > +	case IMX6Q:
> > > > +	case IMX6SX:
> > > > +	case IMX6QP:
> > > > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > > > +				   IMX6Q_GPR12_PCIE_CTL_2, 0);
> > > 
> > > Has this been tested on i.MX6? LTSSM disable requires a more complex
> > > sequence on this SoC to avoid hanging the system. See commit
> > > 3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling
> > > it".
> > 
> > This patch only enables suspend/resume for imx7d with other SOCs to
> > follow later. The ltssm_disable function is just symmetric with
> > ltssm_enable.
> > 
> > The 6Q parts are affected by errata "ERR005723 PCIe: PCIe does not
> > support L2 power down [i.MX 6Dual/6Quad Only]".
> > 
> > This design error seems to have the same root cause as your problem (no
> > dedicated reset control) so this works out quite nicely: the solution
> > is to never power down pci on affected chips.
> 
> I don't quite like code that looks like it is doing the right thing,
> but then doesn't. Can we at least emit a warning that there might be
> dragons if anyone tries to call this on i.MX6?

But the function will indeed toggle the right bits to initiate ltssm
disabling. If this is not useful or can't be used right then it's the
caller's problem :)

I can add a default: clause to the switch which returns -ENOSYS and let
IMX6Q fall that way, would that be OK? This would also help when adding
new variants.

Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Leonard Crestez]

On Tue, 2018-07-24 at 12:09 +0200, Lucas Stach wrote:
> Am Montag, den 23.07.2018, 12:37 +0000 schrieb Leonard Crestez:
> > On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote:
> > > Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> > > > On imx7d the pcie-phy power domain is turned off in suspend and this can
> > > > make the system hang after resume when attempting any read from PCI.
> > > > 
> > > > Fix this by adding minimal suspend/resume code from the nxp internal
> > > > tree. This will prepare for powering down on suspend and reset the block
> > > > on resume.
> > > > 
> > > > Code is only for imx7d but a very similar sequence can be used for
> > > > other socs.
> > > > 
> > > > +static void imx6_pcie_ltssm_disable(struct device *dev)
> > > > +{
> > > > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > > > +
> > > > +	switch (imx6_pcie->variant) {
> > > > +	case IMX6Q:
> > > > +	case IMX6SX:
> > > > +	case IMX6QP:
> > > > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > > > +				   IMX6Q_GPR12_PCIE_CTL_2, 0);
> > > 
> > > Has this been tested on i.MX6? LTSSM disable requires a more complex
> > > sequence on this SoC to avoid hanging the system. See commit
> > > 3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling
> > > it".
> > 
> > This patch only enables suspend/resume for imx7d with other SOCs to
> > follow later. The ltssm_disable function is just symmetric with
> > ltssm_enable.
> > 
> > The 6Q parts are affected by errata "ERR005723 PCIe: PCIe does not
> > support L2 power down [i.MX 6Dual/6Quad Only]".
> > 
> > This design error seems to have the same root cause as your problem (no
> > dedicated reset control) so this works out quite nicely: the solution
> > is to never power down pci on affected chips.
> 
> I don't quite like code that looks like it is doing the right thing,
> but then doesn't. Can we at least emit a warning that there might be
> dragons if anyone tries to call this on i.MX6?

But the function will indeed toggle the right bits to initiate ltssm
disabling. If this is not useful or can't be used right then it's the
caller's problem :)

I can add a default: clause to the switch which returns -ENOSYS and let
IMX6Q fall that way, would that be OK? This would also help when adding
new variants.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Leonard Crestez]

On Tue, 2018-07-24 at 12:09 +0200, Lucas Stach wrote:
> Am Montag, den 23.07.2018, 12:37 +0000 schrieb Leonard Crestez:
> > On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote:
> > > Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> > > > On imx7d the pcie-phy power domain is turned off in suspend and this can
> > > > make the system hang after resume when attempting any read from PCI.
> > > > 
> > > > Fix this by adding minimal suspend/resume code from the nxp internal
> > > > tree. This will prepare for powering down on suspend and reset the block
> > > > on resume.
> > > > 
> > > > Code is only for imx7d but a very similar sequence can be used for
> > > > other socs.
> > > > 
> > > > +static void imx6_pcie_ltssm_disable(struct device *dev)
> > > > +{
> > > > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > > > +
> > > > +	switch (imx6_pcie->variant) {
> > > > +	case IMX6Q:
> > > > +	case IMX6SX:
> > > > +	case IMX6QP:
> > > > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > > > +				   IMX6Q_GPR12_PCIE_CTL_2, 0);
> > > 
> > > Has this been tested on i.MX6? LTSSM disable requires a more complex
> > > sequence on this SoC to avoid hanging the system. See commit
> > > 3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling
> > > it".
> > 
> > This patch only enables suspend/resume for imx7d with other SOCs to
> > follow later. The ltssm_disable function is just symmetric with
> > ltssm_enable.
> > 
> > The 6Q parts are affected by errata "ERR005723 PCIe: PCIe does not
> > support L2 power down [i.MX 6Dual/6Quad Only]".
> > 
> > This design error seems to have the same root cause as your problem (no
> > dedicated reset control) so this works out quite nicely: the solution
> > is to never power down pci on affected chips.
> 
> I don't quite like code that looks like it is doing the right thing,
> but then doesn't. Can we at least emit a warning that there might be
> dragons if anyone tries to call this on i.MX6?

But the function will indeed toggle the right bits to initiate ltssm
disabling. If this is not useful or can't be used right then it's the
caller's problem :)

I can add a default: clause to the switch which returns -ENOSYS and let
IMX6Q fall that way, would that be OK? This would also help when adding
new variants.

[PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Leonard Crestez]

On Tue, 2018-07-24 at 12:09 +0200, Lucas Stach wrote:
> Am Montag, den 23.07.2018, 12:37 +0000 schrieb Leonard Crestez:
> > On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote:
> > > Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> > > > On imx7d the pcie-phy power domain is turned off in suspend and this can
> > > > make the system hang after resume when attempting any read from PCI.
> > > > 
> > > > Fix this by adding minimal suspend/resume code from the nxp internal
> > > > tree. This will prepare for powering down on suspend and reset the block
> > > > on resume.
> > > > 
> > > > Code is only for imx7d but a very similar sequence can be used for
> > > > other socs.
> > > > 
> > > > +static void imx6_pcie_ltssm_disable(struct device *dev)
> > > > +{
> > > > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > > > +
> > > > +	switch (imx6_pcie->variant) {
> > > > +	case IMX6Q:
> > > > +	case IMX6SX:
> > > > +	case IMX6QP:
> > > > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > > > +				   IMX6Q_GPR12_PCIE_CTL_2, 0);
> > > 
> > > Has this been tested on i.MX6? LTSSM disable requires a more complex
> > > sequence on this SoC to avoid hanging the system. See commit
> > > 3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling
> > > it".
> > 
> > This patch only enables suspend/resume for imx7d with other SOCs to
> > follow later. The ltssm_disable function is just symmetric with
> > ltssm_enable.
> > 
> > The 6Q parts are affected by errata "ERR005723 PCIe: PCIe does not
> > support L2 power down [i.MX 6Dual/6Quad Only]".
> > 
> > This design error seems to have the same root cause as your problem (no
> > dedicated reset control) so this works out quite nicely: the solution
> > is to never power down pci on affected chips.
> 
> I don't quite like code that looks like it is doing the right thing,
> but then doesn't. Can we at least emit a warning that there might be
> dragons if anyone tries to call this on i.MX6?

But the function will indeed toggle the right bits to initiate ltssm
disabling. If this is not useful or can't be used right then it's the
caller's problem :)

I can add a default: clause to the switch which returns -ENOSYS and let
IMX6Q fall that way, would that be OK? This would also help when adding
new variants.

Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Lucas Stach]

Am Dienstag, den 24.07.2018, 12:04 +0000 schrieb Leonard Crestez:
> On Tue, 2018-07-24 at 12:09 +0200, Lucas Stach wrote:
> > Am Montag, den 23.07.2018, 12:37 +0000 schrieb Leonard Crestez:
> > > On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote:
> > > > Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> > > > > On imx7d the pcie-phy power domain is turned off in suspend and this can
> > > > > make the system hang after resume when attempting any read from PCI.
> > > > > 
> > > > > Fix this by adding minimal suspend/resume code from the nxp internal
> > > > > tree. This will prepare for powering down on suspend and reset the block
> > > > > on resume.
> > > > > 
> > > > > Code is only for imx7d but a very similar sequence can be used for
> > > > > other socs.
> > > > > 
> > > > > +static void imx6_pcie_ltssm_disable(struct device *dev)
> > > > > +{
> > > > > > > > > > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > > > > +
> > > > > > > > > > +	switch (imx6_pcie->variant) {
> > > > > > > > > > +	case IMX6Q:
> > > > > > > > > > +	case IMX6SX:
> > > > > > > > > > +	case IMX6QP:
> > > > > > > > > > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > > > > +				   IMX6Q_GPR12_PCIE_CTL_2, 0);
> > > > 
> > > > Has this been tested on i.MX6? LTSSM disable requires a more complex
> > > > sequence on this SoC to avoid hanging the system. See commit
> > > > 3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling
> > > > it".
> > > 
> > > This patch only enables suspend/resume for imx7d with other SOCs to
> > > follow later. The ltssm_disable function is just symmetric with
> > > ltssm_enable.
> > > 
> > > The 6Q parts are affected by errata "ERR005723 PCIe: PCIe does not
> > > support L2 power down [i.MX 6Dual/6Quad Only]".
> > > 
> > > This design error seems to have the same root cause as your problem (no
> > > dedicated reset control) so this works out quite nicely: the solution
> > > is to never power down pci on affected chips.
> > 
> > I don't quite like code that looks like it is doing the right thing,
> > but then doesn't. Can we at least emit a warning that there might be
> > dragons if anyone tries to call this on i.MX6?
> 
> But the function will indeed toggle the right bits to initiate ltssm
> disabling. If this is not useful or can't be used right then it's the
> caller's problem :)

I don't agree with that. I would expect a function that is called
ltssm_disable to do so in a way that is safe on the hardware that it
claims to handle, which in case of i.MX6 means bashing the LTSSM into
detect state before toggling the GPR bit.

> I can add a default: clause to the switch which returns -ENOSYS and let
> IMX6Q fall that way, would that be OK? This would also help when adding
> new variants.

Yes, given that there is currently no way to test this on i.MX6,
returning -ENOSYS sound much better. This at least tells anyone who
intends to use this function that there is indeed missing functionality
there.

Regards,
Lucas

Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Lucas Stach]

QW0gRGllbnN0YWcsIGRlbiAyNC4wNy4yMDE4LCAxMjowNCArMDAwMCBzY2hyaWViIExlb25hcmQg
Q3Jlc3RlejoKPiBPbiBUdWUsIDIwMTgtMDctMjQgYXQgMTI6MDkgKzAyMDAsIEx1Y2FzIFN0YWNo
IHdyb3RlOgo+ID4gQW0gTW9udGFnLCBkZW4gMjMuMDcuMjAxOCwgMTI6MzcgKzAwMDAgc2Nocmll
YiBMZW9uYXJkIENyZXN0ZXo6Cj4gPiA+IE9uIE1vbiwgMjAxOC0wNy0yMyBhdCAxMTozOCArMDIw
MCwgTHVjYXMgU3RhY2ggd3JvdGU6Cj4gPiA+ID4gQW0gRnJlaXRhZywgZGVuIDIwLjA3LjIwMTgs
IDE1OjQ3ICswMzAwIHNjaHJpZWIgTGVvbmFyZCBDcmVzdGV6Ogo+ID4gPiA+ID4gT24gaW14N2Qg
dGhlIHBjaWUtcGh5IHBvd2VyIGRvbWFpbiBpcyB0dXJuZWQgb2ZmIGluIHN1c3BlbmQgYW5kIHRo
aXMgY2FuCj4gPiA+ID4gPiBtYWtlIHRoZSBzeXN0ZW0gaGFuZyBhZnRlciByZXN1bWUgd2hlbiBh
dHRlbXB0aW5nIGFueSByZWFkIGZyb20gUENJLgo+ID4gPiA+ID4gCj4gPiA+ID4gPiBGaXggdGhp
cyBieSBhZGRpbmcgbWluaW1hbCBzdXNwZW5kL3Jlc3VtZSBjb2RlIGZyb20gdGhlIG54cCBpbnRl
cm5hbAo+ID4gPiA+ID4gdHJlZS4gVGhpcyB3aWxsIHByZXBhcmUgZm9yIHBvd2VyaW5nIGRvd24g
b24gc3VzcGVuZCBhbmQgcmVzZXQgdGhlIGJsb2NrCj4gPiA+ID4gPiBvbiByZXN1bWUuCj4gPiA+
ID4gPiAKPiA+ID4gPiA+IENvZGUgaXMgb25seSBmb3IgaW14N2QgYnV0IGEgdmVyeSBzaW1pbGFy
IHNlcXVlbmNlIGNhbiBiZSB1c2VkIGZvcgo+ID4gPiA+ID4gb3RoZXIgc29jcy4KPiA+ID4gPiA+
IAo+ID4gPiA+ID4gK3N0YXRpYyB2b2lkIGlteDZfcGNpZV9sdHNzbV9kaXNhYmxlKHN0cnVjdCBk
ZXZpY2UgKmRldikKPiA+ID4gPiA+ICt7Cj4gPiA+ID4gPiA+ID4gPiA+ID4gKwlzdHJ1Y3QgaW14
Nl9wY2llICppbXg2X3BjaWUgPSBkZXZfZ2V0X2RydmRhdGEoZGV2KTsKPiA+ID4gPiA+ICsKPiA+
ID4gPiA+ID4gPiA+ID4gPiArCXN3aXRjaCAoaW14Nl9wY2llLT52YXJpYW50KSB7Cj4gPiA+ID4g
PiA+ID4gPiA+ID4gKwljYXNlIElNWDZROgo+ID4gPiA+ID4gPiA+ID4gPiA+ICsJY2FzZSBJTVg2
U1g6Cj4gPiA+ID4gPiA+ID4gPiA+ID4gKwljYXNlIElNWDZRUDoKPiA+ID4gPiA+ID4gPiA+ID4g
PiArCQlyZWdtYXBfdXBkYXRlX2JpdHMoaW14Nl9wY2llLT5pb211eGNfZ3ByLCBJT01VWENfR1BS
MTIsCj4gPiA+ID4gPiArCQkJCcKgwqDCoElNWDZRX0dQUjEyX1BDSUVfQ1RMXzIsIDApOwo+ID4g
PiA+IAo+ID4gPiA+IEhhcyB0aGlzIGJlZW4gdGVzdGVkIG9uIGkuTVg2PyBMVFNTTSBkaXNhYmxl
IHJlcXVpcmVzIGEgbW9yZSBjb21wbGV4Cj4gPiA+ID4gc2VxdWVuY2Ugb24gdGhpcyBTb0MgdG8g
YXZvaWQgaGFuZ2luZyB0aGUgc3lzdGVtLiBTZWUgY29tbWl0Cj4gPiA+ID4gM2UzZTQwNmUzODA3
ICJQQ0k6IGlteDY6IFB1dCBMVFNTTSBpbiAiRGV0ZWN0IiBzdGF0ZSBiZWZvcmUgZGlzYWJsaW5n
Cj4gPiA+ID4gaXQiLgo+ID4gPiAKPiA+ID4gVGhpcyBwYXRjaCBvbmx5IGVuYWJsZXMgc3VzcGVu
ZC9yZXN1bWUgZm9yIGlteDdkIHdpdGggb3RoZXIgU09DcyB0bwo+ID4gPiBmb2xsb3cgbGF0ZXIu
IFRoZSBsdHNzbV9kaXNhYmxlIGZ1bmN0aW9uIGlzIGp1c3Qgc3ltbWV0cmljIHdpdGgKPiA+ID4g
bHRzc21fZW5hYmxlLgo+ID4gPiAKPiA+ID4gVGhlIDZRIHBhcnRzIGFyZSBhZmZlY3RlZCBieSBl
cnJhdGEgIkVSUjAwNTcyMyBQQ0llOiBQQ0llIGRvZXMgbm90Cj4gPiA+IHN1cHBvcnQgTDIgcG93
ZXIgZG93biBbaS5NWCA2RHVhbC82UXVhZCBPbmx5XSIuCj4gPiA+IAo+ID4gPiBUaGlzIGRlc2ln
biBlcnJvciBzZWVtcyB0byBoYXZlIHRoZSBzYW1lIHJvb3QgY2F1c2UgYXMgeW91ciBwcm9ibGVt
IChubwo+ID4gPiBkZWRpY2F0ZWQgcmVzZXQgY29udHJvbCkgc28gdGhpcyB3b3JrcyBvdXQgcXVp
dGUgbmljZWx5OiB0aGUgc29sdXRpb24KPiA+ID4gaXMgdG8gbmV2ZXIgcG93ZXIgZG93biBwY2kg
b24gYWZmZWN0ZWQgY2hpcHMuCj4gPiAKPiA+IEkgZG9uJ3QgcXVpdGUgbGlrZSBjb2RlIHRoYXQg
bG9va3MgbGlrZSBpdCBpcyBkb2luZyB0aGUgcmlnaHQgdGhpbmcsCj4gPiBidXQgdGhlbiBkb2Vz
bid0LiBDYW4gd2UgYXQgbGVhc3QgZW1pdCBhIHdhcm5pbmcgdGhhdCB0aGVyZSBtaWdodCBiZQo+
ID4gZHJhZ29ucyBpZiBhbnlvbmUgdHJpZXMgdG8gY2FsbCB0aGlzIG9uIGkuTVg2Pwo+IAo+IEJ1
dCB0aGUgZnVuY3Rpb24gd2lsbCBpbmRlZWQgdG9nZ2xlIHRoZSByaWdodCBiaXRzIHRvIGluaXRp
YXRlIGx0c3NtCj4gZGlzYWJsaW5nLiBJZiB0aGlzIGlzIG5vdCB1c2VmdWwgb3IgY2FuJ3QgYmUg
dXNlZCByaWdodCB0aGVuIGl0J3MgdGhlCj4gY2FsbGVyJ3MgcHJvYmxlbSA6KQoKSSBkb24ndCBh
Z3JlZSB3aXRoIHRoYXQuIEkgd291bGQgZXhwZWN0IGEgZnVuY3Rpb24gdGhhdCBpcyBjYWxsZWQK
bHRzc21fZGlzYWJsZSB0byBkbyBzbyBpbiBhIHdheSB0aGF0IGlzIHNhZmUgb24gdGhlIGhhcmR3
YXJlIHRoYXQgaXQKY2xhaW1zIHRvIGhhbmRsZSwgd2hpY2ggaW4gY2FzZSBvZiBpLk1YNiBtZWFu
cyBiYXNoaW5nIHRoZSBMVFNTTSBpbnRvCmRldGVjdCBzdGF0ZSBiZWZvcmUgdG9nZ2xpbmcgdGhl
IEdQUiBiaXQuCgo+IEkgY2FuIGFkZCBhIGRlZmF1bHQ6IGNsYXVzZSB0byB0aGUgc3dpdGNoIHdo
aWNoIHJldHVybnMgLUVOT1NZUyBhbmQgbGV0Cj4gSU1YNlEgZmFsbCB0aGF0IHdheSwgd291bGQg
dGhhdCBiZSBPSz8gVGhpcyB3b3VsZCBhbHNvIGhlbHAgd2hlbiBhZGRpbmcKPiBuZXcgdmFyaWFu
dHMuCgpZZXMsIGdpdmVuIHRoYXQgdGhlcmUgaXMgY3VycmVudGx5IG5vIHdheSB0byB0ZXN0IHRo
aXMgb24gaS5NWDYsCnJldHVybmluZyAtRU5PU1lTIHNvdW5kIG11Y2ggYmV0dGVyLiBUaGlzIGF0
IGxlYXN0IHRlbGxzIGFueW9uZSB3aG8KaW50ZW5kcyB0byB1c2UgdGhpcyBmdW5jdGlvbiB0aGF0
IHRoZXJlIGlzIGluZGVlZCBtaXNzaW5nIGZ1bmN0aW9uYWxpdHkKdGhlcmUuCgpSZWdhcmRzLApM
dWNhcwoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KbGlu
dXgtYXJtLWtlcm5lbCBtYWlsaW5nIGxpc3QKbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRl
YWQub3JnCmh0dHA6Ly9saXN0cy5pbmZyYWRlYWQub3JnL21haWxtYW4vbGlzdGluZm8vbGludXgt
YXJtLWtlcm5lbAo=

Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Lucas Stach]

Am Dienstag, den 24.07.2018, 12:04 +0000 schrieb Leonard Crestez:
> On Tue, 2018-07-24 at 12:09 +0200, Lucas Stach wrote:
> > Am Montag, den 23.07.2018, 12:37 +0000 schrieb Leonard Crestez:
> > > On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote:
> > > > Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> > > > > On imx7d the pcie-phy power domain is turned off in suspend and this can
> > > > > make the system hang after resume when attempting any read from PCI.
> > > > > 
> > > > > Fix this by adding minimal suspend/resume code from the nxp internal
> > > > > tree. This will prepare for powering down on suspend and reset the block
> > > > > on resume.
> > > > > 
> > > > > Code is only for imx7d but a very similar sequence can be used for
> > > > > other socs.
> > > > > 
> > > > > +static void imx6_pcie_ltssm_disable(struct device *dev)
> > > > > +{
> > > > > > > > > > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > > > > +
> > > > > > > > > > +	switch (imx6_pcie->variant) {
> > > > > > > > > > +	case IMX6Q:
> > > > > > > > > > +	case IMX6SX:
> > > > > > > > > > +	case IMX6QP:
> > > > > > > > > > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > > > > +				   IMX6Q_GPR12_PCIE_CTL_2, 0);
> > > > 
> > > > Has this been tested on i.MX6? LTSSM disable requires a more complex
> > > > sequence on this SoC to avoid hanging the system. See commit
> > > > 3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling
> > > > it".
> > > 
> > > This patch only enables suspend/resume for imx7d with other SOCs to
> > > follow later. The ltssm_disable function is just symmetric with
> > > ltssm_enable.
> > > 
> > > The 6Q parts are affected by errata "ERR005723 PCIe: PCIe does not
> > > support L2 power down [i.MX 6Dual/6Quad Only]".
> > > 
> > > This design error seems to have the same root cause as your problem (no
> > > dedicated reset control) so this works out quite nicely: the solution
> > > is to never power down pci on affected chips.
> > 
> > I don't quite like code that looks like it is doing the right thing,
> > but then doesn't. Can we at least emit a warning that there might be
> > dragons if anyone tries to call this on i.MX6?
> 
> But the function will indeed toggle the right bits to initiate ltssm
> disabling. If this is not useful or can't be used right then it's the
> caller's problem :)

I don't agree with that. I would expect a function that is called
ltssm_disable to do so in a way that is safe on the hardware that it
claims to handle, which in case of i.MX6 means bashing the LTSSM into
detect state before toggling the GPR bit.

> I can add a default: clause to the switch which returns -ENOSYS and let
> IMX6Q fall that way, would that be OK? This would also help when adding
> new variants.

Yes, given that there is currently no way to test this on i.MX6,
returning -ENOSYS sound much better. This at least tells anyone who
intends to use this function that there is indeed missing functionality
there.

Regards,
Lucas

[PATCH v2 3/3] PCI: imx: Initial imx7d pm support

[Lucas Stach]

Am Dienstag, den 24.07.2018, 12:04 +0000 schrieb Leonard Crestez:
> On Tue, 2018-07-24 at 12:09 +0200, Lucas Stach wrote:
> > Am Montag, den 23.07.2018, 12:37 +0000 schrieb Leonard Crestez:
> > > On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote:
> > > > Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:
> > > > > On imx7d the pcie-phy power domain is turned off in suspend and this can
> > > > > make the system hang after resume when attempting any read from PCI.
> > > > > 
> > > > > Fix this by adding minimal suspend/resume code from the nxp internal
> > > > > tree. This will prepare for powering down on suspend and reset the block
> > > > > on resume.
> > > > > 
> > > > > Code is only for imx7d but a very similar sequence can be used for
> > > > > other socs.
> > > > > 
> > > > > +static void imx6_pcie_ltssm_disable(struct device *dev)
> > > > > +{
> > > > > > > > > > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > > > > +
> > > > > > > > > > +	switch (imx6_pcie->variant) {
> > > > > > > > > > +	case IMX6Q:
> > > > > > > > > > +	case IMX6SX:
> > > > > > > > > > +	case IMX6QP:
> > > > > > > > > > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > > > > +				???IMX6Q_GPR12_PCIE_CTL_2, 0);
> > > > 
> > > > Has this been tested on i.MX6? LTSSM disable requires a more complex
> > > > sequence on this SoC to avoid hanging the system. See commit
> > > > 3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling
> > > > it".
> > > 
> > > This patch only enables suspend/resume for imx7d with other SOCs to
> > > follow later. The ltssm_disable function is just symmetric with
> > > ltssm_enable.
> > > 
> > > The 6Q parts are affected by errata "ERR005723 PCIe: PCIe does not
> > > support L2 power down [i.MX 6Dual/6Quad Only]".
> > > 
> > > This design error seems to have the same root cause as your problem (no
> > > dedicated reset control) so this works out quite nicely: the solution
> > > is to never power down pci on affected chips.
> > 
> > I don't quite like code that looks like it is doing the right thing,
> > but then doesn't. Can we at least emit a warning that there might be
> > dragons if anyone tries to call this on i.MX6?
> 
> But the function will indeed toggle the right bits to initiate ltssm
> disabling. If this is not useful or can't be used right then it's the
> caller's problem :)

I don't agree with that. I would expect a function that is called
ltssm_disable to do so in a way that is safe on the hardware that it
claims to handle, which in case of i.MX6 means bashing the LTSSM into
detect state before toggling the GPR bit.

> I can add a default: clause to the switch which returns -ENOSYS and let
> IMX6Q fall that way, would that be OK? This would also help when adding
> new variants.

Yes, given that there is currently no way to test this on i.MX6,
returning -ENOSYS sound much better. This at least tells anyone who
intends to use this function that there is indeed missing functionality
there.

Regards,
Lucas