PCI: vmd: Enable PCI PM's L1 substates of remapped PCIe Root Port and NVMe

PATCH Superseded Jian-Hong Pan 4 participants 10 messages Feb 2, 2024 – Feb 7, 2024

Subsystems (3)

PCI PCI native host bridge and endpoint drivers PCI driver for Intel volume management device (vmd)

lore

Share & tools

lore

Jian-Hong Pan

author

2024-02-02 07:25 UTC #0 lore raw

PATCH v2 1/1 PCI: vmd: Enable PCI PM's L1 substates of remapped PCIe Root Port and NVMe

The remapped PCIe Root Port and NVMe have PCI PM L1 substates capability, but they are disabled originally:

Here is an example on ASUS B1400CEAE:

Capabilities: [900 v1] L1 PM Substates

        L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1- L1_PM_Substates+
                  PortCommonModeRestoreTime=32us PortTPowerOnTime=10us
        L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1-
                   T_CommonMode=0us LTR1.2_Threshold=0ns
        L1SubCtl2: T_PwrOn=10us

Power on all of the VMD remapped PCI devices and quirk max snoop LTR before enable PCI-PM L1 PM Substates by following "Section 5.5.4 of PCIe Base Spec Revision 6.0". Then, PCI PM's L1 substates control are initialized & enabled accordingly. Also, update the comments of pci_enable_link_state() and pci_enable_link_state_locked() for kernel-doc, too.

Link https://bugzilla.kernel.org/show_bug.cgi?id=218394

Signed-off-by Jian-Hong Pan <[email protected]>

---
v2:
- Power on the VMD remapped devices with pci_set_power_state_locked()
- Prepare the PCIe LTR parameters before enable L1 Substates
- Add note into the comments of both pci_enable_link_state() and pci_enable_link_state_locked() for kernel-doc.
- The original patch set can be split as individual patches.

 drivers/pci/controller/vmd.c | 15 ++++++++++-----
 drivers/pci/pcie/aspm.c      | 10 ++++++++++
 2 files changed, 20 insertions(+), 5 deletions(-)

drivers/pci/controller/vmd.c +10 -5

index 87b7856f375a..66e47a0dbf1a 100644
@@ -751,11 +751,9 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 	if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
 		return 0;
 
-	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
-
 	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
 	if (!pos)
-		return 0;
+		goto out_enable_link_state;
 
 	/*
 	 * Skip if the max snoop LTR is non-zero, indicating BIOS has set it

@@ -763,7 +761,7 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 	 */
 	pci_read_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &ltr_reg);
 	if (!!(ltr_reg & (PCI_LTR_VALUE_MASK | PCI_LTR_SCALE_MASK)))
-		return 0;
+		goto out_enable_link_state;
 
 	/*
 	 * Set the default values to the maximum required by the platform to

@@ -775,6 +773,14 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 	pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
 	pci_info(pdev, "VMD: Default LTR value set by driver\n");
 
+out_enable_link_state:
+	/*
+	 * Make PCI devices at D0 when enable PCI-PM L1 PM Substates from
+	 * Section 5.5.4 of PCIe Base Spec Revision 6.0
+	 */
+	pci_set_power_state_locked(pdev, PCI_D0);
+	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
+
 	return 0;
 }

@@ -926,7 +932,6 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 				   dev_get_msi_domain(&vmd->dev->dev));
 
 	vmd_acpi_begin();
-
 	pci_scan_child_bus(vmd->bus);
 	vmd_domain_reset(vmd);

drivers/pci/pcie/aspm.c +10 -0

index bc0bd86695ec..5f902c5552ca 100644
@@ -1164,6 +1164,8 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
 		link->aspm_default |= ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1;
 	if (state & PCIE_LINK_STATE_L1_2_PCIPM)
 		link->aspm_default |= ASPM_STATE_L1_2_PCIPM | ASPM_STATE_L1;
+	if (state & ASPM_STATE_L1_2_MASK)
+		aspm_l1ss_init(link);
 	pcie_config_aspm_link(link, policy_to_aspm_state(link));
 
 	link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;

@@ -1182,6 +1184,10 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
  * touch the LNKCTL register. Also note that this does not enable states
  * disabled by pci_disable_link_state(). Return 0 or a negative errno.
  *
+ * Note: The PCIe devices of the link must be in D0, if the PCI-PM L1 PM
+ * substates are going to be enabled. From Section 5.5.4 of PCIe Base Spec
+ * Revision 6.0.
+ *
  * @pdev: PCI device
  * @state: Mask of ASPM link states to enable
  */

