Re: [PATCH v2 2/2] PCI: Add missing link delays required by the PCIe spec

[Mika Westerberg]

On Tue, Nov 05, 2019 at 10:10:17AM -0600, Bjorn Helgaas wrote:
> > > I actually suspect there *is* a dependency -- we should respect the
> > > Target Link Speed and and width so the link resumes in the same
> > > configuration it was before suspend.  And I suspect that may require
> > > an explicit retrain after restoring PCI_EXP_LNKCTL2.
> > 
> > According the PCIe spec the PCI_EXP_LNKCTL2 Target Link Speed is marked
> > as RWS (S for sticky) so I suspect its value is retained after reset in
> > the same way as PME bits. Assuming I understood it correctly.
> 
> This patch is about coming from D3cold, isn't it?  I don't think we
> can assume sticky bits are preserved in D3cold (except maybe when
> auxiliary power is enabled).

Indeed, good point. I see some GPU drivers are programming Target Link
Speed which will not be retained after the hierarchy is put into D3cold
and back. I think this potential problem is not related to the missing
link delays this patch is addressing, though. It has been existing in
pci_restore_pcie_state() already (where it restores PCI_EXP_LNKCTL2).

I think this can be solved as a separate patch by doing something
like:

  1. In pci_restore_pcie_state() check if the saved Target Link Speed
     differs from what is in the register currently.

  2. Restore the value as we already do now.

  3. If there the speed differs then trigger link retrain.

  4. Restore rest of the root/downstream port state.

It is not clear if we need to do anything for upstream ports (PCIe 5.0
sec 6.11 talks about doing this on upstream component e.g downstream
port). After this there will be the link delay (added by this patch)
which takes care of waiting for the downstream component to be
accessible (even after retrain).

However, I'm not sure how this can be properly tested. Maybe hacking
some downstream port to lower the speed, enter D3cold and then resume it
and see if the Target Link Speed gets updated correctly.