diff mbox series

[v2,1/7] PCI/ASPM: Cache device's ASPM link capability in struct pci_dev

Message ID 20200924142443.260861-2-refactormyself@gmail.com (mailing list archive)
State Changes Requested, archived
Delegated to: Bjorn Helgaas
Headers show
Series PCI/ASPM: Move some ASPM info to struct pci_dev | expand

Commit Message

Saheed O. Bolarinwa Sept. 24, 2020, 2:24 p.m. UTC
pcie_get_aspm_reg() reads LNKCAP to learn whether the device supports
ASPM L0s and/or L1 and L1 substates.

If we cache the entire LNKCAP word early enough, we may be able to
use it in other places that read LNKCAP, e.g. pcie_get_speed_cap(),
pcie_get_width_cap(), pcie_init(), etc.

 - Add struct pci_dev.lnkcap (u32)
 - Read PCI_EXP_LNKCAP in set_pcie_port_type() and save it
   in pci_dev.lnkcap
 - Use pdev->lnkcap instead of reading PCI_EXP_LNKCAP

Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
---
 drivers/pci/pcie/aspm.c | 7 ++-----
 drivers/pci/probe.c     | 1 +
 include/linux/pci.h     | 1 +
 3 files changed, 4 insertions(+), 5 deletions(-)

Comments

Bjorn Helgaas Sept. 24, 2020, 10:28 p.m. UTC | #1
On Thu, Sep 24, 2020 at 04:24:37PM +0200, Saheed O. Bolarinwa wrote:
> pcie_get_aspm_reg() reads LNKCAP to learn whether the device supports
> ASPM L0s and/or L1 and L1 substates.

I'm working from this v2 series.  But it's always nice if you include
a cover letter (as you did for the series you posted yesterday) and if
the cover letter includes a note about what changed from the previous
versions.

Bjorn
Bjorn Helgaas Sept. 24, 2020, 10:32 p.m. UTC | #2
On Thu, Sep 24, 2020 at 04:24:37PM +0200, Saheed O. Bolarinwa wrote:
> pcie_get_aspm_reg() reads LNKCAP to learn whether the device supports
> ASPM L0s and/or L1 and L1 substates.
> 
> If we cache the entire LNKCAP word early enough, we may be able to
> use it in other places that read LNKCAP, e.g. pcie_get_speed_cap(),
> pcie_get_width_cap(), pcie_init(), etc.
> 
>  - Add struct pci_dev.lnkcap (u32)
>  - Read PCI_EXP_LNKCAP in set_pcie_port_type() and save it
>    in pci_dev.lnkcap
>  - Use pdev->lnkcap instead of reading PCI_EXP_LNKCAP

I think we might as well go ahead and use the cached copy in these
other places in this patch, i.e.,

  pcie_init
  pcie_get_speed_cap
  pcie_get_width_cap
  pcie_link_bandwidth_notification_supported
Bjorn Helgaas Sept. 24, 2020, 10:53 p.m. UTC | #3
On Thu, Sep 24, 2020 at 04:24:37PM +0200, Saheed O. Bolarinwa wrote:
> pcie_get_aspm_reg() reads LNKCAP to learn whether the device supports
> ASPM L0s and/or L1 and L1 substates.
> 
> If we cache the entire LNKCAP word early enough, we may be able to
> use it in other places that read LNKCAP, e.g. pcie_get_speed_cap(),
> pcie_get_width_cap(), pcie_init(), etc.
> 
>  - Add struct pci_dev.lnkcap (u32)
>  - Read PCI_EXP_LNKCAP in set_pcie_port_type() and save it
>    in pci_dev.lnkcap
>  - Use pdev->lnkcap instead of reading PCI_EXP_LNKCAP

I think we need to be a little careful here because there's a note in
the spec (PCIe r5.0, sec 7.5.3.6):

  Note that exit latencies may be influenced by PCI Express reference
  clock configuration depending upon whether a component uses a common
  or separate reference clock.

So if we change the common clock configuration, e.g., in
pcie_aspm_configure_common_clock() or anything else that writes
PCI_EXP_LNKCTL_CCC, I think we will need to update pdev->lnkcap.
diff mbox series

Patch

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 253c30cc1967..d7e69b3595a0 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -177,15 +177,13 @@  static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
 static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	int capable = 1, enabled = 1;
-	u32 reg32;
 	u16 reg16;
 	struct pci_dev *child;
 	struct pci_bus *linkbus = link->pdev->subordinate;
 
 	/* All functions should have the same cap and state, take the worst */
 	list_for_each_entry(child, &linkbus->devices, bus_list) {
-		pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &reg32);
-		if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
+		if (!(child->lnkcap & PCI_EXP_LNKCAP_CLKPM)) {
 			capable = 0;
 			enabled = 0;
 			break;
@@ -397,9 +395,8 @@  static void pcie_get_aspm_reg(struct pci_dev *pdev,
 			      struct aspm_register_info *info)
 {
 	u16 reg16;
-	u32 reg32;
+	u32 reg32 = pdev->lnkcap;
 
-	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &reg32);
 	info->support = (reg32 & PCI_EXP_LNKCAP_ASPMS) >> 10;
 	info->latency_encoding_l0s = (reg32 & PCI_EXP_LNKCAP_L0SEL) >> 12;
 	info->latency_encoding_l1  = (reg32 & PCI_EXP_LNKCAP_L1EL) >> 15;
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 03d37128a24f..2d5898f05f89 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1486,6 +1486,7 @@  void set_pcie_port_type(struct pci_dev *pdev)
 	pdev->pcie_flags_reg = reg16;
 	pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
 	pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
+	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &pdev->lnkcap);
 
 	parent = pci_upstream_bridge(pdev);
 	if (!parent)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 835530605c0d..5b305cfeb1dc 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -375,6 +375,7 @@  struct pci_dev {
 						   bit manually */
 	unsigned int	d3_delay;	/* D3->D0 transition time in ms */
 	unsigned int	d3cold_delay;	/* D3cold->D0 transition time in ms */
+	u32		lnkcap;		/* Link Capabilities */
 
 #ifdef CONFIG_PCIEASPM
 	struct pcie_link_state	*link_state;	/* ASPM link state */