linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 00/12] PCI/ASPM: Cleanup
@ 2020-10-15 19:30 Bjorn Helgaas
  2020-10-15 19:30 ` [PATCH v3 01/12] PCI/ASPM: Move pci_clear_and_set_dword() earlier Bjorn Helgaas
                   ` (11 more replies)
  0 siblings, 12 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2020-10-15 19:30 UTC (permalink / raw)
  To: Saheed O . Bolarinwa
  Cc: Puranjay Mohan, Rajat Jain, Kai-Heng Feng, Yicong Yang,
	Heiner Kallweit, linux-pci, Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

This is a v3 posting of Saheed's ASPM cleanup.

The intent is that this is strictly cleanup, no functional changes at all.
We want to simplify the code by removing struct aspm_register_info and
pcie_get_aspm_reg().  These are only used to read and store register info,
but the info is only used in one place, so the function and struct only
make things more complicated.

Previous postings:

v2: https://lore.kernel.org/r/20200924142443.260861-2-refactormyself@gmail.com
v1: https://lore.kernel.org/r/20200923231517.221310-1-refactormyself@gmail.com

Bjorn Helgaas (5):
  PCI/ASPM: Move pci_clear_and_set_dword() earlier
  PCI/ASPM: Move LTR path check to where it's used
  PCI/ASPM: Use 'parent' and 'child' for readability
  PCI/ASPM: Remove struct aspm_register_info.l1ss_ctl2 (unused)
  PCI/ASPM: Pass L1SS Capabilities value, not struct aspm_register_info

Saheed O. Bolarinwa (7):
  PCI/ASPM: Remove struct aspm_register_info.support
  PCI/ASPM: Remove struct aspm_register_info.enabled
  PCI/ASPM: Remove struct aspm_register_info.latency_encoding
  PCI/ASPM: Remove struct aspm_register_info.l1ss_cap_ptr
  PCI/ASPM: Remove struct aspm_register_info.l1ss_ctl1
  PCI/ASPM: Remove struct aspm_register_info.l1ss_cap
  PCI/ASPM: Remove struct pcie_link_state.l1ss

 drivers/pci/pcie/aspm.c       | 265 +++++++++++++++-------------------
 drivers/pci/probe.c           |   3 +
 include/linux/pci.h           |   1 +
 include/uapi/linux/pci_regs.h |   2 +
 4 files changed, 120 insertions(+), 151 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 01/12] PCI/ASPM: Move pci_clear_and_set_dword() earlier
  2020-10-15 19:30 [PATCH v3 00/12] PCI/ASPM: Cleanup Bjorn Helgaas
@ 2020-10-15 19:30 ` Bjorn Helgaas
  2020-10-15 19:30 ` [PATCH v3 02/12] PCI/ASPM: Move LTR path check to where it's used Bjorn Helgaas
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2020-10-15 19:30 UTC (permalink / raw)
  To: Saheed O . Bolarinwa
  Cc: Puranjay Mohan, Rajat Jain, Kai-Heng Feng, Yicong Yang,
	Heiner Kallweit, linux-pci, Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

Move pci_clear_and_set_dword() earlier in file to prepare for future patch.
No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 253c30cc1967..237a423e53ae 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -493,6 +493,17 @@ static struct pci_dev *pci_function_0(struct pci_bus *linkbus)
 	return NULL;
 }
 
+static void pci_clear_and_set_dword(struct pci_dev *pdev, int pos,
+				    u32 clear, u32 set)
+{
+	u32 val;
+
+	pci_read_config_dword(pdev, pos, &val);
+	val &= ~clear;
+	val |= set;
+	pci_write_config_dword(pdev, pos, val);
+}
+
 /* Calculate L1.2 PM substate timing parameters */
 static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 				struct aspm_register_info *upreg,
@@ -651,17 +662,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	}
 }
 
