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

[Lucas Stach]

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