[PATCH v1 0/2] PCI/ASPM: Refine L1 PM Substates support

[PATCH v1 0/2] PCI/ASPM: Refine L1 PM Substates support

[Bjorn Helgaas]

The PCIe active link power state is L0.  ASPM defines two low-power
states: L0s and L1.  The L1 PM Substates feature defines two
additional low-power states: L1.1 and L2.2.

The L1.2 state may have substantial entry/exit latency.  Downstream
devices can use the Latency Tolerance Reporting (LTR) feature to
report how much latency they can tolerate.  The L1 PM Substates
capability includes a programmable L1.2 threshold register.  If the
downstream devices can tolerate the latency indicated by the
threshold, L1.2 may be used.

If LTR is not enabled, the L1.2 threshold is ignored and L1.2 can be
used whenever it is enabled and CLKREQ# is de-asserted.  Linux
currently never enables LTR, but firmware may have done so.

The current L1 PM substates support in Linux sets the L1.2 threshold
to a fixed value (163.84us) copied from coreboot [1].  I don't know
where that value came from, and it is incorrect for some platforms,
e.g., Tegra [2].

These patches change that so we calculate the L1.2 threshold based on
values reported by the hardware, and we enable LTR whenever possible.

This is all just my understanding from reading the spec.  I'd be glad
to be corrected if I'm going wrong.

I'm particularly puzzled about the coreboot code in
pciexp_L1_substate_commit() that sets LTR_L1.2_THRESHOLD:

+	pcie_update_cfg(root, root_cap + 0x08, ~0xe3ff0000,
+		(1 << 21) | (1 << 23) | (1 << 30));

This is writing the L1 PM Substates Control 1 register, but the shifts
don't match up with any of the fields in the register, so this is an
awfully funny way to write the threshold.

[1] https://mail.coreboot.org/pipermail/coreboot-gerrit/2015-March/021134.html
[2] https://lkml.kernel.org/r/1510492674-12786-1-git-send-email-vidyas@nvidia.com

---

Bjorn Helgaas (2):
      PCI/ASPM: Calculate LTR_L1.2_THRESHOLD from device characteristics
      PCI/ASPM: Enable Latency Tolerance Reporting when supported


 drivers/pci/pcie/aspm.c |   71 +++++++++++++++++++++++++++++++----------------
 drivers/pci/probe.c     |   33 ++++++++++++++++++++++
 include/linux/pci.h     |    2 +
 3 files changed, 82 insertions(+), 24 deletions(-)

[PATCH v1 1/2] PCI/ASPM: Calculate LTR_L1.2_THRESHOLD from device characteristics

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Per PCIe r3.1, sec 5.5.1, LTR_L1.2_THRESHOLD determines whether we enter
the L1.2 Link state: if L1.2 is enabled and downstream devices have
reported that they can tolerate latency of at least LTR_L1.2_THRESHOLD, we
must enter L1.2 when CLKREQ# is de-asserted.

The implication is that LTR_L1.2_THRESHOLD is the time required to
transition the Link from L0 to L1.2 and back to L0, and per sec 5.5.3.3.1,
Figures 5-16 and 5-17, it appears that the absolute minimum time for those
transitions would be T(POWER_OFF) + T(L1.2) + T(POWER_ON) + T(COMMONMODE).

Therefore, compute LTR_L1.2_THRESHOLD as:

    2us T(POWER_OFF)
  + 4us T(L1.2)
  + T(POWER_ON)
  + T(COMMONMODE)
  = LTR_L1.2_THRESHOLD

Previously we set LTR_L1.2_THRESHOLD to a fixed value of 163840ns
(163.84us):

  #define LTR_L1_2_THRESHOLD_BITS     ((1 << 21) | (1 << 23) | (1 << 30))
  ((1 << 21) | (1 << 23) | (1 << 30)) = 0x40a00000
  LTR_L1.2_THRESHOLD_Value = (0x40a00000 & 0x03ff0000) >> 16 = 0xa0 = 160
  LTR_L1.2_THRESHOLD_Scale = (0x40a00000 & 0xe0000000) >> 29 = 0x2 (* 1024ns)
  LTR_L1.2_THRESHOLD = 160 * 1024ns = 163840ns

Obviously this doesn't account for the circuit characteristics of different
implementations.