-static void pci_clear_and_set_dword(struct pci_dev *pdev, int pos,
-				    u32 clear, u32 set)
-{
-	u32 val;
-
-	pci_read_config_dword(pdev, pos, &val);
-	val &= ~clear;
-	val |= set;
-	pci_write_config_dword(pdev, pos, val);
-}
-
 /* Configure the ASPM L1 substates */
 static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
 {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 02/12] PCI/ASPM: Move LTR path check to where it's used
  2020-10-15 19:30 [PATCH v3 00/12] PCI/ASPM: Cleanup Bjorn Helgaas
  2020-10-15 19:30 ` [PATCH v3 01/12] PCI/ASPM: Move pci_clear_and_set_dword() earlier Bjorn Helgaas
@ 2020-10-15 19:30 ` Bjorn Helgaas
  2020-10-15 19:30 ` [PATCH v3 03/12] PCI/ASPM: Use 'parent' and 'child' for readability Bjorn Helgaas
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2020-10-15 19:30 UTC (permalink / raw)
  To: Saheed O . Bolarinwa
  Cc: Puranjay Mohan, Rajat Jain, Kai-Heng Feng, Yicong Yang,
	Heiner Kallweit, linux-pci, Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

pcie_get_aspm_reg() mostly reads ASPM-related registers, but in some cases
it also updates the value read from PCI_L1SS_CAP based on LTR properties.

Move this update to the point where the value is used to make the code more
readable.

No functional change intended, although previously we could clear
PCI_L1SS_CAP_ASPM_L1_2 for both ends of the link, and now we'll only do it
for the downstream end of a link.  This shouldn't matter because we always
test that bit by ANDing l1ss_cap for the upstream and downstream ends.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 237a423e53ae..386b45eb79ba 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -418,14 +418,6 @@ static void pcie_get_aspm_reg(struct pci_dev *pdev,
 		return;
 	}
 
-	/*
-	 * If we don't have LTR for the entire path from the Root Complex
-	 * to this device, we can't use ASPM L1.2 because it relies on the
-	 * LTR_L1.2_THRESHOLD.  See PCIe r4.0, secs 5.5.4, 6.18.
-	 */
-	if (!pdev->ltr_path)
-		info->l1ss_cap &= ~PCI_L1SS_CAP_ASPM_L1_2;
-
 	pci_read_config_dword(pdev, info->l1ss_cap_ptr + PCI_L1SS_CTL1,
 			      &info->l1ss_ctl1);
 	pci_read_config_dword(pdev, info->l1ss_cap_ptr + PCI_L1SS_CTL2,
@@ -612,7 +604,14 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	link->latency_up.l1 = calc_l1_latency(upreg.latency_encoding_l1);
 	link->latency_dw.l1 = calc_l1_latency(dwreg.latency_encoding_l1);
 
-	/* Setup L1 substate */
+	/* Setup L1 substate
+	 * If we don't have LTR for the entire path from the Root Complex
+	 * to this device, we can't use ASPM L1.2 because it relies on the
+	 * LTR_L1.2_THRESHOLD.  See PCIe r4.0, secs 5.5.4, 6.18.
+	 */
+	if (!child->ltr_path)
+		dwreg.l1ss_cap &= ~PCI_L1SS_CAP_ASPM_L1_2;
+
 	if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_ASPM_L1_1)
 		link->aspm_support |= ASPM_STATE_L1_1;
 	if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_ASPM_L1_2)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 03/12] PCI/ASPM: Use 'parent' and 'child' for readability
  2020-10-15 19:30 [PATCH v3 00/12] PCI/ASPM: Cleanup Bjorn Helgaas
  2020-10-15 19:30 ` [PATCH v3 01/12] PCI/ASPM: Move pci_clear_and_set_dword() earlier Bjorn Helgaas
  2020-10-15 19:30 ` [PATCH v3 02/12] PCI/ASPM: Move LTR path check to where it's used Bjorn Helgaas
@ 2020-10-15 19:30 ` Bjorn Helgaas
  2020-10-15 19:30 ` [PATCH v3 04/12] PCI/ASPM: Remove struct aspm_register_info.support Bjorn Helgaas
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2020-10-15 19:30 UTC (permalink / raw)
  To: Saheed O . Bolarinwa
  Cc: Puranjay Mohan, Rajat Jain, Kai-Heng Feng, Yicong Yang,
	Heiner Kallweit, linux-pci, Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

Other users of link->pdev and link->downstream, e.g., pcie_aspm_cap_init(),
pcie_config_aspm_l1ss(), and pcie_config_aspm_link(), use "parent" and
"child" as local names.

Do the same in aspm_calc_l1ss_info() for readability.  No functional change
intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 386b45eb79ba..0725511cbeb5 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -501,6 +501,7 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 				struct aspm_register_info *upreg,
 				struct aspm_register_info *dwreg)
 {
+	struct pci_dev *child = link->downstream, *parent = link->pdev;
 	u32 val1, val2, scale1, scale2;
 	u32 t_common_mode, t_power_on, l1_2_threshold, scale, value;
 
@@ -522,13 +523,13 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 	val2   = (dwreg->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
 	scale2 = (dwreg->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
 
-	if (calc_l1ss_pwron(link->pdev, scale1, val1) >
-	    calc_l1ss_pwron(link->downstream, scale2, val2)) {
+	if (calc_l1ss_pwron(parent, scale1, val1) >
+	    calc_l1ss_pwron(child, scale2, val2)) {
 		link->l1ss.ctl2 |= scale1 | (val1 << 3);
-		t_power_on = calc_l1ss_pwron(link->pdev, scale1, val1);
+		t_power_on = calc_l1ss_pwron(parent, scale1, val1);
 	} else {
 		link->l1ss.ctl2 |= scale2 | (val2 << 3);
-		t_power_on = calc_l1ss_pwron(link->downstream, scale2, val2);
+		t_power_on = calc_l1ss_pwron(child, scale2, val2);
 	}
 
 	/*
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 04/12] PCI/ASPM: Remove struct aspm_register_info.support
  2020-10-15 19:30 [PATCH v3 00/12] PCI/ASPM: Cleanup Bjorn Helgaas
                   ` (2 preceding siblings ...)
  2020-10-15 19:30 ` [PATCH v3 03/12] PCI/ASPM: Use 'parent' and 'child' for readability Bjorn Helgaas
@ 2020-10-15 19:30 ` Bjorn Helgaas
  2020-10-15 19:30 ` [PATCH v3 05/12] PCI/ASPM: Remove struct aspm_register_info.enabled Bjorn Helgaas
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2020-10-15 19:30 UTC (permalink / raw)
  To: Saheed O . Bolarinwa
  Cc: Puranjay Mohan, Rajat Jain, Kai-Heng Feng, Yicong Yang,
	Heiner Kallweit, linux-pci, Bjorn Helgaas

From: "Saheed O. Bolarinwa" <refactormyself@gmail.com>

Previously we stored the "ASPM Support" field from the Link Capabilities
register in the struct aspm_register_info.

Read the Link Capabilities directly when needed and remove it from the
struct aspm_register_info.  No functional change intended.

[bhelgaas: remove pci_dev cached copy since LNKCAP isn't truly read-only,
add PCI_EXP_LNKCAP_ASPM_L0S & PCI_EXP_LNKCAP_ASPM_L1, check them directly
instead of adding aspm_support()]
Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c       | 25 ++++++++++++++-----------
 include/uapi/linux/pci_regs.h |  2 ++
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 0725511cbeb5..82ce34e2ef53 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -381,7 +381,6 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
 }
 
 struct aspm_register_info {
-	u32 support:2;
 	u32 enabled:2;
 	u32 latency_encoding_l0s;
 	u32 latency_encoding_l1;
@@ -400,7 +399,6 @@ static void pcie_get_aspm_reg(struct pci_dev *pdev,
 	u32 reg32;
 
 	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &reg32);
-	info->support = (reg32 & PCI_EXP_LNKCAP_ASPMS) >> 10;
 	info->latency_encoding_l0s = (reg32 & PCI_EXP_LNKCAP_L0SEL) >> 12;
 	info->latency_encoding_l1  = (reg32 & PCI_EXP_LNKCAP_L1EL) >> 15;
 	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &reg16);
@@ -550,6 +548,7 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
+	u32 parent_lnkcap, child_lnkcap;
 	struct pci_bus *linkbus = parent->subordinate;
 	struct aspm_register_info upreg, dwreg;
 
@@ -560,24 +559,26 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 		return;
 	}
 
-	/* Get upstream/downstream components' register state */
-	pcie_get_aspm_reg(parent, &upreg);
-	pcie_get_aspm_reg(child, &dwreg);
-
 	/*
 	 * If ASPM not supported, don't mess with the clocks and link,
 	 * bail out now.
 	 */
-	if (!(upreg.support & dwreg.support))
+	pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, &parent_lnkcap);
+	pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &child_lnkcap);
+	if (!(parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPMS))
 		return;
 
 	/* Configure common clock before checking latencies */
 	pcie_aspm_configure_common_clock(link);
 
 	/*
-	 * Re-read upstream/downstream components' register state
-	 * after clock configuration
+	 * Re-read upstream/downstream components' register state after
+	 * clock configuration.  L0s & L1 exit latencies in the otherwise
+	 * read-only Link Capabilities may change depending on common clock
+	 * configuration (PCIe r5.0, sec 7.5.3.6).
 	 */
+	pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, &parent_lnkcap);
+	pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &child_lnkcap);
 	pcie_get_aspm_reg(parent, &upreg);
 	pcie_get_aspm_reg(child, &dwreg);
 
@@ -588,8 +589,9 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	 * given link unless components on both sides of the link each
 	 * support L0s.
 	 */
-	if (dwreg.support & upreg.support & PCIE_LINK_STATE_L0S)
+	if (parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)
 		link->aspm_support |= ASPM_STATE_L0S;
+
 	if (dwreg.enabled & PCIE_LINK_STATE_L0S)
 		link->aspm_enabled |= ASPM_STATE_L0S_UP;
 	if (upreg.enabled & PCIE_LINK_STATE_L0S)
@@ -598,8 +600,9 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	link->latency_dw.l0s = calc_l0s_latency(dwreg.latency_encoding_l0s);
 
 	/* Setup L1 state */
-	if (upreg.support & dwreg.support & PCIE_LINK_STATE_L1)
+	if (parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPM_L1)
 		link->aspm_support |= ASPM_STATE_L1;
+
 	if (upreg.enabled & dwreg.enabled & PCIE_LINK_STATE_L1)
 		link->aspm_enabled |= ASPM_STATE_L1;
 	link->latency_up.l1 = calc_l1_latency(upreg.latency_encoding_l1);
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index f9701410d3b5..06846ec2e071 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -532,6 +532,8 @@
 #define  PCI_EXP_LNKCAP_SLS_32_0GB 0x00000005 /* LNKCAP2 SLS Vector bit 4 */
 #define  PCI_EXP_LNKCAP_MLW	0x000003f0 /* Maximum Link Width */
 #define  PCI_EXP_LNKCAP_ASPMS	0x00000c00 /* ASPM Support */
+#define  PCI_EXP_LNKCAP_ASPM_L0S 0x00000400 /* ASPM L0s Support */
+#define  PCI_EXP_LNKCAP_ASPM_L1  0x00000800 /* ASPM L1 Support */
 #define  PCI_EXP_LNKCAP_L0SEL	0x00007000 /* L0s Exit Latency */
 #define  PCI_EXP_LNKCAP_L1EL	0x00038000 /* L1 Exit Latency */
 #define  PCI_EXP_LNKCAP_CLKPM	0x00040000 /* Clock Power Management */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 05/12] PCI/ASPM: Remove struct aspm_register_info.enabled
  2020-10-15 19:30 [PATCH v3 00/12] PCI/ASPM: Cleanup Bjorn Helgaas
                   ` (3 preceding siblings ...)
  2020-10-15 19:30 ` [PATCH v3 04/12] PCI/ASPM: Remove struct aspm_register_info.support Bjorn Helgaas
@ 2020-10-15 19:30 ` Bjorn Helgaas
  2020-10-15 19:30 ` [PATCH v3 06/12] PCI/ASPM: Remove struct aspm_register_info.latency_encoding Bjorn Helgaas
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2020-10-15 19:30 UTC (permalink / raw)
  To: Saheed O . Bolarinwa
  Cc: Puranjay Mohan, Rajat Jain, Kai-Heng Feng, Yicong Yang,
	Heiner Kallweit, linux-pci, Bjorn Helgaas

From: "Saheed O. Bolarinwa" <refactormyself@gmail.com>

Previously we stored the "ASPM Control" bits from the Link Control register
in the struct aspm_register_info.

Read PCI_EXP_LNKCTL directly when needed.  This means we can use the
PCI_EXP_LNKCTL_ASPM_* bits directly instead of the similar but different
PCIE_LINK_STATE_* bits.  No functional change intended.

[bhelgaas: drop get_aspm_enable() and read LNKCTL once directly]
Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 82ce34e2ef53..36540879586b 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -381,10 +381,8 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
 }
 
 struct aspm_register_info {
-	u32 enabled:2;
 	u32 latency_encoding_l0s;
 	u32 latency_encoding_l1;
-
 	/* L1 substates */
 	u32 l1ss_cap_ptr;
 	u32 l1ss_cap;
@@ -395,14 +393,11 @@ struct aspm_register_info {
 static void pcie_get_aspm_reg(struct pci_dev *pdev,
 			      struct aspm_register_info *info)
 {
-	u16 reg16;
 	u32 reg32;
 
 	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &reg32);
 	info->latency_encoding_l0s = (reg32 & PCI_EXP_LNKCAP_L0SEL) >> 12;
 	info->latency_encoding_l1  = (reg32 & PCI_EXP_LNKCAP_L1EL) >> 15;
-	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &reg16);
-	info->enabled = reg16 & PCI_EXP_LNKCTL_ASPMC;
 
 	/* Read L1 PM substate capabilities */
 	info->l1ss_cap = info->l1ss_ctl1 = info->l1ss_ctl2 = 0;
@@ -549,6 +544,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
 	u32 parent_lnkcap, child_lnkcap;
+	u16 parent_lnkctl, child_lnkctl;
 	struct pci_bus *linkbus = parent->subordinate;
 	struct aspm_register_info upreg, dwreg;
 
@@ -579,6 +575,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	 */
 	pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, &parent_lnkcap);
 	pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &child_lnkcap);
+	pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &parent_lnkctl);
+	pcie_capability_read_word(child, PCI_EXP_LNKCTL, &child_lnkctl);
 	pcie_get_aspm_reg(parent, &upreg);
 	pcie_get_aspm_reg(child, &dwreg);
 
@@ -592,9 +590,9 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	if (parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)
 		link->aspm_support |= ASPM_STATE_L0S;
 
-	if (dwreg.enabled & PCIE_LINK_STATE_L0S)
+	if (child_lnkctl & PCI_EXP_LNKCTL_ASPM_L0S)
 		link->aspm_enabled |= ASPM_STATE_L0S_UP;
-	if (upreg.enabled & PCIE_LINK_STATE_L0S)
+	if (parent_lnkctl & PCI_EXP_LNKCTL_ASPM_L0S)
 		link->aspm_enabled |= ASPM_STATE_L0S_DW;
 	link->latency_up.l0s = calc_l0s_latency(upreg.latency_encoding_l0s);
 	link->latency_dw.l0s = calc_l0s_latency(dwreg.latency_encoding_l0s);
@@ -603,7 +601,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	if (parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPM_L1)
 		link->aspm_support |= ASPM_STATE_L1;
 
-	if (upreg.enabled & dwreg.enabled & PCIE_LINK_STATE_L1)
+	if (parent_lnkctl & child_lnkctl & PCI_EXP_LNKCTL_ASPM_L1)
 		link->aspm_enabled |= ASPM_STATE_L1;
 	link->latency_up.l1 = calc_l1_latency(upreg.latency_encoding_l1);
 	link->latency_dw.l1 = calc_l1_latency(dwreg.latency_encoding_l1);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 06/12] PCI/ASPM: Remove struct aspm_register_info.latency_encoding
  2020-10-15 19:30 [PATCH v3 00/12] PCI/ASPM: Cleanup Bjorn Helgaas
                   ` (4 preceding siblings ...)
  2020-10-15 19:30 ` [PATCH v3 05/12] PCI/ASPM: Remove struct aspm_register_info.enabled Bjorn Helgaas
@ 2020-10-15 19:30 ` Bjorn Helgaas
  2020-10-15 19:30 ` [PATCH v3 07/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_cap_ptr Bjorn Helgaas
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2020-10-15 19:30 UTC (permalink / raw)
  To: Saheed O . Bolarinwa
  Cc: Puranjay Mohan, Rajat Jain, Kai-Heng Feng, Yicong Yang,
	Heiner Kallweit, linux-pci, Bjorn Helgaas

From: "Saheed O. Bolarinwa" <refactormyself@gmail.com>

Previously we stored L0s and L1 Exit Latency information from the Link
Capabilities register in the struct aspm_register_info.

We only need these latencies when we already have the Link Capabilities
values, so use those directly and remove the latencies from struct
aspm_register_info.  No functional change intended.

Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 36540879586b..fd6e597b9d74 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -308,8 +308,10 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
 }
 
 /* Convert L0s latency encoding to ns */
-static u32 calc_l0s_latency(u32 encoding)
+static u32 calc_l0s_latency(u32 lnkcap)
 {
+	u32 encoding = (lnkcap & PCI_EXP_LNKCAP_L0SEL) >> 12;
+
 	if (encoding == 0x7)
 		return (5 * 1000);	/* > 4us */
 	return (64 << encoding);
@@ -324,8 +326,10 @@ static u32 calc_l0s_acceptable(u32 encoding)
 }
 
 /* Convert L1 latency encoding to ns */
-static u32 calc_l1_latency(u32 encoding)
+static u32 calc_l1_latency(u32 lnkcap)
 {
+	u32 encoding = (lnkcap & PCI_EXP_LNKCAP_L1EL) >> 15;
+
 	if (encoding == 0x7)
 		return (65 * 1000);	/* > 64us */
 	return (1000 << encoding);
@@ -381,8 +385,6 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
 }
 
 struct aspm_register_info {
-	u32 latency_encoding_l0s;
-	u32 latency_encoding_l1;
 	/* L1 substates */
 	u32 l1ss_cap_ptr;
 	u32 l1ss_cap;
@@ -393,12 +395,6 @@ struct aspm_register_info {
 static void pcie_get_aspm_reg(struct pci_dev *pdev,
 			      struct aspm_register_info *info)
 {
-	u32 reg32;
-
-	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &reg32);
-	info->latency_encoding_l0s = (reg32 & PCI_EXP_LNKCAP_L0SEL) >> 12;
-	info->latency_encoding_l1  = (reg32 & PCI_EXP_LNKCAP_L1EL) >> 15;
-
 	/* Read L1 PM substate capabilities */
 	info->l1ss_cap = info->l1ss_ctl1 = info->l1ss_ctl2 = 0;
 	info->l1ss_cap_ptr = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
@@ -594,8 +590,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 		link->aspm_enabled |= ASPM_STATE_L0S_UP;
 	if (parent_lnkctl & PCI_EXP_LNKCTL_ASPM_L0S)
 		link->aspm_enabled |= ASPM_STATE_L0S_DW;
-	link->latency_up.l0s = calc_l0s_latency(upreg.latency_encoding_l0s);
-	link->latency_dw.l0s = calc_l0s_latency(dwreg.latency_encoding_l0s);
+	link->latency_up.l0s = calc_l0s_latency(parent_lnkcap);
+	link->latency_dw.l0s = calc_l0s_latency(child_lnkcap);
 
 	/* Setup L1 state */
 	if (parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPM_L1)
@@ -603,8 +599,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 
 	if (parent_lnkctl & child_lnkctl & PCI_EXP_LNKCTL_ASPM_L1)
 		link->aspm_enabled |= ASPM_STATE_L1;
-	link->latency_up.l1 = calc_l1_latency(upreg.latency_encoding_l1);
-	link->latency_dw.l1 = calc_l1_latency(dwreg.latency_encoding_l1);
+	link->latency_up.l1 = calc_l1_latency(parent_lnkcap);
+	link->latency_dw.l1 = calc_l1_latency(child_lnkcap);
 
 	/* Setup L1 substate
 	 * If we don't have LTR for the entire path from the Root Complex
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 07/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_cap_ptr
  2020-10-15 19:30 [PATCH v3 00/12] PCI/ASPM: Cleanup Bjorn Helgaas
                   ` (5 preceding siblings ...)
  2020-10-15 19:30 ` [PATCH v3 06/12] PCI/ASPM: Remove struct aspm_register_info.latency_encoding Bjorn Helgaas
@ 2020-10-15 19:30 ` Bjorn Helgaas
  2020-10-15 19:30 ` [PATCH v3 08/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_ctl2 (unused) Bjorn Helgaas
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2020-10-15 19:30 UTC (permalink / raw)
  To: Saheed O . Bolarinwa
  Cc: Puranjay Mohan, Rajat Jain, Kai-Heng Feng, Yicong Yang,
	Heiner Kallweit, linux-pci, Bjorn Helgaas

From: "Saheed O. Bolarinwa" <refactormyself@gmail.com>

Save the L1 Substates Capability pointer in struct pci_dev.  Then we don't
have to keep track of it in the struct aspm_register_info and struct
pcie_link_state, which makes the code easier to read.  No functional change
intended.

[bhelgaas: split to a separate patch]
Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 36 +++++++++++++++---------------------
 drivers/pci/probe.c     |  3 +++
 include/linux/pci.h     |  1 +
 3 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index fd6e597b9d74..77316262f982 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -77,8 +77,6 @@ struct pcie_link_state {
 
 	/* L1 PM Substate info */
 	struct {
-		u32 up_cap_ptr;		/* L1SS cap ptr in upstream dev */
-		u32 dw_cap_ptr;		/* L1SS cap ptr in downstream dev */
 		u32 ctl1;		/* value to be programmed in ctl1 */
 		u32 ctl2;		/* value to be programmed in ctl2 */
 	} l1ss;
@@ -386,7 +384,6 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
 
 struct aspm_register_info {
 	/* L1 substates */
-	u32 l1ss_cap_ptr;
 	u32 l1ss_cap;
 	u32 l1ss_ctl1;
 	u32 l1ss_ctl2;
@@ -397,19 +394,20 @@ static void pcie_get_aspm_reg(struct pci_dev *pdev,
 {
 	/* Read L1 PM substate capabilities */
 	info->l1ss_cap = info->l1ss_ctl1 = info->l1ss_ctl2 = 0;
-	info->l1ss_cap_ptr = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
-	if (!info->l1ss_cap_ptr)
+
+	if (!pdev->l1ss)
 		return;
-	pci_read_config_dword(pdev, info->l1ss_cap_ptr + PCI_L1SS_CAP,
+
+	pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CAP,
 			      &info->l1ss_cap);
 	if (!(info->l1ss_cap & PCI_L1SS_CAP_L1_PM_SS)) {
 		info->l1ss_cap = 0;
 		return;
 	}
 
-	pci_read_config_dword(pdev, info->l1ss_cap_ptr + PCI_L1SS_CTL1,
+	pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL1,
 			      &info->l1ss_ctl1);
-	pci_read_config_dword(pdev, info->l1ss_cap_ptr + PCI_L1SS_CTL2,
+	pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL2,
 			      &info->l1ss_ctl2);
 }
 
@@ -494,8 +492,6 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 	u32 val1, val2, scale1, scale2;
 	u32 t_common_mode, t_power_on, l1_2_threshold, scale, value;
 
-	link->l1ss.up_cap_ptr = upreg->l1ss_cap_ptr;
-	link->l1ss.dw_cap_ptr = dwreg->l1ss_cap_ptr;
 	link->l1ss.ctl1 = link->l1ss.ctl2 = 0;
 
 	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
@@ -664,8 +660,6 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
 {
 	u32 val, enable_req;
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
-	u32 up_cap_ptr = link->l1ss.up_cap_ptr;
-	u32 dw_cap_ptr = link->l1ss.dw_cap_ptr;
 
 	enable_req = (link->aspm_enabled ^ state) & state;
 
@@ -683,9 +677,9 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
 	 */
 
 	/* Disable all L1 substates */
-	pci_clear_and_set_dword(child, dw_cap_ptr + PCI_L1SS_CTL1,
+	pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
 				PCI_L1SS_CTL1_L1SS_MASK, 0);
-	pci_clear_and_set_dword(parent, up_cap_ptr + PCI_L1SS_CTL1,
+	pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
 				PCI_L1SS_CTL1_L1SS_MASK, 0);
 	/*
 	 * If needed, disable L1, and it gets enabled later
@@ -701,22 +695,22 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
 	if (enable_req & ASPM_STATE_L1_2_MASK) {
 
 		/* Program T_POWER_ON times in both ports */
-		pci_write_config_dword(parent, up_cap_ptr + PCI_L1SS_CTL2,
+		pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2,
 				       link->l1ss.ctl2);
-		pci_write_config_dword(child, dw_cap_ptr + PCI_L1SS_CTL2,
+		pci_write_config_dword(child, child->l1ss + PCI_L1SS_CTL2,
 				       link->l1ss.ctl2);
 
 		/* Program Common_Mode_Restore_Time in upstream device */
-		pci_clear_and_set_dword(parent, up_cap_ptr + PCI_L1SS_CTL1,
+		pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
 					PCI_L1SS_CTL1_CM_RESTORE_TIME,
 					link->l1ss.ctl1);
 
 		/* Program LTR_L1.2_THRESHOLD time in both ports */
-		pci_clear_and_set_dword(parent,	up_cap_ptr + PCI_L1SS_CTL1,
+		pci_clear_and_set_dword(parent,	parent->l1ss + PCI_L1SS_CTL1,
 					PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
 					PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
 					link->l1ss.ctl1);
-		pci_clear_and_set_dword(child, dw_cap_ptr + PCI_L1SS_CTL1,
+		pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
 					PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
 					PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
 					link->l1ss.ctl1);
@@ -733,9 +727,9 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
 		val |= PCI_L1SS_CTL1_PCIPM_L1_2;
 
 	/* Enable what we need to enable */
-	pci_clear_and_set_dword(parent, up_cap_ptr + PCI_L1SS_CTL1,
+	pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
 				PCI_L1SS_CTL1_L1SS_MASK, val);
-	pci_clear_and_set_dword(child, dw_cap_ptr + PCI_L1SS_CTL1,
+	pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
 				PCI_L1SS_CTL1_L1SS_MASK, val);
 }
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 03d37128a24f..06f6bbcd8131 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2106,6 +2106,9 @@ static void pci_configure_ltr(struct pci_dev *dev)
 	if (!pci_is_pcie(dev))
 		return;
 
+	/* Read L1 PM substate capabilities */
+	dev->l1ss = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_L1SS);
+
 	pcie_capability_read_dword(dev, PCI_EXP_DEVCAP2, &cap);
 	if (!(cap & PCI_EXP_DEVCAP2_LTR))
 		return;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 835530605c0d..c5288cd71a2e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -380,6 +380,7 @@ struct pci_dev {
 	struct pcie_link_state	*link_state;	/* ASPM link state */
 	unsigned int	ltr_path:1;	/* Latency Tolerance Reporting
 					   supported from root to here */
+	int		l1ss;		/* L1SS Capability pointer */
 #endif
 	unsigned int	eetlp_prefix_path:1;	/* End-to-End TLP Prefix */
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 08/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_ctl2 (unused)
  2020-10-15 19:30 [PATCH v3 00/12] PCI/ASPM: Cleanup Bjorn Helgaas
                   ` (6 preceding siblings ...)
  2020-10-15 19:30 ` [PATCH v3 07/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_cap_ptr Bjorn Helgaas
@ 2020-10-15 19:30 ` Bjorn Helgaas
  2020-10-15 19:30 ` [PATCH v3 09/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_ctl1 Bjorn Helgaas
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2020-10-15 19:30 UTC (permalink / raw)
  To: Saheed O . Bolarinwa
  Cc: Puranjay Mohan, Rajat Jain, Kai-Heng Feng, Yicong Yang,
	Heiner Kallweit, linux-pci, Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

We never use the aspm_register_info.l1ss_ctl2 value, so remove it.  No
functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 77316262f982..3afa6c418f54 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -386,14 +386,13 @@ struct aspm_register_info {
 	/* L1 substates */
 	u32 l1ss_cap;
 	u32 l1ss_ctl1;
-	u32 l1ss_ctl2;
 };
 
 static void pcie_get_aspm_reg(struct pci_dev *pdev,
 			      struct aspm_register_info *info)
 {
 	/* Read L1 PM substate capabilities */
-	info->l1ss_cap = info->l1ss_ctl1 = info->l1ss_ctl2 = 0;
+	info->l1ss_cap = info->l1ss_ctl1 = 0;
 
 	if (!pdev->l1ss)
 		return;
@@ -407,8 +406,6 @@ static void pcie_get_aspm_reg(struct pci_dev *pdev,
 
 	pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL1,
 			      &info->l1ss_ctl1);
-	pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL2,
-			      &info->l1ss_ctl2);
 }
 
 static void pcie_aspm_check_latency(struct pci_dev *endpoint)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 09/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_ctl1
  2020-10-15 19:30 [PATCH v3 00/12] PCI/ASPM: Cleanup Bjorn Helgaas
                   ` (7 preceding siblings ...)
  2020-10-15 19:30 ` [PATCH v3 08/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_ctl2 (unused) Bjorn Helgaas
@ 2020-10-15 19:30 ` Bjorn Helgaas
  2020-10-15 19:30 ` [PATCH v3 10/12] PCI/ASPM: Pass L1SS Capabilities value, not struct aspm_register_info Bjorn Helgaas
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2020-10-15 19:30 UTC (permalink / raw)
  To: Saheed O . Bolarinwa
  Cc: Puranjay Mohan, Rajat Jain, Kai-Heng Feng, Yicong Yang,
	Heiner Kallweit, linux-pci, Bjorn Helgaas

From: "Saheed O. Bolarinwa" <refactormyself@gmail.com>

Previously we stored the L1SS Control 1 register in the struct
aspm_register_info.

We only need this information in one place, so read it there and remove it
from struct aspm_register_info.  No functional change intended.

[bhelgaas: split ctl1/ctl2]
Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 3afa6c418f54..896f6c0b08c6 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -385,27 +385,21 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
 struct aspm_register_info {
 	/* L1 substates */
 	u32 l1ss_cap;
-	u32 l1ss_ctl1;
 };
 
 static void pcie_get_aspm_reg(struct pci_dev *pdev,
 			      struct aspm_register_info *info)
 {
 	/* Read L1 PM substate capabilities */
-	info->l1ss_cap = info->l1ss_ctl1 = 0;
+	info->l1ss_cap = 0;
 
 	if (!pdev->l1ss)
 		return;
 
 	pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CAP,
 			      &info->l1ss_cap);
-	if (!(info->l1ss_cap & PCI_L1SS_CAP_L1_PM_SS)) {
+	if (!(info->l1ss_cap & PCI_L1SS_CAP_L1_PM_SS))
 		info->l1ss_cap = 0;
-		return;
-	}
-
-	pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL1,
-			      &info->l1ss_ctl1);
 }
 
 static void pcie_aspm_check_latency(struct pci_dev *endpoint)
@@ -534,6 +528,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
 	u32 parent_lnkcap, child_lnkcap;
 	u16 parent_lnkctl, child_lnkctl;
+	u32 parent_l1ss_ctl1 = 0, child_l1ss_ctl1 = 0;
 	struct pci_bus *linkbus = parent->subordinate;
 	struct aspm_register_info upreg, dwreg;
 
@@ -612,13 +607,20 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_2)
 		link->aspm_support |= ASPM_STATE_L1_2_PCIPM;
 
-	if (upreg.l1ss_ctl1 & dwreg.l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_1)
+	if (upreg.l1ss_cap)
+		pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
+				      &parent_l1ss_ctl1);
+	if (dwreg.l1ss_cap)
+		pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL1,
+				      &child_l1ss_ctl1);
+
+	if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_1)
 		link->aspm_enabled |= ASPM_STATE_L1_1;
-	if (upreg.l1ss_ctl1 & dwreg.l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_2)
+	if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_2)
 		link->aspm_enabled |= ASPM_STATE_L1_2;
-	if (upreg.l1ss_ctl1 & dwreg.l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_1)
+	if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_1)
 		link->aspm_enabled |= ASPM_STATE_L1_1_PCIPM;
-	if (upreg.l1ss_ctl1 & dwreg.l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_2)
+	if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_2)
 		link->aspm_enabled |= ASPM_STATE_L1_2_PCIPM;
 
 	if (link->aspm_support & ASPM_STATE_L1SS)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 10/12] PCI/ASPM: Pass L1SS Capabilities value, not struct aspm_register_info
  2020-10-15 19:30 [PATCH v3 00/12] PCI/ASPM: Cleanup Bjorn Helgaas
                   ` (8 preceding siblings ...)
  2020-10-15 19:30 ` [PATCH v3 09/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_ctl1 Bjorn Helgaas
@ 2020-10-15 19:30 ` Bjorn Helgaas
  2020-10-15 19:30 ` [PATCH v3 11/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_cap Bjorn Helgaas
  2020-10-15 19:30 ` [PATCH v3 12/12] PCI/ASPM: Remove struct pcie_link_state.l1ss Bjorn Helgaas
  11 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2020-10-15 19:30 UTC (permalink / raw)
  To: Saheed O . Bolarinwa
  Cc: Puranjay Mohan, Rajat Jain, Kai-Heng Feng, Yicong Yang,
	Heiner Kallweit, linux-pci, Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

