[PATCH 0/6] PCI/ASPM: Add PCIe L1 PM substate support

[PATCH 0/6] PCI/ASPM: Add PCIe L1 PM substate support

[Rajat Jain]

This patchset adds the PCIe L1 PM substate support to the kernel.
The feature is described at:
https://pcisig.com/sites/default/files/specification_documents/ECN_L1_PM_Substates_with_CLKREQ_31_May_2013_Rev10a.pdf

Its all logically one patch (and may be some of them should be
squashed later) , but I've broken down into smaller patches for
ease of review. 

This is currently rebased on top of Bjorn's master branch.

Rajat Jain (6):
  PCI: Add L1 substate capability structure register definitions
  PCI/ASPM: Introduce L1 substates and a Kconfig for it
  PCI/ASPM: Read and setup L1 substate capabilities
  PCI/ASPM: Calculate and save the L1.2 timing parameters
  PCI/ASPM: Actually configure the L1 substate settings to the device
  PCI/ASPM: Add comment about L1 substate latency

 drivers/pci/pcie/Kconfig      |   8 ++
 drivers/pci/pcie/aspm.c       | 291 ++++++++++++++++++++++++++++++++++++++++--
 include/uapi/linux/pci_regs.h |  16 +++
 3 files changed, 302 insertions(+), 13 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH 1/6] PCI: Add L1 substate capability structure register definitions

[Rajat Jain]

These definitions will be used in subsequent patches.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 include/uapi/linux/pci_regs.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 174d114..f48d06e 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -682,6 +682,7 @@
 #define PCI_EXT_CAP_ID_PMUX	0x1A	/* Protocol Multiplexing */
 #define PCI_EXT_CAP_ID_PASID	0x1B	/* Process Address Space ID */
 #define PCI_EXT_CAP_ID_DPC	0x1D	/* Downstream Port Containment */
+#define PCI_EXT_CAP_ID_L1SS	0x1E	/* L1 PM Substates */
 #define PCI_EXT_CAP_ID_PTM	0x1F	/* Precision Time Measurement */
 #define PCI_EXT_CAP_ID_MAX	PCI_EXT_CAP_ID_PTM
 
@@ -985,4 +986,19 @@
 #define  PCI_PTM_CTRL_ENABLE		0x00000001  /* PTM enable */
 #define  PCI_PTM_CTRL_ROOT		0x00000002  /* Root select */
 
+/* L1 PM Substates */
+#define PCI_L1SS_CAP		    4	/* capability register */
+#define  PCI_L1SS_CAP_PCIPM_L1_2	 1	/* PCI PM L1.2 Support */
+#define  PCI_L1SS_CAP_PCIPM_L1_1	 2	/* PCI PM L1.1 Support */
+#define  PCI_L1SS_CAP_ASPM_L1_2		 4	/* ASPM L1.2 Support */
+#define  PCI_L1SS_CAP_ASPM_L1_1		 8	/* ASPM L1.1 Support */
+#define  PCI_L1SS_CAP_L1_PM_SS		16	/* L1 PM Substates Support */
+#define PCI_L1SS_CTL1		    8	/* Control Register 1 */
+#define  PCI_L1SS_CTL1_PCIPM_L1_2	1	/* PCI PM L1.2 Enable */
+#define  PCI_L1SS_CTL1_PCIPM_L1_1	2	/* PCI PM L1.1 Support */
+#define  PCI_L1SS_CTL1_ASPM_L1_2	4	/* ASPM L1.2 Support */
+#define  PCI_L1SS_CTL1_ASPM_L1_1	8	/* ASPM L1.1 Support */
+#define  PCI_L1SS_CTL1_L1SS_MASK	0x0000000F
+#define PCI_L1SS_CTL2		    0xC	/* Control Register 2 */
+
 #endif /* LINUX_PCI_REGS_H */
-- 
2.8.0.rc3.226.g39d4020

[PATCH 2/6] PCI/ASPM: Introduce L1 substates and a Kconfig for it

[Rajat Jain]

