Skip to contentSkip to conversation

PCI: imx: Initial imx7d pm support

PATCH Applied Leonard Crestez 5 participants 16 messages Jul 20, 2018 – Jul 24, 2018

Subsystems (6)
Device tree bindings Arm i.MX and Layerscape PCI PCI native host bridge and endpoint drivers +2 more subsystems

v2 of 4 Applied Latest is v4 →

lore
Share & tools
lore

Leonard Crestez

author
#0 lore raw

v2 cover PCI: imx: Initial imx7d pm support

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(-)
Signature · 1 line
2.17.1
author
#1 lore raw

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

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 <[email protected]>
---
 arch/arm/boot/dts/imx7d.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
arch/arm/boot/dts/imx7d.dtsi +4 -4
index 8d3d123d0a5c..12c5ba7e9f1e 100644
@@ -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>,
Signature · 1 line
2.17.1
#2 lore raw

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

Quoted 6 lines · Leonard Crestez

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

author
#3 lore raw

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

Quoted 9 lines · Andrey Smirnov
Quoted 5 lines · Leonard Crestez

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.

Quoted 1 line

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

#4 lore raw

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

Quoted 6 lines · Leonard Crestez
Quoted 4 lines · Andrey Smirnov
Quoted 3 lines · Leonard Crestez

This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d.

Quoted 9 lines
Quoted 5 lines
Quoted 2 lines

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.

Quoted 4 lines
Quoted 1 line

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.

Quoted 2 lines

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

author
#5 lore raw

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

Quoted 12 lines · Andrey Smirnov
Quoted 11 lines · Leonard Crestez
Quoted 9 lines · Andrey Smirnov
Quoted 5 lines · Leonard Crestez

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.

Quoted 8 lines
Quoted 2 lines

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.

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

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.

author
#6 lore raw

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

In reply to 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 <[email protected]>
Reviewed-by Lucas Stach <[email protected]>
---
 drivers/reset/reset-imx7.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/reset/reset-imx7.c +1 -1
index 4db177bc89bc..fdeac1946429 100644
@@ -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
Signature · 1 line
2.17.1
maintainer
#7 lore raw

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

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:

Quoted 34 lines

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.

Quoted 2 lines
Signed-off-by Leonard Crestez <[email protected]>
Reviewed-by Lucas Stach <[email protected]>

---
?drivers/reset/reset-imx7.c | 2 +-
?1 file changed, 1 insertion(+), 1 deletion(-)

drivers/reset/reset-imx7.c +0 -0
index 4db177bc89bc..fdeac1946429 100644
@@ -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;

?

Quoted 4 lines

? switch (id) {
? case IMX7_RESET_PCIEPHY:
? /*
? ?* wait for more than 10us to release phy g_rst and

maintainer Applied
#8 lore raw

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

Quoted 20 lines · 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:

Quoted 13 lines

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.

Quoted 2 lines
Signed-off-by Leonard Crestez <[email protected]>
Reviewed-by Lucas Stach <[email protected]>

Thank you, applied to reset/fixes.

regards
Philipp

author
#9 lore raw

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

In reply to 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 <[email protected]>, this patch adjusts the code to the upstream imx7d implemention using reset controls and power domains.

Signed-off-by Leonard Crestez <[email protected]>
---
 drivers/pci/controller/dwc/pci-imx6.c | 95 +++++++++++++++++++++++++--
 1 file changed, 90 insertions(+), 5 deletions(-)
drivers/pci/controller/dwc/pci-imx6.c +90 -5
index 80f604602783..daebee905108 100644
@@ -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,
 };
Signature · 1 line
2.17.1
reviewer
#10 lore raw

Hi Leonard,

drivers/pci/controller/dwc/pci-imx6.c static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie, · line 745

Quoted 3 lines · Leonard Crestez
drivers/pci/controller/dwc/pci-imx6.c +3 -0
+static int imx6_pcie_resume_noirq(struct device *dev)
+{
+       int ret = 0;

There is no need for initializing 'ret' here.

Quoted 4 lines
drivers/pci/controller/dwc/pci-imx6.c +4 -0
+       struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+       struct pcie_port *pp = &imx6_pcie->pci->pp;
+
+       if (imx6_pcie->variant != IMX7D)

drivers/pci/controller/dwc/pci-imx6.c static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie, · line 759

Quoted 10 lines
drivers/pci/controller/dwc/pci-imx6.c +10 -0
+               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().

maintainer
#11 lore raw
In reply to Leonard Crestez

Hi Leonard,

Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:

Quoted 84 lines

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.

Quoted 1 line

The original author is mostly Richard Zhu <[email protected]>, this

patch adjusts the code to the upstream imx7d implemention using reset controls and power domains.

Quoted 1 line
Signed-off-by Leonard Crestez <[email protected]>

---
?drivers/pci/controller/dwc/pci-imx6.c | 95 +++++++++++++++++++++++++-- ?1 file changed, 90 insertions(+), 5 deletions(-)

drivers/pci/controller/dwc/pci-imx6.c +0 -0
index 80f604602783..daebee905108 100644
@@ -540,10 +540,27 @@ static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie)

?

Quoted 2 lines

? dev_err(dev, "Speed change timeout\n");
? return -EINVAL;

?}
?

drivers/pci/controller/dwc/pci-imx6.c +2 -0
+static void imx6_pcie_ltssm_enable(struct device *dev)
+{

Review

Quoted 1 line
(quoted diff) +1 -0
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);

+

Quoted 11 lines
drivers/pci/controller/dwc/pci-imx6.c +11 -0
+	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);
+	}
drivers/pci/controller/dwc/pci-imx6.c +2 -0
+}
+

?static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie) ?{

Quoted 3 lines

? struct dw_pcie *pci = imx6_pcie->pci;
? struct device *dev = pci->dev;
? u32 tmp;

drivers/pci/controller/dwc/pci-imx6.c +0 -0
@@ -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);

?

drivers/pci/controller/dwc/pci-imx6.c static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie) · line 580

Quoted 7 lines

? /* Start LTSSM. */

drivers/pci/controller/dwc/pci-imx6.c +1 -5
-	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);