aspm_calc_l1ss_info() needs only the L1SS Capabilities.  It doesn't need
anything else from struct aspm_register_info, so pass only the Capabilities
value.  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 896f6c0b08c6..8c6b976d8b50 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -476,8 +476,7 @@ static void pci_clear_and_set_dword(struct pci_dev *pdev, int pos,
 
 /* Calculate L1.2 PM substate timing parameters */
 static void aspm_calc_l1ss_info(struct pcie_link_state *link,
-				struct aspm_register_info *upreg,
-				struct aspm_register_info *dwreg)
+				u32 parent_l1ss_cap, u32 child_l1ss_cap)
 {
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
 	u32 val1, val2, scale1, scale2;
@@ -489,15 +488,15 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 		return;
 
 	/* Choose the greater of the two Port Common_Mode_Restore_Times */
-	val1 = (upreg->l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
-	val2 = (dwreg->l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
+	val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
+	val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
 	t_common_mode = max(val1, val2);
 
 	/* Choose the greater of the two Port T_POWER_ON times */
-	val1   = (upreg->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
-	scale1 = (upreg->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
-	val2   = (dwreg->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
-	scale2 = (dwreg->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
+	val1   = (parent_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
+	scale1 = (parent_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
+	val2   = (child_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
+	scale2 = (child_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
 
 	if (calc_l1ss_pwron(parent, scale1, val1) >
 	    calc_l1ss_pwron(child, scale2, val2)) {
@@ -624,7 +623,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 		link->aspm_enabled |= ASPM_STATE_L1_2_PCIPM;
 
 	if (link->aspm_support & ASPM_STATE_L1SS)
-		aspm_calc_l1ss_info(link, &upreg, &dwreg);
+		aspm_calc_l1ss_info(link, upreg.l1ss_cap, dwreg.l1ss_cap);
 
 	/* Save default state */
 	link->aspm_default = link->aspm_enabled;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 11/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_cap
  2020-10-15 19:30 [PATCH v3 00/12] PCI/ASPM: Cleanup Bjorn Helgaas
                   ` (9 preceding siblings ...)
  2020-10-15 19:30 ` [PATCH v3 10/12] PCI/ASPM: Pass L1SS Capabilities value, not struct aspm_register_info Bjorn Helgaas
@ 2020-10-15 19:30 ` Bjorn Helgaas
  2020-10-15 19:30 ` [PATCH v3 12/12] PCI/ASPM: Remove struct pcie_link_state.l1ss Bjorn Helgaas
  11 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2020-10-15 19:30 UTC (permalink / raw)
  To: Saheed O . Bolarinwa
  Cc: Puranjay Mohan, Rajat Jain, Kai-Heng Feng, Yicong Yang,
	Heiner Kallweit, linux-pci, Bjorn Helgaas

From: "Saheed O. Bolarinwa" <refactormyself@gmail.com>

Previously we stored the L1SS Capabilities value in the struct
aspm_register_info.

We only need this information in one place, so read it there and remove
struct aspm_register_info completely, since it's now empty.  No functional
change intended.

[bhelgaas: split up, don't cache l1ss_cap in pci_dev]
Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 53 ++++++++++++++++-------------------------
 1 file changed, 21 insertions(+), 32 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 8c6b976d8b50..d76f23908d67 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -382,26 +382,6 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
 	}
 }
 
-struct aspm_register_info {
-	/* L1 substates */
-	u32 l1ss_cap;
-};
-
-static void pcie_get_aspm_reg(struct pci_dev *pdev,
-			      struct aspm_register_info *info)
-{
-	/* Read L1 PM substate capabilities */
-	info->l1ss_cap = 0;
-
-	if (!pdev->l1ss)
-		return;
-
-	pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CAP,
-			      &info->l1ss_cap);
-	if (!(info->l1ss_cap & PCI_L1SS_CAP_L1_PM_SS))
-		info->l1ss_cap = 0;
-}
-
 static void pcie_aspm_check_latency(struct pci_dev *endpoint)
 {
 	u32 latency, l1_switch_latency = 0;
@@ -527,9 +507,9 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
 	u32 parent_lnkcap, child_lnkcap;
 	u16 parent_lnkctl, child_lnkctl;
+	u32 parent_l1ss_cap, child_l1ss_cap;
 	u32 parent_l1ss_ctl1 = 0, child_l1ss_ctl1 = 0;
 	struct pci_bus *linkbus = parent->subordinate;
-	struct aspm_register_info upreg, dwreg;
 
 	if (blacklist) {
 		/* Set enabled/disable so that we will disable ASPM later */
@@ -560,8 +540,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &child_lnkcap);
 	pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &parent_lnkctl);
 	pcie_capability_read_word(child, PCI_EXP_LNKCTL, &child_lnkctl);
-	pcie_get_aspm_reg(parent, &upreg);
-	pcie_get_aspm_reg(child, &dwreg);
 
 	/*
 	 * Setup L0s state
@@ -589,27 +567,38 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	link->latency_up.l1 = calc_l1_latency(parent_lnkcap);
 	link->latency_dw.l1 = calc_l1_latency(child_lnkcap);
 
-	/* Setup L1 substate
+	/* Setup L1 substate */
+	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CAP,
+			      &parent_l1ss_cap);
+	pci_read_config_dword(child, child->l1ss + PCI_L1SS_CAP,
+			      &child_l1ss_cap);
+
+	if (!(parent_l1ss_cap & PCI_L1SS_CAP_L1_PM_SS))
+		parent_l1ss_cap = 0;
+	if (!(child_l1ss_cap & PCI_L1SS_CAP_L1_PM_SS))
+		child_l1ss_cap = 0;
+
+	/*
 	 * If we don't have LTR for the entire path from the Root Complex
 	 * to this device, we can't use ASPM L1.2 because it relies on the
 	 * LTR_L1.2_THRESHOLD.  See PCIe r4.0, secs 5.5.4, 6.18.
 	 */
 	if (!child->ltr_path)
-		dwreg.l1ss_cap &= ~PCI_L1SS_CAP_ASPM_L1_2;
+		child_l1ss_cap &= ~PCI_L1SS_CAP_ASPM_L1_2;
 
-	if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_ASPM_L1_1)
+	if (parent_l1ss_cap & child_l1ss_cap & PCI_L1SS_CAP_ASPM_L1_1)
 		link->aspm_support |= ASPM_STATE_L1_1;
-	if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_ASPM_L1_2)
+	if (parent_l1ss_cap & child_l1ss_cap & PCI_L1SS_CAP_ASPM_L1_2)
 		link->aspm_support |= ASPM_STATE_L1_2;
-	if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_1)
+	if (parent_l1ss_cap & child_l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_1)
 		link->aspm_support |= ASPM_STATE_L1_1_PCIPM;
-	if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_2)
+	if (parent_l1ss_cap & child_l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_2)
 		link->aspm_support |= ASPM_STATE_L1_2_PCIPM;
 
-	if (upreg.l1ss_cap)
+	if (parent_l1ss_cap)
 		pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
 				      &parent_l1ss_ctl1);
-	if (dwreg.l1ss_cap)
+	if (child_l1ss_cap)
 		pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL1,
 				      &child_l1ss_ctl1);
 
@@ -623,7 +612,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 		link->aspm_enabled |= ASPM_STATE_L1_2_PCIPM;
 
 	if (link->aspm_support & ASPM_STATE_L1SS)
-		aspm_calc_l1ss_info(link, upreg.l1ss_cap, dwreg.l1ss_cap);
+		aspm_calc_l1ss_info(link, parent_l1ss_cap, child_l1ss_cap);
 
 	/* Save default state */
 	link->aspm_default = link->aspm_enabled;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 12/12] PCI/ASPM: Remove struct pcie_link_state.l1ss
  2020-10-15 19:30 [PATCH v3 00/12] PCI/ASPM: Cleanup Bjorn Helgaas
                   ` (10 preceding siblings ...)
  2020-10-15 19:30 ` [PATCH v3 11/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_cap Bjorn Helgaas
@ 2020-10-15 19:30 ` Bjorn Helgaas
  2020-10-16 20:59   ` Bjorn Helgaas
  11 siblings, 1 reply; 14+ messages in thread
From: Bjorn Helgaas @ 2020-10-15 19:30 UTC (permalink / raw)
  To: Saheed O . Bolarinwa
  Cc: Puranjay Mohan, Rajat Jain, Kai-Heng Feng, Yicong Yang,
	Heiner Kallweit, linux-pci, Bjorn Helgaas

From: "Saheed O. Bolarinwa" <refactormyself@gmail.com>

Previously we computed L1.2 parameters in the enumeration path, saved them
in struct pcie_link_state.l1ss, and programmed them into the devices
whenever we enabled or disabled L1.2 on the link.  But these parameters are
constant and don't need to be updated when enabling/disabling L1.2.

Compute and program the L1.2 parameters once during enumeration and remove
the struct pcie_link_state.l1ss member.  No functional change intended.

[bhelgaas: rework to program L1.2 parameters during enumeration]
Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 55 +++++++++++++++--------------------------
 1 file changed, 20 insertions(+), 35 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index d76f23908d67..361eaa0c615d 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -74,12 +74,6 @@ struct pcie_link_state {
 	 * has one slot under it, so at most there are 8 functions.
 	 */
 	struct aspm_latency acceptable[8];
-
-	/* L1 PM Substate info */
-	struct {
-		u32 ctl1;		/* value to be programmed in ctl1 */
-		u32 ctl2;		/* value to be programmed in ctl2 */
-	} l1ss;
 };
 
 static int aspm_disabled, aspm_force;
@@ -461,8 +455,7 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
 	u32 val1, val2, scale1, scale2;
 	u32 t_common_mode, t_power_on, l1_2_threshold, scale, value;
-
-	link->l1ss.ctl1 = link->l1ss.ctl2 = 0;
+	u32 ctl1 = 0, ctl2 = 0;
 
 	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
 		return;
@@ -480,10 +473,10 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 
 	if (calc_l1ss_pwron(parent, scale1, val1) >
 	    calc_l1ss_pwron(child, scale2, val2)) {
-		link->l1ss.ctl2 |= scale1 | (val1 << 3);
+		ctl2 |= scale1 | (val1 << 3);
 		t_power_on = calc_l1ss_pwron(parent, scale1, val1);
 	} else {
-		link->l1ss.ctl2 |= scale2 | (val2 << 3);
+		ctl2 |= scale2 | (val2 << 3);
 		t_power_on = calc_l1ss_pwron(child, scale2, val2);
 	}
 
@@ -499,7 +492,23 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 	 */
 	l1_2_threshold = 2 + 4 + t_common_mode + t_power_on;
 	encode_l12_threshold(l1_2_threshold, &scale, &value);
-	link->l1ss.ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
+	ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
+
+	/* Program T_POWER_ON times in both ports */
+	pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, ctl2);
+	pci_write_config_dword(child, child->l1ss + PCI_L1SS_CTL2, ctl2);
+
+	/* Program Common_Mode_Restore_Time in upstream device */
+	pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
+				PCI_L1SS_CTL1_CM_RESTORE_TIME, ctl1);
+
+	/* Program LTR_L1.2_THRESHOLD time in both ports */
+	pci_clear_and_set_dword(parent,	parent->l1ss + PCI_L1SS_CTL1,
+				PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+				PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
+	pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
+				PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+				PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
 }
 
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
@@ -679,30 +688,6 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
 						   PCI_EXP_LNKCTL_ASPM_L1, 0);
 	}
 
