[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[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 v6 (https://www.spinics.net/lists/arm-kernel/msg572876.html)
------------------------
- revert the accidental parent check in bridge remove

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

[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[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 v6 (https://www.spinics.net/lists/arm-kernel/msg572876.html)
------------------------
- revert the accidental parent check in bridge remove

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

[PATCH V7 1/5] PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities()

[Sinan Kaya]

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

[PATCH V7 1/5] PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities()

[Sinan Kaya]

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

[PATCH V7 1/5] PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities()

[Sinan Kaya]

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

[PATCH V7 2/5] PCI/ASPM: split pci_aspm_init() into two

[Sinan Kaya]

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

[PATCH V7 2/5] PCI/ASPM: split pci_aspm_init() into two

[Sinan Kaya]

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

[PATCH V7 3/5] PCI/ASPM: add init hook to device_add

[Sinan Kaya]

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

[PATCH V7 3/5] PCI/ASPM: add init hook to device_add

[Sinan Kaya]

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

[PATCH V7 4/5] PCI/ASPM: save power on values during bridge init

[Sinan Kaya]

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

[PATCH V7 4/5] PCI/ASPM: save power on values during bridge init

[Sinan Kaya]

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

[PATCH V7 5/5] 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.

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..e6fd297 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -960,6 +960,21 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
 	struct pci_dev *parent = pdev->bus->self;
 	struct pcie_link_state *link, *root, *parent_link;
 
+	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;
+	}
+
 	if (!parent || !parent->link_state)
 		return;
 
@@ -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

[PATCH V7 5/5] 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.

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..e6fd297 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -960,6 +960,21 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
 	struct pci_dev *parent = pdev->bus->self;
 	struct pcie_link_state *link, *root, *parent_link;
 
+	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;
+	}
+
 	if (!parent || !parent->link_state)
 		return;
 
@@ -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

Re: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

Hi Mayurkumar,

On 3/30/2017 9:30 AM, Sinan Kaya wrote:
> 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 v6 (https://www.spinics.net/lists/arm-kernel/msg572876.html)
> ------------------------
> - revert the accidental parent check in bridge remove
> 
> 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(-)
> 

Did you get a chance to test?

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.

Re: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

Hi Mayurkumar,

On 3/30/2017 9:30 AM, Sinan Kaya wrote:
> 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 v6 (https://www.spinics.net/lists/arm-kernel/msg572876.html)
> ------------------------
> - revert the accidental parent check in bridge remove
> 
> 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(-)
> 

Did you get a chance to test?

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.

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

[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

Hi Mayurkumar,

On 3/30/2017 9:30 AM, Sinan Kaya wrote:
> 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 v6 (https://www.spinics.net/lists/arm-kernel/msg572876.html)
> ------------------------
> - revert the accidental parent check in bridge remove
> 
> 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(-)
> 

Did you get a chance to test?

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.

RE: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Patel, Mayurkumar]

Hi Sinan

>Hi Mayurkumar,
>
>On 3/30/2017 9:30 AM, Sinan Kaya wrote:
>> 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 v6 (https://www.spinics.net/lists/arm-kernel/msg572876.html)
>> ------------------------
>> - revert the accidental parent check in bridge remove
>>
>> 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(-)
>>
>
>Did you get a chance to test?
>

Not yet. Will get back in this week sometimes to give you some feedback.



Thanks
Mayur

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

RE: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Patel, Mayurkumar]

Hi Sinan

>Hi Mayurkumar,
>
>On 3/30/2017 9:30 AM, Sinan Kaya wrote:
>> 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 v6 (https://www.spinics.net/lists/arm-kernel/msg572876.html)
>> ------------------------
>> - revert the accidental parent check in bridge remove
>>
>> 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(-)
>>
>
>Did you get a chance to test?
>

Not yet. Will get back in this week sometimes to give you some feedback.



Thanks
Mayur

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

[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Patel, Mayurkumar]

Hi Sinan

>Hi Mayurkumar,
>
>On 3/30/2017 9:30 AM, Sinan Kaya wrote:
>> 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 v6 (https://www.spinics.net/lists/arm-kernel/msg572876.html)
>> ------------------------
>> - revert the accidental parent check in bridge remove
>>
>> 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(-)
>>
>
>Did you get a chance to test?
>

Not yet. Will get back in this week sometimes to give you some feedback.



Thanks
Mayur

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

RE: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Patel, Mayurkumar]

Hi Sinan, Bjorn,

>
>Hi Sinan
>
>>Hi Mayurkumar,
>>
>>On 3/30/2017 9:30 AM, Sinan Kaya wrote:
>>> 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 v6 (https://www.spinics.net/lists/arm-kernel/msg572876.html)
>>> ------------------------
>>> - revert the accidental parent check in bridge remove
>>>
>>> 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(-)
>>>
>>
>>Did you get a chance to test?
>>
>
>Not yet. Will get back in this week sometimes to give you some feedback.
>

The patch seems to be working for ASPM L1 so far. I am seeing a problem now with your patches for L1.2 as following:

ASPM L1.2 does not get enabled on the Upstream as well as downstream port due to following reasons.
In the case, I see now, if the EP is not connected while reboot of the Machine, Root Port does not configure
It's own L1.2 also from the BIOS. (I am not sure whether it's even advisable to enable L1.2 on root
Port when no downstream device is not connected to it as it may have an impact on CLKREQ# and device is connected
At later stage may have a problem with it)

Later when the Device get connected, BIOS configures L1.2 for Root port and EP but due to policy set to incorrect,
Kernel disables ASPM L1.2.

Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
substates Configuration).  

@Bjorn:
I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
the spec.,

The spec. says following and correct me If I am wrong or I misinterpret the spec. :

5.5.4. L1 PM Substates Configuration

The Setting of any enable bit must be performed at the Downstream Port before the
corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.



Thanks
Mayur

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

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

RE: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Patel, Mayurkumar]

Hi Sinan, Bjorn,

>
>Hi Sinan
>
>>Hi Mayurkumar,
>>
>>On 3/30/2017 9:30 AM, Sinan Kaya wrote:
>>> 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 v6 (https://www.spinics.net/lists/arm-kernel/msg572876.html)
>>> ------------------------
>>> - revert the accidental parent check in bridge remove
>>>
>>> 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(-)
>>>
>>
>>Did you get a chance to test?
>>
>
>Not yet. Will get back in this week sometimes to give you some feedback.
>

The patch seems to be working for ASPM L1 so far. I am seeing a problem now with your patches for L1.2 as following:

ASPM L1.2 does not get enabled on the Upstream as well as downstream port due to following reasons.
In the case, I see now, if the EP is not connected while reboot of the Machine, Root Port does not configure
It's own L1.2 also from the BIOS. (I am not sure whether it's even advisable to enable L1.2 on root
Port when no downstream device is not connected to it as it may have an impact on CLKREQ# and device is connected
At later stage may have a problem with it)

Later when the Device get connected, BIOS configures L1.2 for Root port and EP but due to policy set to incorrect,
Kernel disables ASPM L1.2.

Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
substates Configuration).  

@Bjorn:
I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
the spec.,

The spec. says following and correct me If I am wrong or I misinterpret the spec. :

5.5.4. L1 PM Substates Configuration

The Setting of any enable bit must be performed at the Downstream Port before the
corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.



Thanks
Mayur

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

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

[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Patel, Mayurkumar]

Hi Sinan, Bjorn,

>
>Hi Sinan
>
>>Hi Mayurkumar,
>>
>>On 3/30/2017 9:30 AM, Sinan Kaya wrote:
>>> 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 v6 (https://www.spinics.net/lists/arm-kernel/msg572876.html)
>>> ------------------------
>>> - revert the accidental parent check in bridge remove
>>>
>>> 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(-)
>>>
>>
>>Did you get a chance to test?
>>
>
>Not yet. Will get back in this week sometimes to give you some feedback.
>

The patch seems to be working for ASPM L1 so far. I am seeing a problem now with your patches for L1.2 as following:

ASPM L1.2 does not get enabled on the Upstream as well as downstream port due to following reasons.
In the case, I see now, if the EP is not connected while reboot of the Machine, Root Port does not configure
It's own L1.2 also from the BIOS. (I am not sure whether it's even advisable to enable L1.2 on root
Port when no downstream device is not connected to it as it may have an impact on CLKREQ# and device is connected
At later stage may have a problem with it)

Later when the Device get connected, BIOS configures L1.2 for Root port and EP but due to policy set to incorrect,
Kernel disables ASPM L1.2.

Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
substates Configuration).  

@Bjorn:
I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
the spec.,

The spec. says following and correct me If I am wrong or I misinterpret the spec. :

5.5.4. L1 PM Substates Configuration

The Setting of any enable bit must be performed at the Downstream Port before the
corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.



Thanks
Mayur

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

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

Re: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
> Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
> In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
> In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
> substates Configuration).  
> 
> @Bjorn:
> I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
> the spec.,
> 
> The spec. says following and correct me If I am wrong or I misinterpret the spec. :
> 
> 5.5.4. L1 PM Substates Configuration
> 
> The Setting of any enable bit must be performed at the Downstream Port before the
> corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
> bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
> before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.
> 

Thanks for testing. 

commit a142f4d3e5c54db5e942fa6ee5f3dc0e8c83207b
Author: Rajat Jain <rajatja@google.com>
Date:   Mon Jan 2 22:34:15 2017 -0800

    PCI/ASPM: Add comment about L1 substate latency

    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 currently. I'd be glad to
    take up any suggestions).

    Signed-off-by: Rajat Jain <rajatja@google.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

I added Rajat for discussion on L1SS. I think this deserves its own discussion. 
I'll look at the other part of your email and move things around a little bit
less aggressively for the aspm_default.


-- 
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 V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
> Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
> In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
> In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
> substates Configuration).  
> 
> @Bjorn:
> I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
> the spec.,
> 
> The spec. says following and correct me If I am wrong or I misinterpret the spec. :
> 
> 5.5.4. L1 PM Substates Configuration
> 
> The Setting of any enable bit must be performed at the Downstream Port before the
> corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
> bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
> before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.
> 

Thanks for testing. 

commit a142f4d3e5c54db5e942fa6ee5f3dc0e8c83207b
Author: Rajat Jain <rajatja@google.com>
Date:   Mon Jan 2 22:34:15 2017 -0800

    PCI/ASPM: Add comment about L1 substate latency

    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 currently. I'd be glad to
    take up any suggestions).

    Signed-off-by: Rajat Jain <rajatja@google.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

