From: Neil Greatorex <neil@fatboyfat.co.uk> To: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Cc: "Thomas Petazzoni" <thomas.petazzoni@free-electrons.com>, "Willy Tarreau" <w@1wt.eu>, "Matthew Minter" <matthew_minter@xyratex.com>, "Gerlando Falauto" <gerlando.falauto@keymile.com>, linux-arm-kernel@lists.infradead.org, "Jason Cooper" <jason@lakedaemon.net>, "Gregory Clément" <gregory.clement@free-electrons.com>, "Ezequiel Garcia" <ezequiel.garcia@free-electrons.com>, "Andrew Lunn" <andrew@lunn.ch>, linux-pci@vger.kernel.org, "Tawfik Bayouk" <tawfik@marvell.com>, "Lior Amsalem" <alior@marvell.com> Subject: Re: Fixing PCIe issues on Armada XP Date: Thu, 10 Apr 2014 22:56:00 +0100 (BST) [thread overview] Message-ID: <alpine.DEB.2.10.1404102206090.28003@vroombuntu> (raw) In-Reply-To: <20140410201201.GA12661@obsidianresearch.com> [-- Attachment #1: Type: TEXT/PLAIN, Size: 2101 bytes --] Jason, On Thu, 10 Apr 2014, Jason Gunthorpe wrote: > Gating the clock without disabling the Phy first does sound like a > bad idea.. > > Neil, does this do anything for you? > > diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.c b/arch/arm/mach-mvebu/mvebu-soc-id.c > index f3b325f..e0a032f 100644 > --- a/arch/arm/mach-mvebu/mvebu-soc-id.c > +++ b/arch/arm/mach-mvebu/mvebu-soc-id.c > @@ -107,7 +107,7 @@ static int __init mvebu_soc_id_init(void) > iounmap(pci_base); > > res_ioremap: > - clk_disable_unprepare(clk); > +// clk_disable_unprepare(clk); > > clk_err: > of_node_put(child); > That patch has fixed it for me. The PCIe card seems to be now be always properly detected. > 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 took your attached patch and extended it a bit to print out how long it took. The delays also need to be much longer for me. I also fixed a small typo you made where the bit wasn't being set again to bring the link back up. I've attached the diff to your patch, as well as the combined patch (hope that makes sense). With the attached patch I get the following output: mirabox ~ # dmesg | grep PCIe0.0 [ 0.135947] mvebu-pcie pcie-controller.3: PCIe0.0: performing link reset [ 0.161989] mvebu-pcie pcie-controller.3: PCIe0.0: link went down after 26 tries [ 0.173984] mvebu-pcie pcie-controller.3: PCIe0.0: link came back up after 12 tries mirabox ~ # lspci 00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) 00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) 01:00.0 Ethernet controller: Intel Corporation I350 Gigabit Network Connection (rev 01) 01:00.1 Ethernet controller: Intel Corporation I350 Gigabit Network Connection (rev 01) 03:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev 02) So that seems to also work. I will leave it to you and Thomas to decide which approach is better! Cheers, Neil [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: Combined patch --] [-- Type: TEXT/x-diff; name=pex-combined.diff, Size: 2236 bytes --] diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c index 487c926..d09a7e5 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. @@ -951,6 +952,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; @@ -1022,6 +1024,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; @@ -1035,7 +1040,45 @@ 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); + + for (tries = 0; + mvebu_pcie_link_up(port) && tries < 100; tries++) + mdelay(1); + + dev_info(&pdev->dev, + "PCIe%d.%d: link went down after %d tries\n", + port->port, port->lane, tries); + + 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++) + mdelay(1); + + dev_info(&pdev->dev, + "PCIe%d.%d: link came back up after %d tries\n", + port->port, port->lane, tries); + } 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); [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: Diff to Jason's patch --] [-- Type: TEXT/x-diff; name=pex-diff-to-jasons-patch.diff, Size: 1179 bytes --] diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c index c902ca0..d09a7e5 100644 --- a/drivers/pci/host/pci-mvebu.c +++ b/drivers/pci/host/pci-mvebu.c @@ -1054,15 +1054,25 @@ static int mvebu_pcie_probe(struct platform_device *pdev) mvebu_writel(port, reg & ~BIT(30), // Conf_TrainingDisable PCIE_CTRL_OFF); - do { - udelay(100); // Guess? - } while (mvebu_pcie_link_up(port)); + + for (tries = 0; + mvebu_pcie_link_up(port) && tries < 100; tries++) + mdelay(1); + + dev_info(&pdev->dev, + "PCIe%d.%d: link went down after %d tries\n", + port->port, port->lane, tries); + mvebu_pcie_set_local_dev_nr(port, 1); - mvebu_writel(port, reg | ~BIT(30), PCIE_CTRL_OFF); + mvebu_writel(port, reg | BIT(30), PCIE_CTRL_OFF); for (tries = 0; !mvebu_pcie_link_up(port) && tries != 100; tries++) - udelay(100); + mdelay(1); + + dev_info(&pdev->dev, + "PCIe%d.%d: link came back up after %d tries\n", + port->port, port->lane, tries); } else { /* We expect the bootloader has setup the port and * waited for the link to go up
WARNING: multiple messages have this Message-ID (diff)
From: neil@fatboyfat.co.uk (Neil Greatorex) To: linux-arm-kernel@lists.infradead.org Subject: Fixing PCIe issues on Armada XP Date: Thu, 10 Apr 2014 22:56:00 +0100 (BST) [thread overview] Message-ID: <alpine.DEB.2.10.1404102206090.28003@vroombuntu> (raw) In-Reply-To: <20140410201201.GA12661@obsidianresearch.com> Jason, On Thu, 10 Apr 2014, Jason Gunthorpe wrote: > Gating the clock without disabling the Phy first does sound like a > bad idea.. > > Neil, does this do anything for you? > > diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.c b/arch/arm/mach-mvebu/mvebu-soc-id.c > index f3b325f..e0a032f 100644 > --- a/arch/arm/mach-mvebu/mvebu-soc-id.c > +++ b/arch/arm/mach-mvebu/mvebu-soc-id.c > @@ -107,7 +107,7 @@ static int __init mvebu_soc_id_init(void) > iounmap(pci_base); > > res_ioremap: > - clk_disable_unprepare(clk); > +// clk_disable_unprepare(clk); > > clk_err: > of_node_put(child); > That patch has fixed it for me. The PCIe card seems to be now be always properly detected. > 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 took your attached patch and extended it a bit to print out how long it took. The delays also need to be much longer for me. I also fixed a small typo you made where the bit wasn't being set again to bring the link back up. I've attached the diff to your patch, as well as the combined patch (hope that makes sense). With the attached patch I get the following output: mirabox ~ # dmesg | grep PCIe0.0 [ 0.135947] mvebu-pcie pcie-controller.3: PCIe0.0: performing link reset [ 0.161989] mvebu-pcie pcie-controller.3: PCIe0.0: link went down after 26 tries [ 0.173984] mvebu-pcie pcie-controller.3: PCIe0.0: link came back up after 12 tries mirabox ~ # lspci 00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) 00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) 01:00.0 Ethernet controller: Intel Corporation I350 Gigabit Network Connection (rev 01) 01:00.1 Ethernet controller: Intel Corporation I350 Gigabit Network Connection (rev 01) 03:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev 02) So that seems to also work. I will leave it to you and Thomas to decide which approach is better! Cheers, Neil -------------- next part -------------- A non-text attachment was scrubbed... Name: pex-combined.diff Type: text/x-diff Size: 2236 bytes Desc: Combined patch URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140410/c4f61db5/attachment.bin> -------------- next part -------------- A non-text attachment was scrubbed... Name: pex-diff-to-jasons-patch.diff Type: text/x-diff Size: 1179 bytes Desc: Diff to Jason's patch URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140410/c4f61db5/attachment-0001.bin>
next prev parent reply other threads:[~2014-04-10 21:56 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-04-10 16:19 Fixing PCIe issues on Armada XP Thomas Petazzoni 2014-04-10 16:19 ` Thomas Petazzoni 2014-04-10 16:57 ` Jason Gunthorpe 2014-04-10 16:57 ` Jason Gunthorpe 2014-04-10 18:01 ` Thomas Petazzoni 2014-04-10 18:01 ` Thomas Petazzoni 2014-04-10 20:12 ` Jason Gunthorpe 2014-04-10 20:12 ` Jason Gunthorpe 2014-04-10 21:04 ` Thomas Petazzoni 2014-04-10 21:04 ` Thomas Petazzoni 2014-04-10 21:56 ` Neil Greatorex [this message] 2014-04-10 21:56 ` Neil Greatorex 2014-04-10 22:06 ` Jason Gunthorpe 2014-04-10 22:06 ` Jason Gunthorpe 2014-04-10 22:15 ` Neil Greatorex 2014-04-10 22:15 ` Neil Greatorex 2014-04-11 10:23 ` Thomas Petazzoni 2014-04-11 10:23 ` Thomas Petazzoni 2014-04-11 16:31 ` Jason Gunthorpe 2014-04-11 16:31 ` Jason Gunthorpe 2014-04-11 17:21 ` Matthew Minter 2014-04-11 17:21 ` Matthew Minter 2014-04-11 17:29 ` Jason Gunthorpe 2014-04-11 17:29 ` Jason Gunthorpe 2014-04-18 13:02 ` Thomas Petazzoni 2014-04-18 13:02 ` Thomas Petazzoni 2014-04-22 17:34 ` Jason Gunthorpe 2014-04-22 17:34 ` Jason Gunthorpe 2014-04-18 12:58 ` Thomas Petazzoni 2014-04-18 12:58 ` Thomas Petazzoni 2014-04-22 17:56 ` Jason Gunthorpe 2014-04-22 17:56 ` Jason Gunthorpe 2014-04-10 17:10 ` Willy Tarreau 2014-04-10 17:10 ` Willy Tarreau 2014-04-10 18:02 ` Thomas Petazzoni 2014-04-10 18:02 ` Thomas Petazzoni 2014-04-10 23:13 ` Willy Tarreau 2014-04-10 23:13 ` Willy Tarreau 2014-04-10 23:40 ` Jason Gunthorpe 2014-04-10 23:40 ` Jason Gunthorpe 2014-04-11 6:23 ` Willy Tarreau 2014-04-11 6:23 ` Willy Tarreau 2014-04-10 18:20 ` Neil Greatorex 2014-04-10 18:20 ` Neil Greatorex 2014-04-10 21:07 ` Thomas Petazzoni 2014-04-10 21:07 ` Thomas Petazzoni 2014-04-11 14:32 ` Thomas Petazzoni 2014-04-11 14:32 ` Thomas Petazzoni 2014-04-11 15:57 ` Neil Greatorex 2014-04-11 15:57 ` Neil Greatorex
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=alpine.DEB.2.10.1404102206090.28003@vroombuntu \ --to=neil@fatboyfat.co.uk \ --cc=alior@marvell.com \ --cc=andrew@lunn.ch \ --cc=ezequiel.garcia@free-electrons.com \ --cc=gerlando.falauto@keymile.com \ --cc=gregory.clement@free-electrons.com \ --cc=jason@lakedaemon.net \ --cc=jgunthorpe@obsidianresearch.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-pci@vger.kernel.org \ --cc=matthew_minter@xyratex.com \ --cc=tawfik@marvell.com \ --cc=thomas.petazzoni@free-electrons.com \ --cc=w@1wt.eu \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.