[PATCH 0/4] PCI: imx: Add PME_Turn_Off support

[PATCH 0/4] PCI: imx: Add PME_Turn_Off support

[Leonard Crestez]

When the root complex suspends it must send a PME_Turn_Off TLP.
Implement this by asserting the "turnoff" reset.

On imx7d this is functionality is part of the SRC and exposed through
the linux reset-controller subsystem. On imx6 equivalent bits are in the
IOMUXC GPR area which the imx6-pcie driver accesses directly.

This is only for imx7d right now but it's deliberately implemented as an
optional reset, ignoring the chip variant:
* Older dtbs won't have this reset so it will be ignored.
* Future chips might also expose this as a reset controller.

For example imx8m (not yet supported) has the exact same
PCIE_CTRL_APPS_TURNOFF bit in the same location.

---
Previously posted here: https://patchwork.kernel.org/cover/10565871/
Parts of that were already merged and available in linux-next so
reposting as a focused series.

This is not very complex but needs to be split because it touches
multiple trees. Merging out of order should be fine.

Some patches already carry acks for DT but somebody needs to ack the
pci/reset parts.

Leonard Crestez (4):
  reset: imx7: Add PCIE_CTRL_APPS_TURNOFF
  dt-bindings: imx6q-pcie: Add turnoff reset for imx7d
  ARM: dts: imx7d: Add turnoff reset
  PCI: imx: Add PME_Turn_Off support

 .../devicetree/bindings/pci/fsl,imx6q-pcie.txt    |  1 +
 arch/arm/boot/dts/imx7d.dtsi                      |  5 +++--
 drivers/pci/controller/dwc/pci-imx6.c             | 15 +++++++++++++++
 drivers/reset/reset-imx7.c                        |  1 +
 include/dt-bindings/reset/imx7-reset.h            |  4 +++-
 5 files changed, 23 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH 1/4] reset: imx7: Add PCIE_CTRL_APPS_TURNOFF

[Leonard Crestez]

This is required for the imx pci driver to send the PME_Turn_Off TLP.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 drivers/reset/reset-imx7.c             | 1 +
 include/dt-bindings/reset/imx7-reset.h | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
index 97d9f08271c5..77911fa8f31d 100644
--- a/drivers/reset/reset-imx7.c
+++ b/drivers/reset/reset-imx7.c
@@ -65,10 +65,11 @@ static const struct imx7_src_signal imx7_src_signals[IMX7_RESET_NUM] = {
 	[IMX7_RESET_MIPI_PHY_MRST]	= { SRC_MIPIPHY_RCR, BIT(1) },
 	[IMX7_RESET_MIPI_PHY_SRST]	= { SRC_MIPIPHY_RCR, BIT(2) },
 	[IMX7_RESET_PCIEPHY]		= { SRC_PCIEPHY_RCR, BIT(2) | BIT(1) },
 	[IMX7_RESET_PCIEPHY_PERST]	= { SRC_PCIEPHY_RCR, BIT(3) },
 	[IMX7_RESET_PCIE_CTRL_APPS_EN]	= { SRC_PCIEPHY_RCR, BIT(6) },
+	[IMX7_RESET_PCIE_CTRL_APPS_TURNOFF] = { SRC_PCIEPHY_RCR, BIT(11) },
 	[IMX7_RESET_DDRC_PRST]		= { SRC_DDRC_RCR, BIT(0) },
 	[IMX7_RESET_DDRC_CORE_RST]	= { SRC_DDRC_RCR, BIT(1) },
 };
 
 static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
diff --git a/include/dt-bindings/reset/imx7-reset.h b/include/dt-bindings/reset/imx7-reset.h
index 63948170c7b2..31b3f87dde9a 100644
--- a/include/dt-bindings/reset/imx7-reset.h
+++ b/include/dt-bindings/reset/imx7-reset.h
@@ -54,9 +54,11 @@
  */
 #define IMX7_RESET_PCIE_CTRL_APPS_EN	22
 #define IMX7_RESET_DDRC_PRST		23
 #define IMX7_RESET_DDRC_CORE_RST	24
 
