[PATCH V4 0/3] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[PATCH V4 0/3] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

y
(POLICY_DEFAULT), current code is querying the enable/disable
states from ASPM registers to determine the policy.

For example, a BIOS could set the power saving state to performance
and clear all ASPM control registers. A balanced ASPM policy could
enable L0s and disable L1. A power conscious BIOS could enable both
L0s and L1 to trade off latency and performance vs. power.

After hotplug removal, pcie_aspm_exit_link_state() function clears
the ASPM registers. An insertion following hotplug removal reads
incorrect policy as ASPM disabled even though ASPM was enabled
during boot.

This is caused by the fact that same function is used for reconfiguring
ASPM regardless of the of the power on state.

------------------------
Changes from v3 (https://lkml.org/lkml/2017/3/8/670)
------------------------
- call pcie_aspm_init_link_state() for every device, maybe from
pci_init_capabilities()

- for bridges, have pcie_aspm_init_link_state() allocate a
link_state, regardless of whether it currently has any children,
and save the ASPM settings done by firmware

- for endpoints, have pcie_aspm_init_link_state() do the actual ASPM
setup of the link as it currently does

- for endpoints, change pcie_aspm_exit_link_state() so it cleans up
the device's own state and disables ASPM if necessary, but doesn't
remove the parent's link_state

- for bridges, change pcie_aspm_exit_link_state() so it frees the
bridge's own link_state

Sinan Kaya (3):
  PCI/ASPM: divide ASPM capability init into pre and post init
  PCI/ASPM: move part of ASPM initialization to pci_init_capabilities
  PCI/ASPM: move link_state cleanup to bridge remove

 drivers/pci/pcie/aspm.c | 175 +++++++++++++++++++++++++++++++++++-------------
 drivers/pci/probe.c     |   3 +
 include/linux/pci.h     |   6 ++
 3 files changed, 136 insertions(+), 48 deletions(-)

-- 
1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V4 1/3] PCI/ASPM: divide ASPM capability init into pre and post init

[Sinan Kaya]

The ASPM capability initialization is done in one pass where both
the current settings such as capable/enable/supported field are set
and also all children are scanned for latencies.

Divide the init into two so that part of the code that needs
scan finished is obvious.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pcie/aspm.c | 76 +++++++++++++++++++++++++++----------------------
 1 file changed, 42 insertions(+), 34 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 3dd8bcb..453558d 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -338,11 +338,10 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
 	}
 }
 
-static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
+static void pcie_aspm_cap_post_scan(struct pcie_link_state *link, int blacklist)
 {
 	struct pci_dev *child, *parent = link->pdev;
 	struct pci_bus *linkbus = parent->subordinate;
-	struct aspm_register_info upreg, dwreg;
 
 	if (blacklist) {
 		/* Set enabled/disable so that we will disable ASPM later */
@@ -351,6 +350,45 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 		return;
 	}
 
+	/*
+	 * If the downstream component has pci bridge function, don't
+	 * do ASPM for now.
+	 */
+	list_for_each_entry(child, &linkbus->devices, bus_list) {
+		if (pci_pcie_type(child) == PCI_EXP_TYPE_PCI_BRIDGE) {
+			link->aspm_disable = ASPM_STATE_ALL;
+			break;
+		}
+	}
+
+	/* Get and check endpoint acceptable latencies */
+	list_for_each_entry(child, &linkbus->devices, bus_list) {
+		u32 reg32, encoding;
+		struct aspm_latency *acceptable =
+			&link->acceptable[PCI_FUNC(child->devfn)];
+
+		if (pci_pcie_type(child) != PCI_EXP_TYPE_ENDPOINT &&
+		    pci_pcie_type(child) != PCI_EXP_TYPE_LEG_END)
+			continue;
+
+		pcie_capability_read_dword(child, PCI_EXP_DEVCAP, &reg32);
+		/* Calculate endpoint L0s acceptable latency */
+		encoding = (reg32 & PCI_EXP_DEVCAP_L0S) >> 6;
+		acceptable->l0s = calc_l0s_acceptable(encoding);
+		/* Calculate endpoint L1 acceptable latency */
+		encoding = (reg32 & PCI_EXP_DEVCAP_L1) >> 9;
+		acceptable->l1 = calc_l1_acceptable(encoding);
+
+		pcie_aspm_check_latency(child);
+	}
+}
+
+static void pcie_aspm_cap_init(struct pcie_link_state *link)
+{
+	struct pci_dev *child, *parent = link->pdev;
+	struct pci_bus *linkbus = parent->subordinate;
+	struct aspm_register_info upreg, dwreg;
+
 	/* Get upstream/downstream components' register state */
 	pcie_get_aspm_reg(parent, &upreg);
 	child = list_entry(linkbus->devices.next, struct pci_dev, bus_list);
@@ -402,37 +440,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;
-	/*
-	 * If the downstream component has pci bridge function, don't
-	 * do ASPM for now.
-	 */
-	list_for_each_entry(child, &linkbus->devices, bus_list) {
-		if (pci_pcie_type(child) == PCI_EXP_TYPE_PCI_BRIDGE) {
-			link->aspm_disable = ASPM_STATE_ALL;
-			break;
-		}
-	}
-
-	/* Get and check endpoint acceptable latencies */
-	list_for_each_entry(child, &linkbus->devices, bus_list) {
-		u32 reg32, encoding;
-		struct aspm_latency *acceptable =
-			&link->acceptable[PCI_FUNC(child->devfn)];
-
-		if (pci_pcie_type(child) != PCI_EXP_TYPE_ENDPOINT &&
-		    pci_pcie_type(child) != PCI_EXP_TYPE_LEG_END)
-			continue;
-
-		pcie_capability_read_dword(child, PCI_EXP_DEVCAP, &reg32);
-		/* Calculate endpoint L0s acceptable latency */
-		encoding = (reg32 & PCI_EXP_DEVCAP_L0S) >> 6;
-		acceptable->l0s = calc_l0s_acceptable(encoding);
-		/* Calculate endpoint L1 acceptable latency */
-		encoding = (reg32 & PCI_EXP_DEVCAP_L1) >> 9;
-		acceptable->l1 = calc_l1_acceptable(encoding);
-
-		pcie_aspm_check_latency(child);
-	}
 }
 
 static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
@@ -606,7 +613,8 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
 	 * upstream links also because capable state of them can be
 	 * update through pcie_aspm_cap_init().
 	 */
-	pcie_aspm_cap_init(link, blacklist);
+	pcie_aspm_cap_init(link);
+	pcie_aspm_cap_post_scan(link, blacklist);
 
 	/* Setup initial Clock PM state */
 	pcie_clkpm_cap_init(link, blacklist);
-- 
1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V4 2/3] PCI/ASPM: move part of ASPM initialization to pci_init_capabilities

[Sinan Kaya]

Call pci_aspm_init() for every device, maybe from pci_init_capabilities()

- for bridges, have pcie_aspm_init_link_state() allocate a
  link_state, regardless of whether it currently has any children,
  and save the ASPM settings done by firmware

- for endpoints, have pcie_aspm_init_link_state() do the actual ASPM
  setup of the link as it currently does

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pcie/aspm.c | 82 +++++++++++++++++++++++++++++++++++++++++++------
 drivers/pci/probe.c     |  3 ++
 include/linux/pci.h     |  6 ++++
 3 files changed, 81 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 453558d..ed67710 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -383,15 +383,14 @@ static void pcie_aspm_cap_post_scan(struct pcie_link_state *link, int blacklist)
 	}
 }
 
-static void pcie_aspm_cap_init(struct pcie_link_state *link)
+static void pcie_aspm_cap_init(struct pcie_link_state *link,
+			       struct pci_dev *child)
 {
-	struct pci_dev *child, *parent = link->pdev;
-	struct pci_bus *linkbus = parent->subordinate;
+	struct pci_dev *parent = link->pdev;
 	struct aspm_register_info upreg, dwreg;
 
 	/* Get upstream/downstream components' register state */
 	pcie_get_aspm_reg(parent, &upreg);
-	child = list_entry(linkbus->devices.next, struct pci_dev, bus_list);
 	pcie_get_aspm_reg(child, &dwreg);
 
 	/*
@@ -576,6 +575,71 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
  * It is called after the pcie and its children devices are scanned.
  * @pdev: the root port or switch downstream port
  */
+int pci_aspm_init(struct pci_dev *pdev)
+{
+	int ret;
+	struct pcie_link_state *link;
+
+	if (!aspm_support_enabled)
+		return 0;
+
+	if (pdev->link_state)
+		return 0;
+
+	/* VIA has a strange chipset, root port is under a bridge */
+	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT &&
+	    pdev->bus->self)
+		return 0;
+
+	down_read(&pci_bus_sem);
+	mutex_lock(&aspm_lock);
+
+	if (pdev->has_secondary_link) {
+		link = alloc_pcie_link_state(pdev);
+		WARN_ON(!link);
+		if (!link) {
+			ret = -ENOMEM;
+			goto unlock;
+		}
+	} else {
+		WARN_ON(!pdev->bus);
+		if (!pdev->bus) {
+			ret = -ENODEV;
+			goto unlock;
+		}
+
+		WARN_ON(!pdev->bus->self);
+		if (!pdev->bus->self) {
+			ret = -ENODEV;
+			goto unlock;
+		}
+
+		WARN_ON(!pdev->bus->self->link_state);
+		if (!pdev->bus->self->link_state) {
+			ret = -ENODEV;
+			goto unlock;
+		}
+
+		link = pdev->bus->self->link_state;
+
+		/* Setup initial ASPM state. */
+		pcie_aspm_cap_init(link, pdev);
+
+	}
+
+	ret = 0;
+unlock:
+	mutex_unlock(&aspm_lock);
+	up_read(&pci_bus_sem);
+
+	return ret;
+}
+
+/*
+ * pcie_aspm_init_link_state: Initiate PCI express link state.
+ * It is called after the pcie and its children devices are scanned.
+ * @pdev: the root port or switch downstream port
+ */
 void pcie_aspm_init_link_state(struct pci_dev *pdev)
 {
 	struct pcie_link_state *link;
@@ -584,9 +648,11 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
 	if (!aspm_support_enabled)
 		return;
 
-	if (pdev->link_state)
+	if (!pdev->link_state)
 		return;
 
+	link = pdev->link_state;
+
 	/*
 	 * We allocate pcie_link_state for the component on the upstream
 	 * end of a Link, so there's nothing to do unless this device has a
@@ -605,15 +671,12 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
 		goto out;
 
 	mutex_lock(&aspm_lock);
-	link = alloc_pcie_link_state(pdev);
-	if (!link)
-		goto unlock;
+
 	/*
 	 * Setup initial ASPM state. Note that we need to configure
 	 * upstream links also because capable state of them can be
 	 * update through pcie_aspm_cap_init().
 	 */
-	pcie_aspm_cap_init(link);
 	pcie_aspm_cap_post_scan(link, blacklist);
 
 	/* Setup initial Clock PM state */
@@ -632,7 +695,6 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
 		pcie_set_clkpm(link, policy_to_clkpm_state(link));
 	}
 
-unlock:
 	mutex_unlock(&aspm_lock);
 out:
 	up_read(&pci_bus_sem);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 204960e..d7f10fb 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1818,6 +1818,9 @@ static void pci_init_capabilities(struct pci_dev *dev)
 
 	/* Advanced Error Reporting */
 	pci_aer_init(dev);
+
+	/* Active State Power Management */
+	pci_aspm_init(dev);
 }
 
 /*
diff --git a/include/linux/pci.h b/include/linux/pci.h
index e2d1a12..97e13f0 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1418,6 +1418,12 @@ static inline void pci_no_aer(void) { }
 static inline int pci_aer_init(struct pci_dev *d) { return -ENODEV; }
 #endif
 
+#ifdef CONFIG_PCIEASPM
+int pci_aspm_init(struct pci_dev *pdev);
+#else
+static inline int pci_aspm_init(struct pci_dev *pdev) { return -ENODEV; };
+#endif
+
 #ifdef CONFIG_PCIE_ECRC
 void pcie_set_ecrc_checking(struct pci_dev *dev);
 void pcie_ecrc_get_policy(char *str);
-- 
1.9.1

[PATCH V4 3/3] PCI/ASPM: move link_state cleanup to bridge remove

[Sinan Kaya]

For endpoints, change pcie_aspm_exit_link_state() so it cleans up
the device's own state and disables ASPM if necessary, but doesn't
remove the parent's link_state.

For bridges, change pcie_aspm_exit_link_state() so it frees the
bridge's own link_state.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pcie/aspm.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index ed67710..6c2d7ea 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -419,8 +419,10 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link,
 	 */
 	if (dwreg.support & upreg.support & PCIE_LINK_STATE_L0S)
 		link->aspm_support |= ASPM_STATE_L0S;
-	if (dwreg.enabled & PCIE_LINK_STATE_L0S)
+	if (dwreg.enabled & PCIE_LINK_STATE_L0S) {
 		link->aspm_enabled |= ASPM_STATE_L0S_UP;
+		link->aspm_default |= ASPM_STATE_L0S_UP;
+	}
 	if (upreg.enabled & PCIE_LINK_STATE_L0S)
 		link->aspm_enabled |= ASPM_STATE_L0S_DW;
 	link->latency_up.l0s = calc_l0s_latency(upreg.latency_encoding_l0s);
@@ -434,9 +436,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link,
 	link->latency_up.l1 = calc_l1_latency(upreg.latency_encoding_l1);
 	link->latency_dw.l1 = calc_l1_latency(dwreg.latency_encoding_l1);
 
-	/* Save default state */
-	link->aspm_default = link->aspm_enabled;
-
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;
 }
@@ -579,6 +578,7 @@ int pci_aspm_init(struct pci_dev *pdev)
 {
 	int ret;
 	struct pcie_link_state *link;
+	struct aspm_register_info upreg;
 
 	if (!aspm_support_enabled)
 		return 0;
@@ -601,6 +601,13 @@ int pci_aspm_init(struct pci_dev *pdev)
 			ret = -ENOMEM;
 			goto unlock;
 		}
+
+		pcie_get_aspm_reg(pdev, &upreg);
+		if (upreg.enabled & PCIE_LINK_STATE_L0S)
+			link->aspm_default |= ASPM_STATE_L0S_DW;
+		if (upreg.enabled & PCIE_LINK_STATE_L1)
+			link->aspm_default |= ASPM_STATE_L1;
+
 	} else {
 		WARN_ON(!pdev->bus);
 		if (!pdev->bus) {
@@ -748,10 +755,12 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
 
 	/* All functions are removed, so just disable ASPM for the link */
 	pcie_config_aspm_link(link, 0);
-	list_del(&link->sibling);
-	list_del(&link->link);
-	/* Clock PM is for endpoint device */
-	free_link_state(link);
+	if (pdev->has_secondary_link) {
+		list_del(&link->sibling);
+		list_del(&link->link);
+		/* Clock PM is for endpoint device */
+		free_link_state(link);
+	}
 
 	/* Recheck latencies and configure upstream links */
 	if (parent_link) {
-- 
1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V4 0/3] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Bjorn Helgaas]

On Mon, Mar 13, 2017 at 04:48:02PM -0400, Sinan Kaya wrote:
> y
> (POLICY_DEFAULT), current code is querying the enable/disable
> states from ASPM registers to determine the policy.
> 
> For example, a BIOS could set the power saving state to performance
> and clear all ASPM control registers. A balanced ASPM policy could
> enable L0s and disable L1. A power conscious BIOS could enable both
> L0s and L1 to trade off latency and performance vs. power.
> 
> After hotplug removal, pcie_aspm_exit_link_state() function clears
> the ASPM registers. An insertion following hotplug removal reads
> incorrect policy as ASPM disabled even though ASPM was enabled
> during boot.
> 
> This is caused by the fact that same function is used for reconfiguring
> ASPM regardless of the of the power on state.
> 
> ------------------------
> Changes from v3 (https://lkml.org/lkml/2017/3/8/670)
> ------------------------
> - call pcie_aspm_init_link_state() for every device, maybe from
> pci_init_capabilities()
> 
> - for bridges, have pcie_aspm_init_link_state() allocate a
> link_state, regardless of whether it currently has any children,
> and save the ASPM settings done by firmware
> 
> - for endpoints, have pcie_aspm_init_link_state() do the actual ASPM
> setup of the link as it currently does
> 
> - for endpoints, change pcie_aspm_exit_link_state() so it cleans up
> the device's own state and disables ASPM if necessary, but doesn't
> remove the parent's link_state
> 
> - for bridges, change pcie_aspm_exit_link_state() so it frees the
> bridge's own link_state
> 
> Sinan Kaya (3):
>   PCI/ASPM: divide ASPM capability init into pre and post init
>   PCI/ASPM: move part of ASPM initialization to pci_init_capabilities
>   PCI/ASPM: move link_state cleanup to bridge remove
> 
>  drivers/pci/pcie/aspm.c | 175 +++++++++++++++++++++++++++++++++++-------------
>  drivers/pci/probe.c     |   3 +
>  include/linux/pci.h     |   6 ++
>  3 files changed, 136 insertions(+), 48 deletions(-)

What is this series based on?  Unless they depend on other in-flight
patches, I apply patches to branches based on my "master" branch,
which is typically -rc1 or -rc2 (it's currently v4.11-rc1).  These
don't apply to either.

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V4 0/3] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

On 3/13/2017 5:46 PM, Bjorn Helgaas wrote:
> What is this series based on?  Unless they depend on other in-flight
> patches, I apply patches to branches based on my "master" branch,
> which is typically -rc1 or -rc2 (it's currently v4.11-rc1).  These
> don't apply to either.

It looks like I am on 4.10. I can rebase and post again. 

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH V4 0/3] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Bjorn Helgaas]

On Mon, Mar 13, 2017 at 06:05:55PM -0400, Sinan Kaya wrote:
> On 3/13/2017 5:46 PM, Bjorn Helgaas wrote:
> > What is this series based on?  Unless they depend on other in-flight
> > patches, I apply patches to branches based on my "master" branch,
> > which is typically -rc1 or -rc2 (it's currently v4.11-rc1).  These
> > don't apply to either.
> 
> It looks like I am on 4.10. I can rebase and post again. 

Rebasing would be good, but I can give you some comments on your v4,
now that I can apply it and see what it looks like.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V4 0/3] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

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

On 3/13/2017 7:08 PM, Bjorn Helgaas wrote:
> On Mon, Mar 13, 2017 at 06:05:55PM -0400, Sinan Kaya wrote:
>> On 3/13/2017 5:46 PM, Bjorn Helgaas wrote:
>>> What is this series based on?  Unless they depend on other in-flight
>>> patches, I apply patches to branches based on my "master" branch,
>>> which is typically -rc1 or -rc2 (it's currently v4.11-rc1).  These
>>> don't apply to either.
>>
>> It looks like I am on 4.10. I can rebase and post again. 
> 
> Rebasing would be good, but I can give you some comments on your v4,
> now that I can apply it and see what it looks like.
> 

Thanks, 

I'm mostly done with the rebase. I'll have to retest tomorrow morning and
then post.

I had to drop this

   - for endpoints, have pcie_aspm_init_link_state() do the actual ASPM
      setup of the link as it currently does.

during rebase.

The reason is that the clock configuration needs to switch to common clock
mode across all the devices before any ASPM latency is read/configured.

Common clock configuration seem to be trying to walk the device list.

Here is the untested version. It might not even compile.
I'll wait until I receive your comments.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[-- Attachment #2: 0001-PCI-ASPM-move-part-of-ASPM-initialization-to-pci_ini.patch --]
[-- Type: text/plain, Size: 2323 bytes --]

>From 0ac5e06c99106de612e596398a3add44c54ddf42 Mon Sep 17 00:00:00 2001
From: Sinan Kaya <okaya@codeaurora.org>
Date: Mon, 13 Mar 2017 19:08:57 -0400
Subject: [PATCH 1/4] PCI/ASPM: move part of ASPM initialization to
 pci_init_capabilities

Call pci_aspm_init() for every device, maybe from pci_init_capabilities()

- for bridges, have pcie_aspm_init_link_state() allocate a
  link_state, regardless of whether it currently has any children,
  and save the ASPM settings done by firmware.

- for endpoints, have pcie_aspm_init_link_state() do the actual ASPM
  setup of the link as it currently does.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pcie/aspm.c | 10 ++++++++++
 drivers/pci/probe.c     |  3 +++
 include/linux/pci.h     |  2 ++
 3 files changed, 15 insertions(+)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 973472c..74fd7c5 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -828,6 +828,16 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
 }
 
 /*
+ * pci_aspm_init: Initiate PCI express link state.
+ * It is called from device_add for every single pci device.
+ * @pdev: all pci devices
+ */
+int pci_aspm_init(struct pci_dev *pdev)
+{
+	return 0;
+}
+
+/*
  * pcie_aspm_init_link_state: Initiate PCI express link state.
  * It is called after the pcie and its children devices are scanned.
  * @pdev: the root port or switch downstream port
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index dfc9a27..1e19364 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1847,6 +1847,9 @@ static void pci_init_capabilities(struct pci_dev *dev)
 
 	/* Advanced Error Reporting */
 	pci_aer_init(dev);
+
+	/* Active State Power Management */
+	pci_aspm_init(dev);
 }
 
 /*
diff --git a/include/linux/pci.h b/include/linux/pci.h
index eb3da1a..8828dd7 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1396,8 +1396,10 @@ static inline int pci_irq_get_node(struct pci_dev *pdev, int vec)
 
 #ifdef CONFIG_PCIEASPM
 bool pcie_aspm_support_enabled(void);
+int pci_aspm_init(struct pci_dev *pdev);
 #else
 static inline bool pcie_aspm_support_enabled(void) { return false; }
+static inline int pci_aspm_init(struct pci_dev *pdev) { return -ENODEV; }
 #endif
 
 #ifdef CONFIG_PCIEAER
-- 
1.9.1


[-- Attachment #3: 0002-PCI-ASPM-add-init-hook-to-device_add.patch --]
[-- Type: text/plain, Size: 2445 bytes --]

>From 36d315a7198bbe90cc5b9a06ddd5cb9ac4ebdd1a Mon Sep 17 00:00:00 2001
From: Sinan Kaya <okaya@codeaurora.org>
Date: Mon, 13 Mar 2017 19:00:46 -0400
Subject: [PATCH 2/4] PCI/ASPM: add init hook to device_add

The ASPM capability initialization is done in one pass where both
the current settings such as capable/enable/supported field are set
and also all children are scanned for latencies.

Add a callback from device_add to save the power up values.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pcie/aspm.c | 43 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 74fd7c5..7312a37 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -834,7 +834,39 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
  */
 int pci_aspm_init(struct pci_dev *pdev)
 {
-	return 0;
+	int ret;
+	struct pcie_link_state *link;
+	struct aspm_register_info upreg;
+
+	if (!aspm_support_enabled)
+		return 0;
+
+	if (pdev->link_state)
+		return 0;
+
+	/* VIA has a strange chipset, root port is under a bridge */
+	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT &&
+	    pdev->bus->self)
+		return 0;
+
+	if (!pdev->has_secondary_link)
+		return 0;
+
+	down_read(&pci_bus_sem);
+	mutex_lock(&aspm_lock);
+
+	link = alloc_pcie_link_state(pdev);
+	WARN_ON(!link);
+	if (!link) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+	ret = 0;
+unlock:
+	mutex_unlock(&aspm_lock);
+	up_read(&pci_bus_sem);
+
+	return ret;
 }
 
 /*
@@ -850,9 +882,11 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
 	if (!aspm_support_enabled)
 		return;
 
-	if (pdev->link_state)
+	if (!pdev->link_state)
 		return;
 
+	link = pdev->link_state;
+
 	/*
 	 * We allocate pcie_link_state for the component on the upstream
 	 * end of a Link, so there's nothing to do unless this device has a
@@ -871,9 +905,7 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
 		goto out;
 
 	mutex_lock(&aspm_lock);
-	link = alloc_pcie_link_state(pdev);
-	if (!link)
-		goto unlock;
+
 	/*
 	 * Setup initial ASPM state. Note that we need to configure
 	 * upstream links also because capable state of them can be
@@ -898,7 +930,6 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
 		pcie_set_clkpm(link, policy_to_clkpm_state(link));
 	}
 
-unlock:
 	mutex_unlock(&aspm_lock);
 out:
 	up_read(&pci_bus_sem);
-- 
1.9.1


[-- Attachment #4: 0003-PCI-ASPM-save-power-on-values-during-bridge-init.patch --]
[-- Type: text/plain, Size: 2379 bytes --]

>From 1e6d3b2856a6ba3f38f67ab9c8286e4bf93f0968 Mon Sep 17 00:00:00 2001
From: Sinan Kaya <okaya@codeaurora.org>
Date: Sun, 12 Mar 2017 01:08:54 -0500
Subject: [PATCH 3/4] PCI/ASPM: save power on values during bridge init

Now that we added a hook to be called from device_add, save the
default values from the HW registers early in the boot for further
reuse during hot device add/remove operations.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pcie/aspm.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 7312a37..7754445 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -521,8 +521,10 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	 */
 	if (dwreg.support & upreg.support & PCIE_LINK_STATE_L0S)
 		link->aspm_support |= ASPM_STATE_L0S;
-	if (dwreg.enabled & PCIE_LINK_STATE_L0S)
+	if (dwreg.enabled & PCIE_LINK_STATE_L0S) {
 		link->aspm_enabled |= ASPM_STATE_L0S_UP;
+		link->aspm_default |= ASPM_STATE_L0S_UP;
+	}
 	if (upreg.enabled & PCIE_LINK_STATE_L0S)
 		link->aspm_enabled |= ASPM_STATE_L0S_DW;
 	link->latency_up.l0s = calc_l0s_latency(upreg.latency_encoding_l0s);
@@ -558,9 +560,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	if (link->aspm_support & ASPM_STATE_L1SS)
 		aspm_calc_l1ss_info(link, &upreg, &dwreg);
 
-	/* Save default state */
-	link->aspm_default = link->aspm_enabled;
-
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;
 	/*
@@ -860,7 +859,21 @@ int pci_aspm_init(struct pci_dev *pdev)
 	if (!link) {
 		ret = -ENOMEM;
 		goto unlock;
-	}
+
+	pcie_get_aspm_reg(pdev, &upreg);
+	if (upreg.enabled & PCIE_LINK_STATE_L0S)
+		link->aspm_default |= ASPM_STATE_L0S_DW;
+	if (upreg.enabled & PCIE_LINK_STATE_L1)
+		link->aspm_default |= ASPM_STATE_L1;
+	if (upreg.l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_1)
+		link->aspm_default |= ASPM_STATE_L1_1;
+	if (upreg.l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_2)
+		link->aspm_default |= ASPM_STATE_L1_2;
+	if (upreg.l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_1)
+		link->aspm_default |= ASPM_STATE_L1_1_PCIPM;
+	if (upreg.l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_2)
+		link->aspm_default |= ASPM_STATE_L1_2_PCIPM;
+
 	ret = 0;
 unlock:
 	mutex_unlock(&aspm_lock);
-- 
1.9.1


[-- Attachment #5: 0004-PCI-ASPM-move-link_state-cleanup-to-bridge-remove.patch --]
[-- Type: text/plain, Size: 1323 bytes --]

>From e54d1b44f6701c5423ff8aa3501164352f57e37f Mon Sep 17 00:00:00 2001
From: Sinan Kaya <okaya@codeaurora.org>
Date: Mon, 13 Mar 2017 18:43:35 -0400
Subject: [PATCH 4/4] PCI/ASPM: move link_state cleanup to bridge remove

For endpoints, change pcie_aspm_exit_link_state() so it cleans up
the device's own state and disables ASPM if necessary, but doesn't
remove the parent's link_state.

For bridges, change pcie_aspm_exit_link_state() so it frees the
bridge's own link_state.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pcie/aspm.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 7754445..e1a1b47 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -996,10 +996,12 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
 
 	/* All functions are removed, so just disable ASPM for the link */
 	pcie_config_aspm_link(link, 0);
-	list_del(&link->sibling);
-	list_del(&link->link);
-	/* Clock PM is for endpoint device */
-	free_link_state(link);
+	if (pdev->has_secondary_link) {
+		list_del(&link->sibling);
+		list_del(&link->link);
+		/* Clock PM is for endpoint device */
+		free_link_state(link);
+	}
 
 	/* Recheck latencies and configure upstream links */
 	if (parent_link) {
-- 
1.9.1

Re: [PATCH V4 0/3] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Bjorn Helgaas]

On Mon, Mar 13, 2017 at 07:21:42PM -0400, Sinan Kaya wrote:
> On 3/13/2017 7:08 PM, Bjorn Helgaas wrote:
> > On Mon, Mar 13, 2017 at 06:05:55PM -0400, Sinan Kaya wrote:
> >> On 3/13/2017 5:46 PM, Bjorn Helgaas wrote:
> >>> What is this series based on?  Unless they depend on other in-flight
> >>> patches, I apply patches to branches based on my "master" branch,
> >>> which is typically -rc1 or -rc2 (it's currently v4.11-rc1).  These
> >>> don't apply to either.
> >>
> >> It looks like I am on 4.10. I can rebase and post again. 
> > 
> > Rebasing would be good, but I can give you some comments on your v4,
> > now that I can apply it and see what it looks like.
> > 
> 
> Thanks, 
> 
> I'm mostly done with the rebase. I'll have to retest tomorrow morning and
> then post.
> 
> I had to drop this
> 
>    - for endpoints, have pcie_aspm_init_link_state() do the actual ASPM
>       setup of the link as it currently does.
> 
> during rebase.
> 
> The reason is that the clock configuration needs to switch to common clock
> mode across all the devices before any ASPM latency is read/configured.
> 
> Common clock configuration seem to be trying to walk the device list.
> 
> Here is the untested version. It might not even compile.
> I'll wait until I receive your comments.

I had already started looking at v4, so these comments are based on
that.  Your v4 is a great start and these are all general comments so
I didn't try to match each one up with the code.

  - Goal: retain BIOS ASPM config of bridge even if children are
    removed and re-added.  Is there a bugzilla for this problem?

  - pci_aspm_init(): Fix function comment.  Called for every device we
    enumerate.  Maybe split body into pci_aspm_init_upstream() (the
    has_secondary_link part) and pci_aspm_init_downstream() (or maybe
    these end up being similar enough that splitting doesn't make
    sense).  I don't think we should need locking in either case
    because this is a new device we're in the process of enumerating.

    - Upstream link partner:  Shouldn't need to check pdev->link_state
      (should always be NULL for a brand-new device).  Should allocate
      link_state and capture everything that doesn't depend on the
      downstream link partner, i.e., all DEVCAP & LNKCAP info (this
      includes the initial BIOS config you're after).

    - Downstream link partner: (devices where dev->has_secondary_link
      is not set).  Capture device-specific state, i.e., DEVCAP &
      LNKCAP info, exit latencies and latency requirements.  The
      sanity check/blacklist stuff only depends on the downstream link
      partner and maybe could be computed and saved here, e.g., in a
      new pdev->aspm_blacklist or pdev->link->blacklist.  

      Probably should not *write* to any hardware configuration, e.g.,
      common clock configuration, because some of that depends on all
      siblings, and we haven't enumerated them all yet.  The hardware
      configuration should probably be done in
      pcie_aspm_init_link_state().

  - pcie_aspm_init_link_state(): Called for bridges (upstream end of
    link) after all children have been enumerated.  No longer needs to
    check aspm_support_enabled or pdev->has_secondary_link or the VIA
    quirk: pci_aspm_init() already checked that stuff, so we only need
    to check pdev->link_state here.  Probably should do the common
    clock config here instead of in pci_aspm_init().

  - pcie_aspm_configure_common_clock(): I don't think you touched
    this, but this might be a good opportunity to move the re-read
    (pcie_get_aspm_reg() stuff) into this function so it doesn't
    clutter the caller?  Could be a preliminary cleanup patch.

    PCI_EXP_LNKSTA_SLC is HwInit, which means it's read-only and could
    be captured at pci_aspm_init()-time.  Currently we only look at
    SLC for the first child, but pci_aspm_init_upstream() could set a
    link->downstream_slc bit, and pci_aspm_init_downstream() could
    clear it if it found a device with SLC not set.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V4 0/3] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

On 3/14/2017 3:21 PM, Bjorn Helgaas wrote:
> On Mon, Mar 13, 2017 at 07:21:42PM -0400, Sinan Kaya wrote:
>> On 3/13/2017 7:08 PM, Bjorn Helgaas wrote:
>>> On Mon, Mar 13, 2017 at 06:05:55PM -0400, Sinan Kaya wrote:
>>>> On 3/13/2017 5:46 PM, Bjorn Helgaas wrote:
>>>>> What is this series based on?  Unless they depend on other in-flight
>>>>> patches, I apply patches to branches based on my "master" branch,
>>>>> which is typically -rc1 or -rc2 (it's currently v4.11-rc1).  These
>>>>> don't apply to either.
>>>>
>>>> It looks like I am on 4.10. I can rebase and post again. 
>>>
>>> Rebasing would be good, but I can give you some comments on your v4,
>>> now that I can apply it and see what it looks like.
>>>
>>
>> Thanks, 
>>
>> I'm mostly done with the rebase. I'll have to retest tomorrow morning and
>> then post.
>>
>> I had to drop this
>>
>>    - for endpoints, have pcie_aspm_init_link_state() do the actual ASPM
>>       setup of the link as it currently does.
>>
>> during rebase.
>>
>> The reason is that the clock configuration needs to switch to common clock
>> mode across all the devices before any ASPM latency is read/configured.
>>
>> Common clock configuration seem to be trying to walk the device list.
>>
>> Here is the untested version. It might not even compile.
>> I'll wait until I receive your comments.
> 
> I had already started looking at v4, so these comments are based on
> that.  Your v4 is a great start and these are all general comments so
> I didn't try to match each one up with the code.
> 
>   - Goal: retain BIOS ASPM config of bridge even if children are
>     removed and re-added.  Is there a bugzilla for this problem?

No, I do not have a bugzilla.

> 
>   - pci_aspm_init(): Fix function comment.  Called for every device we
>     enumerate.  

Sure.

>     Maybe split body into pci_aspm_init_upstream() (the
>     has_secondary_link part) and pci_aspm_init_downstream() (or maybe
>     these end up being similar enough that splitting doesn't make
>     sense).  I don't think we should need locking in either case
>     because this is a new device we're in the process of enumerating.

I'll get rid of the locking. 
> 
>     - Upstream link partner:  Shouldn't need to check pdev->link_state
>       (should always be NULL for a brand-new device).  Should allocate
>       link_state and capture everything that doesn't depend on the
>       downstream link partner, i.e., all DEVCAP & LNKCAP info (this
>       includes the initial BIOS config you're after).

Yup, this is done on my last patch. I moved it to a separate patch to make
it obvious. Once I finish incorporating your comments and tests, I'll post
the updated version.

> 
>     - Downstream link partner: (devices where dev->has_secondary_link
>       is not set).  Capture device-specific state, i.e., DEVCAP &
>       LNKCAP info, exit latencies and latency requirements.  The
>       sanity check/blacklist stuff only depends on the downstream link
>       partner and maybe could be computed and saved here, e.g., in a
>       new pdev->aspm_blacklist or pdev->link->blacklist.  

I removed reading upstream device specific pieces out of this function on
the last patch. The reason is that common clock may not have been set up
by the time we reach to this function. Any register reads from capabilities
will be incorrect until we switch to common clock and switching to common
clock requires walking the children objects.

> 
>       Probably should not *write* to any hardware configuration, e.g.,
>       common clock configuration, because some of that depends on all
>       siblings, and we haven't enumerated them all yet.  The hardware
>       configuration should probably be done in
>       pcie_aspm_init_link_state().

Even reading the capabilities are incorrect. Devices maintain two sets of
capabilities. One for common clock and one without common clock. We would
be reading the incorrect one based on whether common clock is setup or not
during boot.

> 
>   - pcie_aspm_init_link_state(): Called for bridges (upstream end of
>     link) after all children have been enumerated.  No longer needs to
>     check aspm_support_enabled or pdev->has_secondary_link or the VIA
>     quirk: pci_aspm_init() already checked that stuff, so we only need
>     to check pdev->link_state here.  Probably should do the common
>     clock config here instead of in pci_aspm_init().

I agree. This is the correct place. I'll remove the extra checks. The only
thing that I do not understand well is the VIA configuration and I'm afraid
I'm going to break it. 

> 
>   - pcie_aspm_configure_common_clock(): I don't think you touched
>     this, but this might be a good opportunity to move the re-read
>     (pcie_get_aspm_reg() stuff) into this function so it doesn't
>     clutter the caller?  Could be a preliminary cleanup patch.
> 
>     PCI_EXP_LNKSTA_SLC is HwInit, which means it's read-only and could
>     be captured at pci_aspm_init()-time.  Currently we only look at
>     SLC for the first child, but pci_aspm_init_upstream() could set a
>     link->downstream_slc bit, and pci_aspm_init_downstream() could
>     clear it if it found a device with SLC not set.
> 

Let me look at these more.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH V4 0/3] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

Hi Bjorn,

On 3/15/2017 10:33 AM, Sinan Kaya wrote:
> On 3/14/2017 3:21 PM, Bjorn Helgaas wrote:
>> On Mon, Mar 13, 2017 at 07:21:42PM -0400, Sinan Kaya wrote:
>>> On 3/13/2017 7:08 PM, Bjorn Helgaas wrote:
>>>> On Mon, Mar 13, 2017 at 06:05:55PM -0400, Sinan Kaya wrote:
>>>>> On 3/13/2017 5:46 PM, Bjorn Helgaas wrote:
>>>>>> What is this series based on?  Unless they depend on other in-flight
>>>>>> patches, I apply patches to branches based on my "master" branch,
>>>>>> which is typically -rc1 or -rc2 (it's currently v4.11-rc1).  These
>>>>>> don't apply to either.
>>>>>
>>>>> It looks like I am on 4.10. I can rebase and post again. 
>>>>
>>>> Rebasing would be good, but I can give you some comments on your v4,
>>>> now that I can apply it and see what it looks like.
>>>>
>>>
>>> Thanks, 
>>>
>>> I'm mostly done with the rebase. I'll have to retest tomorrow morning and
>>> then post.
>>>
>>> I had to drop this
>>>
>>>    - for endpoints, have pcie_aspm_init_link_state() do the actual ASPM
>>>       setup of the link as it currently does.
>>>
>>> during rebase.
>>>
>>> The reason is that the clock configuration needs to switch to common clock
>>> mode across all the devices before any ASPM latency is read/configured.
>>>
>>> Common clock configuration seem to be trying to walk the device list.
>>>
>>> Here is the untested version. It might not even compile.
>>> I'll wait until I receive your comments.
>>
>> I had already started looking at v4, so these comments are based on
>> that.  Your v4 is a great start and these are all general comments so
>> I didn't try to match each one up with the code.
>>
>>   - Goal: retain BIOS ASPM config of bridge even if children are
>>     removed and re-added.  Is there a bugzilla for this problem?
> 

Is there a common place to add code after the bus scan similar to pci_init_capabilities(). 
I really don't like the ASPM call in the middle of scan function.

Sinan

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.