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