Note that while firmware may enable LTR, Linux itself currently does not
enable LTR.  When L1.2 is enabled but LTR is not, LTR_L1.2_THRESHOLD is
ignored and we always enter L1.2 when it is enabled and CLKREQ# is
de-asserted.  So this patch should not have any effect unless firmware
enables LTR.

Fixes: f1f0366dd6be ("PCI/ASPM: Calculate and save the L1.2 timing parameters")
Link: https://www.coreboot.org/pipermail/coreboot-gerrit/2015-March/021134.html
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Kenji Chen <kenji.chen@intel.com>
Cc: Patrick Georgi <pgeorgi@google.com>
Cc: Rajat Jain <rajatja@google.com>
---
 drivers/pci/pcie/aspm.c |   71 +++++++++++++++++++++++++++++++----------------
 1 file changed, 47 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 9783e10da3a9..3b9b4d50cd98 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -43,18 +43,6 @@
 #define ASPM_STATE_ALL		(ASPM_STATE_L0S | ASPM_STATE_L1 |	\
 				 ASPM_STATE_L1SS)
 
-/*
- * When L1 substates are enabled, the LTR L1.2 threshold is a timing parameter
- * that decides whether L1.1 or L1.2 is entered (Refer PCIe spec for details).
- * Not sure is there is a way to "calculate" this on the fly, but maybe we
- * could turn it into a parameter in future.  This value has been taken from
- * the following files from Intel's coreboot (which is the only code I found
- * to have used this):
- * https://www.coreboot.org/pipermail/coreboot-gerrit/2015-March/021134.html
- * https://review.coreboot.org/#/c/8832/
- */
-#define LTR_L1_2_THRESHOLD_BITS	((1 << 21) | (1 << 23) | (1 << 30))
-
 struct aspm_latency {
 	u32 l0s;			/* L0s latency (nsec) */
 	u32 l1;				/* L1 latency (nsec) */
@@ -333,6 +321,32 @@ static u32 calc_l1ss_pwron(struct pci_dev *pdev, u32 scale, u32 val)
 	return 0;
 }
 
+static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
+{
+	u64 threshold_ns = threshold_us * 1000;
+
+	/* See PCIe r3.1, sec 7.33.3 and sec 6.18 */
+	if (threshold_ns < 32) {
+		*scale = 0;
+		*value = threshold_ns;
+	} else if (threshold_ns < 1024) {
+		*scale = 1;
+		*value = threshold_ns >> 5;
+	} else if (threshold_ns < 32768) {
+		*scale = 2;
+		*value = threshold_ns >> 10;
+	} else if (threshold_ns < 1048576) {
+		*scale = 3;
+		*value = threshold_ns >> 15;
+	} else if (threshold_ns < 33554432) {
+		*scale = 4;
+		*value = threshold_ns >> 20;
+	} else {
+		*scale = 5;
+		*value = threshold_ns >> 25;
+	}
+}
+
 struct aspm_register_info {
 	u32 support:2;
 	u32 enabled:2;
@@ -443,6 +457,7 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 				struct aspm_register_info *dwreg)
 {
 	u32 val1, val2, scale1, scale2;
+	u32 t_common_mode, t_power_on, l1_2_threshold, scale, value;
 
 	link->l1ss.up_cap_ptr = upreg->l1ss_cap_ptr;
 	link->l1ss.dw_cap_ptr = dwreg->l1ss_cap_ptr;
@@ -454,16 +469,7 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 	/* Choose the greater of the two Port Common_Mode_Restore_Times */
 	val1 = (upreg->l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
 	val2 = (dwreg->l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
-	if (val1 > val2)
-		link->l1ss.ctl1 |= val1 << 8;
-	else
-		link->l1ss.ctl1 |= val2 << 8;
-
-	/*
-	 * We currently use LTR L1.2 threshold to be fixed constant picked from
-	 * Intel's coreboot.
-	 */
-	link->l1ss.ctl1 |= LTR_L1_2_THRESHOLD_BITS;
+	t_common_mode = max(val1, val2);
 
 	/* Choose the greater of the two Port T_POWER_ON times */
 	val1   = (upreg->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
@@ -472,10 +478,27 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 	scale2 = (dwreg->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
 
 	if (calc_l1ss_pwron(link->pdev, scale1, val1) >
-	    calc_l1ss_pwron(link->downstream, scale2, val2))
+	    calc_l1ss_pwron(link->downstream, scale2, val2)) {
 		link->l1ss.ctl2 |= scale1 | (val1 << 3);
-	else
+		t_power_on = calc_l1ss_pwron(link->pdev, scale1, val1);
+	} else {
 		link->l1ss.ctl2 |= scale2 | (val2 << 3);
+		t_power_on = calc_l1ss_pwron(link->downstream, scale2, val2);
+	}
+
+	/*
+	 * Set LTR_L1.2_THRESHOLD to the time required to transition the
+	 * Link from L0 to L1.2 and back to L0 so we enter L1.2 only if
+	 * downstream devices report (via LTR) that they can tolerate at
+	 * least that much latency.
+	 *
+	 * Based on PCIe r3.1, sec 5.5.3.3.1, Figures 5-16 and 5-17, and
+	 * Table 5-11.  T(POWER_OFF) is at most 2us and T(L1.2) is at
+	 * least 4us.
+	 */
+	l1_2_threshold = 2 + 4 + t_common_mode + t_power_on;
+	encode_l12_threshold(l1_2_threshold, &scale, &value);
+	link->l1ss.ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
 }
 
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)

[PATCH v1 2/2] PCI/ASPM: Enable Latency Tolerance Reporting when supported

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Enable Latency Tolerance Reporting (LTR).  Note that LTR must be enabled in
the Root Port first, and must not be enabled in any downstream device
unless the Root Port and all intermediate Switches also support LTR.
See PCIe r3.1, sec 6.18.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/probe.c |   33 +++++++++++++++++++++++++++++++++
 include/linux/pci.h |    2 ++
 2 files changed, 35 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 14e0ea1ff38b..3761b1303529 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1875,6 +1875,38 @@ static void pci_configure_relaxed_ordering(struct pci_dev *dev)
 	}
 }
 
