On Fri, Aug 26, 2022 at 06:25:26PM +0530, Vidya Sagar wrote:
> Previously ASPM L1 Substates control registers (CTL1 and CTL2) weren't
> saved and restored during suspend/resume leading to L1 Substates
> configuration being lost post-resume.
>
> Save the L1 Substates control registers so that the configuration is
> retained post-resume.
>
> Signed-off-by: Vidya Sagar <vidyas@xxxxxxxxxx>
> ---
> V3:
> * Disabled L1.2 enable fields while restoring Control-1 register
This really looks promising! Has somebody confirmed that the
disappearing L1SS capability problem doesn't happen here?
> +void pci_save_aspm_l1ss_state(struct pci_dev *dev)
> +{
> + int aspm_l1ss;
> + struct pci_cap_saved_state *save_state;
> + u32 *cap;
> +
> + if (!pci_is_pcie(dev))
> + return;
> +
> + aspm_l1ss = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_L1SS);
> + if (!aspm_l1ss)
> + return;
Isn't it enough to check this?
if (!dev->l1ss)
return;
> +void pci_restore_aspm_l1ss_state(struct pci_dev *dev)
> +{
> + int aspm_l1ss;
> + struct pci_cap_saved_state *save_state;
> + u32 *cap, l1_2_enable;
> +
> + if (!pci_is_pcie(dev))
> + return;
> +
> + aspm_l1ss = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_L1SS);
> + if (!aspm_l1ss)
> + return;
> +
> + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_L1SS);
> + if (!save_state)
> + return;
> +
> + cap = (u32 *)&save_state->cap.data[0];
> + pci_write_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL2, *cap++);
> + /* Disable L1.2 while updating. See PCIe r5.0, sec 5.5.4, 7.8.3.3 */
> + l1_2_enable = *cap & PCI_L1SS_CTL1_L1_2_MASK;
> + pci_write_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL1,
> + (*cap & ~PCI_L1SS_CTL1_L1_2_MASK));
> + if (l1_2_enable)
> + pci_clear_and_set_dword(dev, aspm_l1ss + PCI_L1SS_CTL1, 0,
> + l1_2_enable);
> +}
What if we did something like the following? Then we wouldn't have to
duplicate the fancy logic in aspm_calc_l1ss_info() and
pci_restore_aspm_l1ss_state(), and we'd only need the big comment in
one place.
+static void aspm_program_l1ss(struct pci_dev *dev, u32 ctl1, u32 ctl2)
+{
+ u16 l1ss = dev->l1ss;
+ u32 l1_2_enable;
+
+ /*
+ * Per PCIe r6.0, sec 5.5.4, T_POWER_ON in PCI_L1SS_CTL2 must be
+ * programmed prior to setting the L1.2 enable bits in PCI_L1SS_CTL1.
+ */
+ pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL2, ctl2);
+
+ /*
+ * In addition, Common_Mode_Restore_Time and LTR_L1.2_THRESHOLD in
+ * PCI_L1SS_CTL1 must be programmed *before* setting the L1.2
+ * enable bits, even though they're all in PCI_L1SS_CTL1.
+ */
+ l1_2_enable = ctl1 & PCI_L1SS_CTL1_L1_2_MASK;
+ ctl1 &= ~PCI_L1SS_CTL1_L1_2_MASK;
+
+ pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL1, ctl1);
+ if (l1_2_enable)
+ pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL1,
+ ctl1 | l1_2_enable);
+}
(This is somewhat simplified from what aspm_calc_l1ss_info() does
today. It looks to me like aspm_calc_l1ss_info() does more config
reads than necessary.)