@@ -1198,6 +1204,10 @@ EXPORT_SYMBOL(pci_enable_link_state);
  * can't touch the LNKCTL register. Also note that this does not enable states
  * disabled by pci_disable_link_state(). Return 0 or a negative errno.
  *
+ * Note: The PCIe devices of the link must be in D0, if the PCI-PM L1 PM
+ * substates are going to be enabled. From Section 5.5.4 of PCIe Base Spec
+ * Revision 6.0.
+ *
  * @pdev: PCI device
  * @state: Mask of ASPM link states to enable
  *

Signature · 1 line

2.43.0

Bjorn Helgaas

2024-02-03 00:05 UTC #1 lore raw

Quoted 12 lines · Jian-Hong Pan

The remapped PCIe Root Port and NVMe have PCI PM L1 substates capability, but they are disabled originally:
Here is an example on ASUS B1400CEAE:
Capabilities: [900 v1] L1 PM Substates
        L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1- L1_PM_Substates+
                  PortCommonModeRestoreTime=32us PortTPowerOnTime=10us
        L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1-
                   T_CommonMode=0us LTR1.2_Threshold=0ns
        L1SubCtl2: T_PwrOn=10us

Quoted 8 lines

Power on all of the VMD remapped PCI devices and quirk max snoop LTR before enable PCI-PM L1 PM Substates by following "Section 5.5.4 of PCIe Base Spec Revision 6.0". Then, PCI PM's L1 substates control are initialized & enabled accordingly.
Also, update the comments of
pci_enable_link_state() and pci_enable_link_state_locked() for kernel-doc, too.

The aspm.c changes should be in a separate patch. Presumably the aspm.c code change fixes a defect, and that defect can be described in that patch. That fix may be needed for non-VMD situations, and having it in this vmd patch means it won't be as easy to find and backport.

Nit: rewrap commit log to fill 75 columns.

Quoted 23 lines

drivers/pci/controller/vmd.c +2 -4

@@ -751,11 +751,9 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 	if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
 		return 0;
 
-	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
-
 	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
 	if (!pos)
-		return 0;
+		goto out_enable_link_state;
 
 	/*
 	 * Skip if the max snoop LTR is non-zero, indicating BIOS has set it

@@ -763,7 +761,7 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 	 */
 	pci_read_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &ltr_reg);
 	if (!!(ltr_reg & (PCI_LTR_VALUE_MASK | PCI_LTR_SCALE_MASK)))
-		return 0;
+		goto out_enable_link_state;
 
 	/*
 	 * Set the default values to the maximum required by the platform to

drivers/pci/controller/vmd.c static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata) · line 774

Quoted 2 lines

drivers/pci/controller/vmd.c +0 -0

@@ -775,6 +773,14 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 	pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
 	pci_info(pdev, "VMD: Default LTR value set by driver\n");

You're not changing this part, and I don't understand exactly how LTR works, but it makes me a little bit queasy to read "set the LTR value to the maximum required to allow the deepest power management savings" and then we set the max snoop values to a fixed constant.

I don't think the goal is to "allow the deepest power savings"; I think it's to enable L1.2 *when the device has enough buffering to absorb L1.2 entry/exit latencies*.

The spec (PCIe r6.0, sec 7.8.2.2) says "Software should set this to the platform's maximum supported latency or less," so it seems like that value must be platform-dependent, not fixed.

And I assume the "_DSM for Latency Tolerance Reporting" is part of the way to get those platform-dependent values, but Linux doesn't actually use that yet.

I assume that setting the max values incorrectly may lead to either being too conservative, so we don't use L1.2 when we could, or too aggressive, so we use L1.2 when we shouldn't, and the device loses data because it doesn't have enough internal buffering to absorb the entry/exit delays.

This paper has a lot of background and might help answer some of my questions:
https://www.intel.co.za/content/dam/doc/white-paper/energy-efficient-platforms-white-paper.pdf

drivers/pci/controller/vmd.c static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata) · line 782

Quoted 7 lines

drivers/pci/controller/vmd.c +7 -0

+out_enable_link_state:
+	/*
+	 * Make PCI devices at D0 when enable PCI-PM L1 PM Substates from
+	 * Section 5.5.4 of PCIe Base Spec Revision 6.0
+	 */
+	pci_set_power_state_locked(pdev, PCI_D0);
+	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);

