* [PATCH 0/2] PCI/ASPM: Define consolidation
@ 2024-03-22 12:39 Ilpo Järvinen
2024-03-22 12:39 ` [PATCH 1/2] PCI/ASPM: Consolidate link state defines Ilpo Järvinen
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Ilpo Järvinen @ 2024-03-22 12:39 UTC (permalink / raw)
To: Bjorn Helgaas, linux-pci; +Cc: linux-kernel, Ilpo Järvinen
This series consolidates ASPM related defines into the ones in
linux/pci.h and eliminates the almost duplicate ones from aspm.c. This
allows using the input without mapping between the defines so the state
mapping code to focus on the special cases.
Ilpo Järvinen (2):
PCI/ASPM: Consolidate link state defines
PCI/ASPM: Cleanup ASPM disable/enable mask calculation
drivers/pci/pcie/aspm.c | 180 ++++++++++++++++++++--------------------
include/linux/pci.h | 22 ++---
2 files changed, 100 insertions(+), 102 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/2] PCI/ASPM: Consolidate link state defines
2024-03-22 12:39 [PATCH 0/2] PCI/ASPM: Define consolidation Ilpo Järvinen
@ 2024-03-22 12:39 ` Ilpo Järvinen
2024-03-22 12:39 ` [PATCH 2/2] PCI/ASPM: Cleanup ASPM disable/enable mask calculation Ilpo Järvinen
2024-05-02 22:04 ` [PATCH 0/2] PCI/ASPM: Define consolidation Bjorn Helgaas
2 siblings, 0 replies; 4+ messages in thread
From: Ilpo Järvinen @ 2024-03-22 12:39 UTC (permalink / raw)
To: Bjorn Helgaas, linux-pci, linux-kernel; +Cc: Ilpo Järvinen
The linux/pci.h and aspm.c files define own set of link state related
defines which are almost the same.
Consolidate the use of defines into those defined by linux/pci.h and
expand PCIE_LINK_STATE_L0S to match earlier ASPM_STATE_L0S that is
combination of up and down bits. Rename also the defines that are
internal to aspm.c to start with PCIE_LINK_STATE for consistency.
While PCIE_LINK_STATE_L0S BIT(0) -> (BIT(0) | BIT(1)) transformation is
not 1:1, in practice aspm.c already used ASPM_STATE_L0S that has those
both bits enabled except during mapping.
While at it, place the PCIE_LINK_STATE_CLKPM define last to have more
logical grouping.
Use static_assert() to ensure PCIE_LINK_STATE_L0S is strictly equal to
the combination of PCIE_LINK_STATE_L0S_UP/DW.
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
drivers/pci/pcie/aspm.c | 153 ++++++++++++++++++++--------------------
include/linux/pci.h | 22 +++---
2 files changed, 88 insertions(+), 87 deletions(-)
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 2428d278e015..505af49f48de 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -8,6 +8,8 @@
*/
#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/build_bug.h>
#include <linux/kernel.h>
#include <linux/limits.h>
#include <linux/math.h>
@@ -190,20 +192,17 @@ void pci_restore_aspm_l1ss_state(struct pci_dev *pdev)
#define MODULE_PARAM_PREFIX "pcie_aspm."
/* Note: those are not register definitions */
-#define ASPM_STATE_L0S_UP (1) /* Upstream direction L0s state */
-#define ASPM_STATE_L0S_DW (2) /* Downstream direction L0s state */
-#define ASPM_STATE_L1 (4) /* L1 state */
-#define ASPM_STATE_L1_1 (8) /* ASPM L1.1 state */
-#define ASPM_STATE_L1_2 (0x10) /* ASPM L1.2 state */
-#define ASPM_STATE_L1_1_PCIPM (0x20) /* PCI PM L1.1 state */
-#define ASPM_STATE_L1_2_PCIPM (0x40) /* PCI PM L1.2 state */
-#define ASPM_STATE_L1_SS_PCIPM (ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1_2_PCIPM)
-#define ASPM_STATE_L1_2_MASK (ASPM_STATE_L1_2 | ASPM_STATE_L1_2_PCIPM)
-#define ASPM_STATE_L1SS (ASPM_STATE_L1_1 | ASPM_STATE_L1_1_PCIPM |\
- ASPM_STATE_L1_2_MASK)
-#define ASPM_STATE_L0S (ASPM_STATE_L0S_UP | ASPM_STATE_L0S_DW)
-#define ASPM_STATE_ALL (ASPM_STATE_L0S | ASPM_STATE_L1 | \
- ASPM_STATE_L1SS)
+#define PCIE_LINK_STATE_L0S_UP BIT(0) /* Upstream direction L0s state */
+#define PCIE_LINK_STATE_L0S_DW BIT(1) /* Downstream direction L0s state */
+static_assert(PCIE_LINK_STATE_L0S == (PCIE_LINK_STATE_L0S_UP | PCIE_LINK_STATE_L0S_DW));
+
+#define PCIE_LINK_STATE_L1_SS_PCIPM (PCIE_LINK_STATE_L1_1_PCIPM |\
+ PCIE_LINK_STATE_L1_2_PCIPM)
+#define PCIE_LINK_STATE_L1_2_MASK (PCIE_LINK_STATE_L1_2 |\
+ PCIE_LINK_STATE_L1_2_PCIPM)
+#define PCIE_LINK_STATE_L1SS (PCIE_LINK_STATE_L1_1 |\
+ PCIE_LINK_STATE_L1_1_PCIPM |\
+ PCIE_LINK_STATE_L1_2_MASK)
struct pcie_link_state {
struct pci_dev *pdev; /* Upstream component of the Link */
@@ -275,10 +274,10 @@ static int policy_to_aspm_state(struct pcie_link_state *link)
return 0;
case POLICY_POWERSAVE:
/* Enable ASPM L0s/L1 */
- return (ASPM_STATE_L0S | ASPM_STATE_L1);
+ return PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1;
case POLICY_POWER_SUPERSAVE:
/* Enable Everything */
- return ASPM_STATE_ALL;
+ return PCIE_LINK_STATE_ASPM_ALL;
case POLICY_DEFAULT:
return link->aspm_default;
}
@@ -581,14 +580,14 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
latency_dw_l1 = calc_l1_latency(lnkcap_dw);
/* Check upstream direction L0s latency */
- if ((link->aspm_capable & ASPM_STATE_L0S_UP) &&
+ if ((link->aspm_capable & PCIE_LINK_STATE_L0S_UP) &&
(latency_up_l0s > acceptable_l0s))
- link->aspm_capable &= ~ASPM_STATE_L0S_UP;
+ link->aspm_capable &= ~PCIE_LINK_STATE_L0S_UP;
/* Check downstream direction L0s latency */
- if ((link->aspm_capable & ASPM_STATE_L0S_DW) &&
+ if ((link->aspm_capable & PCIE_LINK_STATE_L0S_DW) &&
(latency_dw_l0s > acceptable_l0s))
- link->aspm_capable &= ~ASPM_STATE_L0S_DW;
+ link->aspm_capable &= ~PCIE_LINK_STATE_L0S_DW;
/*
* Check L1 latency.
* Every switch on the path to root complex need 1
@@ -603,9 +602,9 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
* substate latencies (and hence do not do any check).
*/
latency = max_t(u32, latency_up_l1, latency_dw_l1);
- if ((link->aspm_capable & ASPM_STATE_L1) &&
+ if ((link->aspm_capable & PCIE_LINK_STATE_L1) &&
(latency + l1_switch_latency > acceptable_l1))
- link->aspm_capable &= ~ASPM_STATE_L1;
+ link->aspm_capable &= ~PCIE_LINK_STATE_L1;
l1_switch_latency += NSEC_PER_USEC;
link = link->parent;
@@ -741,13 +740,13 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
child_l1ss_cap &= ~PCI_L1SS_CAP_ASPM_L1_2;
if (parent_l1ss_cap & child_l1ss_cap & PCI_L1SS_CAP_ASPM_L1_1)
- link->aspm_support |= ASPM_STATE_L1_1;
+ link->aspm_support |= PCIE_LINK_STATE_L1_1;
if (parent_l1ss_cap & child_l1ss_cap & PCI_L1SS_CAP_ASPM_L1_2)
- link->aspm_support |= ASPM_STATE_L1_2;
+ link->aspm_support |= PCIE_LINK_STATE_L1_2;
if (parent_l1ss_cap & child_l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_1)
- link->aspm_support |= ASPM_STATE_L1_1_PCIPM;
+ link->aspm_support |= PCIE_LINK_STATE_L1_1_PCIPM;
if (parent_l1ss_cap & child_l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_2)
- link->aspm_support |= ASPM_STATE_L1_2_PCIPM;
+ link->aspm_support |= PCIE_LINK_STATE_L1_2_PCIPM;
if (parent_l1ss_cap)
pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
@@ -757,15 +756,15 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
&child_l1ss_ctl1);
if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_1)
- link->aspm_enabled |= ASPM_STATE_L1_1;
+ link->aspm_enabled |= PCIE_LINK_STATE_L1_1;
if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_2)
- link->aspm_enabled |= ASPM_STATE_L1_2;
+ link->aspm_enabled |= PCIE_LINK_STATE_L1_2;
if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_1)
- link->aspm_enabled |= ASPM_STATE_L1_1_PCIPM;
+ link->aspm_enabled |= PCIE_LINK_STATE_L1_1_PCIPM;
if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_2)
- link->aspm_enabled |= ASPM_STATE_L1_2_PCIPM;
+ link->aspm_enabled |= PCIE_LINK_STATE_L1_2_PCIPM;
- if (link->aspm_support & ASPM_STATE_L1_2_MASK)
+ if (link->aspm_support & PCIE_LINK_STATE_L1_2_MASK)
aspm_calc_l12_info(link, parent_l1ss_cap, child_l1ss_cap);
}
@@ -778,8 +777,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
if (blacklist) {
/* Set enabled/disable so that we will disable ASPM later */
- link->aspm_enabled = ASPM_STATE_ALL;
- link->aspm_disable = ASPM_STATE_ALL;
+ link->aspm_enabled = PCIE_LINK_STATE_ASPM_ALL;
+ link->aspm_disable = PCIE_LINK_STATE_ASPM_ALL;
return;
}
@@ -814,19 +813,19 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
* support L0s.
*/
if (parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)
- link->aspm_support |= ASPM_STATE_L0S;
+ link->aspm_support |= PCIE_LINK_STATE_L0S;
if (child_lnkctl & PCI_EXP_LNKCTL_ASPM_L0S)
- link->aspm_enabled |= ASPM_STATE_L0S_UP;
+ link->aspm_enabled |= PCIE_LINK_STATE_L0S_UP;
if (parent_lnkctl & PCI_EXP_LNKCTL_ASPM_L0S)
- link->aspm_enabled |= ASPM_STATE_L0S_DW;
+ link->aspm_enabled |= PCIE_LINK_STATE_L0S_DW;
/* Setup L1 state */
if (parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPM_L1)
- link->aspm_support |= ASPM_STATE_L1;
+ link->aspm_support |= PCIE_LINK_STATE_L1;
if (parent_lnkctl & child_lnkctl & PCI_EXP_LNKCTL_ASPM_L1)
- link->aspm_enabled |= ASPM_STATE_L1;
+ link->aspm_enabled |= PCIE_LINK_STATE_L1;
aspm_l1ss_init(link);
@@ -876,7 +875,7 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
* If needed, disable L1, and it gets enabled later
* in pcie_config_aspm_link().
*/
- if (enable_req & (ASPM_STATE_L1_1 | ASPM_STATE_L1_2)) {
+ 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,
@@ -884,13 +883,13 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
}
val = 0;
- if (state & ASPM_STATE_L1_1)
+ if (state & PCIE_LINK_STATE_L1_1)
val |= PCI_L1SS_CTL1_ASPM_L1_1;
- if (state & ASPM_STATE_L1_2)
+ if (state & PCIE_LINK_STATE_L1_2)
val |= PCI_L1SS_CTL1_ASPM_L1_2;
- if (state & ASPM_STATE_L1_1_PCIPM)
+ if (state & PCIE_LINK_STATE_L1_1_PCIPM)
val |= PCI_L1SS_CTL1_PCIPM_L1_1;
- if (state & ASPM_STATE_L1_2_PCIPM)
+ if (state & PCIE_LINK_STATE_L1_2_PCIPM)
val |= PCI_L1SS_CTL1_PCIPM_L1_2;
/* Enable what we need to enable */
@@ -916,29 +915,29 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
state &= (link->aspm_capable & ~link->aspm_disable);
/* Can't enable any substates if L1 is not enabled */
- if (!(state & ASPM_STATE_L1))
- state &= ~ASPM_STATE_L1SS;
+ if (!(state & PCIE_LINK_STATE_L1))
+ state &= ~PCIE_LINK_STATE_L1SS;
/* Spec says both ports must be in D0 before enabling PCI PM substates*/
if (parent->current_state != PCI_D0 || child->current_state != PCI_D0) {
- state &= ~ASPM_STATE_L1_SS_PCIPM;
- state |= (link->aspm_enabled & ASPM_STATE_L1_SS_PCIPM);
+ state &= ~PCIE_LINK_STATE_L1_SS_PCIPM;
+ state |= (link->aspm_enabled & PCIE_LINK_STATE_L1_SS_PCIPM);
}
/* Nothing to do if the link is already in the requested state */
if (link->aspm_enabled == state)
return;
/* Convert ASPM state to upstream/downstream ASPM register state */
- if (state & ASPM_STATE_L0S_UP)
+ if (state & PCIE_LINK_STATE_L0S_UP)
dwstream |= PCI_EXP_LNKCTL_ASPM_L0S;
- if (state & ASPM_STATE_L0S_DW)
+ if (state & PCIE_LINK_STATE_L0S_DW)
upstream |= PCI_EXP_LNKCTL_ASPM_L0S;
- if (state & ASPM_STATE_L1) {
+ if (state & PCIE_LINK_STATE_L1) {
upstream |= PCI_EXP_LNKCTL_ASPM_L1;
dwstream |= PCI_EXP_LNKCTL_ASPM_L1;
}
- if (link->aspm_capable & ASPM_STATE_L1SS)
+ if (link->aspm_capable & PCIE_LINK_STATE_L1SS)
pcie_config_aspm_l1ss(link, state);
/*
@@ -947,11 +946,11 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
* upstream component first and then downstream, and vice
* versa for disabling ASPM L1. Spec doesn't mention L0S.
*/
- if (state & ASPM_STATE_L1)
+ if (state & PCIE_LINK_STATE_L1)
pcie_config_aspm_dev(parent, upstream);
list_for_each_entry(child, &linkbus->devices, bus_list)
pcie_config_aspm_dev(child, dwstream);
- if (!(state & ASPM_STATE_L1))
+ if (!(state & PCIE_LINK_STATE_L1))
pcie_config_aspm_dev(parent, upstream);
link->aspm_enabled = state;
@@ -1347,18 +1346,18 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool locked
down_read(&pci_bus_sem);
mutex_lock(&aspm_lock);
if (state & PCIE_LINK_STATE_L0S)
- link->aspm_disable |= ASPM_STATE_L0S;
+ link->aspm_disable |= PCIE_LINK_STATE_L0S;
if (state & PCIE_LINK_STATE_L1)
/* L1 PM substates require L1 */
- link->aspm_disable |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
+ link->aspm_disable |= PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L1SS;
if (state & PCIE_LINK_STATE_L1_1)
- link->aspm_disable |= ASPM_STATE_L1_1;
+ link->aspm_disable |= PCIE_LINK_STATE_L1_1;
if (state & PCIE_LINK_STATE_L1_2)
- link->aspm_disable |= ASPM_STATE_L1_2;
+ link->aspm_disable |= PCIE_LINK_STATE_L1_2;
if (state & PCIE_LINK_STATE_L1_1_PCIPM)
- link->aspm_disable |= ASPM_STATE_L1_1_PCIPM;
+ link->aspm_disable |= PCIE_LINK_STATE_L1_1_PCIPM;
if (state & PCIE_LINK_STATE_L1_2_PCIPM)
- link->aspm_disable |= ASPM_STATE_L1_2_PCIPM;
+ link->aspm_disable |= PCIE_LINK_STATE_L1_2_PCIPM;
pcie_config_aspm_link(link, policy_to_aspm_state(link));
if (state & PCIE_LINK_STATE_CLKPM)
@@ -1416,18 +1415,18 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
mutex_lock(&aspm_lock);
link->aspm_default = 0;
if (state & PCIE_LINK_STATE_L0S)
- link->aspm_default |= ASPM_STATE_L0S;
+ link->aspm_default |= PCIE_LINK_STATE_L0S;
if (state & PCIE_LINK_STATE_L1)
- link->aspm_default |= ASPM_STATE_L1;
+ link->aspm_default |= PCIE_LINK_STATE_L1;
/* L1 PM substates require L1 */
if (state & PCIE_LINK_STATE_L1_1)
- link->aspm_default |= ASPM_STATE_L1_1 | ASPM_STATE_L1;
+ link->aspm_default |= PCIE_LINK_STATE_L1_1 | PCIE_LINK_STATE_L1;
if (state & PCIE_LINK_STATE_L1_2)
- link->aspm_default |= ASPM_STATE_L1_2 | ASPM_STATE_L1;
+ link->aspm_default |= PCIE_LINK_STATE_L1_2 | PCIE_LINK_STATE_L1;
if (state & PCIE_LINK_STATE_L1_1_PCIPM)
- link->aspm_default |= ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1;
+ link->aspm_default |= PCIE_LINK_STATE_L1_1_PCIPM | PCIE_LINK_STATE_L1;
if (state & PCIE_LINK_STATE_L1_2_PCIPM)
- link->aspm_default |= ASPM_STATE_L1_2_PCIPM | ASPM_STATE_L1;
+ link->aspm_default |= PCIE_LINK_STATE_L1_2_PCIPM | PCIE_LINK_STATE_L1;
pcie_config_aspm_link(link, policy_to_aspm_state(link));
link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
@@ -1563,12 +1562,12 @@ static ssize_t aspm_attr_store_common(struct device *dev,
if (state_enable) {
link->aspm_disable &= ~state;
/* need to enable L1 for substates */
- if (state & ASPM_STATE_L1SS)
- link->aspm_disable &= ~ASPM_STATE_L1;
+ if (state & PCIE_LINK_STATE_L1SS)
+ link->aspm_disable &= ~PCIE_LINK_STATE_L1;
} else {
link->aspm_disable |= state;
- if (state & ASPM_STATE_L1)
- link->aspm_disable |= ASPM_STATE_L1SS;
+ if (state & PCIE_LINK_STATE_L1)
+ link->aspm_disable |= PCIE_LINK_STATE_L1SS;
}
pcie_config_aspm_link(link, policy_to_aspm_state(link));
@@ -1582,12 +1581,12 @@ static ssize_t aspm_attr_store_common(struct device *dev,
#define ASPM_ATTR(_f, _s) \
static ssize_t _f##_show(struct device *dev, \
struct device_attribute *attr, char *buf) \
-{ return aspm_attr_show_common(dev, attr, buf, ASPM_STATE_##_s); } \
+{ return aspm_attr_show_common(dev, attr, buf, PCIE_LINK_STATE_##_s); } \
\
static ssize_t _f##_store(struct device *dev, \
struct device_attribute *attr, \
const char *buf, size_t len) \
-{ return aspm_attr_store_common(dev, attr, buf, len, ASPM_STATE_##_s); }
+{ return aspm_attr_store_common(dev, attr, buf, len, PCIE_LINK_STATE_##_s); }
ASPM_ATTR(l0s_aspm, L0S)
ASPM_ATTR(l1_aspm, L1)
@@ -1654,12 +1653,12 @@ static umode_t aspm_ctrl_attrs_are_visible(struct kobject *kobj,
struct pci_dev *pdev = to_pci_dev(dev);
struct pcie_link_state *link = pcie_aspm_get_link(pdev);
static const u8 aspm_state_map[] = {
- ASPM_STATE_L0S,
- ASPM_STATE_L1,
- ASPM_STATE_L1_1,
- ASPM_STATE_L1_2,
- ASPM_STATE_L1_1_PCIPM,
- ASPM_STATE_L1_2_PCIPM,
+ PCIE_LINK_STATE_L0S,
+ PCIE_LINK_STATE_L1,
+ PCIE_LINK_STATE_L1_1,
+ PCIE_LINK_STATE_L1_2,
+ PCIE_LINK_STATE_L1_1_PCIPM,
+ PCIE_LINK_STATE_L1_2_PCIPM,
};
if (aspm_disabled || !link)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 16493426a04f..4c46ccb28b98 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1821,17 +1821,19 @@ extern bool pcie_ports_native;
#define pcie_ports_native false
#endif
-#define PCIE_LINK_STATE_L0S BIT(0)
-#define PCIE_LINK_STATE_L1 BIT(1)
-#define PCIE_LINK_STATE_CLKPM BIT(2)
-#define PCIE_LINK_STATE_L1_1 BIT(3)
-#define PCIE_LINK_STATE_L1_2 BIT(4)
-#define PCIE_LINK_STATE_L1_1_PCIPM BIT(5)
-#define PCIE_LINK_STATE_L1_2_PCIPM BIT(6)
-#define PCIE_LINK_STATE_ALL (PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |\
- PCIE_LINK_STATE_CLKPM | PCIE_LINK_STATE_L1_1 |\
- PCIE_LINK_STATE_L1_2 | PCIE_LINK_STATE_L1_1_PCIPM |\
+#define PCIE_LINK_STATE_L0S (BIT(0) | BIT(1)) /* Upstream & downstream L0s */
+#define PCIE_LINK_STATE_L1 BIT(2) /* L1 state */
+#define PCIE_LINK_STATE_L1_1 BIT(3) /* ASPM L1.1 state */
+#define PCIE_LINK_STATE_L1_2 BIT(4) /* ASPM L1.2 state */
+#define PCIE_LINK_STATE_L1_1_PCIPM BIT(5) /* PCI PM L1.1 state */
+#define PCIE_LINK_STATE_L1_2_PCIPM BIT(6) /* PCI PM L1.2 state */
+#define PCIE_LINK_STATE_ASPM_ALL (PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |\
+ PCIE_LINK_STATE_L1_1 | PCIE_LINK_STATE_L1_2 |\
+ PCIE_LINK_STATE_L1_1_PCIPM |\
PCIE_LINK_STATE_L1_2_PCIPM)
+#define PCIE_LINK_STATE_CLKPM BIT(7)
+#define PCIE_LINK_STATE_ALL (PCIE_LINK_STATE_ASPM_ALL |\
+ PCIE_LINK_STATE_CLKPM)
#ifdef CONFIG_PCIEASPM
int pci_disable_link_state(struct pci_dev *pdev, int state);
--
2.39.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] PCI/ASPM: Cleanup ASPM disable/enable mask calculation
2024-03-22 12:39 [PATCH 0/2] PCI/ASPM: Define consolidation Ilpo Järvinen
2024-03-22 12:39 ` [PATCH 1/2] PCI/ASPM: Consolidate link state defines Ilpo Järvinen
@ 2024-03-22 12:39 ` Ilpo Järvinen
2024-05-02 22:04 ` [PATCH 0/2] PCI/ASPM: Define consolidation Bjorn Helgaas
2 siblings, 0 replies; 4+ messages in thread
From: Ilpo Järvinen @ 2024-03-22 12:39 UTC (permalink / raw)
To: Bjorn Helgaas, linux-pci, linux-kernel; +Cc: Ilpo Järvinen
With only one set of defines remaining, state can be almost used as is
to set ->aspm_disable/default. Only CLKPM and L1 PM substates need
special handling.
Remove unnecessary if conditions that can use the state variable bits
directly. Move the ASPM mask calculation code into
pci_calc_aspm_{disable,enable}_mask() helpers which makes it easier to
alter state variable directly.
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
drivers/pci/pcie/aspm.c | 51 +++++++++++++++++++----------------------
1 file changed, 24 insertions(+), 27 deletions(-)
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 505af49f48de..14d13fc519d8 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1323,6 +1323,28 @@ static struct pcie_link_state *pcie_aspm_get_link(struct pci_dev *pdev)
return bridge->link_state;
}
+static u8 pci_calc_aspm_disable_mask(int state)
+{
+ state &= ~PCIE_LINK_STATE_CLKPM;
+
+ /* L1 PM substates require L1 */
+ if (state & PCIE_LINK_STATE_L1)
+ state |= PCIE_LINK_STATE_L1SS;
+
+ return state;
+}
+
+static u8 pci_calc_aspm_enable_mask(int state)
+{
+ state &= ~PCIE_LINK_STATE_CLKPM;
+
+ /* L1 PM substates require L1 */
+ if (state & PCIE_LINK_STATE_L1SS)
+ state |= PCIE_LINK_STATE_L1;
+
+ return state;
+}
+
static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool locked)
{
struct pcie_link_state *link = pcie_aspm_get_link(pdev);
@@ -1345,19 +1367,7 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool locked
if (!locked)
down_read(&pci_bus_sem);
mutex_lock(&aspm_lock);
- if (state & PCIE_LINK_STATE_L0S)
- link->aspm_disable |= PCIE_LINK_STATE_L0S;
- if (state & PCIE_LINK_STATE_L1)
- /* L1 PM substates require L1 */
- link->aspm_disable |= PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L1SS;
- if (state & PCIE_LINK_STATE_L1_1)
- link->aspm_disable |= PCIE_LINK_STATE_L1_1;
- if (state & PCIE_LINK_STATE_L1_2)
- link->aspm_disable |= PCIE_LINK_STATE_L1_2;
- if (state & PCIE_LINK_STATE_L1_1_PCIPM)
- link->aspm_disable |= PCIE_LINK_STATE_L1_1_PCIPM;
- if (state & PCIE_LINK_STATE_L1_2_PCIPM)
- link->aspm_disable |= PCIE_LINK_STATE_L1_2_PCIPM;
+ link->aspm_disable |= pci_calc_aspm_disable_mask(state);
pcie_config_aspm_link(link, policy_to_aspm_state(link));
if (state & PCIE_LINK_STATE_CLKPM)
@@ -1413,20 +1423,7 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
if (!locked)
down_read(&pci_bus_sem);
mutex_lock(&aspm_lock);
- link->aspm_default = 0;
- if (state & PCIE_LINK_STATE_L0S)
- link->aspm_default |= PCIE_LINK_STATE_L0S;
- if (state & PCIE_LINK_STATE_L1)
- link->aspm_default |= PCIE_LINK_STATE_L1;
- /* L1 PM substates require L1 */
- if (state & PCIE_LINK_STATE_L1_1)
- link->aspm_default |= PCIE_LINK_STATE_L1_1 | PCIE_LINK_STATE_L1;
- if (state & PCIE_LINK_STATE_L1_2)
- link->aspm_default |= PCIE_LINK_STATE_L1_2 | PCIE_LINK_STATE_L1;
- if (state & PCIE_LINK_STATE_L1_1_PCIPM)
- link->aspm_default |= PCIE_LINK_STATE_L1_1_PCIPM | PCIE_LINK_STATE_L1;
- if (state & PCIE_LINK_STATE_L1_2_PCIPM)
- link->aspm_default |= PCIE_LINK_STATE_L1_2_PCIPM | PCIE_LINK_STATE_L1;
+ link->aspm_default = pci_calc_aspm_enable_mask(state);
pcie_config_aspm_link(link, policy_to_aspm_state(link));
link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
--
2.39.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 0/2] PCI/ASPM: Define consolidation
2024-03-22 12:39 [PATCH 0/2] PCI/ASPM: Define consolidation Ilpo Järvinen
2024-03-22 12:39 ` [PATCH 1/2] PCI/ASPM: Consolidate link state defines Ilpo Järvinen
2024-03-22 12:39 ` [PATCH 2/2] PCI/ASPM: Cleanup ASPM disable/enable mask calculation Ilpo Järvinen
@ 2024-05-02 22:04 ` Bjorn Helgaas
2 siblings, 0 replies; 4+ messages in thread
From: Bjorn Helgaas @ 2024-05-02 22:04 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: Bjorn Helgaas, linux-pci, linux-kernel
On Fri, Mar 22, 2024 at 02:39:50PM +0200, Ilpo Järvinen wrote:
> This series consolidates ASPM related defines into the ones in
> linux/pci.h and eliminates the almost duplicate ones from aspm.c. This
> allows using the input without mapping between the defines so the state
> mapping code to focus on the special cases.
>
> Ilpo Järvinen (2):
> PCI/ASPM: Consolidate link state defines
> PCI/ASPM: Cleanup ASPM disable/enable mask calculation
>
> drivers/pci/pcie/aspm.c | 180 ++++++++++++++++++++--------------------
> include/linux/pci.h | 22 ++---
> 2 files changed, 100 insertions(+), 102 deletions(-)
Applied to pci/aspm for v6.10, thanks!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-05-02 22:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-22 12:39 [PATCH 0/2] PCI/ASPM: Define consolidation Ilpo Järvinen
2024-03-22 12:39 ` [PATCH 1/2] PCI/ASPM: Consolidate link state defines Ilpo Järvinen
2024-03-22 12:39 ` [PATCH 2/2] PCI/ASPM: Cleanup ASPM disable/enable mask calculation Ilpo Järvinen
2024-05-02 22:04 ` [PATCH 0/2] PCI/ASPM: Define consolidation Bjorn Helgaas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.