[2/2] PCI: imx: Initial imx7d pm support

Message ID a6176c8bcfbca08ce4e93e304bdcd48c4600f8e2.1527621510.git.leonard.crestez@nxp.com
State Changes Requested
Delegated to: Lorenzo Pieralisi
Headers show
Series
  • PCI: Initial imx7d pm support
Related show

Commit Message

Leonard Crestez May 29, 2018, 7:39 p.m.
On imx7d the phy is turned off in suspend and must be reset on resume.
Right now lspci -v fails after a suspend/resume cycle, fix this by
adding minimal suspend/resume code from the nxp vendor tree.

This is currently only enabled for imx7 but the same sequence can be
applied to other imx pcie variants.

Tested on imx7d-sabresd with an Intel 5622ANHMW wireless pcie adapter.

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/dwc/pci-imx6.c | 94 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 89 insertions(+), 5 deletions(-)

Comments

Lucas Stach June 8, 2018, 2:33 p.m. | #1
Am Dienstag, den 29.05.2018, 22:39 +0300 schrieb Leonard Crestez:
> On imx7d the phy is turned off in suspend and must be reset on resume.
> Right now lspci -v fails after a suspend/resume cycle, fix this by
> adding minimal suspend/resume code from the nxp vendor tree.
> 
> This is currently only enabled for imx7 but the same sequence can be
> applied to other imx pcie variants.
> 
> Tested on imx7d-sabresd with an Intel 5622ANHMW wireless pcie adapter.
> 
> > 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/dwc/pci-imx6.c | 94 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 89 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c
> index 4818ef875f8a..ff6077eeb387 100644
> --- a/drivers/pci/dwc/pci-imx6.c
> +++ b/drivers/pci/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,80 @@ 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 int imx6_pcie_suspend_noirq(struct device *dev)
> +{
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> +
> +	if (imx6_pcie->variant == IMX7D) {

Instead of this large indented block, please just have an early return
at the start of this function, like:

if (imx6_pcie->variant != IMX7D)
	return 0;

Same for the resume function.

> +		/* Disable clks */
> > +		clk_disable_unprepare(imx6_pcie->pcie);
> > +		clk_disable_unprepare(imx6_pcie->pcie_phy);
> > +		clk_disable_unprepare(imx6_pcie->pcie_bus);
> > +		/* turn off external osc input */
> > +		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > +				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> > +				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> > +	}
> +
> > +	return 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);
> > +		break;
> > +	case IMX7D:
> > +		reset_control_assert(imx6_pcie->apps_reset);
> > +		break;
> > +	}
> +}
> +
> +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) {
> > +		imx6_pcie_ltssm_disable(dev);

The LTSSM disable seems misplaced here. I would have expected it to be
in the suspend function.

> +		imx6_pcie_assert_core_reset(imx6_pcie);
> > +		imx6_pcie_init_phy(imx6_pcie);
> > +		imx6_pcie_deassert_core_reset(imx6_pcie);
> +
> > +		/*
> > +		 * controller maybe turn off, re-configure again
> > +		 */
> > +		dw_pcie_setup_rc(pp);
> +
> > +		imx6_pcie_ltssm_enable(dev);
> +
> > +		ret = imx6_pcie_wait_for_link(imx6_pcie);
> > +		if (ret < 0)
> +			pr_info("pcie link is down after resume.\n");

If the PHY has been powered down and LTSSM stopped I guess we need to
do the full imx6_pcie_establish_link() dance again here to reliably re-
establish the link. The above seems unsafe.

> +	}
> +
> > +	return 0;
> +}
> +
> +static const struct dev_pm_ops imx6_pcie_pm_ops = {
> > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx6_pcie_suspend_noirq,
> > +				      imx6_pcie_resume_noirq)
> +};
> +#endif

This structure must be outside of the ifdef, or you'll break the build
on !CONFIG_PM_SLEEP.

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

Regards,
Lucas
Leonard Crestez July 2, 2018, 5:18 p.m. | #2
On Fri, 2018-06-08 at 16:33 +0200, Lucas Stach wrote:
> Am Dienstag, den 29.05.2018, 22:39 +0300 schrieb Leonard Crestez:
> > On imx7d the phy is turned off in suspend and must be reset on resume.
> > Right now lspci -v fails after a suspend/resume cycle, fix this by
> > adding minimal suspend/resume code from the nxp vendor tree.
> > 
> > This is currently only enabled for imx7 but the same sequence can be
> > applied to other imx pcie variants.

