linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] PCI: Initial imx7d pm support
@ 2018-05-29 19:39 Leonard Crestez
  2018-05-29 19:39 ` [PATCH 1/2] reset: imx7: Fix always writing bits as 0 Leonard Crestez
  2018-05-29 19:39 ` [PATCH 2/2] PCI: imx: Initial imx7d pm support Leonard Crestez
  0 siblings, 2 replies; 11+ messages in thread
From: Leonard Crestez @ 2018-05-29 19:39 UTC (permalink / raw)
  To: Andrey Smirnov, Philipp Zabel, Lucas Stach, Richard Zhu,
	linux-pci, linux-pm, linux-arm-kernel, linux-kernel
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Anson Huang, Jingoo Han,
	Joao Pinto, Rafael J. Wysocki, Abel Vesa

This series adds initial pm support on imx7d so that after
suspend/resume lspci works again. This mostly copies the resume sequence
from the imx tree.

More can be done later to reduce power in suspend as well as adding
support for other socs.

This is motivated mostly by a desire to bring imx PM code closer to
upstream. It is possible that I am missing some things about how PM
should be done for pci host drivers, it would be great if you could
point me the right way.

It also relies on this bugfix for PGC offsets:
    https://lkml.org/lkml/2018/5/29/138
Without that patch resume hangs on first PCI read from PCI-PM core. It
is not strictly related to PCI but pci-imx6 is the only user of gpcv2
power domains.

Patch 1 in this series is also technically an unrelated bugfix, however
pci-imx6 is the only user.

Leonard Crestez (2):
  reset: imx7: Fix always writing bits as 0
  PCI: imx: Initial imx7d pm support

 drivers/pci/dwc/pci-imx6.c | 94 ++++++++++++++++++++++++++++++++++++--
 drivers/reset/reset-imx7.c |  2 +-
 2 files changed, 90 insertions(+), 6 deletions(-)

-- 
2.17.0

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

* [PATCH 1/2] reset: imx7: Fix always writing bits as 0
  2018-05-29 19:39 [PATCH 0/2] PCI: Initial imx7d pm support Leonard Crestez
@ 2018-05-29 19:39 ` Leonard Crestez
  2018-06-08 14:23   ` Lucas Stach
  2018-07-04 16:35   ` Lorenzo Pieralisi
  2018-05-29 19:39 ` [PATCH 2/2] PCI: imx: Initial imx7d pm support Leonard Crestez
  1 sibling, 2 replies; 11+ messages in thread
From: Leonard Crestez @ 2018-05-29 19:39 UTC (permalink / raw)
  To: Andrey Smirnov, Philipp Zabel, Lucas Stach, Richard Zhu,
	linux-pci, linux-pm, linux-arm-kernel, linux-kernel
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Anson Huang, Jingoo Han,
	Joao Pinto, Rafael J. Wysocki, Abel Vesa

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>
---
 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.0

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

* [PATCH 2/2] PCI: imx: Initial imx7d pm support
  2018-05-29 19:39 [PATCH 0/2] PCI: Initial imx7d pm support Leonard Crestez
  2018-05-29 19:39 ` [PATCH 1/2] reset: imx7: Fix always writing bits as 0 Leonard Crestez
@ 2018-05-29 19:39 ` Leonard Crestez
  2018-06-08 14:33   ` Lucas Stach
  1 sibling, 1 reply; 11+ messages in thread
From: Leonard Crestez @ 2018-05-29 19:39 UTC (permalink / raw)
  To: Andrey Smirnov, Philipp Zabel, Lucas Stach, Richard Zhu,
	linux-pci, linux-pm, linux-arm-kernel, linux-kernel
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Anson Huang, Jingoo Han,
	Joao Pinto, Rafael J. Wysocki, Abel Vesa

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) {
+		/* 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,
 };
 
-- 
2.17.0

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

* Re: [PATCH 1/2] reset: imx7: Fix always writing bits as 0
  2018-05-29 19:39 ` [PATCH 1/2] reset: imx7: Fix always writing bits as 0 Leonard Crestez
@ 2018-06-08 14:23   ` Lucas Stach
  2018-07-04 16:35   ` Lorenzo Pieralisi
  1 sibling, 0 replies; 11+ messages in thread
From: Lucas Stach @ 2018-06-08 14:23 UTC (permalink / raw)
  To: Leonard Crestez, Andrey Smirnov, Philipp Zabel, Richard Zhu,
	linux-pci, linux-pm, linux-arm-kernel, linux-kernel
  Cc: Joao Pinto, Abel Vesa, Anson Huang, Jingoo Han,
	Rafael J. Wysocki, Lorenzo Pieralisi, Bjorn Helgaas

Am Dienstag, den 29.05.2018, 22:39 +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] 11+ messages in thread

* Re: [PATCH 2/2] PCI: imx: Initial imx7d pm support
  2018-05-29 19:39 ` [PATCH 2/2] PCI: imx: Initial imx7d pm support Leonard Crestez
@ 2018-06-08 14:33   ` Lucas Stach
  2018-07-02 17:18     ` Leonard Crestez
  0 siblings, 1 reply; 11+ messages in thread
From: Lucas Stach @ 2018-06-08 14:33 UTC (permalink / raw)
  To: Leonard Crestez, Andrey Smirnov, Philipp Zabel, Richard Zhu,
	linux-pci, linux-pm, linux-arm-kernel, linux-kernel
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Anson Huang, Jingoo Han,
	Joao Pinto, Rafael J. Wysocki, Abel Vesa

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

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

* Re: [PATCH 2/2] PCI: imx: Initial imx7d pm support
  2018-06-08 14:33   ` Lucas Stach