Hmmm. PCIE_LINK_STATE_ALL includes ASPM L1.2. We may do this even if the device doesn't have an LTR Capability. ASPM L1.2 cannot work without LTR.

I only took a quick look but was not convinced that pci_enable_link_state() does the right thing when we enable PCIE_LINK_STATE_ALL (including ASPM L1.2) on a device that doesn't have LTR. I think it *should* decline to set PCI_L1SS_CTL1_ASPM_L1_2, but I'm not sure it does. Can you double check that path? Maybe that's another defect in aspm.c.

Quoted 9 lines

drivers/pci/pcie/aspm.c +2 -0

@@ -1164,6 +1164,8 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
 		link->aspm_default |= ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1;
 	if (state & PCIE_LINK_STATE_L1_2_PCIPM)
 		link->aspm_default |= ASPM_STATE_L1_2_PCIPM | ASPM_STATE_L1;
+	if (state & ASPM_STATE_L1_2_MASK)
+		aspm_l1ss_init(link);
 	pcie_config_aspm_link(link, policy_to_aspm_state(link));
 
 	link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;

David E. Box

2024-02-05 19:37 UTC #2 lore raw

Quoted 70 lines · Bjorn Helgaas

Quoted 13 lines · Jian-Hong Pan
The remapped PCIe Root Port and NVMe have PCI PM L1 substates capability, but they are disabled originally:
Here is an example on ASUS B1400CEAE:
Capabilities: [900 v1] L1 PM Substates
        L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1- L1_PM_Substates+
                  PortCommonModeRestoreTime=32us PortTPowerOnTime=10us
        L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1- T_CommonMode=0us LTR1.2_Threshold=0ns
        L1SubCtl2: T_PwrOn=10us
Quoted 8 lines
Power on all of the VMD remapped PCI devices and quirk max snoop LTR before enable PCI-PM L1 PM Substates by following "Section 5.5.4 of PCIe Base Spec Revision 6.0". Then, PCI PM's L1 substates control are initialized & enabled accordingly.
Also, update the comments of
pci_enable_link_state() and pci_enable_link_state_locked() for kernel-doc, too.
The aspm.c changes should be in a separate patch. Presumably the aspm.c code change fixes a defect, and that defect can be described in that patch. That fix may be needed for non-VMD situations, and having it in this vmd patch means it won't be as easy to find and backport.
Nit: rewrap commit log to fill 75 columns.
Quoted 27 lines
drivers/pci/controller/vmd.c +0 -0
@@ -751,11 +751,9 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev,
 void *userdata)
  	if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
  		return 0;
drivers/pci/controller/vmd.c +0 -2
-	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
-
pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR); if (!pos)
(quoted diff) +1 -1
-		return 0;
+		goto out_enable_link_state;
/*
* Skip if the max snoop LTR is non-zero, indicating BIOS has set it
drivers/pci/controller/vmd.c +1 -1
@@ -763,7 +761,7 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev,
 void *userdata)
  	 */
  	pci_read_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &ltr_reg);
  	if (!!(ltr_reg & (PCI_LTR_VALUE_MASK | PCI_LTR_SCALE_MASK)))