> > +#ifdef CONFIG_PM_SLEEP
> > +static int imx6_pcie_suspend_noirq(struct device *dev)
> > +{
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > +
> > +	if (imx6_pcie->variant == IMX7D) {
> 
> Instead of this large indented block, please just have an early return
> at the start of this function, like:
> 
> if (imx6_pcie->variant != IMX7D)
> 	return 0;
> 
> Same for the resume function.

OK. The resume sequence is mostly the same for all SOCs where it
applies.

> > +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) {
> > +		imx6_pcie_ltssm_disable(dev);
> 
> The LTSSM disable seems misplaced here. I would have expected it to be
> in the suspend function.

This is a requirement for reinitializing the core: LTSSM needs to be
turned off during initialization.

> > +		/*
> > +		 * controller maybe turn off, re-configure again
> > +		 */
> > +		dw_pcie_setup_rc(pp);
> > +
> > +		imx6_pcie_ltssm_enable(dev);
> > +
> > +		ret = imx6_pcie_wait_for_link(imx6_pcie);
> > +		if (ret < 0)
> > +			pr_info("pcie link is down after resume.\n");
> 
> If the PHY has been powered down and LTSSM stopped I guess we need to
> do the full imx6_pcie_establish_link() dance again here to reliably re-
> establish the link. The above seems unsafe.

What imx6_pcie_establish_link does additionally is some workaround for
link gen detection. I agree that it should be included.

This would make resume mostly the same as imx_pcie_host_init except for
dw_pcie_msi_init; that needs to be saved/restored separately.


Another issue that should be discussed here is that on 6sx and 7d the
gpc PCIE power domain is not defined correctly: the PCIE block is in
the DISPMIX domain on both SOCs and it is only PCIE_PHY which has a
separate power domain.

This matters: enabling power-gating for displays will break pcie if
this relationship is not taken into account. Here are some options:

1) Make &pd_pcie a child of &pd_disp by hacking into gpc probe
functions and calling pm_genpd_add_subdomain. Not very nice.

2) Support nesting PGCs in GPC code? Lots of code and still an
incorrect representation of hardware.

3) Set power-domains = <&pd_disp> and enable runtime PM on &pd_pcie?

4) Add separate devices for the pcie-phy. These would be mostly empty
but still different, for example on imx6sx the PHY is not even
accessible on the bus but only though PCIE registers.

Solutions 1-3 seem like workarounds while 4 seems excessive, would
appreciate some feedback.

--
Regards,
Leonard
Lucas Stach July 3, 2018, 8:42 a.m. | #3
Am Montag, den 02.07.2018, 17:18 +0000 schrieb Leonard Crestez:
> On Fri, 2018-06-08 at 16:33 +0200, Lucas Stach wrote:
> > Am Dienstag, den 29.05.2018, 22:39 +0300 schrieb Leonard Crestez:
> > > On imx7d the phy is turned off in suspend and must be reset on resume.
> > > Right now lspci -v fails after a suspend/resume cycle, fix this by
> > > adding minimal suspend/resume code from the nxp vendor tree.
> > > 
> > > This is currently only enabled for imx7 but the same sequence can be
> > > applied to other imx pcie variants.
> > > +#ifdef CONFIG_PM_SLEEP
> > > +static int imx6_pcie_suspend_noirq(struct device *dev)
> > > +{
> > > > > > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > > +
> > > +	if (imx6_pcie->variant == IMX7D) {
> > 
> > Instead of this large indented block, please just have an early return
> > at the start of this function, like:
> > 
> > if (imx6_pcie->variant != IMX7D)
> > 	return 0;
> > 
> > Same for the resume function.
> 
> OK. The resume sequence is mostly the same for all SOCs where it
> applies.
> 
> > > +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) {
> > > +		imx6_pcie_ltssm_disable(dev);
> > 
> > The LTSSM disable seems misplaced here. I would have expected it to be
> > in the suspend function.
> 
> This is a requirement for reinitializing the core: LTSSM needs to be
> turned off during initialization.

If you disable LTSSM during suspend, it should be off when entering
this resume function, no?

> > > +		/*
> > > > > > +		 * controller maybe turn off, re-configure again
> > > > > > +		 */
> > > > > > +		dw_pcie_setup_rc(pp);
> > > +
> > > > > > +		imx6_pcie_ltssm_enable(dev);
> > > +
> > > > > > +		ret = imx6_pcie_wait_for_link(imx6_pcie);
> > > > > > +		if (ret < 0)
> > > +			pr_info("pcie link is down after resume.\n");
> > 
> > If the PHY has been powered down and LTSSM stopped I guess we need to
> > do the full imx6_pcie_establish_link() dance again here to reliably re-
> > establish the link. The above seems unsafe.
> 
> What imx6_pcie_establish_link does additionally is some workaround for
> link gen detection. I agree that it should be included.
> 
> This would make resume mostly the same as imx_pcie_host_init except for
> dw_pcie_msi_init; that needs to be saved/restored separately.

Right, so maybe we can even cut down on lines of code by splitting and
reusing existing functions.

> 
> Another issue that should be discussed here is that on 6sx and 7d the
> gpc PCIE power domain is not defined correctly: the PCIE block is in
> the DISPMIX domain on both SOCs and it is only PCIE_PHY which has a
> separate power domain.
> 
> This matters: enabling power-gating for displays will break pcie if
> this relationship is not taken into account. Here are some options:
> 
> 1) Make &pd_pcie a child of &pd_disp by hacking into gpc probe
> functions and calling pm_genpd_add_subdomain. Not very nice.
> 
> 2) Support nesting PGCs in GPC code? Lots of code and still an
> incorrect representation of hardware.
> 
> 3) Set power-domains = <&pd_disp> and enable runtime PM on &pd_pcie?
> 
> 4) Add separate devices for the pcie-phy. These would be mostly empty
> but still different, for example on imx6sx the PHY is not even
> accessible on the bus but only though PCIE registers.

