On Tue, Oct 01, 2024 at 07:05:18PM +0530, Ajay Agarwal wrote:
> The current sequence in the driver for L1ss update is as follows.
>
> Disable L1ss
> Disable L1
> Enable L1ss as required
> Enable L1 if required
>
> PCIe spec r6.2, section 5.5.4, recommends that setting either
> or both of the enable bits for ASPM L1 PM Substates must be done
> while ASPM L1 is disabled. My interpretation here is that
> clearing L1ss should also be done when L1 is disabled. Thereby,
> change the sequence as follows.
>
> Disable L1
> Disable L1ss
> Enable L1ss as required
> Enable L1 if required
This seems reasonable to me. If you have seen a failure that is fixed
by this change, please include some details here.
> Signed-off-by: Ajay Agarwal <ajayagarwal@xxxxxxxxxx>
> ---
> drivers/pci/pcie/aspm.c | 22 ++++++++--------------
> 1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index cee2365e54b8..d37f66f9e9c8 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -848,16 +848,14 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
> /* Configure the ASPM L1 substates */
> static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
> {
> - u32 val, enable_req;
> + u32 val;
> struct pci_dev *child = link->downstream, *parent = link->pdev;
>
> - enable_req = (link->aspm_enabled ^ state) & state;
> -
> /*
> * Here are the rules specified in the PCIe spec for enabling L1SS:
> * - When enabling L1.x, enable bit at parent first, then at child
> * - When disabling L1.x, disable bit at child first, then at parent
> - * - When enabling ASPM L1.x, need to disable L1
> + * - When enabling/disabling ASPM L1.x, need to disable L1
> * (at child followed by parent).
> * - The ASPM/PCIPM L1.2 must be disabled while programming timing
> * parameters
Since you're updating this comment already, can you add the spec
citation here, e.g., "PCIe r6.2, sec 5.5.4"?
> @@ -866,21 +864,17 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
> * what is needed.
> */
>
> + /* Disable L1, and it gets enabled later in pcie_config_aspm_link() */
> + pcie_capability_clear_word(child, PCI_EXP_LNKCTL,
> + PCI_EXP_LNKCTL_ASPM_L1);
> + pcie_capability_clear_word(parent, PCI_EXP_LNKCTL,
> + PCI_EXP_LNKCTL_ASPM_L1);
I think it would be nice to have the disable and enable in the same
place. Could we move this to pcie_config_aspm_link()? I'm not sure
the pcie_config_aspm_dev() wrapper adds a lot of value, but we could
at least do both ends using the same wrapper.
pcie_config_aspm_link() configures all children; here we only do one
child. I suspect we should do disable L1 for all of them here, and
doing both in pcie_config_aspm_link() would make that clearer.
pcie_config_aspm_link() has a comment ("Spec 2.0 ...") about the
configuration order, but I'd like to update that, add a section
reference, and make sure we do the disable in the right order.
> /* Disable all L1 substates */
> pci_clear_and_set_config_dword(child, child->l1ss + PCI_L1SS_CTL1,
> PCI_L1SS_CTL1_L1SS_MASK, 0);
> pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
> PCI_L1SS_CTL1_L1SS_MASK, 0);
> - /*
> - * If needed, disable L1, and it gets enabled later
> - * in pcie_config_aspm_link().
> - */
> - if (enable_req & (PCIE_LINK_STATE_L1_1 | PCIE_LINK_STATE_L1_2)) {
> - pcie_capability_clear_word(child, PCI_EXP_LNKCTL,
> - PCI_EXP_LNKCTL_ASPM_L1);
> - pcie_capability_clear_word(parent, PCI_EXP_LNKCTL,
> - PCI_EXP_LNKCTL_ASPM_L1);
> - }
>
> val = 0;
> if (state & PCIE_LINK_STATE_L1_1)
> --
> 2.46.1.824.gd892dcdcdd-goog
>