-	if (enable_req & ASPM_STATE_L1_2_MASK) {
-
-		/* Program T_POWER_ON times in both ports */
-		pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2,
-				       link->l1ss.ctl2);
-		pci_write_config_dword(child, child->l1ss + PCI_L1SS_CTL2,
-				       link->l1ss.ctl2);
-
-		/* Program Common_Mode_Restore_Time in upstream device */
-		pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
-					PCI_L1SS_CTL1_CM_RESTORE_TIME,
-					link->l1ss.ctl1);
-
-		/* Program LTR_L1.2_THRESHOLD time in both ports */
-		pci_clear_and_set_dword(parent,	parent->l1ss + PCI_L1SS_CTL1,
-					PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
-					PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
-					link->l1ss.ctl1);
-		pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
-					PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
-					PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
-					link->l1ss.ctl1);
-	}
-
 	val = 0;
 	if (state & ASPM_STATE_L1_1)
 		val |= PCI_L1SS_CTL1_ASPM_L1_1;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 12/12] PCI/ASPM: Remove struct pcie_link_state.l1ss
  2020-10-15 19:30 ` [PATCH v3 12/12] PCI/ASPM: Remove struct pcie_link_state.l1ss Bjorn Helgaas
@ 2020-10-16 20:59   ` Bjorn Helgaas
  0 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2020-10-16 20:59 UTC (permalink / raw)
  To: Saheed O . Bolarinwa
  Cc: Puranjay Mohan, Rajat Jain, Kai-Heng Feng, Yicong Yang,
	Heiner Kallweit, linux-pci, Bjorn Helgaas