?

Quoted 3 lines

? ret = imx6_pcie_wait_for_link(imx6_pcie);
? if (ret)
? goto err_reset_phy;

?

drivers/pci/controller/dwc/pci-imx6.c +0 -0
@@ -681,10 +694,81 @@ static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie,

?
?static const struct dw_pcie_ops dw_pcie_ops = {

Quoted 1 line

? .link_up = imx6_pcie_link_up,

?};
?

drivers/pci/controller/dwc/pci-imx6.c +3 -0
+#ifdef CONFIG_PM_SLEEP
+static void imx6_pcie_ltssm_disable(struct device *dev)
+{

Review

Quoted 1 line
(quoted diff) +1 -0
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);

+

Quoted 5 lines
drivers/pci/controller/dwc/pci-imx6.c +5 -0
+	switch (imx6_pcie->variant) {
+	case IMX6Q:
+	case IMX6SX:
+	case IMX6QP:
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,

drivers/pci/controller/dwc/pci-imx6.c

Quoted 1 line
drivers/pci/controller/dwc/pci-imx6.c +1 -0
+				???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.

Quoted 49 lines
drivers/pci/controller/dwc/pci-imx6.c +1 -0
+		break;
Quoted 4 lines
drivers/pci/controller/dwc/pci-imx6.c +4 -0
+	case IMX7D:
+		reset_control_assert(imx6_pcie->apps_reset);
+		break;
+	}
drivers/pci/controller/dwc/pci-imx6.c +4 -0
+}
+
+static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
+{

drivers/pci/controller/dwc/pci-imx6.c static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie, · line 721

Quoted 3 lines
drivers/pci/controller/dwc/pci-imx6.c +3 -0
+	clk_disable_unprepare(imx6_pcie->pcie);
+	clk_disable_unprepare(imx6_pcie->pcie_phy);
+	clk_disable_unprepare(imx6_pcie->pcie_bus);

+

Quoted 5 lines
drivers/pci/controller/dwc/pci-imx6.c +5 -0
+	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);
+	}
drivers/pci/controller/dwc/pci-imx6.c +4 -0
+}
+
+static int imx6_pcie_suspend_noirq(struct device *dev)
+{

Review

Quoted 1 line
(quoted diff) +1 -0
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);

+

Review

Quoted 2 lines
(quoted diff) +2 -0
+	if (imx6_pcie->variant != IMX7D)
+		return 0;

+

drivers/pci/controller/dwc/pci-imx6.c static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie, · line 738

Quoted 2 lines
drivers/pci/controller/dwc/pci-imx6.c +2 -0
+	imx6_pcie_clk_disable(imx6_pcie);
+	imx6_pcie_ltssm_disable(dev);

+

Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/pci/controller/dwc/pci-imx6.c +4 -0
+}
+
+static int imx6_pcie_resume_noirq(struct device *dev)
+{

drivers/pci/controller/dwc/pci-imx6.c static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie, · line 747

Quoted 3 lines
drivers/pci/controller/dwc/pci-imx6.c +3 -0
+	int ret = 0;
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+	struct pcie_port *pp = &imx6_pcie->pci->pp;

+

Review

Quoted 2 lines
(quoted diff) +2 -0
+	if (imx6_pcie->variant != IMX7D)
+		return 0;

+

drivers/pci/controller/dwc/pci-imx6.c static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie, · line 755

Quoted 4 lines
drivers/pci/controller/dwc/pci-imx6.c +4 -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);

+

Quoted 2 lines
drivers/pci/controller/dwc/pci-imx6.c +2 -0
+	ret = imx6_pcie_establish_link(imx6_pcie);
+	if (ret < 0)

drivers/pci/controller/dwc/pci-imx6.c static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie, · line 759

Quoted 1 line
drivers/pci/controller/dwc/pci-imx6.c +1 -0
+		pr_err("pcie link is down after resume.\n");

dev_err(), please.

Regards,
Lucas

Quoted 27 lines

