Fixing PCIe issues on Armada XP

Fixing PCIe issues on Armada XP

[Thomas Petazzoni]

Hello all,

This is an e-mail that attempts to summarize the situation in terms of
Armada XP PCIe issues.

At
https://github.com/MISL-EBU-System-SW/mainline-public/commits/3.14/pci-debug,
I've pushed a branch based on top of v3.14 that contains:

 * 2 backports for the igb driver, needed to get the igb driver to work
   in situations where MSI-X support is not available. These patches
   are already in mainline (post v3.14), and I've sent a mail to the
   maintainers and the netdev@ mailing list to ask for these patches to
   be pushed to 3.14 stable.

 * 3 backports of patches that use the 0xf1000000 internal register
   address for Armada XP development boards. These are needed for me
   because I have a recent Marvell bootloader. You may or may not want
   to apply these patches depending on which Marvell board you're
   using, and which version of the bootloader you have.

 * 3 patches on the irq-armada-370-xp driver, to fix the MSI support.
   One from Neil Greatorex, two from me.

 * 2 patches on the mvebu-mbus driver. One from Jason Gunthorpe which
   adds loud warnings when a non power-of-two window size is requested,
   and one from me to allow the creation of several windows having the
   same target and attribute values, which is needed if we want to
   create multiple windows to describe a single PCI BAR.

 * 2 patches on the pci-mvebu driver. One from Willy Tarreau to fix the
   off by one on the sizes. And another one from me which splits the
   PCI BAR into power-of-two sized chunks, in order to create valid
   MBus windows. I've tested this with my IGB card which needs a 9 MB
   BAR (so 8 MB + 1 MB needed), and I've also faked the code to code to
   simulate a 11.5 MB BAR (so 8 + 2 + 1 + 0.5 MB), and it worked. I
   also checked that if we have an error when creating one of the
   windows, then all the previous windows needed for the current BAR
   are properly removed.

Can you test this stack of patches on your system and configuration, and
let me know if that works for you? Of course, please do not include any
other PCI related fix: the goal is to be aware of *all* the issues, and
fix them in mainline.

Gerlando: I remember you also had a power-of-two related issues, that
you reported a while ago. This patch series should fix it. Would it be
possible for you to test this patch series?

Remaining issues:

 * The link up problem. Unfortunately, I tried to reproduce it today,
   and didn't manage to. It's weird, because I'm sure I was able to
   produce it in the past, but I'm no longer able to, I don't know.
   Therefore, it's not easy for me to work on this topic. Neil, Jason,
   do you think this is a topic you could potentially handle?

 * On my Armada XP DB board, if I plug 5 PCIe cards, the IGB card for
   some reason isn't able to read data from its non-volatile memory. So
   the window points to something, but it doesn't seem to patch what
   the igb driver expects. I've double checked the MBus windows, and
   they look correct. I haven't tested this PCIe configuration with the
   Marvell LSP though, so maybe I'm hitting an unrelated hardware
   problem or something.

Thanks a lot for your feedback and participation around these PCIe
issues!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Fixing PCIe issues on Armada XP

[Jason Gunthorpe]

On Thu, Apr 10, 2014 at 06:19:53PM +0200, Thomas Petazzoni wrote:

> This is an e-mail that attempts to summarize the situation in terms of
> Armada XP PCIe issues.
> 
> At
> https://github.com/MISL-EBU-System-SW/mainline-public/commits/3.14/pci-debug,
> I've pushed a branch based on top of v3.14 that contains:

mvebu_pcie_del_windows / mvebu_pcie_add_windows I wonder if these
functions should be dropped into the mbus driver.. They are pretty
generic.

'bus: mvebu-mbus: Avoid setting an undefined window size' and
'pci: mvebu: fix off-by-one in the computed size of the mbus windows'
need to be swapped in order to maintain bisect-ability.

> Can you test this stack of patches on your system and configuration, and
> let me know if that works for you? 

Continues to work as expected here, and I see the new error message:

mvebu_mbus: cannot add window '4:e8', conflicts with another window
mvebu-pcie pex.1: Could not create MBus window at 0xe0000000, size 0x100000: -22

(this is due to the PEX window being in the DT mbus ranges already)

Tested-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

> 
>  * The link up problem. Unfortunately, I tried to reproduce it today,
>    and didn't manage to. It's weird, because I'm sure I was able to
>    produce it in the past, but I'm no longer able to, I don't know.
>    Therefore, it's not easy for me to work on this topic. Neil, Jason,
>    do you think this is a topic you could potentially handle?

You said you had a system that sometimes required the udelay? 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?

>  * On my Armada XP DB board, if I plug 5 PCIe cards, the IGB card for
>    some reason isn't able to read data from its non-volatile memory. So
>    the window points to something, but it doesn't seem to patch what
>    the igb driver expects. I've double checked the MBus windows, and
>    they look correct. I haven't tested this PCIe configuration with the
>    Marvell LSP though, so maybe I'm hitting an unrelated hardware
>    problem or something.

Certainly troubling.. And the IGB works if it is the only card in the
system?

That does sound like more mbus troubles.

Regrads,
Jason

Fixing PCIe issues on Armada XP

[Willy Tarreau]

Hi Thomas,

On Thu, Apr 10, 2014 at 06:19:53PM +0200, Thomas Petazzoni wrote:
> Hello all,
> 
> This is an e-mail that attempts to summarize the situation in terms of
> Armada XP PCIe issues.
> 
> At
> https://github.com/MISL-EBU-System-SW/mainline-public/commits/3.14/pci-debug,
> I've pushed a branch based on top of v3.14 that contains:

Thanks for putting all this online.

I have a minor comment below :

>  * 2 patches on the pci-mvebu driver. One from Willy Tarreau to fix the
>    off by one on the sizes. And another one from me which splits the
>    PCI BAR into power-of-two sized chunks, in order to create valid
>    MBus windows.

As suggested by Jason, this one should be merged before his that's just
before, to ensure that it will not cause a regression.

> I've tested this with my IGB card which needs a 9 MB
>    BAR (so 8 MB + 1 MB needed), and I've also faked the code to code to
>    simulate a 11.5 MB BAR (so 8 + 2 + 1 + 0.5 MB), and it worked. I
>    also checked that if we have an error when creating one of the
>    windows, then all the previous windows needed for the current BAR
>    are properly removed.

Really cool, I'm going to test that on a few PCIe cards and will report
the results here. How can we check the number of mbus windows in use ?

Thanks,
Willy

Fixing PCIe issues on Armada XP

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Thu, 10 Apr 2014 10:57:33 -0600, Jason Gunthorpe wrote:

> > At
> > https://github.com/MISL-EBU-System-SW/mainline-public/commits/3.14/pci-debug,
> > I've pushed a branch based on top of v3.14 that contains:
> 
> mvebu_pcie_del_windows / mvebu_pcie_add_windows I wonder if these
> functions should be dropped into the mbus driver.. They are pretty
> generic.

Yes. I must say I hesitated quite a bit of time between having them in
pci-mvebu or in mvebu-mbus. I settled on pci-mvebu because PCI is so
far the only user of this functionality. But this is not a strong
opinion.

> 'bus: mvebu-mbus: Avoid setting an undefined window size' and
> 'pci: mvebu: fix off-by-one in the computed size of the mbus windows'
> need to be swapped in order to maintain bisect-ability.

They could, indeed, but on the other hand, PCI and bus are handled by
different maintainers, so they will flow through distinct branches, so
I'm not sure it's going to be easy to guarantee the bisectability here.

Unless we get the Acks from both maintainers, so that Jason Cooper can
carry the patches and maintain the right order.

I'll reorder them when posting the patch series anyway.

> > Can you test this stack of patches on your system and configuration, and
> > let me know if that works for you? 
> 
> Continues to work as expected here, and I see the new error message:
> 
> mvebu_mbus: cannot add window '4:e8', conflicts with another window
> mvebu-pcie pex.1: Could not create MBus window at 0xe0000000, size 0x100000: -22
> 
> (this is due to the PEX window being in the DT mbus ranges already)

I'm not sure to understand here: you have hardcoded in your DT the
necessary PEX window? If so, then yes, it's expected.

> Tested-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Thanks!

> >  * The link up problem. Unfortunately, I tried to reproduce it today,
> >    and didn't manage to. It's weird, because I'm sure I was able to
> >    produce it in the past, but I'm no longer able to, I don't know.
> >    Therefore, it's not easy for me to work on this topic. Neil, Jason,
> >    do you think this is a topic you could potentially handle?
> 
> You said you had a system that sometimes required the udelay?

Yes, and I tried to reproduce it today, and I didn't manage to
reproduce the problem, even on this platform, in the same conditions.
I'll try again tomorrow.