I added Rajat for discussion on L1SS. I think this deserves its own discussion. 
I'll look at the other part of your email and move things around a little bit
less aggressively for the aspm_default.


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

[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
> Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
> In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
> In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
> substates Configuration).  
> 
> @Bjorn:
> I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
> the spec.,
> 
> The spec. says following and correct me If I am wrong or I misinterpret the spec. :
> 
> 5.5.4. L1 PM Substates Configuration
> 
> The Setting of any enable bit must be performed at the Downstream Port before the
> corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
> bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
> before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.
> 

Thanks for testing. 

commit a142f4d3e5c54db5e942fa6ee5f3dc0e8c83207b
Author: Rajat Jain <rajatja@google.com>
Date:   Mon Jan 2 22:34:15 2017 -0800

    PCI/ASPM: Add comment about L1 substate latency

    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 currently. I'd be glad to
    take up any suggestions).

    Signed-off-by: Rajat Jain <rajatja@google.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

I added Rajat for discussion on L1SS. I think this deserves its own discussion. 
I'll look at the other part of your email and move things around a little bit
less aggressively for the aspm_default.


-- 
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 V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

Hi Mayurkumar,

On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
> The patch seems to be working for ASPM L1 so far. I am seeing a problem now with your patches for L1.2 as following:
> 
> ASPM L1.2 does not get enabled on the Upstream as well as downstream port due to following reasons.
> In the case, I see now, if the EP is not connected while reboot of the Machine, Root Port does not configure
> It's own L1.2 also from the BIOS. (I am not sure whether it's even advisable to enable L1.2 on root
> Port when no downstream device is not connected to it as it may have an impact on CLKREQ# and device is connected
> At later stage may have a problem with it)
> 

I have two questions:

1. if the endpoint is connected during boot and have ASPM L1.2 enabled during boot,
do you see that L1.2 gets re-enabled following a hotplug remove and then insert. 
This is the goal of this patch. If yes, we achieved our goal. This is working on
my platform but I do not have L1SS support on my platform.

2. if you do not connect any endpoint during boot and insert the card, do you
see any ASPM enabled at all with and without my patch using the default policy? 
I think this is the part you are describing above. I'll confirm this on my platform
too. I think this one will require another patch/discussion unless I broke something.


> Later when the Device get connected, BIOS configures L1.2 for Root port and EP but due to policy set to incorrect,
> Kernel disables ASPM L1.2.

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.

Re: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

Hi Mayurkumar,

On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
> The patch seems to be working for ASPM L1 so far. I am seeing a problem now with your patches for L1.2 as following:
> 
> ASPM L1.2 does not get enabled on the Upstream as well as downstream port due to following reasons.
> In the case, I see now, if the EP is not connected while reboot of the Machine, Root Port does not configure
> It's own L1.2 also from the BIOS. (I am not sure whether it's even advisable to enable L1.2 on root
> Port when no downstream device is not connected to it as it may have an impact on CLKREQ# and device is connected
> At later stage may have a problem with it)
> 

I have two questions:

1. if the endpoint is connected during boot and have ASPM L1.2 enabled during boot,
do you see that L1.2 gets re-enabled following a hotplug remove and then insert. 
This is the goal of this patch. If yes, we achieved our goal. This is working on
my platform but I do not have L1SS support on my platform.

2. if you do not connect any endpoint during boot and insert the card, do you
see any ASPM enabled at all with and without my patch using the default policy? 
I think this is the part you are describing above. I'll confirm this on my platform
too. I think this one will require another patch/discussion unless I broke something.


> Later when the Device get connected, BIOS configures L1.2 for Root port and EP but due to policy set to incorrect,
> Kernel disables ASPM L1.2.

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.

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

[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

Hi Mayurkumar,

On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
> The patch seems to be working for ASPM L1 so far. I am seeing a problem now with your patches for L1.2 as following:
> 
> ASPM L1.2 does not get enabled on the Upstream as well as downstream port due to following reasons.
> In the case, I see now, if the EP is not connected while reboot of the Machine, Root Port does not configure
> It's own L1.2 also from the BIOS. (I am not sure whether it's even advisable to enable L1.2 on root
> Port when no downstream device is not connected to it as it may have an impact on CLKREQ# and device is connected
> At later stage may have a problem with it)
> 

I have two questions:

1. if the endpoint is connected during boot and have ASPM L1.2 enabled during boot,
do you see that L1.2 gets re-enabled following a hotplug remove and then insert. 
This is the goal of this patch. If yes, we achieved our goal. This is working on
my platform but I do not have L1SS support on my platform.

2. if you do not connect any endpoint during boot and insert the card, do you
see any ASPM enabled at all with and without my patch using the default policy? 
I think this is the part you are describing above. I'll confirm this on my platform
too. I think this one will require another patch/discussion unless I broke something.


> Later when the Device get connected, BIOS configures L1.2 for Root port and EP but due to policy set to incorrect,
> Kernel disables ASPM L1.2.

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.

RE: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Patel, Mayurkumar]

Hi Sinan,

>
>Hi Mayurkumar,
>
>On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
>> The patch seems to be working for ASPM L1 so far. I am seeing a problem now with your patches for L1.2 as following:
>>
>> ASPM L1.2 does not get enabled on the Upstream as well as downstream port due to following reasons.
>> In the case, I see now, if the EP is not connected while reboot of the Machine, Root Port does not configure
>> It's own L1.2 also from the BIOS. (I am not sure whether it's even advisable to enable L1.2 on root
>> Port when no downstream device is not connected to it as it may have an impact on CLKREQ# and device is connected
>> At later stage may have a problem with it)
>>
>
>I have two questions:
>
>1. if the endpoint is connected during boot and have ASPM L1.2 enabled during boot,
>do you see that L1.2 gets re-enabled following a hotplug remove and then insert.
>This is the goal of this patch. If yes, we achieved our goal. This is working on
>my platform but I do not have L1SS support on my platform.
>

Yes this works fine if the EP is connected during platform boot up and I could see the L1SS enabled.


>2. if you do not connect any endpoint during boot and insert the card, do you
>see any ASPM enabled at all with and without my patch using the default policy?
>I think this is the part you are describing above. I'll confirm this on my platform
>too. I think this one will require another patch/discussion unless I broke something.
>

Actually, without your patches, I see during boot if EP is connected then L1SS are enabled.
Also, if EP is connected afterwards, L1SS are enabled. But behavior is as previously, that
It gets disabled due to POLICY_DEFAULT bug and does not stay consistent following next power cycle of Endpoint.




>
>> Later when the Device get connected, BIOS configures L1.2 for Root port and EP but due to policy set to incorrect,
>> Kernel disables ASPM L1.2.
>
>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.
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

RE: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Patel, Mayurkumar]

Hi Sinan,

>
>Hi Mayurkumar,
>
>On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
>> The patch seems to be working for ASPM L1 so far. I am seeing a problem now with your patches for L1.2 as following:
>>
>> ASPM L1.2 does not get enabled on the Upstream as well as downstream port due to following reasons.
>> In the case, I see now, if the EP is not connected while reboot of the Machine, Root Port does not configure
>> It's own L1.2 also from the BIOS. (I am not sure whether it's even advisable to enable L1.2 on root
>> Port when no downstream device is not connected to it as it may have an impact on CLKREQ# and device is connected
>> At later stage may have a problem with it)
>>
>
>I have two questions:
>
>1. if the endpoint is connected during boot and have ASPM L1.2 enabled during boot,
>do you see that L1.2 gets re-enabled following a hotplug remove and then insert.
>This is the goal of this patch. If yes, we achieved our goal. This is working on
>my platform but I do not have L1SS support on my platform.
>

Yes this works fine if the EP is connected during platform boot up and I could see the L1SS enabled.


>2. if you do not connect any endpoint during boot and insert the card, do you
>see any ASPM enabled at all with and without my patch using the default policy?
>I think this is the part you are describing above. I'll confirm this on my platform
>too. I think this one will require another patch/discussion unless I broke something.
>

Actually, without your patches, I see during boot if EP is connected then L1SS are enabled.
Also, if EP is connected afterwards, L1SS are enabled. But behavior is as previously, that
It gets disabled due to POLICY_DEFAULT bug and does not stay consistent following next power cycle of Endpoint.




>
>> Later when the Device get connected, BIOS configures L1.2 for Root port and EP but due to policy set to incorrect,
>> Kernel disables ASPM L1.2.
>
>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.
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

