On Mon, Oct 07, 2024 at 08:59:17AM +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
>
> With this sequence, a bus hang is observed during the L1ss
> disable sequence when the RC CPU attempts to clear the RC L1ss
> register after clearing the EP L1ss register. It looks like the
> RC attempts to enter L1ss again and at the same time, access to
> RC L1ss register fails because aux clk is still not active.
>
> 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
>
> Signed-off-by: Ajay Agarwal <ajayagarwal@xxxxxxxxxx>
Applied to pci/aspm for v6.13, thank you, Ajay!
> ---
> Changelog since v2:
> - Add the logic in pcie_aspm_cap_init()
>
> Changelog since v1:
> - Move the L1 disable/enable logic to pcie_config_aspm_link()
> - Add detailed comments including spec version and section number
>
> drivers/pci/pcie/aspm.c | 66 +++++++++++++++++++++++++----------------
> 1 file changed, 40 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index cee2365e54b8..50997e378631 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -805,6 +805,15 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
> pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &parent_lnkctl);
> pcie_capability_read_word(child, PCI_EXP_LNKCTL, &child_lnkctl);
>
> + /* Make sure L0s/L1 are disabled before updating L1SS config */
> + if (FIELD_GET(PCI_EXP_LNKCTL_ASPMC, child_lnkctl) ||
> + FIELD_GET(PCI_EXP_LNKCTL_ASPMC, parent_lnkctl)) {
> + pcie_capability_write_word(child, PCI_EXP_LNKCTL,
> + child_lnkctl & ~PCI_EXP_LNKCTL_ASPMC);
> + pcie_capability_write_word(parent, PCI_EXP_LNKCTL,
> + parent_lnkctl & ~PCI_EXP_LNKCTL_ASPMC);
> + }
> +
> /*
> * Setup L0s state
> *
> @@ -829,6 +838,13 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>
> aspm_l1ss_init(link);
>
> + /* Restore L0s/L1 if they were enabled */
> + if (FIELD_GET(PCI_EXP_LNKCTL_ASPMC, child_lnkctl) ||
> + FIELD_GET(PCI_EXP_LNKCTL_ASPMC, parent_lnkctl)) {
> + pcie_capability_write_word(parent, PCI_EXP_LNKCTL, parent_lnkctl);
> + pcie_capability_write_word(child, PCI_EXP_LNKCTL, child_lnkctl);
> + }
> +
> /* Save default state */
> link->aspm_default = link->aspm_enabled;
>
> @@ -848,17 +864,13 @@ 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:
> + * Spec r6.2, section 5.5.4, mentions the rules 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
> - * (at child followed by parent).
> * - The ASPM/PCIPM L1.2 must be disabled while programming timing
> * parameters
> *
> @@ -871,16 +883,6 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
> 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)
> @@ -937,21 +939,33 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
> dwstream |= PCI_EXP_LNKCTL_ASPM_L1;
> }
>
> + /*
> + * 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. So disable L1 here, and it gets enabled later after the
> + * L1ss configuration has been completed.
> + *
> + * Spec r6.2, section 7.5.3.7, mentions that ASPM L1 must be enabled by
> + * software in the Upstream component on a Link prior to enabling ASPM
> + * L1 in the Downstream component on the Link. When disabling L1,
> + * software must disable ASPM L1 in the Downstream component on a Link
> + * prior to disabling ASPM L1 in the Upstream component on that Link.
> + *
> + * Spec doesn't mention L0s.
> + *
> + * Disable L1 and L0s here, and they get enabled later after the L1ss
> + * configuration has been completed.
> + */
> + list_for_each_entry(child, &linkbus->devices, bus_list)
> + pcie_config_aspm_dev(child, 0);
> + pcie_config_aspm_dev(parent, 0);
> +
> if (link->aspm_capable & PCIE_LINK_STATE_L1SS)
> pcie_config_aspm_l1ss(link, state);
>
> - /*
> - * Spec 2.0 suggests all functions should be configured the
> - * same setting for ASPM. Enabling ASPM L1 should be done in
> - * upstream component first and then downstream, and vice
> - * versa for disabling ASPM L1. Spec doesn't mention L0S.
> - */
> - if (state & PCIE_LINK_STATE_L1)
> - pcie_config_aspm_dev(parent, upstream);
> + pcie_config_aspm_dev(parent, upstream);
> list_for_each_entry(child, &linkbus->devices, bus_list)
> pcie_config_aspm_dev(child, dwstream);
> - if (!(state & PCIE_LINK_STATE_L1))
> - pcie_config_aspm_dev(parent, upstream);
>
> link->aspm_enabled = state;
>
> --
> 2.47.0.rc0.187.ge670bccf7e-goog
>