-#define IMX7_RESET_NUM			25
+#define IMX7_RESET_PCIE_CTRL_APPS_TURNOFF 25
+
+#define IMX7_RESET_NUM			26
 
 #endif
 
-- 
2.17.1

[PATCH 2/4] dt-bindings: imx6q-pcie: Add turnoff reset for imx7d

[Leonard Crestez]

This is documented as "required" but won't be present in old dtbs.

These resets are also present on other imx chips but right now only
imx7d implements them through the reset controller subsystem.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
index cb33421184a0..f37494d5a7be 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
@@ -48,10 +48,11 @@ Additional required properties for imx7d-pcie:
 - resets: Must contain phandles to PCIe-related reset lines exposed by SRC
   IP block
 - reset-names: Must contain the following entires:
 	       - "pciephy"
 	       - "apps"
+	       - "turnoff"
 
 Example:
 
 	pcie@01000000 {
 		compatible = "fsl,imx6q-pcie", "snps,dw-pcie";
-- 
2.17.1

[PATCH 3/4] ARM: dts: imx7d: Add turnoff reset

[Leonard Crestez]

This is required for the imx pci driver to send the PME_Turn_Off TLP.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Acked-by: Shawn Guo <shawnguo@kernel.org>
---
 arch/arm/boot/dts/imx7d.dtsi | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi
index fa675b8d84e9..a6b2932634f5 100644
--- a/arch/arm/boot/dts/imx7d.dtsi
+++ b/arch/arm/boot/dts/imx7d.dtsi
@@ -145,12 +145,13 @@
 					 <&clks IMX7D_PLL_ENET_MAIN_100M_CLK>;
 
 		fsl,max-link-speed = <2>;
 		power-domains = <&pgc_pcie_phy>;
 		resets = <&src IMX7_RESET_PCIEPHY>,
-			 <&src IMX7_RESET_PCIE_CTRL_APPS_EN>;
-		reset-names = "pciephy", "apps";
+			 <&src IMX7_RESET_PCIE_CTRL_APPS_EN>,
+			 <&src IMX7_RESET_PCIE_CTRL_APPS_TURNOFF>;
+		reset-names = "pciephy", "apps", "turnoff";
 		status = "disabled";
 	};
 };
 
 &ca_funnel_ports {
-- 
2.17.1

[PATCH 4/4] PCI: imx: Add PME_Turn_Off support

[Leonard Crestez]

When the root complex suspends it must send a PME_Turn_Off TLP.
Implement this by asserting the "turnoff" reset.

On imx7d this is functionality is part of the SRC and exposed through
the linux reset-controller subsystem. On imx6 equivalent bits are in the
IOMUXC GPR area which the imx6-pcie driver accesses directly.

This is only for imx7d right now but it's deliberately implemented as an
optional reset, ignoring the chip variant:
* Older dtbs won't have this reset so it will be ignored.
* Future chips might also expose this as a reset controller.

For example imx8m (not yet supported) has the exact same
PCIE_CTRL_APPS_TURNOFF bit in the same location.

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 6ba16fd1373c..13cb1a200442 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -50,10 +50,11 @@ struct imx6_pcie {
 	struct clk		*pcie_inbound_axi;
 	struct clk		*pcie;
 	struct regmap		*iomuxc_gpr;
 	struct reset_control	*pciephy_reset;
 	struct reset_control	*apps_reset;
+	struct reset_control	*turnoff_reset;
 	enum imx6_pcie_variants variant;
 	u32			tx_deemph_gen1;
 	u32			tx_deemph_gen2_3p5db;
 	u32			tx_deemph_gen2_6db;
 	u32			tx_swing_full;
@@ -812,10 +813,16 @@ static void imx6_pcie_ltssm_disable(struct device *dev)
 	default:
 		dev_err(dev, "ltssm_disable not supported\n");
 	}
 }
 