5) Take a look at the linux-pm list. ;) The power domain framework has
just gained support for multiple power domains per device. I think that
 is the right solution for this, as like you mentioned the PHY isn't
really a separate block on i.MX6, but part of the PCIe controller
device.

Regards,
Lucas
Lorenzo Pieralisi July 4, 2018, 4:37 p.m. | #4
On Tue, Jul 03, 2018 at 10:42:08AM +0200, Lucas Stach wrote:
> Am Montag, den 02.07.2018, 17:18 +0000 schrieb Leonard Crestez:
> > On Fri, 2018-06-08 at 16:33 +0200, Lucas Stach wrote:
> > > Am Dienstag, den 29.05.2018, 22:39 +0300 schrieb Leonard Crestez:
> > > > On imx7d the phy is turned off in suspend and must be reset on resume.
> > > > Right now lspci -v fails after a suspend/resume cycle, fix this by
> > > > adding minimal suspend/resume code from the nxp vendor tree.
> > > > 
> > > > This is currently only enabled for imx7 but the same sequence can be
> > > > applied to other imx pcie variants.
> > > > +#ifdef CONFIG_PM_SLEEP
> > > > +static int imx6_pcie_suspend_noirq(struct device *dev)
> > > > +{
> > > > > > > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > > > +
> > > > +	if (imx6_pcie->variant == IMX7D) {
> > > 
> > > Instead of this large indented block, please just have an early return
> > > at the start of this function, like:
> > > 
> > > if (imx6_pcie->variant != IMX7D)
> > > 	return 0;
> > > 
> > > Same for the resume function.
> > 
> > OK. The resume sequence is mostly the same for all SOCs where it
> > applies.
> > 
> > > > +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) {
> > > > +		imx6_pcie_ltssm_disable(dev);
> > > 
> > > The LTSSM disable seems misplaced here. I would have expected it to be
> > > in the suspend function.
> > 
> > This is a requirement for reinitializing the core: LTSSM needs to be
> > turned off during initialization.
> 
> If you disable LTSSM during suspend, it should be off when entering
> this resume function, no?
> 
> > > > +		/*
> > > > > > > +		 * controller maybe turn off, re-configure again
> > > > > > > +		 */
> > > > > > > +		dw_pcie_setup_rc(pp);
> > > > +
> > > > > > > +		imx6_pcie_ltssm_enable(dev);
> > > > +
> > > > > > > +		ret = imx6_pcie_wait_for_link(imx6_pcie);
> > > > > > > +		if (ret < 0)
> > > > +			pr_info("pcie link is down after resume.\n");
> > > 
> > > If the PHY has been powered down and LTSSM stopped I guess we need to
> > > do the full imx6_pcie_establish_link() dance again here to reliably re-
> > > establish the link. The above seems unsafe.
> > 
> > What imx6_pcie_establish_link does additionally is some workaround for
> > link gen detection. I agree that it should be included.
> > 
> > This would make resume mostly the same as imx_pcie_host_init except for
> > dw_pcie_msi_init; that needs to be saved/restored separately.
> 
> Right, so maybe we can even cut down on lines of code by splitting and
> reusing existing functions.
> 
> > 
> > Another issue that should be discussed here is that on 6sx and 7d the
> > gpc PCIE power domain is not defined correctly: the PCIE block is in
> > the DISPMIX domain on both SOCs and it is only PCIE_PHY which has a
> > separate power domain.
> > 
> > This matters: enabling power-gating for displays will break pcie if
> > this relationship is not taken into account. Here are some options:
> > 
> > 1) Make &pd_pcie a child of &pd_disp by hacking into gpc probe
> > functions and calling pm_genpd_add_subdomain. Not very nice.
> > 
> > 2) Support nesting PGCs in GPC code? Lots of code and still an
> > incorrect representation of hardware.
> > 
> > 3) Set power-domains = <&pd_disp> and enable runtime PM on &pd_pcie?
> > 
> > 4) Add separate devices for the pcie-phy. These would be mostly empty
> > but still different, for example on imx6sx the PHY is not even
> > accessible on the bus but only though PCIE registers.
> 
> 5) Take a look at the linux-pm list. ;) The power domain framework has
> just gained support for multiple power domains per device. I think that
>  is the right solution for this, as like you mentioned the PHY isn't
> really a separate block on i.MX6, but part of the PCIe controller
> device.

