linux-kernel - Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1532426953.32306.5.camel@pengutronix.de>
Date:   Tue, 24 Jul 2018 12:09:13 +0200
From:   Lucas Stach <l.stach@...gutronix.de>
To:     Leonard Crestez <leonard.crestez@....com>,
        Richard Zhu <hongxing.zhu@....com>,
        Fabio Estevam <fabio.estevam@....com>
Cc:     "A.s. Dong" <aisheng.dong@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>,
        "jingoohan1@...il.com" <jingoohan1@...il.com>,
        "lorenzo.pieralisi@....com" <lorenzo.pieralisi@....com>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        "Joao.Pinto@...opsys.com" <Joao.Pinto@...opsys.com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "andrew.smirnov@...il.com" <andrew.smirnov@...il.com>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>
Subject: Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support

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