> 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.

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.

> >  * On my Armada XP DB board, if I plug 5 PCIe cards, the IGB card for
> >    some reason isn't able to read data from its non-volatile memory. So
> >    the window points to something, but it doesn't seem to patch what
> >    the igb driver expects. I've double checked the MBus windows, and
> >    they look correct. I haven't tested this PCIe configuration with the
> >    Marvell LSP though, so maybe I'm hitting an unrelated hardware
> >    problem or something.
> 
> Certainly troubling.. And the IGB works if it is the only card in the
> system?

Yes, the IGB card works fine when plugged as the only card. It also
works when plugged with 3 other PCIe cards. But when I plug the five
PCIe cards I have, the IGB stops working.

However, note that some interfaces are x1, some other x4, and maybe
there was a mismatch/incompatibility with the PCIe cards I was plugging
in the board.

> 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.

Here is a dump of the MBus windows with 4 PCIe cards (including the IGB
one) :

[00] 00000000ffe30000 - 00000000ffe40000 : 0004:00f0 (remap 0000000000030000)
[01] disabled
[02] disabled
[03] disabled
[04] disabled
[05] disabled
[06] disabled
[07] disabled
[08] 00000000fff00000 - 0000000100000000 : 0001:001d -> bootrom
[09] 00000000f0000000 - 00000000f1000000 : 0001:002f -> NOR flash
[10] 00000000f8000000 - 00000000f8100000 : 0004:00e8 -> some other PCIe card
[11] 00000000f8800000 - 00000000f9000000 : 0004:00f8 -> this is the IGB
[12] 00000000f9000000 - 00000000f9100000 : 0004:00f8 -> this is the IGB
[13] disabled
[14] disabled
[15] disabled
[16] disabled
[17] disabled
[18] disabled
[19] disabled

And now, with the SATA card plugged in:

[00] 00000000ffe40000 - 00000000ffe50000 : 0008:00f0 (remap 0000000000040000)
[01] 00000000ffe30000 - 00000000ffe40000 : 0004:00f0 (remap 0000000000030000)
[02] disabled
[03] disabled
[04] disabled
[05] disabled
[06] disabled
[07] disabled
[08] 00000000fff00000 - 0000000100000000 : 0001:001d	=> bootrom
[09] 00000000f0000000 - 00000000f1000000 : 0001:002f	=> NOR flash
[10] 00000000f8000000 - 00000000f8100000 : 0004:00e8	=> port 0.0 mem
[11] 00000000f8400000 - 00000000f8600000 : 0008:00f8	=> port 3.0 mem (SATA)
[12] 00000000f8800000 - 00000000f9000000 : 0004:00f8	=> port 2.0 mem (igb)
[13] 00000000f9000000 - 00000000f9100000 : 0004:00f8	=> port 2.0 mem (igb)
[14] disabled
[15] disabled
[16] disabled
[17] disabled
[18] disabled
[19] disabled

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Fixing PCIe issues on Armada XP

[Thomas Petazzoni]

Dear Willy Tarreau,

On Thu, 10 Apr 2014 19:10:00 +0200, Willy Tarreau wrote:

> Thanks for putting all this online.
> 
> I have a minor comment below :
> 
> >  * 2 patches on the pci-mvebu driver. One from Willy Tarreau to fix the
> >    off by one on the sizes. And another one from me which splits the
> >    PCI BAR into power-of-two sized chunks, in order to create valid
> >    MBus windows.
> 
> As suggested by Jason, this one should be merged before his that's just
> before, to ensure that it will not cause a regression.

Ok, will change this, thanks.

> > I've tested this with my IGB card which needs a 9 MB
> >    BAR (so 8 MB + 1 MB needed), and I've also faked the code to code to
> >    simulate a 11.5 MB BAR (so 8 + 2 + 1 + 0.5 MB), and it worked. I
> >    also checked that if we have an error when creating one of the
> >    windows, then all the previous windows needed for the current BAR
> >    are properly removed.
> 
> Really cool, I'm going to test that on a few PCIe cards and will report
> the results here. How can we check the number of mbus windows in use ?

# cat /sys/kernel/debug/mvebu-mbus/devices

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Fixing PCIe issues on Armada XP

[Neil Greatorex]

Thomas,

On Thu, 10 Apr 2014, Thomas Petazzoni wrote:

> Can you test this stack of patches on your system and configuration, and
> let me know if that works for you? Of course, please do not include any
> other PCI related fix: the goal is to be aware of *all* the issues, and
> fix them in mainline.

I have tested this branch on my Mirabox. I still get the link up problem 
but if I work around that (see below) the igb driver works flawlessly.

Tested-by: Neil Greatorex <neil@fatboyfat.co.uk>

> Remaining issues:
>
> * The link up problem. Unfortunately, I tried to reproduce it today,
>   and didn't manage to. It's weird, because I'm sure I was able to
>   produce it in the past, but I'm no longer able to, I don't know.
>   Therefore, it's not easy for me to work on this topic. Neil, Jason,
>   do you think this is a topic you could potentially handle?

Do you have earlyprintk enabled? I've found that if I have earlyprintk in 
my bootargs / command line then I don't get the issue. I assume that is 
because of timing, but it could be something else?

Cheers,
Neil

Fixing PCIe issues on Armada XP

[Jason Gunthorpe]

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?

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);

> 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?

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.

> > 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?