-		return 0;
+		goto out_enable_link_state;
/*
* Set the default values to the maximum required by the platform to
drivers/pci/controller/vmd.c static int vmd_pm_enable_quirk(struct pci_dev *pdev, · line 775
Quoted 3 lines
drivers/pci/controller/vmd.c +0 -0
@@ -775,6 +773,14 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev,
 void *userdata)
  	pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
  	pci_info(pdev, "VMD: Default LTR value set by driver\n");
You're not changing this part, and I don't understand exactly how LTR works, but it makes me a little bit queasy to read "set the LTR value to the maximum required to allow the deepest power management savings" and then we set the max snoop values to a fixed constant.
I don't think the goal is to "allow the deepest power savings"; I think it's to enable L1.2 *when the device has enough buffering to absorb L1.2 entry/exit latencies*.

Quoted 7 lines

The spec (PCIe r6.0, sec 7.8.2.2) says "Software should set this to the platform's maximum supported latency or less," so it seems like that value must be platform-dependent, not fixed.
And I assume the "_DSM for Latency Tolerance Reporting" is part of the way to get those platform-dependent values, but Linux doesn't actually use that yet.

This may indeed be the best way but we need to double check with our BIOS folks. AFAIK BIOS writes the LTR values directly so there hasn't been a need to use this _DSM. But under VMD the ports are hidden from BIOS which is why we added it here. I've brought up the question internally to find out how Windows handles the DSM and to get a recommendation from our firmware leads.

Quoted 23 lines

I assume that setting the max values incorrectly may lead to either being too conservative, so we don't use L1.2 when we could, or too aggressive, so we use L1.2 when we shouldn't, and the device loses data because it doesn't have enough internal buffering to absorb the entry/exit delays.
This paper has a lot of background and might help answer some of my questions:
https://www.intel.co.za/content/dam/doc/white-paper/energy-efficient-platforms-white-paper.pdf
drivers/pci/controller/vmd.c static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata) · line 782
Quoted 7 lines
drivers/pci/controller/vmd.c +7 -0
+out_enable_link_state:
+	/*
+	 * Make PCI devices at D0 when enable PCI-PM L1 PM Substates from
+	 * Section 5.5.4 of PCIe Base Spec Revision 6.0
+	 */
+	pci_set_power_state_locked(pdev, PCI_D0);
+	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
Hmmm. PCIE_LINK_STATE_ALL includes ASPM L1.2. We may do this even if the device doesn't have an LTR Capability. ASPM L1.2 cannot work without LTR.

Quoted 6 lines

I only took a quick look but was not convinced that pci_enable_link_state() does the right thing when we enable PCIE_LINK_STATE_ALL (including ASPM L1.2) on a device that doesn't have LTR. I think it *should* decline to set PCI_L1SS_CTL1_ASPM_L1_2, but I'm not sure it does. Can you double check that path? Maybe that's another defect in aspm.c.

It doesn't currently decline. The same scenario happens when the user selects powersupersave. If a device advertises L1.2 with the capable registers set, it should also have the LTR register present. But it doesn't hurt to check.

David

Quoted 13 lines

Quoted 13 lines
(quoted diff) +0 -0
@@ -1164,6 +1164,8 @@ static int __pci_enable_link_state(struct pci_dev
 *pdev, int state, bool locked)
link->aspm_default |= ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1;
if (state & PCIE_LINK_STATE_L1_2_PCIPM) link->aspm_default |= ASPM_STATE_L1_2_PCIPM | ASPM_STATE_L1;
drivers/pci/pcie/aspm.c +2 -0
+	if (state & ASPM_STATE_L1_2_MASK)
+		aspm_l1ss_init(link);
pcie_config_aspm_link(link, policy_to_aspm_state(link));
link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;

Bjorn Helgaas

2024-02-05 22:42 UTC #3 lore raw

Quoted 26 lines · David E. Box

Quoted 1 line · Bjorn Helgaas
On Fri, Feb 02, 2024 at 03:11:12PM +0800, Jian-Hong Pan wrote:
...
Quoted 14 lines
drivers/pci/controller/vmd.c static int vmd_pm_enable_quirk(struct pci_dev *pdev, · line 775
Quoted 4 lines
drivers/pci/controller/vmd.c +0 -0
@@ -775,6 +773,14 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev,
 void *userdata)
  	pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
  	pci_info(pdev, "VMD: Default LTR value set by driver\n");
You're not changing this part, and I don't understand exactly how LTR works, but it makes me a little bit queasy to read "set the LTR value to the maximum required to allow the deepest power management savings" and then we set the max snoop values to a fixed constant.
I don't think the goal is to "allow the deepest power savings"; I think it's to enable L1.2 *when the device has enough buffering to absorb L1.2 entry/exit latencies*.
Quoted 7 lines
The spec (PCIe r6.0, sec 7.8.2.2) says "Software should set this to the platform's maximum supported latency or less," so it seems like that value must be platform-dependent, not fixed.
And I assume the "_DSM for Latency Tolerance Reporting" is part of the way to get those platform-dependent values, but Linux doesn't actually use that yet.

Quoted 6 lines

This may indeed be the best way but we need to double check with our BIOS folks. AFAIK BIOS writes the LTR values directly so there hasn't been a need to use this _DSM. But under VMD the ports are hidden from BIOS which is why we added it here. I've brought up the question internally to find out how Windows handles the DSM and to get a recommendation from our firmware leads.

We want Linux to be able to program LTR itself, don't we? We shouldn't have to rely on firmware to do it. If Linux can't do it, hot-added devices aren't going to be able to use L1.2, right?

Bjorn

David E. Box