+static void pci_configure_ltr(struct pci_dev *dev)
+{
+#ifdef CONFIG_PCIEASPM
+	u32 cap;
+	struct pci_dev *bridge;
+
+	if (!pci_is_pcie(dev))
+		return;
+
+	pcie_capability_read_dword(dev, PCI_EXP_DEVCAP2, &cap);
+	if (!(cap & PCI_EXP_DEVCAP2_LTR))
+		return;
+
+	/*
+	 * Software must not enable LTR in an Endpoint unless the Root
+	 * Complex and all intermediate Switches indicate support for LTR.
+	 * PCIe r3.1, sec 6.18.
+	 */
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
+		dev->ltr_path = 1;
+	else {
+		bridge = pci_upstream_bridge(dev);
+		if (bridge && bridge->ltr_path)
+			dev->ltr_path = 1;
+	}
+
+	if (dev->ltr_path)
+		pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
+					 PCI_EXP_DEVCTL2_LTR_EN);
+#endif
+}
+
 static void pci_configure_device(struct pci_dev *dev)
 {
 	struct hotplug_params hpp;
@@ -1883,6 +1915,7 @@ static void pci_configure_device(struct pci_dev *dev)
 	pci_configure_mps(dev);
 	pci_configure_extended_tags(dev, NULL);
 	pci_configure_relaxed_ordering(dev);
+	pci_configure_ltr(dev);
 
 	memset(&hpp, 0, sizeof(hpp));
 	ret = pci_get_hp_params(dev, &hpp);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0403894147a3..d4e176701b48 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -350,6 +350,8 @@ struct pci_dev {
 
 #ifdef CONFIG_PCIEASPM
 	struct pcie_link_state	*link_state;	/* ASPM link state */
+	unsigned int	ltr_path:1;	/* Latency Tolerance Reporting
+					   supported from root to here */
 #endif
 
 	pci_channel_state_t error_state;	/* current connectivity state */

Re: [PATCH v1 0/2] PCI/ASPM: Refine L1 PM Substates support

[Vidya Sagar]

Reviewed-by: Vidya Sagar <vidyas@nvidia.com>

On Sunday 03 December 2017 03:15 AM, Bjorn Helgaas wrote:
> The PCIe active link power state is L0.  ASPM defines two low-power
> states: L0s and L1.  The L1 PM Substates feature defines two
> additional low-power states: L1.1 and L2.2.
>
> The L1.2 state may have substantial entry/exit latency.  Downstream
> devices can use the Latency Tolerance Reporting (LTR) feature to
> report how much latency they can tolerate.  The L1 PM Substates
> capability includes a programmable L1.2 threshold register.  If the
> downstream devices can tolerate the latency indicated by the
> threshold, L1.2 may be used.
>
> If LTR is not enabled, the L1.2 threshold is ignored and L1.2 can be
> used whenever it is enabled and CLKREQ# is de-asserted.  Linux
> currently never enables LTR, but firmware may have done so.
>
> The current L1 PM substates support in Linux sets the L1.2 threshold
> to a fixed value (163.84us) copied from coreboot [1].  I don't know
> where that value came from, and it is incorrect for some platforms,
> e.g., Tegra [2].
>
> These patches change that so we calculate the L1.2 threshold based on
> values reported by the hardware, and we enable LTR whenever possible.
>
> This is all just my understanding from reading the spec.  I'd be glad
> to be corrected if I'm going wrong.
>
> I'm particularly puzzled about the coreboot code in
> pciexp_L1_substate_commit() that sets LTR_L1.2_THRESHOLD:
>
> +	pcie_update_cfg(root, root_cap + 0x08, ~0xe3ff0000,
> +		(1 << 21) | (1 << 23) | (1 << 30));
>
> This is writing the L1 PM Substates Control 1 register, but the shifts
> don't match up with any of the fields in the register, so this is an
> awfully funny way to write the threshold.
>
> [1] https://mail.coreboot.org/pipermail/coreboot-gerrit/2015-March/021134.html
> [2] https://lkml.kernel.org/r/1510492674-12786-1-git-send-email-vidyas@nvidia.com
>
> ---
>
> Bjorn Helgaas (2):
>        PCI/ASPM: Calculate LTR_L1.2_THRESHOLD from device characteristics
>        PCI/ASPM: Enable Latency Tolerance Reporting when supported
>
>
>   drivers/pci/pcie/aspm.c |   71 +++++++++++++++++++++++++++++++----------------
>   drivers/pci/probe.c     |   33 ++++++++++++++++++++++
>   include/linux/pci.h     |    2 +
>   3 files changed, 82 insertions(+), 24 deletions(-)

Re: [PATCH v1 0/2] PCI/ASPM: Refine L1 PM Substates support

[Bjorn Helgaas]

On Sat, Dec 02, 2017 at 03:45:38PM -0600, Bjorn Helgaas wrote:
> The PCIe active link power state is L0.  ASPM defines two low-power
> states: L0s and L1.  The L1 PM Substates feature defines two
> additional low-power states: L1.1 and L2.2.
> 
> The L1.2 state may have substantial entry/exit latency.  Downstream
> devices can use the Latency Tolerance Reporting (LTR) feature to
> report how much latency they can tolerate.  The L1 PM Substates
> capability includes a programmable L1.2 threshold register.  If the
> downstream devices can tolerate the latency indicated by the
> threshold, L1.2 may be used.
> 
> If LTR is not enabled, the L1.2 threshold is ignored and L1.2 can be
> used whenever it is enabled and CLKREQ# is de-asserted.  Linux
> currently never enables LTR, but firmware may have done so.
> 
> The current L1 PM substates support in Linux sets the L1.2 threshold
> to a fixed value (163.84us) copied from coreboot [1].  I don't know
> where that value came from, and it is incorrect for some platforms,
> e.g., Tegra [2].
> 
> These patches change that so we calculate the L1.2 threshold based on
> values reported by the hardware, and we enable LTR whenever possible.
> 
> This is all just my understanding from reading the spec.  I'd be glad
> to be corrected if I'm going wrong.
> 
> I'm particularly puzzled about the coreboot code in
> pciexp_L1_substate_commit() that sets LTR_L1.2_THRESHOLD:
> 
> +	pcie_update_cfg(root, root_cap + 0x08, ~0xe3ff0000,
> +		(1 << 21) | (1 << 23) | (1 << 30));
> 
> This is writing the L1 PM Substates Control 1 register, but the shifts
> don't match up with any of the fields in the register, so this is an
> awfully funny way to write the threshold.
> 
> [1] https://mail.coreboot.org/pipermail/coreboot-gerrit/2015-March/021134.html
> [2] https://lkml.kernel.org/r/1510492674-12786-1-git-send-email-vidyas@nvidia.com
> 
> ---
> 
> Bjorn Helgaas (2):
>       PCI/ASPM: Calculate LTR_L1.2_THRESHOLD from device characteristics
>       PCI/ASPM: Enable Latency Tolerance Reporting when supported
> 
> 
>  drivers/pci/pcie/aspm.c |   71 +++++++++++++++++++++++++++++++----------------
>  drivers/pci/probe.c     |   33 ++++++++++++++++++++++
>  include/linux/pci.h     |    2 +
>  3 files changed, 82 insertions(+), 24 deletions(-)

I applied these with Vidya's Reviewed-by to pci/aspm for v4.16.

I'd still really like to hear from the coreboot folks about the
rationale for the current coreboot code.  You folks are in a much
better position to actually understand the hardware details.

Bjorn

RE: [PATCH v1 0/2] PCI/ASPM: Refine L1 PM Substates support

[Chen, Kenji]

https://pcisig.com/sites/default/files/specification_documents/ECN_L1_PM_Substates_with_CLKREQ_31_May_2013_Rev10a.pdf
Pls also check My Intel Doc# 545845 for the bits fields description.

-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas@kernel.org] 
Sent: Wednesday, December 6, 2017 3:41 AM
To: linux-pci@vger.kernel.org
Cc: Chen, Kenji <kenji.chen@intel.com>; Thierry Reding <treding@nvidia.com>; Manikanta Maddireddy <mmaddireddy@nvidia.com>; David Daney <david.daney@cavium.com>; Krishna Kishore <kthota@nvidia.com>; linux-kernel@vger.kernel.org; Vidya Sagar <vidyas@nvidia.com>; Julia Lawall <Julia.Lawall@lip6.fr>; linux-tegra@vger.kernel.org; Patrick Georgi <pgeorgi@chromium.org>; Rajat Jain <rajatja@google.com>; Yinghai Lu <yinghai@kernel.org>; Stefan Reinauer <stefan.reinauer@coreboot.org>; Duncan Laurie <dlaurie@chromium.org>
Subject: Re: [PATCH v1 0/2] PCI/ASPM: Refine L1 PM Substates support

On Sat, Dec 02, 2017 at 03:45:38PM -0600, Bjorn Helgaas wrote:
> The PCIe active link power state is L0.  ASPM defines two low-power
> states: L0s and L1.  The L1 PM Substates feature defines two 
> additional low-power states: L1.1 and L2.2.
> 
> The L1.2 state may have substantial entry/exit latency.  Downstream 
> devices can use the Latency Tolerance Reporting (LTR) feature to 
> report how much latency they can tolerate.  The L1 PM Substates 
> capability includes a programmable L1.2 threshold register.  If the 
> downstream devices can tolerate the latency indicated by the 
> threshold, L1.2 may be used.
> 
> If LTR is not enabled, the L1.2 threshold is ignored and L1.2 can be 
> used whenever it is enabled and CLKREQ# is de-asserted.  Linux 
> currently never enables LTR, but firmware may have done so.
> 
> The current L1 PM substates support in Linux sets the L1.2 threshold 
> to a fixed value (163.84us) copied from coreboot [1].  I don't know 
> where that value came from, and it is incorrect for some platforms, 
> e.g., Tegra [2].
> 
> These patches change that so we calculate the L1.2 threshold based on 
> values reported by the hardware, and we enable LTR whenever possible.
> 
> This is all just my understanding from reading the spec.  I'd be glad 
> to be corrected if I'm going wrong.
> 
> I'm particularly puzzled about the coreboot code in
> pciexp_L1_substate_commit() that sets LTR_L1.2_THRESHOLD:
> 
> +	pcie_update_cfg(root, root_cap + 0x08, ~0xe3ff0000,
> +		(1 << 21) | (1 << 23) | (1 << 30));
> 
> This is writing the L1 PM Substates Control 1 register, but the shifts 
> don't match up with any of the fields in the register, so this is an 
> awfully funny way to write the threshold.
> 
> [1] 
> https://mail.coreboot.org/pipermail/coreboot-gerrit/2015-March/021134.
> html [2] 
> https://lkml.kernel.org/r/1510492674-12786-1-git-send-email-vidyas@nvi
> dia.com
> 
> ---
> 
> Bjorn Helgaas (2):
>       PCI/ASPM: Calculate LTR_L1.2_THRESHOLD from device characteristics
>       PCI/ASPM: Enable Latency Tolerance Reporting when supported
> 
> 
>  drivers/pci/pcie/aspm.c |   71 +++++++++++++++++++++++++++++++----------------
>  drivers/pci/probe.c     |   33 ++++++++++++++++++++++
>  include/linux/pci.h     |    2 +
>  3 files changed, 82 insertions(+), 24 deletions(-)

I applied these with Vidya's Reviewed-by to pci/aspm for v4.16.

I'd still really like to hear from the coreboot folks about the rationale for the current coreboot code.  You folks are in a much better position to actually understand the hardware details.

Bjorn

Re: [PATCH v1 0/2] PCI/ASPM: Refine L1 PM Substates support

[Bjorn Helgaas]

On Wed, Dec 06, 2017 at 01:55:37AM +0000, Chen, Kenji wrote:
> https://pcisig.com/sites/default/files/specification_documents/ECN_L1_PM_Substates_with_CLKREQ_31_May_2013_Rev10a.pdf

With all due respect, this doesn't seem to add any new information.
For example, the L1 PM Substates Control 1 register description from
the above is basically identical to the published PCIe r3.1, sec
7.33.3.

So it doesn't answer the questions of:

  1) Why coreboot sets the L1.2 threshold to a fixed value, and

  2) How "(1 << 21) | (1 << 23) | (1 << 30)" relates to the Control 1
     register