--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -299,7 +299,7 @@ static int mvebu_mbus_alloc_window(struct mvebu_mbus_state *mbus,
        int win;
 
        if (remap == MVEBU_MBUS_NO_REMAP) {
-               for (win = mbus->soc->num_remappable_wins;
+               for (win = 0;
                     win < mbus->soc->num_wins; win++)
                        if (mvebu_mbus_window_is_free(mbus, win))
                                return mvebu_mbus_setup_window(mbus, win, base,

Jason
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pex-reset.diff
Type: text/x-diff
Size: 1897 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140410/e9227692/attachment.bin>

Fixing PCIe issues on Armada XP

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Fixing PCIe issues on Armada XP

[Thomas Petazzoni]

Dear Neil Greatorex,

On Thu, 10 Apr 2014 19:20:37 +0100 (BST), Neil Greatorex wrote:

> I have tested this branch on my Mirabox. I still get the link up problem 
> but if I work around that (see below) the igb driver works flawlessly.
> 
> Tested-by: Neil Greatorex <neil@fatboyfat.co.uk>

Good, thanks, we're making progress!

> > Remaining issues:
> >
> > * The link up problem. Unfortunately, I tried to reproduce it today,
> >   and didn't manage to. It's weird, because I'm sure I was able to
> >   produce it in the past, but I'm no longer able to, I don't know.
> >   Therefore, it's not easy for me to work on this topic. Neil, Jason,
> >   do you think this is a topic you could potentially handle?
> 
> Do you have earlyprintk enabled? I've found that if I have earlyprintk in 
> my bootargs / command line then I don't get the issue. I assume that is 
> because of timing, but it could be something else?

I have indeed disabled earlyprintk. Initially by removing it from the
command line, and then even by disabling CONFIG_DEBUG_LL.

In fact the original bug report I had came from my colleague Gregory
Clement, who precisely reported to me that a PCIe card was properly
detected on his Armada 385 board when earlyprintk was enabled, but the
PCIe card was not detected when earlyprintk was disabled. At the time,
I was able to reproduce the problem as well on my Armada 385, and
debugged it to find that the mvebu_pcie_set_local_dev_nr().

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Fixing PCIe issues on Armada XP

[Neil Greatorex]

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>

Fixing PCIe issues on Armada XP

[Jason Gunthorpe]

On Thu, Apr 10, 2014 at 10:56:00PM +0100, Neil Greatorex wrote:

> >diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.c b/arch/arm/mach-mvebu/mvebu-soc-id.c
> >index f3b325f..e0a032f 100644
> >+++ 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.

Okay, that makes lots of sense to me at least.

Gregory: I have this vauge recollection it was discussed when you
wrote the mvebu_soc_id_init patch to use a __clk_is_enabled but got
shot down? Clearly this needs to be fixed.

> >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).

Just to be clear you tried this alone without the above?

Thanks for fixing the patch, I think it also confirms the theory.

IMHO, both approaches are required.

The first prevents messing up the PEX as was left by the bootloader

The second allows the driver to startup a PEX that wasn't enabled by
the bootloader, and recover from clock gating in the kernel (eg the
modular case)

Both seem valuable..

Ideally I'd like to see the clk driver turn off the PEX PHY when it
gates the clock, but I have no great idea how to accomplish that sort
of cross register space adventure...

Thomas, if we can figure out how to properly do __clk_is_enabled I
can probably send a proper patch?

Thanks,
Jason

Fixing PCIe issues on Armada XP

[Neil Greatorex]

Jason,

On Thu, 10 Apr 2014, Jason Gunthorpe wrote:

>> 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).
>
> Just to be clear you tried this alone without the above?

Yes I did.

>
> Thanks for fixing the patch, I think it also confirms the theory.
>

No problem!

> IMHO, both approaches are required.
>
> The first prevents messing up the PEX as was left by the bootloader
>
> The second allows the driver to startup a PEX that wasn't enabled by
> the bootloader, and recover from clock gating in the kernel (eg the
> modular case)
>
> Both seem valuable..
>
> Ideally I'd like to see the clk driver turn off the PEX PHY when it
> gates the clock, but I have no great idea how to accomplish that sort
> of cross register space adventure...
>
> Thomas, if we can figure out how to properly do __clk_is_enabled I
> can probably send a proper patch?

Let me know when you do and I will test it and add my Tested-by.

>
> Thanks,
> Jason
>

Cheers,
Neil

Fixing PCIe issues on Armada XP

[Willy Tarreau]

Hi Thomas,

On Thu, Apr 10, 2014 at 08:02:22PM +0200, Thomas Petazzoni wrote:
> > Really cool, I'm going to test that on a few PCIe cards and will report
> > the results here. How can we check the number of mbus windows in use ?
> 
> # cat /sys/kernel/debug/mvebu-mbus/devices

Thanks, so here we go :

XP-GP with igb, works perfectly :

root at xpgp:~# dmesg|grep igb
igb: Intel(R) Gigabit Ethernet Network Driver - version 5.0.5-k
igb: Copyright (c) 2007-2013 Intel Corporation.
igb 0000:02:00.0: added PHC on eth4
igb 0000:02:00.0: Intel(R) Gigabit Ethernet Network Connection
igb 0000:02:00.0: eth4: (PCIe:5.0Gb/s:Width x1) 00:30:18:a6:6c:6a
igb 0000:02:00.0: eth4: PBA No: FFFFFF-0FF
igb 0000:02:00.0: Using MSI interrupts. 1 rx queue(s), 1 tx queue(s)
igb 0000:02:00.1: added PHC on eth5
igb 0000:02:00.1: Intel(R) Gigabit Ethernet Network Connection
igb 0000:02:00.1: eth5: (PCIe:5.0Gb/s:Width x1) 00:30:18:a6:6c:6b
igb 0000:02:00.1: eth5: PBA No: FFFFFF-0FF
igb 0000:02:00.1: Using MSI interrupts. 1 rx queue(s), 1 tx queue(s)

root at xpgp:~# lspci -v -s 02:00 | egrep -i '^0|Memory'
02:00.0 Ethernet controller: Intel Corporation Device 1521 (rev 01)
        Memory at e0000000 (32-bit, non-prefetchable) [size=512K]
        Memory at e0200000 (32-bit, non-prefetchable) [size=16K]
02:00.1 Ethernet controller: Intel Corporation Device 1521 (rev 01)
        Memory at e0100000 (32-bit, non-prefetchable) [size=512K]
        Memory at e0204000 (32-bit, non-prefetchable) [size=16K]

root at xpgp:~# grep -v disabled /sys/kernel/debug/mvebu-mbus/devices 
[00] 00000000e8010000 - 00000000e8020000 : 0004:00f0 (remap 0000000000010000)
[08] 00000000fff00000 - 0000000100000000 : 0001:001d
[09] 00000000f0000000 - 00000000f1000000 : 0001:002f
[10] 00000000e0000000 - 00000000e0200000 : 0004:00f8
[11] 00000000e0200000 - 00000000e0300000 : 0004:00f8

So the areas are well covered, though #11 seems larger than needed
but I seem to remember that they're all rounded up by 1 MB anyway,
then if so, that's OK.

Now with igb + myricom :

root at xpgp:~# lspci -v -s 02:00 | egrep -i '^0|Memory'
02:00.0 Ethernet controller: Intel Corporation Device 1521 (rev 01)
        Memory at e1800000 (32-bit, non-prefetchable) [disabled] [size=512K]
        Memory at e1a00000 (32-bit, non-prefetchable) [disabled] [size=16K]
02:00.1 Ethernet controller: Intel Corporation Device 1521 (rev 01)
        Memory at e1900000 (32-bit, non-prefetchable) [disabled] [size=512K]
        Memory at e1a04000 (32-bit, non-prefetchable) [disabled] [size=16K]

root at xpgp:~# lspci -v -s 03:00 | egrep -i '^0|Memory'
03:00.0 Ethernet controller: MYRICOM Inc. Myri-10G Dual-Protocol NIC
        Memory at e0000000 (64-bit, prefetchable) [size=16M]
        Memory at e1000000 (64-bit, non-prefetchable) [size=1M]

root at xpgp:~# modprobe igb
igb: Intel(R) Gigabit Ethernet Network Driver - version 5.0.5-k
igb: Copyright (c) 2007-2013 Intel Corporation.
PCI: enabling device 0000:00:09.0 (0140 -> 0143)
PCI: enabling device 0000:02:00.0 (0140 -> 0142)
igb 0000:02:00.0: added PHC on eth4
igb 0000:02:00.0: Intel(R) Gigabit Ethernet Network Connection
igb 0000:02:00.0: eth4: (PCIe:5.0Gb/s:Width x1) 00:30:18:a6:6c:6a
igb 0000:02:00.0: eth4: PBA No: FFFFFF-0FF
igb 0000:02:00.0: Using MSI interrupts. 1 rx queue(s), 1 tx queue(s)
PCI: enabling device 0000:02:00.1 (0140 -> 0142)
igb 0000:02:00.1: added PHC on eth5
igb 0000:02:00.1: Intel(R) Gigabit Ethernet Network Connection
igb 0000:02:00.1: eth5: (PCIe:5.0Gb/s:Width x1) 00:30:18:a6:6c:6b
igb 0000:02:00.1: eth5: PBA No: FFFFFF-0FF
igb 0000:02:00.1: Using MSI interrupts. 1 rx queue(s), 1 tx queue(s)

root at xpgp:~# modprobe myri10ge
myri10ge: Version 1.5.3-1.534
PCI: enabling device 0000:00:0a.0 (0140 -> 0143)
myri10ge 0000:03:00.0: PCIE x4 Link
myri10ge 0000:03:00.0: Direct firmware load failed with error -2
myri10ge 0000:03:00.0: Falling back to user helper
myri10ge 0000:03:00.0: Unable to load myri10ge_eth_z8e.dat firmware image via hotplug
myri10ge 0000:03:00.0: hotplug firmware loading failed
myri10ge 0000:03:00.0: Successfully adopted running firmware
myri10ge 0000:03:00.0: Using firmware currently running on NIC.  For optimal
myri10ge 0000:03:00.0: performance consider loading optimized firmware
myri10ge 0000:03:00.0: via hotplug
myri10ge 0000:03:00.0: MSI IRQ 113, tx bndry 2048, fw adopted, WC Disabled

root at xpgp:~# grep -v disabled /sys/kernel/debug/mvebu-mbus/devices
[00] 00000000e8010000 - 00000000e8020000 : 0004:00f0 (remap 0000000000010000)
[08] 00000000fff00000 - 0000000100000000 : 0001:001d
[09] 00000000f0000000 - 00000000f1000000 : 0001:002f
[10] 00000000e1800000 - 00000000e1a00000 : 0004:00f8
[11] 00000000e1a00000 - 00000000e1b00000 : 0004:00f8
[12] 00000000e0000000 - 00000000e1000000 : 0008:00f8
[13] 00000000e1000000 - 00000000e1800000 : 0008:00f8

I noticed above that both igb ports share the same window #11. So I
tried to rmmod igb, remove both PCI devices, check mbus again (which
did not change), rescan PCI and modprobe igb again, and everything is
still operational with the same windows. I don't know if it is normal
that they're not unregistered when the device goes away (maybe there's
no refcount) ?

If we have to keep them forever, then maybe a further improvement
will consist in merging adjacent windows which sum up as a power of
two (eg: #10 and #11 may be merged).

I tried to add a 3rd NIC in the mix (broadcom tg3), which caused the
myri10ge to fail to load for an obscure reason after loading igb
properly :

root at xpgp:~# dmesg
tg3.c:v3.134 (Sep 16, 2013)
PCI: enabling device 0000:00:01.0 (0140 -> 0143)
tg3 0000:01:00.0 eth4: Tigon3 [partno(BCM95721A211) rev 4001] (PCI Express) MAC address 00:07:11:04:3e:e6
tg3 0000:01:00.0 eth4: attached PHY is 5750 (10/100/1000Base-T Ethernet) (WireSpeed[1], EEE[0])
tg3 0000:01:00.0 eth4: RXcsums[1] LinkChgREG[0] MIirq[0] ASF[0] TSOcap[1]
tg3 0000:01:00.0 eth4: dma_rwctrl[76180000] dma_mask[64-bit]
igb: Intel(R) Gigabit Ethernet Network Driver - version 5.0.5-k
igb: Copyright (c) 2007-2013 Intel Corporation.
PCI: enabling device 0000:00:09.0 (0140 -> 0143)
PCI: enabling device 0000:02:00.0 (0140 -> 0142)
igb 0000:02:00.0: added PHC on eth5
igb 0000:02:00.0: Intel(R) Gigabit Ethernet Network Connection
igb 0000:02:00.0: eth5: (PCIe:5.0Gb/s:Width x1) 00:30:18:a6:6c:6a
igb 0000:02:00.0: eth5: PBA No: FFFFFF-0FF
igb 0000:02:00.0: Using MSI interrupts. 1 rx queue(s), 1 tx queue(s)
PCI: enabling device 0000:02:00.1 (0140 -> 0142)
igb 0000:02:00.1: added PHC on eth6
igb 0000:02:00.1: Intel(R) Gigabit Ethernet Network Connection
igb 0000:02:00.1: eth6: (PCIe:5.0Gb/s:Width x1) 00:30:18:a6:6c:6b
igb 0000:02:00.1: eth6: PBA No: FFFFFF-0FF
igb 0000:02:00.1: Using MSI interrupts. 1 rx queue(s), 1 tx queue(s)
myri10ge: Version 1.5.3-1.534
PCI: enabling device 0000:00:0a.0 (0140 -> 0143)
myri10ge 0000:03:00.0: invalid sram_size -1B or board span 16777216B
root at xpgp:~# 

root at xpgp:~# lspci -v -s 01:00 | egrep -i '^0| at '
01:00.0 Ethernet controller: Broadcom Corporation NetXtreme BCM5721 Gigabit Ethernet PCI Express (rev 01)
        Memory at e1800000 (64-bit, non-prefetchable) [size=64K]
        Expansion ROM at e1810000 [disabled] [size=64K]

root at xpgp:~# lspci -v -s 02:00 | egrep -i '^0| at '
02:00.0 Ethernet controller: Intel Corporation Device 1521 (rev 01)
        Memory at e1a00000 (32-bit, non-prefetchable) [size=512K]
        I/O ports at 10000 [disabled] [size=32]
        Memory at e1c00000 (32-bit, non-prefetchable) [size=16K]
        [virtual] Expansion ROM at e1a80000 [disabled] [size=512K]
02:00.1 Ethernet controller: Intel Corporation Device 1521 (rev 01)
        Memory at e1b00000 (32-bit, non-prefetchable) [size=512K]
        I/O ports at 10020 [disabled] [size=32]
        Memory at e1c04000 (32-bit, non-prefetchable) [size=16K]
        [virtual] Expansion ROM at e1b80000 [disabled] [size=512K]

root at xpgp:~# lspci -v -s 03:00 | egrep -i '^0| at '
03:00.0 Ethernet controller: MYRICOM Inc. Myri-10G Dual-Protocol NIC
        Memory at e0000000 (64-bit, prefetchable) [size=16M]
        Memory at e1000000 (64-bit, non-prefetchable) [size=1M]
        [virtual] Expansion ROM at e1100000 [disabled] [size=512K]
root at xpgp:~# 

root at xpgp:~# grep -v disabled /sys/kernel/debug/mvebu-mbus/devices
[00] 00000000e8010000 - 00000000e8020000 : 0004:00f0 (remap 0000000000010000)
[08] 00000000fff00000 - 0000000100000000 : 0001:001d
[09] 00000000f0000000 - 00000000f1000000 : 0001:002f
[10] 00000000e1800000 - 00000000e1900000 : 0004:00e8
[11] 00000000e1a00000 - 00000000e1c00000 : 0004:00f8
[12] 00000000e1c00000 - 00000000e1d00000 : 0004:00f8
[13] 00000000e0000000 - 00000000e1000000 : 0008:00f8
[14] 00000000e1000000 - 00000000e1800000 : 0008:00f8

At least nothing seems wrong anywhere, so for now we should probably
ignore it, unless someone has a good idea about something to look at.

Now I'm using a Realtek instead of TG3, so I have this :

root at xpgp:~# lspci -v -s 01:00 | egrep -i '^0| at '
01:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168B PCI Express Gigabit Ethernet controller (rev 02)
        I/O ports at 10000 [disabled] [size=256]
        Memory at e1820000 (64-bit, non-prefetchable) [size=4K]
        Memory at e1800000 (64-bit, prefetchable) [size=64K]
        Expansion ROM at e1810000 [size=64K]

root at xpgp:~# lspci -v -s 02:00 | egrep -i '^0| at '
02:00.0 Ethernet controller: Intel Corporation Device 1521 (rev 01)
        Memory at e1a00000 (32-bit, non-prefetchable) [disabled] [size=512K]
        I/O ports at 20000 [disabled] [size=32]
        Memory at e1c00000 (32-bit, non-prefetchable) [disabled] [size=16K]
        [virtual] Expansion ROM at e1a80000 [disabled] [size=512K]
02:00.1 Ethernet controller: Intel Corporation Device 1521 (rev 01)
        Memory at e1b00000 (32-bit, non-prefetchable) [disabled] [size=512K]
        I/O ports at 20020 [disabled] [size=32]
        Memory at e1c04000 (32-bit, non-prefetchable) [disabled] [size=16K]
        [virtual] Expansion ROM at e1b80000 [disabled] [size=512K]

root at xpgp:~# lspci -v -s 03:00 | egrep -i '^0| at '
03:00.0 Ethernet controller: MYRICOM Inc. Myri-10G Dual-Protocol NIC
        Memory at e0000000 (64-bit, prefetchable) [size=16M]
        Memory at e1000000 (64-bit, non-prefetchable) [size=1M]
        [virtual] Expansion ROM at e1100000 [disabled] [size=512K]

I get similar results :

root at xpgp:~# dmesg
r8169 Gigabit Ethernet driver 2.3LK-NAPI loaded
PCI: enabling device 0000:00:01.0 (0140 -> 0143)
PCI: enabling device 0000:01:00.0 (0146 -> 0147)
r8169 0000:01:00.0 eth4: RTL8168c/8111c at 0xf0346000, 00:e0:4c:81:20:79, XID 1c4000c0 IRQ 119
r8169 0000:01:00.0 eth4: jumbo features [frames: 6128 bytes, tx checksumming: ko]
igb: Intel(R) Gigabit Ethernet Network Driver - version 5.0.5-k
igb: Copyright (c) 2007-2013 Intel Corporation.
PCI: enabling device 0000:00:09.0 (0140 -> 0143)
PCI: enabling device 0000:02:00.0 (0140 -> 0142)
igb 0000:02:00.0: added PHC on eth5
igb 0000:02:00.0: Intel(R) Gigabit Ethernet Network Connection
igb 0000:02:00.0: eth5: (PCIe:5.0Gb/s:Width x1) 00:30:18:a6:6c:6a
igb 0000:02:00.0: eth5: PBA No: FFFFFF-0FF
igb 0000:02:00.0: Using MSI interrupts. 1 rx queue(s), 1 tx queue(s)
PCI: enabling device 0000:02:00.1 (0140 -> 0142)
igb 0000:02:00.1: added PHC on eth6
igb 0000:02:00.1: Intel(R) Gigabit Ethernet Network Connection
igb 0000:02:00.1: eth6: (PCIe:5.0Gb/s:Width x1) 00:30:18:a6:6c:6b
igb 0000:02:00.1: eth6: PBA No: FFFFFF-0FF
igb 0000:02:00.1: Using MSI interrupts. 1 rx queue(s), 1 tx queue(s)
myri10ge: Version 1.5.3-1.534
PCI: enabling device 0000:00:0a.0 (0140 -> 0143)
myri10ge 0000:03:00.0: invalid sram_size -1B or board span 16777216B

root at xpgp:~# grep -v disabled /sys/kernel/debug/mvebu-mbus/devices
[00] 00000000e8010000 - 00000000e8020000 : 0004:00e0 (remap 0000000000010000)
[01] 00000000e8020000 - 00000000e8030000 : 0004:00f0 (remap 0000000000020000)
[08] 00000000fff00000 - 0000000100000000 : 0001:001d
[09] 00000000f0000000 - 00000000f1000000 : 0001:002f
[10] 00000000e1800000 - 00000000e1900000 : 0004:00e8
[11] 00000000e1a00000 - 00000000e1c00000 : 0004:00f8
[12] 00000000e1c00000 - 00000000e1d00000 : 0004:00f8
[13] 00000000e0000000 - 00000000e1000000 : 0008:00f8
[14] 00000000e1000000 - 00000000e1800000 : 0008:00f8
root at xpgp:~# 

Ah, interestingly if I load the NICs in the opposite order, they all load
properly (myri10ge, igb, r8169) :

root at xpgp:~# dmesg
myri10ge: Version 1.5.3-1.534
PCI: enabling device 0000:00:0a.0 (0140 -> 0143)
myri10ge 0000:03:00.0: PCIE x4 Link
myri10ge 0000:03:00.0: Direct firmware load failed with error -2
myri10ge 0000:03:00.0: Falling back to user helper
myri10ge 0000:03:00.0: Unable to load myri10ge_eth_z8e.dat firmware image via hotplug
myri10ge 0000:03:00.0: hotplug firmware loading failed
myri10ge 0000:03:00.0: Successfully adopted running firmware
myri10ge 0000:03:00.0: Using firmware currently running on NIC.  For optimal
myri10ge 0000:03:00.0: performance consider loading optimized firmware
myri10ge 0000:03:00.0: via hotplug
myri10ge 0000:03:00.0: MSI IRQ 114, tx bndry 2048, fw adopted, WC Disabled
igb: Intel(R) Gigabit Ethernet Network Driver - version 5.0.5-k
igb: Copyright (c) 2007-2013 Intel Corporation.
PCI: enabling device 0000:00:09.0 (0140 -> 0143)
PCI: enabling device 0000:02:00.0 (0140 -> 0142)
igb 0000:02:00.0: added PHC on eth5
igb 0000:02:00.0: Intel(R) Gigabit Ethernet Network Connection
igb 0000:02:00.0: eth5: (PCIe:5.0Gb/s:Width x1) 00:30:18:a6:6c:6a
igb 0000:02:00.0: eth5: PBA No: FFFFFF-0FF
igb 0000:02:00.0: Using MSI interrupts. 1 rx queue(s), 1 tx queue(s)
PCI: enabling device 0000:02:00.1 (0140 -> 0142)
igb 0000:02:00.1: added PHC on eth6
igb 0000:02:00.1: Intel(R) Gigabit Ethernet Network Connection
igb 0000:02:00.1: eth6: (PCIe:5.0Gb/s:Width x1) 00:30:18:a6:6c:6b
igb 0000:02:00.1: eth6: PBA No: FFFFFF-0FF
igb 0000:02:00.1: Using MSI interrupts. 1 rx queue(s), 1 tx queue(s)
r8169 Gigabit Ethernet driver 2.3LK-NAPI loaded
PCI: enabling device 0000:00:01.0 (0140 -> 0143)
PCI: enabling device 0000:01:00.0 (0146 -> 0147)
r8169 0000:01:00.0 eth7: RTL8168c/8111c at 0xf037e000, 00:e0:4c:81:20:79, XID 1c4000c0 IRQ 121
r8169 0000:01:00.0 eth7: jumbo features [frames: 6128 bytes, tx checksumming: ko]

root at xpgp:~# grep -v disabled /sys/kernel/debug/mvebu-mbus/devices
[00] 00000000e8020000 - 00000000e8030000 : 0004:00f0 (remap 0000000000020000)
[01] 00000000e8010000 - 00000000e8020000 : 0004:00e0 (remap 0000000000010000)
[08] 00000000fff00000 - 0000000100000000 : 0001:001d
[09] 00000000f0000000 - 00000000f1000000 : 0001:002f
[10] 00000000e0000000 - 00000000e1000000 : 0008:00f8
[11] 00000000e1000000 - 00000000e1800000 : 0008:00f8
[12] 00000000e1a00000 - 00000000e1c00000 : 0004:00f8
[13] 00000000e1c00000 - 00000000e1d00000 : 0004:00f8
[14] 00000000e1800000 - 00000000e1900000 : 0004:00e8

On the Mirabox, I don't see the igb NIC on lspci, but it's late and
I start to think slowly so I'll have to dig this out tomorrow. Hmmm
I'm seeing it after a rescan, it looks like the same issue that Neil
initially reported about the link up delay.

All the devices are detected now (including the USB3 controller) :

root at mirabox:~# lspci -v  | egrep -i '^0| at '
00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) (prog-if 00 [Normal decode])
00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) (prog-if 00 [Normal decode])
01:00.0 Ethernet controller: Intel Corporation Device 1521 (rev 01)
        Memory at e0200000 (32-bit, non-prefetchable) [size=512K]
        I/O ports at 10000 [disabled] [size=32]
        Memory at e0400000 (32-bit, non-prefetchable) [size=16K]
        [virtual] Expansion ROM at e0280000 [disabled] [size=512K]