On Thu, Oct 15, 2020 at 02:30:39PM -0500, Bjorn Helgaas wrote:
> From: "Saheed O. Bolarinwa" <refactormyself@gmail.com>
> 
> Previously we computed L1.2 parameters in the enumeration path, saved them
> in struct pcie_link_state.l1ss, and programmed them into the devices
> whenever we enabled or disabled L1.2 on the link.  But these parameters are
> constant and don't need to be updated when enabling/disabling L1.2.
> 
> Compute and program the L1.2 parameters once during enumeration and remove
> the struct pcie_link_state.l1ss member.  No functional change intended.
> 
> [bhelgaas: rework to program L1.2 parameters during enumeration]
> Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pcie/aspm.c | 55 +++++++++++++++--------------------------
>  1 file changed, 20 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index d76f23908d67..361eaa0c615d 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -74,12 +74,6 @@ struct pcie_link_state {
>  	 * has one slot under it, so at most there are 8 functions.
>  	 */
>  	struct aspm_latency acceptable[8];
> -
> -	/* L1 PM Substate info */
> -	struct {
> -		u32 ctl1;		/* value to be programmed in ctl1 */
> -		u32 ctl2;		/* value to be programmed in ctl2 */
> -	} l1ss;
>  };
>  
>  static int aspm_disabled, aspm_force;
> @@ -461,8 +455,7 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>  	struct pci_dev *child = link->downstream, *parent = link->pdev;
>  	u32 val1, val2, scale1, scale2;
>  	u32 t_common_mode, t_power_on, l1_2_threshold, scale, value;
> -
> -	link->l1ss.ctl1 = link->l1ss.ctl2 = 0;
> +	u32 ctl1 = 0, ctl2 = 0;
>  
>  	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
>  		return;
> @@ -480,10 +473,10 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>  
>  	if (calc_l1ss_pwron(parent, scale1, val1) >
>  	    calc_l1ss_pwron(child, scale2, val2)) {
> -		link->l1ss.ctl2 |= scale1 | (val1 << 3);
> +		ctl2 |= scale1 | (val1 << 3);
>  		t_power_on = calc_l1ss_pwron(parent, scale1, val1);
>  	} else {
> -		link->l1ss.ctl2 |= scale2 | (val2 << 3);
> +		ctl2 |= scale2 | (val2 << 3);
>  		t_power_on = calc_l1ss_pwron(child, scale2, val2);
>  	}
>  
> @@ -499,7 +492,23 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>  	 */
>  	l1_2_threshold = 2 + 4 + t_common_mode + t_power_on;
>  	encode_l12_threshold(l1_2_threshold, &scale, &value);
> -	link->l1ss.ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
> +	ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
> +
> +	/* Program T_POWER_ON times in both ports */
> +	pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, ctl2);
> +	pci_write_config_dword(child, child->l1ss + PCI_L1SS_CTL2, ctl2);
> +
> +	/* Program Common_Mode_Restore_Time in upstream device */
> +	pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
> +				PCI_L1SS_CTL1_CM_RESTORE_TIME, ctl1);
> +
> +	/* Program LTR_L1.2_THRESHOLD time in both ports */
> +	pci_clear_and_set_dword(parent,	parent->l1ss + PCI_L1SS_CTL1,
> +				PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
> +				PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
> +	pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
> +				PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
> +				PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);