> Pls also check My Intel Doc# 545845 for the bits fields description.

I don't have access to this document.  The Linux code I'm proposing is
based on the PCIe spec and is intended to work on all hardware that
conforms to that spec.  I'm not very familiar with coreboot, but maybe
the L1 Substates code there is only intended to work on certain Intel
chipsets and doesn't need to work on other hardware?

Bjorn

> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas@kernel.org] 
> Sent: Wednesday, December 6, 2017 3:41 AM
> To: linux-pci@vger.kernel.org
> Cc: Chen, Kenji <kenji.chen@intel.com>; Thierry Reding <treding@nvidia.com>; Manikanta Maddireddy <mmaddireddy@nvidia.com>; David Daney <david.daney@cavium.com>; Krishna Kishore <kthota@nvidia.com>; linux-kernel@vger.kernel.org; Vidya Sagar <vidyas@nvidia.com>; Julia Lawall <Julia.Lawall@lip6.fr>; linux-tegra@vger.kernel.org; Patrick Georgi <pgeorgi@chromium.org>; Rajat Jain <rajatja@google.com>; Yinghai Lu <yinghai@kernel.org>; Stefan Reinauer <stefan.reinauer@coreboot.org>; Duncan Laurie <dlaurie@chromium.org>
> Subject: Re: [PATCH v1 0/2] PCI/ASPM: Refine L1 PM Substates support
> 
> On Sat, Dec 02, 2017 at 03:45:38PM -0600, Bjorn Helgaas wrote:
> > The PCIe active link power state is L0.  ASPM defines two low-power
> > states: L0s and L1.  The L1 PM Substates feature defines two 
> > additional low-power states: L1.1 and L2.2.
> > 
> > The L1.2 state may have substantial entry/exit latency.  Downstream 
> > devices can use the Latency Tolerance Reporting (LTR) feature to 
> > report how much latency they can tolerate.  The L1 PM Substates 
> > capability includes a programmable L1.2 threshold register.  If the 
> > downstream devices can tolerate the latency indicated by the 
> > threshold, L1.2 may be used.
> > 
> > If LTR is not enabled, the L1.2 threshold is ignored and L1.2 can be 
> > used whenever it is enabled and CLKREQ# is de-asserted.  Linux 
> > currently never enables LTR, but firmware may have done so.
> > 
> > The current L1 PM substates support in Linux sets the L1.2 threshold 
> > to a fixed value (163.84us) copied from coreboot [1].  I don't know 
> > where that value came from, and it is incorrect for some platforms, 
> > e.g., Tegra [2].
> > 
> > These patches change that so we calculate the L1.2 threshold based on 
> > values reported by the hardware, and we enable LTR whenever possible.
> > 
> > This is all just my understanding from reading the spec.  I'd be glad 
> > to be corrected if I'm going wrong.
> > 
> > I'm particularly puzzled about the coreboot code in
> > pciexp_L1_substate_commit() that sets LTR_L1.2_THRESHOLD:
> > 
> > +	pcie_update_cfg(root, root_cap + 0x08, ~0xe3ff0000,
> > +		(1 << 21) | (1 << 23) | (1 << 30));
> > 
> > This is writing the L1 PM Substates Control 1 register, but the shifts 
> > don't match up with any of the fields in the register, so this is an 
> > awfully funny way to write the threshold.
> > 
> > [1] 
> > https://mail.coreboot.org/pipermail/coreboot-gerrit/2015-March/021134.
> > html [2] 
> > https://lkml.kernel.org/r/1510492674-12786-1-git-send-email-vidyas@nvi
> > dia.com
> > 
> > ---
> > 
> > Bjorn Helgaas (2):
> >       PCI/ASPM: Calculate LTR_L1.2_THRESHOLD from device characteristics
> >       PCI/ASPM: Enable Latency Tolerance Reporting when supported
> > 
> > 
> >  drivers/pci/pcie/aspm.c |   71 +++++++++++++++++++++++++++++++----------------
> >  drivers/pci/probe.c     |   33 ++++++++++++++++++++++
> >  include/linux/pci.h     |    2 +
> >  3 files changed, 82 insertions(+), 24 deletions(-)
> 
> I applied these with Vidya's Reviewed-by to pci/aspm for v4.16.
> 
> I'd still really like to hear from the coreboot folks about the rationale for the current coreboot code.  You folks are in a much better position to actually understand the hardware details.
> 
> Bjorn