2024-02-05 23:05 UTC #4 lore raw

Quoted 36 lines · Bjorn Helgaas

Quoted 28 lines · David E. Box
Quoted 1 line · Bjorn Helgaas
On Fri, Feb 02, 2024 at 03:11:12PM +0800, Jian-Hong Pan wrote:
...
Quoted 16 lines
Review static int vmd_pm_enable_quirk(struct pci_dev · line 777
Quoted 6 lines
(quoted diff) +0 -0
@@ -775,6 +773,14 @@ static int vmd_pm_enable_quirk(struct pci_dev
 *pdev,
 void *userdata)
  	pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT,
 ltr_reg);
  	pci_info(pdev, "VMD: Default LTR value set by driver\n");
You're not changing this part, and I don't understand exactly how LTR works, but it makes me a little bit queasy to read "set the LTR value to the maximum required to allow the deepest power management savings" and then we set the max snoop values to a fixed constant.
I don't think the goal is to "allow the deepest power savings"; I think it's to enable L1.2 *when the device has enough buffering to absorb L1.2 entry/exit latencies*.
Quoted 7 lines
The spec (PCIe r6.0, sec 7.8.2.2) says "Software should set this to the platform's maximum supported latency or less," so it seems like that value must be platform-dependent, not fixed.
And I assume the "_DSM for Latency Tolerance Reporting" is part of the way to get those platform-dependent values, but Linux doesn't actually use that yet.
Quoted 6 lines
This may indeed be the best way but we need to double check with our BIOS folks. AFAIK BIOS writes the LTR values directly so there hasn't been a need to use this _DSM. But under VMD the ports are hidden from BIOS which is why we added it here. I've brought up the question internally to find out how Windows handles the DSM and to get a recommendation from our firmware leads.

Quoted 3 lines

We want Linux to be able to program LTR itself, don't we? We shouldn't have to rely on firmware to do it. If Linux can't do it, hot-added devices aren't going to be able to use L1.2, right?

Agreed. We just want to make sure we are not conflicting with what BIOS may be doing.

David

David E. Box

2024-02-06 21:25 UTC #5 lore raw

Adding Puranjay

Quoted 37 lines · David E. Box

Quoted 36 lines · Bjorn Helgaas
Quoted 34 lines · David E. Box
Quoted 1 line · Bjorn Helgaas
On Fri, Feb 02, 2024 at 03:11:12PM +0800, Jian-Hong Pan wrote:
...
Quoted 16 lines
Review static int vmd_pm_enable_quirk(struct pci_dev · line 777
Quoted 6 lines
(quoted diff) +0 -0
@@ -775,6 +773,14 @@ static int vmd_pm_enable_quirk(struct pci_dev
 *pdev,
 void *userdata)
         pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT,
 ltr_reg);
         pci_info(pdev, "VMD: Default LTR value set by driver\n");
You're not changing this part, and I don't understand exactly how LTR works, but it makes me a little bit queasy to read "set the LTR value to the maximum required to allow the deepest power management savings" and then we set the max snoop values to a fixed constant.
I don't think the goal is to "allow the deepest power savings"; I think it's to enable L1.2 *when the device has enough buffering to absorb L1.2 entry/exit latencies*.
Quoted 7 lines
The spec (PCIe r6.0, sec 7.8.2.2) says "Software should set this to the platform's maximum supported latency or less," so it seems like that value must be platform-dependent, not fixed.
And I assume the "_DSM for Latency Tolerance Reporting" is part of the way to get those platform-dependent values, but Linux doesn't actually use that yet.
This may indeed be the best way but we need to double check with our BIOS folks. AFAIK BIOS writes the LTR values directly so there hasn't been a need to use this _DSM. But under VMD the ports are hidden from BIOS which is why we added it here. I've brought up the question internally to find out how Windows handles the DSM and to get a recommendation from our firmware leads.

Quoted 6 lines

Quoted 3 lines
We want Linux to be able to program LTR itself, don't we? We shouldn't have to rely on firmware to do it. If Linux can't do it, hot-added devices aren't going to be able to use L1.2, right?
Agreed. We just want to make sure we are not conflicting with what BIOS may be doing.

So the feedback is to run the _DSM and just overwrite any BIOS values. Looking up the _DSM I saw there was an attempt to upstream this 4 years ago [1]. I'm not sure why the effort stalled but we can pick up this work again.

https://patchwork.kernel.org/project/linux-pci/patch/[email protected]/