[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Patel, Mayurkumar]

Hi Sinan,

>
>Hi Mayurkumar,
>
>On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
>> The patch seems to be working for ASPM L1 so far. I am seeing a problem now with your patches for L1.2 as following:
>>
>> ASPM L1.2 does not get enabled on the Upstream as well as downstream port due to following reasons.
>> In the case, I see now, if the EP is not connected while reboot of the Machine, Root Port does not configure
>> It's own L1.2 also from the BIOS. (I am not sure whether it's even advisable to enable L1.2 on root
>> Port when no downstream device is not connected to it as it may have an impact on CLKREQ# and device is connected
>> At later stage may have a problem with it)
>>
>
>I have two questions:
>
>1. if the endpoint is connected during boot and have ASPM L1.2 enabled during boot,
>do you see that L1.2 gets re-enabled following a hotplug remove and then insert.
>This is the goal of this patch. If yes, we achieved our goal. This is working on
>my platform but I do not have L1SS support on my platform.
>

Yes this works fine if the EP is connected during platform boot up and I could see the L1SS enabled.


>2. if you do not connect any endpoint during boot and insert the card, do you
>see any ASPM enabled at all with and without my patch using the default policy?
>I think this is the part you are describing above. I'll confirm this on my platform
>too. I think this one will require another patch/discussion unless I broke something.
>

Actually, without your patches, I see during boot if EP is connected then L1SS are enabled.
Also, if EP is connected afterwards, L1SS are enabled. But behavior is as previously, that
It gets disabled due to POLICY_DEFAULT bug and does not stay consistent following next power cycle of Endpoint.




>
>> Later when the Device get connected, BIOS configures L1.2 for Root port and EP but due to policy set to incorrect,
>> Kernel disables ASPM L1.2.
>
>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.
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

Re: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Rajat Jain]

Hello,

On Thu, Apr 6, 2017 at 8:19 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
>
> On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
> > Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
> > In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
> > In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
> > substates Configuration).
> >
> > @Bjorn:
> > I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
> > the spec.,
> >
> > The spec. says following and correct me If I am wrong or I misinterpret the spec. :
> >
> > 5.5.4. L1 PM Substates Configuration
> >
> > The Setting of any enable bit must be performed at the Downstream Port before the
> > corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
> > bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
> > before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.
> >

Thanks for bringing to attention. My understanding / interpretation of
"Downstream port" was the port pointing downwards (from the "Upstream"
component). E.g. when an EP connects to a hub port, PCIe text refers
to the hub port as the "downstream port". Similarly "upstream port" is
used for referring to a switch's port that "points" upwards towards
the root port. Thus, I interpreted the text to mean that I need to
enable it in the "downstream port" (in the root port / switch port)
followed by the "upstream port" (in the device).

It would have been great if the PCIe spec was as clear for L1
substates as it was for L1:
----------------------------
ASPM L1 must be enabled by software in the Upstream
component on a Link prior to enabling ASPM L1 in the
Downstream component on that Link.
-----------------------------
For ASPM L1, the spec clearly says "Upstream component" which can only
mean the switch's "downstream" port. I'm open to changing it if there
is consensus that my interpretation is wrong.

I'm actually not sure if I understood what is the problem that is
being seen with L1 PM substates. Can you please elaborate?


>
>
> Thanks for testing.
>
> commit a142f4d3e5c54db5e942fa6ee5f3dc0e8c83207b
> Author: Rajat Jain <rajatja@google.com>
> Date:   Mon Jan 2 22:34:15 2017 -0800
>
>     PCI/ASPM: Add comment about L1 substate latency
>
>     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 currently. I'd be glad to
>     take up any suggestions).
>
>     Signed-off-by: Rajat Jain <rajatja@google.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> I added Rajat for discussion on L1SS. I think this deserves its own discussion.

Thanks, The above commit specifically adds a comment to
pcie_aspm_check_latency(), because I wanted to leave a note
highlighting that potentially, we could add a more stringent check for
exit latency for L1SS. But that has nothing to do with how we are
configuring or enabling / disabling the L1 substates.

Thanks & Best Regards,

Rajat

> I'll look at the other part of your email and move things around a little bit
> less aggressively for the aspm_default.
>
>
> --
> 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 V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Rajat Jain]

Hello,

On Thu, Apr 6, 2017 at 8:19 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
>
> On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
> > Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
> > In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
> > In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
> > substates Configuration).
> >
> > @Bjorn:
> > I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
> > the spec.,
> >
> > The spec. says following and correct me If I am wrong or I misinterpret the spec. :
> >
> > 5.5.4. L1 PM Substates Configuration
> >
> > The Setting of any enable bit must be performed at the Downstream Port before the
> > corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
> > bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
> > before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.
> >

Thanks for bringing to attention. My understanding / interpretation of
"Downstream port" was the port pointing downwards (from the "Upstream"
component). E.g. when an EP connects to a hub port, PCIe text refers
to the hub port as the "downstream port". Similarly "upstream port" is
used for referring to a switch's port that "points" upwards towards
the root port. Thus, I interpreted the text to mean that I need to
enable it in the "downstream port" (in the root port / switch port)
followed by the "upstream port" (in the device).

It would have been great if the PCIe spec was as clear for L1
substates as it was for L1:
----------------------------
ASPM L1 must be enabled by software in the Upstream
component on a Link prior to enabling ASPM L1 in the
Downstream component on that Link.
-----------------------------
For ASPM L1, the spec clearly says "Upstream component" which can only
mean the switch's "downstream" port. I'm open to changing it if there
is consensus that my interpretation is wrong.

I'm actually not sure if I understood what is the problem that is
being seen with L1 PM substates. Can you please elaborate?


>
>
> Thanks for testing.
>
> commit a142f4d3e5c54db5e942fa6ee5f3dc0e8c83207b
> Author: Rajat Jain <rajatja@google.com>
> Date:   Mon Jan 2 22:34:15 2017 -0800
>
>     PCI/ASPM: Add comment about L1 substate latency
>
>     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 currently. I'd be glad to
>     take up any suggestions).
>
>     Signed-off-by: Rajat Jain <rajatja@google.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> I added Rajat for discussion on L1SS. I think this deserves its own discussion.

Thanks, The above commit specifically adds a comment to
pcie_aspm_check_latency(), because I wanted to leave a note
highlighting that potentially, we could add a more stringent check for
exit latency for L1SS. But that has nothing to do with how we are
configuring or enabling / disabling the L1 substates.

Thanks & Best Regards,

Rajat

> I'll look at the other part of your email and move things around a little bit
> less aggressively for the aspm_default.
>
>
> --
> 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

[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Rajat Jain]

Hello,

On Thu, Apr 6, 2017 at 8:19 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
>
> On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
> > Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
> > In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
> > In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
> > substates Configuration).
> >
> > @Bjorn:
> > I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
> > the spec.,
> >
> > The spec. says following and correct me If I am wrong or I misinterpret the spec. :
> >
> > 5.5.4. L1 PM Substates Configuration
> >
> > The Setting of any enable bit must be performed at the Downstream Port before the
> > corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
> > bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
> > before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.
> >

Thanks for bringing to attention. My understanding / interpretation of
"Downstream port" was the port pointing downwards (from the "Upstream"
component). E.g. when an EP connects to a hub port, PCIe text refers
to the hub port as the "downstream port". Similarly "upstream port" is
used for referring to a switch's port that "points" upwards towards
the root port. Thus, I interpreted the text to mean that I need to
enable it in the "downstream port" (in the root port / switch port)
followed by the "upstream port" (in the device).

It would have been great if the PCIe spec was as clear for L1
substates as it was for L1:
----------------------------
ASPM L1 must be enabled by software in the Upstream
component on a Link prior to enabling ASPM L1 in the
Downstream component on that Link.
-----------------------------
For ASPM L1, the spec clearly says "Upstream component" which can only
mean the switch's "downstream" port. I'm open to changing it if there
is consensus that my interpretation is wrong.

I'm actually not sure if I understood what is the problem that is
being seen with L1 PM substates. Can you please elaborate?


>
>
> Thanks for testing.
>
> commit a142f4d3e5c54db5e942fa6ee5f3dc0e8c83207b
> Author: Rajat Jain <rajatja@google.com>
> Date:   Mon Jan 2 22:34:15 2017 -0800
>
>     PCI/ASPM: Add comment about L1 substate latency
>
>     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 currently. I'd be glad to
>     take up any suggestions).
>
>     Signed-off-by: Rajat Jain <rajatja@google.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> I added Rajat for discussion on L1SS. I think this deserves its own discussion.

Thanks, The above commit specifically adds a comment to
pcie_aspm_check_latency(), because I wanted to leave a note
highlighting that potentially, we could add a more stringent check for
exit latency for L1SS. But that has nothing to do with how we are
configuring or enabling / disabling the L1 substates.

Thanks & Best Regards,

Rajat

> I'll look at the other part of your email and move things around a little bit
> less aggressively for the aspm_default.
>
>
> --
> 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 V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

Hi Rajat,

On 4/6/2017 1:10 PM, Rajat Jain wrote:
> Thanks, The above commit specifically adds a comment to
> pcie_aspm_check_latency(), because I wanted to leave a note
> highlighting that potentially, we could add a more stringent check for
> exit latency for L1SS. But that has nothing to do with how we are
> configuring or enabling / disabling the L1 substates.