+static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
+{
+	reset_control_assert(imx6_pcie->turnoff_reset);
+	reset_control_deassert(imx6_pcie->turnoff_reset);
+}
+
 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);
@@ -832,10 +839,11 @@ 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_pm_turnoff(imx6_pcie);
 	imx6_pcie_clk_disable(imx6_pcie);
 	imx6_pcie_ltssm_disable(dev);
 
 	return 0;
 }
@@ -959,10 +967,17 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 		break;
 	default:
 		break;
 	}
 
+	/* Grab turnoff reset */
+	imx6_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff");
+	if (IS_ERR(imx6_pcie->turnoff_reset)) {
+		dev_err(dev, "Failed to get TURNOFF reset control\n");
+		return PTR_ERR(imx6_pcie->turnoff_reset);
+	}
+
 	/* Grab GPR config register range */
 	imx6_pcie->iomuxc_gpr =
 		 syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
 	if (IS_ERR(imx6_pcie->iomuxc_gpr)) {
 		dev_err(dev, "unable to find iomuxc registers\n");
-- 
2.17.1

Re: [PATCH 4/4] PCI: imx: Add PME_Turn_Off support

[Lorenzo Pieralisi]

On Mon, Oct 01, 2018 at 10:53:48PM +0300, Leonard Crestez wrote:
> When the root complex suspends it must send a PME_Turn_Off TLP.
> Implement this by asserting the "turnoff" reset.
> 
> On imx7d this is functionality is part of the SRC and exposed through
> the linux reset-controller subsystem. On imx6 equivalent bits are in the
> IOMUXC GPR area which the imx6-pcie driver accesses directly.
> 
> This is only for imx7d right now but it's deliberately implemented as an
> optional reset, ignoring the chip variant:
> * Older dtbs won't have this reset so it will be ignored.
> * Future chips might also expose this as a reset controller.
> 
> For example imx8m (not yet supported) has the exact same
> PCIE_CTRL_APPS_TURNOFF bit in the same location.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)

I need an ACK from Lucas on this patch to merge this series; also, if
you can keep versioning your patch series please it would be easier for
me to track them, I understand the patches content changed over time but
that's for future postings too.

Thanks,
Lorenzo

> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 6ba16fd1373c..13cb1a200442 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -50,10 +50,11 @@ struct imx6_pcie {
>  	struct clk		*pcie_inbound_axi;
>  	struct clk		*pcie;
>  	struct regmap		*iomuxc_gpr;
>  	struct reset_control	*pciephy_reset;
>  	struct reset_control	*apps_reset;
> +	struct reset_control	*turnoff_reset;
>  	enum imx6_pcie_variants variant;
>  	u32			tx_deemph_gen1;
>  	u32			tx_deemph_gen2_3p5db;
>  	u32			tx_deemph_gen2_6db;
>  	u32			tx_swing_full;
> @@ -812,10 +813,16 @@ static void imx6_pcie_ltssm_disable(struct device *dev)
>  	default:
>  		dev_err(dev, "ltssm_disable not supported\n");
>  	}
>  }
>  
> +static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
> +{
> +	reset_control_assert(imx6_pcie->turnoff_reset);
> +	reset_control_deassert(imx6_pcie->turnoff_reset);
> +}
> +
>  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);
> @@ -832,10 +839,11 @@ 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_pm_turnoff(imx6_pcie);
>  	imx6_pcie_clk_disable(imx6_pcie);
>  	imx6_pcie_ltssm_disable(dev);
>  
>  	return 0;
>  }
> @@ -959,10 +967,17 @@ static int imx6_pcie_probe(struct platform_device *pdev)
>  		break;
>  	default:
>  		break;
>  	}
>  
> +	/* Grab turnoff reset */
> +	imx6_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff");
> +	if (IS_ERR(imx6_pcie->turnoff_reset)) {
> +		dev_err(dev, "Failed to get TURNOFF reset control\n");
> +		return PTR_ERR(imx6_pcie->turnoff_reset);
> +	}
> +
>  	/* Grab GPR config register range */
>  	imx6_pcie->iomuxc_gpr =
>  		 syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
>  	if (IS_ERR(imx6_pcie->iomuxc_gpr)) {
>  		dev_err(dev, "unable to find iomuxc registers\n");
> -- 
> 2.17.1
> 

Re: [PATCH 4/4] PCI: imx: Add PME_Turn_Off support

[Lucas Stach]

Am Montag, den 01.10.2018, 22:53 +0300 schrieb Leonard Crestez:
> When the root complex suspends it must send a PME_Turn_Off TLP.
> Implement this by asserting the "turnoff" reset.
> 
> On imx7d this is functionality is part of the SRC and exposed through
> the linux reset-controller subsystem. On imx6 equivalent bits are in the
> IOMUXC GPR area which the imx6-pcie driver accesses directly.
> 
> This is only for imx7d right now but it's deliberately implemented as an
> optional reset, ignoring the chip variant:
> * Older dtbs won't have this reset so it will be ignored.
> * Future chips might also expose this as a reset controller.
> 
> For example imx8m (not yet supported) has the exact same
> PCIE_CTRL_APPS_TURNOFF bit in the same location.
> 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 6ba16fd1373c..13cb1a200442 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -50,10 +50,11 @@ struct imx6_pcie {
> > >  	struct clk		*pcie_inbound_axi;
> > >  	struct clk		*pcie;
> > >  	struct regmap		*iomuxc_gpr;
> > >  	struct reset_control	*pciephy_reset;
> > >  	struct reset_control	*apps_reset;
> > > +	struct reset_control	*turnoff_reset;
> >  	enum imx6_pcie_variants variant;
> > >  	u32			tx_deemph_gen1;
> > >  	u32			tx_deemph_gen2_3p5db;
> > >  	u32			tx_deemph_gen2_6db;
> > >  	u32			tx_swing_full;
> @@ -812,10 +813,16 @@ static void imx6_pcie_ltssm_disable(struct device *dev)
> >  	default:
> >  		dev_err(dev, "ltssm_disable not supported\n");
> >  	}
>  }
>  
> +static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
> +{
> > +	reset_control_assert(imx6_pcie->turnoff_reset);
> +	reset_control_deassert(imx6_pcie->turnoff_reset);

I'm a bit surprised to see no timing requirements here. I would have
expected that there is a minimum time from asserting the reset, so the
turnoff message gets transmitted to the EP before the clocks are
stopped.

Regards,
Lucas

> +}
> +
>  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);
> @@ -832,10 +839,11 @@ 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_pm_turnoff(imx6_pcie);
> >  	imx6_pcie_clk_disable(imx6_pcie);
> >  	imx6_pcie_ltssm_disable(dev);
>  
> >  	return 0;
>  }
> @@ -959,10 +967,17 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> >  		break;
> >  	default:
> >  		break;
> >  	}
>  
> > +	/* Grab turnoff reset */
> > +	imx6_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff");
> > +	if (IS_ERR(imx6_pcie->turnoff_reset)) {
> > +		dev_err(dev, "Failed to get TURNOFF reset control\n");
> > +		return PTR_ERR(imx6_pcie->turnoff_reset);
> > +	}
> +
> >  	/* Grab GPR config register range */
> >  	imx6_pcie->iomuxc_gpr =
> >  		 syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> >  	if (IS_ERR(imx6_pcie->iomuxc_gpr)) {
> >  		dev_err(dev, "unable to find iomuxc registers\n");

Re: [PATCH 4/4] PCI: imx: Add PME_Turn_Off support

[Leonard Crestez]

On Thu, 2018-10-04 at 10:59 +0200, Lucas Stach wrote:
> Am Montag, den 01.10.2018, 22:53 +0300 schrieb Leonard Crestez:
> > When the root complex suspends it must send a PME_Turn_Off TLP.
> > Implement this by asserting the "turnoff" reset.
> > 
> > +static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
> > +{
> > +	reset_control_assert(imx6_pcie->turnoff_reset);
> > +	reset_control_deassert(imx6_pcie->turnoff_reset);
> 
> I'm a bit surprised to see no timing requirements here. I would have
> expected that there is a minimum time from asserting the reset, so the
> turnoff message gets transmitted to the EP before the clocks are
> stopped.

According to the PCI standard after PME_Turn_Off is sent all components
must respond with PME_TO_Ack. There doesn't seem to be any bit exposed
to check if acks were received but the standard recommends a 1-10ms
timeout after which you should proceed anyway.

It seems the NXP vendor tree has an udelay(1000) which I missed, it
seems like an acceptable solution. Or maybe it should be msleep(10)?

--
Regards,
Leonard

Re: [PATCH 4/4] PCI: imx: Add PME_Turn_Off support

[Philipp Zabel]

On Thu, 2018-10-04 at 13:20 +0000, Leonard Crestez wrote:
> On Thu, 2018-10-04 at 10:59 +0200, Lucas Stach wrote:
> > Am Montag, den 01.10.2018, 22:53 +0300 schrieb Leonard Crestez:
> > > When the root complex suspends it must send a PME_Turn_Off TLP.
> > > Implement this by asserting the "turnoff" reset.
> > > 
> > > +static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
> > > +{
> > > +	reset_control_assert(imx6_pcie->turnoff_reset);
> > > +	reset_control_deassert(imx6_pcie->turnoff_reset);
> > 
> > I'm a bit surprised to see no timing requirements here. I would have
> > expected that there is a minimum time from asserting the reset, so the
> > turnoff message gets transmitted to the EP before the clocks are
> > stopped.
> 
> According to the PCI standard after PME_Turn_Off is sent all components
> must respond with PME_TO_Ack. There doesn't seem to be any bit exposed
> to check if acks were received but the standard recommends a 1-10ms
> timeout after which you should proceed anyway.
> 
> It seems the NXP vendor tree has an udelay(1000) which I missed, it
> seems like an acceptable solution. Or maybe it should be msleep(10)?

Maybe usleep_range(1000, 10000) ?

regards
Philipp

Re: [PATCH 4/4] PCI: imx: Add PME_Turn_Off support

[Lucas Stach]

Am Donnerstag, den 04.10.2018, 15:25 +0200 schrieb Philipp Zabel:
> On Thu, 2018-10-04 at 13:20 +0000, Leonard Crestez wrote:
> > On Thu, 2018-10-04 at 10:59 +0200, Lucas Stach wrote:
> > > Am Montag, den 01.10.2018, 22:53 +0300 schrieb Leonard Crestez:
> > > > When the root complex suspends it must send a PME_Turn_Off TLP.
> > > > Implement this by asserting the "turnoff" reset.
> > > > 
> > > > +static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
> > > > +{
> > > > > > > > +	reset_control_assert(imx6_pcie->turnoff_reset);
> > > > +	reset_control_deassert(imx6_pcie->turnoff_reset);
> > > 
> > > I'm a bit surprised to see no timing requirements here. I would have
> > > expected that there is a minimum time from asserting the reset, so the
> > > turnoff message gets transmitted to the EP before the clocks are
> > > stopped.
> > 
> > According to the PCI standard after PME_Turn_Off is sent all components
> > must respond with PME_TO_Ack. There doesn't seem to be any bit exposed
> > to check if acks were received but the standard recommends a 1-10ms
> > timeout after which you should proceed anyway.
> > 
> > It seems the NXP vendor tree has an udelay(1000) which I missed, it
> > seems like an acceptable solution. Or maybe it should be msleep(10)?
> 
> Maybe usleep_range(1000, 10000) ?

Yep, we aren't in atomic context here and the timeout is relatively
short so usleep_range() looks like the correct thing to use.

Regards,
Lucas