From the discussion I take this as there is going to be a v2 so
I will mark this as Changes Requested, please let me know if that's
a problem.

Thanks,
Lorenzo
Leonard Crestez July 9, 2018, 2:59 p.m. | #5
On Tue, 2018-07-03 at 10:42 +0200, Lucas Stach wrote:
> Am Montag, den 02.07.2018, 17:18 +0000 schrieb Leonard Crestez:
> > On Fri, 2018-06-08 at 16:33 +0200, Lucas Stach wrote:
> > > Am Dienstag, den 29.05.2018, 22:39 +0300 schrieb Leonard Crestez:
> > > > On imx7d the phy is turned off in suspend and must be reset on resume.
> > > > Right now lspci -v fails after a suspend/resume cycle, fix this by
> > > > adding minimal suspend/resume code from the nxp vendor tree.
> > > > 
> > > > This is currently only enabled for imx7 but the same sequence can be
> > > > applied to other imx pcie variants.
> > > > +#ifdef CONFIG_PM_SLEEP
> > 
> > Another issue that should be discussed here is that on 6sx and 7d the
> > gpc PCIE power domain is not defined correctly: the PCIE block is in
> > the DISPMIX domain on both SOCs and it is only PCIE_PHY which has a
> > separate power domain.
> > 
> > This matters: enabling power-gating for displays will break pcie if
> > this relationship is not taken into account. Here are some options:
> > 
> > 1) Make &pd_pcie a child of &pd_disp by hacking into gpc probe
> > functions and calling pm_genpd_add_subdomain. Not very nice.
> > 
> > 2) Support nesting PGCs in GPC code? Lots of code and still an
> > incorrect representation of hardware.
> > 
> > 3) Set power-domains = <&pd_disp> and enable runtime PM on &pd_pcie?
> > 
> > 4) Add separate devices for the pcie-phy. These would be mostly empty
> > but still different, for example on imx6sx the PHY is not even
> > accessible on the bus but only though PCIE registers.
> 
> 5) Take a look at the linux-pm list. ;) The power domain framework has
> just gained support for multiple power domains per device. I think that
>  is the right solution for this, as like you mentioned the PHY isn't
> really a separate block on i.MX6, but part of the PCIe controller
> device.

Yes, I noticed that earlier but not that it was merged recently. It is
a better solution.

Unfortunately the multiple power-domain code seems to require devices
to explicitly fetch references to the power domains and manipulate
them. As far as I can tell this means that every device using multiple
power domains has to be modified to do so. In this particular case it
would be useful to just have all power domains turned on before probe,
just like when a single power-domain is specified:

    power-domains = <&pd_pci>, <&pd_disp>;

But this issue is not strictly related to imx7d pci hanging on
suspend/resume, it can be dealt with later.