I saw your name in two L1SS reviews. I wanted to pull you into the discussion
as an L1SS expert. I didn't want to imply that there was something wrong
with the review. Apologies for that.

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.

Re: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

Hi Rajat,

On 4/6/2017 1:10 PM, Rajat Jain wrote:
> Thanks, The above commit specifically adds a comment to
> pcie_aspm_check_latency(), because I wanted to leave a note
> highlighting that potentially, we could add a more stringent check for
> exit latency for L1SS. But that has nothing to do with how we are
> configuring or enabling / disabling the L1 substates.

I saw your name in two L1SS reviews. I wanted to pull you into the discussion
as an L1SS expert. I didn't want to imply that there was something wrong
with the review. Apologies for that.

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.

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

[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

Hi Rajat,

On 4/6/2017 1:10 PM, Rajat Jain wrote:
> Thanks, The above commit specifically adds a comment to
> pcie_aspm_check_latency(), because I wanted to leave a note
> highlighting that potentially, we could add a more stringent check for
> exit latency for L1SS. But that has nothing to do with how we are
> configuring or enabling / disabling the L1 substates.

I saw your name in two L1SS reviews. I wanted to pull you into the discussion
as an L1SS expert. I didn't want to imply that there was something wrong
with the review. Apologies for that.

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.

RE: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Patel, Mayurkumar]

Hi Rajat


>On Thu, Apr 6, 2017 at 8:19 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>
>> On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
>> > Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
>> > In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
>> > In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
>> > substates Configuration).
>> >
>> > @Bjorn:
>> > I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
>> > the spec.,
>> >
>> > The spec. says following and correct me If I am wrong or I misinterpret the spec. :
>> >
>> > 5.5.4. L1 PM Substates Configuration
>> >
>> > The Setting of any enable bit must be performed at the Downstream Port before the
>> > corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
>> > bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
>> > before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.
>> >
>
>Thanks for bringing to attention. My understanding / interpretation of
>"Downstream port" was the port pointing downwards (from the "Upstream"
>component). E.g. when an EP connects to a hub port, PCIe text refers
>to the hub port as the "downstream port". Similarly "upstream port" is
>used for referring to a switch's port that "points" upwards towards
>the root port. Thus, I interpreted the text to mean that I need to
>enable it in the "downstream port" (in the root port / switch port)
>followed by the "upstream port" (in the device).
>
>It would have been great if the PCIe spec was as clear for L1
>substates as it was for L1:
>----------------------------
>ASPM L1 must be enabled by software in the Upstream
>component on a Link prior to enabling ASPM L1 in the
>Downstream component on that Link.
>-----------------------------
>For ASPM L1, the spec clearly says "Upstream component" which can only
>mean the switch's "downstream" port. I'm open to changing it if there
>is consensus that my interpretation is wrong.

In fact, Your understanding seems to be correct. It was my mistake that I raised it without
actually, keeping in mind or understanding port/component difference which I didn't notice.
My sincere apologies for that.


>
>I'm actually not sure if I understood what is the problem that is
>being seen with L1 PM substates. Can you please elaborate?
>


The actual problem, what Sinan and me were seeing with ASPM L1 with POLICY_DEFAULT
as described in https://patchwork.kernel.org/patch/9548321/ 


Sinan worked out patches to resolve the issue but on my platform now I am starting to see
that ASPM L1SS gets impacted by these patches. Basically, with his patches, when Policy is set to default,
and if no PCIe EP is connected to Root Port during boot up, BIOS does not configure L1SS for it, on my
platform. So the link->aspm_default in aspm.c stores configuration without L1SS. When the EP gets
connected afterwards, due to link->aspm_default set to non L1SS configuration, even if BIOS
sets ASPM L1SS for Root Port and Endpoint both, it gets cleared in the aspm driver.

Without the patches Sinan has worked, it was a different issue as described above, and L1/L1SS were
getting always enabled by the BIOS for endpoint and RP, but kernel enabled/disabled them alternatively.

So basically, I am not currently 100% sure, what is the proper fix for this. Whether BIOS should
enable L1SS on Root Port no matter Endpoint is connected or not during boot?

Or I am not sure, whether could we should still update link->aspm_default partly for L1SS in the old patch way
https://patchwork.ozlabs.org/patch/736771/ 

Any feedback would be helpful then may be if required I can or if Sinan can work out the patches for L1SS also fine for me.


>
>>
>>
>> Thanks for testing.
>>
>> commit a142f4d3e5c54db5e942fa6ee5f3dc0e8c83207b
>> Author: Rajat Jain <rajatja@google.com>
>> Date:   Mon Jan 2 22:34:15 2017 -0800
>>
>>     PCI/ASPM: Add comment about L1 substate latency
>>
>>     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 currently. I'd be glad to
>>     take up any suggestions).
>>
>>     Signed-off-by: Rajat Jain <rajatja@google.com>
>>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> I added Rajat for discussion on L1SS. I think this deserves its own discussion.
>
>Thanks, The above commit specifically adds a comment to
>pcie_aspm_check_latency(), because I wanted to leave a note
>highlighting that potentially, we could add a more stringent check for
>exit latency for L1SS. But that has nothing to do with how we are
>configuring or enabling / disabling the L1 substates.
>
>Thanks & Best Regards,
>
>Rajat
>
>> I'll look at the other part of your email and move things around a little bit
>> less aggressively for the aspm_default.
>>
>>
>> --
>> 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.
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

RE: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Patel, Mayurkumar]

Hi Rajat


>On Thu, Apr 6, 2017 at 8:19 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>
>> On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
>> > Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
>> > In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
>> > In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
>> > substates Configuration).
>> >
>> > @Bjorn:
>> > I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
>> > the spec.,
>> >
>> > The spec. says following and correct me If I am wrong or I misinterpret the spec. :
>> >
>> > 5.5.4. L1 PM Substates Configuration
>> >
>> > The Setting of any enable bit must be performed at the Downstream Port before the
>> > corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
>> > bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
>> > before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.
>> >
>
>Thanks for bringing to attention. My understanding / interpretation of
>"Downstream port" was the port pointing downwards (from the "Upstream"
>component). E.g. when an EP connects to a hub port, PCIe text refers
>to the hub port as the "downstream port". Similarly "upstream port" is
>used for referring to a switch's port that "points" upwards towards
>the root port. Thus, I interpreted the text to mean that I need to
>enable it in the "downstream port" (in the root port / switch port)
>followed by the "upstream port" (in the device).
>
>It would have been great if the PCIe spec was as clear for L1
>substates as it was for L1:
>----------------------------
>ASPM L1 must be enabled by software in the Upstream
>component on a Link prior to enabling ASPM L1 in the
>Downstream component on that Link.
>-----------------------------
>For ASPM L1, the spec clearly says "Upstream component" which can only
>mean the switch's "downstream" port. I'm open to changing it if there
>is consensus that my interpretation is wrong.

In fact, Your understanding seems to be correct. It was my mistake that I raised it without
actually, keeping in mind or understanding port/component difference which I didn't notice.
My sincere apologies for that.


>
>I'm actually not sure if I understood what is the problem that is
>being seen with L1 PM substates. Can you please elaborate?
>


The actual problem, what Sinan and me were seeing with ASPM L1 with POLICY_DEFAULT
as described in https://patchwork.kernel.org/patch/9548321/ 


Sinan worked out patches to resolve the issue but on my platform now I am starting to see
that ASPM L1SS gets impacted by these patches. Basically, with his patches, when Policy is set to default,
and if no PCIe EP is connected to Root Port during boot up, BIOS does not configure L1SS for it, on my
platform. So the link->aspm_default in aspm.c stores configuration without L1SS. When the EP gets
connected afterwards, due to link->aspm_default set to non L1SS configuration, even if BIOS
sets ASPM L1SS for Root Port and Endpoint both, it gets cleared in the aspm driver.

Without the patches Sinan has worked, it was a different issue as described above, and L1/L1SS were
getting always enabled by the BIOS for endpoint and RP, but kernel enabled/disabled them alternatively.

So basically, I am not currently 100% sure, what is the proper fix for this. Whether BIOS should
enable L1SS on Root Port no matter Endpoint is connected or not during boot?

Or I am not sure, whether could we should still update link->aspm_default partly for L1SS in the old patch way
https://patchwork.ozlabs.org/patch/736771/ 

Any feedback would be helpful then may be if required I can or if Sinan can work out the patches for L1SS also fine for me.


>
>>
>>
>> Thanks for testing.
>>
>> commit a142f4d3e5c54db5e942fa6ee5f3dc0e8c83207b
>> Author: Rajat Jain <rajatja@google.com>
>> Date:   Mon Jan 2 22:34:15 2017 -0800
>>
>>     PCI/ASPM: Add comment about L1 substate latency
>>
>>     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 currently. I'd be glad to
>>     take up any suggestions).
>>
>>     Signed-off-by: Rajat Jain <rajatja@google.com>
>>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> I added Rajat for discussion on L1SS. I think this deserves its own discussion.
>
>Thanks, The above commit specifically adds a comment to
>pcie_aspm_check_latency(), because I wanted to leave a note
>highlighting that potentially, we could add a more stringent check for
>exit latency for L1SS. But that has nothing to do with how we are
>configuring or enabling / disabling the L1 substates.
>
>Thanks & Best Regards,
>
>Rajat
>
>> I'll look at the other part of your email and move things around a little bit
>> less aggressively for the aspm_default.
>>
>>
>> --
>> 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.
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

[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Patel, Mayurkumar]

Hi Rajat


>On Thu, Apr 6, 2017 at 8:19 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>
>> On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
>> > Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
>> > In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
>> > In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
>> > substates Configuration).
>> >
>> > @Bjorn:
>> > I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
>> > the spec.,
>> >
>> > The spec. says following and correct me If I am wrong or I misinterpret the spec. :
>> >
>> > 5.5.4. L1 PM Substates Configuration
>> >
>> > The Setting of any enable bit must be performed at the Downstream Port before the
>> > corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
>> > bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
>> > before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.
>> >
>
>Thanks for bringing to attention. My understanding / interpretation of
>"Downstream port" was the port pointing downwards (from the "Upstream"
>component). E.g. when an EP connects to a hub port, PCIe text refers
>to the hub port as the "downstream port". Similarly "upstream port" is
>used for referring to a switch's port that "points" upwards towards
>the root port. Thus, I interpreted the text to mean that I need to
>enable it in the "downstream port" (in the root port / switch port)
>followed by the "upstream port" (in the device).
>
>It would have been great if the PCIe spec was as clear for L1
>substates as it was for L1:
>----------------------------
>ASPM L1 must be enabled by software in the Upstream
>component on a Link prior to enabling ASPM L1 in the
>Downstream component on that Link.
>-----------------------------
>For ASPM L1, the spec clearly says "Upstream component" which can only
>mean the switch's "downstream" port. I'm open to changing it if there
>is consensus that my interpretation is wrong.

In fact, Your understanding seems to be correct. It was my mistake that I raised it without
actually, keeping in mind or understanding port/component difference which I didn't notice.
My sincere apologies for that.


>
>I'm actually not sure if I understood what is the problem that is
>being seen with L1 PM substates. Can you please elaborate?
>


The actual problem, what Sinan and me were seeing with ASPM L1 with POLICY_DEFAULT
as described in https://patchwork.kernel.org/patch/9548321/ 


Sinan worked out patches to resolve the issue but on my platform now I am starting to see
that ASPM L1SS gets impacted by these patches. Basically, with his patches, when Policy is set to default,
and if no PCIe EP is connected to Root Port during boot up, BIOS does not configure L1SS for it, on my
platform. So the link->aspm_default in aspm.c stores configuration without L1SS. When the EP gets
connected afterwards, due to link->aspm_default set to non L1SS configuration, even if BIOS
sets ASPM L1SS for Root Port and Endpoint both, it gets cleared in the aspm driver.

Without the patches Sinan has worked, it was a different issue as described above, and L1/L1SS were
getting always enabled by the BIOS for endpoint and RP, but kernel enabled/disabled them alternatively.

So basically, I am not currently 100% sure, what is the proper fix for this. Whether BIOS should
enable L1SS on Root Port no matter Endpoint is connected or not during boot?

Or I am not sure, whether could we should still update link->aspm_default partly for L1SS in the old patch way
https://patchwork.ozlabs.org/patch/736771/ 

Any feedback would be helpful then may be if required I can or if Sinan can work out the patches for L1SS also fine for me.


>
>>
>>
>> Thanks for testing.
>>
>> commit a142f4d3e5c54db5e942fa6ee5f3dc0e8c83207b
>> Author: Rajat Jain <rajatja@google.com>
>> Date:   Mon Jan 2 22:34:15 2017 -0800
>>
>>     PCI/ASPM: Add comment about L1 substate latency
>>
>>     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 currently. I'd be glad to
>>     take up any suggestions).
>>
>>     Signed-off-by: Rajat Jain <rajatja@google.com>
>>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> I added Rajat for discussion on L1SS. I think this deserves its own discussion.
>
>Thanks, The above commit specifically adds a comment to
>pcie_aspm_check_latency(), because I wanted to leave a note
>highlighting that potentially, we could add a more stringent check for
>exit latency for L1SS. But that has nothing to do with how we are
>configuring or enabling / disabling the L1 substates.
>
>Thanks & Best Regards,
>
>Rajat
>
>> I'll look at the other part of your email and move things around a little bit
>> less aggressively for the aspm_default.
>>
>>
>> --
>> 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.
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

Re: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

On 4/7/2017 5:03 AM, Patel, Mayurkumar wrote:
> So basically, I am not currently 100% sure, what is the proper fix for this. Whether BIOS should
> enable L1SS on Root Port no matter Endpoint is connected or not during boot?

I'd like to force the kernel to reconfigure ASPM for all combinations
when there is no card attached during boot and we do a hotplug insertion. 

I'm thinking about how to do this now.

-- 
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 V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

On 4/7/2017 5:03 AM, Patel, Mayurkumar wrote:
> So basically, I am not currently 100% sure, what is the proper fix for this. Whether BIOS should
> enable L1SS on Root Port no matter Endpoint is connected or not during boot?

I'd like to force the kernel to reconfigure ASPM for all combinations
when there is no card attached during boot and we do a hotplug insertion. 

I'm thinking about how to do this now.

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

[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

On 4/7/2017 5:03 AM, Patel, Mayurkumar wrote:
> So basically, I am not currently 100% sure, what is the proper fix for this. Whether BIOS should
> enable L1SS on Root Port no matter Endpoint is connected or not during boot?

I'd like to force the kernel to reconfigure ASPM for all combinations
when there is no card attached during boot and we do a hotplug insertion. 

I'm thinking about how to do this now.

-- 
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 V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Rajat Jain]

On Thu, Apr 6, 2017 at 5:34 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> Hi Rajat,
>
> On 4/6/2017 1:10 PM, Rajat Jain wrote:
>> Thanks, The above commit specifically adds a comment to
>> pcie_aspm_check_latency(), because I wanted to leave a note
>> highlighting that potentially, we could add a more stringent check for
>> exit latency for L1SS. But that has nothing to do with how we are
>> configuring or enabling / disabling the L1 substates.
>
> I saw your name in two L1SS reviews. I wanted to pull you into the discussion
> as an L1SS expert. I didn't want to imply that there was something wrong
> with the review. Apologies for that.

No worries, and actually thanks for pulling me in. :-)

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

[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Rajat Jain]

On Thu, Apr 6, 2017 at 5:34 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> Hi Rajat,
>
> On 4/6/2017 1:10 PM, Rajat Jain wrote:
>> Thanks, The above commit specifically adds a comment to
>> pcie_aspm_check_latency(), because I wanted to leave a note
>> highlighting that potentially, we could add a more stringent check for
>> exit latency for L1SS. But that has nothing to do with how we are
>> configuring or enabling / disabling the L1 substates.
>
> I saw your name in two L1SS reviews. I wanted to pull you into the discussion
> as an L1SS expert. I didn't want to imply that there was something wrong
> with the review. Apologies for that.

No worries, and actually thanks for pulling me in. :-)

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

Re: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Rajat Jain]

On Fri, Apr 7, 2017 at 2:03 AM, Patel, Mayurkumar
<mayurkumar.patel@intel.com> wrote:
> Hi Rajat
>
>
>>On Thu, Apr 6, 2017 at 8:19 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>>
>>> On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
>>> > Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
>>> > In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
>>> > In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
>>> > substates Configuration).
>>> >
>>> > @Bjorn:
>>> > I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
>>> > the spec.,
>>> >
>>> > The spec. says following and correct me If I am wrong or I misinterpret the spec. :
>>> >
>>> > 5.5.4. L1 PM Substates Configuration
>>> >
>>> > The Setting of any enable bit must be performed at the Downstream Port before the
>>> > corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
>>> > bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
>>> > before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.
>>> >
>>
>>Thanks for bringing to attention. My understanding / interpretation of
>>"Downstream port" was the port pointing downwards (from the "Upstream"
>>component). E.g. when an EP connects to a hub port, PCIe text refers
>>to the hub port as the "downstream port". Similarly "upstream port" is
>>used for referring to a switch's port that "points" upwards towards
>>the root port. Thus, I interpreted the text to mean that I need to
>>enable it in the "downstream port" (in the root port / switch port)
>>followed by the "upstream port" (in the device).
>>
>>It would have been great if the PCIe spec was as clear for L1
>>substates as it was for L1:
>>----------------------------
>>ASPM L1 must be enabled by software in the Upstream
>>component on a Link prior to enabling ASPM L1 in the
>>Downstream component on that Link.
>>-----------------------------
>>For ASPM L1, the spec clearly says "Upstream component" which can only
>>mean the switch's "downstream" port. I'm open to changing it if there
>>is consensus that my interpretation is wrong.
>
> In fact, Your understanding seems to be correct. It was my mistake that I raised it without
> actually, keeping in mind or understanding port/component difference which I didn't notice.
> My sincere apologies for that.

No worries, yes, it is tricky and even I had missed it when I
implemented it first. :-)

>
>>
>>I'm actually not sure if I understood what is the problem that is
>>being seen with L1 PM substates. Can you please elaborate?
>>
>
>
> The actual problem, what Sinan and me were seeing with ASPM L1 with POLICY_DEFAULT
> as described in https://patchwork.kernel.org/patch/9548321/

OK, I understood this problem that you are trying to solve. Lets call
this problem (1). Sorry, I haven't yet looked at your patches, and I
will take a look.

>
> Sinan worked out patches to resolve the issue but on my platform now I am starting to see
> that ASPM L1SS gets impacted by these patches.

OK, lets call this problem (2) - are you saying that this only impacts
L1SS and not L1? Sorry, just trying to understand because I can't
think of anyway L1 and L1SS bits are handled differently..

> Basically, with his patches, when Policy is set to default,
> and if no PCIe EP is connected to Root Port during boot up, BIOS does not configure L1SS for it, on my
> platform. So the link->aspm_default in aspm.c stores configuration without L1SS.

I understand uptil here:
No Device on boot, ASPM Policy=default => BIOS does not enable any
ASPM bits => kernel does the same.

> When the EP gets
> connected afterwards, due to link->aspm_default set to non L1SS configuration, even if BIOS
> sets ASPM L1SS for Root Port and Endpoint both,

Sorry, I don't think I followed this part. You are saying when the EP
gets hot-plugged later (after booting up with no EP), the BIOS sets
ASPM L1SS for root port & endpoint at that time??

> it gets cleared in the aspm driver.
>
> Without the patches Sinan has worked, it was a different issue as described above, and L1/L1SS were
> getting always enabled by the BIOS for endpoint and RP, but kernel enabled/disabled them alternatively.
>
> So basically, I am not currently 100% sure, what is the proper fix for this. Whether BIOS should
> enable L1SS on Root Port no matter Endpoint is connected or not during boot?

I'd think not.

While I don't understand the problem (2) currently, wouldn't you face
the same problem (2) with L1? Are you seeing L1 & L1SS bits handled
differently in your use case / problem case?

Thanks,

Rajat

>
> Or I am not sure, whether could we should still update link->aspm_default partly for L1SS in the old patch way
> https://patchwork.ozlabs.org/patch/736771/
>
> Any feedback would be helpful then may be if required I can or if Sinan can work out the patches for L1SS also fine for me.
>
>
>>
>>>
>>>
>>> Thanks for testing.
>>>
>>> commit a142f4d3e5c54db5e942fa6ee5f3dc0e8c83207b
>>> Author: Rajat Jain <rajatja@google.com>
>>> Date:   Mon Jan 2 22:34:15 2017 -0800
>>>
>>>     PCI/ASPM: Add comment about L1 substate latency
>>>
>>>     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 currently. I'd be glad to
>>>     take up any suggestions).
>>>
>>>     Signed-off-by: Rajat Jain <rajatja@google.com>
>>>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> I added Rajat for discussion on L1SS. I think this deserves its own discussion.
>>
>>Thanks, The above commit specifically adds a comment to
>>pcie_aspm_check_latency(), because I wanted to leave a note
>>highlighting that potentially, we could add a more stringent check for
>>exit latency for L1SS. But that has nothing to do with how we are
>>configuring or enabling / disabling the L1 substates.
>>
>>Thanks & Best Regards,
>>
>>Rajat
>>
>>> I'll look at the other part of your email and move things around a little bit
>>> less aggressively for the aspm_default.
>>>
>>>
>>> --
>>> 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.
> 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

[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Rajat Jain]

On Fri, Apr 7, 2017 at 2:03 AM, Patel, Mayurkumar
<mayurkumar.patel@intel.com> wrote:
> Hi Rajat
>
>
>>On Thu, Apr 6, 2017 at 8:19 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>>
>>> On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
>>> > Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
>>> > In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
>>> > In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
>>> > substates Configuration).
>>> >
>>> > @Bjorn:
>>> > I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
>>> > the spec.,
>>> >
>>> > The spec. says following and correct me If I am wrong or I misinterpret the spec. :
>>> >
>>> > 5.5.4. L1 PM Substates Configuration
>>> >
>>> > The Setting of any enable bit must be performed at the Downstream Port before the
>>> > corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
>>> > bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
>>> > before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.
>>> >
>>
>>Thanks for bringing to attention. My understanding / interpretation of
>>"Downstream port" was the port pointing downwards (from the "Upstream"
>>component). E.g. when an EP connects to a hub port, PCIe text refers
>>to the hub port as the "downstream port". Similarly "upstream port" is
>>used for referring to a switch's port that "points" upwards towards
>>the root port. Thus, I interpreted the text to mean that I need to
>>enable it in the "downstream port" (in the root port / switch port)
>>followed by the "upstream port" (in the device).
>>
>>It would have been great if the PCIe spec was as clear for L1
>>substates as it was for L1:
>>----------------------------
>>ASPM L1 must be enabled by software in the Upstream
>>component on a Link prior to enabling ASPM L1 in the
>>Downstream component on that Link.
>>-----------------------------
>>For ASPM L1, the spec clearly says "Upstream component" which can only
>>mean the switch's "downstream" port. I'm open to changing it if there
>>is consensus that my interpretation is wrong.
>
> In fact, Your understanding seems to be correct. It was my mistake that I raised it without
> actually, keeping in mind or understanding port/component difference which I didn't notice.
> My sincere apologies for that.

No worries, yes, it is tricky and even I had missed it when I
implemented it first. :-)

>
>>
>>I'm actually not sure if I understood what is the problem that is
>>being seen with L1 PM substates. Can you please elaborate?
>>
>
>
> The actual problem, what Sinan and me were seeing with ASPM L1 with POLICY_DEFAULT
> as described in https://patchwork.kernel.org/patch/9548321/

OK, I understood this problem that you are trying to solve. Lets call
this problem (1). Sorry, I haven't yet looked at your patches, and I
will take a look.

>
> Sinan worked out patches to resolve the issue but on my platform now I am starting to see
> that ASPM L1SS gets impacted by these patches.

OK, lets call this problem (2) - are you saying that this only impacts
L1SS and not L1? Sorry, just trying to understand because I can't
think of anyway L1 and L1SS bits are handled differently..

> Basically, with his patches, when Policy is set to default,
> and if no PCIe EP is connected to Root Port during boot up, BIOS does not configure L1SS for it, on my
> platform. So the link->aspm_default in aspm.c stores configuration without L1SS.

I understand uptil here:
No Device on boot, ASPM Policy=default => BIOS does not enable any
ASPM bits => kernel does the same.

> When the EP gets
> connected afterwards, due to link->aspm_default set to non L1SS configuration, even if BIOS
> sets ASPM L1SS for Root Port and Endpoint both,

Sorry, I don't think I followed this part. You are saying when the EP
gets hot-plugged later (after booting up with no EP), the BIOS sets
ASPM L1SS for root port & endpoint at that time??

> it gets cleared in the aspm driver.
>
> Without the patches Sinan has worked, it was a different issue as described above, and L1/L1SS were
> getting always enabled by the BIOS for endpoint and RP, but kernel enabled/disabled them alternatively.
>
> So basically, I am not currently 100% sure, what is the proper fix for this. Whether BIOS should
> enable L1SS on Root Port no matter Endpoint is connected or not during boot?

I'd think not.

While I don't understand the problem (2) currently, wouldn't you face
the same problem (2) with L1? Are you seeing L1 & L1SS bits handled
differently in your use case / problem case?

Thanks,

Rajat

>
> Or I am not sure, whether could we should still update link->aspm_default partly for L1SS in the old patch way
> https://patchwork.ozlabs.org/patch/736771/
>
> Any feedback would be helpful then may be if required I can or if Sinan can work out the patches for L1SS also fine for me.
>
>
>>
>>>
>>>
>>> Thanks for testing.
>>>
>>> commit a142f4d3e5c54db5e942fa6ee5f3dc0e8c83207b
>>> Author: Rajat Jain <rajatja@google.com>
>>> Date:   Mon Jan 2 22:34:15 2017 -0800
>>>
>>>     PCI/ASPM: Add comment about L1 substate latency
>>>
>>>     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 currently. I'd be glad to
>>>     take up any suggestions).
>>>
>>>     Signed-off-by: Rajat Jain <rajatja@google.com>
>>>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> I added Rajat for discussion on L1SS. I think this deserves its own discussion.
>>
>>Thanks, The above commit specifically adds a comment to
>>pcie_aspm_check_latency(), because I wanted to leave a note
>>highlighting that potentially, we could add a more stringent check for
>>exit latency for L1SS. But that has nothing to do with how we are
>>configuring or enabling / disabling the L1 substates.
>>
>>Thanks & Best Regards,
>>
>>Rajat
>>
>>> I'll look at the other part of your email and move things around a little bit
>>> less aggressively for the aspm_default.
>>>
>>>
>>> --
>>> 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.
> 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

Re: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

Hi Mayurkumar,

On 4/7/2017 9:09 AM, Sinan Kaya wrote:
> On 4/7/2017 5:03 AM, Patel, Mayurkumar wrote:
>> So basically, I am not currently 100% sure, what is the proper fix for this. Whether BIOS should
>> enable L1SS on Root Port no matter Endpoint is connected or not during boot?
> 
> I'd like to force the kernel to reconfigure ASPM for all combinations
> when there is no card attached during boot and we do a hotplug insertion. 
> 
> I'm thinking about how to do this now.
> 

I posted V8. can you please test it again?

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.

Re: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

Hi Mayurkumar,

On 4/7/2017 9:09 AM, Sinan Kaya wrote:
> On 4/7/2017 5:03 AM, Patel, Mayurkumar wrote:
>> So basically, I am not currently 100% sure, what is the proper fix for this. Whether BIOS should
>> enable L1SS on Root Port no matter Endpoint is connected or not during boot?
> 
> I'd like to force the kernel to reconfigure ASPM for all combinations
> when there is no card attached during boot and we do a hotplug insertion. 
> 
> I'm thinking about how to do this now.
> 

I posted V8. can you please test it again?

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.

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

[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Sinan Kaya]

Hi Mayurkumar,

On 4/7/2017 9:09 AM, Sinan Kaya wrote:
> On 4/7/2017 5:03 AM, Patel, Mayurkumar wrote:
>> So basically, I am not currently 100% sure, what is the proper fix for this. Whether BIOS should
>> enable L1SS on Root Port no matter Endpoint is connected or not during boot?
> 
> I'd like to force the kernel to reconfigure ASPM for all combinations
> when there is no card attached during boot and we do a hotplug insertion. 
> 
> I'm thinking about how to do this now.
> 

I posted V8. can you please test it again?

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.

RE: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Patel, Mayurkumar]

>> Hi Rajat
>>
>>
>>>On Thu, Apr 6, 2017 at 8:19 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>>>
>>>> On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
>>>> > Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
>>>> > In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
>>>> > In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
>>>> > substates Configuration).
>>>> >
>>>> > @Bjorn:
>>>> > I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
>>>> > the spec.,
>>>> >
>>>> > The spec. says following and correct me If I am wrong or I misinterpret the spec. :
>>>> >
>>>> > 5.5.4. L1 PM Substates Configuration
>>>> >
>>>> > The Setting of any enable bit must be performed at the Downstream Port before the
>>>> > corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
>>>> > bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
>>>> > before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.
>>>> >
>>>
>>>Thanks for bringing to attention. My understanding / interpretation of
>>>"Downstream port" was the port pointing downwards (from the "Upstream"
>>>component). E.g. when an EP connects to a hub port, PCIe text refers
>>>to the hub port as the "downstream port". Similarly "upstream port" is
>>>used for referring to a switch's port that "points" upwards towards
>>>the root port. Thus, I interpreted the text to mean that I need to
>>>enable it in the "downstream port" (in the root port / switch port)
>>>followed by the "upstream port" (in the device).
>>>
>>>It would have been great if the PCIe spec was as clear for L1
>>>substates as it was for L1:
>>>----------------------------
>>>ASPM L1 must be enabled by software in the Upstream
>>>component on a Link prior to enabling ASPM L1 in the
>>>Downstream component on that Link.
>>>-----------------------------
>>>For ASPM L1, the spec clearly says "Upstream component" which can only
>>>mean the switch's "downstream" port. I'm open to changing it if there
>>>is consensus that my interpretation is wrong.
>>
>> In fact, Your understanding seems to be correct. It was my mistake that I raised it without
>> actually, keeping in mind or understanding port/component difference which I didn't notice.
>> My sincere apologies for that.
>
>No worries, yes, it is tricky and even I had missed it when I
>implemented it first. :-)
>
>>
>>>
>>>I'm actually not sure if I understood what is the problem that is
>>>being seen with L1 PM substates. Can you please elaborate?
>>>
>>
>>
>> The actual problem, what Sinan and me were seeing with ASPM L1 with POLICY_DEFAULT
>> as described in https://patchwork.kernel.org/patch/9548321/
>
>OK, I understood this problem that you are trying to solve. Lets call
>this problem (1). Sorry, I haven't yet looked at your patches, and I
>will take a look.
>
>>
>> Sinan worked out patches to resolve the issue but on my platform now I am starting to see
>> that ASPM L1SS gets impacted by these patches.
>
>OK, lets call this problem (2) - are you saying that this only impacts
>L1SS and not L1? Sorry, just trying to understand because I can't
>think of anyway L1 and L1SS bits are handled differently..
>

Actually, in my platform, BIOS configures ASPM L1 if no EP found during boot on Root Port but
L1SS does not get enabled during boot on Root Port if no device found during boot. So it's bit different.


>> Basically, with his patches, when Policy is set to default,
>> and if no PCIe EP is connected to Root Port during boot up, BIOS does not configure L1SS for it, on my
>> platform. So the link->aspm_default in aspm.c stores configuration without L1SS.
>
>I understand uptil here:
>No Device on boot, ASPM Policy=default => BIOS does not enable any
>ASPM bits => kernel does the same.
>
>> When the EP gets
>> connected afterwards, due to link->aspm_default set to non L1SS configuration, even if BIOS
>> sets ASPM L1SS for Root Port and Endpoint both,
>
>Sorry, I don't think I followed this part. You are saying when the EP
>gets hot-plugged later (after booting up with no EP), the BIOS sets
>ASPM L1SS for root port & endpoint at that time??
>

Yes when EP gets connected at any time(i.e. during boot or after boot), BIOS sets L1 and L1SS both for Root Port and Endpoint.

>> it gets cleared in the aspm driver.
>>
>> Without the patches Sinan has worked, it was a different issue as described above, and L1/L1SS were
>> getting always enabled by the BIOS for endpoint and RP, but kernel enabled/disabled them alternatively.
>>
>> So basically, I am not currently 100% sure, what is the proper fix for this. Whether BIOS should
>> enable L1SS on Root Port no matter Endpoint is connected or not during boot?
>
>I'd think not.
>
>While I don't understand the problem (2) currently, wouldn't you face
>the same problem (2) with L1? Are you seeing L1 & L1SS bits handled
>differently in your use case / problem case?
>

Yes it is indeed, only during boot without Endpoint connected (L1 is set on Root port but L1SS is not).

>Thanks,
>
>Rajat
>
>>
>> Or I am not sure, whether could we should still update link->aspm_default partly for L1SS in the old patch way
>> https://patchwork.ozlabs.org/patch/736771/
>>
>> Any feedback would be helpful then may be if required I can or if Sinan can work out the patches for L1SS also fine for me.
>>
>>
>>>
>>>>
>>>>
>>>> Thanks for testing.
>>>>
>>>> commit a142f4d3e5c54db5e942fa6ee5f3dc0e8c83207b
>>>> Author: Rajat Jain <rajatja@google.com>
>>>> Date:   Mon Jan 2 22:34:15 2017 -0800
>>>>
>>>>     PCI/ASPM: Add comment about L1 substate latency
>>>>
>>>>     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 currently. I'd be glad to
>>>>     take up any suggestions).
>>>>
>>>>     Signed-off-by: Rajat Jain <rajatja@google.com>
>>>>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>>
>>>> I added Rajat for discussion on L1SS. I think this deserves its own discussion.
>>>
>>>Thanks, The above commit specifically adds a comment to
>>>pcie_aspm_check_latency(), because I wanted to leave a note
>>>highlighting that potentially, we could add a more stringent check for
>>>exit latency for L1SS. But that has nothing to do with how we are
>>>configuring or enabling / disabling the L1 substates.
>>>
>>>Thanks & Best Regards,
>>>
>>>Rajat
>>>
>>>> I'll look at the other part of your email and move things around a little bit
>>>> less aggressively for the aspm_default.
>>>>
>>>>
>>>> --
>>>> 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.
>> 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
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

RE: [PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Patel, Mayurkumar]

>> Hi Rajat
>>
>>
>>>On Thu, Apr 6, 2017 at 8:19 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>>>
>>>> On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
>>>> > Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
>>>> > In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
>>>> > In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
>>>> > substates Configuration).
>>>> >
>>>> > @Bjorn:
>>>> > I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
>>>> > the spec.,
>>>> >
>>>> > The spec. says following and correct me If I am wrong or I misinterpret the spec. :
>>>> >
>>>> > 5.5.4. L1 PM Substates Configuration
>>>> >
>>>> > The Setting of any enable bit must be performed at the Downstream Port before the
>>>> > corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
>>>> > bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
>>>> > before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.
>>>> >
>>>
>>>Thanks for bringing to attention. My understanding / interpretation of
>>>"Downstream port" was the port pointing downwards (from the "Upstream"
>>>component). E.g. when an EP connects to a hub port, PCIe text refers
>>>to the hub port as the "downstream port". Similarly "upstream port" is
>>>used for referring to a switch's port that "points" upwards towards
>>>the root port. Thus, I interpreted the text to mean that I need to
>>>enable it in the "downstream port" (in the root port / switch port)
>>>followed by the "upstream port" (in the device).
>>>
>>>It would have been great if the PCIe spec was as clear for L1
>>>substates as it was for L1:
>>>----------------------------
>>>ASPM L1 must be enabled by software in the Upstream
>>>component on a Link prior to enabling ASPM L1 in the
>>>Downstream component on that Link.
>>>-----------------------------
>>>For ASPM L1, the spec clearly says "Upstream component" which can only
>>>mean the switch's "downstream" port. I'm open to changing it if there
>>>is consensus that my interpretation is wrong.
>>
>> In fact, Your understanding seems to be correct. It was my mistake that I raised it without
>> actually, keeping in mind or understanding port/component difference which I didn't notice.
>> My sincere apologies for that.
>
>No worries, yes, it is tricky and even I had missed it when I
>implemented it first. :-)
>
>>
>>>
>>>I'm actually not sure if I understood what is the problem that is
>>>being seen with L1 PM substates. Can you please elaborate?
>>>
>>
>>
>> The actual problem, what Sinan and me were seeing with ASPM L1 with POLICY_DEFAULT
>> as described in https://patchwork.kernel.org/patch/9548321/
>
>OK, I understood this problem that you are trying to solve. Lets call
>this problem (1). Sorry, I haven't yet looked at your patches, and I
>will take a look.
>
>>
>> Sinan worked out patches to resolve the issue but on my platform now I am starting to see
>> that ASPM L1SS gets impacted by these patches.
>
>OK, lets call this problem (2) - are you saying that this only impacts
>L1SS and not L1? Sorry, just trying to understand because I can't
>think of anyway L1 and L1SS bits are handled differently..
>

