[PATCH 0/2] PCI: Initial imx7d pm support

[PATCH 0/2] PCI: Initial imx7d pm support

[Leonard Crestez]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] reset: imx7: Fix always writing bits as 0

[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>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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) {
+		/* 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

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

[Lucas Stach]

QW0gRGllbnN0YWcsIGRlbiAyOS4wNS4yMDE4LCAyMjozOSArMDMwMCBzY2hyaWViIExlb25hcmQg
Q3Jlc3RlejoKPiBSaWdodCBub3cgdGhlIG9ubHkgdXNlciBvZiByZXNldC1pbXg3IGlzIHBjaS1p
bXg2IGFuZCB0aGUKPiByZXNldF9jb250cm9sX2Fzc2VydCBhbmQgZGVhc3NlcnQgY2FsbHMgb24g
cGNpZXBoeV9yZXNldCBkb24ndCB0b2dnbGUKPiB0aGUgUENJRVBIWV9CVE4gYW5kIFBDSUVQSFlf
R19SU1QgYml0cyBhcyBleHBlY3RlZC4gRml4IHRoaXMgYnkgd3JpdGluZwo+IDEgb3IgMCByZXNw
ZWN0aXZlbHkuCj4gCj4gVGhlIHJlZmVyZW5jZSBtYW51YWwgaXMgbm90IHZlcnkgY2xlYXIgcmVn
YXJkaW5nIFNSQ19QQ0lFUEhZX1JDUiBidXQgZm9yCj4gb3RoZXIgcmVnaXN0ZXJzIGxpa2UgTUlQ
SVBIWSBhbmQgSFNJQ1BIWSB0aGUgYml0cyBhcmUgZXhwbGljaXRseQo+IGRvY3VtZW50ZWQgYXMg
IjEgbWVhbnMgYXNzZXJ0LCAwIG1lYW5zIGRlYXNzZXJ0Ii4KPiAKPiBUaGUgdmFsdWVzIGFyZSBz
dGlsbCByZXZlcnNlZCBmb3IgSU1YN19SRVNFVF9QQ0lFX0NUUkxfQVBQU19FTi4KPiAKPiBTaWdu
ZWQtb2ZmLWJ5OiBMZW9uYXJkIENyZXN0ZXogPGxlb25hcmQuY3Jlc3RlekBueHAuY29tPgoKUmV2
aWV3ZWQtYnk6IEx1Y2FzIFN0YWNoIDxsLnN0YWNoQHBlbmd1dHJvbml4LmRlPgoKPiAtLS0KPiDC
oGRyaXZlcnMvcmVzZXQvcmVzZXQtaW14Ny5jIHwgMiArLQo+IMKgMSBmaWxlIGNoYW5nZWQsIDEg
aW5zZXJ0aW9uKCspLCAxIGRlbGV0aW9uKC0pCj4gCj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvcmVz
ZXQvcmVzZXQtaW14Ny5jIGIvZHJpdmVycy9yZXNldC9yZXNldC1pbXg3LmMKPiBpbmRleCA0ZGIx
NzdiYzg5YmMuLmZkZWFjMTk0NjQyOSAxMDA2NDQKPiAtLS0gYS9kcml2ZXJzL3Jlc2V0L3Jlc2V0
LWlteDcuYwo+ICsrKyBiL2RyaXZlcnMvcmVzZXQvcmVzZXQtaW14Ny5jCj4gQEAgLTc4LDExICs3
OCwxMSBAQCBzdGF0aWMgc3RydWN0IGlteDdfc3JjICp0b19pbXg3X3NyYyhzdHJ1Y3QgcmVzZXRf
Y29udHJvbGxlcl9kZXYgKnJjZGV2KQo+IMKgc3RhdGljIGludCBpbXg3X3Jlc2V0X3NldChzdHJ1
Y3QgcmVzZXRfY29udHJvbGxlcl9kZXYgKnJjZGV2LAo+ID4gwqAJCQnCoMKgdW5zaWduZWQgbG9u
ZyBpZCwgYm9vbCBhc3NlcnQpCj4gwqB7Cj4gPiDCoAlzdHJ1Y3QgaW14N19zcmMgKmlteDdzcmMg
PSB0b19pbXg3X3NyYyhyY2Rldik7Cj4gPiDCoAljb25zdCBzdHJ1Y3QgaW14N19zcmNfc2lnbmFs
ICpzaWduYWwgPSAmaW14N19zcmNfc2lnbmFsc1tpZF07Cj4gPiAtCXVuc2lnbmVkIGludCB2YWx1
ZSA9IDA7Cj4gPiArCXVuc2lnbmVkIGludCB2YWx1ZSA9IGFzc2VydCA/IHNpZ25hbC0+Yml0IDog
MDsKPiDCoAo+ID4gwqAJc3dpdGNoIChpZCkgewo+ID4gwqAJY2FzZSBJTVg3X1JFU0VUX1BDSUVQ
SFk6Cj4gPiDCoAkJLyoKPiA+IMKgCQnCoCogd2FpdCBmb3IgbW9yZSB0aGFuIDEwdXMgdG8gcmVs
ZWFzZSBwaHkgZ19yc3QgYW5kCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fX19fX19fXwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdApsaW51eC1hcm0ta2VybmVs
QGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9s
aXN0aW5mby9saW51eC1hcm0ta2VybmVsCg==

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

[Lucas Stach]

QW0gRGllbnN0YWcsIGRlbiAyOS4wNS4yMDE4LCAyMjozOSArMDMwMCBzY2hyaWViIExlb25hcmQg
Q3Jlc3RlejoKPiBPbiBpbXg3ZCB0aGUgcGh5IGlzIHR1cm5lZCBvZmYgaW4gc3VzcGVuZCBhbmQg
bXVzdCBiZSByZXNldCBvbiByZXN1bWUuCj4gUmlnaHQgbm93IGxzcGNpIC12IGZhaWxzIGFmdGVy
IGEgc3VzcGVuZC9yZXN1bWUgY3ljbGUsIGZpeCB0aGlzIGJ5Cj4gYWRkaW5nIG1pbmltYWwgc3Vz
cGVuZC9yZXN1bWUgY29kZSBmcm9tIHRoZSBueHAgdmVuZG9yIHRyZWUuCj4gCj4gVGhpcyBpcyBj
dXJyZW50bHkgb25seSBlbmFibGVkIGZvciBpbXg3IGJ1dCB0aGUgc2FtZSBzZXF1ZW5jZSBjYW4g
YmUKPiBhcHBsaWVkIHRvIG90aGVyIGlteCBwY2llIHZhcmlhbnRzLgo+IAo+IFRlc3RlZCBvbiBp
bXg3ZC1zYWJyZXNkIHdpdGggYW4gSW50ZWwgNTYyMkFOSE1XIHdpcmVsZXNzIHBjaWUgYWRhcHRl
ci4KPiAKPiA+IFRoZSBvcmlnaW5hbCBhdXRob3IgaXMgbW9zdGx5IFJpY2hhcmQgWmh1IDxob25n
eGluZy56aHVAbnhwLmNvbT4sIHRoaXMKPiBwYXRjaCBhZGp1c3RzIHRoZSBjb2RlIHRvIHRoZSB1
cHN0cmVhbSBpbXg3ZCBpbXBsZW1lbnRpb24gdXNpbmcgcmVzZXQKPiBjb250cm9scyBhbmQgcG93
ZXIgZG9tYWlucy4KPiAKPiA+IFNpZ25lZC1vZmYtYnk6IExlb25hcmQgQ3Jlc3RleiA8bGVvbmFy
ZC5jcmVzdGV6QG54cC5jb20+Cj4gLS0tCj4gwqBkcml2ZXJzL3BjaS9kd2MvcGNpLWlteDYuYyB8
IDk0ICsrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKy0tCj4gwqAxIGZpbGUgY2hh
bmdlZCwgODkgaW5zZXJ0aW9ucygrKSwgNSBkZWxldGlvbnMoLSkKPiAKPiBkaWZmIC0tZ2l0IGEv
ZHJpdmVycy9wY2kvZHdjL3BjaS1pbXg2LmMgYi9kcml2ZXJzL3BjaS9kd2MvcGNpLWlteDYuYwo+
IGluZGV4IDQ4MThlZjg3NWY4YS4uZmY2MDc3ZWViMzg3IDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMv
cGNpL2R3Yy9wY2ktaW14Ni5jCj4gKysrIGIvZHJpdmVycy9wY2kvZHdjL3BjaS1pbXg2LmMKPiBA
QCAtNTQwLDEwICs1NDAsMjcgQEAgc3RhdGljIGludCBpbXg2X3BjaWVfd2FpdF9mb3Jfc3BlZWRf
Y2hhbmdlKHN0cnVjdCBpbXg2X3BjaWUgKmlteDZfcGNpZSkKPiDCoAo+ID4gwqAJZGV2X2Vycihk
ZXYsICJTcGVlZCBjaGFuZ2UgdGltZW91dFxuIik7Cj4gPiDCoAlyZXR1cm4gLUVJTlZBTDsKPiDC
oH0KPiDCoAo+ICtzdGF0aWMgdm9pZCBpbXg2X3BjaWVfbHRzc21fZW5hYmxlKHN0cnVjdCBkZXZp
Y2UgKmRldikKPiArewo+ID4gKwlzdHJ1Y3QgaW14Nl9wY2llICppbXg2X3BjaWUgPSBkZXZfZ2V0
X2RydmRhdGEoZGV2KTsKPiArCj4gPiArCXN3aXRjaCAoaW14Nl9wY2llLT52YXJpYW50KSB7Cj4g
PiArCWNhc2UgSU1YNlE6Cj4gPiArCWNhc2UgSU1YNlNYOgo+ID4gKwljYXNlIElNWDZRUDoKPiA+
ICsJCXJlZ21hcF91cGRhdGVfYml0cyhpbXg2X3BjaWUtPmlvbXV4Y19ncHIsIElPTVVYQ19HUFIx
MiwKPiA+ICsJCQkJwqDCoMKgSU1YNlFfR1BSMTJfUENJRV9DVExfMiwKPiA+ICsJCQkJwqDCoMKg
SU1YNlFfR1BSMTJfUENJRV9DVExfMik7Cj4gPiArCQlicmVhazsKPiA+ICsJY2FzZSBJTVg3RDoK
PiA+ICsJCXJlc2V0X2NvbnRyb2xfZGVhc3NlcnQoaW14Nl9wY2llLT5hcHBzX3Jlc2V0KTsKPiA+
ICsJfQo+ICt9Cj4gKwo+IMKgc3RhdGljIGludCBpbXg2X3BjaWVfZXN0YWJsaXNoX2xpbmsoc3Ry
dWN0IGlteDZfcGNpZSAqaW14Nl9wY2llKQo+IMKgewo+ID4gwqAJc3RydWN0IGR3X3BjaWUgKnBj
aSA9IGlteDZfcGNpZS0+cGNpOwo+ID4gwqAJc3RydWN0IGRldmljZSAqZGV2ID0gcGNpLT5kZXY7
Cj4gPiDCoAl1MzIgdG1wOwo+IEBAIC01NTgsMTUgKzU3NSwxMSBAQCBzdGF0aWMgaW50IGlteDZf
cGNpZV9lc3RhYmxpc2hfbGluayhzdHJ1Y3QgaW14Nl9wY2llICppbXg2X3BjaWUpCj4gPiDCoAl0
bXAgJj0gflBDSUVfUkNfTENSX01BWF9MSU5LX1NQRUVEU19NQVNLOwo+ID4gwqAJdG1wIHw9IFBD
SUVfUkNfTENSX01BWF9MSU5LX1NQRUVEU19HRU4xOwo+ID4gwqAJZHdfcGNpZV93cml0ZWxfZGJp
KHBjaSwgUENJRV9SQ19MQ1IsIHRtcCk7Cj4gwqAKPiA+IMKgCS8qIFN0YXJ0IExUU1NNLiAqLwo+
ID4gLQlpZiAoaW14Nl9wY2llLT52YXJpYW50ID09IElNWDdEKQo+ID4gLQkJcmVzZXRfY29udHJv
bF9kZWFzc2VydChpbXg2X3BjaWUtPmFwcHNfcmVzZXQpOwo+ID4gLQllbHNlCj4gPiAtCQlyZWdt
YXBfdXBkYXRlX2JpdHMoaW14Nl9wY2llLT5pb211eGNfZ3ByLCBJT01VWENfR1BSMTIsCj4gPiAt
CQkJCcKgwqDCoElNWDZRX0dQUjEyX1BDSUVfQ1RMXzIsIDEgPDwgMTApOwo+ID4gKwlpbXg2X3Bj
aWVfbHRzc21fZW5hYmxlKGRldik7Cj4gwqAKPiA+IMKgCXJldCA9IGlteDZfcGNpZV93YWl0X2Zv
cl9saW5rKGlteDZfcGNpZSk7Cj4gPiDCoAlpZiAocmV0KQo+ID4gwqAJCWdvdG8gZXJyX3Jlc2V0
X3BoeTsKPiDCoAo+IEBAIC02ODEsMTAgKzY5NCw4MCBAQCBzdGF0aWMgaW50IGlteDZfYWRkX3Bj
aWVfcG9ydChzdHJ1Y3QgaW14Nl9wY2llICppbXg2X3BjaWUsCj4gwqAKPiDCoHN0YXRpYyBjb25z
dCBzdHJ1Y3QgZHdfcGNpZV9vcHMgZHdfcGNpZV9vcHMgPSB7Cj4gPiDCoAkubGlua191cCA9IGlt
eDZfcGNpZV9saW5rX3VwLAo+IMKgfTsKPiDCoAo+ICsjaWZkZWYgQ09ORklHX1BNX1NMRUVQCj4g
K3N0YXRpYyBpbnQgaW14Nl9wY2llX3N1c3BlbmRfbm9pcnEoc3RydWN0IGRldmljZSAqZGV2KQo+
ICt7Cj4gPiArCXN0cnVjdCBpbXg2X3BjaWUgKmlteDZfcGNpZSA9IGRldl9nZXRfZHJ2ZGF0YShk
ZXYpOwo+ICsKPiArCWlmIChpbXg2X3BjaWUtPnZhcmlhbnQgPT0gSU1YN0QpIHsKCkluc3RlYWQg
b2YgdGhpcyBsYXJnZSBpbmRlbnRlZCBibG9jaywgcGxlYXNlIGp1c3QgaGF2ZSBhbiBlYXJseSBy
ZXR1cm4KYXQgdGhlIHN0YXJ0IG9mIHRoaXMgZnVuY3Rpb24sIGxpa2U6CgppZiAoaW14Nl9wY2ll
LT52YXJpYW50ICE9IElNWDdEKQoJcmV0dXJuIDA7CgpTYW1lIGZvciB0aGUgcmVzdW1lIGZ1bmN0
aW9uLgoKPiArCQkvKiBEaXNhYmxlIGNsa3MgKi8KPiA+ICsJCWNsa19kaXNhYmxlX3VucHJlcGFy
ZShpbXg2X3BjaWUtPnBjaWUpOwo+ID4gKwkJY2xrX2Rpc2FibGVfdW5wcmVwYXJlKGlteDZfcGNp
ZS0+cGNpZV9waHkpOwo+ID4gKwkJY2xrX2Rpc2FibGVfdW5wcmVwYXJlKGlteDZfcGNpZS0+cGNp
ZV9idXMpOwo+ID4gKwkJLyogdHVybiBvZmYgZXh0ZXJuYWwgb3NjIGlucHV0ICovCj4gPiArCQly
ZWdtYXBfdXBkYXRlX2JpdHMoaW14Nl9wY2llLT5pb211eGNfZ3ByLCBJT01VWENfR1BSMTIsCj4g
PiArCQkJCcKgwqDCoElNWDdEX0dQUjEyX1BDSUVfUEhZX1JFRkNMS19TRUwsCj4gPiArCQkJCcKg
wqDCoElNWDdEX0dQUjEyX1BDSUVfUEhZX1JFRkNMS19TRUwpOwo+ID4gKwl9Cj4gKwo+ID4gKwly
ZXR1cm4gMDsKPiArfQo+ICsKPiArc3RhdGljIHZvaWQgaW14Nl9wY2llX2x0c3NtX2Rpc2FibGUo
c3RydWN0IGRldmljZSAqZGV2KQo+ICt7Cj4gPiArCXN0cnVjdCBpbXg2X3BjaWUgKmlteDZfcGNp
ZSA9IGRldl9nZXRfZHJ2ZGF0YShkZXYpOwo+ICsKPiA+ICsJc3dpdGNoIChpbXg2X3BjaWUtPnZh
cmlhbnQpIHsKPiA+ICsJY2FzZSBJTVg2UToKPiA+ICsJY2FzZSBJTVg2U1g6Cj4gPiArCWNhc2Ug
SU1YNlFQOgo+ID4gKwkJcmVnbWFwX3VwZGF0ZV9iaXRzKGlteDZfcGNpZS0+aW9tdXhjX2dwciwg
SU9NVVhDX0dQUjEyLAo+ID4gKwkJCQnCoMKgwqBJTVg2UV9HUFIxMl9QQ0lFX0NUTF8yLCAwKTsK
PiA+ICsJCWJyZWFrOwo+ID4gKwljYXNlIElNWDdEOgo+ID4gKwkJcmVzZXRfY29udHJvbF9hc3Nl
cnQoaW14Nl9wY2llLT5hcHBzX3Jlc2V0KTsKPiA+ICsJCWJyZWFrOwo+ID4gKwl9Cj4gK30KPiAr
Cj4gK3N0YXRpYyBpbnQgaW14Nl9wY2llX3Jlc3VtZV9ub2lycShzdHJ1Y3QgZGV2aWNlICpkZXYp
Cj4gK3sKPiA+ICsJaW50IHJldCA9IDA7Cj4gPiArCXN0cnVjdCBpbXg2X3BjaWUgKmlteDZfcGNp
ZSA9IGRldl9nZXRfZHJ2ZGF0YShkZXYpOwo+ID4gKwlzdHJ1Y3QgcGNpZV9wb3J0ICpwcCA9ICZp
bXg2X3BjaWUtPnBjaS0+cHA7Cj4gKwo+ID4gKwlpZiAoaW14Nl9wY2llLT52YXJpYW50ID09IElN
WDdEKSB7Cj4gPiArCQlpbXg2X3BjaWVfbHRzc21fZGlzYWJsZShkZXYpOwoKVGhlIExUU1NNIGRp
c2FibGUgc2VlbXMgbWlzcGxhY2VkIGhlcmUuIEkgd291bGQgaGF2ZSBleHBlY3RlZCBpdCB0byBi
ZQppbiB0aGUgc3VzcGVuZCBmdW5jdGlvbi4KCj4gKwkJaW14Nl9wY2llX2Fzc2VydF9jb3JlX3Jl
c2V0KGlteDZfcGNpZSk7Cj4gPiArCQlpbXg2X3BjaWVfaW5pdF9waHkoaW14Nl9wY2llKTsKPiA+
ICsJCWlteDZfcGNpZV9kZWFzc2VydF9jb3JlX3Jlc2V0KGlteDZfcGNpZSk7Cj4gKwo+ID4gKwkJ
LyoKPiA+ICsJCcKgKiBjb250cm9sbGVyIG1heWJlIHR1cm4gb2ZmLCByZS1jb25maWd1cmUgYWdh
aW4KPiA+ICsJCcKgKi8KPiA+ICsJCWR3X3BjaWVfc2V0dXBfcmMocHApOwo+ICsKPiA+ICsJCWlt
eDZfcGNpZV9sdHNzbV9lbmFibGUoZGV2KTsKPiArCj4gPiArCQlyZXQgPSBpbXg2X3BjaWVfd2Fp
dF9mb3JfbGluayhpbXg2X3BjaWUpOwo+ID4gKwkJaWYgKHJldCA8IDApCj4gKwkJCXByX2luZm8o
InBjaWUgbGluayBpcyBkb3duIGFmdGVyIHJlc3VtZS5cbiIpOwoKSWYgdGhlIFBIWSBoYXMgYmVl
biBwb3dlcmVkIGRvd24gYW5kIExUU1NNIHN0b3BwZWQgSSBndWVzcyB3ZSBuZWVkIHRvCmRvIHRo
ZSBmdWxsIGlteDZfcGNpZV9lc3RhYmxpc2hfbGluaygpIGRhbmNlIGFnYWluIGhlcmUgdG8gcmVs
aWFibHkgcmUtCmVzdGFibGlzaCB0aGUgbGluay4gVGhlIGFib3ZlIHNlZW1zIHVuc2FmZS4KCj4g
Kwl9Cj4gKwo+ID4gKwlyZXR1cm4gMDsKPiArfQo+ICsKPiArc3RhdGljIGNvbnN0IHN0cnVjdCBk
ZXZfcG1fb3BzIGlteDZfcGNpZV9wbV9vcHMgPSB7Cj4gPiArCVNFVF9OT0lSUV9TWVNURU1fU0xF
RVBfUE1fT1BTKGlteDZfcGNpZV9zdXNwZW5kX25vaXJxLAo+ID4gKwkJCQnCoMKgwqDCoMKgwqBp
bXg2X3BjaWVfcmVzdW1lX25vaXJxKQo+ICt9Owo+ICsjZW5kaWYKClRoaXMgc3RydWN0dXJlIG11
c3QgYmUgb3V0c2lkZSBvZiB0aGUgaWZkZWYsIG9yIHlvdSdsbCBicmVhayB0aGUgYnVpbGQKb24g
IUNPTkZJR19QTV9TTEVFUC4KCj4gwqBzdGF0aWMgaW50IGlteDZfcGNpZV9wcm9iZShzdHJ1Y3Qg
cGxhdGZvcm1fZGV2aWNlICpwZGV2KQo+IMKgewo+ID4gwqAJc3RydWN0IGRldmljZSAqZGV2ID0g
JnBkZXYtPmRldjsKPiA+IMKgCXN0cnVjdCBkd19wY2llICpwY2k7Cj4gPiDCoAlzdHJ1Y3QgaW14
Nl9wY2llICppbXg2X3BjaWU7Cj4gQEAgLTg0NywxMCArOTMwLDExIEBAIHN0YXRpYyBjb25zdCBz
dHJ1Y3Qgb2ZfZGV2aWNlX2lkIGlteDZfcGNpZV9vZl9tYXRjaFtdID0gewo+IMKgc3RhdGljIHN0
cnVjdCBwbGF0Zm9ybV9kcml2ZXIgaW14Nl9wY2llX2RyaXZlciA9IHsKPiA+IMKgCS5kcml2ZXIg
PSB7Cj4gPiA+IMKgCQkubmFtZQk9ICJpbXg2cS1wY2llIiwKPiA+IMKgCQkub2ZfbWF0Y2hfdGFi
bGUgPSBpbXg2X3BjaWVfb2ZfbWF0Y2gsCj4gPiDCoAkJLnN1cHByZXNzX2JpbmRfYXR0cnMgPSB0
cnVlLAo+ID4gKwkJLnBtID0gJmlteDZfcGNpZV9wbV9vcHMsCj4gPiDCoAl9LAo+ID4gwqAJLnBy
b2JlwqDCoMKgwqA9IGlteDZfcGNpZV9wcm9iZSwKPiA+IMKgCS5zaHV0ZG93biA9IGlteDZfcGNp
ZV9zaHV0ZG93biwKPiDCoH07Cj4gwqAKClJlZ2FyZHMsCkx1Y2FzCgpfX19fX19fX19fX19fX19f
X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcg
bGlzdApsaW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmlu
ZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5mby9saW51eC1hcm0ta2VybmVsCg==

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

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

> > +		/*
> > +		 * 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Lucas Stach]

QW0gTW9udGFnLCBkZW4gMDIuMDcuMjAxOCwgMTc6MTggKzAwMDAgc2NocmllYiBMZW9uYXJkIENy
ZXN0ZXo6Cj4gT24gRnJpLCAyMDE4LTA2LTA4IGF0IDE2OjMzICswMjAwLCBMdWNhcyBTdGFjaCB3
cm90ZToKPiA+IEFtIERpZW5zdGFnLCBkZW4gMjkuMDUuMjAxOCwgMjI6MzkgKzAzMDAgc2Nocmll
YiBMZW9uYXJkIENyZXN0ZXo6Cj4gPiA+IE9uIGlteDdkIHRoZSBwaHkgaXMgdHVybmVkIG9mZiBp
biBzdXNwZW5kIGFuZCBtdXN0IGJlIHJlc2V0IG9uIHJlc3VtZS4KPiA+ID4gUmlnaHQgbm93IGxz
cGNpIC12IGZhaWxzIGFmdGVyIGEgc3VzcGVuZC9yZXN1bWUgY3ljbGUsIGZpeCB0aGlzIGJ5Cj4g
PiA+IGFkZGluZyBtaW5pbWFsIHN1c3BlbmQvcmVzdW1lIGNvZGUgZnJvbSB0aGUgbnhwIHZlbmRv
ciB0cmVlLgo+ID4gPiAKPiA+ID4gVGhpcyBpcyBjdXJyZW50bHkgb25seSBlbmFibGVkIGZvciBp
bXg3IGJ1dCB0aGUgc2FtZSBzZXF1ZW5jZSBjYW4gYmUKPiA+ID4gYXBwbGllZCB0byBvdGhlciBp
bXggcGNpZSB2YXJpYW50cy4KPiA+ID4gKyNpZmRlZiBDT05GSUdfUE1fU0xFRVAKPiA+ID4gK3N0
YXRpYyBpbnQgaW14Nl9wY2llX3N1c3BlbmRfbm9pcnEoc3RydWN0IGRldmljZSAqZGV2KQo+ID4g
PiArewo+ID4gPiA+ID4gPiArCXN0cnVjdCBpbXg2X3BjaWUgKmlteDZfcGNpZSA9IGRldl9nZXRf
ZHJ2ZGF0YShkZXYpOwo+ID4gPiArCj4gPiA+ICsJaWYgKGlteDZfcGNpZS0+dmFyaWFudCA9PSBJ
TVg3RCkgewo+ID4gCj4gPiBJbnN0ZWFkIG9mIHRoaXMgbGFyZ2UgaW5kZW50ZWQgYmxvY2ssIHBs
ZWFzZSBqdXN0IGhhdmUgYW4gZWFybHkgcmV0dXJuCj4gPiBhdCB0aGUgc3RhcnQgb2YgdGhpcyBm
dW5jdGlvbiwgbGlrZToKPiA+IAo+ID4gaWYgKGlteDZfcGNpZS0+dmFyaWFudCAhPSBJTVg3RCkK
PiA+IAlyZXR1cm4gMDsKPiA+IAo+ID4gU2FtZSBmb3IgdGhlIHJlc3VtZSBmdW5jdGlvbi4KPiAK
PiBPSy4gVGhlIHJlc3VtZSBzZXF1ZW5jZSBpcyBtb3N0bHkgdGhlIHNhbWUgZm9yIGFsbCBTT0Nz
IHdoZXJlIGl0Cj4gYXBwbGllcy4KPiAKPiA+ID4gK3N0YXRpYyBpbnQgaW14Nl9wY2llX3Jlc3Vt
ZV9ub2lycShzdHJ1Y3QgZGV2aWNlICpkZXYpCj4gPiA+ICt7Cj4gPiA+ID4gPiA+ICsJaW50IHJl
dCA9IDA7Cj4gPiA+ID4gPiA+ICsJc3RydWN0IGlteDZfcGNpZSAqaW14Nl9wY2llID0gZGV2X2dl
dF9kcnZkYXRhKGRldik7Cj4gPiA+ID4gPiA+ICsJc3RydWN0IHBjaWVfcG9ydCAqcHAgPSAmaW14
Nl9wY2llLT5wY2ktPnBwOwo+ID4gPiAKPiA+ID4gKwo+ID4gPiA+ID4gPiArCWlmIChpbXg2X3Bj
aWUtPnZhcmlhbnQgPT0gSU1YN0QpIHsKPiA+ID4gKwkJaW14Nl9wY2llX2x0c3NtX2Rpc2FibGUo
ZGV2KTsKPiA+IAo+ID4gVGhlIExUU1NNIGRpc2FibGUgc2VlbXMgbWlzcGxhY2VkIGhlcmUuIEkg
d291bGQgaGF2ZSBleHBlY3RlZCBpdCB0byBiZQo+ID4gaW4gdGhlIHN1c3BlbmQgZnVuY3Rpb24u
Cj4gCj4gVGhpcyBpcyBhIHJlcXVpcmVtZW50IGZvciByZWluaXRpYWxpemluZyB0aGUgY29yZTog
TFRTU00gbmVlZHMgdG8gYmUKPiB0dXJuZWQgb2ZmIGR1cmluZyBpbml0aWFsaXphdGlvbi4KCklm
IHlvdSBkaXNhYmxlIExUU1NNIGR1cmluZyBzdXNwZW5kLCBpdCBzaG91bGQgYmUgb2ZmIHdoZW4g
ZW50ZXJpbmcKdGhpcyByZXN1bWUgZnVuY3Rpb24sIG5vPwoKPiA+ID4gKwkJLyoKPiA+ID4gPiA+
ID4gKwkJwqAqIGNvbnRyb2xsZXIgbWF5YmUgdHVybiBvZmYsIHJlLWNvbmZpZ3VyZSBhZ2Fpbgo+
ID4gPiA+ID4gPiArCQnCoCovCj4gPiA+ID4gPiA+ICsJCWR3X3BjaWVfc2V0dXBfcmMocHApOwo+
ID4gPiArCj4gPiA+ID4gPiA+ICsJCWlteDZfcGNpZV9sdHNzbV9lbmFibGUoZGV2KTsKPiA+ID4g
Kwo+ID4gPiA+ID4gPiArCQlyZXQgPSBpbXg2X3BjaWVfd2FpdF9mb3JfbGluayhpbXg2X3BjaWUp
Owo+ID4gPiA+ID4gPiArCQlpZiAocmV0IDwgMCkKPiA+ID4gKwkJCXByX2luZm8oInBjaWUgbGlu
ayBpcyBkb3duIGFmdGVyIHJlc3VtZS5cbiIpOwo+ID4gCj4gPiBJZiB0aGUgUEhZIGhhcyBiZWVu
IHBvd2VyZWQgZG93biBhbmQgTFRTU00gc3RvcHBlZCBJIGd1ZXNzIHdlIG5lZWQgdG8KPiA+IGRv
IHRoZSBmdWxsIGlteDZfcGNpZV9lc3RhYmxpc2hfbGluaygpIGRhbmNlIGFnYWluIGhlcmUgdG8g
cmVsaWFibHkgcmUtCj4gPiBlc3RhYmxpc2ggdGhlIGxpbmsuIFRoZSBhYm92ZSBzZWVtcyB1bnNh
ZmUuCj4gCj4gV2hhdCBpbXg2X3BjaWVfZXN0YWJsaXNoX2xpbmsgZG9lcyBhZGRpdGlvbmFsbHkg
aXMgc29tZSB3b3JrYXJvdW5kIGZvcgo+IGxpbmsgZ2VuIGRldGVjdGlvbi4gSSBhZ3JlZSB0aGF0
IGl0IHNob3VsZCBiZSBpbmNsdWRlZC4KPiAKPiBUaGlzIHdvdWxkIG1ha2UgcmVzdW1lIG1vc3Rs
eSB0aGUgc2FtZSBhcyBpbXhfcGNpZV9ob3N0X2luaXQgZXhjZXB0IGZvcgo+IGR3X3BjaWVfbXNp
X2luaXQ7IHRoYXQgbmVlZHMgdG8gYmUgc2F2ZWQvcmVzdG9yZWQgc2VwYXJhdGVseS4KClJpZ2h0
LCBzbyBtYXliZSB3ZSBjYW4gZXZlbiBjdXQgZG93biBvbiBsaW5lcyBvZiBjb2RlIGJ5IHNwbGl0
dGluZyBhbmQKcmV1c2luZyBleGlzdGluZyBmdW5jdGlvbnMuCgo+IAo+IEFub3RoZXIgaXNzdWUg
dGhhdCBzaG91bGQgYmUgZGlzY3Vzc2VkIGhlcmUgaXMgdGhhdCBvbiA2c3ggYW5kIDdkIHRoZQo+
IGdwYyBQQ0lFIHBvd2VyIGRvbWFpbiBpcyBub3QgZGVmaW5lZCBjb3JyZWN0bHk6IHRoZSBQQ0lF
IGJsb2NrIGlzIGluCj4gdGhlIERJU1BNSVggZG9tYWluIG9uIGJvdGggU09DcyBhbmQgaXQgaXMg
b25seSBQQ0lFX1BIWSB3aGljaCBoYXMgYQo+IHNlcGFyYXRlIHBvd2VyIGRvbWFpbi4KPiAKPiBU
aGlzIG1hdHRlcnM6IGVuYWJsaW5nIHBvd2VyLWdhdGluZyBmb3IgZGlzcGxheXMgd2lsbCBicmVh
ayBwY2llIGlmCj4gdGhpcyByZWxhdGlvbnNoaXAgaXMgbm90IHRha2VuIGludG8gYWNjb3VudC4g
SGVyZSBhcmUgc29tZSBvcHRpb25zOgo+IAo+IDEpIE1ha2UgJnBkX3BjaWUgYSBjaGlsZCBvZiAm
cGRfZGlzcCBieSBoYWNraW5nIGludG8gZ3BjIHByb2JlCj4gZnVuY3Rpb25zIGFuZCBjYWxsaW5n
IHBtX2dlbnBkX2FkZF9zdWJkb21haW4uIE5vdCB2ZXJ5IG5pY2UuCj4gCj4gMikgU3VwcG9ydCBu
ZXN0aW5nIFBHQ3MgaW4gR1BDIGNvZGU/IExvdHMgb2YgY29kZSBhbmQgc3RpbGwgYW4KPiBpbmNv
cnJlY3QgcmVwcmVzZW50YXRpb24gb2YgaGFyZHdhcmUuCj4gCj4gMykgU2V0IHBvd2VyLWRvbWFp
bnMgPSA8JnBkX2Rpc3A+IGFuZCBlbmFibGUgcnVudGltZSBQTSBvbiAmcGRfcGNpZT8KPiAKPiA0
KSBBZGQgc2VwYXJhdGUgZGV2aWNlcyBmb3IgdGhlIHBjaWUtcGh5LiBUaGVzZSB3b3VsZCBiZSBt
b3N0bHkgZW1wdHkKPiBidXQgc3RpbGwgZGlmZmVyZW50LCBmb3IgZXhhbXBsZSBvbiBpbXg2c3gg
dGhlIFBIWSBpcyBub3QgZXZlbgo+IGFjY2Vzc2libGUgb24gdGhlIGJ1cyBidXQgb25seSB0aG91
Z2ggUENJRSByZWdpc3RlcnMuCgo1KSBUYWtlIGEgbG9vayBhdCB0aGUgbGludXgtcG0gbGlzdC4g
OykgVGhlIHBvd2VyIGRvbWFpbiBmcmFtZXdvcmsgaGFzCmp1c3QgZ2FpbmVkIHN1cHBvcnQgZm9y
IG11bHRpcGxlIHBvd2VyIGRvbWFpbnMgcGVyIGRldmljZS4gSSB0aGluayB0aGF0CiBpcyB0aGUg
cmlnaHQgc29sdXRpb24gZm9yIHRoaXMsIGFzIGxpa2UgeW91IG1lbnRpb25lZCB0aGUgUEhZIGlz
bid0CnJlYWxseSBhIHNlcGFyYXRlIGJsb2NrIG9uIGkuTVg2LCBidXQgcGFydCBvZiB0aGUgUENJ
ZSBjb250cm9sbGVyCmRldmljZS4KClJlZ2FyZHMsCkx1Y2FzCgpfX19fX19fX19fX19fX19fX19f
X19fX19fX19fX19fX19fX19fX19fX19fX19fXwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlz
dApsaW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJh
ZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5mby9saW51eC1hcm0ta2VybmVsCg==

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

[Lorenzo Pieralisi]

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Lorenzo Pieralisi]

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 resu=
me.
> > > > 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 =3D dev_get_drvdata(dev);
> > > > +
> > > > +	if (imx6_pcie->variant =3D=3D IMX7D) {
> > > =

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

> > > if (imx6_pcie->variant !=3D 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 =3D 0;
> > > > > > > +	struct imx6_pcie *imx6_pcie =3D dev_get_drvdata(dev);
> > > > > > > +	struct pcie_port *pp =3D &imx6_pcie->pci->pp;
> > > > =

> > > > +
> > > > > > > +	if (imx6_pcie->variant =3D=3D 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?
> =

> > > > +		/*
> > > > > > > +		=A0* controller maybe turn off, re-configure again
> > > > > > > +		=A0*/
> > > > > > > +		dw_pcie_setup_rc(pp);
> > > > +
> > > > > > > +		imx6_pcie_ltssm_enable(dev);
> > > > +
> > > > > > > +		ret =3D 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 r=
e-
> > > 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 =3D <&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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Leonard Crestez]

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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Ulf Hansson]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel