All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V6 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT
@ 2017-03-29 23:04 ` Sinan Kaya
  0 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-29 23:04 UTC (permalink / raw)
  To: linux-pci, timur
  Cc: mayurkumar.patel, linux-arm-msm, linux-arm-kernel, Sinan Kaya

When the operating system is booted with the default ASPM policy
(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 power on state.

------------------------
Changes from v5 (https://www.spinics.net/lists/arm-kernel/msg571758.html)
------------------------
- rebase to 4.11-rc3
- Split pci_aspm_init() body into pci_aspm_init_upstream()
  and pci_aspm_init_downstream() for bridge and endpoint
  specific code behavior.
- Get rid of function0 function.
  Detect downstream link in pci_aspm_init_upstream() instead

Sinan Kaya (5):
  PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities()
  PCI/ASPM: split pci_aspm_init() into two
  PCI/ASPM: add init hook to device_add
  PCI/ASPM: save power on values during bridge init
  PCI/ASPM: move link_state cleanup to bridge remove

 drivers/pci/pcie/aspm.c | 137 ++++++++++++++++++++++++++++++++----------------
 drivers/pci/probe.c     |   3 ++
 drivers/pci/remove.c    |   3 +-
 include/linux/pci.h     |   2 +
 4 files changed, 98 insertions(+), 47 deletions(-)

-- 
1.9.1

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

* [PATCH V6 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT
@ 2017-03-29 23:04 ` Sinan Kaya
  0 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-29 23:04 UTC (permalink / raw)
  To: linux-arm-kernel

When the operating system is booted with the default ASPM policy
(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 power on state.

------------------------
Changes from v5 (https://www.spinics.net/lists/arm-kernel/msg571758.html)
------------------------
- rebase to 4.11-rc3
- Split pci_aspm_init() body into pci_aspm_init_upstream()
  and pci_aspm_init_downstream() for bridge and endpoint
  specific code behavior.
- Get rid of function0 function.
  Detect downstream link in pci_aspm_init_upstream() instead

Sinan Kaya (5):
  PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities()
  PCI/ASPM: split pci_aspm_init() into two
  PCI/ASPM: add init hook to device_add
  PCI/ASPM: save power on values during bridge init
  PCI/ASPM: move link_state cleanup to bridge remove

 drivers/pci/pcie/aspm.c | 137 ++++++++++++++++++++++++++++++++----------------
 drivers/pci/probe.c     |   3 ++
 drivers/pci/remove.c    |   3 +-
 include/linux/pci.h     |   2 +
 4 files changed, 98 insertions(+), 47 deletions(-)

-- 
1.9.1

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

* [PATCH V6 1/5] PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities()
  2017-03-29 23:04 ` Sinan Kaya
  (?)
@ 2017-03-29 23:04   ` Sinan Kaya
  -1 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-29 23:04 UTC (permalink / raw)
  To: linux-pci, timur
  Cc: mayurkumar.patel, linux-arm-msm, linux-arm-kernel, Sinan Kaya,
	Bjorn Helgaas, Rajat Jain, Julia Lawall, Yinghai Lu, David Daney,
	Shawn Lin, linux-kernel

We need a callback from pci_init_capabilities function for every
single new PCI device that is currently being added.

pci_aspm_init() will be used to save the power on state of the HW.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895
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 1dfa10c..dc36717 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -827,6 +827,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

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

* [PATCH V6 1/5] PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities()
@ 2017-03-29 23:04   ` Sinan Kaya
  0 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-29 23:04 UTC (permalink / raw)
  To: linux-pci, timur
  Cc: mayurkumar.patel, David Daney, linux-arm-msm, Shawn Lin,
	linux-kernel, Sinan Kaya, Julia Lawall, Bjorn Helgaas,
	Rajat Jain, Yinghai Lu, linux-arm-kernel

We need a callback from pci_init_capabilities function for every
single new PCI device that is currently being added.

pci_aspm_init() will be used to save the power on state of the HW.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895
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 1dfa10c..dc36717 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -827,6 +827,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


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

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

* [PATCH V6 1/5] PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities()
@ 2017-03-29 23:04   ` Sinan Kaya
  0 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-29 23:04 UTC (permalink / raw)
  To: linux-arm-kernel

We need a callback from pci_init_capabilities function for every
single new PCI device that is currently being added.

pci_aspm_init() will be used to save the power on state of the HW.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895
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 1dfa10c..dc36717 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -827,6 +827,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

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

* [PATCH V6 2/5] PCI/ASPM: split pci_aspm_init() into two
  2017-03-29 23:04 ` Sinan Kaya
@ 2017-03-29 23:04   ` Sinan Kaya
  -1 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-29 23:04 UTC (permalink / raw)
  To: linux-pci, timur
  Cc: mayurkumar.patel, linux-arm-msm, linux-arm-kernel, Sinan Kaya,
	Bjorn Helgaas, Rajat Jain, Julia Lawall, David Daney,
	linux-kernel

Split pci_aspm_init() body into pci_aspm_init_upstream()
and pci_aspm_init_downstream() for bridge and endpoint
specific code behavior.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pcie/aspm.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index dc36717..a80d64b 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -826,6 +826,16 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
 	return link;
 }
 
+static int pci_aspm_init_downstream(struct pci_dev *pdev)
+{
+	return 0;
+}
+
+static int pci_aspm_init_upstream(struct pci_dev *pdev)
+{
+	return 0;
+}
+
 /*
  * pci_aspm_init: Initiate PCI express link state.
  * It is called from device_add for every single pci device.
@@ -833,7 +843,10 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
  */
 int pci_aspm_init(struct pci_dev *pdev)
 {
-	return 0;
+	if (!pdev->has_secondary_link)
+		return pci_aspm_init_downstream(pdev);
+
+	return pci_aspm_init_upstream(pdev);
 }
 
 /*
-- 
1.9.1

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

* [PATCH V6 2/5] PCI/ASPM: split pci_aspm_init() into two
@ 2017-03-29 23:04   ` Sinan Kaya
  0 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-29 23:04 UTC (permalink / raw)
  To: linux-arm-kernel

Split pci_aspm_init() body into pci_aspm_init_upstream()
and pci_aspm_init_downstream() for bridge and endpoint
specific code behavior.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pcie/aspm.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index dc36717..a80d64b 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -826,6 +826,16 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
 	return link;
 }
 
+static int pci_aspm_init_downstream(struct pci_dev *pdev)
+{
+	return 0;
+}
+
+static int pci_aspm_init_upstream(struct pci_dev *pdev)
+{
+	return 0;
+}
+
 /*
  * pci_aspm_init: Initiate PCI express link state.
  * It is called from device_add for every single pci device.
@@ -833,7 +843,10 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
  */
 int pci_aspm_init(struct pci_dev *pdev)
 {
-	return 0;
+	if (!pdev->has_secondary_link)
+		return pci_aspm_init_downstream(pdev);
+
+	return pci_aspm_init_upstream(pdev);
 }
 
 /*
-- 
1.9.1

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

* [PATCH V6 3/5] PCI/ASPM: add init hook to device_add
  2017-03-29 23:04 ` Sinan Kaya
@ 2017-03-29 23:04   ` Sinan Kaya
  -1 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-29 23:04 UTC (permalink / raw)
  To: linux-pci, timur
  Cc: mayurkumar.patel, linux-arm-msm, linux-arm-kernel, Sinan Kaya,
	Bjorn Helgaas, Rajat Jain, Shawn Lin, David Daney, Yinghai Lu,
	Julia Lawall, linux-kernel

For bridges, have pcie_aspm_init_link_state() allocate a link_state,
regardless of whether it currently has any children.

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.

Now that we are calling alloc_pcie_link_state() from pci_aspm_init()
, get rid of pci_function_0 function and detect downstream link in
pci_aspm_init_upstream() instead when the function number is 0.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pcie/aspm.c | 72 ++++++++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index a80d64b..e33f84b 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -422,20 +422,6 @@ 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 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;
-}
-
 /* Calculate L1.2 PM substate timing parameters */
 static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 				struct aspm_register_info *upreg,
@@ -798,7 +784,6 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
 	INIT_LIST_HEAD(&link->children);
 	INIT_LIST_HEAD(&link->link);
 	link->pdev = pdev;
-	link->downstream = pci_function_0(pdev->subordinate);
 
 	/*
 	 * Root Ports and PCI/PCI-X to PCIe Bridges are roots of PCIe
@@ -828,11 +813,33 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
 
 static int pci_aspm_init_downstream(struct pci_dev *pdev)
 {
+	struct pcie_link_state *link;
+	struct pci_dev *parent;
+
+	/*
+	 * The L1 PM substate capability is only implemented in function 0 in a
+	 * multi function device.
+	 */
+	if (PCI_FUNC(pdev->devfn) != 0)
+		return -EINVAL;
+
+	parent = pdev->bus->self;
+	if (!parent)
+		return -EINVAL;
+
+	link = parent->link_state;
+	link->downstream = pdev;
 	return 0;
 }
 
 static int pci_aspm_init_upstream(struct pci_dev *pdev)
 {
+	struct pcie_link_state *link;
+
+	link = alloc_pcie_link_state(pdev);
+	if (!link)
+		return -ENOMEM;
+
 	return 0;
 }
 
@@ -843,6 +850,17 @@ static int pci_aspm_init_upstream(struct pci_dev *pdev)
  */
 int pci_aspm_init(struct pci_dev *pdev)
 {
+	if (!aspm_support_enabled)
+		return 0;
+
+	if (!pci_is_pcie(pdev))
+		return -EINVAL;
+
+	/* 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 pci_aspm_init_downstream(pdev);
 
@@ -859,33 +877,16 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
 	struct pcie_link_state *link;
 	int blacklist = !!pcie_aspm_sanity_check(pdev);
 
-	if (!aspm_support_enabled)
-		return;
-
-	if (pdev->link_state)
-		return;
-
-	/*
-	 * 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
-	 * Link on its secondary side.
-	 */
-	if (!pdev->has_secondary_link)
-		return;
-
-	/* VIA has a strange chipset, root port is under a bridge */
-	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT &&
-	    pdev->bus->self)
+	if (!pdev->link_state)
 		return;
 
+	link = pdev->link_state;
 	down_read(&pci_bus_sem);
 	if (list_empty(&pdev->subordinate->devices))
 		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
@@ -910,7 +911,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

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

* [PATCH V6 3/5] PCI/ASPM: add init hook to device_add
@ 2017-03-29 23:04   ` Sinan Kaya
  0 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-29 23:04 UTC (permalink / raw)
  To: linux-arm-kernel

For bridges, have pcie_aspm_init_link_state() allocate a link_state,
regardless of whether it currently has any children.

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.

Now that we are calling alloc_pcie_link_state() from pci_aspm_init()
, get rid of pci_function_0 function and detect downstream link in
pci_aspm_init_upstream() instead when the function number is 0.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pcie/aspm.c | 72 ++++++++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index a80d64b..e33f84b 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -422,20 +422,6 @@ 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 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;
-}
-
 /* Calculate L1.2 PM substate timing parameters */
 static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 				struct aspm_register_info *upreg,
@@ -798,7 +784,6 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
 	INIT_LIST_HEAD(&link->children);
 	INIT_LIST_HEAD(&link->link);
 	link->pdev = pdev;
-	link->downstream = pci_function_0(pdev->subordinate);
 
 	/*
 	 * Root Ports and PCI/PCI-X to PCIe Bridges are roots of PCIe
@@ -828,11 +813,33 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
 
 static int pci_aspm_init_downstream(struct pci_dev *pdev)
 {
+	struct pcie_link_state *link;
+	struct pci_dev *parent;
+
+	/*
+	 * The L1 PM substate capability is only implemented in function 0 in a
+	 * multi function device.
+	 */
+	if (PCI_FUNC(pdev->devfn) != 0)
+		return -EINVAL;
+
+	parent = pdev->bus->self;
+	if (!parent)
+		return -EINVAL;
+
+	link = parent->link_state;
+	link->downstream = pdev;
 	return 0;
 }
 
 static int pci_aspm_init_upstream(struct pci_dev *pdev)
 {
+	struct pcie_link_state *link;
+
+	link = alloc_pcie_link_state(pdev);
+	if (!link)
+		return -ENOMEM;
+
 	return 0;
 }
 
@@ -843,6 +850,17 @@ static int pci_aspm_init_upstream(struct pci_dev *pdev)
  */
 int pci_aspm_init(struct pci_dev *pdev)
 {
+	if (!aspm_support_enabled)
+		return 0;
+
+	if (!pci_is_pcie(pdev))
+		return -EINVAL;
+
+	/* 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 pci_aspm_init_downstream(pdev);
 
@@ -859,33 +877,16 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
 	struct pcie_link_state *link;
 	int blacklist = !!pcie_aspm_sanity_check(pdev);
 
-	if (!aspm_support_enabled)
-		return;
-
-	if (pdev->link_state)
-		return;
-
-	/*
-	 * 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
-	 * Link on its secondary side.
-	 */
-	if (!pdev->has_secondary_link)
-		return;
-
-	/* VIA has a strange chipset, root port is under a bridge */
-	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT &&
-	    pdev->bus->self)
+	if (!pdev->link_state)
 		return;
 
+	link = pdev->link_state;
 	down_read(&pci_bus_sem);
 	if (list_empty(&pdev->subordinate->devices))
 		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
@@ -910,7 +911,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

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

* [PATCH V6 4/5] PCI/ASPM: save power on values during bridge init
  2017-03-29 23:04 ` Sinan Kaya
@ 2017-03-29 23:04   ` Sinan Kaya
  -1 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-29 23:04 UTC (permalink / raw)
  To: linux-pci, timur
  Cc: mayurkumar.patel, linux-arm-msm, linux-arm-kernel, Sinan Kaya,
	Bjorn Helgaas, Rajat Jain, David Daney, Yinghai Lu, Julia Lawall,
	linux-kernel

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.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pcie/aspm.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index e33f84b..a38602a 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -505,8 +505,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);
@@ -542,9 +544,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;
 	/*
@@ -835,11 +834,26 @@ static int pci_aspm_init_downstream(struct pci_dev *pdev)
 static int pci_aspm_init_upstream(struct pci_dev *pdev)
 {
 	struct pcie_link_state *link;
+	struct aspm_register_info upreg;
 
 	link = alloc_pcie_link_state(pdev);
 	if (!link)
 		return -ENOMEM;
 
+	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;
+
 	return 0;
 }
 
-- 
1.9.1

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

* [PATCH V6 4/5] PCI/ASPM: save power on values during bridge init
@ 2017-03-29 23:04   ` Sinan Kaya
  0 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-29 23:04 UTC (permalink / raw)
  To: linux-arm-kernel

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.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pcie/aspm.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index e33f84b..a38602a 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -505,8 +505,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);
@@ -542,9 +544,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;
 	/*
@@ -835,11 +834,26 @@ static int pci_aspm_init_downstream(struct pci_dev *pdev)
 static int pci_aspm_init_upstream(struct pci_dev *pdev)
 {
 	struct pcie_link_state *link;
+	struct aspm_register_info upreg;
 
 	link = alloc_pcie_link_state(pdev);
 	if (!link)
 		return -ENOMEM;
 
+	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;
+
 	return 0;
 }
 
-- 
1.9.1

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

* [PATCH V6 5/5] PCI/ASPM: move link_state cleanup to bridge remove
  2017-03-29 23:04 ` Sinan Kaya
@ 2017-03-29 23:04   ` Sinan Kaya
  -1 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-29 23:04 UTC (permalink / raw)
  To: linux-pci, timur
  Cc: mayurkumar.patel, linux-arm-msm, linux-arm-kernel, Sinan Kaya,
	Bjorn Helgaas, Rajat Jain, David Daney, Yinghai Lu, linux-kernel

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.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pcie/aspm.c | 20 +++++++++++++++-----
 drivers/pci/remove.c    |  3 +--
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index a38602a..37ca9b3 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -963,6 +963,21 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
 	if (!parent || !parent->link_state)
 		return;
 
+	if (pdev->has_secondary_link) {
+		link = pdev->link_state;
+		down_read(&pci_bus_sem);
+		mutex_lock(&aspm_lock);
+
+		list_del(&link->sibling);
+		list_del(&link->link);
+
+		/* Clock PM is for endpoint device */
+		free_link_state(link);
+		mutex_unlock(&aspm_lock);
+		up_read(&pci_bus_sem);
+		return;
+	}
+
 	down_read(&pci_bus_sem);
 	mutex_lock(&aspm_lock);
 	/*
@@ -978,11 +993,6 @@ 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);
-
 	/* Recheck latencies and configure upstream links */
 	if (parent_link) {
 		pcie_update_aspm_capable(root);
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 73a03d3..7e14ebd 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -25,8 +25,7 @@ static void pci_stop_dev(struct pci_dev *dev)
 		dev->is_added = 0;
 	}
 
-	if (dev->bus->self)
-		pcie_aspm_exit_link_state(dev);
+	pcie_aspm_exit_link_state(dev);
 }
 
 static void pci_destroy_dev(struct pci_dev *dev)
-- 
1.9.1

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

* [PATCH V6 5/5] PCI/ASPM: move link_state cleanup to bridge remove
@ 2017-03-29 23:04   ` Sinan Kaya
  0 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-29 23:04 UTC (permalink / raw)
  To: linux-arm-kernel

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.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pcie/aspm.c | 20 +++++++++++++++-----
 drivers/pci/remove.c    |  3 +--
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index a38602a..37ca9b3 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -963,6 +963,21 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
 	if (!parent || !parent->link_state)
 		return;
 
+	if (pdev->has_secondary_link) {
+		link = pdev->link_state;
+		down_read(&pci_bus_sem);
+		mutex_lock(&aspm_lock);
+
+		list_del(&link->sibling);
+		list_del(&link->link);
+
+		/* Clock PM is for endpoint device */
+		free_link_state(link);
+		mutex_unlock(&aspm_lock);
+		up_read(&pci_bus_sem);
+		return;
+	}
+
 	down_read(&pci_bus_sem);
 	mutex_lock(&aspm_lock);
 	/*
@@ -978,11 +993,6 @@ 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);
-
 	/* Recheck latencies and configure upstream links */
 	if (parent_link) {
 		pcie_update_aspm_capable(root);
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 73a03d3..7e14ebd 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -25,8 +25,7 @@ static void pci_stop_dev(struct pci_dev *dev)
 		dev->is_added = 0;
 	}
 
-	if (dev->bus->self)
-		pcie_aspm_exit_link_state(dev);
+	pcie_aspm_exit_link_state(dev);
 }
 
 static void pci_destroy_dev(struct pci_dev *dev)
-- 
1.9.1

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

* RE: [PATCH V6 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT
  2017-03-29 23:04 ` Sinan Kaya
  (?)
@ 2017-03-30 13:07   ` Patel, Mayurkumar
  -1 siblings, 0 replies; 22+ messages in thread
From: Patel, Mayurkumar @ 2017-03-30 13:07 UTC (permalink / raw)
  To: Sinan Kaya, linux-pci, timur; +Cc: linux-arm-msm, linux-arm-kernel

Hi Sinan

>
>When the operating system is booted with the default ASPM policy
>(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 power on state.
>
>------------------------
>Changes from v5 (https://www.spinics.net/lists/arm-kernel/msg571758.html)
>------------------------
>- rebase to 4.11-rc3
>- Split pci_aspm_init() body into pci_aspm_init_upstream()
>  and pci_aspm_init_downstream() for bridge and endpoint
>  specific code behavior.
>- Get rid of function0 function.
>  Detect downstream link in pci_aspm_init_upstream() instead
>
>Sinan Kaya (5):
>  PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities()
>  PCI/ASPM: split pci_aspm_init() into two
>  PCI/ASPM: add init hook to device_add
>  PCI/ASPM: save power on values during bridge init
>  PCI/ASPM: move link_state cleanup to bridge remove
>

Thanks for quick patches. I can boot up now without any crash. I will test actual
functionality by next week as I am out for this week.




> drivers/pci/pcie/aspm.c | 137 ++++++++++++++++++++++++++++++++----------------
> drivers/pci/probe.c     |   3 ++
> drivers/pci/remove.c    |   3 +-
> include/linux/pci.h     |   2 +
> 4 files changed, 98 insertions(+), 47 deletions(-)
>
>--
>1.9.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

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

* RE: [PATCH V6 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT
@ 2017-03-30 13:07   ` Patel, Mayurkumar
  0 siblings, 0 replies; 22+ messages in thread
From: Patel, Mayurkumar @ 2017-03-30 13:07 UTC (permalink / raw)
  To: Sinan Kaya, linux-pci, timur; +Cc: linux-arm-msm, linux-arm-kernel

Hi Sinan

>
>When the operating system is booted with the default ASPM policy
>(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 power on state.
>
>------------------------
>Changes from v5 (https://www.spinics.net/lists/arm-kernel/msg571758.html)
>------------------------
>- rebase to 4.11-rc3
>- Split pci_aspm_init() body into pci_aspm_init_upstream()
>  and pci_aspm_init_downstream() for bridge and endpoint
>  specific code behavior.
>- Get rid of function0 function.
>  Detect downstream link in pci_aspm_init_upstream() instead
>
>Sinan Kaya (5):
>  PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities()
>  PCI/ASPM: split pci_aspm_init() into two
>  PCI/ASPM: add init hook to device_add
>  PCI/ASPM: save power on values during bridge init
>  PCI/ASPM: move link_state cleanup to bridge remove
>

Thanks for quick patches. I can boot up now without any crash. I will test actual
functionality by next week as I am out for this week.




> drivers/pci/pcie/aspm.c | 137 ++++++++++++++++++++++++++++++++----------------
> drivers/pci/probe.c     |   3 ++
> drivers/pci/remove.c    |   3 +-
> include/linux/pci.h     |   2 +
> 4 files changed, 98 insertions(+), 47 deletions(-)
>
>--
>1.9.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928


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

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

* [PATCH V6 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT
@ 2017-03-30 13:07   ` Patel, Mayurkumar
  0 siblings, 0 replies; 22+ messages in thread
From: Patel, Mayurkumar @ 2017-03-30 13:07 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Sinan

>
>When the operating system is booted with the default ASPM policy
>(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 power on state.
>
>------------------------
>Changes from v5 (https://www.spinics.net/lists/arm-kernel/msg571758.html)
>------------------------
>- rebase to 4.11-rc3
>- Split pci_aspm_init() body into pci_aspm_init_upstream()
>  and pci_aspm_init_downstream() for bridge and endpoint
>  specific code behavior.
>- Get rid of function0 function.
>  Detect downstream link in pci_aspm_init_upstream() instead
>
>Sinan Kaya (5):
>  PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities()
>  PCI/ASPM: split pci_aspm_init() into two
>  PCI/ASPM: add init hook to device_add
>  PCI/ASPM: save power on values during bridge init
>  PCI/ASPM: move link_state cleanup to bridge remove
>

Thanks for quick patches. I can boot up now without any crash. I will test actual
functionality by next week as I am out for this week.




> drivers/pci/pcie/aspm.c | 137 ++++++++++++++++++++++++++++++++----------------
> drivers/pci/probe.c     |   3 ++
> drivers/pci/remove.c    |   3 +-
> include/linux/pci.h     |   2 +
> 4 files changed, 98 insertions(+), 47 deletions(-)
>
>--
>1.9.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

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

* Re: [PATCH V6 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT
  2017-03-30 13:07   ` Patel, Mayurkumar
  (?)
@ 2017-03-30 13:20     ` Sinan Kaya
  -1 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-30 13:20 UTC (permalink / raw)
  To: Patel, Mayurkumar, linux-pci, timur; +Cc: linux-arm-msm, linux-arm-kernel

On 3/30/2017 9:07 AM, Patel, Mayurkumar wrote:
> Thanks for quick patches. I can boot up now without any crash. I will test actual
> functionality by next week as I am out for this week.
> 

Yeah, no problem. Let me know how it goes.

> 
> 
> 


-- 
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.

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

* Re: [PATCH V6 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT
@ 2017-03-30 13:20     ` Sinan Kaya
  0 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-30 13:20 UTC (permalink / raw)
  To: Patel, Mayurkumar, linux-pci, timur; +Cc: linux-arm-msm, linux-arm-kernel

On 3/30/2017 9:07 AM, Patel, Mayurkumar wrote:
> Thanks for quick patches. I can boot up now without any crash. I will test actual
> functionality by next week as I am out for this week.
> 

Yeah, no problem. Let me know how it goes.

> 
> 
> 


-- 
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.

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

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

* [PATCH V6 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT
@ 2017-03-30 13:20     ` Sinan Kaya
  0 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-30 13:20 UTC (permalink / raw)
  To: linux-arm-kernel

On 3/30/2017 9:07 AM, Patel, Mayurkumar wrote:
> Thanks for quick patches. I can boot up now without any crash. I will test actual
> functionality by next week as I am out for this week.
> 

Yeah, no problem. Let me know how it goes.

> 
> 
> 


-- 
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.

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

* Re: [PATCH V6 5/5] PCI/ASPM: move link_state cleanup to bridge remove
  2017-03-29 23:04   ` Sinan Kaya
  (?)
@ 2017-03-30 13:25     ` Sinan Kaya
  -1 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-30 13:25 UTC (permalink / raw)
  To: linux-pci, timur
  Cc: mayurkumar.patel, linux-arm-msm, linux-arm-kernel, Bjorn Helgaas,
	Rajat Jain, David Daney, Yinghai Lu, linux-kernel

On 3/29/2017 7:04 PM, Sinan Kaya wrote:
>  	if (!parent || !parent->link_state)
>  		return;
>  
> +	if (pdev->has_secondary_link) {
> +		link = pdev->link_state;

I think I accidentally moved the parent check above this code
and broke the case where you can actually remove the root port.

I'll post v7 with reverting this change.

-- 
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.

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

* Re: [PATCH V6 5/5] PCI/ASPM: move link_state cleanup to bridge remove
@ 2017-03-30 13:25     ` Sinan Kaya
  0 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-30 13:25 UTC (permalink / raw)
  To: linux-pci, timur
  Cc: mayurkumar.patel, David Daney, linux-arm-msm, linux-kernel,
	Bjorn Helgaas, Rajat Jain, Yinghai Lu, linux-arm-kernel

On 3/29/2017 7:04 PM, Sinan Kaya wrote:
>  	if (!parent || !parent->link_state)
>  		return;
>  
> +	if (pdev->has_secondary_link) {
> +		link = pdev->link_state;

I think I accidentally moved the parent check above this code
and broke the case where you can actually remove the root port.

I'll post v7 with reverting this change.

-- 
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.

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

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

* [PATCH V6 5/5] PCI/ASPM: move link_state cleanup to bridge remove
@ 2017-03-30 13:25     ` Sinan Kaya
  0 siblings, 0 replies; 22+ messages in thread
From: Sinan Kaya @ 2017-03-30 13:25 UTC (permalink / raw)
  To: linux-arm-kernel

On 3/29/2017 7:04 PM, Sinan Kaya wrote:
>  	if (!parent || !parent->link_state)
>  		return;
>  
> +	if (pdev->has_secondary_link) {
> +		link = pdev->link_state;

I think I accidentally moved the parent check above this code
and broke the case where you can actually remove the root port.

I'll post v7 with reverting this change.

-- 
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.

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

end of thread, other threads:[~2017-03-30 13:25 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-29 23:04 [PATCH V6 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT Sinan Kaya
2017-03-29 23:04 ` Sinan Kaya
2017-03-29 23:04 ` [PATCH V6 1/5] PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities() Sinan Kaya
2017-03-29 23:04   ` Sinan Kaya
2017-03-29 23:04   ` Sinan Kaya
2017-03-29 23:04 ` [PATCH V6 2/5] PCI/ASPM: split pci_aspm_init() into two Sinan Kaya
2017-03-29 23:04   ` Sinan Kaya
2017-03-29 23:04 ` [PATCH V6 3/5] PCI/ASPM: add init hook to device_add Sinan Kaya
2017-03-29 23:04   ` Sinan Kaya
2017-03-29 23:04 ` [PATCH V6 4/5] PCI/ASPM: save power on values during bridge init Sinan Kaya
2017-03-29 23:04   ` Sinan Kaya
2017-03-29 23:04 ` [PATCH V6 5/5] PCI/ASPM: move link_state cleanup to bridge remove Sinan Kaya
2017-03-29 23:04   ` Sinan Kaya
2017-03-30 13:25   ` Sinan Kaya
2017-03-30 13:25     ` Sinan Kaya
2017-03-30 13:25     ` Sinan Kaya
2017-03-30 13:07 ` [PATCH V6 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT Patel, Mayurkumar
2017-03-30 13:07   ` Patel, Mayurkumar
2017-03-30 13:07   ` Patel, Mayurkumar
2017-03-30 13:20   ` Sinan Kaya
2017-03-30 13:20     ` Sinan Kaya
2017-03-30 13:20     ` Sinan Kaya

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.