[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <b1933ca4-13a8-83a0-d9f3-1ecea467948d@nvidia.com>
Date: Tue, 13 Sep 2022 18:45:43 +0530
From: Vidya Sagar <vidyas@...dia.com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: bhelgaas@...gle.com, lorenzo.pieralisi@....com,
refactormyself@...il.com, kw@...ux.com, rajatja@...gle.com,
kenny@...ix.com, kai.heng.feng@...onical.com, treding@...dia.com,
jonathanh@...dia.com, abhsahu@...dia.com, sagupta@...dia.com,
benchuanggli@...il.com, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org, kthota@...dia.com,
mmaddireddy@...dia.com, sagar.tv@...il.com
Subject: Re: [PATCH V3] PCI/ASPM: Save/restore L1SS Capability for
suspend/resume
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@...dia.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.)
>
Powered by blists - more mailing lists