Fixing PCIe issues on Armada XP

Commit Message

Jason Gunthorpe April 10, 2014, 8:12 p.m. UTC

On Thu, Apr 10, 2014 at 08:01:53PM +0200, Thomas Petazzoni wrote:
> > Can you run Neil's patch and see if your system behaves the same?
> > Specifically that the link eventually goes down after
> > mvebu_pcie_set_local_dev_nr ?
> > 
> > I couldn't find any case where the BDF leaks below the TLP layer, and
> > the spec is very clear that the assigned BDF can change at run time,
> > so I don't have an explanation for why the link reset is happening.
> > Do you have a contact at Marvell that might shed some light on that
> > behavior?
> 
> There was a potential explanation about the mvebu-soc-id driver that
> enables the clock and then disables it, before the pci-mvebu driver.
> This is different that what was happening before, where the pci-mvebu
> driver was the only one to enable the clock, which was already enabled
> by the bootloader. So if the clock takes some time to stabilize, the
> introduction of mvebu-soc-id may lead to problems.

Oh, that certainly sounds like a potential problem. Disabling the
clock will certainly cause 'interesting' effects on the PEX, depending
on exactly what it does it could confuse the link partner (trigger a
timeout based retrain?).

Gating the clock without disabling the Phy first does sound like a
bad idea..

Neil, does this do anything for you?


Jason

Comments

Thomas Petazzoni April 10, 2014, 9:04 p.m. UTC | #1

Dear Jason Gunthorpe,

On Thu, 10 Apr 2014 14:12:01 -0600, Jason Gunthorpe wrote:

> > But I'm not entirely convinced by this, because in my testing, I saw:
> > 
> >  * Enable the clock
> >  * Values in the PCI configuration space are correct (like
> >    vendor/product ID)
> >  * mvebu_pcie_set_local_dev_nr()
> >  * Values in the PCI configuration space are no longer correct, unless
> >    you wait a little bit.
> 
> Were you reading the configuation space through the MMIO mapping or
> through the configuration indirection?

I was simply calling the mvebu_pcie_hw_rd_conf() function, so I guess
it goes through what you call the "configuration indirection".

> In any event, turning on the clock should almost certainly be
> accompanied by a phy reset sequence to get both link ends on the same
> page.
> 
> Attached is a rough, untested patch along those lines.

I'll try tomorrow, if I manage to reproduce the initial bug to start
with.

> > > That does sound like more mbus troubles.
> > 
> > Interestingly, the problem occurred when I was plugging a SATA PCIe
> > card. And regardless of whether the SATA PCIe card is present or not,
> > the MBus mappings for the IGB are exactly the same.
> 
> Maybe something wrong with mbus window index 13?
> 
> Any change if you use other windows?

Don't know, will try tomorrow and report back :-)

Thanks for the suggestions!

Thomas

Patch

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 0d638b7..7b7d19a 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -21,6 +21,7 @@ 
 #include <linux/of_gpio.h>
 #include <linux/of_pci.h>
 #include <linux/of_platform.h>
+#include <linux/clk-provider.h>
 
 /*
  * PCIe unit register offsets.
@@ -973,6 +974,7 @@  static int mvebu_pcie_probe(struct platform_device *pdev)
 	for_each_child_of_node(pdev->dev.of_node, child) {
 		struct mvebu_pcie_port *port = &pcie->ports[i];
 		enum of_gpio_flags flags;
+		bool enabled;
 
 		if (!of_device_is_available(child))
 			continue;
@@ -1044,6 +1046,9 @@  static int mvebu_pcie_probe(struct platform_device *pdev)
 			continue;
 		}
 
+		// Does this work on MVEBU?
+		enabled = __clk_is_enabled(port->clk);
+
 		ret = clk_prepare_enable(port->clk);
 		if (ret)
 			continue;
@@ -1057,7 +1062,35 @@  static int mvebu_pcie_probe(struct platform_device *pdev)
 			continue;
 		}
 
-		mvebu_pcie_set_local_dev_nr(port, 1);
+		if (!enabled) {
+			u32 reg;
+			unsigned int tries;
+
+			/* The clock is being turned on for the first time, do
+			 * a PHY reset
+			 */
+			dev_info(&pdev->dev,
+				 "PCIe%d.%d: performing link reset\n",
+				 port->port, port->lane);
+			reg = mvebu_readl(port, PCIE_CTRL_OFF);
+			mvebu_writel(port,
+				     reg & ~BIT(30), // Conf_TrainingDisable
+				     PCIE_CTRL_OFF);
+			do {
+				udelay(100); // Guess?
+			} while (mvebu_pcie_link_up(port));
+			mvebu_pcie_set_local_dev_nr(port, 1);
+			mvebu_writel(port, reg | ~BIT(30), PCIE_CTRL_OFF);
+
+			for (tries = 0;
+			     !mvebu_pcie_link_up(port) && tries != 100; tries++)
+				udelay(100);
+		} else {
+			/* We expect the bootloader has setup the port and
+			 * waited for the link to go up
+			 */
+			mvebu_pcie_set_local_dev_nr(port, 1);
+		}
 
 		port->dn = child;
 		spin_lock_init(&port->conf_lock);