Bjorn Helgaas

2024-02-06 23:30 UTC #6 lore raw

Quoted 46 lines · David E. Box

Quoted 37 lines · David E. Box
Quoted 36 lines · Bjorn Helgaas
Quoted 34 lines · David E. Box
Quoted 1 line · Bjorn Helgaas
On Fri, Feb 02, 2024 at 03:11:12PM +0800, Jian-Hong Pan wrote:
...
Quoted 16 lines
Review static int vmd_pm_enable_quirk(struct pci_dev · line 777
Quoted 6 lines
(quoted diff) +0 -0
@@ -775,6 +773,14 @@ static int vmd_pm_enable_quirk(struct pci_dev
 *pdev,
 void *userdata)
         pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT,
 ltr_reg);
         pci_info(pdev, "VMD: Default LTR value set by driver\n");
You're not changing this part, and I don't understand exactly how LTR works, but it makes me a little bit queasy to read "set the LTR value to the maximum required to allow the deepest power management savings" and then we set the max snoop values to a fixed constant.
I don't think the goal is to "allow the deepest power savings"; I think it's to enable L1.2 *when the device has enough buffering to absorb L1.2 entry/exit latencies*.
Quoted 7 lines
The spec (PCIe r6.0, sec 7.8.2.2) says "Software should set this to the platform's maximum supported latency or less," so it seems like that value must be platform-dependent, not fixed.
And I assume the "_DSM for Latency Tolerance Reporting" is part of the way to get those platform-dependent values, but Linux doesn't actually use that yet.
This may indeed be the best way but we need to double check with our BIOS folks. AFAIK BIOS writes the LTR values directly so there hasn't been a need to use this _DSM. But under VMD the ports are hidden from BIOS which is why we added it here. I've brought up the question internally to find out how Windows handles the DSM and to get a recommendation from our firmware leads.
Quoted 7 lines
Quoted 4 lines
We want Linux to be able to program LTR itself, don't we? We shouldn't have to rely on firmware to do it. If Linux can't do it, hot-added devices aren't going to be able to use L1.2, right?
Agreed. We just want to make sure we are not conflicting with what BIOS may be doing.

Quoted 6 lines

So the feedback is to run the _DSM and just overwrite any BIOS values. Looking up the _DSM I saw there was an attempt to upstream this 4 years ago [1]. I'm not sure why the effort stalled but we can pick up this work again.
https://patchwork.kernel.org/project/linux-pci/patch/[email protected]/

There was a PCI SIG discussion about this a few years ago that never really seemed to get resolved:
https://members.pcisig.com/wg/PCIe-Protocol/mail/thread/35064

Unfortunately that discussion is not public, but the summary is:

Q: How is the LTR_L1.2_THRESHOLD value determined?

PCIe r5.0, sec 5.5.4, says the same value must be programmed into both Ports.

A: As noted in sec 5.5.4, the value is determined primarily by
the amount of time it will take to re-establish the common mode bias on the AC coupling caps, and it is assumed that the BIOS knows this.

Q: How are the LTR Max Snoop values determined?

PCI Firmware r3.3, sec 4.6.6, says the LTR _DSM reports the max values for each Downstream Port embedded in the platform, and the OS should calculate latencies along the path between each Downstream Port and any Upstream Port (Switch Upstream Port or Endpoint).

Of course, Switches not embedded in the platform (e.g., external Thunderbolt hierarchies) will not have this _DSM, but I assume they should contribute to this sum?

A: The fundamental problem is that there is no practical way for
software to discover the AC coupling capacitor sizes and common mode bias circuit impedance.

Software could compute conservative values, but they would likely be 10x worse than typical, so the L1.2 exit latency would be significantly longer than actually required to be.

The interoperability issues here were understood when designing L1 Substates, but no viable solution was found.

So the main reason Puranjay's work got stalled is that I didn't feel confident enough that we understood how to do this, especially for external devices.

It would be great if somebody *did* feel confident about interpreting and implementing all this.

Bjorn

David E. Box

2024-02-07 19:51 UTC #7 lore raw

Quoted 97 lines · Bjorn Helgaas