+

Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/pci/controller/dwc/pci-imx6.c +4 -0
+}
+#endif
+
+static const struct dev_pm_ops imx6_pcie_pm_ops = {
Quoted 2 lines
(quoted diff) +2 -0
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx6_pcie_suspend_noirq,
+				??????imx6_pcie_resume_noirq)
drivers/pci/controller/dwc/pci-imx6.c +2 -0
+};
+

?static int imx6_pcie_probe(struct platform_device *pdev) ?{

Quoted 3 lines

? struct device *dev = &pdev->dev;
? struct dw_pcie *pci;
? struct imx6_pcie *imx6_pcie;

drivers/pci/controller/dwc/pci-imx6.c +0 -0
@@ -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,
 ?};

?

author
#12 lore raw
Quoted 13 lines · Lucas Stach

Hi Leonard,

Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:

Quoted 10 lines

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.

Quoted 15 lines

drivers/pci/controller/dwc/pci-imx6.c static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie, · line 709

Quoted 10 lines
drivers/pci/controller/dwc/pci-imx6.c +10 -0
+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.

Quoted 11 lines
Quoted 11 lines
drivers/pci/controller/dwc/pci-imx6.c +11 -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);
Quoted 8 lines

drivers/pci/controller/dwc/pci-imx6.c static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie, · line 759

Quoted 6 lines
drivers/pci/controller/dwc/pci-imx6.c +6 -0
+	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.

maintainer
#13 lore raw

Am Montag, den 23.07.2018, 12:37 +0000 schrieb Leonard Crestez:

Quoted 34 lines · Lucas Stach
Quoted 17 lines

Hi Leonard,

Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:

Quoted 14 lines

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.

(quoted diff) +2 -0
+static void imx6_pcie_ltssm_disable(struct device *dev)
+{
Quoted 1 line
Quoted 1 line

Review

Quoted 1 line
(quoted diff) +1 -0
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);

+

Quoted 11 lines

Review

Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/pci/controller/dwc/pci-imx6.c +5 -0
+	switch (imx6_pcie->variant) {
+	case IMX6Q:
+	case IMX6SX:
+	case IMX6QP:
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
(quoted diff) +1 -0
+				???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.

Quoted 6 lines

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?

Quoted 20 lines
Quoted 14 lines
Quoted 14 lines
drivers/pci/controller/dwc/pci-imx6.c +2 -0
+static int imx6_pcie_resume_noirq(struct device *dev)
+{
Quoted 3 lines
Quoted 3 lines

drivers/pci/controller/dwc/pci-imx6.c static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie, · line 747

Quoted 3 lines
drivers/pci/controller/dwc/pci-imx6.c +3 -0
+	int ret = 0;
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+	struct pcie_port *pp = &imx6_pcie->pci->pp;

+

Quoted 2 lines
Quoted 2 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+	if (imx6_pcie->variant != IMX7D)
+		return 0;

+

Quoted 4 lines
Quoted 4 lines

drivers/pci/controller/dwc/pci-imx6.c static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie, · line 755

Quoted 4 lines
drivers/pci/controller/dwc/pci-imx6.c +4 -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);

+

Quoted 5 lines

drivers/pci/controller/dwc/pci-imx6.c static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie, · line 759

Quoted 3 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/pci/controller/dwc/pci-imx6.c +2 -0
+	ret = imx6_pcie_establish_link(imx6_pcie);
+	if (ret < 0)
drivers/pci/controller/dwc/pci-imx6.c +1 -0
+		pr_err("pcie link is down after resume.\n");

dev_err(), please.

Quoted 8 lines

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.

Quoted 1 line

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

Yes, that's fine with me.

Quoted 8 lines

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

author
#14 lore raw
Quoted 36 lines · Lucas Stach

Am Montag, den 23.07.2018, 12:37 +0000 schrieb Leonard Crestez:

Quoted 35 lines · Lucas Stach
Quoted 11 lines

Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:

Quoted 10 lines

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.

Quoted 15 lines

drivers/pci/controller/dwc/pci-imx6.c static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie, · line 709

Quoted 10 lines
drivers/pci/controller/dwc/pci-imx6.c +10 -0
+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]".

Quoted 7 lines
Quoted 3 lines

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.

maintainer
#15 lore raw

Am Dienstag, den 24.07.2018, 12:04 +0000 schrieb Leonard Crestez:

Quoted 41 lines · Lucas Stach
Quoted 40 lines

Am Montag, den 23.07.2018, 12:37 +0000 schrieb Leonard Crestez:

Quoted 38 lines · Lucas Stach
Quoted 15 lines

Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez:

Quoted 14 lines

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.

(quoted diff) +2 -0
+static void imx6_pcie_ltssm_disable(struct device *dev)
+{
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

Review

Quoted 1 line
(quoted diff) +1 -0
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);

+

Quoted 11 lines

Review

Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/pci/controller/dwc/pci-imx6.c +5 -0
+	switch (imx6_pcie->variant) {
+	case IMX6Q:
+	case IMX6SX:
+	case IMX6QP:
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
(quoted diff) +1 -0
+				???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.

Quoted 7 lines
Quoted 3 lines

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.

Quoted 3 lines

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