Actually, in my platform, BIOS configures ASPM L1 if no EP found during boot on Root Port but
L1SS does not get enabled during boot on Root Port if no device found during boot. So it's bit different.


>> Basically, with his patches, when Policy is set to default,
>> and if no PCIe EP is connected to Root Port during boot up, BIOS does not configure L1SS for it, on my
>> platform. So the link->aspm_default in aspm.c stores configuration without L1SS.
>
>I understand uptil here:
>No Device on boot, ASPM Policy=default => BIOS does not enable any
>ASPM bits => kernel does the same.
>
>> When the EP gets
>> connected afterwards, due to link->aspm_default set to non L1SS configuration, even if BIOS
>> sets ASPM L1SS for Root Port and Endpoint both,
>
>Sorry, I don't think I followed this part. You are saying when the EP
>gets hot-plugged later (after booting up with no EP), the BIOS sets
>ASPM L1SS for root port & endpoint at that time??
>

Yes when EP gets connected at any time(i.e. during boot or after boot), BIOS sets L1 and L1SS both for Root Port and Endpoint.

>> it gets cleared in the aspm driver.
>>
>> Without the patches Sinan has worked, it was a different issue as described above, and L1/L1SS were
>> getting always enabled by the BIOS for endpoint and RP, but kernel enabled/disabled them alternatively.
>>
>> So basically, I am not currently 100% sure, what is the proper fix for this. Whether BIOS should
>> enable L1SS on Root Port no matter Endpoint is connected or not during boot?
>
>I'd think not.
>
>While I don't understand the problem (2) currently, wouldn't you face
>the same problem (2) with L1? Are you seeing L1 & L1SS bits handled
>differently in your use case / problem case?
>

Yes it is indeed, only during boot without Endpoint connected (L1 is set on Root port but L1SS is not).

>Thanks,
>
>Rajat
>
>>
>> Or I am not sure, whether could we should still update link->aspm_default partly for L1SS in the old patch way
>> https://patchwork.ozlabs.org/patch/736771/
>>
>> Any feedback would be helpful then may be if required I can or if Sinan can work out the patches for L1SS also fine for me.
>>
>>
>>>
>>>>
>>>>
>>>> Thanks for testing.
>>>>
>>>> commit a142f4d3e5c54db5e942fa6ee5f3dc0e8c83207b
>>>> Author: Rajat Jain <rajatja@google.com>
>>>> Date:   Mon Jan 2 22:34:15 2017 -0800
>>>>
>>>>     PCI/ASPM: Add comment about L1 substate latency
>>>>
>>>>     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 currently. I'd be glad to
>>>>     take up any suggestions).
>>>>
>>>>     Signed-off-by: Rajat Jain <rajatja@google.com>
>>>>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>>
>>>> I added Rajat for discussion on L1SS. I think this deserves its own discussion.
>>>
>>>Thanks, The above commit specifically adds a comment to
>>>pcie_aspm_check_latency(), because I wanted to leave a note
>>>highlighting that potentially, we could add a more stringent check for
>>>exit latency for L1SS. But that has nothing to do with how we are
>>>configuring or enabling / disabling the L1 substates.
>>>
>>>Thanks & Best Regards,
>>>
>>>Rajat
>>>
>>>> I'll look at the other part of your email and move things around a little bit
>>>> less aggressively for the aspm_default.
>>>>
>>>>
>>>> --
>>>> 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.
>> 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
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

[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Patel, Mayurkumar]

>> Hi Rajat
>>
>>
>>>On Thu, Apr 6, 2017 at 8:19 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>>>
>>>> On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
>>>> > Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
>>>> > In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
>>>> > In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
>>>> > substates Configuration).
>>>> >
>>>> > @Bjorn:
>>>> > I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
>>>> > the spec.,
>>>> >
>>>> > The spec. says following and correct me If I am wrong or I misinterpret the spec. :
>>>> >
>>>> > 5.5.4. L1 PM Substates Configuration
>>>> >
>>>> > The Setting of any enable bit must be performed at the Downstream Port before the
>>>> > corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
>>>> > bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
>>>> > before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.
>>>> >
>>>
>>>Thanks for bringing to attention. My understanding / interpretation of
>>>"Downstream port" was the port pointing downwards (from the "Upstream"
>>>component). E.g. when an EP connects to a hub port, PCIe text refers
>>>to the hub port as the "downstream port". Similarly "upstream port" is
>>>used for referring to a switch's port that "points" upwards towards
>>>the root port. Thus, I interpreted the text to mean that I need to
>>>enable it in the "downstream port" (in the root port / switch port)
>>>followed by the "upstream port" (in the device).
>>>
>>>It would have been great if the PCIe spec was as clear for L1
>>>substates as it was for L1:
>>>----------------------------
>>>ASPM L1 must be enabled by software in the Upstream
>>>component on a Link prior to enabling ASPM L1 in the
>>>Downstream component on that Link.
>>>-----------------------------
>>>For ASPM L1, the spec clearly says "Upstream component" which can only
>>>mean the switch's "downstream" port. I'm open to changing it if there
>>>is consensus that my interpretation is wrong.
>>
>> In fact, Your understanding seems to be correct. It was my mistake that I raised it without
>> actually, keeping in mind or understanding port/component difference which I didn't notice.
>> My sincere apologies for that.
>
>No worries, yes, it is tricky and even I had missed it when I
>implemented it first. :-)
>
>>
>>>
>>>I'm actually not sure if I understood what is the problem that is
>>>being seen with L1 PM substates. Can you please elaborate?
>>>
>>
>>
>> The actual problem, what Sinan and me were seeing with ASPM L1 with POLICY_DEFAULT
>> as described in https://patchwork.kernel.org/patch/9548321/
>
>OK, I understood this problem that you are trying to solve. Lets call
>this problem (1). Sorry, I haven't yet looked at your patches, and I
>will take a look.
>
>>
>> Sinan worked out patches to resolve the issue but on my platform now I am starting to see
>> that ASPM L1SS gets impacted by these patches.
>
>OK, lets call this problem (2) - are you saying that this only impacts
>L1SS and not L1? Sorry, just trying to understand because I can't
>think of anyway L1 and L1SS bits are handled differently..
>

Actually, in my platform, BIOS configures ASPM L1 if no EP found during boot on Root Port but
L1SS does not get enabled during boot on Root Port if no device found during boot. So it's bit different.


>> Basically, with his patches, when Policy is set to default,
>> and if no PCIe EP is connected to Root Port during boot up, BIOS does not configure L1SS for it, on my
>> platform. So the link->aspm_default in aspm.c stores configuration without L1SS.
>
>I understand uptil here:
>No Device on boot, ASPM Policy=default => BIOS does not enable any
>ASPM bits => kernel does the same.
>
>> When the EP gets
>> connected afterwards, due to link->aspm_default set to non L1SS configuration, even if BIOS
>> sets ASPM L1SS for Root Port and Endpoint both,
>
>Sorry, I don't think I followed this part. You are saying when the EP
>gets hot-plugged later (after booting up with no EP), the BIOS sets
>ASPM L1SS for root port & endpoint at that time??
>

Yes when EP gets connected at any time(i.e. during boot or after boot), BIOS sets L1 and L1SS both for Root Port and Endpoint.

>> it gets cleared in the aspm driver.
>>
>> Without the patches Sinan has worked, it was a different issue as described above, and L1/L1SS were
>> getting always enabled by the BIOS for endpoint and RP, but kernel enabled/disabled them alternatively.
>>
>> So basically, I am not currently 100% sure, what is the proper fix for this. Whether BIOS should
>> enable L1SS on Root Port no matter Endpoint is connected or not during boot?
>
>I'd think not.
>
>While I don't understand the problem (2) currently, wouldn't you face
>the same problem (2) with L1? Are you seeing L1 & L1SS bits handled
>differently in your use case / problem case?
>

Yes it is indeed, only during boot without Endpoint connected (L1 is set on Root port but L1SS is not).

>Thanks,
>
>Rajat
>
>>
>> Or I am not sure, whether could we should still update link->aspm_default partly for L1SS in the old patch way
>> https://patchwork.ozlabs.org/patch/736771/
>>
>> Any feedback would be helpful then may be if required I can or if Sinan can work out the patches for L1SS also fine for me.
>>
>>
>>>
>>>>
>>>>
>>>> Thanks for testing.
>>>>
>>>> commit a142f4d3e5c54db5e942fa6ee5f3dc0e8c83207b
>>>> Author: Rajat Jain <rajatja@google.com>
>>>> Date:   Mon Jan 2 22:34:15 2017 -0800
>>>>
>>>>     PCI/ASPM: Add comment about L1 substate latency
>>>>
>>>>     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 currently. I'd be glad to
>>>>     take up any suggestions).
>>>>
>>>>     Signed-off-by: Rajat Jain <rajatja@google.com>
>>>>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>>
>>>> I added Rajat for discussion on L1SS. I think this deserves its own discussion.
>>>
>>>Thanks, The above commit specifically adds a comment to
>>>pcie_aspm_check_latency(), because I wanted to leave a note
>>>highlighting that potentially, we could add a more stringent check for
>>>exit latency for L1SS. But that has nothing to do with how we are
>>>configuring or enabling / disabling the L1 substates.
>>>
>>>Thanks & Best Regards,
>>>
>>>Rajat
>>>
>>>> I'll look at the other part of your email and move things around a little bit
>>>> less aggressively for the aspm_default.
>>>>
>>>>
>>>> --
>>>> 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.
>> 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
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