I think this is slightly problematic because L1.2 must be disabled
while updating T_POWER_ON, Common_Mode_Restore_Time, and
LTR_L1.2_THRESHOLD.  Updated patch to address this attached below.

>  }
>  
>  static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
> @@ -679,30 +688,6 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
>  						   PCI_EXP_LNKCTL_ASPM_L1, 0);
>  	}
>  
> -	if (enable_req & ASPM_STATE_L1_2_MASK) {
> -
> -		/* Program T_POWER_ON times in both ports */
> -		pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2,
> -				       link->l1ss.ctl2);
> -		pci_write_config_dword(child, child->l1ss + PCI_L1SS_CTL2,
> -				       link->l1ss.ctl2);
> -
> -		/* Program Common_Mode_Restore_Time in upstream device */
> -		pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
> -					PCI_L1SS_CTL1_CM_RESTORE_TIME,
> -					link->l1ss.ctl1);
> -
> -		/* Program LTR_L1.2_THRESHOLD time in both ports */
> -		pci_clear_and_set_dword(parent,	parent->l1ss + PCI_L1SS_CTL1,
> -					PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
> -					PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
> -					link->l1ss.ctl1);
> -		pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
> -					PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
> -					PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
> -					link->l1ss.ctl1);
> -	}
> -
>  	val = 0;
>  	if (state & ASPM_STATE_L1_1)
>  		val |= PCI_L1SS_CTL1_ASPM_L1_1;
> -- 
> 2.25.1
> 

