[PATCH] pci: Work around PCIe link training failures

[Maciej W. Rozycki]

Hi Pali,

> >  Well, this code checks for non-zero lnkcap2 first and ignores it if it's 
> > zero, so I believe it does the right thing.  Have I missed anything?
> 
> I'm reading spec again and I'm not sure now. It has following section:
> 
>   For software to determine the supported Link speeds for components
>   where the Link Capabilities 2 register is either not implemented, or
>   the value of its Supported Link Speeds Vector is 0000000b, software
>   can read bits 3:0 of the Link Capabilities register (now defined to be
>   the Max Link Speed field), and interpret the value as follows:
>     0001b 2.5 GT/s Link speed supported
>     0010b 5.0 GT/s and 2.5 GT/s Link speeds supported

 This is consistent with speeds defined with PCIe rev. 2.0, which is the 
last revision not to define the Link Capabilities 2 register.

> I'm not sure how it should be interpreted, but my assumption is that
> empty lnkcap2 implicates that only 5GT/s or 2.5GT/s speeds are supported
> and therefore trying higher should not be done...

 I can see no ambiguity here.

 The Link Capabilities register always defines the maximum speed supported 
and given that there is no speed masking defined for PCIe rev. 2.0 and 
below all speeds up to the maximum must be always supported with hardware 
compliant to these specification revisions.

 As from PCIe rev. 3.0 the Link Capabilities 2 register provides support 
for speed masking and therefore not all speeds below the maximum given by 
the Link Capabilities register must be supported with hardware compliant 
to this and higher specification revisions.

> Also in spec is following note:
> 
>   It is strongly encouraged that software primarily utilize the
>   Supported Link Speeds Vector instead of the Max Link Speed field.
> 
> So based on this note, iteration should be done via lnkcap2 bits instead
> of lnkcap speed.

 I believe my code does iterate over lnkcap2 bits already, because speed 
encodings that correspond to those lnkcap2 bits that are zero are skipped.

> >  It could be done in Linux in addition to U-Boot, although I think doing 
> > that in the firmware is more important, especially as there could be a 
> > boot device downstream such a switch.  And depending on the platform Linux 
> > does not always reassign buses, so a user intervention (i.e. an explicitly 
> > added kernel parameter) would have to be required.
> 
> It is important to have it in both components (U-Boot and Linux). For
> example native PCIe controller drivers in linux kernel as a first thing
> do complete reset of controller together with connected devices. So
> whatever do U-Boot is completely lost. And important is that Linux
> kernel drivers should not depend on some bootloader configuration. And
> note that your patch implements this workaround in CONFIG_PCI_PNP code,
> so if board disable this option, workaround is not applied.

 Well, everything is not Linux.  The lone Unmatched board can run at least 
FreeBSD or Haiku right now, and it is only one of the numerous machines 
supported by U-Boot.  A piece of code in Linux will not help other OSes.

 Also as I noted not all Linux platforms discard the firmware settings 
with PCI/e devices, let alone bring them to their power-on defaults; this 
certainly does not happen with the Unmatched or I couldn't have benefitted 
with this change of mine proposed or earlier hacks with poking at the 
relevant registers from the U-Boot command line.  So in my scenario this 
fix surely qualifies as good enough.  Interestingly Linux does notice the 
clamp imposed by my change with the offending link:

pci 0000:05:00.0: 2.000 Gb/s available PCIe bandwidth, limited by 2.5 GT/s PCIe x1 link at 0000:02:03.0 (capable of 8.000 Gb/s with 5.0 GT/s PCIe x2 link)

(the x1 vs x2 limitation is imposed by wiring one lane only in the Delock 
device in the first place; then I wired it to a single-lane M.2 connector 
of the Unmatched).

 That said I'm not opposed to porting this code to Linux, I have certainly 
contributed infinitely more code to Linux than I did to U-Boot.  It is 
just that my resources are limited, so I need to cautiously allocate them.  
I have other priorities, the Unmatched box is in my remote lab and I am 
only going to have physical access to it this week only.  The next 
opportunity may be next year at the earliest.  And I dare not reflashing 
U-Boot remotely so as not to lose access with a broken build because I 
need the machine for other purposes like GCC verification.

 So I'd rather focus on getting the change right for U-Boot now, while I 
am here.  If we agree on the way to move forward with this code, then I 
can look into porting this stuff to Linux, as I can surely add another 
kernel image to boot the system from remotely in a safe manner.

> >  And these are generic PCIe switches, they could be anywhere and with the 
> > weird combinations of hardware interfaces available now (e.g. PCI-PCIe 
> > bridge adapters or M.2 to regular PCIe slot adapters) virtually any 
> > combination is possible.
> > 
> >  E.g. I have a 1997-vintage dual Pentium MMX box (82439HX host bridge; 
> > [8086:1250]) with PCIe devices, although it does require a lot of Linux 
> > interventions to cope with its firmware limitations.  NB I plan to add 
> > some NVMe storage to that box, and I believe the ASM2824 has been used in 
> > some M.2 carrier boards meant for NVMe devices.
> 
> Hm... now thinking about your patch... and if it is general, applied to
> all devices, should it also be applied to any type of downstream port?
> 
> Meaning also for root ports, or PCIe port of PCI/X to PCIe bridge (I
> guess that such old platforms with only PCI host bridge without PCIe
> could be affected too if you connect PCIe card via PCI/X to PCIe bridge
> and then this bridge to host PCI slot).

 I guess so.  I considered root ports, but somehow it did not occur to me 
that they can be wired directly to slots (any sane manufacturer would get 
their onboard devices sorted), which of course they can.  I forgot about 
the PCI/PCI-X to PCI Express bridges though.  Thanks for the suggestion.

 As this is a trivial change to make I'll post an update, but I will wait 
till the end of the day or so so as to let people who do this stuff as a 
part of their dayjob have a chance to give feedback.

> >  As I noted ASMedia declined to comment, so it's hard to say if the cause 
> > has been nailed correctly and which devices if any, beyond [1b21:2824], 
> > are affected.
> 
> Yea, we do not know... And because we know that there is lot of broken
> HW which works with current version, we need to be very careful when
> introducing some workaround which is called on every hardware.
> 
> For example something similar like in your patch was implemented in
> Marvell SerDes U-Boot driver, which controls physical layer (so on place
> where nobody would expect touching higher layer code). Just this code
> did not touched retrain link bit... And so it could not have worked and
> was recently removed in patch series:
> https://lore.kernel.org/u-boot/20210924205922.25432-1-marek.behun@nic.cz/

 Sure, which is why this change tries hard to be conservative and checks 
multiple conditions to make sure a link is in trouble before it pokes at 
it.  If you suspect that these checks may be insufficient, then I'm open 
to further suggestions.

 NB I forgot to mention in the change description and I can add it in v2 
that the rate of the speed oscillation/link training with the ASM2824 is 
34-35 times per second.  So it's quite a lengthy process.

> I'm not saying that I'm opposing this patch. Just I would like see how
> is this issue will be fixed in kernel as kernel general workaround would
> affect lot of more devices. And so solution accepted by kernel project
> should be perfectly fine also for smaller project like U-Boot.

 Well, as far as I'm concerned I'd propose a functional equivalent that 
has been merely mechanically adapted to Linux internal interfaces (though 
busy-looping for extended periods with interrupts disabled, which is what 
is needed here, may be problematic in Linux or indeed any OS, unlike with 
the firmware; maybe PCI enumeration is early enough for that not to matter 
in reality though).

 Again, thanks for your input.

  Maciej