01:00.1 Ethernet controller: Intel Corporation Device 1521 (rev 01)
        Memory at e0300000 (32-bit, non-prefetchable) [size=512K]
        I/O ports at 10020 [disabled] [size=32]
        Memory at e0404000 (32-bit, non-prefetchable) [size=16K]
        [virtual] Expansion ROM at e0380000 [disabled] [size=512K]
02:00.0 USB Controller: Device 1b73:1009 (rev 02) (prog-if 30)
        Memory at e0000000 (64-bit, non-prefetchable) [size=64K]
        Memory at e0010000 (64-bit, non-prefetchable) [size=4K]
        Memory at e0011000 (64-bit, non-prefetchable) [size=4K]

The nic properly loads :

root at mirabox:~# dmesg
igb: Intel(R) Gigabit Ethernet Network Driver - version 5.0.5-k
igb: Copyright (c) 2007-2013 Intel Corporation.
PCI: enabling device 0000:00:01.0 (0140 -> 0143)
PCI: enabling device 0000:01:00.0 (0000 -> 0002)
igb 0000:01:00.0: added PHC on eth2
igb 0000:01:00.0: Intel(R) Gigabit Ethernet Network Connection
igb 0000:01:00.0: eth2: (PCIe:2.5Gb/s:Width x1) 00:30:18:a6:6c:6a
igb 0000:01:00.0: eth2: PBA No: FFFFFF-0FF
igb 0000:01:00.0: Using MSI interrupts. 1 rx queue(s), 1 tx queue(s)
PCI: enabling device 0000:01:00.1 (0000 -> 0002)
igb 0000:01:00.1: added PHC on eth3
igb 0000:01:00.1: Intel(R) Gigabit Ethernet Network Connection
igb 0000:01:00.1: eth3: (PCIe:2.5Gb/s:Width x1) 00:30:18:a6:6c:6b
igb 0000:01:00.1: eth3: PBA No: FFFFFF-0FF
igb 0000:01:00.1: Using MSI interrupts. 1 rx queue(s), 1 tx queue(s)
root at mirabox:~# 