commit df8f10587d3d ("PCI/ASPM: Remove struct pcie_link_state.l1ss")
Author: Saheed O. Bolarinwa <refactormyself@gmail.com>
Date:   Thu Oct 15 14:30:39 2020 -0500

    PCI/ASPM: Remove struct pcie_link_state.l1ss
    
    Previously we computed L1.2 parameters in the enumeration path, saved them
    in struct pcie_link_state.l1ss, and programmed them into the devices
    whenever we enabled or disabled L1.2 on the link.  But these parameters are
    constant and don't need to be updated when enabling/disabling L1.2.
    
    Compute and program the L1.2 parameters once during enumeration and remove
    the struct pcie_link_state.l1ss member.  No functional change intended.
    
    [bhelgaas: rework to program L1.2 parameters during enumeration]
    Link: https://lore.kernel.org/r/20201015193039.12585-13-helgaas@kernel.org
    Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index d76f23908d67..ac0557a305af 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -74,12 +74,6 @@ struct pcie_link_state {
 	 * has one slot under it, so at most there are 8 functions.
 	 */
 	struct aspm_latency acceptable[8];
-
-	/* L1 PM Substate info */
-	struct {
-		u32 ctl1;		/* value to be programmed in ctl1 */
-		u32 ctl2;		/* value to be programmed in ctl2 */
-	} l1ss;
 };
 
 static int aspm_disabled, aspm_force;