@ 2018-07-02 17:18     ` Leonard Crestez
  2018-07-03  8:42       ` Lucas Stach
  0 siblings, 1 reply; 11+ messages in thread
From: Leonard Crestez @ 2018-07-02 17:18 UTC (permalink / raw)
  To: l.stach, andrew.smirnov, Richard Zhu
  Cc: linux-kernel, jingoohan1, lorenzo.pieralisi, linux-pm, p.zabel,
	rjw, Joao.Pinto, Abel Vesa, linux-arm-kernel, Anson Huang,
	bhelgaas, linux-pci

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

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

* Re: [PATCH 2/2] PCI: imx: Initial imx7d pm support
  2018-07-02 17:18     ` Leonard Crestez
@ 2018-07-03  8:42       ` Lucas Stach
  2018-07-04 16:37         ` Lorenzo Pieralisi
  2018-07-09 14:59         ` Leonard Crestez
  0 siblings, 2 replies; 11+ messages in thread
From: Lucas Stach @ 2018-07-03  8:42 UTC (permalink / raw)
  To: Leonard Crestez, andrew.smirnov, Richard Zhu
  Cc: linux-kernel, jingoohan1, lorenzo.pieralisi, linux-pm, p.zabel,
	rjw, Joao.Pinto, Abel Vesa, linux-arm-kernel, Anson Huang,
	bhelgaas, linux-pci

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

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

* Re: [PATCH 1/2] reset: imx7: Fix always writing bits as 0
  2018-05-29 19:39 ` [PATCH 1/2] reset: imx7: Fix always writing bits as 0 Leonard Crestez
  2018-06-08 14:23   ` Lucas Stach
@ 2018-07-04 16:35   ` Lorenzo Pieralisi
  1 sibling, 0 replies; 11+ messages in thread
From: Lorenzo Pieralisi @ 2018-07-04 16:35 UTC (permalink / raw)
  To: Leonard Crestez, Philipp Zabel
  Cc: Andrey Smirnov, Lucas Stach, Richard Zhu, linux-pci, linux-pm,
	linux-arm-kernel, linux-kernel, Bjorn Helgaas, Anson Huang,
	Jingoo Han, Joao Pinto, Rafael J. Wysocki, Abel Vesa

On Tue, May 29, 2018 at 10:39:16PM +0300, Leonard Crestez wrote:
> 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>
> ---
>  drivers/reset/reset-imx7.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I think Philipp will pick it up, so I will drop it from the PCI
patchwork, if there is a problem please let me know.

Thanks,
Lorenzo

> 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.0
> 

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

* Re: [PATCH 2/2] PCI: imx: Initial imx7d pm support
  2018-07-03  8:42       ` Lucas Stach
@ 2018-07-04 16:37         ` Lorenzo Pieralisi
  2018-07-09 14:59         ` Leonard Crestez
  1 sibling, 0 replies; 11+ messages in thread
From: Lorenzo Pieralisi @ 2018-07-04 16:37 UTC (permalink / raw)
  To: Lucas Stach
  Cc: Leonard Crestez, andrew.smirnov, Richard Zhu, linux-kernel,
	jingoohan1, linux-pm, p.zabel, rjw, Joao.Pinto, Abel Vesa,
	linux-arm-kernel, Anson Huang, bhelgaas, linux-pci

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

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

* Re: [PATCH 2/2] PCI: imx: Initial imx7d pm support
  2018-07-03  8:42       ` Lucas Stach
  2018-07-04 16:37         ` Lorenzo Pieralisi
@ 2018-07-09 14:59         ` Leonard Crestez
  2018-07-10 10:26           ` Ulf Hansson
  1 sibling, 1 reply; 11+ messages in thread
From: Leonard Crestez @ 2018-07-09 14:59 UTC (permalink / raw)
  To: l.stach, ulf.hansson, andrew.smirnov, Richard Zhu, viresh.kumar
  Cc: linux-kernel, jingoohan1, lorenzo.pieralisi, linux-pm, p.zabel,
	rjw, Joao.Pinto, Abel Vesa, linux-arm-kernel, Anson Huang,
	bhelgaas, linux-pci

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

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

* Re: [PATCH 2/2] PCI: imx: Initial imx7d pm support
  2018-07-09 14:59         ` Leonard Crestez
@ 2018-07-10 10:26           ` Ulf Hansson
  0 siblings, 0 replies; 11+ messages in thread
From: Ulf Hansson @ 2018-07-10 10:26 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: l.stach, andrew.smirnov, Richard Zhu, viresh.kumar, linux-kernel,
	jingoohan1, lorenzo.pieralisi, linux-pm, p.zabel, rjw,
	Joao.Pinto, Abel Vesa, linux-arm-kernel, Anson Huang, bhelgaas,
	linux-pci

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

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

end of thread, other threads:[~2018-07-10 10:26 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-29 19:39 [PATCH 0/2] PCI: Initial imx7d pm support Leonard Crestez
2018-05-29 19:39 ` [PATCH 1/2] reset: imx7: Fix always writing bits as 0 Leonard Crestez
2018-06-08 14:23   ` Lucas Stach
2018-07-04 16:35   ` Lorenzo Pieralisi
2018-05-29 19:39 ` [PATCH 2/2] PCI: imx: Initial imx7d pm support Leonard Crestez
2018-06-08 14:33   ` Lucas Stach
2018-07-02 17:18     ` Leonard Crestez
2018-07-03  8:42       ` Lucas Stach
2018-07-04 16:37         ` Lorenzo Pieralisi
2018-07-09 14:59         ` Leonard Crestez
2018-07-10 10:26           ` Ulf Hansson

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