Discussion:
[PATCH 2/2] PCI: imx6: fix boot hang when link already enabled
Lucas Stach
2014-07-31 18:16:05 UTC
Permalink
This fixes a boot hang observed when the bootloader
already enabled the PCIe link for it's own use. The
fundamental problem is that Freescale forgot to wire
up the core reset, so software doesn't have a sane way
to get the core into a defined state.

According to the DW PCIe core reference manual configuration
of the core may only happen when the LTSSM is disabled, so
this is one of the first things we need to do. Apparently
this isn't safe to do when the LTSSM is in any other state
than "detect" as we observe an instant machine hang when
trying to do so while the link is already up.

As a workaround force LTSSM into detect state right before
hitting the disable switch.

Reported-by: Fabio Estevam <***@freescale.com>
Signed-off-by: Lucas Stach <***@pengutronix.de>
Acked-by: Tim Harvey <***@gateworks.com>
---
v2: - messed up the first submission by omitting a chunk
v3: - check if bootloader actually enabled link before
touching any core registers
- add a comment to explain things a bit
- add shutdown hook to clean state on reboot

Fabios delay workaround worked because of the following
conditions:
1. The driver gets probed and pulls the peripheral reset GPIO
2. Peripheral is held in reset, so won't answer any link
negotiation requests
3. The LTSSM times out and falls back into detect state
after 24ms (that's why a 30ms delay helps)
4. After LTSSM entered detect state it's safe to hit the
disable switch
---
drivers/pci/host/pci-imx6.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index a568efaa331c..1be607360988 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -49,6 +49,9 @@ struct imx6_pcie {

/* PCIe Port Logic registers (memory-mapped) */
#define PL_OFFSET 0x700
+#define PCIE_PL_PFLR (PL_OFFSET + 0x08)
+#define PCIE_PL_PFLR_LINK_STATE_MASK (0x3f << 16)
+#define PCIE_PL_PFLR_FORCE_LINK (1 << 15)
#define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
#define PCIE_PHY_DEBUG_R1 (PL_OFFSET + 0x2c)
#define PCIE_PHY_DEBUG_R1_XMLH_LINK_IN_TRAINING (1 << 29)
@@ -214,6 +217,31 @@ static int imx6q_pcie_abort_handler(unsigned long addr,
static int imx6_pcie_assert_core_reset(struct pcie_port *pp)
{
struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
+ u32 val, gpr1, gpr12;
+
+ /*
+ * If the bootloader already enabled the link we need some special
+ * handling to get the core back into a state where it is safe to
+ * touch it for configuration. As there is no dedicated reset signal
+ * wired up for MX6QDL, we need to manually force LTSSM into "detect"
+ * state before completely disabling LTSSM, which is a prerequisite
+ * for core configuration.
+ * If both LTSSM_ENABLE and REF_SSP_ENABLE are active we have a strong
+ * indication that the bootloader activated the link.
+ */
+ regmap_read(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, &gpr1);
+ regmap_read(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, &gpr12);
+
+ if ((gpr1 & IMX6Q_GPR1_PCIE_REF_CLK_EN) &&
+ (gpr12 & IMX6Q_GPR12_PCIE_CTL_2)) {
+ val = readl(pp->dbi_base + PCIE_PL_PFLR);
+ val &= ~PCIE_PL_PFLR_LINK_STATE_MASK;
+ val |= PCIE_PL_PFLR_FORCE_LINK;
+ writel(val, pp->dbi_base + PCIE_PL_PFLR);
+
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX6Q_GPR12_PCIE_CTL_2, 0 << 10);
+ }

regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
IMX6Q_GPR1_PCIE_TEST_PD, 1 << 18);
@@ -589,6 +617,14 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
return 0;
}

+static void imx6_pcie_shutdown(struct platform_device *pdev)
+{
+ struct imx6_pcie *imx6_pcie = platform_get_drvdata(pdev);
+
+ /* bring down link, so bootloader gets clean state in case of reboot */
+ imx6_pcie_assert_core_reset(&imx6_pcie->pp);
+}
+
static const struct of_device_id imx6_pcie_of_match[] = {
{ .compatible = "fsl,imx6q-pcie", },
{},
@@ -601,6 +637,7 @@ static struct platform_driver imx6_pcie_driver = {
.owner = THIS_MODULE,
.of_match_table = imx6_pcie_of_match,
},
+ .shutdown = imx6_pcie_shutdown,
};

/* Freescale PCIe driver does not allow module unload */
--
2.0.1
Lucas Stach
2014-07-31 18:16:04 UTC
Permalink
Also drop Shawn Guo per his request.

Signed-off-by: Lucas Stach <***@pengutronix.de>
Acked-by: Shawn Guo <***@freescale.com>
Acked-by: Richard Zhu <***@freescale.com>
Acked-by: Jingoo Han <***@samsung.com>
---
MAINTAINERS | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index f35d07d284b6..cd4224d0a029 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6774,7 +6774,7 @@ F: arch/x86/kernel/quirks.c

PCI DRIVER FOR IMX6
M: Richard Zhu <***@freescale.com>
-M: Shawn Guo <***@freescale.com>
+M: Lucas Stach <***@pengutronix.de>
L: linux-***@vger.kernel.org
L: linux-arm-***@lists.infradead.org (moderated for non-subscribers)
S: Maintained
--
2.0.1
Tim Harvey
2014-08-09 17:49:57 UTC
Permalink
Post by Lucas Stach
This fixes a boot hang observed when the bootloader
already enabled the PCIe link for it's own use. The
fundamental problem is that Freescale forgot to wire
up the core reset, so software doesn't have a sane way
to get the core into a defined state.
According to the DW PCIe core reference manual configuration
of the core may only happen when the LTSSM is disabled, so
this is one of the first things we need to do. Apparently
this isn't safe to do when the LTSSM is in any other state
than "detect" as we observe an instant machine hang when
trying to do so while the link is already up.
As a workaround force LTSSM into detect state right before
hitting the disable switch.
---
v2: - messed up the first submission by omitting a chunk
v3: - check if bootloader actually enabled link before
touching any core registers
- add a comment to explain things a bit
- add shutdown hook to clean state on reboot
Fabios delay workaround worked because of the following
1. The driver gets probed and pulls the peripheral reset GPIO
2. Peripheral is held in reset, so won't answer any link
negotiation requests
3. The LTSSM times out and falls back into detect state
after 24ms (that's why a 30ms delay helps)
4. After LTSSM entered detect state it's safe to hit the
disable switch
---
drivers/pci/host/pci-imx6.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index a568efaa331c..1be607360988 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -49,6 +49,9 @@ struct imx6_pcie {
/* PCIe Port Logic registers (memory-mapped) */
#define PL_OFFSET 0x700
+#define PCIE_PL_PFLR (PL_OFFSET + 0x08)
+#define PCIE_PL_PFLR_LINK_STATE_MASK (0x3f << 16)
+#define PCIE_PL_PFLR_FORCE_LINK (1 << 15)
#define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
#define PCIE_PHY_DEBUG_R1 (PL_OFFSET + 0x2c)
#define PCIE_PHY_DEBUG_R1_XMLH_LINK_IN_TRAINING (1 << 29)
@@ -214,6 +217,31 @@ static int imx6q_pcie_abort_handler(unsigned long addr,
static int imx6_pcie_assert_core_reset(struct pcie_port *pp)
{
struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
+ u32 val, gpr1, gpr12;
+
+ /*
+ * If the bootloader already enabled the link we need some special
+ * handling to get the core back into a state where it is safe to
+ * touch it for configuration. As there is no dedicated reset signal
+ * wired up for MX6QDL, we need to manually force LTSSM into "detect"
+ * state before completely disabling LTSSM, which is a prerequisite
+ * for core configuration.
+ * If both LTSSM_ENABLE and REF_SSP_ENABLE are active we have a strong
+ * indication that the bootloader activated the link.
+ */
+ regmap_read(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, &gpr1);
+ regmap_read(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, &gpr12);
+
+ if ((gpr1 & IMX6Q_GPR1_PCIE_REF_CLK_EN) &&
+ (gpr12 & IMX6Q_GPR12_PCIE_CTL_2)) {
+ val = readl(pp->dbi_base + PCIE_PL_PFLR);
+ val &= ~PCIE_PL_PFLR_LINK_STATE_MASK;
+ val |= PCIE_PL_PFLR_FORCE_LINK;
+ writel(val, pp->dbi_base + PCIE_PL_PFLR);
Lucas,

I hate to delay this patch any longer as I believe its critical that
this gets in the current kernel. I did however encounter an issue when
I backported this to another (older 3.10 kernel I use). The access
above to dbi_base to force the link down assumes that PCIe clock is
enabled (or it will hang the system). This obviously must be the case
for the current kernel (the bootloader enables it and I guess the
clock setup for imx6 doesn't disturb it), but on the particular kernel
I was backporting to there must be enough differences in the clock
tree that the clock setup disabled the PCIe clocks. Thus I had to
enable them early, right before the access to dbi_base above in that
kernel.

I wonder if we should at least put a comment before the dbi_base
access above about the assumption that the bootloader also enabled
PCIe clock and the clock config left it as such so we don't get burned
later on if the clock config changes and causes a hang here.

Regards,

Tim
Post by Lucas Stach
+
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX6Q_GPR12_PCIE_CTL_2, 0 << 10);
+ }
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
IMX6Q_GPR1_PCIE_TEST_PD, 1 << 18);
@@ -589,6 +617,14 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
return 0;
}
+static void imx6_pcie_shutdown(struct platform_device *pdev)
+{
+ struct imx6_pcie *imx6_pcie = platform_get_drvdata(pdev);
+
+ /* bring down link, so bootloader gets clean state in case of reboot */
+ imx6_pcie_assert_core_reset(&imx6_pcie->pp);
+}
+
static const struct of_device_id imx6_pcie_of_match[] = {
{ .compatible = "fsl,imx6q-pcie", },
{},
@@ -601,6 +637,7 @@ static struct platform_driver imx6_pcie_driver = {
.owner = THIS_MODULE,
.of_match_table = imx6_pcie_of_match,
},
+ .shutdown = imx6_pcie_shutdown,
};
/* Freescale PCIe driver does not allow module unload */
--
2.0.1
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
More majordomo info at http://vger.kernel.org/majordomo-info.html
Lucas Stach
2014-08-11 08:24:13 UTC
Permalink
Hi Tim,

Am Samstag, den 09.08.2014, 10:49 -0700 schrieb Tim Harvey:
[...]
Post by Lucas Stach
i/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
Post by Lucas Stach
index a568efaa331c..1be607360988 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -49,6 +49,9 @@ struct imx6_pcie {
/* PCIe Port Logic registers (memory-mapped) */
#define PL_OFFSET 0x700
+#define PCIE_PL_PFLR (PL_OFFSET + 0x08)
+#define PCIE_PL_PFLR_LINK_STATE_MASK (0x3f << 16)
+#define PCIE_PL_PFLR_FORCE_LINK (1 << 15)
#define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
#define PCIE_PHY_DEBUG_R1 (PL_OFFSET + 0x2c)
#define PCIE_PHY_DEBUG_R1_XMLH_LINK_IN_TRAINING (1 << 29)
@@ -214,6 +217,31 @@ static int imx6q_pcie_abort_handler(unsigned long addr,
static int imx6_pcie_assert_core_reset(struct pcie_port *pp)
{
struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
+ u32 val, gpr1, gpr12;
+
+ /*
+ * If the bootloader already enabled the link we need some special
+ * handling to get the core back into a state where it is safe to
+ * touch it for configuration. As there is no dedicated reset signal
+ * wired up for MX6QDL, we need to manually force LTSSM into "detect"
+ * state before completely disabling LTSSM, which is a prerequisite
+ * for core configuration.
+ * If both LTSSM_ENABLE and REF_SSP_ENABLE are active we have a strong
+ * indication that the bootloader activated the link.
+ */
+ regmap_read(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, &gpr1);
+ regmap_read(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, &gpr12);
+
+ if ((gpr1 & IMX6Q_GPR1_PCIE_REF_CLK_EN) &&
+ (gpr12 & IMX6Q_GPR12_PCIE_CTL_2)) {
+ val = readl(pp->dbi_base + PCIE_PL_PFLR);
+ val &= ~PCIE_PL_PFLR_LINK_STATE_MASK;
+ val |= PCIE_PL_PFLR_FORCE_LINK;
+ writel(val, pp->dbi_base + PCIE_PL_PFLR);
Lucas,
I hate to delay this patch any longer as I believe its critical that
this gets in the current kernel. I did however encounter an issue when
I backported this to another (older 3.10 kernel I use). The access
above to dbi_base to force the link down assumes that PCIe clock is
enabled (or it will hang the system). This obviously must be the case
for the current kernel (the bootloader enables it and I guess the
clock setup for imx6 doesn't disturb it), but on the particular kernel
I was backporting to there must be enough differences in the clock
tree that the clock setup disabled the PCIe clocks. Thus I had to
enable them early, right before the access to dbi_base above in that
kernel.
I wonder if we should at least put a comment before the dbi_base
access above about the assumption that the bootloader also enabled
PCIe clock and the clock config left it as such so we don't get burned
later on if the clock config changes and causes a hang here.
What clock are we talking about here exactly? If it's just the register
clock "pcie" we may as well enable it early to make things robust. Other
clocks may be a bit more problematic.

Regards,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Tim Harvey
2014-08-15 05:01:48 UTC
Permalink
Post by Lucas Stach
Hi Tim,
[...]
Post by Lucas Stach
i/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
Post by Lucas Stach
index a568efaa331c..1be607360988 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -49,6 +49,9 @@ struct imx6_pcie {
/* PCIe Port Logic registers (memory-mapped) */
#define PL_OFFSET 0x700
+#define PCIE_PL_PFLR (PL_OFFSET + 0x08)
+#define PCIE_PL_PFLR_LINK_STATE_MASK (0x3f << 16)
+#define PCIE_PL_PFLR_FORCE_LINK (1 << 15)
#define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
#define PCIE_PHY_DEBUG_R1 (PL_OFFSET + 0x2c)
#define PCIE_PHY_DEBUG_R1_XMLH_LINK_IN_TRAINING (1 << 29)
@@ -214,6 +217,31 @@ static int imx6q_pcie_abort_handler(unsigned long addr,
static int imx6_pcie_assert_core_reset(struct pcie_port *pp)
{
struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
+ u32 val, gpr1, gpr12;
+
+ /*
+ * If the bootloader already enabled the link we need some special
+ * handling to get the core back into a state where it is safe to
+ * touch it for configuration. As there is no dedicated reset signal
+ * wired up for MX6QDL, we need to manually force LTSSM into "detect"
+ * state before completely disabling LTSSM, which is a prerequisite
+ * for core configuration.
+ * If both LTSSM_ENABLE and REF_SSP_ENABLE are active we have a strong
+ * indication that the bootloader activated the link.
+ */
+ regmap_read(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, &gpr1);
+ regmap_read(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, &gpr12);
+
+ if ((gpr1 & IMX6Q_GPR1_PCIE_REF_CLK_EN) &&
+ (gpr12 & IMX6Q_GPR12_PCIE_CTL_2)) {
+ val = readl(pp->dbi_base + PCIE_PL_PFLR);
+ val &= ~PCIE_PL_PFLR_LINK_STATE_MASK;
+ val |= PCIE_PL_PFLR_FORCE_LINK;
+ writel(val, pp->dbi_base + PCIE_PL_PFLR);
Lucas,
I hate to delay this patch any longer as I believe its critical that
this gets in the current kernel. I did however encounter an issue when
I backported this to another (older 3.10 kernel I use). The access
above to dbi_base to force the link down assumes that PCIe clock is
enabled (or it will hang the system). This obviously must be the case
for the current kernel (the bootloader enables it and I guess the
clock setup for imx6 doesn't disturb it), but on the particular kernel
I was backporting to there must be enough differences in the clock
tree that the clock setup disabled the PCIe clocks. Thus I had to
enable them early, right before the access to dbi_base above in that
kernel.
I wonder if we should at least put a comment before the dbi_base
access above about the assumption that the bootloader also enabled
PCIe clock and the clock config left it as such so we don't get burned
later on if the clock config changes and causes a hang here.
What clock are we talking about here exactly? If it's just the register
clock "pcie" we may as well enable it early to make things robust. Other
clocks may be a bit more problematic.
Lucas,

I'm not really sure, I just duplicated what was being done in the
regular clock setup to resolve my issue on a 3.10 vendor kernel where
I have backported the pci driver from mainline. I'm not clear yet why
the clocks still wouldn't be enabled from the bootloader, but I assume
its because of something else that is happening in that vendor kernel.
I don't feel there is anything wrong with your patch and have already
acked it - it solves a real issue with the current driver. Its just
worth realizing that clocks do have to be setup before you access
dbi_base which currently is true, but in the future may depend on
changes that occur outside of the pci driver.

Tim
Bjorn Helgaas
2014-08-14 20:22:48 UTC
Permalink
Post by Lucas Stach
This fixes a boot hang observed when the bootloader
already enabled the PCIe link for it's own use. The
fundamental problem is that Freescale forgot to wire
up the core reset, so software doesn't have a sane way
to get the core into a defined state.
According to the DW PCIe core reference manual configuration
of the core may only happen when the LTSSM is disabled, so
this is one of the first things we need to do. Apparently
this isn't safe to do when the LTSSM is in any other state
than "detect" as we observe an instant machine hang when
trying to do so while the link is already up.
As a workaround force LTSSM into detect state right before
hitting the disable switch.
I don't know anything about this hardware, but from the description, this
sounds racy. As I understand it, LTSSM is a hardware-managed state
machine. What prevents the hardware from transitioning out of the "Detect"
state after you force it into "Detect" but before you hit the disable
switch?
Post by Lucas Stach
---
v2: - messed up the first submission by omitting a chunk
v3: - check if bootloader actually enabled link before
touching any core registers
- add a comment to explain things a bit
- add shutdown hook to clean state on reboot
Fabios delay workaround worked because of the following
1. The driver gets probed and pulls the peripheral reset GPIO
2. Peripheral is held in reset, so won't answer any link
negotiation requests
3. The LTSSM times out and falls back into detect state
after 24ms (that's why a 30ms delay helps)
4. After LTSSM entered detect state it's safe to hit the
disable switch
---
drivers/pci/host/pci-imx6.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index a568efaa331c..1be607360988 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -49,6 +49,9 @@ struct imx6_pcie {
/* PCIe Port Logic registers (memory-mapped) */
#define PL_OFFSET 0x700
+#define PCIE_PL_PFLR (PL_OFFSET + 0x08)
+#define PCIE_PL_PFLR_LINK_STATE_MASK (0x3f << 16)
+#define PCIE_PL_PFLR_FORCE_LINK (1 << 15)
#define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
#define PCIE_PHY_DEBUG_R1 (PL_OFFSET + 0x2c)
#define PCIE_PHY_DEBUG_R1_XMLH_LINK_IN_TRAINING (1 << 29)
@@ -214,6 +217,31 @@ static int imx6q_pcie_abort_handler(unsigned long addr,
static int imx6_pcie_assert_core_reset(struct pcie_port *pp)
{
struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
+ u32 val, gpr1, gpr12;
+
+ /*
+ * If the bootloader already enabled the link we need some special
+ * handling to get the core back into a state where it is safe to
+ * touch it for configuration. As there is no dedicated reset signal
+ * wired up for MX6QDL, we need to manually force LTSSM into "detect"
+ * state before completely disabling LTSSM, which is a prerequisite
+ * for core configuration.
+ * If both LTSSM_ENABLE and REF_SSP_ENABLE are active we have a strong
+ * indication that the bootloader activated the link.
+ */
+ regmap_read(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, &gpr1);
+ regmap_read(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, &gpr12);
+
+ if ((gpr1 & IMX6Q_GPR1_PCIE_REF_CLK_EN) &&
+ (gpr12 & IMX6Q_GPR12_PCIE_CTL_2)) {
+ val = readl(pp->dbi_base + PCIE_PL_PFLR);
+ val &= ~PCIE_PL_PFLR_LINK_STATE_MASK;
+ val |= PCIE_PL_PFLR_FORCE_LINK;
+ writel(val, pp->dbi_base + PCIE_PL_PFLR);
+
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX6Q_GPR12_PCIE_CTL_2, 0 << 10);
+ }
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
IMX6Q_GPR1_PCIE_TEST_PD, 1 << 18);
@@ -589,6 +617,14 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
return 0;
}
+static void imx6_pcie_shutdown(struct platform_device *pdev)
+{
+ struct imx6_pcie *imx6_pcie = platform_get_drvdata(pdev);
+
+ /* bring down link, so bootloader gets clean state in case of reboot */
+ imx6_pcie_assert_core_reset(&imx6_pcie->pp);
+}
+
static const struct of_device_id imx6_pcie_of_match[] = {
{ .compatible = "fsl,imx6q-pcie", },
{},
@@ -601,6 +637,7 @@ static struct platform_driver imx6_pcie_driver = {
.owner = THIS_MODULE,
.of_match_table = imx6_pcie_of_match,
},
+ .shutdown = imx6_pcie_shutdown,
};
/* Freescale PCIe driver does not allow module unload */
--
2.0.1
Lucas Stach
2014-08-15 09:57:29 UTC
Permalink
Post by Bjorn Helgaas
Post by Lucas Stach
This fixes a boot hang observed when the bootloader
already enabled the PCIe link for it's own use. The
fundamental problem is that Freescale forgot to wire
up the core reset, so software doesn't have a sane way
to get the core into a defined state.
According to the DW PCIe core reference manual configuration
of the core may only happen when the LTSSM is disabled, so
this is one of the first things we need to do. Apparently
this isn't safe to do when the LTSSM is in any other state
than "detect" as we observe an instant machine hang when
trying to do so while the link is already up.
As a workaround force LTSSM into detect state right before
hitting the disable switch.
I don't know anything about this hardware, but from the description, this
sounds racy. As I understand it, LTSSM is a hardware-managed state
machine. What prevents the hardware from transitioning out of the "Detect"
state after you force it into "Detect" but before you hit the disable
switch?
There is indeed a small race window here, but I don't see how we could
avoid it. Getting the core back into a sane state without a proper
hardware reset isn't an easy task. Initially I thought the LinkDisable
bit in the Link Control Register might do the trick, but this also shows
lockups. I'm still investigating if we can make this more safe.

But for the time being I would rather accept this small race window in
this workaround than hanging the machine every time someone enabled the
PCIe link before jumping into the kernel.
Post by Bjorn Helgaas
Post by Lucas Stach
---
v2: - messed up the first submission by omitting a chunk
v3: - check if bootloader actually enabled link before
touching any core registers
- add a comment to explain things a bit
- add shutdown hook to clean state on reboot
Fabios delay workaround worked because of the following
1. The driver gets probed and pulls the peripheral reset GPIO
2. Peripheral is held in reset, so won't answer any link
negotiation requests
3. The LTSSM times out and falls back into detect state
after 24ms (that's why a 30ms delay helps)
4. After LTSSM entered detect state it's safe to hit the
disable switch
---
drivers/pci/host/pci-imx6.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index a568efaa331c..1be607360988 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -49,6 +49,9 @@ struct imx6_pcie {
/* PCIe Port Logic registers (memory-mapped) */
#define PL_OFFSET 0x700
+#define PCIE_PL_PFLR (PL_OFFSET + 0x08)
+#define PCIE_PL_PFLR_LINK_STATE_MASK (0x3f << 16)
+#define PCIE_PL_PFLR_FORCE_LINK (1 << 15)
#define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
#define PCIE_PHY_DEBUG_R1 (PL_OFFSET + 0x2c)
#define PCIE_PHY_DEBUG_R1_XMLH_LINK_IN_TRAINING (1 << 29)
@@ -214,6 +217,31 @@ static int imx6q_pcie_abort_handler(unsigned long addr,
static int imx6_pcie_assert_core_reset(struct pcie_port *pp)
{
struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
+ u32 val, gpr1, gpr12;
+
+ /*
+ * If the bootloader already enabled the link we need some special
+ * handling to get the core back into a state where it is safe to
+ * touch it for configuration. As there is no dedicated reset signal
+ * wired up for MX6QDL, we need to manually force LTSSM into "detect"
+ * state before completely disabling LTSSM, which is a prerequisite
+ * for core configuration.
+ * If both LTSSM_ENABLE and REF_SSP_ENABLE are active we have a strong
+ * indication that the bootloader activated the link.
+ */
+ regmap_read(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, &gpr1);
+ regmap_read(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, &gpr12);
+
+ if ((gpr1 & IMX6Q_GPR1_PCIE_REF_CLK_EN) &&
+ (gpr12 & IMX6Q_GPR12_PCIE_CTL_2)) {
+ val = readl(pp->dbi_base + PCIE_PL_PFLR);
+ val &= ~PCIE_PL_PFLR_LINK_STATE_MASK;
+ val |= PCIE_PL_PFLR_FORCE_LINK;
+ writel(val, pp->dbi_base + PCIE_PL_PFLR);
+
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX6Q_GPR12_PCIE_CTL_2, 0 << 10);
+ }
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
IMX6Q_GPR1_PCIE_TEST_PD, 1 << 18);
@@ -589,6 +617,14 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
return 0;
}
+static void imx6_pcie_shutdown(struct platform_device *pdev)
+{
+ struct imx6_pcie *imx6_pcie = platform_get_drvdata(pdev);
+
+ /* bring down link, so bootloader gets clean state in case of reboot */
+ imx6_pcie_assert_core_reset(&imx6_pcie->pp);
+}
+
static const struct of_device_id imx6_pcie_of_match[] = {
{ .compatible = "fsl,imx6q-pcie", },
{},
@@ -601,6 +637,7 @@ static struct platform_driver imx6_pcie_driver = {
.owner = THIS_MODULE,
.of_match_table = imx6_pcie_of_match,
},
+ .shutdown = imx6_pcie_shutdown,
};
/* Freescale PCIe driver does not allow module unload */
--
2.0.1
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Lucas Stach
2014-09-03 11:16:47 UTC
Permalink
Post by Lucas Stach
This fixes a boot hang observed when the bootloader
already enabled the PCIe link for it's own use. The
fundamental problem is that Freescale forgot to wire
up the core reset, so software doesn't have a sane way
to get the core into a defined state.
According to the DW PCIe core reference manual configuration
of the core may only happen when the LTSSM is disabled, so
this is one of the first things we need to do. Apparently
this isn't safe to do when the LTSSM is in any other state
than "detect" as we observe an instant machine hang when
trying to do so while the link is already up.
As a workaround force LTSSM into detect state right before
hitting the disable switch.
Bjorn,

I would have expected for this to show up in 3.17 as it fixes a real bug
and has been posted some time ago. While you have picked it up into your
pci/host-imx6 branch it doesn't show up in Linus tree nor in any pull
requests AFAICS.
Was it intentional from you to hold this patch or is this just an
oversight?

Regards,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Bjorn Helgaas
2014-09-04 20:24:36 UTC
Permalink
Post by Lucas Stach
Post by Lucas Stach
This fixes a boot hang observed when the bootloader
already enabled the PCIe link for it's own use. The
fundamental problem is that Freescale forgot to wire
up the core reset, so software doesn't have a sane way
to get the core into a defined state.
According to the DW PCIe core reference manual configuration
of the core may only happen when the LTSSM is disabled, so
this is one of the first things we need to do. Apparently
this isn't safe to do when the LTSSM is in any other state
than "detect" as we observe an instant machine hang when
trying to do so while the link is already up.
As a workaround force LTSSM into detect state right before
hitting the disable switch.
Bjorn,
I would have expected for this to show up in 3.17 as it fixes a real bug
and has been posted some time ago. While you have picked it up into your
pci/host-imx6 branch it doesn't show up in Linus tree nor in any pull
requests AFAICS.
Was it intentional from you to hold this patch or is this just an
oversight?
Nope, I just missed it, thanks for the reminder. I put in my
for-linus branch and will ask Linus to pull it after it's been
in -next for a few days.

Bjorn
Fabio Estevam
2014-09-22 13:27:54 UTC
Permalink
Bjorn,
Post by Bjorn Helgaas
Post by Lucas Stach
Bjorn,
I would have expected for this to show up in 3.17 as it fixes a real bug
and has been posted some time ago. While you have picked it up into your
pci/host-imx6 branch it doesn't show up in Linus tree nor in any pull
requests AFAICS.
Was it intentional from you to hold this patch or is this just an
oversight?
Nope, I just missed it, thanks for the reminder. I put in my
for-linus branch and will ask Linus to pull it after it's been
in -next for a few days.
We still don't see this patch applied in linux-next.
Lucas Stach
2014-09-22 13:32:38 UTC
Permalink
Hi Fabio,
Post by Lucas Stach
Bjorn,
Post by Bjorn Helgaas
Post by Lucas Stach
Bjorn,
I would have expected for this to show up in 3.17 as it fixes a real bug
and has been posted some time ago. While you have picked it up into your
pci/host-imx6 branch it doesn't show up in Linus tree nor in any pull
requests AFAICS.
Was it intentional from you to hold this patch or is this just an
oversight?
Nope, I just missed it, thanks for the reminder. I put in my
for-linus branch and will ask Linus to pull it after it's been
in -next for a few days.
We still don't see this patch applied in linux-next.
I see this commit in -next for some time now. It changed subject tho, so
look out for a patch called "PCI: imx6: Put LTSSM in "Detect" state
before disabling it".

Also Bjorn issued a pull request including this patch last Friday, so
things should go ok.

Regards,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Fabio Estevam
2014-09-22 14:08:17 UTC
Permalink
Hi Lucas,
Post by Lucas Stach
I see this commit in -next for some time now. It changed subject tho, so
look out for a patch called "PCI: imx6: Put LTSSM in "Detect" state
before disabling it".
Yes, sorry.

I replied in the wrong thread.

The one I don't see in -next is Tim Harvey's patch:
https://patchwork.ozlabs.org/patch/378077/

Regards,

Fabio Estevam

Loading...