@@ -461,8 +455,9 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
 	u32 val1, val2, scale1, scale2;
 	u32 t_common_mode, t_power_on, l1_2_threshold, scale, value;
-
-	link->l1ss.ctl1 = link->l1ss.ctl2 = 0;
+	u32 ctl1 = 0, ctl2 = 0;
+	u32 pctl1, pctl2, cctl1, cctl2;
+	u32 pl1_2_enables, cl1_2_enables;
 
 	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
 		return;
@@ -480,10 +475,10 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 
 	if (calc_l1ss_pwron(parent, scale1, val1) >
 	    calc_l1ss_pwron(child, scale2, val2)) {
-		link->l1ss.ctl2 |= scale1 | (val1 << 3);
+		ctl2 |= scale1 | (val1 << 3);
 		t_power_on = calc_l1ss_pwron(parent, scale1, val1);
 	} else {
-		link->l1ss.ctl2 |= scale2 | (val2 << 3);
+		ctl2 |= scale2 | (val2 << 3);
 		t_power_on = calc_l1ss_pwron(child, scale2, val2);
 	}
 
@@ -499,7 +494,50 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 	 */
 	l1_2_threshold = 2 + 4 + t_common_mode + t_power_on;
 	encode_l12_threshold(l1_2_threshold, &scale, &value);
-	link->l1ss.ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
+	ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
+
+	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1);
+	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, &pctl2);
+	pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL1, &cctl1);
+	pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL2, &cctl2);
+
+	if (ctl1 == pctl1 && ctl1 == cctl1 &&
+	    ctl2 == pctl2 && ctl2 == cctl2)
+		return;
+
+	/* Disable L1.2 while updating.  See PCIe r5.0, sec 5.5.4, 7.8.3.3 */
+	pl1_2_enables = pctl1 & PCI_L1SS_CTL1_L1_2_MASK;
+	cl1_2_enables = cctl1 & PCI_L1SS_CTL1_L1_2_MASK;
+
+	if (pl1_2_enables || cl1_2_enables) {
+		pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
+					PCI_L1SS_CTL1_L1_2_MASK, 0);
+		pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
+					PCI_L1SS_CTL1_L1_2_MASK, 0);
+	}
+
+	/* Program T_POWER_ON times in both ports */
+	pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, ctl2);
+	pci_write_config_dword(child, child->l1ss + PCI_L1SS_CTL2, ctl2);
+
+	/* Program Common_Mode_Restore_Time in upstream device */
+	pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
+				PCI_L1SS_CTL1_CM_RESTORE_TIME, ctl1);
+
+	/* Program LTR_L1.2_THRESHOLD time in both ports */
+	pci_clear_and_set_dword(parent,	parent->l1ss + PCI_L1SS_CTL1,
+				PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+				PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
+	pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
+				PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+				PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
+
+	if (pl1_2_enables || cl1_2_enables) {
+		pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1, 0,
+					pl1_2_enables);
+		pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1, 0,
+					cl1_2_enables);
+	}
 }
 
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
@@ -679,30 +717,6 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
 						   PCI_EXP_LNKCTL_ASPM_L1, 0);
 	}
 
-	if (enable_req & ASPM_STATE_L1_2_MASK) {
-
-		/* Program T_POWER_ON times in both ports */
-		pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2,
-				       link->l1ss.ctl2);
-		pci_write_config_dword(child, child->l1ss + PCI_L1SS_CTL2,
-				       link->l1ss.ctl2);
-
-		/* Program Common_Mode_Restore_Time in upstream device */
-		pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
-					PCI_L1SS_CTL1_CM_RESTORE_TIME,
-					link->l1ss.ctl1);
-
-		/* Program LTR_L1.2_THRESHOLD time in both ports */
-		pci_clear_and_set_dword(parent,	parent->l1ss + PCI_L1SS_CTL1,
-					PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
-					PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
-					link->l1ss.ctl1);
-		pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
-					PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
-					PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
-					link->l1ss.ctl1);
-	}
-
 	val = 0;
 	if (state & ASPM_STATE_L1_1)
 		val |= PCI_L1SS_CTL1_ASPM_L1_1;
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 06846ec2e071..c7e0acba0e20 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1058,6 +1058,7 @@
 #define  PCI_L1SS_CTL1_PCIPM_L1_1	0x00000002  /* PCI-PM L1.1 Enable */
 #define  PCI_L1SS_CTL1_ASPM_L1_2	0x00000004  /* ASPM L1.2 Enable */
 #define  PCI_L1SS_CTL1_ASPM_L1_1	0x00000008  /* ASPM L1.1 Enable */
+#define  PCI_L1SS_CTL1_L1_2_MASK	0x00000005
 #define  PCI_L1SS_CTL1_L1SS_MASK	0x0000000f
 #define  PCI_L1SS_CTL1_CM_RESTORE_TIME	0x0000ff00  /* Common_Mode_Restore_Time */
 #define  PCI_L1SS_CTL1_LTR_L12_TH_VALUE	0x03ff0000  /* LTR_L1.2_THRESHOLD_Value */

^ permalink raw reply related	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-10-16 20:59 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-15 19:30 [PATCH v3 00/12] PCI/ASPM: Cleanup Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 01/12] PCI/ASPM: Move pci_clear_and_set_dword() earlier Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 02/12] PCI/ASPM: Move LTR path check to where it's used Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 03/12] PCI/ASPM: Use 'parent' and 'child' for readability Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 04/12] PCI/ASPM: Remove struct aspm_register_info.support Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 05/12] PCI/ASPM: Remove struct aspm_register_info.enabled Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 06/12] PCI/ASPM: Remove struct aspm_register_info.latency_encoding Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 07/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_cap_ptr Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 08/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_ctl2 (unused) Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 09/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_ctl1 Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 10/12] PCI/ASPM: Pass L1SS Capabilities value, not struct aspm_register_info Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 11/12] PCI/ASPM: Remove struct aspm_register_info.l1ss_cap Bjorn Helgaas
2020-10-15 19:30 ` [PATCH v3 12/12] PCI/ASPM: Remove struct pcie_link_state.l1ss Bjorn Helgaas
2020-10-16 20:59   ` Bjorn Helgaas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).