Re: [PATCH v2] PCI/ASPM: Add back L1 PM Substate save and restore

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi Ilpo,

On Mon, Sep 18, 2023 at 02:46:07PM +0300, Ilpo Järvinen wrote:
> On Mon, 11 Sep 2023, Mika Westerberg wrote:
> 
> > Commit a7152be79b62 ("Revert "PCI/ASPM: Save L1 PM Substates Capability
> > for suspend/resume"") reverted saving and restoring of ASPM L1 Substates
> > due to a regression that caused resume from suspend to fail on certain
> > systems. However, we never added this capability back and this is now
> > causing systems fail to enter low power CPU states, drawing more power
> > from the battery.
> > 
> > The original revert mentioned that we restore L1 PM substate configuration
> > even though ASPM L1 may already be enabled. This is due the fact that
> > the pci_restore_aspm_l1ss_state() was called before pci_restore_pcie_state().
> > 
> > Try to enable this functionality again following PCIe r6.0.1, sec 5.5.4
> > more closely by:
> > 
> >   1) Do not restore ASPM configuration in pci_restore_pcie_state() but
> >      do that after PCIe capability is restored in pci_restore_aspm_state()
> >      following PCIe r6.0, sec 5.5.4.
> > 
> >   2) ASPM is first enabled on the upstream component and then downstream
> >      (this is already forced by the parent-child ordering of Linux
> >      Device Power Management framework).
> > 
> >   3) Program ASPM L1 PM substate configuration before L1 enables.
> > 
> >   4) Program ASPM L1 PM substate enables last after rest of the fields
> >      in the capability are programmed.
> > 
> >   5) Add denylist that skips restoring on the ASUS and TUXEDO systems
> >      where these regressions happened, just in case. For the TUXEDO case
> >      we only skip restore if the BIOS is involved in system suspend
> >      (that's forcing "mem_sleep=deep" in the command line). This is to
> >      avoid possible power regression when the default suspend to idle is
> >      used, and at the same time make sure the devices continue working
> >      after resume when the BIOS is involved.
> > 
> > Reported-by: Koba Ko <koba.ko@canonical.com>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217321
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216782
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216877
> > Cc: Tasev Nikola <tasev.stefanoska@skynet.be>
> > Cc: Mark Enriquez <enriquezmark36@gmail.com>
> > Cc: Thomas Witt <kernel@witt.link>
> > Cc: Werner Sembach <wse@tuxedocomputers.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> > Hi,
> > 
> > This is second try. The previous version of the patch can be found here:
> > 
> >   https://lore.kernel.org/linux-pci/20230627062442.54008-1-mika.westerberg@linux.intel.com/
> > 
> > In this version:
> > 
> >   - We move ASPM enables from pci_restore_pcie_state() into
> >     pci_restore_aspm_state() to make sure they are clear when L1SS bits
> >     are programmed (as per PCIe spec).
> > 
> >   - The denylist includes the TUXEDO system as well but only if suspend
> >     is done via BIOS (e.g mem_sleep=deep is forced by user). This way
> >     the PCIe devices should continue working after S3 resume, and at the
> >     same time allow better power savings. If the default s2idle is used
> >     then we restore L1SS to allow the CPU enter lower power states. This
> >     is the best I was able to come up to make everyone happy.
> > 
> >  drivers/pci/pci.c       |  18 ++++-
> >  drivers/pci/pci.h       |   4 ++
> >  drivers/pci/pcie/aspm.c | 148 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 168 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 59c01d68c6d5..7c72d40ec0ff 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -1576,7 +1576,7 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
> >  {
> >  	int i = 0;
> >  	struct pci_cap_saved_state *save_state;
> > -	u16 *cap;
> > +	u16 *cap, val;
> >  
> >  	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
> >  	if (!save_state)
> > @@ -1591,7 +1591,14 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
> >  
> >  	cap = (u16 *)&save_state->cap.data[0];
> >  	pcie_capability_write_word(dev, PCI_EXP_DEVCTL, cap[i++]);
> > -	pcie_capability_write_word(dev, PCI_EXP_LNKCTL, cap[i++]);
> > +	/*
> > +	 * Restoring ASPM L1 substates has special requirements
> > +	 * according to the PCIe spec 6.0. So we restore here only the
> > +	 * LNKCTL register with the ASPM control field clear. ASPM will
> > +	 * be restored in pci_restore_aspm_state().
> > +	 */
> > +	val = cap[i++] & ~PCI_EXP_LNKCTL_ASPMC;
> > +	pcie_capability_write_word(dev, PCI_EXP_LNKCTL, val);
> >  	pcie_capability_write_word(dev, PCI_EXP_SLTCTL, cap[i++]);
> >  	pcie_capability_write_word(dev, PCI_EXP_RTCTL, cap[i++]);
> >  	pcie_capability_write_word(dev, PCI_EXP_DEVCTL2, cap[i++]);
> > @@ -1702,6 +1709,7 @@ int pci_save_state(struct pci_dev *dev)
> >  	pci_save_ltr_state(dev);
> >  	pci_save_dpc_state(dev);
> >  	pci_save_aer_state(dev);
> > +	pci_save_aspm_state(dev);
> >  	pci_save_ptm_state(dev);
> >  	return pci_save_vc_state(dev);
> >  }
> > @@ -1815,6 +1823,7 @@ void pci_restore_state(struct pci_dev *dev)
> >  	pci_restore_rebar_state(dev);
> >  	pci_restore_dpc_state(dev);
> >  	pci_restore_ptm_state(dev);
> > +	pci_restore_aspm_state(dev);
> >  
> >  	pci_aer_clear_status(dev);
> >  	pci_restore_aer_state(dev);
> > @@ -3507,6 +3516,11 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev)
> >  	if (error)
> >  		pci_err(dev, "unable to allocate suspend buffer for LTR\n");
> >  
> > +	error = pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_L1SS,
> > +					    2 * sizeof(u32));
> > +	if (error)
> > +		pci_err(dev, "unable to allocate suspend buffer for ASPM-L1SS\n");
> > +
> >  	pci_allocate_vc_save_buffers(dev);
> >  }
> >  
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 39a8932dc340..11cec757a624 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -567,10 +567,14 @@ int pcie_retrain_link(struct pci_dev *pdev, bool use_lt);
> >  void pcie_aspm_init_link_state(struct pci_dev *pdev);
> >  void pcie_aspm_exit_link_state(struct pci_dev *pdev);
> >  void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
> > +void pci_save_aspm_state(struct pci_dev *pdev);
> > +void pci_restore_aspm_state(struct pci_dev *pdev);
> >  #else
> >  static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
> >  static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
> >  static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
> > +static inline void pci_save_aspm_state(struct pci_dev *pdev) { }
> > +static inline void pci_restore_aspm_state(struct pci_dev *pdev) { }
> >  #endif
> >  
> >  #ifdef CONFIG_PCIE_ECRC
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index 1bf630059264..94e7a21c37dc 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -7,6 +7,7 @@
> >   * Copyright (C) Shaohua Li (shaohua.li@intel.com)
> >   */
> >  
> > +#include <linux/dmi.h>
> >  #include <linux/kernel.h>
> >  #include <linux/math.h>
> >  #include <linux/module.h>
> > @@ -17,6 +18,7 @@
> >  #include <linux/pm.h>
> >  #include <linux/init.h>
> >  #include <linux/slab.h>
> > +#include <linux/suspend.h>
> >  #include <linux/jiffies.h>
> >  #include <linux/delay.h>
> >  #include "../pci.h"
> > @@ -712,6 +714,152 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
> >  				PCI_L1SS_CTL1_L1SS_MASK, val);
> >  }
> >  
> > +void pci_save_aspm_state(struct pci_dev *pdev)
> > +{
> > +	struct pci_cap_saved_state *save_state;
> > +	u16 l1ss = pdev->l1ss;
> > +	u32 *cap;
> > +
> > +	/*
> > +	 * Save L1 substate configuration. The ASPM L0s/L1 configuration
> > +	 * is already saved in pci_save_pcie_state().
> > +	 */
> > +	if (!l1ss)
> > +		return;
> > +
> > +	save_state = pci_find_saved_ext_cap(pdev, PCI_EXT_CAP_ID_L1SS);
> > +	if (!save_state)
> > +		return;
> > +
> > +	cap = (u32 *)&save_state->cap.data[0];
> 
> Isn't .data u32 already so why cast it??

This is basically a revert of a7152be79b627428c628da2a887ca4b2512a78fd so
I left them there but you are right, those should not be needed.

> > +	pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL2, cap++);
> > +	pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, cap++);
> > +}
> > +
> > +static int aspm_l1ss_suspend_via_firmware(const struct dmi_system_id *not_used)
> > +{
> > +	return pm_suspend_via_firmware();
> > +}
> > +
> > +/*
> > + * Do not restore L1 substates for the below systems even if BIOS has enabled
> > + * it initially. This breaks resume from suspend otherwise on these.
> > + */
> > +static const struct dmi_system_id aspm_l1ss_denylist[] = {
> > +	{
> > +		/* https://bugzilla.kernel.org/show_bug.cgi?id=216782 */
> > +		.ident = "ASUS UX305FA",
> > +		.matches = {
> > +			DMI_MATCH(DMI_BOARD_VENDOR, "ASUSTeK COMPUTER INC."),
> > +			DMI_MATCH(DMI_BOARD_NAME, "UX305FA"),
> > +		},
> > +	},
> > +	{
> > +		/*
> > +		 * https://bugzilla.kernel.org/show_bug.cgi?id=216877
> > +		 *
> > +		 * This system needs to use suspend to mem instead of its
> > +		 * default (suspend to idle) to avoid draining the battery.
> > +		 * However, the BIOS gets confused if we try to restore the
> > +		 * L1SS registers so avoid doing that if the user forced
> > +		 * suspend to mem. The default suspend to idle on the other
> > +		 * hand needs restoring L1SS to allow the CPU to enter low
> > +		 * power states. This entry should handle both.
> > +		 */
> > +		.callback = aspm_l1ss_suspend_via_firmware,
> > +		.ident = "TUXEDO InfinityBook S 14 v5",
> > +		.matches = {
> > +			DMI_MATCH(DMI_BOARD_VENDOR, "TUXEDO"),
> > +			DMI_MATCH(DMI_BOARD_NAME, "L140CU"),
> > +		},
> > +	},
> > +	{ }
> > +};
> > +
> > +static bool aspm_l1ss_skip_restore(const struct pci_dev *pdev)
> > +{
> > +	const struct dmi_system_id *dmi;
> > +
> > +	dmi = dmi_first_match(aspm_l1ss_denylist);
> > +	if (dmi) {
> > +		/* If the callback returns zero we can restore L1SS */
> > +		if (dmi->callback && !dmi->callback(dmi))
> > +			return false;
> > +
> > +		pci_dbg(pdev, "skipping restoring L1 substates on this system\n");
> > +		return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +static void pcie_restore_aspm_l1ss(struct pci_dev *pdev)
> > +{
> > +	struct pci_cap_saved_state *save_state;
> > +	u32 *cap, ctl1, ctl2, l1_2_enable;
> > +	u16 l1ss = pdev->l1ss;
> > +
> > +	if (!l1ss)
> > +		return;
> > +
> > +	if (aspm_l1ss_skip_restore(pdev))
> > +		return;
> > +
> > +	save_state = pci_find_saved_ext_cap(pdev, PCI_EXT_CAP_ID_L1SS);
> > +	if (!save_state)
> > +		return;
> > +
> > +	cap = (u32 *)&save_state->cap.data[0];
> 
> The same cast here.

Same explanation :)

> > +	ctl2 = *cap++;
> > +	ctl1 = *cap;
> > +
> > +	/*
> > +	 * 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;
> > +
> > +	/* Write back without enables first (above we cleared them in ctl1) */
> > +	pci_write_config_dword(pdev, l1ss + PCI_L1SS_CTL1, ctl1);
> > +	pci_write_config_dword(pdev, l1ss + PCI_L1SS_CTL2, ctl2);
> > +
> > +	/* Then write back the enables */
> > +	if (l1_2_enable)
> > +		pci_write_config_dword(pdev, l1ss + PCI_L1SS_CTL1,
> > +				       ctl1 | l1_2_enable);
> > +}
> > +
> > +void pci_restore_aspm_state(struct pci_dev *pdev)
> > +{
> > +	struct pci_cap_saved_state *save_state;
> > +	u16 *cap, val, tmp;
> > +
> > +	save_state = pci_find_saved_cap(pdev, PCI_CAP_ID_EXP);
> > +	if (!save_state)
> > +		return;
> > +
> > +	cap = (u16 *)&save_state->cap.data[0];
> > +	/*
> > +	 * Must match the ordering in pci_save/restore_pcie_state().
> > +	 * This is PCI_EXP_LNKCTL.
> 
> I don't understand the purpose of the second sentence (or it's just
> very obvious from the name of the constant on the following line).

I tried to document that the index matching PCI_EXP_LNKCTL (that's 1) is
being used below. pci_save_pcie_state() saves bunch of registers and
this needs to match the one saved from PCI_EXP_LNKCTL.

> > +	 */
> > +	val = cap[1] & PCI_EXP_LNKCTL_ASPMC;
> > +	if (!val)
> > +		return;
> > +
> > +	/*
> > +	 * We restore L1 substate configuration first before enabling L1
> > +	 * as the PCIe spec 6.0 sec 5.5.4 suggests.
> > +	 */
> > +	pcie_restore_aspm_l1ss(pdev);
> > +
> > +	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &tmp);
> > +	/* Re-enable L0s/L1 */
> > +	pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, tmp | val);
> > +}
> > +
> >  static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
> >  {
> >  	pcie_capability_clear_and_set_word(pdev, PCI_EXP_LNKCTL,
> > 
> 
> -- 
>  i.