And the mbus windows match expectations :

root at mirabox:~# grep -v disabled /sys/kernel/debug/mvebu-mbus/devices
[00] 00000000e8010000 - 00000000e8020000 : 0004:00e0 (remap 0000000000010000)
[08] 00000000fff00000 - 0000000100000000 : 0001:00e0
[09] 00000000e0000000 - 00000000e0100000 : 0008:00e8
[10] 00000000e0200000 - 00000000e0400000 : 0004:00e8
[11] 00000000e0400000 - 00000000e0500000 : 0004:00e8

So overall, it's a big Ack from my side considering the huge improvements,
let's retry tomorrow with the link up workaround/fix to see if the detection
issue is related. Great work!

Best regards,
Willy

Fixing PCIe issues on Armada XP

[Jason Gunthorpe]

On Fri, Apr 11, 2014 at 01:13:36AM +0200, Willy Tarreau wrote:

> So the areas are well covered, though #11 seems larger than needed
> but I seem to remember that they're all rounded up by 1 MB anyway,
> then if so, that's OK.

Right, PCI bridge windows are 1MB aligned

> root at xpgp:~# grep -v disabled /sys/kernel/debug/mvebu-mbus/devices
> [00] 00000000e8010000 - 00000000e8020000 : 0004:00f0 (remap 0000000000010000)
> [08] 00000000fff00000 - 0000000100000000 : 0001:001d
> [09] 00000000f0000000 - 00000000f1000000 : 0001:002f
> [10] 00000000e1800000 - 00000000e1a00000 : 0004:00f8
> [11] 00000000e1a00000 - 00000000e1b00000 : 0004:00f8
> [12] 00000000e0000000 - 00000000e1000000 : 0008:00f8
> [13] 00000000e1000000 - 00000000e1800000 : 0008:00f8
> 
> I noticed above that both igb ports share the same window #11. So I
> tried to rmmod igb, remove both PCI devices, check mbus again (which
> did not change), rescan PCI and modprobe igb again, and everything is
> still operational with the same windows. I don't know if it is normal
> that they're not unregistered when the device goes away (maybe there's
> no refcount) ?

