* [PATCH v2 0/3] PCI: imx: Initial imx7d pm support @ 2018-07-20 12:47 Leonard Crestez 2018-07-20 12:47 ` [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping" Leonard Crestez ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Leonard Crestez @ 2018-07-20 12:47 UTC (permalink / raw) To: Lucas Stach, Richard Zhu, Andrey Smirnov Cc: Shawn Guo, Joao Pinto, Jingoo Han, Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, linux-pm, linux-arm-kernel, linux-kernel, Fabio Estevam, Dong Aisheng, kernel, linux-imx Changes since v1: * Indentation fixes * Use imx6_pcie_establish_link instead of imx6_pcie_wait_for_link * Move imx6_pcie_ltssm_disable to suspend * Make imx6_pcie_clk_disable a separate function * Revert PCI irq mapping (also useful separately) Link to v1: https://lkml.org/lkml/2018/5/29/1042 Leonard Crestez (3): Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping" reset: imx7: Fix always writing bits as 0 PCI: imx: Initial imx7d pm support arch/arm/boot/dts/imx7d.dtsi | 8 +-- drivers/pci/controller/dwc/pci-imx6.c | 95 +++++++++++++++++++++++++-- drivers/reset/reset-imx7.c | 2 +- 3 files changed, 95 insertions(+), 10 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping" 2018-07-20 12:47 [PATCH v2 0/3] PCI: imx: Initial imx7d pm support Leonard Crestez @ 2018-07-20 12:47 ` Leonard Crestez 2018-07-20 15:33 ` Andrey Smirnov 2018-07-20 12:47 ` [PATCH v2 2/3] reset: imx7: Fix always writing bits as 0 Leonard Crestez 2018-07-20 12:47 ` [PATCH v2 3/3] PCI: imx: Initial imx7d pm support Leonard Crestez 2 siblings, 1 reply; 16+ messages in thread From: Leonard Crestez @ 2018-07-20 12:47 UTC (permalink / raw) To: Lucas Stach, Richard Zhu, Andrey Smirnov Cc: Shawn Guo, Joao Pinto, Jingoo Han, Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, linux-pm, linux-arm-kernel, linux-kernel, Fabio Estevam, Dong Aisheng, kernel, linux-imx This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d. That commit followed the reference manual but unfortunately the imx7d manual is incorrect. Tested with ath9k pcie card and confirmed internally. Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> --- arch/arm/boot/dts/imx7d.dtsi | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi index 8d3d123d0a5c..12c5ba7e9f1e 100644 --- a/arch/arm/boot/dts/imx7d.dtsi +++ b/arch/arm/boot/dts/imx7d.dtsi @@ -123,14 +123,14 @@ num-lanes = <1>; interrupts = <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "msi"; #interrupt-cells = <1>; interrupt-map-mask = <0 0 0 0x7>; - interrupt-map = <0 0 0 1 &intc GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>, - <0 0 0 2 &intc GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>, - <0 0 0 3 &intc GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>, - <0 0 0 4 &intc GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>; + interrupt-map = <0 0 0 1 &intc GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>, + <0 0 0 2 &intc GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>, + <0 0 0 3 &intc GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>, + <0 0 0 4 &intc GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>; clocks = <&clks IMX7D_PCIE_CTRL_ROOT_CLK>, <&clks IMX7D_PLL_ENET_MAIN_100M_CLK>, <&clks IMX7D_PCIE_PHY_ROOT_CLK>; clock-names = "pcie", "pcie_bus", "pcie_phy"; assigned-clocks = <&clks IMX7D_PCIE_CTRL_ROOT_SRC>, -- 2.17.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping" 2018-07-20 12:47 ` [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping" Leonard Crestez @ 2018-07-20 15:33 ` Andrey Smirnov 2018-07-23 12:41 ` Leonard Crestez 0 siblings, 1 reply; 16+ messages in thread From: Andrey Smirnov @ 2018-07-20 15:33 UTC (permalink / raw) To: Leonard Crestez Cc: Lucas Stach, Richard Zhu, Shawn Guo, Joao.Pinto, Jingoo Han, Bjorn Helgaas, lorenzo.pieralisi, linux-pci, linux-pm, linux-arm-kernel, linux-kernel, Fabio Estevam, Dong Aisheng, Sascha Hauer, linux-imx On Fri, Jul 20, 2018 at 5:48 AM Leonard Crestez <leonard.crestez@nxp.com> wrote: > > This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d. > > That commit followed the reference manual but unfortunately the imx7d > manual is incorrect. > I'd also add similar comment to DT file to prevent people from trying to "fix" this in the future. Also, this change is going to break QEMU's mapping found here: https://git.qemu.org/?p=qemu.git;a=blob;f=hw/arm/fsl-imx7.c;h=d5e26855a552e2d52b91f0435a607fae2f88456b;hb=HEAD#l464 any change you are planning to make the change there as well? Thanks, Andrey Smirnov ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping" 2018-07-20 15:33 ` Andrey Smirnov @ 2018-07-23 12:41 ` Leonard Crestez 2018-07-23 18:38 ` Andrey Smirnov 0 siblings, 1 reply; 16+ messages in thread From: Leonard Crestez @ 2018-07-23 12:41 UTC (permalink / raw) To: andrew.smirnov Cc: Richard Zhu, linux-kernel, A.s. Dong, jingoohan1, dl-linux-imx, lorenzo.pieralisi, linux-pm, Fabio Estevam, Joao.Pinto, shawnguo, linux-arm-kernel, bhelgaas, l.stach, kernel, linux-pci On Fri, 2018-07-20 at 08:33 -0700, Andrey Smirnov wrote: > On Fri, Jul 20, 2018 at 5:48 AM Leonard Crestez <leonard.crestez@nxp.com> wrote: > > > > This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d. > > > > That commit followed the reference manual but unfortunately the imx7d > > manual is incorrect. > > I'd also add similar comment to DT file to prevent people from trying > to "fix" this in the future. I'll try to see if I can follow up internally with docs team to get this updated in the next revision of the reference manual. > Also, this change is going to break QEMU's mapping found here: I had no idea that existed, I guess somebody needs to fix that as well. Do you have an imx7d board using pci or did you just test in emulation? -- Regards, Leonard ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping" 2018-07-23 12:41 ` Leonard Crestez @ 2018-07-23 18:38 ` Andrey Smirnov 2018-07-24 11:34 ` Leonard Crestez 0 siblings, 1 reply; 16+ messages in thread From: Andrey Smirnov @ 2018-07-23 18:38 UTC (permalink / raw) To: Leonard Crestez Cc: Richard Zhu, linux-kernel, Dong Aisheng, Jingoo Han, linux-imx, lorenzo.pieralisi, linux-pm, Fabio Estevam, Joao.Pinto, Shawn Guo, linux-arm-kernel, Bjorn Helgaas, Lucas Stach, Sascha Hauer, linux-pci On Mon, Jul 23, 2018 at 5:41 AM Leonard Crestez <leonard.crestez@nxp.com> wrote: > > On Fri, 2018-07-20 at 08:33 -0700, Andrey Smirnov wrote: > > On Fri, Jul 20, 2018 at 5:48 AM Leonard Crestez <leonard.crestez@nxp.com> wrote: > > > > > > This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d. > > > > > > That commit followed the reference manual but unfortunately the imx7d > > > manual is incorrect. > > > > I'd also add similar comment to DT file to prevent people from trying > > to "fix" this in the future. > > I'll try to see if I can follow up internally with docs team to get > this updated in the next revision of the reference manual. > Not sure if we are on the same page here, but what I meant is adding the same explanation that is in your commit message to Device Tree file as well so that if anyone looks at DT code and goes "Huh?" in the future, they wouldn't have to research commit history to see the reason why things the way they are. > > Also, this change is going to break QEMU's mapping found here: > > I had no idea that existed, I guess somebody needs to fix that as well. > I take it it's a no to my "any chance you can fix that as well?" ;-). I'll see if I can find time to do that this week. > Do you have an imx7d board using pci or did you just test in emulation? > I used real i.MX7D Sabre board with i210 network card, when I was porting i.MX7 PCIe patches. However, as per note I made in the original submission: https://lkml.org/lkml/2017/10/9/913 this IRQ swap patch came up as a result of "connecting" emulated ICH4 in QEMU which was using legacy interrupts. Thanks, Andrey Smirnov ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping" 2018-07-23 18:38 ` Andrey Smirnov @ 2018-07-24 11:34 ` Leonard Crestez 0 siblings, 0 replies; 16+ messages in thread From: Leonard Crestez @ 2018-07-24 11:34 UTC (permalink / raw) To: andrew.smirnov Cc: Richard Zhu, linux-kernel, dl-linux-imx, jingoohan1, A.s. Dong, lorenzo.pieralisi, linux-pm, Fabio Estevam, Joao.Pinto, shawnguo, linux-arm-kernel, bhelgaas, l.stach, kernel, linux-pci On Mon, 2018-07-23 at 11:38 -0700, Andrey Smirnov wrote: > On Mon, Jul 23, 2018 at 5:41 AM Leonard Crestez <leonard.crestez@nxp.com> wrote: > > On Fri, 2018-07-20 at 08:33 -0700, Andrey Smirnov wrote: > > > On Fri, Jul 20, 2018 at 5:48 AM Leonard Crestez <leonard.crestez@nxp.com> wrote: > > > > > > > > This reverts commit 1c86c9dd82f859b474474a7fee0d5195da2c9c1d. > > > > > > > > That commit followed the reference manual but unfortunately the imx7d > > > > manual is incorrect. > > > > > > I'd also add similar comment to DT file to prevent people from trying > > > to "fix" this in the future. > > > > I'll try to see if I can follow up internally with docs team to get > > this updated in the next revision of the reference manual. > > Not sure if we are on the same page here, but what I meant is adding > the same explanation that is in your commit message to Device Tree > file as well so that if anyone looks at DT code and goes "Huh?" in the > future, they wouldn't have to research commit history to see the > reason why things the way they are. OK, I will add a comment on irq mappings in the next version explaining that the reference manual is incorrect. > > > Also, this change is going to break QEMU's mapping found here: > > > > I had no idea that existed, I guess somebody needs to fix that as well. > > I take it it's a no to my "any chance you can fix that as well?" ;-). > I'll see if I can find time to do that this week. Sorry, I'm not familiar with qemu source at all. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 2/3] reset: imx7: Fix always writing bits as 0 2018-07-20 12:47 [PATCH v2 0/3] PCI: imx: Initial imx7d pm support Leonard Crestez 2018-07-20 12:47 ` [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping" Leonard Crestez @ 2018-07-20 12:47 ` Leonard Crestez 2018-07-23 9:41 ` Lucas Stach 2018-07-20 12:47 ` [PATCH v2 3/3] PCI: imx: Initial imx7d pm support Leonard Crestez 2 siblings, 1 reply; 16+ messages in thread From: Leonard Crestez @ 2018-07-20 12:47 UTC (permalink / raw) To: Lucas Stach, Richard Zhu, Andrey Smirnov Cc: Shawn Guo, Joao Pinto, Jingoo Han, Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, linux-pm, linux-arm-kernel, linux-kernel, Fabio Estevam, Dong Aisheng, kernel, linux-imx Right now the only user of reset-imx7 is pci-imx6 and the reset_control_assert and deassert calls on pciephy_reset don't toggle the PCIEPHY_BTN and PCIEPHY_G_RST bits as expected. Fix this by writing 1 or 0 respectively. The reference manual is not very clear regarding SRC_PCIEPHY_RCR but for other registers like MIPIPHY and HSICPHY the bits are explicitly documented as "1 means assert, 0 means deassert". The values are still reversed for IMX7_RESET_PCIE_CTRL_APPS_EN. Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> Reviewed-by: Lucas Stach <l.stach@pengutronix.de> --- drivers/reset/reset-imx7.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c index 4db177bc89bc..fdeac1946429 100644 --- a/drivers/reset/reset-imx7.c +++ b/drivers/reset/reset-imx7.c @@ -78,11 +78,11 @@ static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev) static int imx7_reset_set(struct reset_controller_dev *rcdev, unsigned long id, bool assert) { struct imx7_src *imx7src = to_imx7_src(rcdev); const struct imx7_src_signal *signal = &imx7_src_signals[id]; - unsigned int value = 0; + unsigned int value = assert ? signal->bit : 0; switch (id) { case IMX7_RESET_PCIEPHY: /* * wait for more than 10us to release phy g_rst and -- 2.17.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/3] reset: imx7: Fix always writing bits as 0 2018-07-20 12:47 ` [PATCH v2 2/3] reset: imx7: Fix always writing bits as 0 Leonard Crestez @ 2018-07-23 9:41 ` Lucas Stach 2018-07-23 11:02 ` Philipp Zabel 0 siblings, 1 reply; 16+ messages in thread From: Lucas Stach @ 2018-07-23 9:41 UTC (permalink / raw) To: Leonard Crestez, Richard Zhu, Andrey Smirnov, Philipp Zabel Cc: Shawn Guo, Joao Pinto, Jingoo Han, Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, linux-pm, linux-arm-kernel, linux-kernel, Fabio Estevam, Dong Aisheng, kernel, linux-imx As this doesn't depend on any other patch in this series, I think it would be fine if Philipp takes this patch through the reset tree. Regards, Lucas Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez: > Right now the only user of reset-imx7 is pci-imx6 and the > reset_control_assert and deassert calls on pciephy_reset don't toggle > the PCIEPHY_BTN and PCIEPHY_G_RST bits as expected. Fix this by writing > 1 or 0 respectively. > > The reference manual is not very clear regarding SRC_PCIEPHY_RCR but for > other registers like MIPIPHY and HSICPHY the bits are explicitly > documented as "1 means assert, 0 means deassert". > > The values are still reversed for IMX7_RESET_PCIE_CTRL_APPS_EN. > > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > > Reviewed-by: Lucas Stach <l.stach@pengutronix.de> > --- > drivers/reset/reset-imx7.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c > index 4db177bc89bc..fdeac1946429 100644 > --- a/drivers/reset/reset-imx7.c > +++ b/drivers/reset/reset-imx7.c > @@ -78,11 +78,11 @@ static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev) > static int imx7_reset_set(struct reset_controller_dev *rcdev, > > unsigned long id, bool assert) > { > > struct imx7_src *imx7src = to_imx7_src(rcdev); > > const struct imx7_src_signal *signal = &imx7_src_signals[id]; > > - unsigned int value = 0; > > + unsigned int value = assert ? signal->bit : 0; > > > switch (id) { > > case IMX7_RESET_PCIEPHY: > > /* > > * wait for more than 10us to release phy g_rst and ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/3] reset: imx7: Fix always writing bits as 0 2018-07-23 9:41 ` Lucas Stach @ 2018-07-23 11:02 ` Philipp Zabel 0 siblings, 0 replies; 16+ messages in thread From: Philipp Zabel @ 2018-07-23 11:02 UTC (permalink / raw) To: Lucas Stach, Leonard Crestez, Richard Zhu, Andrey Smirnov Cc: Shawn Guo, Joao Pinto, Jingoo Han, Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, linux-pm, linux-arm-kernel, linux-kernel, Fabio Estevam, Dong Aisheng, kernel, linux-imx On Mon, 2018-07-23 at 11:41 +0200, Lucas Stach wrote: > As this doesn't depend on any other patch in this series, I think it > would be fine if Philipp takes this patch through the reset tree. > > Regards, > Lucas > > Am Freitag, den 20.07.2018, 15:47 +0300 schrieb Leonard Crestez: > > Right now the only user of reset-imx7 is pci-imx6 and the > > reset_control_assert and deassert calls on pciephy_reset don't toggle > > the PCIEPHY_BTN and PCIEPHY_G_RST bits as expected. Fix this by writing > > 1 or 0 respectively. > > > > The reference manual is not very clear regarding SRC_PCIEPHY_RCR but for > > other registers like MIPIPHY and HSICPHY the bits are explicitly > > documented as "1 means assert, 0 means deassert". > > > > The values are still reversed for IMX7_RESET_PCIE_CTRL_APPS_EN. > > > > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > > > Reviewed-by: Lucas Stach <l.stach@pengutronix.de> Thank you, applied to reset/fixes. regards Philipp ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 3/3] PCI: imx: Initial imx7d pm support 2018-07-20 12:47 [PATCH v2 0/3] PCI: imx: Initial imx7d pm support Leonard Crestez 2018-07-20 12:47 ` [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping" Leonard Crestez 2018-07-20 12:47 ` [PATCH v2 2/3] reset: imx7: Fix always writing bits as 0 Leonard Crestez @ 2018-07-20 12:47 ` Leonard Crestez 2018-07-20 13:38 ` Fabio Estevam 2018-07-23 9:38 ` Lucas Stach 2 siblings, 2 replies; 16+ messages in thread From: Leonard Crestez @ 2018-07-20 12:47 UTC (permalink / raw) To: Lucas Stach, Richard Zhu, Andrey Smirnov Cc: Shawn Guo, Joao Pinto, Jingoo Han, Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, linux-pm, linux-arm-kernel, linux-kernel, Fabio Estevam, Dong Aisheng, kernel, linux-imx 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. 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/controller/dwc/pci-imx6.c | 95 +++++++++++++++++++++++++-- 1 file changed, 90 insertions(+), 5 deletions(-) diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index 80f604602783..daebee905108 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/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,81 @@ 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 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 void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie) +{ + clk_disable_unprepare(imx6_pcie->pcie); + clk_disable_unprepare(imx6_pcie->pcie_phy); + clk_disable_unprepare(imx6_pcie->pcie_bus); + + if (imx6_pcie->variant == IMX7D) { + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, + IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, + IMX7D_GPR12_PCIE_PHY_REFCLK_SEL); + } +} + +static int imx6_pcie_suspend_noirq(struct device *dev) +{ + struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev); + + if (imx6_pcie->variant != IMX7D) + return 0; + + imx6_pcie_clk_disable(imx6_pcie); + imx6_pcie_ltssm_disable(dev); + + return 0; +} + +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"); + + return 0; +} +#endif + +static const struct dev_pm_ops imx6_pcie_pm_ops = { + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx6_pcie_suspend_noirq, + imx6_pcie_resume_noirq) +}; + 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 +931,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, }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support 2018-07-20 12:47 ` [PATCH v2 3/3] PCI: imx: Initial imx7d pm support Leonard Crestez @ 2018-07-20 13:38 ` Fabio Estevam 2018-07-23 9:38 ` Lucas Stach 1 sibling, 0 replies; 16+ messages in thread From: Fabio Estevam @ 2018-07-20 13:38 UTC (permalink / raw) To: Leonard Crestez Cc: Lucas Stach, Richard Zhu, Andrey Smirnov, Dong Aisheng, Joao Pinto, linux-pm, Jingoo Han, linux-kernel, Fabio Estevam, Lorenzo Pieralisi, NXP Linux Team, Sascha Hauer, linux-pci, Bjorn Helgaas, Shawn Guo, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE Hi Leonard, On Fri, Jul 20, 2018 at 9:47 AM, Leonard Crestez <leonard.crestez@nxp.com> wrote: > +static int imx6_pcie_resume_noirq(struct device *dev) > +{ > + int ret = 0; There is no need for initializing 'ret' here. > + 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"); Shouldn't the error be propagated? Also, dev_err() is preferred instead of pr_err(). ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support 2018-07-20 12:47 ` [PATCH v2 3/3] PCI: imx: Initial imx7d pm support Leonard Crestez 2018-07-20 13:38 ` Fabio Estevam @ 2018-07-23 9:38 ` Lucas Stach 2018-07-23 12:37 ` Leonard Crestez 1 sibling, 1 reply; 16+ messages in thread From: Lucas Stach @ 2018-07-23 9:38 UTC (permalink / raw) To: Leonard Crestez, Richard Zhu, Andrey Smirnov Cc: Shawn Guo, Joao Pinto, Jingoo Han, Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, linux-pm, linux-arm-kernel, linux-kernel, Fabio Estevam, Dong Aisheng, kernel, linux-imx 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. > > > 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/controller/dwc/pci-imx6.c | 95 +++++++++++++++++++++++++-- > 1 file changed, 90 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 80f604602783..daebee905108 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/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,81 @@ 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 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". Note that implementing the correct sequence requires the core clocks to still be on when disabling LTSSM, so would need to switch ordering of the function calls in imx6_pcie_suspend_noirq. > + break; > > + case IMX7D: > > + reset_control_assert(imx6_pcie->apps_reset); > > + break; > > + } > +} > + > +static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie) > +{ > > + clk_disable_unprepare(imx6_pcie->pcie); > > + clk_disable_unprepare(imx6_pcie->pcie_phy); > > + clk_disable_unprepare(imx6_pcie->pcie_bus); > + > > + if (imx6_pcie->variant == IMX7D) { > > + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, > > + IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, > > + IMX7D_GPR12_PCIE_PHY_REFCLK_SEL); > > + } > +} > + > +static int imx6_pcie_suspend_noirq(struct device *dev) > +{ > > + struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev); > + > > + if (imx6_pcie->variant != IMX7D) > > + return 0; > + > > + imx6_pcie_clk_disable(imx6_pcie); > > + imx6_pcie_ltssm_disable(dev); > + > > + return 0; > +} > + > +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. Regards, Lucas > + > > + return 0; > +} > +#endif > + > +static const struct dev_pm_ops imx6_pcie_pm_ops = { > > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx6_pcie_suspend_noirq, > > + imx6_pcie_resume_noirq) > +}; > + > 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 +931,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, > }; > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support 2018-07-23 9:38 ` Lucas Stach @ 2018-07-23 12:37 ` Leonard Crestez 2018-07-24 10:09 ` Lucas Stach 0 siblings, 1 reply; 16+ messages in thread From: Leonard Crestez @ 2018-07-23 12:37 UTC (permalink / raw) To: l.stach, Richard Zhu, Fabio Estevam Cc: A.s. Dong, linux-kernel, dl-linux-imx, jingoohan1, lorenzo.pieralisi, linux-pm, Joao.Pinto, shawnguo, linux-arm-kernel, andrew.smirnov, bhelgaas, linux-pci, kernel 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. > > +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'll switch to dev_info and drop the error code, is this OK? 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support 2018-07-23 12:37 ` Leonard Crestez @ 2018-07-24 10:09 ` Lucas Stach 2018-07-24 12:04 ` Leonard Crestez 0 siblings, 1 reply; 16+ messages in thread From: Lucas Stach @ 2018-07-24 10:09 UTC (permalink / raw) To: Leonard Crestez, Richard Zhu, Fabio Estevam Cc: A.s. Dong, linux-kernel, dl-linux-imx, jingoohan1, lorenzo.pieralisi, linux-pm, Joao.Pinto, shawnguo, linux-arm-kernel, andrew.smirnov, bhelgaas, linux-pci, kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support 2018-07-24 10:09 ` Lucas Stach @ 2018-07-24 12:04 ` Leonard Crestez 2018-07-24 12:28 ` Lucas Stach 0 siblings, 1 reply; 16+ messages in thread From: Leonard Crestez @ 2018-07-24 12:04 UTC (permalink / raw) To: l.stach, Richard Zhu, Fabio Estevam Cc: dl-linux-imx, linux-kernel, A.s. Dong, jingoohan1, lorenzo.pieralisi, linux-pm, Joao.Pinto, shawnguo, linux-arm-kernel, andrew.smirnov, bhelgaas, linux-pci, kernel On Tue, 2018-07-24 at 12:09 +0200, Lucas Stach wrote: > Am Montag, den 23.07.2018, 12:37 +0000 schrieb Leonard Crestez: > > On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote: > > > 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? But the function will indeed toggle the right bits to initiate ltssm disabling. If this is not useful or can't be used right then it's the caller's problem :) I can add a default: clause to the switch which returns -ENOSYS and let IMX6Q fall that way, would that be OK? This would also help when adding new variants. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] PCI: imx: Initial imx7d pm support 2018-07-24 12:04 ` Leonard Crestez @ 2018-07-24 12:28 ` Lucas Stach 0 siblings, 0 replies; 16+ messages in thread From: Lucas Stach @ 2018-07-24 12:28 UTC (permalink / raw) To: Leonard Crestez, Richard Zhu, Fabio Estevam Cc: dl-linux-imx, linux-kernel, A.s. Dong, jingoohan1, lorenzo.pieralisi, linux-pm, Joao.Pinto, shawnguo, linux-arm-kernel, andrew.smirnov, bhelgaas, linux-pci, kernel Am Dienstag, den 24.07.2018, 12:04 +0000 schrieb Leonard Crestez: > On Tue, 2018-07-24 at 12:09 +0200, Lucas Stach wrote: > > Am Montag, den 23.07.2018, 12:37 +0000 schrieb Leonard Crestez: > > > On Mon, 2018-07-23 at 11:38 +0200, Lucas Stach wrote: > > > > 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? > > But the function will indeed toggle the right bits to initiate ltssm > disabling. If this is not useful or can't be used right then it's the > caller's problem :) I don't agree with that. I would expect a function that is called ltssm_disable to do so in a way that is safe on the hardware that it claims to handle, which in case of i.MX6 means bashing the LTSSM into detect state before toggling the GPR bit. > I can add a default: clause to the switch which returns -ENOSYS and let > IMX6Q fall that way, would that be OK? This would also help when adding > new variants. Yes, given that there is currently no way to test this on i.MX6, returning -ENOSYS sound much better. This at least tells anyone who intends to use this function that there is indeed missing functionality there. Regards, Lucas ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-07-24 12:28 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-20 12:47 [PATCH v2 0/3] PCI: imx: Initial imx7d pm support Leonard Crestez 2018-07-20 12:47 ` [PATCH v2 1/3] Revert "ARM: dts: imx7d: Invert legacy PCI irq mapping" Leonard Crestez 2018-07-20 15:33 ` Andrey Smirnov 2018-07-23 12:41 ` Leonard Crestez 2018-07-23 18:38 ` Andrey Smirnov 2018-07-24 11:34 ` Leonard Crestez 2018-07-20 12:47 ` [PATCH v2 2/3] reset: imx7: Fix always writing bits as 0 Leonard Crestez 2018-07-23 9:41 ` Lucas Stach 2018-07-23 11:02 ` Philipp Zabel 2018-07-20 12:47 ` [PATCH v2 3/3] PCI: imx: Initial imx7d pm support Leonard Crestez 2018-07-20 13:38 ` Fabio Estevam 2018-07-23 9:38 ` Lucas Stach 2018-07-23 12:37 ` Leonard Crestez 2018-07-24 10:09 ` Lucas Stach 2018-07-24 12:04 ` Leonard Crestez 2018-07-24 12:28 ` Lucas Stach
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).