RE: [PATCH v1 0/2] PCI/ASPM: Refine L1 PM Substates support

[Chen, Kenji]

For Intel Broadwell, SKylake, and KabyLake PCIe Root Port, the threshold is recommended as it is in the commit. If the BIOS/Coreboot porting between platforms is taken into consideration, using a build definition or variables from somewhere of customizable zone is preferred. Let Coreboot guys make the call since I am working for Intel Only.

-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas@kernel.org] 
Sent: Thursday, December 7, 2017 10:45 PM
To: Chen, Kenji <kenji.chen@intel.com>
Cc: linux-pci@vger.kernel.org; Thierry Reding <treding@nvidia.com>; Manikanta Maddireddy <mmaddireddy@nvidia.com>; David Daney <david.daney@cavium.com>; Krishna Kishore <kthota@nvidia.com>; linux-kernel@vger.kernel.org; Vidya Sagar <vidyas@nvidia.com>; Julia Lawall <Julia.Lawall@lip6.fr>; linux-tegra@vger.kernel.org; Patrick Georgi <pgeorgi@chromium.org>; Rajat Jain <rajatja@google.com>; Yinghai Lu <yinghai@kernel.org>; Stefan Reinauer <stefan.reinauer@coreboot.org>; Duncan Laurie <dlaurie@chromium.org>
Subject: Re: [PATCH v1 0/2] PCI/ASPM: Refine L1 PM Substates support