Quoted 50 lines · David E. Box
Quoted 41 lines · David E. Box
Quoted 40 lines · Bjorn Helgaas
Quoted 38 lines · David E. Box
Quoted 1 line · Bjorn Helgaas
On Fri, Feb 02, 2024 at 03:11:12PM +0800, Jian-Hong Pan wrote:
...
Quoted 22 lines
Review static int vmd_pm_enable_quirk(struct pci_dev · line 777
Quoted 6 lines
(quoted diff) +0 -0
@@ -775,6 +773,14 @@ static int vmd_pm_enable_quirk(struct pci_dev
 *pdev,
 void *userdata)
         pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT,
 ltr_reg);
         pci_info(pdev, "VMD: Default LTR value set by driver\n");
You're not changing this part, and I don't understand exactly how LTR
works, but it makes me a little bit queasy to read "set the LTR value
to the maximum required to allow the deepest power management savings" and then we set the max snoop values to a fixed constant.
I don't think the goal is to "allow the deepest power savings"; I think it's to enable L1.2 *when the device has enough buffering to absorb L1.2 entry/exit latencies*.
The spec (PCIe r6.0, sec 7.8.2.2) says "Software should set this to the platform's maximum supported latency or less," so it seems like that value must be platform-dependent, not fixed.
Quoted 5 lines
And I assume the "_DSM for Latency Tolerance Reporting" is part of the
way to get those platform-dependent values, but Linux doesn't actually
use that yet.
This may indeed be the best way but we need to double check with our BIOS folks. AFAIK BIOS writes the LTR values directly so there hasn't been a need to use this _DSM. But under VMD the ports are hidden from BIOS which is why we added it here. I've brought up the question internally to find out how Windows handles the DSM and to get a recommendation from our firmware leads.
Quoted 7 lines
Quoted 4 lines
We want Linux to be able to program LTR itself, don't we? We shouldn't have to rely on firmware to do it. If Linux can't do it, hot-added devices aren't going to be able to use L1.2, right?
Agreed. We just want to make sure we are not conflicting with what BIOS may be doing.
Quoted 6 lines
So the feedback is to run the _DSM and just overwrite any BIOS values. Looking up the _DSM I saw there was an attempt to upstream this 4 years ago [1]. I'm not sure why the effort stalled but we can pick up this work again.
https://patchwork.kernel.org/project/linux-pci/patch/[email protected]/
There was a PCI SIG discussion about this a few years ago that never really seemed to get resolved:
https://members.pcisig.com/wg/PCIe-Protocol/mail/thread/35064
Unfortunately that discussion is not public, but the summary is:
Q: How is the LTR_L1.2_THRESHOLD value determined?
     PCIe r5.0, sec 5.5.4, says the same value must be programmed into both Ports.
     A: As noted in sec 5.5.4, the value is determined primarily by the amount of time it will take to re-establish the common mode bias on the AC coupling caps, and it is assumed that the BIOS knows this.
Q: How are the LTR Max Snoop values determined?
     PCI Firmware r3.3, sec 4.6.6, says the LTR _DSM reports the max values for each Downstream Port embedded in the platform, and the OS should calculate latencies along the path between each Downstream Port and any Upstream Port (Switch Upstream Port or Endpoint).
     Of course, Switches not embedded in the platform (e.g., external Thunderbolt hierarchies) will not have this _DSM, but I assume they should contribute to this sum?
     A: The fundamental problem is that there is no practical way for software to discover the AC coupling capacitor sizes and common mode bias circuit impedance.
        Software could compute conservative values, but they would likely be 10x worse than typical, so the L1.2 exit latency would be significantly longer than actually required to be.
        The interoperability issues here were understood when designing L1 Substates, but no viable solution was found.

Quoted 6 lines

So the main reason Puranjay's work got stalled is that I didn't feel confident enough that we understood how to do this, especially for external devices.
It would be great if somebody *did* feel confident about interpreting and implementing all this.

As it is BIOS (at least Intel BIOS) is already writing the maximum allowed LTR value on Upstream Ports that have it set to 0. So we can't do any worse if we write the BIOS provided _DSM value for all Upstream Ports, including external devices. Sounds like the worst case scenario is that devices take longer than needed to exit L1.2 (I'm still asking about this detail). But I think this is better than not programming the LTR at all which could prevent the platform from power gating they very resources that LTR is meant to help manage.

If that reasoning is okay with you, I'll submit patches to use the _DSM.

David

Ilpo Järvinen

2024-02-06 12:29 UTC #8 lore raw

In reply to Jian-Hong Pan

Quoted 60 lines · Jian-Hong Pan

