From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 70C7DC6778A for ; Tue, 24 Jul 2018 10:09:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2522820852 for ; Tue, 24 Jul 2018 10:09:23 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2522820852 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=pengutronix.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2388320AbeGXLPE (ORCPT ); Tue, 24 Jul 2018 07:15:04 -0400 Received: from metis.ext.pengutronix.de ([85.220.165.71]:33531 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726367AbeGXLPD (ORCPT ); Tue, 24 Jul 2018 07:15:03 -0400 Received: from kresse.hi.pengutronix.de ([2001:67c:670:100:1d::2a]) by metis.ext.pengutronix.de with esmtp (Exim 4.89) (envelope-from ) id 1fhuFp-0006Pk-1m; Tue, 24 Jul 2018 12:09:17 +0200 Message-ID: <1532426953.32306.5.camel@pengutronix.de> Subject: Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support From: Lucas Stach To: Leonard Crestez , Richard Zhu , Fabio Estevam Cc: "A.s. Dong" , "linux-kernel@vger.kernel.org" , dl-linux-imx , "jingoohan1@gmail.com" , "lorenzo.pieralisi@arm.com" , "linux-pm@vger.kernel.org" , "Joao.Pinto@synopsys.com" , "shawnguo@kernel.org" , "linux-arm-kernel@lists.infradead.org" , "andrew.smirnov@gmail.com" , "bhelgaas@google.com" , "linux-pci@vger.kernel.org" , "kernel@pengutronix.de" Date: Tue, 24 Jul 2018 12:09:13 +0200 In-Reply-To: References: <1532338685.3163.93.camel@pengutronix.de> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.6-1+deb9u1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::2a X-SA-Exim-Mail-From: l.stach@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Am Montag, den 23.07.2018, 12:37 +0000 schrieb Leonard Crestez: > On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote: > > Hi Leonard, > > > > Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez: > > > On imx7d the pcie-phy power domain is turned off in suspend and this can > > > make the system hang after resume when attempting any read from PCI. > > > > > > Fix this by adding minimal suspend/resume code from the nxp internal > > > tree. This will prepare for powering down on suspend and reset the block > > > on resume. > > > > > > Code is only for imx7d but a very similar sequence can be used for > > > other socs. > > > > > > +static void imx6_pcie_ltssm_disable(struct device *dev) > > > +{ > > > > > > + struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev); > > > + > > > > > > + switch (imx6_pcie->variant) { > > > > > > + case IMX6Q: > > > > > > + case IMX6SX: > > > > > > + case IMX6QP: > > > > > > + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, > > > +    IMX6Q_GPR12_PCIE_CTL_2, 0); > > > > Has this been tested on i.MX6? LTSSM disable requires a more complex > > sequence on this SoC to avoid hanging the system. See commit > > 3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling > > it". > > This patch only enables suspend/resume for imx7d with other SOCs to > follow later. The ltssm_disable function is just symmetric with > ltssm_enable. > > The 6Q parts are affected by errata "ERR005723 PCIe: PCIe does not > support L2 power down [i.MX 6Dual/6Quad Only]". > > This design error seems to have the same root cause as your problem (no > dedicated reset control) so this works out quite nicely: the solution > is to never power down pci on affected chips. I don't quite like code that looks like it is doing the right thing, but then doesn't. Can we at least emit a warning that there might be dragons if anyone tries to call this on i.MX6? > > > +static int imx6_pcie_resume_noirq(struct device *dev) > > > +{ > > > > > > + int ret = 0; > > > > > > + struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev); > > > > > > + struct pcie_port *pp = &imx6_pcie->pci->pp; > > > + > > > > > > + if (imx6_pcie->variant != IMX7D) > > > > > > + return 0; > > > + > > > > > > + imx6_pcie_assert_core_reset(imx6_pcie); > > > > > > + imx6_pcie_init_phy(imx6_pcie); > > > > > > + imx6_pcie_deassert_core_reset(imx6_pcie); > > > > > > + dw_pcie_setup_rc(pp); > > > + > > > > > > + ret = imx6_pcie_establish_link(imx6_pcie); > > > > > > + if (ret < 0) > > > + pr_err("pcie link is down after resume.\n"); > > > > dev_err(), please. > > The imx6_pcie_establish_link function already seems to link error > information so the message could be dropped. However it's still helpful > to know that those pci link errors are specifically related to resume. > > Fabio suggested I propagate the return code but I'm not sure that's > helpful since "link down" is what happens when the slot is empty and > this is clearly not a "error" or "failure". It's not clear if "slot > empty" can be distinguished in some way. I don't think we have a generic way to distinguish between the two cases. > I'll switch to dev_info and drop the error code, is this OK? Yes, that's fine with me. > > One aspect that I skipped is PME_Turn_Off support: The PCI standard > mandates that this is sent before entering L2/L3 and the designware > core supports this but it's not part of this patch. > > Is it fine if I post this separately or should it be part of the same > series? The turnoff bit is in IOMUX gpr for imx6 but SRC for imx7 so it > would require additional patches in reset, dts and then pci. IMO it is fine to have this as a follow on patchset. Regards, Lucas From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: Message-ID: <1532426953.32306.5.camel@pengutronix.de> Subject: Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support From: Lucas Stach To: Leonard Crestez , Richard Zhu , Fabio Estevam Date: Tue, 24 Jul 2018 12:09:13 +0200 In-Reply-To: References: <1532338685.3163.93.camel@pengutronix.de> Mime-Version: 1.0 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "A.s. Dong" , "lorenzo.pieralisi@arm.com" , "linux-pm@vger.kernel.org" , "andrew.smirnov@gmail.com" , "jingoohan1@gmail.com" , "linux-kernel@vger.kernel.org" , "Joao.Pinto@synopsys.com" , dl-linux-imx , "kernel@pengutronix.de" , "linux-pci@vger.kernel.org" , "bhelgaas@google.com" , "shawnguo@kernel.org" , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset="utf-8" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+bjorn=helgaas.com@lists.infradead.org List-ID: QW0gTW9udGFnLCBkZW4gMjMuMDcuMjAxOCwgMTI6MzcgKzAwMDAgc2NocmllYiBMZW9uYXJkIENy ZXN0ZXo6Cj4gT24gTW9uLCAyMDE4LTA3LTIzIGF0IDExOjM4ICswMjAwLCBMdWNhcyBTdGFjaCB3 cm90ZToKPiA+IEhpIExlb25hcmQsCj4gPiAKPiA+IEFtIEZyZWl0YWcsIGRlbiAyMC4wNy4yMDE4 LCAxNTo0NyArMDMwMCBzY2hyaWViIExlb25hcmQgQ3Jlc3RlejoKPiA+ID4gT24gaW14N2QgdGhl IHBjaWUtcGh5IHBvd2VyIGRvbWFpbiBpcyB0dXJuZWQgb2ZmIGluIHN1c3BlbmQgYW5kIHRoaXMg Y2FuCj4gPiA+IG1ha2UgdGhlIHN5c3RlbSBoYW5nIGFmdGVyIHJlc3VtZSB3aGVuIGF0dGVtcHRp bmcgYW55IHJlYWQgZnJvbSBQQ0kuCj4gPiA+IAo+ID4gPiBGaXggdGhpcyBieSBhZGRpbmcgbWlu aW1hbCBzdXNwZW5kL3Jlc3VtZSBjb2RlIGZyb20gdGhlIG54cCBpbnRlcm5hbAo+ID4gPiB0cmVl LiBUaGlzIHdpbGwgcHJlcGFyZSBmb3IgcG93ZXJpbmcgZG93biBvbiBzdXNwZW5kIGFuZCByZXNl dCB0aGUgYmxvY2sKPiA+ID4gb24gcmVzdW1lLgo+ID4gPiAKPiA+ID4gQ29kZSBpcyBvbmx5IGZv ciBpbXg3ZCBidXQgYSB2ZXJ5IHNpbWlsYXIgc2VxdWVuY2UgY2FuIGJlIHVzZWQgZm9yCj4gPiA+ IG90aGVyIHNvY3MuCj4gPiA+IAo+ID4gPiArc3RhdGljIHZvaWQgaW14Nl9wY2llX2x0c3NtX2Rp c2FibGUoc3RydWN0IGRldmljZSAqZGV2KQo+ID4gPiArewo+ID4gPiA+ID4gPiArCXN0cnVjdCBp bXg2X3BjaWUgKmlteDZfcGNpZSA9IGRldl9nZXRfZHJ2ZGF0YShkZXYpOwo+ID4gPiArCj4gPiA+ ID4gPiA+ICsJc3dpdGNoIChpbXg2X3BjaWUtPnZhcmlhbnQpIHsKPiA+ID4gPiA+ID4gKwljYXNl IElNWDZROgo+ID4gPiA+ID4gPiArCWNhc2UgSU1YNlNYOgo+ID4gPiA+ID4gPiArCWNhc2UgSU1Y NlFQOgo+ID4gPiA+ID4gPiArCQlyZWdtYXBfdXBkYXRlX2JpdHMoaW14Nl9wY2llLT5pb211eGNf Z3ByLCBJT01VWENfR1BSMTIsCj4gPiA+ICsJCQkJwqDCoMKgSU1YNlFfR1BSMTJfUENJRV9DVExf MiwgMCk7Cj4gPiAKPiA+IEhhcyB0aGlzIGJlZW4gdGVzdGVkIG9uIGkuTVg2PyBMVFNTTSBkaXNh YmxlIHJlcXVpcmVzIGEgbW9yZSBjb21wbGV4Cj4gPiBzZXF1ZW5jZSBvbiB0aGlzIFNvQyB0byBh dm9pZCBoYW5naW5nIHRoZSBzeXN0ZW0uIFNlZSBjb21taXQKPiA+IDNlM2U0MDZlMzgwNyAiUENJ OiBpbXg2OiBQdXQgTFRTU00gaW4gIkRldGVjdCIgc3RhdGUgYmVmb3JlIGRpc2FibGluZwo+ID4g aXQiLgo+IAo+IFRoaXMgcGF0Y2ggb25seSBlbmFibGVzIHN1c3BlbmQvcmVzdW1lIGZvciBpbXg3 ZCB3aXRoIG90aGVyIFNPQ3MgdG8KPiBmb2xsb3cgbGF0ZXIuIFRoZSBsdHNzbV9kaXNhYmxlIGZ1 bmN0aW9uIGlzIGp1c3Qgc3ltbWV0cmljIHdpdGgKPiBsdHNzbV9lbmFibGUuCj4gCj4gVGhlIDZR IHBhcnRzIGFyZSBhZmZlY3RlZCBieSBlcnJhdGEgIkVSUjAwNTcyMyBQQ0llOiBQQ0llIGRvZXMg bm90Cj4gc3VwcG9ydCBMMiBwb3dlciBkb3duIFtpLk1YIDZEdWFsLzZRdWFkIE9ubHldIi4KPiAK PiBUaGlzIGRlc2lnbiBlcnJvciBzZWVtcyB0byBoYXZlIHRoZSBzYW1lIHJvb3QgY2F1c2UgYXMg eW91ciBwcm9ibGVtIChubwo+IGRlZGljYXRlZCByZXNldCBjb250cm9sKSBzbyB0aGlzIHdvcmtz IG91dCBxdWl0ZSBuaWNlbHk6IHRoZSBzb2x1dGlvbgo+IGlzIHRvIG5ldmVyIHBvd2VyIGRvd24g cGNpIG9uIGFmZmVjdGVkIGNoaXBzLgoKSSBkb24ndCBxdWl0ZSBsaWtlIGNvZGUgdGhhdCBsb29r cyBsaWtlIGl0IGlzIGRvaW5nIHRoZSByaWdodCB0aGluZywKYnV0IHRoZW4gZG9lc24ndC4gQ2Fu IHdlIGF0IGxlYXN0IGVtaXQgYSB3YXJuaW5nIHRoYXQgdGhlcmUgbWlnaHQgYmUKZHJhZ29ucyBp ZiBhbnlvbmUgdHJpZXMgdG8gY2FsbCB0aGlzIG9uIGkuTVg2PwoKPiA+ID4gK3N0YXRpYyBpbnQg aW14Nl9wY2llX3Jlc3VtZV9ub2lycShzdHJ1Y3QgZGV2aWNlICpkZXYpCj4gPiA+ICt7Cj4gPiA+ ID4gPiA+ICsJaW50IHJldCA9IDA7Cj4gPiA+ID4gPiA+ICsJc3RydWN0IGlteDZfcGNpZSAqaW14 Nl9wY2llID0gZGV2X2dldF9kcnZkYXRhKGRldik7Cj4gPiA+ID4gPiA+ICsJc3RydWN0IHBjaWVf cG9ydCAqcHAgPSAmaW14Nl9wY2llLT5wY2ktPnBwOwo+ID4gPiArCj4gPiA+ID4gPiA+ICsJaWYg KGlteDZfcGNpZS0+dmFyaWFudCAhPSBJTVg3RCkKPiA+ID4gPiA+ID4gKwkJcmV0dXJuIDA7Cj4g PiA+ICsKPiA+ID4gPiA+ID4gKwlpbXg2X3BjaWVfYXNzZXJ0X2NvcmVfcmVzZXQoaW14Nl9wY2ll KTsKPiA+ID4gPiA+ID4gKwlpbXg2X3BjaWVfaW5pdF9waHkoaW14Nl9wY2llKTsKPiA+ID4gPiA+ ID4gKwlpbXg2X3BjaWVfZGVhc3NlcnRfY29yZV9yZXNldChpbXg2X3BjaWUpOwo+ID4gPiA+ID4g PiArCWR3X3BjaWVfc2V0dXBfcmMocHApOwo+ID4gPiArCj4gPiA+ID4gPiA+ICsJcmV0ID0gaW14 Nl9wY2llX2VzdGFibGlzaF9saW5rKGlteDZfcGNpZSk7Cj4gPiA+ID4gPiA+ICsJaWYgKHJldCA8 IDApCj4gPiA+ICsJCXByX2VycigicGNpZSBsaW5rIGlzIGRvd24gYWZ0ZXIgcmVzdW1lLlxuIik7 Cj4gPiAKPiA+IGRldl9lcnIoKSwgcGxlYXNlLgo+IAo+IFRoZSBpbXg2X3BjaWVfZXN0YWJsaXNo X2xpbmsgZnVuY3Rpb24gYWxyZWFkeSBzZWVtcyB0byBsaW5rIGVycm9yCj4gaW5mb3JtYXRpb24g c28gdGhlIG1lc3NhZ2UgY291bGQgYmUgZHJvcHBlZC4gSG93ZXZlciBpdCdzIHN0aWxsIGhlbHBm dWwKPiB0byBrbm93IHRoYXQgdGhvc2UgcGNpIGxpbmsgZXJyb3JzIGFyZSBzcGVjaWZpY2FsbHkg cmVsYXRlZCB0byByZXN1bWUuCj4gCj4gRmFiaW8gc3VnZ2VzdGVkIEkgcHJvcGFnYXRlIHRoZSBy ZXR1cm4gY29kZSBidXQgSSdtIG5vdCBzdXJlIHRoYXQncwo+IGhlbHBmdWwgc2luY2UgImxpbmsg ZG93biIgaXMgd2hhdCBoYXBwZW5zIHdoZW4gdGhlIHNsb3QgaXMgZW1wdHkgYW5kCj4gdGhpcyBp cyBjbGVhcmx5IG5vdCBhICJlcnJvciIgb3IgImZhaWx1cmUiLiBJdCdzIG5vdCBjbGVhciBpZiAi c2xvdAo+IGVtcHR5IiBjYW4gYmUgZGlzdGluZ3Vpc2hlZCBpbiBzb21lIHdheS4KCkkgZG9uJ3Qg dGhpbmsgd2UgaGF2ZSBhIGdlbmVyaWMgd2F5IHRvIGRpc3Rpbmd1aXNoIGJldHdlZW4gdGhlIHR3 bwpjYXNlcy4KCj4gSSdsbCBzd2l0Y2ggdG8gZGV2X2luZm8gYW5kIGRyb3AgdGhlIGVycm9yIGNv ZGUsIGlzIHRoaXMgT0s/CgpZZXMsIHRoYXQncyBmaW5lIHdpdGggbWUuCgo+IAo+IE9uZSBhc3Bl Y3QgdGhhdCBJIHNraXBwZWQgaXMgUE1FX1R1cm5fT2ZmIHN1cHBvcnQ6IFRoZSBQQ0kgc3RhbmRh cmQKPiBtYW5kYXRlcyB0aGF0IHRoaXMgaXMgc2VudCBiZWZvcmUgZW50ZXJpbmcgTDIvTDMgYW5k IHRoZSBkZXNpZ253YXJlCj4gY29yZSBzdXBwb3J0cyB0aGlzIGJ1dCBpdCdzIG5vdCBwYXJ0IG9m IHRoaXMgcGF0Y2guCj4gCj4gSXMgaXQgZmluZSBpZiBJIHBvc3QgdGhpcyBzZXBhcmF0ZWx5IG9y IHNob3VsZCBpdCBiZSBwYXJ0IG9mIHRoZSBzYW1lCj4gc2VyaWVzPyBUaGUgdHVybm9mZiBiaXQg aXMgaW4gSU9NVVggZ3ByIGZvciBpbXg2IGJ1dCBTUkMgZm9yIGlteDcgc28gaXQKPiB3b3VsZCBy ZXF1aXJlIGFkZGl0aW9uYWwgcGF0Y2hlcyBpbiByZXNldCwgZHRzIGFuZCB0aGVuIHBjaS4KCklN TyBpdCBpcyBmaW5lIHRvIGhhdmUgdGhpcyBhcyBhIGZvbGxvdyBvbiBwYXRjaHNldC4KClJlZ2Fy ZHMsCkx1Y2FzCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f XwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdApsaW51eC1hcm0ta2VybmVsQGxpc3RzLmlu ZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5mby9s aW51eC1hcm0ta2VybmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lucas Stach Subject: Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support Date: Tue, 24 Jul 2018 12:09:13 +0200 Message-ID: <1532426953.32306.5.camel@pengutronix.de> References: <1532338685.3163.93.camel@pengutronix.de> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Leonard Crestez , Richard Zhu , Fabio Estevam Cc: "A.s. Dong" , "linux-kernel@vger.kernel.org" , dl-linux-imx , "jingoohan1@gmail.com" , "lorenzo.pieralisi@arm.com" , "linux-pm@vger.kernel.org" , "Joao.Pinto@synopsys.com" , "shawnguo@kernel.org" , "linux-arm-kernel@lists.infradead.org" , "andrew.smirnov@gmail.com" , "bhelgaas@google.com" , "linux-pci@vger.kernel.org" , "kernel@pengutronix.de" List-Id: linux-pm@vger.kernel.org Am Montag, den 23.07.2018, 12:37 +0000 schrieb Leonard Crestez: > On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote: > > Hi Leonard, > > > > Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez: > > > On imx7d the pcie-phy power domain is turned off in suspend and this can > > > make the system hang after resume when attempting any read from PCI. > > > > > > Fix this by adding minimal suspend/resume code from the nxp internal > > > tree. This will prepare for powering down on suspend and reset the block > > > on resume. > > > > > > Code is only for imx7d but a very similar sequence can be used for > > > other socs. > > > > > > +static void imx6_pcie_ltssm_disable(struct device *dev) > > > +{ > > > > > > + struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev); > > > + > > > > > > + switch (imx6_pcie->variant) { > > > > > > + case IMX6Q: > > > > > > + case IMX6SX: > > > > > > + case IMX6QP: > > > > > > + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, > > > +    IMX6Q_GPR12_PCIE_CTL_2, 0); > > > > Has this been tested on i.MX6? LTSSM disable requires a more complex > > sequence on this SoC to avoid hanging the system. See commit > > 3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling > > it". > > This patch only enables suspend/resume for imx7d with other SOCs to > follow later. The ltssm_disable function is just symmetric with > ltssm_enable. > > The 6Q parts are affected by errata "ERR005723 PCIe: PCIe does not > support L2 power down [i.MX 6Dual/6Quad Only]". > > This design error seems to have the same root cause as your problem (no > dedicated reset control) so this works out quite nicely: the solution > is to never power down pci on affected chips. I don't quite like code that looks like it is doing the right thing, but then doesn't. Can we at least emit a warning that there might be dragons if anyone tries to call this on i.MX6? > > > +static int imx6_pcie_resume_noirq(struct device *dev) > > > +{ > > > > > > + int ret = 0; > > > > > > + struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev); > > > > > > + struct pcie_port *pp = &imx6_pcie->pci->pp; > > > + > > > > > > + if (imx6_pcie->variant != IMX7D) > > > > > > + return 0; > > > + > > > > > > + imx6_pcie_assert_core_reset(imx6_pcie); > > > > > > + imx6_pcie_init_phy(imx6_pcie); > > > > > > + imx6_pcie_deassert_core_reset(imx6_pcie); > > > > > > + dw_pcie_setup_rc(pp); > > > + > > > > > > + ret = imx6_pcie_establish_link(imx6_pcie); > > > > > > + if (ret < 0) > > > + pr_err("pcie link is down after resume.\n"); > > > > dev_err(), please. > > The imx6_pcie_establish_link function already seems to link error > information so the message could be dropped. However it's still helpful > to know that those pci link errors are specifically related to resume. > > Fabio suggested I propagate the return code but I'm not sure that's > helpful since "link down" is what happens when the slot is empty and > this is clearly not a "error" or "failure". It's not clear if "slot > empty" can be distinguished in some way. I don't think we have a generic way to distinguish between the two cases. > I'll switch to dev_info and drop the error code, is this OK? Yes, that's fine with me. > > One aspect that I skipped is PME_Turn_Off support: The PCI standard > mandates that this is sent before entering L2/L3 and the designware > core supports this but it's not part of this patch. > > Is it fine if I post this separately or should it be part of the same > series? The turnoff bit is in IOMUX gpr for imx6 but SRC for imx7 so it > would require additional patches in reset, dts and then pci. IMO it is fine to have this as a follow on patchset. Regards, Lucas From mboxrd@z Thu Jan 1 00:00:00 1970 From: l.stach@pengutronix.de (Lucas Stach) Date: Tue, 24 Jul 2018 12:09:13 +0200 Subject: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support In-Reply-To: References: <1532338685.3163.93.camel@pengutronix.de> Message-ID: <1532426953.32306.5.camel@pengutronix.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Am Montag, den 23.07.2018, 12:37 +0000 schrieb Leonard Crestez: > On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote: > > Hi Leonard, > > > > Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez: > > > On imx7d the pcie-phy power domain is turned off in suspend and this can > > > make the system hang after resume when attempting any read from PCI. > > > > > > Fix this by adding minimal suspend/resume code from the nxp internal > > > tree. This will prepare for powering down on suspend and reset the block > > > on resume. > > > > > > Code is only for imx7d but a very similar sequence can be used for > > > other socs. > > > > > > +static void imx6_pcie_ltssm_disable(struct device *dev) > > > +{ > > > > > > + struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev); > > > + > > > > > > + switch (imx6_pcie->variant) { > > > > > > + case IMX6Q: > > > > > > + case IMX6SX: > > > > > > + case IMX6QP: > > > > > > + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, > > > + ???IMX6Q_GPR12_PCIE_CTL_2, 0); > > > > Has this been tested on i.MX6? LTSSM disable requires a more complex > > sequence on this SoC to avoid hanging the system. See commit > > 3e3e406e3807 "PCI: imx6: Put LTSSM in "Detect" state before disabling > > it". > > This patch only enables suspend/resume for imx7d with other SOCs to > follow later. The ltssm_disable function is just symmetric with > ltssm_enable. > > The 6Q parts are affected by errata "ERR005723 PCIe: PCIe does not > support L2 power down [i.MX 6Dual/6Quad Only]". > > This design error seems to have the same root cause as your problem (no > dedicated reset control) so this works out quite nicely: the solution > is to never power down pci on affected chips. I don't quite like code that looks like it is doing the right thing, but then doesn't. Can we at least emit a warning that there might be dragons if anyone tries to call this on i.MX6? > > > +static int imx6_pcie_resume_noirq(struct device *dev) > > > +{ > > > > > > + int ret = 0; > > > > > > + struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev); > > > > > > + struct pcie_port *pp = &imx6_pcie->pci->pp; > > > + > > > > > > + if (imx6_pcie->variant != IMX7D) > > > > > > + return 0; > > > + > > > > > > + imx6_pcie_assert_core_reset(imx6_pcie); > > > > > > + imx6_pcie_init_phy(imx6_pcie); > > > > > > + imx6_pcie_deassert_core_reset(imx6_pcie); > > > > > > + dw_pcie_setup_rc(pp); > > > + > > > > > > + ret = imx6_pcie_establish_link(imx6_pcie); > > > > > > + if (ret < 0) > > > + pr_err("pcie link is down after resume.\n"); > > > > dev_err(), please. > > The imx6_pcie_establish_link function already seems to link error > information so the message could be dropped. However it's still helpful > to know that those pci link errors are specifically related to resume. > > Fabio suggested I propagate the return code but I'm not sure that's > helpful since "link down" is what happens when the slot is empty and > this is clearly not a "error" or "failure". It's not clear if "slot > empty" can be distinguished in some way. I don't think we have a generic way to distinguish between the two cases. > I'll switch to dev_info and drop the error code, is this OK? Yes, that's fine with me. > > One aspect that I skipped is PME_Turn_Off support: The PCI standard > mandates that this is sent before entering L2/L3 and the designware > core supports this but it's not part of this patch. > > Is it fine if I post this separately or should it be part of the same > series? The turnoff bit is in IOMUX gpr for imx6 but SRC for imx7 so it > would require additional patches in reset, dts and then pci. IMO it is fine to have this as a follow on patchset. Regards, Lucas