| Message ID | a6176c8bcfbca08ce4e93e304bdcd48c4600f8e2.1527621510.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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. > > 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) { 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. > + /* 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); The LTSSM disable seems misplaced here. I would have expected it to be in the suspend function. > + 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"); 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. > + } > + > > + 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 This structure must be outside of the ifdef, or you'll break the build on !CONFIG_PM_SLEEP. > 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, > }; > Regards, Lucas
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
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 > > > +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. If you disable LTSSM during suspend, it should be off when entering this resume function, no? > > > + /* > > > > > > + * 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. 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 = <&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. Regards, Lucas
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 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. > > If you disable LTSSM during suspend, it should be off when entering > this resume function, no? > > > > > + /* > > > > > > > + * 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. > > 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 = <&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
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
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
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, };
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(-)