Introduce the L1 sub states. (For details about L1 substates,
please refer to:
https://pcisig.com/sites/default/files/specification_documents/ECN_L1_PM_Substates_with_CLKREQ_31_May_2013_Rev10a.pdf)

This patch adds macros for the 4 new L1 substates, and adds
a new ASPM "POWER_SUPERSAVE" policy that can be used to enable
L1 substates on a system if desired. The new policy is in a sense,
a super set of the existing POWERSAVE policy. The 4 policies are now:

DEFAULT: Reads and uses whatever ASPM states BIOS enabled
PERFORMANCE: Everything except L0 disabled.
POWERSAVE: L0s and L1 enabled (but not L1 substates)
POWER_SUPERSAVE: L0s + L1 + L1 substates also enabled.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/pci/pcie/Kconfig |  8 ++++++++
 drivers/pci/pcie/aspm.c  | 39 +++++++++++++++++++++++++++++----------
 2 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index 7ce7763..ac53edb 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -71,6 +71,14 @@ config PCIEASPM_POWERSAVE
 	  Enable PCI Express ASPM L0s and L1 where possible, even if the
 	  BIOS did not.
 
+config PCIEASPM_POWER_SUPERSAVE
+	bool "Power Supersave"
+	depends on PCIEASPM
+	help
+	  Same as PCIEASPM_POWERSAVE, except it also enables L1 substates where
+	  possible. This would result in higher power savings while staying in L1
+	  where the components support it.
+
 config PCIEASPM_PERFORMANCE
 	bool "Performance"
 	depends on PCIEASPM
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 17ac1dc..402c229 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -30,8 +30,17 @@
 #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)
+#define ASPM_STATE_ALL		(ASPM_STATE_L0S | ASPM_STATE_L1 |	\
+				 ASPM_STATE_L1SS)
 
 struct aspm_latency {
 	u32 l0s;			/* L0s latency (nsec) */
@@ -47,11 +56,11 @@ struct pcie_link_state {
 	struct list_head link;		/* node in parent's children list */
 
 	/* ASPM state */
-	u32 aspm_support:3;		/* Supported ASPM state */
-	u32 aspm_enabled:3;		/* Enabled ASPM state */
-	u32 aspm_capable:3;		/* Capable ASPM state with latency */
-	u32 aspm_default:3;		/* Default ASPM state by BIOS */
-	u32 aspm_disable:3;		/* Disabled ASPM state */
+	u32 aspm_support:7;		/* Supported ASPM state */
+	u32 aspm_enabled:7;		/* Enabled ASPM state */
+	u32 aspm_capable:7;		/* Capable ASPM state with latency */
+	u32 aspm_default:7;		/* Default ASPM state by BIOS */
+	u32 aspm_disable:7;		/* Disabled ASPM state */
 
 	/* Clock PM state */
 	u32 clkpm_capable:1;		/* Clock PM capable? */
@@ -76,11 +85,14 @@ static LIST_HEAD(link_list);
 #define POLICY_DEFAULT 0	/* BIOS default setting */
 #define POLICY_PERFORMANCE 1	/* high performance */
 #define POLICY_POWERSAVE 2	/* high power saving */
+#define POLICY_POWER_SUPERSAVE 3 /* Possibly Even higher power saving */
 
 #ifdef CONFIG_PCIEASPM_PERFORMANCE
 static int aspm_policy = POLICY_PERFORMANCE;
 #elif defined CONFIG_PCIEASPM_POWERSAVE
 static int aspm_policy = POLICY_POWERSAVE;
+#elif defined CONFIG_PCIEASPM_POWER_SUPERSAVE
+static int aspm_policy = POLICY_POWER_SUPERSAVE;
 #else
 static int aspm_policy;
 #endif
@@ -88,7 +100,8 @@ static int aspm_policy;
 static const char *policy_str[] = {
 	[POLICY_DEFAULT] = "default",
 	[POLICY_PERFORMANCE] = "performance",
-	[POLICY_POWERSAVE] = "powersave"
+	[POLICY_POWERSAVE] = "powersave",
+	[POLICY_POWER_SUPERSAVE] = "powersupersave"
 };
 
 #define LINK_RETRAIN_TIMEOUT HZ
@@ -101,6 +114,9 @@ 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);
+	case POLICY_POWER_SUPERSAVE:
+		/* Enable Everything */
 		return ASPM_STATE_ALL;
 	case POLICY_DEFAULT:
 		return link->aspm_default;
@@ -115,7 +131,8 @@ static int policy_to_clkpm_state(struct pcie_link_state *link)
 		/* Disable ASPM and Clock PM */
 		return 0;
 	case POLICY_POWERSAVE:
-		/* Disable Clock PM */
+	case POLICY_POWER_SUPERSAVE:
+		/* Enable Clock PM */
 		return 1;
 	case POLICY_DEFAULT:
 		return link->clkpm_default;
@@ -612,7 +629,8 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
 	 * the BIOS's expectation, we'll do so once pci_enable_device() is
 	 * called.
 	 */
-	if (aspm_policy != POLICY_POWERSAVE) {
+	if (aspm_policy != POLICY_POWERSAVE &&
+	    aspm_policy != POLICY_POWER_SUPERSAVE) {
 		pcie_config_aspm_path(link);
 		pcie_set_clkpm(link, policy_to_clkpm_state(link));
 	}
@@ -712,7 +730,8 @@ void pcie_aspm_powersave_config_link(struct pci_dev *pdev)
 	if (aspm_disabled || !link)
 		return;
 
-	if (aspm_policy != POLICY_POWERSAVE)
+	if (aspm_policy != POLICY_POWERSAVE &&
+	    aspm_policy != POLICY_POWER_SUPERSAVE)
 		return;
 
 	down_read(&pci_bus_sem);
-- 
2.8.0.rc3.226.g39d4020

[PATCH 3/6] PCI/ASPM: Read and setup L1 substate capabilities

[Rajat Jain]

The PCIe spec says that only function 0 of a multi-function
downstream component would implement the capability structure.

This patch adds code to read the L1 substate capability structures
of upstream and downstream components of the link, and sets it up
in the device structure.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/pci/pcie/aspm.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 402c229..7a3ad85 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -49,6 +49,7 @@ struct aspm_latency {
 
 struct pcie_link_state {
 	struct pci_dev *pdev;		/* Upstream component of the Link */
+	struct pci_dev *downstream;	/* Downstream component, function 0 */
 	struct pcie_link_state *root;	/* pointer to the root port link */
 	struct pcie_link_state *parent;	/* pointer to the parent Link state */
 	struct list_head sibling;	/* node in link_list */
@@ -300,6 +301,12 @@ struct aspm_register_info {
 	u32 enabled:2;
 	u32 latency_encoding_l0s;
 	u32 latency_encoding_l1;
+
+	/* L1 substates */
+	u32 l1ss_cap_ptr;
+	u32 l1ss_cap;
+	u32 l1ss_ctl1;
+	u32 l1ss_ctl2;
 };
 
 static void pcie_get_aspm_reg(struct pci_dev *pdev,
@@ -314,6 +321,22 @@ static void pcie_get_aspm_reg(struct pci_dev *pdev,
 	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;
+	info->l1ss_cap_ptr = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
+	if (!info->l1ss_cap_ptr)
+		return;
+	pci_read_config_dword(pdev, info->l1ss_cap_ptr + 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,
+			      &info->l1ss_ctl1);
+	pci_read_config_dword(pdev, info->l1ss_cap_ptr + PCI_L1SS_CTL2,
+			      &info->l1ss_ctl2);
 }
 
 static void pcie_aspm_check_latency(struct pci_dev *endpoint)
@@ -355,6 +378,20 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
 	}
 }
 
+/*
+ * The L1 PM substate capability is only implemented in function 0 in a
+ * multi function device.
+ */
+static inline struct pci_dev *pci_function_0(struct pci_bus *linkbus)
+{
+	struct pci_dev *child;
+
+	list_for_each_entry(child, &linkbus->devices, bus_list)
+		if (PCI_FUNC(child->devfn) == 0)
+			return child;
+	return NULL;
+}
+
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	struct pci_dev *child, *parent = link->pdev;
@@ -370,8 +407,9 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 
 	/* Get upstream/downstream components' register state */
 	pcie_get_aspm_reg(parent, &upreg);
-	child = list_entry(linkbus->devices.next, struct pci_dev, bus_list);
+	child = pci_function_0(linkbus);
 	pcie_get_aspm_reg(child, &dwreg);
+	link->downstream = child;
 
 	/*
 	 * If ASPM not supported, don't mess with the clocks and link,
@@ -414,6 +452,25 @@ 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 */
+	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)
+		link->aspm_support |= ASPM_STATE_L1_2;
+	if (upreg.l1ss_cap & dwreg.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)
+		link->aspm_support |= ASPM_STATE_L1_2_PCIPM;
+
+	if (upreg.l1ss_ctl1 & dwreg.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)
+		link->aspm_enabled |= ASPM_STATE_L1_2;
+	if (upreg.l1ss_ctl1 & dwreg.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)
+		link->aspm_enabled |= ASPM_STATE_L1_2_PCIPM;
+
 	/* Save default state */
 	link->aspm_default = link->aspm_enabled;
 
-- 
2.8.0.rc3.226.g39d4020

[PATCH 4/6] PCI/ASPM: Calculate and save the L1.2 timing parameters

[Rajat Jain]

Calculate and save the timing parameters that need to be programmed
if we need to enable L1.2 substates later.

We use the same logic (and a constant value for 1 of the
parameters) as used by Intel's coreboot:

https://www.coreboot.org/pipermail/coreboot-gerrit/2015-March/021134.html
https://review.coreboot.org/#/c/8832/

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/pci/pcie/aspm.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 7a3ad85..a70afdf 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -42,6 +42,18 @@
 #define ASPM_STATE_ALL		(ASPM_STATE_L0S | ASPM_STATE_L1 |	\
 				 ASPM_STATE_L1SS)
 
+/*
+ * When L1 substates are enabled, the LTR L1.2 threshold is a timing parameter
+ * that decides whether L1.1 or L1.2 is entered (Refer PCIe spec for details).
+ * Not sure is there is a way to "calculate" this on the fly, but may be we
+ * could turn it into a parameter in future. This value has been taken from
+ * the following files from Intel's coreboot (which is the only code I found
+ * to have used this):
+ * https://www.coreboot.org/pipermail/coreboot-gerrit/2015-March/021134.html
+ * https://review.coreboot.org/#/c/8832/
+ */
+#define LTR_L1_2_THRESHOLD_BITS	((1 << 21) | (1 << 23) | (1 << 30))
+
 struct aspm_latency {
 	u32 l0s;			/* L0s latency (nsec) */
 	u32 l1;				/* L1 latency (nsec) */
@@ -76,6 +88,14 @@ 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 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;
 };
 
 static int aspm_disabled, aspm_force;
@@ -296,6 +316,22 @@ static u32 calc_l1_acceptable(u32 encoding)
 	return (1000 << encoding);
 }
 