The remapped PCIe Root Port and NVMe have PCI PM L1 substates capability, but they are disabled originally:
Here is an example on ASUS B1400CEAE:
Capabilities: [900 v1] L1 PM Substates
        L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1- L1_PM_Substates+
                  PortCommonModeRestoreTime=32us PortTPowerOnTime=10us
        L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1-
                   T_CommonMode=0us LTR1.2_Threshold=0ns
        L1SubCtl2: T_PwrOn=10us
Power on all of the VMD remapped PCI devices and quirk max snoop LTR before enable PCI-PM L1 PM Substates by following "Section 5.5.4 of PCIe Base Spec Revision 6.0". Then, PCI PM's L1 substates control are initialized & enabled accordingly. Also, update the comments of pci_enable_link_state() and pci_enable_link_state_locked() for kernel-doc, too.
Link https://bugzilla.kernel.org/show_bug.cgi?id=218394
Signed-off-by Jian-Hong Pan <[email protected]>
---
v2:
- Power on the VMD remapped devices with pci_set_power_state_locked()
- Prepare the PCIe LTR parameters before enable L1 Substates
- Add note into the comments of both pci_enable_link_state() and pci_enable_link_state_locked() for kernel-doc.
- The original patch set can be split as individual patches.
 drivers/pci/controller/vmd.c | 15 ++++++++++-----
 drivers/pci/pcie/aspm.c      | 10 ++++++++++
 2 files changed, 20 insertions(+), 5 deletions(-)
drivers/pci/controller/vmd.c +2 -4
index 87b7856f375a..66e47a0dbf1a 100644
@@ -751,11 +751,9 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 	if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
 		return 0;
 
-	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
-
 	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
 	if (!pos)
-		return 0;
+		goto out_enable_link_state;
 
 	/*
 	 * Skip if the max snoop LTR is non-zero, indicating BIOS has set it
@@ -763,7 +761,7 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 	 */
 	pci_read_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &ltr_reg);
 	if (!!(ltr_reg & (PCI_LTR_VALUE_MASK | PCI_LTR_SCALE_MASK)))
-		return 0;
+		goto out_enable_link_state;
 
 	/*
 	 * Set the default values to the maximum required by the platform to

drivers/pci/controller/vmd.c static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata) · line 779

Quoted 7 lines

drivers/pci/controller/vmd.c +4 -0

@@ -775,6 +773,14 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 	pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
 	pci_info(pdev, "VMD: Default LTR value set by driver\n");
 
+out_enable_link_state:
+	/*
+	 * Make PCI devices at D0 when enable PCI-PM L1 PM Substates from
+	 * Section 5.5.4 of PCIe Base Spec Revision 6.0

I don't understand what are you trying to say here? Are there some typos or grammar errors or something entire missing from the comment?

Quoted 8 lines

drivers/pci/controller/vmd.c +4 -0

+	 */
+	pci_set_power_state_locked(pdev, PCI_D0);
+	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
+
 	return 0;
 }

drivers/pci/controller/vmd.c static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) · line 936

Quoted 6 lines

drivers/pci/controller/vmd.c +0 -1

@@ -926,7 +932,6 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 				   dev_get_msi_domain(&vmd->dev->dev));
 
 	vmd_acpi_begin();
-
 	pci_scan_child_bus(vmd->bus);
 	vmd_domain_reset(vmd);

Spurious newline change.

Signature · 1 line

i.

Bjorn Helgaas

2024-02-06 16:00 UTC #9 lore raw

Quoted 10 lines · Ilpo Järvinen

On Fri, 2 Feb 2024, Jian-Hong Pan wrote:
...
drivers/pci/controller/vmd.c static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata) · line 779
Quoted 4 lines
drivers/pci/controller/vmd.c +4 -0
+out_enable_link_state:
+	/*
+	 * Make PCI devices at D0 when enable PCI-PM L1 PM Substates from
+	 * Section 5.5.4 of PCIe Base Spec Revision 6.0
I don't understand what are you trying to say here? Are there some typos or grammar errors or something entire missing from the comment?

This is about the fact that per sec 5.5.4, "If setting either or both of the enable bits for PCI-PM L1 PM Substates, both ports must be configured as described in this section while in D0."

We can wordsmith this a little, maybe:

Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per PCIe r6.0, sec 5.5.4.

I look a little askance at having to do this separately from pci_enable_link_state_locked(), but we can solve that elsewhere if need be.

Quoted 2 lines

drivers/pci/controller/vmd.c +2 -0

+	pci_set_power_state_locked(pdev, PCI_D0);
+	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);