On 10/12/21 17:39, Bjorn Helgaas wrote:
[+cc Vidya, Victor, ASPM L1.2 config issue; beginning of thread:
https://lore.kernel.org/all/20211011134238.16551-1-verdre@xxxxxxx/]
On Tue, Oct 12, 2021 at 10:55:03AM +0200, Jonas Dreßler wrote:
On 10/11/21 19:02, Pali Rohár wrote:
On Monday 11 October 2021 15:42:38 Jonas Dreßler wrote:
The most recent firmware (15.68.19.p21) of the 88W8897 PCIe+USB card
reports a hardcoded LTR value to the system during initialization,
probably as an (unsuccessful) attempt of the developers to fix firmware
crashes. This LTR value prevents most of the Microsoft Surface devices
from entering deep powersaving states (either platform C-State 10 or
S0ix state), because the exit latency of that state would be higher than
what the card can tolerate.
This description looks like a generic issue in 88W8897 chip or its
firmware and not something to Surface PCIe controller or Surface HW. But
please correct me if I'm wrong here.
Has somebody 88W8897-based PCIe card in non-Surface device and can check
or verify if this issue happens also outside of the Surface device?
It would be really nice to know if this is issue in Surface or in 8897.
Fairly sure the LTR value is something that's reported by the firmware
and will be the same on all 8897 devices (as mentioned in my reply to Bjorn
the second-latest firmware doesn't report that fixed LTR value).
I suggested earlier that the LTR values reported by the device might
depend on the electrical characteristics of the link and hence be
platform-dependent, but I think that might be wrong.
The spec (PCIe r5.0, sec 5.5.4) does say that some of the *other*
parameters related to L1.2 entry are platform-dependent:
Prior to setting either or both of the enable bits for L1.2, the
values for TPOWER_ON, Common_Mode_Restore_Time, and, if the ASPM
L1.2 Enable bit is to be Set, the LTR_L1.2_THRESHOLD (both Value
and Scale fields) must be programmed. The TPOWER_ON and
Common_Mode_Restore_Time fields must be programmed to the
appropriate values based on the components and AC coupling
capacitors used in the connection linking the two components. The
determination of these values is design implementation specific.
These T_POWER_ON, Common_Mode_Restore_Time, and LTR_L1.2_THRESHOLD
values are in the L1 PM Substates Control registers.
I don't know of a way for the kernel or the device firmware to learn
these circuit characteristics or the appropriate values, so I think
only system firmware can program the L1 PM Substates Control registers
(a corollary of this is that I don't see a way for hot-plugged devices
to *ever* use L1.2).
I wonder if this reset quirk works because pci_reset_function() saves
and restores much of config space, but it currently does *not* restore
the L1 PM Substates capability, so those T_POWER_ON,
Common_Mode_Restore_Time, and LTR_L1.2_THRESHOLD values probably get
cleared out by the reset. We did briefly save/restore it [1], but we
had to revert that because of a regression that AFAIK was never
resolved [2]. I expect we will eventually save/restore this, so if
the quirk depends on it *not* being restored, that would be a problem.
You should be able to test whether this is the critical thing by
clearing those registers with setpci instead of doing the reset. Per
spec, they can only be modified when L1.2 is disabled, so you would
have to disable it via sysfs (for the endpoint, I think)
/sys/.../l1_2_aspm and /sys/.../l1_2_pcipm, do the setpci on the root
port, then re-enable L1.2.
[1] https://git.kernel.org/linus/4257f7e008ea
[2] https://lore.kernel.org/all/20210127160449.2990506-1-helgaas@xxxxxxxxxx/
Hmm, interesting, thanks for those links.
Are you sure the config values will get lost on the reset? If we only reset
the port by going into D3hot and back into D0, the device will remain powered
and won't lose the config space, will it?
Because when I reset the bridge using pci_reset_function() (ie. pci_pm_reset())
or when I suspend and resume the laptop, all the L1 PM Substates registers are
still the same as before, nothing is lost.
That said, our new mwifiex_pcie_reset_d3cold_quirk() puts *both the card and
the bridge* into D3cold, so I gave that a try, and indeed the cards L1 Substate
Ctl registers are cleared out (so T_CommonMode, LTR1.2_Threshold and T_PwrOn),
but the bridge still has its values, no clue why that's the case.