--
Regards,
Leonard
Ulf Hansson July 10, 2018, 10:26 a.m. | #6
On 9 July 2018 at 16:59, Leonard Crestez <leonard.crestez@nxp.com> wrote:
> On Tue, 2018-07-03 at 10:42 +0200, Lucas Stach wrote:
>> Am Montag, den 02.07.2018, 17:18 +0000 schrieb Leonard Crestez:
>> > On Fri, 2018-06-08 at 16:33 +0200, Lucas Stach wrote:
>> > > Am Dienstag, den 29.05.2018, 22:39 +0300 schrieb Leonard Crestez:
>> > > > On imx7d the phy is turned off in suspend and must be reset on resume.
>> > > > Right now lspci -v fails after a suspend/resume cycle, fix this by
>> > > > adding minimal suspend/resume code from the nxp vendor tree.
>> > > >
>> > > > This is currently only enabled for imx7 but the same sequence can be
>> > > > applied to other imx pcie variants.
>> > > > +#ifdef CONFIG_PM_SLEEP
>> >
>> > Another issue that should be discussed here is that on 6sx and 7d the
>> > gpc PCIE power domain is not defined correctly: the PCIE block is in
>> > the DISPMIX domain on both SOCs and it is only PCIE_PHY which has a
>> > separate power domain.
>> >
>> > This matters: enabling power-gating for displays will break pcie if
>> > this relationship is not taken into account. Here are some options:
>> >
>> > 1) Make &pd_pcie a child of &pd_disp by hacking into gpc probe
>> > functions and calling pm_genpd_add_subdomain. Not very nice.
>> >
>> > 2) Support nesting PGCs in GPC code? Lots of code and still an
>> > incorrect representation of hardware.
>> >
>> > 3) Set power-domains = <&pd_disp> and enable runtime PM on &pd_pcie?
>> >
>> > 4) Add separate devices for the pcie-phy. These would be mostly empty
>> > but still different, for example on imx6sx the PHY is not even
>> > accessible on the bus but only though PCIE registers.
>>
>> 5) Take a look at the linux-pm list. ;) The power domain framework has
>> just gained support for multiple power domains per device. I think that
>>  is the right solution for this, as like you mentioned the PHY isn't
>> really a separate block on i.MX6, but part of the PCIe controller
>> device.

Yep, it sounds like the PCIe controller device is partitioned across
multiple PM domains.

>
> Yes, I noticed that earlier but not that it was merged recently. It is
> a better solution.
>
> Unfortunately the multiple power-domain code seems to require devices
> to explicitly fetch references to the power domains and manipulate
> them.

Actually, it's the other way around.

You need only one device to attach the PM domains. However, in genpd,
one device will be created per PM domain and its that device you get
back to operate upon.

Typically the returned device(s) should be linked with the "master"
device, device_link_add().

> As far as I can tell this means that every device using multiple
> power domains has to be modified to do so. In this particular case it
> would be useful to just have all power domains turned on before probe,
> just like when a single power-domain is specified:

I you need them to be powered on, just use the corresponding device
links flags when you create the link during probe.

Something along the lines of this:

device_link_add(dev, genpd_dev0, DL_FLAG_STATELESS |
DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);

>
>     power-domains = <&pd_pci>, <&pd_disp>;
>
> But this issue is not strictly related to imx7d pci hanging on
> suspend/resume, it can be dealt with later.

Maybe not, I don't have the complete picture.

However, you do get the opportunity to use the genpd infrastructure,
which calls the ->power_on|off() callbacks of the PM domain during
suspend/resume. And by using the dev_link_add(), you can make sure
that the PM domain gets powered on/off in the order as the
corresponding supplier device.

Kind regards
Uffe

Patch

diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c
index 4818ef875f8a..ff6077eeb387 100644
--- a/drivers/pci/dwc/pci-imx6.c
+++ b/drivers/pci/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,80 @@  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 int imx6_pcie_suspend_noirq(struct device *dev)
+{
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+
+	if (imx6_pcie->variant == IMX7D) {
+		/* Disable clks */
+		clk_disable_unprepare(imx6_pcie->pcie);
+		clk_disable_unprepare(imx6_pcie->pcie_phy);
+		clk_disable_unprepare(imx6_pcie->pcie_bus);
+		/* turn off external osc input */
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
+				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
+	}
+
+	return 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);
+		break;
+	case IMX7D:
+		reset_control_assert(imx6_pcie->apps_reset);
+		break;
+	}
+}
+
+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) {
+		imx6_pcie_ltssm_disable(dev);
+
+		imx6_pcie_assert_core_reset(imx6_pcie);
+		imx6_pcie_init_phy(imx6_pcie);
+		imx6_pcie_deassert_core_reset(imx6_pcie);
+
+		/*
+		 * controller maybe turn off, re-configure again
+		 */
+		dw_pcie_setup_rc(pp);
+
+		imx6_pcie_ltssm_enable(dev);
+
+		ret = imx6_pcie_wait_for_link(imx6_pcie);
+		if (ret < 0)
+			pr_info("pcie link is down after resume.\n");
+	}
+
+	return 0;
+}
+
+static const struct dev_pm_ops imx6_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx6_pcie_suspend_noirq,
+				      imx6_pcie_resume_noirq)
+};
+#endif
+
 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 +930,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,
 };