Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 7 Sep 2022 16:05:40 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Vidya Sagar <vidyas@nvidia.com>
Cc:     bhelgaas@google.com, lorenzo.pieralisi@arm.com,
        refactormyself@gmail.com, kw@linux.com, rajatja@google.com,
        kenny@panix.com, kai.heng.feng@canonical.com, treding@nvidia.com,
        jonathanh@nvidia.com, abhsahu@nvidia.com, sagupta@nvidia.com,
        benchuanggli@gmail.com, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org, kthota@nvidia.com,
        mmaddireddy@nvidia.com, sagar.tv@gmail.com
Subject: Re: [PATCH V3] PCI/ASPM: Save/restore L1SS Capability for
 suspend/resume
Message-ID: <20220907210540.GA140988@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220826125526.28859-1-vidyas@nvidia.com>
Precedence: bulk

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@nvidia.com>
> ---
> 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.)