On Wed, Dec 06, 2017 at 01:55:37AM +0000, Chen, Kenji wrote:
> https://pcisig.com/sites/default/files/specification_documents/ECN_L1_
> PM_Substates_with_CLKREQ_31_May_2013_Rev10a.pdf

With all due respect, this doesn't seem to add any new information.
For example, the L1 PM Substates Control 1 register description from the above is basically identical to the published PCIe r3.1, sec 7.33.3.

So it doesn't answer the questions of:

  1) Why coreboot sets the L1.2 threshold to a fixed value, and

  2) How "(1 << 21) | (1 << 23) | (1 << 30)" relates to the Control 1
     register

> Pls also check My Intel Doc# 545845 for the bits fields description.

I don't have access to this document.  The Linux code I'm proposing is based on the PCIe spec and is intended to work on all hardware that conforms to that spec.  I'm not very familiar with coreboot, but maybe the L1 Substates code there is only intended to work on certain Intel chipsets and doesn't need to work on other hardware?

Bjorn

> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> Sent: Wednesday, December 6, 2017 3:41 AM
> To: linux-pci@vger.kernel.org
> Cc: Chen, Kenji <kenji.chen@intel.com>; Thierry Reding 
> <treding@nvidia.com>; Manikanta Maddireddy <mmaddireddy@nvidia.com>; 
> David Daney <david.daney@cavium.com>; Krishna Kishore 
> <kthota@nvidia.com>; linux-kernel@vger.kernel.org; Vidya Sagar 
> <vidyas@nvidia.com>; Julia Lawall <Julia.Lawall@lip6.fr>; 
> linux-tegra@vger.kernel.org; Patrick Georgi <pgeorgi@chromium.org>; 
> Rajat Jain <rajatja@google.com>; Yinghai Lu <yinghai@kernel.org>; 
> Stefan Reinauer <stefan.reinauer@coreboot.org>; Duncan Laurie 
> <dlaurie@chromium.org>
> Subject: Re: [PATCH v1 0/2] PCI/ASPM: Refine L1 PM Substates support
> 
> On Sat, Dec 02, 2017 at 03:45:38PM -0600, Bjorn Helgaas wrote:
> > The PCIe active link power state is L0.  ASPM defines two low-power
> > states: L0s and L1.  The L1 PM Substates feature defines two 
> > additional low-power states: L1.1 and L2.2.
> > 
> > The L1.2 state may have substantial entry/exit latency.  Downstream 
> > devices can use the Latency Tolerance Reporting (LTR) feature to 
> > report how much latency they can tolerate.  The L1 PM Substates 
> > capability includes a programmable L1.2 threshold register.  If the 
> > downstream devices can tolerate the latency indicated by the 
> > threshold, L1.2 may be used.
> > 
> > If LTR is not enabled, the L1.2 threshold is ignored and L1.2 can be 
> > used whenever it is enabled and CLKREQ# is de-asserted.  Linux 
> > currently never enables LTR, but firmware may have done so.
> > 
> > The current L1 PM substates support in Linux sets the L1.2 threshold 
> > to a fixed value (163.84us) copied from coreboot [1].  I don't know 
> > where that value came from, and it is incorrect for some platforms, 
> > e.g., Tegra [2].
> > 
> > These patches change that so we calculate the L1.2 threshold based 
> > on values reported by the hardware, and we enable LTR whenever possible.
> > 
> > This is all just my understanding from reading the spec.  I'd be 
> > glad to be corrected if I'm going wrong.
> > 
> > I'm particularly puzzled about the coreboot code in
> > pciexp_L1_substate_commit() that sets LTR_L1.2_THRESHOLD:
> > 
> > +	pcie_update_cfg(root, root_cap + 0x08, ~0xe3ff0000,
> > +		(1 << 21) | (1 << 23) | (1 << 30));
> > 
> > This is writing the L1 PM Substates Control 1 register, but the 
> > shifts don't match up with any of the fields in the register, so 
> > this is an awfully funny way to write the threshold.
> > 
> > [1]
> > https://mail.coreboot.org/pipermail/coreboot-gerrit/2015-March/021134.
> > html [2]
> > https://lkml.kernel.org/r/1510492674-12786-1-git-send-email-vidyas@n
> > vi
> > dia.com
> > 
> > ---
> > 
> > Bjorn Helgaas (2):
> >       PCI/ASPM: Calculate LTR_L1.2_THRESHOLD from device characteristics
> >       PCI/ASPM: Enable Latency Tolerance Reporting when supported
> > 
> > 
> >  drivers/pci/pcie/aspm.c |   71 +++++++++++++++++++++++++++++++----------------
> >  drivers/pci/probe.c     |   33 ++++++++++++++++++++++
> >  include/linux/pci.h     |    2 +
> >  3 files changed, 82 insertions(+), 24 deletions(-)
> 
> I applied these with Vidya's Reviewed-by to pci/aspm for v4.16.
> 
> I'd still really like to hear from the coreboot folks about the rationale for the current coreboot code.  You folks are in a much better position to actually understand the hardware details.
> 
> Bjorn