lkml.org 
[lkml]   [2018]   [Jul]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 2/2] PCI: imx: Initial imx7d pm support
    Date
    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
    \
     
     \ /
      Last update: 2018-07-02 19:19    [W:2.961 / U:0.212 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site