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 <[email protected]>
---
V3:
* Disabled L1.2 enable fields while restoring Control-1 register
Hi Kai-Heng,
Could you please try this patch on your setup?
Thanks & Regards,
Vidya Sagar
drivers/pci/pci.c | 7 ++++++
drivers/pci/pci.h | 4 ++++
drivers/pci/pcie/aspm.c | 50 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 61 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 95bc329e74c0..68a49fbaabde 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1663,6 +1663,7 @@ int pci_save_state(struct pci_dev *dev)
return i;
pci_save_ltr_state(dev);
+ pci_save_aspm_l1ss_state(dev);
pci_save_dpc_state(dev);
pci_save_aer_state(dev);
pci_save_ptm_state(dev);
@@ -1769,6 +1770,7 @@ void pci_restore_state(struct pci_dev *dev)
* LTR itself (in the PCIe capability).
*/
pci_restore_ltr_state(dev);
+ pci_restore_aspm_l1ss_state(dev);
pci_restore_pcie_state(dev);
pci_restore_pasid_state(dev);
@@ -3485,6 +3487,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 785f31086313..365a844ec430 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -561,10 +561,14 @@ bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
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_l1ss_state(struct pci_dev *dev);
+void pci_restore_aspm_l1ss_state(struct pci_dev *dev);
#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_l1ss_state(struct pci_dev *dev) { }
+static inline void pci_restore_aspm_l1ss_state(struct pci_dev *dev) { }
#endif
#ifdef CONFIG_PCIE_ECRC
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index a8aec190986c..ee1f651bb1af 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -726,6 +726,56 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
PCI_L1SS_CTL1_L1SS_MASK, val);
}
+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;
+
+ 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_read_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL2, cap++);
+ pci_read_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL1, cap++);
+}
+
+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);
+}
+
static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
{
pcie_capability_clear_and_set_word(pdev, PCI_EXP_LNKCTL,
--
2.17.1
On 8/26/2022 6:25 PM, 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.
>
Tested-by: Abhishek Sahu <[email protected]>
> Signed-off-by: Vidya Sagar <[email protected]>
> ---
> V3:
> * Disabled L1.2 enable fields while restoring Control-1 register
>
Thanks Vidya.
I have applied your v3 patch in 6.0-rc3 kernel and tested again.
I did 100 cycles of suspend/resume in a Alder lake based notebook
which has NVIDIA discrete GPU and it is working fine.
# lspci -d "0x10de:" -vvv|grep "L1SubCtl" -A 2
After Boot:
L1SubCtl1: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
T_CommonMode=0us LTR1.2_Threshold=753664ns
L1SubCtl2: T_PwrOn=500us
After Suspend/resume without this patch:
L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2- ASPM_L1.1-
T_CommonMode=0us LTR1.2_Threshold=0ns
L1SubCtl2: T_PwrOn=10us
After Suspend/resume with this patch:
L1SubCtl1: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
T_CommonMode=0us LTR1.2_Threshold=753664ns
L1SubCtl2: T_PwrOn=500us
So with this patch, the L1SubCtl1 and L1SubCtl2 settings are being
restored back correctly.
Regards,
Abhishek
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 <[email protected]>
> ---
> 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.)
On 9/8/2022 2:35 AM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
>
>
> 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 <[email protected]>
>> ---
>> 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?
Based on the update from Lukasz in
https://patchwork.kernel.org/project/linux-pci/patch/[email protected]/
(last update), the L1SS capability registers disappearing issue is not
due to this change and they are working on it seems.
>
>> +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?
Yup. I'll address it in the next version.
>
> 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.
I'll refactor the existing code accommodating this suggestion and push
L1SS save/restore change on top of it in the next version.
Thanks for your review comments.
>
> +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.)
>