The windows are tied to the PCI core, not to the using driver
module. So they will only changed based on rescan an dynamic resource
assignment in the PCI core. PCI rescan has a 'memory' of the last
bridge windows and won't make dramtic changes, so expect the windows
to fairly sticky.

> If we have to keep them forever, then maybe a further improvement
> will consist in merging adjacent windows which sum up as a power of
> two (eg: #10 and #11 may be merged).

0x1b00000 - 0x1800000 = 0x300000 which is not a power of two..

> I tried to add a 3rd NIC in the mix (broadcom tg3), which caused the
> myri10ge to fail to load for an obscure reason after loading igb
> properly :

Oh, this looks a lot like what Thomas reported with his 5 NICs.

I really wonder what could be going on here.....

> Ah, interestingly if I load the NICs in the opposite order, they all load
> properly (myri10ge, igb, r8169) :

Load the NICs means insmod the driver ?

That is repeatable?

Certainly spooky, and suggests a kernel bug.....

It would be interesting to see what register values the driver is
getting back, is it all 0xF? 

I wonder if something is going wrong with the config write to enable
the memory decoder. That is triggered by the driver...

> So overall, it's a big Ack from my side considering the huge
> improvements, let's retry tomorrow with the link up workaround/fix
> to see if the detection issue is related. Great work!

Seems very likely to me, if the modified patch from Neil fixes it for
you too then we need to get that into mergable shape too!

Regards,
Jason

Fixing PCIe issues on Armada XP

[Willy Tarreau]

Hi Jason,

On Thu, Apr 10, 2014 at 05:40:00PM -0600, Jason Gunthorpe wrote:
> The windows are tied to the PCI core, not to the using driver
> module. So they will only changed based on rescan an dynamic resource
> assignment in the PCI core. PCI rescan has a 'memory' of the last
> bridge windows and won't make dramtic changes, so expect the windows
> to fairly sticky.

OK.

> > If we have to keep them forever, then maybe a further improvement
> > will consist in merging adjacent windows which sum up as a power of
> > two (eg: #10 and #11 may be merged).
> 
> 0x1b00000 - 0x1800000 = 0x300000 which is not a power of two..

Of course you're right. It was late last night, and I was having
a hard time thinking the addresses were not inclusive so in my
mind it was 0x18..0x1b inclusive, thus 4MB... Never mind.

> > I tried to add a 3rd NIC in the mix (broadcom tg3), which caused the
> > myri10ge to fail to load for an obscure reason after loading igb
> > properly :
> 
> Oh, this looks a lot like what Thomas reported with his 5 NICs.
> 
> I really wonder what could be going on here.....

I don't know but I have the hardware to easily reproduce it, if we want
to add printks again.

> > Ah, interestingly if I load the NICs in the opposite order, they all load
> > properly (myri10ge, igb, r8169) :
> 
> Load the NICs means insmod the driver ?

Yes.

> That is repeatable?

Yes, 100% it seems.

> Certainly spooky, and suggests a kernel bug.....
> 
> It would be interesting to see what register values the driver is
> getting back, is it all 0xF? 

That's what I suspected from the -1, but since the driver says "or 16MB"
and one of the windows is 16MB, I'm still confused, I need to add some
printk there.

> I wonder if something is going wrong with the config write to enable
> the memory decoder. That is triggered by the driver...

Thomas told me that the mbus driver is able to suggest a different
start address for the PCI windows. Maybe we fall in this case and the
driver doesn't expect this and uses a different register for the start
address.

> > So overall, it's a big Ack from my side considering the huge
> > improvements, let's retry tomorrow with the link up workaround/fix
> > to see if the detection issue is related. Great work!
> 
> Seems very likely to me, if the modified patch from Neil fixes it for
> you too then we need to get that into mergable shape too!

I can confirm that simply commenting out clk_disable_unprepare(clk)
fixes this problem, so yes it's the same issue. Just tried Neil's
modified patch and it works fine as well. So yes, we're making a lot
of progress.

Just in case anyone is interested, this is the NIC I'm using, both
on the mirabox and on the XP-GP ; it was worth an acquisition
considering how many corner cases it triggers in the kernel code :

  http://www.jetway.com.tw/jw/ipcboard_view.asp?productid=873&proname=ADMPEIDLA

Cheers,
Willy

Fixing PCIe issues on Armada XP

[Thomas Petazzoni]

Dear Neil Greatorex,

On Thu, 10 Apr 2014 22:56:00 +0100 (BST), Neil Greatorex wrote:
> > 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).

I managed to reproduce the problem of the PCIe device not being
detected on Mirabox when earlyprintk is disabled.

However, the proposed patch doesn't seem to work:

mvebu-pcie pcie-controller.3: PCIe0.0: performing link reset
mvebu-pcie pcie-controller.3: PCIe0.0: link went down after 100 tries
mvebu-pcie pcie-controller.3: PCIe0.0: link came back up after 0 tries

It waits hundred times for the link to go down, but it never goes down,
and then it doesn't wait at all to go up... because it never went down.

Moreover, I am not entirely convinced that a PHY reset is needed here.
In my tests, doing just a wait after setting the local dev number was
sufficient. The fact is works with earlyprintk also indicates that we
don't need to do any specific action with the PHY, just to wait a bit
of time. I am worried that resetting the PHY might actually take more
time than what is needed, and may have other consequences that we don't
necessarily understand at this point.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Fixing PCIe issues on Armada XP

[Thomas Petazzoni]

Hello all,

On Thu, 10 Apr 2014 18:19:53 +0200, Thomas Petazzoni wrote:

> This is an e-mail that attempts to summarize the situation in terms of
> Armada XP PCIe issues.

Attached is a v2 of the patches to fix the various pci-mvebu issues.
Changes since the version posted yesterday:

 * Include a fix for the timing issue of the PCIe interface that gets
   its clock disabled. I've chosen a different approach than the one
   suggested by Jason Gunthorpe, which does not involve resetting the
   PHY. I've tested my fix on the Mirabox, and the Armada 385 DB board
   on which Gregory originally reported the problem (I finally managed
   to reproduce the problem, it was due to the fact that only one of
   the PCIe interfaces is actually affected by the problem, because
   only the clock of the first PCIe interface is used by the
   mvebu-soc-id stuff).

 * Invert the order of Willy's and Jason's patches around MBus
   addresses.

I've also:

 * Pushed the patches at
   https://github.com/MISL-EBU-System-SW/mainline-public/tree/3.14/pci-debug

 * Included a single combined patch, because I know one of you needs
   that to test easily.

Can everybody test these patches, and confirm that they solve all the
outstanding problems?

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-igb-Fix-Null-pointer-dereference-in-igb_reset_q_vect.patch
Type: text/x-patch
Size: 1808 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140411/1b58ef39/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-igb-Unset-IGB_FLAG_HAS_MSIX-flag-when-falling-back-t.patch
Type: text/x-patch
Size: 5116 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140411/1b58ef39/attachment-0015.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-ARM-mvebu-change-the-default-PCIe-apertures-for-Arma.patch
Type: text/x-patch
Size: 2556 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140411/1b58ef39/attachment-0016.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-ARM-mvebu-switch-the-Armada-XP-DB-to-use-internal-re.patch
Type: text/x-patch
Size: 2498 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140411/1b58ef39/attachment-0017.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-ARM-mvebu-switch-the-Armada-XP-GP-to-use-internal-re.patch
Type: text/x-patch
Size: 3574 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140411/1b58ef39/attachment-0018.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-irqchip-armada-370-xp-fix-invalid-cast-of-signed-val.patch
Type: text/x-patch
Size: 1377 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140411/1b58ef39/attachment-0019.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-irqchip-armada-370-xp-implement-the-check_device-msi.patch
Type: text/x-patch
Size: 1700 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140411/1b58ef39/attachment-0020.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-irqchip-armada-370-xp-Fix-releasing-of-MSIs.patch
Type: text/x-patch
Size: 1273 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140411/1b58ef39/attachment-0021.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0009-pci-mvebu-fix-off-by-one-in-the-computed-size-of-the.patch
Type: text/x-patch
Size: 1860 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140411/1b58ef39/attachment-0022.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0010-bus-mvebu-mbus-Avoid-setting-an-undefined-window-siz.patch
Type: text/x-patch
Size: 2238 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140411/1b58ef39/attachment-0023.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0011-bus-mvebu-mbus-allow-several-windows-with-the-same-t.patch
Type: text/x-patch
Size: 1136 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140411/1b58ef39/attachment-0024.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0012-pci-pci-mvebu-split-PCIe-BARs-into-multiple-MBus-win.patch
Type: text/x-patch
Size: 6126 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140411/1b58ef39/attachment-0025.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0013-pci-pci-mvebu-wait-for-a-device-to-appear-to-fix-clo.patch
Type: text/x-patch
Size: 3592 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140411/1b58ef39/attachment-0026.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: combined.patch
Type: text/x-patch
Size: 15073 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140411/1b58ef39/attachment-0027.bin>

Fixing PCIe issues on Armada XP

[Neil Greatorex]

Thomas,

On Fri, 11 Apr 2014, Thomas Petazzoni wrote:

> Can everybody test these patches, and confirm that they solve all the
> outstanding problems?

PCIe now works well for me on my Mirabox / i350 combination with this 
patch set. Thanks!

Tested-by: Neil Greatorex <neil@fatboyfat.co.uk>

Cheers,
Neil

Fixing PCIe issues on Armada XP

[Jason Gunthorpe]

On Fri, Apr 11, 2014 at 12:23:14PM +0200, Thomas Petazzoni wrote:

> On Thu, 10 Apr 2014 22:56:00 +0100 (BST), Neil Greatorex wrote:
> > > 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).
> 
> I managed to reproduce the problem of the PCIe device not being
> detected on Mirabox when earlyprintk is disabled.
> 
> However, the proposed patch doesn't seem to work:
> 
> mvebu-pcie pcie-controller.3: PCIe0.0: performing link reset
> mvebu-pcie pcie-controller.3: PCIe0.0: link went down after 100 tries
> mvebu-pcie pcie-controller.3: PCIe0.0: link came back up after 0 tries
> 
> It waits hundred times for the link to go down, but it never goes down,
> and then it doesn't wait at all to go up... because it never went down.

