On Friday 25 June 2021 15:19:36 Bjorn Helgaas wrote: > On Mon, Jun 21, 2021 at 04:28:55PM +0200, Pali Rohár wrote: > > On Wednesday 16 June 2021 16:38:19 Bjorn Helgaas wrote: > > > On Wed, Jun 02, 2021 at 09:03:02PM +0200, Pali Rohár wrote: > > > > On Wednesday 02 June 2021 10:55:59 Bjorn Helgaas wrote: > > > > > On Wed, Jun 02, 2021 at 02:08:16PM +0200, Pali Rohár wrote: > > > > > > On Tuesday 01 June 2021 19:00:36 Bjorn Helgaas wrote: > > > > > > > > > > > > I wonder if this could be restructured as a generic quirk > > > > > > > in quirks.c that simply set the bridge's TLS to 2.5 GT/s > > > > > > > during enumeration. Or would the retrain fail even in > > > > > > > that case? > > > > > > > > > > > > If I understand it correctly then PCIe link is already up > > > > > > when kernel starts enumeration. So setting Bridge TLS to 2.5 > > > > > > GT/s does not change anything here. > > > > > > > > > > > > Moreover it would have side effect that cards which are > > > > > > already set to 5+ GT/s would be downgraded to 2.5 GT/s > > > > > > during enumeration and for increasing speed would be needed > > > > > > another round of "enumeration" to set a new TLS and retrain > > > > > > link again. As TLS affects link only after link goes into > > > > > > Recovery state. > > > > > > > > > > > > So this would just complicate card enumeration and settings. > > > > > > > > > > The current quirk complicates the ASPM code. I'm hoping that > > > > > if we set the bridge's Target Link Speed during enumeration, > > > > > the link retrain will "just work" without complicating the > > > > > ASPM code. > > > > > > > > > > An enumeration quirk wouldn't have to set the bridge's TLS to > > > > > 2.5 GT/s; the quirk would be attached to specific endpoint > > > > > devices and could set the bridge's TLS to whatever the > > > > > endpoint supports. > > > > > > > > Now I see what you mean. Yes, I agree this is a good idea and > > > > can simplify code. Quirk is not related to ASPM code and > > > > basically has nothing with it, just I put it into aspm.c because > > > > this is the only place where link retraining was activated. > > > > > > > > But with this proposal there is one issue. Some kernel drivers > > > > already overwrite PCI_EXP_LNKCTL2_TLS value. So if PCI > > > > enumeration code set some value into PCI_EXP_LNKCTL2_TLS bits > > > > then drivers can change it and once ASPM will try to retrain > > > > link this may cause this issue. > > > > > > I guess you mean the amdgpu, radeon, and hfi1 drivers. They > > > really shouldn't be mucking with that stuff anyway. But they do > > > and are unlikely to change because we don't have any good > > > alternative. > > > > Yea, these are examples of such drivers... Maybe it is a good idea > > to ask those people why changing PCI_EXP_LNKCTL2_TLS is needed. As > > these drivers are often derived from codebase of shared multisystem > > drivers or from common documentation, it is possible that original > > source has this code as a workaround or common pattern used in other > > operating systems, not related to linux... > > > > > One way around that would be to add some quirk code to > > > pcie_capability_write_word(). Ugly, but we do have something sort > > > of similar in pcie_capability_read_word() already. > > > > Bjorn, do you really want such ugly hack in > > pcie_capability_write_word? It is common code used and called from > > lot of places so it may affect whole system if in future somebody > > changes it again... > > I don't know which is uglier, a quirk in pcie_capability_write_word() > or a quirk in aspm.c that has nothing to do with ASPM. They're both > ugly :) Ok :-) > FWIW, in pcie_capability_write_word() I would envision not a check for > Atheros, but rather something like a "dev->max_target_link_speed" that > could be set by an Atheros quirk. It does get uglier if we want to > restrict the bridge's link speed via a quirk, then unrestrict it when > the endpoint is unplugged. > > I know pcie_downgrade_link_to_gen1() only returns failure for corner > cases that "should not occur," It is not only corner case. It happens _always_ with at least two pci drivers. As I wrote in other email due to this issue, some quirk code which allows / disallows link retraining is required in aspm.c file. > but I don't like the fact that it's > possible to change Common Clock Configuration without doing the > retrain. That would leave us with incorrect ASPM exit latencies, > which is really hard to debug. I see... Any idea how to solve this issue? > Here's the relevant text in the spec (PCIe r5.0): > > 7.5.3.6 Link Capabilities > > L0s Exit Latency - This field indicates the L0s exit latency for > the given PCI Express Link. The value reported indicates the > length of time this Port requires to complete transition from L0s > to L0. ... > > Note that exit latencies may be influenced by PCI Express > reference clock configuration depending upon whether a component > uses a common or separate reference clock. > > 7.5.3.6 Link Control > Common Clock Configuration - When Set, this bit indicates that > this component and the component at the opposite end of this Link > are operating with a distributed common reference clock. ... > > After changing the value in this bit in both components on a Link, > software must trigger the Link to retrain by writing a 1b to the > Retrain Link bit of the Downstream Port.
