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

[Stefan Roese]

Hi Maciej,

On 11/18/21 01:32, Maciej W. Rozycki wrote:
> 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.

Sure, this is correct. But still, review comments from other developers
(e.g. non maintainers) are often very helpful. And especially Pali has
done quite a lot of work in the area of PCIe lately (amongst others) and
IMHO his thoughts and comments seem quite reasonable - at least to me.

>> 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, please don't get me wrong. I value your input here. But this
sounds a bit offensive IMHO and I would like to keep the discussion here
on the technical level.

Thanks,
Stefan