I wonder if bit 30 only disables link training, but doesn't force the
link down. There may be another bit that is required here.

Alternatively, are you seeing a different problem? If you apply the
hack to the socid does your symptom go away as well?

> Moreover, I am not entirely convinced that a PHY reset is needed here.
> In my tests, doing just a wait after setting the local dev number was
> sufficient. The fact is works with earlyprintk also indicates that we
> don't need to do any specific action with the PHY, just to wait a bit
> of time. I am worried that resetting the PHY might actually take more
> time than what is needed, and may have other consequences that we don't
> necessarily understand at this point.

I really disagree - clearly the fundamental problem is suspending one
side of the PEX link while the other remains operating. That isn't
specified to work and really shouldn't work.

If you suspend one side of the PEX you *MUST* reset the
link. Absolutely. No Doubt In My Mind.

Remember, PCI-E is a serial shared state protocol. The two sides must
remain in sync or a link reset is required to recover the shared
state. Halting one side obviously destroys this invariant.

I think in many cases the reset happens autonomously. The remote side
will force it to happen. This is what the debugging from Neil shows -
the link just resets at some inconvenient point. Now we know why.

The timing sensitivity also makes sense, if you suspend for a very
short time window the other side might not notice. If you suspend for
longer the other side will reset the link autonomously and your local
side will quickly notice the reset once it comes back.

If you suspend for a little bit the link might not retrain immediately
but the shared state can become corrupted (eg sequence number
mismatch) this will eventually trigger a reset, after the local PEX
has been operating again.

The LinkUp bit after resume is also clearly a lie - most likely the
PEX takes some time to detect the change in remote state to trigger a
link down. After all, it was suspended while the remote was busy
trying to recover.

The fundamental problem here is the clock driver. It should not be
gating PEX clocks so naively. A PEX suspend needs to be sequenced to
ensure the link is cleanly brought down before the PEX is put to
sleep. That way it can cleanly and unambiguously be started up when it
resumes. No risk of glitching/corrupting the far side with some kind
of crap on the serial bus.

In any event, the most important required patch here is one that fixes
socid. It must not turn off the PEX clock. Then we can talk about how
to fix pci-mvebu to work as a module...

Jason

Fixing PCIe issues on Armada XP

[Matthew Minter]

Hi Guys,

I just wanted to confirm that both V1 and V2 of the patch set fixes my
problems, I still have an odd bug where a certain peripheral causes
the link to drop down from x4 to x1 but I think that is far more
likely a hardware issue with the device as it does not happen with any
other peripheral. So to confirm you can put my "tested by" on the
patches if you like.

I would also like to note, my board uses an external clock for the PCI
ports, thus it is unlikely to be effected by any issues relating to
clock drivers.

Please keep me up to date if there are any new revisions of the set
and I will try and test them.

Many thanks,
Matt

On 11 April 2014 17:31, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> On Fri, Apr 11, 2014 at 12:23:14PM +0200, Thomas Petazzoni wrote:
>
>> On Thu, 10 Apr 2014 22:56:00 +0100 (BST), Neil Greatorex wrote:
>> > > 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).
>>
>> I managed to reproduce the problem of the PCIe device not being
>> detected on Mirabox when earlyprintk is disabled.
>>
>> However, the proposed patch doesn't seem to work:
>>
>> mvebu-pcie pcie-controller.3: PCIe0.0: performing link reset
>> mvebu-pcie pcie-controller.3: PCIe0.0: link went down after 100 tries
>> mvebu-pcie pcie-controller.3: PCIe0.0: link came back up after 0 tries
>>
>> It waits hundred times for the link to go down, but it never goes down,
>> and then it doesn't wait at all to go up... because it never went down.
>
> I wonder if bit 30 only disables link training, but doesn't force the
> link down. There may be another bit that is required here.
>
> Alternatively, are you seeing a different problem? If you apply the
> hack to the socid does your symptom go away as well?
>
>> Moreover, I am not entirely convinced that a PHY reset is needed here.
>> In my tests, doing just a wait after setting the local dev number was
>> sufficient. The fact is works with earlyprintk also indicates that we
>> don't need to do any specific action with the PHY, just to wait a bit
>> of time. I am worried that resetting the PHY might actually take more
>> time than what is needed, and may have other consequences that we don't
>> necessarily understand at this point.
>
> I really disagree - clearly the fundamental problem is suspending one
> side of the PEX link while the other remains operating. That isn't
> specified to work and really shouldn't work.
>
> If you suspend one side of the PEX you *MUST* reset the
> link. Absolutely. No Doubt In My Mind.
>
> Remember, PCI-E is a serial shared state protocol. The two sides must
> remain in sync or a link reset is required to recover the shared
> state. Halting one side obviously destroys this invariant.
>
> I think in many cases the reset happens autonomously. The remote side
> will force it to happen. This is what the debugging from Neil shows -
> the link just resets at some inconvenient point. Now we know why.
>
> The timing sensitivity also makes sense, if you suspend for a very
> short time window the other side might not notice. If you suspend for
> longer the other side will reset the link autonomously and your local
> side will quickly notice the reset once it comes back.
>
> If you suspend for a little bit the link might not retrain immediately
> but the shared state can become corrupted (eg sequence number
> mismatch) this will eventually trigger a reset, after the local PEX
> has been operating again.
>
> The LinkUp bit after resume is also clearly a lie - most likely the
> PEX takes some time to detect the change in remote state to trigger a
> link down. After all, it was suspended while the remote was busy
> trying to recover.
>
> The fundamental problem here is the clock driver. It should not be
> gating PEX clocks so naively. A PEX suspend needs to be sequenced to
> ensure the link is cleanly brought down before the PEX is put to
> sleep. That way it can cleanly and unambiguously be started up when it
> resumes. No risk of glitching/corrupting the far side with some kind
> of crap on the serial bus.
>
> In any event, the most important required patch here is one that fixes
> socid. It must not turn off the PEX clock. Then we can talk about how
> to fix pci-mvebu to work as a module...
>
> Jason

-- 


------------------------------
For additional information including the registered office and the treatment of Xyratex confidential information please visit www.xyratex.com

------------------------------

Fixing PCIe issues on Armada XP

[Jason Gunthorpe]

On Fri, Apr 11, 2014 at 06:21:08PM +0100, Matthew Minter wrote:

> I would also like to note, my board uses an external clock for the
> PCI ports, thus it is unlikely to be effected by any issues relating
> to clock drivers.

It doesn't matter, the clock in question is the internal divided CPU
synchronous clock that drives the PEX TLP logic. Externally sourcing
the PHY clock won't change the synchronous side.

Jason

Fixing PCIe issues on Armada XP

[Thomas Petazzoni]

Neil, Jason,

On Thu, 10 Apr 2014 22:56:00 +0100 (BST), Neil Greatorex wrote:

