| Message ID | 20220826125526.28859-1-vidyas@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [V3] PCI/ASPM: Save/restore L1SS Capability for suspend/resume | expand |
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 <abhsahu@nvidia.com> > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > 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 <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.)
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 <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? Based on the update from Lukasz in https://patchwork.kernel.org/project/linux-pci/patch/20220705060014.10050-1-vidyas@nvidia.com/ (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.) >
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,
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 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(+)