PCI/ASPM: Disable L1 before disabling L1 PM Substates

PCIe r6.2, sec 5.5.4, requires that:

  If setting either or both of the enable bits for ASPM L1 PM Substates,
  both ports must be configured as described in this section while ASPM L1
  is disabled.

Previously, pcie_config_aspm_l1ss() assumed that "setting enable bits"
meant "setting them to 1", and it configured L1SS as follows:

  - Clear L1SS enable bits
  - Disable L1
  - Configure L1SS enable bits as required
  - Enable L1 if required

With this sequence, when disabling L1SS on an ARM A-core with a Synopsys
DesignWare PCIe core, the CPU occasionally hangs when reading
PCI_L1SS_CTL1, leading to a reboot when the CPU watchdog expires.

Move the L1 disable to the caller (pcie_config_aspm_link(), where L1 was
already enabled) so L1 is always disabled while updating the L1SS bits:

  - Disable L1
  - Clear L1SS enable bits
  - Configure L1SS enable bits as required
  - Enable L1 if required

Change pcie_aspm_cap_init() similarly.

Link: https://lore.kernel.org/r/20241007032917.872262-1-ajayagarwal@google.com
Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
[bhelgaas: comments, commit log, compute L1SS setting before config access]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index cee2365e54b8b23d270f9e6fbd1ead201fda6abb..e943691bc931721874311914f805fe5575c423b4 100644 (file)
--- 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);
 
+       /* Disable L0s/L1 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;
 
@@ -845,25 +861,28 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
        }
 }
 
-/* Configure the ASPM L1 substates */
+/* Configure the ASPM L1 substates. Caller must disable L1 first. */
 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;
+       val = 0;
+       if (state & PCIE_LINK_STATE_L1_1)
+               val |= PCI_L1SS_CTL1_ASPM_L1_1;
+       if (state & PCIE_LINK_STATE_L1_2)
+               val |= PCI_L1SS_CTL1_ASPM_L1_2;
+       if (state & PCIE_LINK_STATE_L1_1_PCIPM)
+               val |= PCI_L1SS_CTL1_PCIPM_L1_1;
+       if (state & PCIE_LINK_STATE_L1_2_PCIPM)
+               val |= PCI_L1SS_CTL1_PCIPM_L1_2;
 
        /*
-        * 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
-        *   (at child followed by parent).
-        * - The ASPM/PCIPM L1.2 must be disabled while programming timing
+        * PCIe r6.2, sec 5.5.4, rules for enabling L1 PM Substates:
+        * - Clear L1.x enable bits at child first, then at parent
+        * - Set L1.x enable bits at parent first, then at child
+        * - ASPM/PCIPM L1.2 must be disabled while programming timing
         *   parameters
-        *
-        * To keep it simple, disable all L1SS bits first, and later enable
-        * what is needed.
         */
 
        /* Disable all L1 substates */
@@ -871,26 +890,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)
-               val |= PCI_L1SS_CTL1_ASPM_L1_1;
-       if (state & PCIE_LINK_STATE_L1_2)
-               val |= PCI_L1SS_CTL1_ASPM_L1_2;
-       if (state & PCIE_LINK_STATE_L1_1_PCIPM)
-               val |= PCI_L1SS_CTL1_PCIPM_L1_1;
-       if (state & PCIE_LINK_STATE_L1_2_PCIPM)
-               val |= PCI_L1SS_CTL1_PCIPM_L1_2;
 
        /* Enable what we need to enable */
        pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
@@ -937,21 +936,30 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
                dwstream |= PCI_EXP_LNKCTL_ASPM_L1;
        }
 
+       /*
+        * Per PCIe r6.2, sec 5.5.4, setting either or both of the enable
+        * bits for ASPM L1 PM Substates must be done while ASPM L1 is
+        * disabled. Disable L1 here and apply new configuration after L1SS
+        * configuration has been completed.
+        *
+        * Per sec 7.5.3.7, when disabling ASPM L1, software must disable
+        * it in the Downstream component prior to disabling it in the
+        * Upstream component, and ASPM L1 must be enabled in the Upstream
+        * component prior to enabling it in the Downstream component.
+        *
+        * Sec 7.5.3.7 also recommends programming the same ASPM Control
+        * value for all functions of a multi-function device.
+        */
+       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;