> 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).

Unfortunately here your patch doesn't work (and neither does the patch
from Jason Gunthorpe). On Armada 370 DB, without the patch, the e1000e
NIC is detected when earlyprintk is enabled, and not detected when
earlyprintk is disabled. With the patch applied, the e1000e is no
longer detected *at all*, even if earlyprintk is enabled.

Extract from a boot log:

Linux version 3.15.0-rc1-00007-gedf643a-dirty (thomas at skate) (gcc version 4.8.1 (Ubuntu/Linaro 4.8.1-10ubuntu7) ) #317 SMP Fri Apr 18 14:54:13 CEST 2014
[...]
Kernel command line: console=ttyS0,115200 earlyprintk loglevel=8 root=/dev/nfs nfsroot=192.168.1.22:/home/thomas/nfsroot ip=192.168.1.142:192.168.1.22:192.168.1.1:255.255.255.0:devboard:eth0:on
[...]
mvebu-pcie pcie-controller.2: PCIe0.0: performing link reset
mvebu-pcie pcie-controller.2: PCIe0.0: link went down after 20 tries
mvebu-pcie pcie-controller.2: PCIe0.0: link came back up after 100 tries
mvebu-pcie pcie-controller.2: PCI host bridge to bus 0000:00
pci_bus 0000:00: root bus resource [io  0x1000-0xfffff]
pci_bus 0000:00: root bus resource [mem 0xf8000000-0xffdfffff]
pci_bus 0000:00: root bus resource [bus 00-ff]
pci 0000:00:01.0: [11ab:6710] type 01 class 0x060400
pci 0000:00:02.0: [11ab:6710] type 01 class 0x060400
PCI: bus0: Fast back to back transfers disabled
pci 0000:00:01.0: bridge configuration invalid ([bus 00-00]), reconfiguring
pci 0000:00:02.0: bridge configuration invalid ([bus 00-00]), reconfiguring
PCI: bus1: Fast back to back transfers enabled
pci_bus 0000:01: busn_res: [bus 01-ff] end is updated to 01
PCI: bus2: Fast back to back transfers enabled
pci_bus 0000:02: busn_res: [bus 02-ff] end is updated to 02
pci 0000:00:01.0: PCI bridge to [bus 01]
pci 0000:00:02.0: PCI bridge to [bus 02]
[...]
# /usr/sbin/lspci 
00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710
00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710
#

Any idea?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Fixing PCIe issues on Armada XP

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Fri, 11 Apr 2014 10:31:29 -0600, Jason Gunthorpe wrote:

> > Moreover, I am not entirely convinced that a PHY reset is needed here.
> > In my tests, doing just a wait after setting the local dev number was
> > sufficient. The fact is works with earlyprintk also indicates that we
> > don't need to do any specific action with the PHY, just to wait a bit
> > of time. I am worried that resetting the PHY might actually take more
> > time than what is needed, and may have other consequences that we don't
> > necessarily understand at this point.
> 
> I really disagree - clearly the fundamental problem is suspending one
> side of the PEX link while the other remains operating. That isn't
> specified to work and really shouldn't work.
> 
> If you suspend one side of the PEX you *MUST* reset the
> link. Absolutely. No Doubt In My Mind.
> 
> Remember, PCI-E is a serial shared state protocol. The two sides must
> remain in sync or a link reset is required to recover the shared
> state. Halting one side obviously destroys this invariant.
> 
> I think in many cases the reset happens autonomously. The remote side
> will force it to happen. This is what the debugging from Neil shows -
> the link just resets at some inconvenient point. Now we know why.
> 
> The timing sensitivity also makes sense, if you suspend for a very
> short time window the other side might not notice. If you suspend for
> longer the other side will reset the link autonomously and your local
> side will quickly notice the reset once it comes back.
> 
> If you suspend for a little bit the link might not retrain immediately
> but the shared state can become corrupted (eg sequence number
> mismatch) this will eventually trigger a reset, after the local PEX
> has been operating again.
> 
> The LinkUp bit after resume is also clearly a lie - most likely the
> PEX takes some time to detect the change in remote state to trigger a
> link down. After all, it was suspended while the remote was busy
> trying to recover.

Ok, fair enough. However, Neil's patch (which is basically your patch
with longer delays) isn't working here, as I just reported in a
separate e-mail.

So we don't have a solution right now. Do you have another proposal to
try ?

> The fundamental problem here is the clock driver. It should not be
> gating PEX clocks so naively. A PEX suspend needs to be sequenced to
> ensure the link is cleanly brought down before the PEX is put to
> sleep. That way it can cleanly and unambiguously be started up when it
> resumes. No risk of glitching/corrupting the far side with some kind
> of crap on the serial bus.
> 
> In any event, the most important required patch here is one that fixes
> socid. It must not turn off the PEX clock. Then we can talk about how
> to fix pci-mvebu to work as a module...

I don't really understand this: all clocks are gated at boot time, so
regardless of mvebu-soc-id behavior, the PCIe driver should take care
of doing all the necessary initialization without making the
assumptions that the clocks were left turned on from the bootloader
time. This is needed if we want to support pci-mvebu as a module, so I
don't see why hacking mvebu-soc-id is going to solve this.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Fixing PCIe issues on Armada XP

[Jason Gunthorpe]

On Fri, Apr 18, 2014 at 03:02:44PM +0200, Thomas Petazzoni wrote:

> > The LinkUp bit after resume is also clearly a lie - most likely the
> > PEX takes some time to detect the change in remote state to trigger a
> > link down. After all, it was suspended while the remote was busy
> > trying to recover.
> 
> Ok, fair enough. However, Neil's patch (which is basically your patch
> with longer delays) isn't working here, as I just reported in a
> separate e-mail.
> 
> So we don't have a solution right now. Do you have another proposal to
> try ?

Do you have time to see if we can isolate the difference on your
system?

> > The fundamental problem here is the clock driver. It should not be
> > gating PEX clocks so naively. A PEX suspend needs to be sequenced to
> > ensure the link is cleanly brought down before the PEX is put to
> > sleep. That way it can cleanly and unambiguously be started up when it
> > resumes. No risk of glitching/corrupting the far side with some kind
> > of crap on the serial bus.
> > 
> > In any event, the most important required patch here is one that fixes
> > socid. It must not turn off the PEX clock. Then we can talk about how
> > to fix pci-mvebu to work as a module...
 
> I don't really understand this: all clocks are gated at boot time, so
> regardless of mvebu-soc-id behavior, the PCIe driver should take care
> of doing all the necessary initialization without making the
> assumptions that the clocks were left turned on from the bootloader

The current mvebu-soc-id makes it impossible to do a clean hand off of
the boot loader PEX setup to the PEX driver. I think that is a
problem. Certainly if we fix it non-modular PEX will start working
reliably.

Keep in mind the current driver cannot startup a PEX without
bootloader support. It does not clear the set-at-reset
Conf_TrainingDisable bit, and it doesn't wait for a link to come up
after doing so.

> time. This is needed if we want to support pci-mvebu as a module, so I
> don't see why hacking mvebu-soc-id is going to solve this.

I agree we should try to figure the modular case out, but it looks to
me that suspending the PEX is a bigger job that just gating the
clock..

The automatic gating of the PEX clocks should be more clever. If there
are no DT elements that reference the clock it can be disabled,
otherwise we should probably leave it enabled and rely on the PEX
driver to do power management once it gets loaded.

So broadly, my thinking is the following largely orthogonal items:
 1) PEX's that are setup by the bootloader must be cleanly handed off
    to the driver. The clocks must never gate.
 2) The driver should learn to turn on a PEX from either the
    Conf_TrainingDisable=1 state or the clock gated state
 3) PEX clocks that are never used in DT can be safely shutoff,
    otherwise they must remain on
 4) The PEX driver can learn to properly suspend the PEX for power
    savings, via the normal Linux power management stuff.

What do you think?

Jason

Fixing PCIe issues on Armada XP

[Jason Gunthorpe]

> mvebu-pcie pcie-controller.2: PCIe0.0: performing link reset
> mvebu-pcie pcie-controller.2: PCIe0.0: link went down after 20 tries
> mvebu-pcie pcie-controller.2: PCIe0.0: link came back up after 100 tries

So the '100' here says the timeout expired. My first stab would be to
set the timeout longer.. Maybe your chip takes longer to train the
link.

You also mentioned that setting Conf_TrainingDisable doesn't cause the
link to go down? That would be useful to verify (in a clean
environment without clock gating weirdness..)

> # /usr/sbin/lspci 
> 00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710
> 00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710

I'm guessing:

echo 1 > /sys/bus/pci/rescan

Will make it appear?

Jason