[PATCH v2] pci: Work around PCIe link training failures

[Maciej W. Rozycki]

Hi Pali,

> > If that has indicated failure, reduce the target speed, request a link 
> > retrain and check again if the link has stabilised.  Repeat until either 
> > successful or the link speeds supported by the downstream port have been 
> > exhausted.
> 
> I would like to hear what Bjorn Helgaas thinks about this issue at all
> and how to handle it, without potentially break something else. And
> based on the fact that in U-Boot's PCI core code there is no such global
> quirk implemented, I really do not know if U-Boot maintainers and
> developers want to review and understand all PCI Express aspect of this
> code and possible side-effects. This is just what I think about it, I do
> not have maintainer hat.

 Please leave the decision up to actual maintainers then.  They can speak 
for themselves.

> I suggested to to first send this fixup to the Linux kernel for proper
> review (or to any other similar bigger project with PCI Express support)
> and then implement (accepted) solution into U-Boot. As I think this can
> speed up inclusion of this fix/workaround in U-Boot. I do not have a
> problem to ack/review PCI patches for U-Boot which just re-implements
> PCI logic from Linux kernel.

 I think that with a change functionally reviewed elsewhere and merely 
carried over there'd be little to nothing to ack/review here.  However 
based on my most recent findings reported in the other message I don't 
think we're going to have the same workaround as Linux likely will, 
because U-Boot and Linux have different objectives each.

> > +	for (speed = (exp_lnkcap & PCI_EXP_LNKCAP_SLS) - 1;
> > +	     speed >= PCI_EXP_LNKCAP_SLS_2_5GB;
> > +	     speed--) {
> > +		if (exp_lnkcap2 && (exp_lnkcap2 & 1 << speed) == 0)
> > +			continue;
> > +
> > +		debug("PCI Autoconfig: %02x.%02x.%02x: Trying speed: %u\n",
> > +		      PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf), speed);
> > +
> > +		dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL2,
> > +				      exp_lnkctl2 | speed);
> > +		dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL,
> > +				      exp_lnkctl | PCI_EXP_LNKCTL_RETRLNK);
> > +
> > +		if (dm_pciauto_exp_link_stable(dev, pcie_off))
> > +			return;
> 
> I was thinking more about it and maybe this code still needs some
> improvements. PCIe link consist of two ends and you should not force
> speed on one end which is unsupported by other. Because iteration is
> going from highest speed to slowest speed, it means that this code can
> try to setup also 8GT/s speed on root/downstream port to which is
> connected only 5GT/s card. I have 5GT/s PCIe cards which successfully
> initialize link when downstream port forces 8GT/s speed, but link is
> setup only to 2.5GT/s. So this code for those PCIe cards connected to
> unstable ports (when this workaround is needed) degrades performance
> from 5GT/s to just 2.5GT/s. But I do not know how to correctly handle it
> (one quick idea: always force 2.5GT/s, stabilize link, read capability
> from card and retrain again; but I do not know if this is correct).

 Are you sure you know what you are talking about here?

 As a part of link training ends first negotiate link at 2.5GT/s, which is 
a speed all devices must support, then exchange information as to speeds 
actually supported, and only then possibly choose to switch to another 
speed supported by both.  How lowering the target link speed is supposed 
to break it?  It's no different to connecting a device the maximum 
supported link speed of which is the target link speed chosen.  At worst 
they'll stay at 2.5GT/s.

> And... I have also PCIe HW which does not support DLLA bit and RL bit is
> write-only. I'm not sure right now, if this code would not cause issues
> for such HW (if to it is connected broken PCIe card which does not like
> link retraining), this needs to be tested.

 RL is always write-only and DLLLA is often not supported.  I think both 
code itself and my description is clear as to the handling of these bits.  

 Are you sure you are up to reviewing code in this area?

  Maciej