Re: [PATCH V3] PCI/ASPM: Save/restore L1SS Capability for suspend/resume

[Bjorn Helgaas]

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.)