linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] PCI: imx: Initial imx7d pm support
@ 2018-07-20 12:47 Leonard Crestez
  2018-07-20 12:47 ` [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping" Leonard Crestez
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Leonard Crestez @ 2018-07-20 12:47 UTC (permalink / raw)
  To: Lucas Stach, Richard Zhu, Andrey Smirnov
  Cc: Shawn Guo, Joao Pinto, Jingoo Han, Bjorn Helgaas,
	Lorenzo Pieralisi, linux-pci, linux-pm, linux-arm-kernel,
	linux-kernel, Fabio Estevam, Dong Aisheng, kernel, linux-imx

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"
  2018-07-20 12:47 [PATCH v2 0/3] PCI: imx: Initial imx7d pm support Leonard Crestez
@ 2018-07-20 12:47 ` Leonard Crestez
  2018-07-20 15:33   ` Andrey Smirnov
  2018-07-20 12:47 ` [PATCH v2 2/3] reset: imx7: Fix always writing bits as 0 Leonard Crestez
  2018-07-20 12:47 ` [PATCH v2 3/3] PCI: imx: Initial imx7d pm support Leonard Crestez
  2 siblings, 1 reply; 16+ messages in thread
From: Leonard Crestez @ 2018-07-20 12:47 UTC (permalink / raw)
  To: Lucas Stach, Richard Zhu, Andrey Smirnov
  Cc: Shawn Guo, Joao Pinto, Jingoo Han, Bjorn Helgaas,
	Lorenzo Pieralisi, linux-pci, linux-pm, linux-arm-kernel,
	linux-kernel, Fabio Estevam, Dong Aisheng, kernel, linux-imx

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 2/3] reset: imx7: Fix always writing bits as 0
  2018-07-20 12:47 [PATCH v2 0/3] PCI: imx: Initial imx7d pm support Leonard Crestez
  2018-07-20 12:47 ` [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping" Leonard Crestez
@ 2018-07-20 12:47 ` Leonard Crestez
  2018-07-23  9:41   ` Lucas Stach
  2018-07-20 12:47 ` [PATCH v2 3/3] PCI: imx: Initial imx7d pm support Leonard Crestez
  2 siblings, 1 reply; 16+ messages in thread
From: Leonard Crestez @ 2018-07-20 12:47 UTC (permalink / raw)
  To: Lucas Stach, Richard Zhu, Andrey Smirnov
  Cc: Shawn Guo, Joao Pinto, Jingoo Han, Bjorn Helgaas,
	Lorenzo Pieralisi, linux-pci, linux-pm, linux-arm-kernel,
	linux-kernel, Fabio Estevam, Dong Aisheng, kernel, linux-imx

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 3/3] PCI: imx: Initial imx7d pm support
  2018-07-20 12:47 [PATCH v2 0/3] PCI: imx: Initial imx7d pm support Leonard Crestez
  2018-07-20 12:47 ` [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping" Leonard Crestez
  2018-07-20 12:47 ` [PATCH v2 2/3] reset: imx7: Fix always writing bits as 0 Leonard Crestez
@ 2018-07-20 12:47 ` Leonard Crestez
  2018-07-20 13:38   ` Fabio Estevam
  2018-07-23  9:38   ` Lucas Stach
  2 siblings, 2 replies; 16+ messages in thread
From: Leonard Crestez @ 2018-07-20 12:47 UTC (permalink / raw)
  To: Lucas Stach, Richard Zhu, Andrey Smirnov
  Cc: Shawn Guo, Joao Pinto, Jingoo Han, Bjorn Helgaas,
	Lorenzo Pieralisi, linux-pci, linux-pm, linux-arm-kernel,
	linux-kernel, Fabio Estevam, Dong Aisheng, kernel, linux-imx

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support
  2018-07-20 12:47 ` [PATCH v2 3/3] PCI: imx: Initial imx7d pm support Leonard Crestez
@ 2018-07-20 13:38   ` Fabio Estevam
  2018-07-23  9:38   ` Lucas Stach
  1 sibling, 0 replies; 16+ messages in thread
From: Fabio Estevam @ 2018-07-20 13:38 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Lucas Stach, Richard Zhu, Andrey Smirnov, Dong Aisheng,
	Joao Pinto, linux-pm, Jingoo Han, linux-kernel, Fabio Estevam,
	Lorenzo Pieralisi, NXP Linux Team, Sascha Hauer, linux-pci,
	Bjorn Helgaas, Shawn Guo,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

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().

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"
  2018-07-20 12:47 ` [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping" Leonard Crestez
@ 2018-07-20 15:33   ` Andrey Smirnov
  2018-07-23 12:41     ` Leonard Crestez
  0 siblings, 1 reply; 16+ messages in thread
From: Andrey Smirnov @ 2018-07-20 15:33 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Lucas Stach, Richard Zhu, Shawn Guo, Joao.Pinto, Jingoo Han,
	Bjorn Helgaas, lorenzo.pieralisi, linux-pci, linux-pm,
	linux-arm-kernel, linux-kernel, Fabio Estevam, Dong Aisheng,
	Sascha Hauer, linux-imx

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support
  2018-07-20 12:47 ` [PATCH v2 3/3] PCI: imx: Initial imx7d pm support Leonard Crestez
  2018-07-20 13:38   ` Fabio Estevam
@ 2018-07-23  9:38   ` Lucas Stach
  2018-07-23 12:37     ` Leonard Crestez
  1 sibling, 1 reply; 16+ messages in thread
From: Lucas Stach @ 2018-07-23  9:38 UTC (permalink / raw)
  To: Leonard Crestez, Richard Zhu, Andrey Smirnov
  Cc: Shawn Guo, Joao Pinto, Jingoo Han, Bjorn Helgaas,
	Lorenzo Pieralisi, linux-pci, linux-pm, linux-arm-kernel,
	linux-kernel, Fabio Estevam, Dong Aisheng, kernel, linux-imx

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,
>  };
>  

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 2/3] reset: imx7: Fix always writing bits as 0
  2018-07-20 12:47 ` [PATCH v2 2/3] reset: imx7: Fix always writing bits as 0 Leonard Crestez
@ 2018-07-23  9:41   ` Lucas Stach
  2018-07-23 11:02     ` Philipp Zabel
  0 siblings, 1 reply; 16+ messages in thread
From: Lucas Stach @ 2018-07-23  9:41 UTC (permalink / raw)
  To: Leonard Crestez, Richard Zhu, Andrey Smirnov, Philipp Zabel
  Cc: Shawn Guo, Joao Pinto, Jingoo Han, Bjorn Helgaas,
	Lorenzo Pieralisi, linux-pci, linux-pm, linux-arm-kernel,
	linux-kernel, Fabio Estevam, Dong Aisheng, kernel, linux-imx

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 2/3] reset: imx7: Fix always writing bits as 0
  2018-07-23  9:41   ` Lucas Stach
@ 2018-07-23 11:02     ` Philipp Zabel
  0 siblings, 0 replies; 16+ messages in thread
From: Philipp Zabel @ 2018-07-23 11:02 UTC (permalink / raw)
  To: Lucas Stach, Leonard Crestez, Richard Zhu, Andrey Smirnov
  Cc: Shawn Guo, Joao Pinto, Jingoo Han, Bjorn Helgaas,
	Lorenzo Pieralisi, linux-pci, linux-pm, linux-arm-kernel,
	linux-kernel, Fabio Estevam, Dong Aisheng, kernel, linux-imx

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support
  2018-07-23  9:38   ` Lucas Stach
@ 2018-07-23 12:37     ` Leonard Crestez
  2018-07-24 10:09       ` Lucas Stach
  0 siblings, 1 reply; 16+ messages in thread
From: Leonard Crestez @ 2018-07-23 12:37 UTC (permalink / raw)
  To: l.stach, Richard Zhu, Fabio Estevam
  Cc: A.s. Dong, linux-kernel, dl-linux-imx, jingoohan1,
	lorenzo.pieralisi, linux-pm, Joao.Pinto, shawnguo,
	linux-arm-kernel, andrew.smirnov, bhelgaas, linux-pci, kernel

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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"
  2018-07-20 15:33   ` Andrey Smirnov
@ 2018-07-23 12:41     ` Leonard Crestez
  2018-07-23 18:38       ` Andrey Smirnov
  0 siblings, 1 reply; 16+ messages in thread
From: Leonard Crestez @ 2018-07-23 12:41 UTC (permalink / raw)
  To: andrew.smirnov
  Cc: Richard Zhu, linux-kernel, A.s. Dong, jingoohan1, dl-linux-imx,
	lorenzo.pieralisi, linux-pm, Fabio Estevam, Joao.Pinto, shawnguo,
	linux-arm-kernel, bhelgaas, l.stach, kernel, linux-pci

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"
  2018-07-23 12:41     ` Leonard Crestez
@ 2018-07-23 18:38       ` Andrey Smirnov
  2018-07-24 11:34         ` Leonard Crestez
  0 siblings, 1 reply; 16+ messages in thread
From: Andrey Smirnov @ 2018-07-23 18:38 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Richard Zhu, linux-kernel, Dong Aisheng, Jingoo Han, linux-imx,
	lorenzo.pieralisi, linux-pm, Fabio Estevam, Joao.Pinto,
	Shawn Guo, linux-arm-kernel, Bjorn Helgaas, Lucas Stach,
	Sascha Hauer, linux-pci

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support
  2018-07-23 12:37     ` Leonard Crestez
@ 2018-07-24 10:09       ` Lucas Stach
  2018-07-24 12:04         ` Leonard Crestez
  0 siblings, 1 reply; 16+ messages in thread
From: Lucas Stach @ 2018-07-24 10:09 UTC (permalink / raw)
  To: Leonard Crestez, Richard Zhu, Fabio Estevam
  Cc: A.s. Dong, linux-kernel, dl-linux-imx, jingoohan1,
	lorenzo.pieralisi, linux-pm, Joao.Pinto, shawnguo,
	linux-arm-kernel, andrew.smirnov, bhelgaas, linux-pci, kernel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping"
  2018-07-23 18:38       ` Andrey Smirnov
@ 2018-07-24 11:34         ` Leonard Crestez
  0 siblings, 0 replies; 16+ messages in thread
From: Leonard Crestez @ 2018-07-24 11:34 UTC (permalink / raw)
  To: andrew.smirnov
  Cc: Richard Zhu, linux-kernel, dl-linux-imx, jingoohan1, A.s. Dong,
	lorenzo.pieralisi, linux-pm, Fabio Estevam, Joao.Pinto, shawnguo,
	linux-arm-kernel, bhelgaas, l.stach, kernel, linux-pci

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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support
  2018-07-24 10:09       ` Lucas Stach
@ 2018-07-24 12:04         ` Leonard Crestez
  2018-07-24 12:28           ` Lucas Stach
  0 siblings, 1 reply; 16+ messages in thread
From: Leonard Crestez @ 2018-07-24 12:04 UTC (permalink / raw)
  To: l.stach, Richard Zhu, Fabio Estevam
  Cc: dl-linux-imx, linux-kernel, A.s. Dong, jingoohan1,
	lorenzo.pieralisi, linux-pm, Joao.Pinto, shawnguo,
	linux-arm-kernel, andrew.smirnov, bhelgaas, linux-pci, kernel

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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support
  2018-07-24 12:04         ` Leonard Crestez
@ 2018-07-24 12:28           ` Lucas Stach
  0 siblings, 0 replies; 16+ messages in thread
From: Lucas Stach @ 2018-07-24 12:28 UTC (permalink / raw)
  To: Leonard Crestez, Richard Zhu, Fabio Estevam
  Cc: dl-linux-imx, linux-kernel, A.s. Dong, jingoohan1,
	lorenzo.pieralisi, linux-pm, Joao.Pinto, shawnguo,
	linux-arm-kernel, andrew.smirnov, bhelgaas, linux-pci, kernel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-07-24 12:28 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-20 12:47 [PATCH v2 0/3] PCI: imx: Initial imx7d pm support Leonard Crestez
2018-07-20 12:47 ` [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping" Leonard Crestez
2018-07-20 15:33   ` Andrey Smirnov
2018-07-23 12:41     ` Leonard Crestez
2018-07-23 18:38       ` Andrey Smirnov
2018-07-24 11:34         ` Leonard Crestez
2018-07-20 12:47 ` [PATCH v2 2/3] reset: imx7: Fix always writing bits as 0 Leonard Crestez
2018-07-23  9:41   ` Lucas Stach
2018-07-23 11:02     ` Philipp Zabel
2018-07-20 12:47 ` [PATCH v2 3/3] PCI: imx: Initial imx7d pm support Leonard Crestez
2018-07-20 13:38   ` Fabio Estevam
2018-07-23  9:38   ` Lucas Stach
2018-07-23 12:37     ` Leonard Crestez
2018-07-24 10:09       ` Lucas Stach
2018-07-24 12:04         ` Leonard Crestez
2018-07-24 12:28           ` Lucas Stach

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).