+/* Convert L1SS T_pwr encoding to usec */
+static u32 calc_l1ss_pwron(struct pci_dev *pdev, u32 scale, u32 val)
+{
+	switch (scale) {
+	case 0:
+		return val * 2;
+	case 1:
+		return val * 10;
+	case 2:
+		return val * 100;
+	}
+	dev_err(&pdev->dev, "%s: Invalid T_PwrOn scale: %u\n",
+		__func__, scale);
+	return 0;
+}
+
 struct aspm_register_info {
 	u32 support:2;
 	u32 enabled:2;
@@ -392,6 +428,46 @@ static inline struct pci_dev *pci_function_0(struct pci_bus *linkbus)
 	return NULL;
 }
 
+/* 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 val1, val2, scale1, scale2;
+
+	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))
+		return;
+
+	/* Choose the greater of the two T_cmn_mode_rstr_time */
+	val1 = (upreg->l1ss_cap >> 8) & 0xFF;
+	val2 = (upreg->l1ss_cap >> 8) & 0xFF;
+	if (val1 > val2)
+		link->l1ss.ctl1 |= val1 << 8;
+	else
+		link->l1ss.ctl1 |= val2 << 8;
+	/*
+	 * We currently use LTR L1.2 threshold to be fixed constant picked from
+	 * Intel's coreboot.
+	 */
+	link->l1ss.ctl1 |= LTR_L1_2_THRESHOLD_BITS;
+
+	/* Choose the greater of the two T_pwr_on */
+	val1 = (upreg->l1ss_cap >> 19) & 0x1F;
+	scale1 = (upreg->l1ss_cap >> 16) & 0x03;
+	val2 = (dwreg->l1ss_cap >> 19) & 0x1F;
+	scale2 = (dwreg->l1ss_cap >> 16) & 0x03;
+
+	if (calc_l1ss_pwron(link->pdev, scale1, val1) >
+	    calc_l1ss_pwron(link->downstream, scale2, val2))
+		link->l1ss.ctl2 |= scale1 | (val1 << 3);
+	else
+		link->l1ss.ctl2 |= scale2 | (val2 << 3);
+}
+
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	struct pci_dev *child, *parent = link->pdev;
@@ -471,6 +547,9 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	if (upreg.l1ss_ctl1 & dwreg.l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_2)
 		link->aspm_enabled |= ASPM_STATE_L1_2_PCIPM;
 
+	if (link->aspm_support & ASPM_STATE_L1SS)
+		aspm_calc_l1ss_info(link, &upreg, &dwreg);
+
 	/* Save default state */
 	link->aspm_default = link->aspm_enabled;
 
-- 
2.8.0.rc3.226.g39d4020

[PATCH 5/6] PCI/ASPM: Actually configure the L1 substate settings to the device

[Rajat Jain]

Add code to actually configure the L1 substate settigns on the
upstream and downstream device, while taking care of the rules
dictated by the PCIe spec.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/pci/pcie/aspm.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 104 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index a70afdf..6735f38 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -588,6 +588,92 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	}
 }
 
+static inline 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)
+{
+	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;
+
+	/*
+	 * 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 timin
+	 *   parameters
+	 *
+	 * To keep it simple, disable all L1SS bits first, and later enable
+	 * what is needed.
+	 */
+
+	/* Disable all L1 substates */
+	pci_clear_and_set_dword(child, dw_cap_ptr + PCI_L1SS_CTL1,
+				PCI_L1SS_CTL1_L1SS_MASK, 0);
+	pci_clear_and_set_dword(parent, up_cap_ptr + 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 & (ASPM_STATE_L1_1 | ASPM_STATE_L1_2)) {
+		pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
+						   PCI_EXP_LNKCTL_ASPM_L1, 0);
+		pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
+						   PCI_EXP_LNKCTL_ASPM_L1, 0);
+	}
+
+	if (enable_req & ASPM_STATE_L1_2_MASK) {
+
+		/* Program T_pwr_on in both ports */
+		pci_write_config_dword(parent, up_cap_ptr + PCI_L1SS_CTL2,
+				       link->l1ss.ctl2);
+		pci_write_config_dword(child, dw_cap_ptr + PCI_L1SS_CTL2,
+				       link->l1ss.ctl2);
+
+		/* Program T_cmn_mode in parent */
+		pci_clear_and_set_dword(parent, up_cap_ptr + PCI_L1SS_CTL1,
+					0xFF00, link->l1ss.ctl1);
+
+		/* Program LTR L1.2 threshold in both ports */
+		pci_clear_and_set_dword(parent,	dw_cap_ptr + PCI_L1SS_CTL1,
+					0xE3FF0000, link->l1ss.ctl1);
+		pci_clear_and_set_dword(child, dw_cap_ptr + PCI_L1SS_CTL1,
+					0xE3FF0000, link->l1ss.ctl1);
+	}
+
+	val = 0;
+	if (state & ASPM_STATE_L1_1)
+		val |= PCI_L1SS_CTL1_ASPM_L1_1;
+	if (state & ASPM_STATE_L1_2)
+		val |= PCI_L1SS_CTL1_ASPM_L1_2;
+	if (state & ASPM_STATE_L1_1_PCIPM)
+		val |= PCI_L1SS_CTL1_PCIPM_L1_1;
+	if (state & ASPM_STATE_L1_2_PCIPM)
+		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_L1SS_CAP_L1_PM_SS, val);
+	pci_clear_and_set_dword(child, dw_cap_ptr + PCI_L1SS_CTL1,
+				PCI_L1SS_CAP_L1_PM_SS, val);
+}
+
 static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
 {
 	pcie_capability_clear_and_set_word(pdev, PCI_EXP_LNKCTL,
@@ -597,11 +683,23 @@ static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
 static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
 {
 	u32 upstream = 0, dwstream = 0;
-	struct pci_dev *child, *parent = link->pdev;
+	struct pci_dev *child = link->downstream, *parent = link->pdev;
 	struct pci_bus *linkbus = parent->subordinate;
 
-	/* Nothing to do if the link is already in the requested state */
+	/* Enable only the states that were not explicitly disabled */
 	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;
+
+	/* 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);
+	}
+
+	/* 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 */
@@ -613,6 +711,10 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
 		upstream |= PCI_EXP_LNKCTL_ASPM_L1;
 		dwstream |= PCI_EXP_LNKCTL_ASPM_L1;
 	}
+
+	if (link->aspm_capable & ASPM_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
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 5/6] PCI/ASPM: Actually configure the L1 substate settings to the device

[James Morse]

Hi!

On 03/01/17 06:34, Rajat Jain wrote:
> Add code to actually configure the L1 substate settigns on the
> upstream and downstream device, while taking care of the rules
> dictated by the PCIe spec.

While testing hibernate on an arm64 juno with v4.11-rc1, I get a NULL pointer
dereference from pcie_config_aspm_link():


> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index a70afdf..6735f38 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -597,11 +683,23 @@ static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
>  static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
>  {
>  	u32 upstream = 0, dwstream = 0;
> -	struct pci_dev *child, *parent = link->pdev;
> +	struct pci_dev *child = link->downstream, *parent = link->pdev;


Here link->downstream is NULL,


>  	struct pci_bus *linkbus = parent->subordinate;
>  
> -	/* Nothing to do if the link is already in the requested state */
> +	/* Enable only the states that were not explicitly disabled */
>  	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;
> +
> +	/* Spec says both ports must be in D0 before enabling PCI PM substates*/
> +	if (parent->current_state != PCI_D0 || child->current_state != PCI_D0) {

Which causes a crash[0] here.


> +		state &= ~ASPM_STATE_L1_SS_PCIPM;
> +		state |= (link->aspm_enabled & ASPM_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 */


(For trees based on v4.10-rc1 I need to cherry-pick b4b8664d291a to make arm64
build)

Testing either side of aeda9adebab8 gives this patch as the first to expose the
problem.


At boot I get the message:
> pci 0000:03:00.0: disabling ASPM on pre-1.1 PCIe device.  You can enable it
with 'pcie_aspm=force'

for the pcie device being resumed here, should this code be called at all for
this device?

The 'pci' section of the boot log for v4.11-rc1 is below[1], and lspci[2].


Thanks,

James


[0] git checkout aeda9adebab8; git cherry-pick b4b8664d291a
[  170.379816] PM: noirq thaw of devices complete after 0.702 msecs
[  170.387234] PM: early thaw of devices complete after 1.390 msecs
[  170.421447] Unable to handle kernel NULL pointer dereference at virtual addre
ss 00000080
[  170.429497] pgd = ffff000008f10000
[  170.432878] [00000080] *pgd=00000009ffffe003
[  170.432882] , *pud=00000009ffffd003
[  170.437118] , *pmd=0000000000000000
[  170.440579]

[  170.445513] Internal error: Oops: 96000004 [#1] PREEMPT SMP
[  170.451027] Modules linked in:
[  170.454055] CPU: 4 PID: 2509 Comm: kworker/u12:9 Not tainted 4.10.0-rc1-00006
-g51e9dca242d2 #7202
[  170.462837] Hardware name: ARM Juno development board (r1) (DT)
[  170.468707] Workqueue: events_unbound async_run_entry_fn
[  170.473965] task: ffff8009764c2580 task.stack: ffff80097374c000
[  170.479831] PC is at pcie_config_aspm_link+0x4c/0x300
[  170.484833] LR is at pcie_config_aspm_path+0x40/0x88
[  170.489747] pc : [<ffff000008411ebc>] lr : [<ffff0000084121b0>] pstate: 40000
045
[  170.497066] sp : ffff80097374fb40
[  170.500343] x29: ffff80097374fb40 x28: ffff800976c0c2a8
[  170.505603] x27: ffff800976c0c078 x26: ffff800976c0c020
[  170.510863] x25: 0000000000000001 x24: ffff8009750f9120
[  170.516122] x23: ffff8009764bd100 x22: ffff000008045000
[  170.521381] x21: ffff800976c24000 x20: ffff000008ef3320
[  170.526640] x19: 0000000000000000 x18: 0000000000000000
[  170.531899] x17: 0000000000000000 x16: 0000000000000000
[  170.537158] x15: 0000000000000000 x14: 00000000000000dc
[  170.542417] x13: ffff000008e17628 x12: ffff800976fa8028
[  170.547676] x11: ffff80097641f000 x10: ffff000008e17608
[  170.552936] x9 : ffff800976fa8028 x8 : 0000000000000000
[  170.558194] x7 : 000000000000007b x6 : 0000000000000000
[  170.563453] x5 : 0000000000000fa0 x4 : ffff80097641fd60
[  170.568712] x3 : 0000000000000000 x2 : 0000000000000000
[  170.573971] x1 : 0000000000000000 x0 : ffff80097641fd00
[  170.579229]
[  170.580701] Process kworker/u12:9 (pid: 2509, stack limit = 0xffff80097374c00
0)
[  170.587936] Stack: (0xffff80097374fb40 to 0xffff800973750000)
[  170.888248] Call trace:
[  170.974579] [<ffff000008411ebc>] pcie_config_aspm_link+0x4c/0x300
[  170.980614] [<ffff0000084121b0>] pcie_config_aspm_path+0x40/0x88
[  170.986564] [<ffff0000084130f4>] pcie_aspm_pm_state_change+0x5c/0x80
[  170.992856] [<ffff0000083ff0a4>] pci_raw_set_power_state+0x15c/0x230
[  170.999148] [<ffff0000084018ec>] pci_set_power_state+0x64/0x150
[  171.005013] [<ffff00000859ae20>] ata_pci_device_do_resume+0x18/0x70
[  171.011220] [<ffff0000085baa3c>] sil24_pci_device_resume+0x24/0x78
[  171.017341] [<ffff000008404fd8>] pci_legacy_resume+0x38/0x58
[  171.022945] [<ffff000008405f00>] pci_pm_thaw+0xa0/0xd0
[  171.028034] [<ffff000008546ce4>] dpm_run_callback.isra.4+0x1c/0x70
[  171.034154] [<ffff0000085470a0>] device_resume+0x88/0x150
[  171.039500] [<ffff00000854718c>] async_resume+0x24/0x58
[  171.044674] [<ffff0000080de55c>] async_run_entry_fn+0x3c/0xf8
[  171.050365] [<ffff0000080d4de8>] process_one_work+0x1d0/0x390
[  171.056055] [<ffff0000080d4ff0>] worker_thread+0x48/0x480
[  171.061400] [<ffff0000080daec0>] kthread+0xf8/0x128
[  171.066230] [<ffff0000080836c0>] ret_from_fork+0x10/0x50
[  171.071490] Code: 12196e61 f27e027f 1a930033 35000063 (b9408101)
[  171.077566] ---[ end trace 93b51adc52c63085 ]---

[1] v4.11-rc1 pci boot messages
[    0.732114] OF: PCI: host bridge /pcie-controller@30000000 ranges:
[    0.732133] OF: PCI:    IO 0x5f800000..0x5fffffff -> 0x5f800000
[    0.732145] OF: PCI:   MEM 0x50000000..0x57ffffff -> 0x50000000
[    0.732152] OF: PCI:   MEM 0x4000000000..0x40ffffffff -> 0x4000000000
[    0.732204] pci-host-generic 40000000.pcie-controller: ECAM at [mem 0x4000000
0-0x4fffffff] for [bus 00-ff]
[    0.732401] pci-host-generic 40000000.pcie-controller: PCI host bridge to bus
 0000:00
[    0.732410] pci_bus 0000:00: root bus resource [bus 00-ff]
[    0.732421] pci_bus 0000:00: root bus resource [io  0x0000-0x7fffff] (bus add
ress [0x5f800000-0x5fffffff])
[    0.732428] pci_bus 0000:00: root bus resource [mem 0x50000000-0x57ffffff]
[    0.732435] pci_bus 0000:00: root bus resource [mem 0x4000000000-0x40ffffffff
 pref]
[    0.732462] pci 0000:00:00.0: [1556:1100] type 01 class 0x060400
[    0.732485] pci 0000:00:00.0: reg 0x10: [mem 0x57d00000-0x57d03fff 64bit pref]
[    0.732543] pci 0000:00:00.0: supports D1 D2
[    0.732832] pci 0000:00:00.0: PME# supported from D0 D1 D2 D3hot D3cold
[    0.733252] pci 0000:01:00.0: [111d:8090] type 01 class 0x060400
[    0.733423] pci 0000:01:00.0: PME# supported from D0 D3hot D3cold
[    0.744804] pci 0000:02:01.0: [111d:8090] type 01 class 0x060400
[    0.744993] pci 0000:02:01.0: PME# supported from D0 D3hot D3cold
[    0.745223] pci 0000:02:02.0: [111d:8090] type 01 class 0x060400
[    0.745414] pci 0000:02:02.0: PME# supported from D0 D3hot D3cold
[    0.745641] pci 0000:02:03.0: [111d:8090] type 01 class 0x060400
[    0.745828] pci 0000:02:03.0: PME# supported from D0 D3hot D3cold
[    0.746076] pci 0000:02:0c.0: [111d:8090] type 01 class 0x060400
[    0.746263] pci 0000:02:0c.0: PME# supported from D0 D3hot D3cold
[    0.746482] pci 0000:02:10.0: [111d:8090] type 01 class 0x060400
[    0.746646] pci 0000:02:10.0: PME# supported from D0 D3hot D3cold
[    0.746886] pci 0000:02:1f.0: [111d:8090] type 01 class 0x060400
[    0.747074] pci 0000:02:1f.0: PME# supported from D0 D3hot D3cold
[    0.747490] pci 0000:03:00.0: [1095:3132] type 00 class 0x018000
[    0.747531] pci 0000:03:00.0: reg 0x10: [mem 0x57f04000-0x57f0407f 64bit]
[    0.747559] pci 0000:03:00.0: reg 0x18: [mem 0x57f00000-0x57f03fff 64bit]
[    0.747577] pci 0000:03:00.0: reg 0x20: initial BAR value 0x00000000 invalid
[    0.747584] pci 0000:03:00.0: reg 0x20: [io  size 0x0080]
[    0.747614] pci 0000:03:00.0: reg 0x30: [mem 0xfff80000-0xffffffff pref]
[    0.747720] pci 0000:03:00.0: supports D1 D2
[    0.747886] pci 0000:03:00.0: disabling ASPM on pre-1.1 PCIe device.  You can
 enable it with 'pcie_aspm=force'
[    0.747910] pci_bus 0000:03: busn_res: [bus 03-ff] end is updated to 03
[    0.748044] pci_bus 0000:04: busn_res: [bus 04-ff] end is updated to 04
[    0.748180] pci_bus 0000:05: busn_res: [bus 05-ff] end is updated to 05
[    0.748311] pci_bus 0000:06: busn_res: [bus 06-ff] end is updated to 06
[    0.748441] pci_bus 0000:07: busn_res: [bus 07-ff] end is updated to 07
[    0.748614] pci 0000:08:00.0: [11ab:4380] type 00 class 0x020000
[    0.748651] pci 0000:08:00.0: reg 0x10: [mem 0x57e00000-0x57e03fff 64bit]
[    0.748668] pci 0000:08:00.0: reg 0x18: initial BAR value 0x00000000 invalid
[    0.748675] pci 0000:08:00.0: reg 0x18: [io  size 0x0100]
[    0.748814] pci 0000:08:00.0: supports D1 D2
[    0.748821] pci 0000:08:00.0: PME# supported from D0 D1 D2 D3hot D3cold
[    0.749085] pci_bus 0000:08: busn_res: [bus 08-ff] end is updated to 08
[    0.749099] pci_bus 0000:02: busn_res: [bus 02-ff] end is updated to 08
[    0.749112] pci_bus 0000:01: busn_res: [bus 01-ff] end is updated to 08
[    0.749249] pci 0000:00:00.0: BAR 14: assigned [mem 0x50000000-0x501fffff]
[    0.749260] pci 0000:00:00.0: BAR 0: assigned [mem 0x4000000000-0x4000003fff
64bit pref]
[    0.749272] pci 0000:00:00.0: BAR 13: assigned [io  0x1000-0x2fff]
[    0.749283] pci 0000:01:00.0: BAR 14: assigned [mem 0x50000000-0x501fffff]
[    0.749290] pci 0000:01:00.0: BAR 13: assigned [io  0x1000-0x2fff]
[    0.749303] pci 0000:02:01.0: BAR 14: assigned [mem 0x50000000-0x500fffff]
[    0.749311] pci 0000:02:1f.0: BAR 14: assigned [mem 0x50100000-0x501fffff]
[    0.749318] pci 0000:02:01.0: BAR 13: assigned [io  0x1000-0x1fff]
[    0.749325] pci 0000:02:1f.0: BAR 13: assigned [io  0x2000-0x2fff]
[    0.749337] pci 0000:03:00.0: BAR 6: assigned [mem 0x50000000-0x5007ffff pref]
[    0.749347] pci 0000:03:00.0: BAR 2: assigned [mem 0x50080000-0x50083fff 64bit]
[    0.749372] pci 0000:03:00.0: BAR 0: assigned [mem 0x50084000-0x5008407f 64bit]
[    0.749394] pci 0000:03:00.0: BAR 4: assigned [io  0x1000-0x107f]
[    0.749407] pci 0000:02:01.0: PCI bridge to [bus 03]
[    0.749416] pci 0000:02:01.0:   bridge window [io  0x1000-0x1fff]
[    0.749429] pci 0000:02:01.0:   bridge window [mem 0x50000000-0x500fffff]
[    0.749451] pci 0000:02:02.0: PCI bridge to [bus 04]
[    0.749479] pci 0000:02:03.0: PCI bridge to [bus 05]
[    0.749507] pci 0000:02:0c.0: PCI bridge to [bus 06]
[    0.749535] pci 0000:02:10.0: PCI bridge to [bus 07]
[    0.749569] pci 0000:08:00.0: BAR 0: assigned [mem 0x50100000-0x50103fff 64bit]
[    0.749590] pci 0000:08:00.0: BAR 2: assigned [io  0x2000-0x20ff]
[    0.749600] pci 0000:02:1f.0: PCI bridge to [bus 08]
[    0.749608] pci 0000:02:1f.0:   bridge window [io  0x2000-0x2fff]
[    0.749622] pci 0000:02:1f.0:   bridge window [mem 0x50100000-0x501fffff]
[    0.749643] pci 0000:01:00.0: PCI bridge to [bus 02-08]
[    0.749651] pci 0000:01:00.0:   bridge window [io  0x1000-0x2fff]
[    0.749664] pci 0000:01:00.0:   bridge window [mem 0x50000000-0x501fffff]
[    0.749685] pci 0000:00:00.0: PCI bridge to [bus 01-08]
[    0.749691] pci 0000:00:00.0:   bridge window [io  0x1000-0x2fff]
[    0.749699] pci 0000:00:00.0:   bridge window [mem 0x50000000-0x501fffff]
[    0.750009] pcieport 0000:00:00.0: Signaling PME with IRQ 39
[    0.750155] pcieport 0000:00:00.0: AER enabled with IRQ 39

[2] lspci
root@juno-r1:~# lspci
00:00.0 PCI bridge: PLDA PCI Express Core Reference Design (rev 01)
01:00.0 PCI bridge: Integrated Device Technology, Inc. [IDT] Device 8090 (rev 02
)
02:01.0 PCI bridge: Integrated Device Technology, Inc. [IDT] Device 8090 (rev 02
)
02:02.0 PCI bridge: Integrated Device Technology, Inc. [IDT] Device 8090 (rev 02
)
02:03.0 PCI bridge: Integrated Device Technology, Inc. [IDT] Device 8090 (rev 02
)
02:0c.0 PCI bridge: Integrated Device Technology, Inc. [IDT] Device 8090 (rev 02
)
02:10.0 PCI bridge: Integrated Device Technology, Inc. [IDT] Device 8090 (rev 02
)
02:1f.0 PCI bridge: Integrated Device Technology, Inc. [IDT] Device 8090 (rev 02
)
03:00.0 Mass storage controller: Silicon Image, Inc. SiI 3132 Serial ATA Raid II
 Controller (rev 01)
08:00.0 Ethernet controller: Marvell Technology Group Ltd. 88E8057 PCI-E Gigabit
 Ethernet Controller

Re: [PATCH 5/6] PCI/ASPM: Actually configure the L1 substate settings to the device

[Bjorn Helgaas]

Hi James,

On Wed, Mar 08, 2017 at 06:44:36PM +0000, James Morse wrote:
> Hi!
> 
> On 03/01/17 06:34, Rajat Jain wrote:
> > Add code to actually configure the L1 substate settigns on the
> > upstream and downstream device, while taking care of the rules
> > dictated by the PCIe spec.
> 
> While testing hibernate on an arm64 juno with v4.11-rc1, I get a NULL pointer
> dereference from pcie_config_aspm_link():
> 
> 
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index a70afdf..6735f38 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -597,11 +683,23 @@ static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
> >  static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
> >  {
> >  	u32 upstream = 0, dwstream = 0;
> > -	struct pci_dev *child, *parent = link->pdev;
> > +	struct pci_dev *child = link->downstream, *parent = link->pdev;
> 
> 
> Here link->downstream is NULL,

Sorry about the breakage.  Can you try also cherry-picking this
commit:

https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=for-linus&id=3bd7db63a841e8c5297bb18ad801df67d5e38ad2

Yinghai tripped over a similar problem in a different way, but I
suspect his fix might also fix the problem you're seeing.

If that doesn't fix it, we'll have to look farther.

I'm planning to ask Linus to pull this fix for v4.11-rc2.

Bjorn

Re: [PATCH 5/6] PCI/ASPM: Actually configure the L1 substate settings to the device

[Rajat Jain]

[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]

On Wed, Mar 8, 2017 at 2:39 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:

> Hi James,
>
> On Wed, Mar 08, 2017 at 06:44:36PM +0000, James Morse wrote:
> > Hi!
> >
> > On 03/01/17 06:34, Rajat Jain wrote:
> > > Add code to actually configure the L1 substate settigns on the
> > > upstream and downstream device, while taking care of the rules
> > > dictated by the PCIe spec.
> >
> > While testing hibernate on an arm64 juno with v4.11-rc1, I get a NULL
> pointer
> > dereference from pcie_config_aspm_link():
> >
> >
> > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > > index a70afdf..6735f38 100644
> > > --- a/drivers/pci/pcie/aspm.c
> > > +++ b/drivers/pci/pcie/aspm.c
> > > @@ -597,11 +683,23 @@ static void pcie_config_aspm_dev(struct pci_dev
> *pdev, u32 val)
> > >  static void pcie_config_aspm_link(struct pcie_link_state *link, u32
> state)
> > >  {
> > >     u32 upstream = 0, dwstream = 0;
> > > -   struct pci_dev *child, *parent = link->pdev;
> > > +   struct pci_dev *child = link->downstream, *parent = link->pdev;
> >
> >
> > Here link->downstream is NULL,
>
> Sorry about the breakage.  Can you try also cherry-picking this
> commit:
>
> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.
> git/commit/?h=for-linus&id=3bd7db63a841e8c5297bb18ad801df67d5e38ad2
>
> Yinghai tripped over a similar problem in a different way, but I
> suspect his fix might also fix the problem you're seeing.
>

Yes, I think that should fix it.

Sorry for the breakage, can you please let me know if it doesn't fix it for
you.

Rajat


>
> If that doesn't fix it, we'll have to look farther.
>
> I'm planning to ask Linus to pull this fix for v4.11-rc2.
>
> Bjorn
>

[-- Attachment #2: Type: text/html, Size: 2782 bytes --]

Re: [PATCH 5/6] PCI/ASPM: Actually configure the L1 substate settings to the device

[James Morse]

Hi guys,

On 08/03/17 23:08, Rajat Jain wrote:
> On Wed, Mar 8, 2017 at 2:39 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> On Wed, Mar 08, 2017 at 06:44:36PM +0000, James Morse wrote:
>>> On 03/01/17 06:34, Rajat Jain wrote:
>>>> Add code to actually configure the L1 substate settigns on the
>>>> upstream and downstream device, while taking care of the rules
>>>> dictated by the PCIe spec.
>>>
>>> While testing hibernate on an arm64 juno with v4.11-rc1, I get a NULL
>> pointer
>>> dereference from pcie_config_aspm_link():
>>>
>>>
>>>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>>>> index a70afdf..6735f38 100644
>>>> --- a/drivers/pci/pcie/aspm.c
>>>> +++ b/drivers/pci/pcie/aspm.c
>>>> @@ -597,11 +683,23 @@ static void pcie_config_aspm_dev(struct pci_dev
>> *pdev, u32 val)
>>>>  static void pcie_config_aspm_link(struct pcie_link_state *link, u32
>> state)
>>>>  {
>>>>     u32 upstream = 0, dwstream = 0;
>>>> -   struct pci_dev *child, *parent = link->pdev;
>>>> +   struct pci_dev *child = link->downstream, *parent = link->pdev;
>>>
>>>
>>> Here link->downstream is NULL,
>>
>> Sorry about the breakage.  Can you try also cherry-picking this
>> commit:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.
>> git/commit/?h=for-linus&id=3bd7db63a841e8c5297bb18ad801df67d5e38ad2
>>
>> Yinghai tripped over a similar problem in a different way, but I
>> suspect his fix might also fix the problem you're seeing.
>>
> 
> Yes, I think that should fix it.

Yes, this fixes the problem. Thanks!

I should have dug through the list a little more to see if there was an existing
fix...


Thanks,

James

[PATCH 6/6] PCI/ASPM: Add comment about L1 substate latency

[Rajat Jain]

Since the exit latencies for L1 substates are not advertised by
a device, it is not clear in spec how to do a L1 substate exit
latency check. We assume that the L1 exit latencies advertised
by a device include L1 substate latencies (and hence do not do any
check). If that is not true, we should do some sort of check here.

(I'm not clear about what that check should like currenlty. I'd be
glad to take up any suggestions).

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/pci/pcie/aspm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 6735f38..cb5602c 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -403,6 +403,14 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
 		 * Check L1 latency.
 		 * Every switch on the path to root complex need 1
 		 * more microsecond for L1. Spec doesn't mention L0s.
+		 *
+		 * The exit latencies for L1 substates are not advertised
+		 * by a device. Since the spec also doesn't mention a way
+		 * to determine max latencies introduced by enabling L1
+		 * substates on the components,  it is not clear how to do
+		 * a L1 substate exit latency check. We assume that the
+		 * L1 exit latencies advertised by a device include L1
+		 * substate latencies (and hence do not do any check)
 		 */
 		latency = max_t(u32, link->latency_up.l1, link->latency_dw.l1);
 		if ((link->aspm_capable & ASPM_STATE_L1) &&
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 0/6] PCI/ASPM: Add PCIe L1 PM substate support

[Bjorn Helgaas]

On Mon, Jan 02, 2017 at 10:34:09PM -0800, Rajat Jain wrote:
> This patchset adds the PCIe L1 PM substate support to the kernel.
> The feature is described at:
> https://pcisig.com/sites/default/files/specification_documents/ECN_L1_PM_Substates_with_CLKREQ_31_May_2013_Rev10a.pdf
> 
> Its all logically one patch (and may be some of them should be
> squashed later) , but I've broken down into smaller patches for
> ease of review. 
> 
> This is currently rebased on top of Bjorn's master branch.
> 
> Rajat Jain (6):
>   PCI: Add L1 substate capability structure register definitions
>   PCI/ASPM: Introduce L1 substates and a Kconfig for it
>   PCI/ASPM: Read and setup L1 substate capabilities
>   PCI/ASPM: Calculate and save the L1.2 timing parameters
>   PCI/ASPM: Actually configure the L1 substate settings to the device
>   PCI/ASPM: Add comment about L1 substate latency
> 
>  drivers/pci/pcie/Kconfig      |   8 ++
>  drivers/pci/pcie/aspm.c       | 291 ++++++++++++++++++++++++++++++++++++++++--
>  include/uapi/linux/pci_regs.h |  16 +++
>  3 files changed, 302 insertions(+), 13 deletions(-)

Applied to pci/aspm for v4.11, thanks, Rajat!