[PATCH v2 0/7] PCI/ASPM: Move some ASPM info to struct pci_dev

[PATCH v2 0/7] PCI/ASPM: Move some ASPM info to struct pci_dev

[Saheed O. Bolarinwa]

These series migrates some ASPM information into struct pci_dev.
struct pcie_link_state.l1ss and struct aspm_register_info were
removed.

Changes since version 1:
 - Rework patches to remove redundant variables in the same patch.

Saheed O. Bolarinwa (7):
  PCI/ASPM: Cache device's ASPM link capability in struct pci_dev
  PCI/ASPM: Rework calc_l*_latency() to take a struct pci_dev
  PCI/ASPM: Compute the value of aspm_register_info.support directly
  PCI/ASPM: Replace aspm_register_info.l1ss_cap* with pci_dev.l1ss_cap*
  PCI/ASPM: Remove aspm_register_info.l1ss_ctl*
  PCI/ASPM: Remove struct aspm_register_info and pcie_get_aspm_reg()
  PCI/ASPM: Remove struct pcie_link_state.l1ss

 drivers/pci/pcie/aspm.c | 203 +++++++++++++++-------------------------
 drivers/pci/probe.c     |   7 ++
 include/linux/pci.h     |   3 +
 3 files changed, 84 insertions(+), 129 deletions(-)

-- 
2.18.4

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

[Saheed O. Bolarinwa]

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

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 */
-- 
2.18.4

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

[Bjorn Helgaas]

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

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

[Bjorn Helgaas]

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

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

[Bjorn Helgaas]

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.

[PATCH v2 2/7] PCI/ASPM: Rework calc_l*_latency() to take a struct pci_dev

[Saheed O. Bolarinwa]

 - Change the argument of calc_l0s_latency() to  pci_dev *,
 - Compute latency_encoding_l0s encoding inside calc_l0s_latency()
 - Compute latency_encoding_l1 encoding inside calc_l1_latency()
 - Make calc_l*_latency() take only pci_dev *,
 - Make callers to calc_l0s_latency() and calc_l1_latency() pass
   in struct pci_dev
 - In pcie_get_aspm_reg() remove assignments to the latency encodings
 - Remove aspm_register_info.latency_encoding_l1
 - Remove aspm_register_info.latency_encoding_l0s

Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
---
 drivers/pci/pcie/aspm.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index d7e69b3595a0..5f7cf47b6a40 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -306,8 +306,10 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
 }
 
 /* Convert L0s latency encoding to ns */
-static u32 calc_l0s_latency(u32 encoding)
+static u32 calc_l0s_latency(struct pci_dev *pdev)
 {
+	u32 encoding = (pdev->lnkcap & PCI_EXP_LNKCAP_L0SEL) >> 12;
+
 	if (encoding == 0x7)
 		return (5 * 1000);	/* > 4us */
 	return (64 << encoding);
@@ -322,8 +324,10 @@ static u32 calc_l0s_acceptable(u32 encoding)
 }
 
 /* Convert L1 latency encoding to ns */
-static u32 calc_l1_latency(u32 encoding)
+static u32 calc_l1_latency(struct pci_dev *pdev)
 {
+	u32 encoding = (pdev->lnkcap & PCI_EXP_LNKCAP_L1EL) >> 15;
+
 	if (encoding == 0x7)
 		return (65 * 1000);	/* > 64us */
 	return (1000 << encoding);
@@ -381,8 +385,6 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
 struct aspm_register_info {
 	u32 support:2;
 	u32 enabled:2;
-	u32 latency_encoding_l0s;
-	u32 latency_encoding_l1;
 
 	/* L1 substates */
 	u32 l1ss_cap_ptr;
@@ -398,8 +400,6 @@ static void pcie_get_aspm_reg(struct pci_dev *pdev,
 	u32 reg32 = pdev->lnkcap;
 
 	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;
 	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &reg16);
 	info->enabled = reg16 & PCI_EXP_LNKCTL_ASPMC;
 
@@ -587,16 +587,16 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 		link->aspm_enabled |= 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);
-	link->latency_dw.l0s = calc_l0s_latency(dwreg.latency_encoding_l0s);
+	link->latency_up.l0s = calc_l0s_latency(parent);
+	link->latency_dw.l0s = calc_l0s_latency(child);
 
 	/* Setup L1 state */
 	if (upreg.support & dwreg.support & PCIE_LINK_STATE_L1)
 		link->aspm_support |= ASPM_STATE_L1;
 	if (upreg.enabled & dwreg.enabled & PCIE_LINK_STATE_L1)
 		link->aspm_enabled |= ASPM_STATE_L1;
-	link->latency_up.l1 = calc_l1_latency(upreg.latency_encoding_l1);
-	link->latency_dw.l1 = calc_l1_latency(dwreg.latency_encoding_l1);
+	link->latency_up.l1 = calc_l1_latency(parent);
+	link->latency_dw.l1 = calc_l1_latency(child);
 
 	/* Setup L1 substate */
 	if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_ASPM_L1_1)
-- 
2.18.4

[PATCH v2 3/7] PCI/ASPM: Compute the value of aspm_register_info.support directly

[Saheed O. Bolarinwa]

 - Calculate aspm_register_info.support inside aspm_support()
 - Replace references to aspm_register_info.support with aspm_support().
 - In pcie_get_aspm_reg() remove assignment to aspm_register_info.support
 - Remove aspm_register_info.support

Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
---
 drivers/pci/pcie/aspm.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 5f7cf47b6a40..321b328347c1 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -383,7 +383,6 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
 }
 
 struct aspm_register_info {
-	u32 support:2;
 	u32 enabled:2;
 
 	/* L1 substates */
@@ -396,12 +395,10 @@ struct aspm_register_info {
 static void pcie_get_aspm_reg(struct pci_dev *pdev,
 			      struct aspm_register_info *info)
 {
-	u16 reg16;
-	u32 reg32 = pdev->lnkcap;
+	u16 ctl;
 
-	info->support = (reg32 & PCI_EXP_LNKCAP_ASPMS) >> 10;
-	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &reg16);
-	info->enabled = reg16 & PCI_EXP_LNKCTL_ASPMC;
+	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl);
+	info->enabled = ctl & PCI_EXP_LNKCTL_ASPMC;
 
 	/* Read L1 PM substate capabilities */
 	info->l1ss_cap = info->l1ss_ctl1 = info->l1ss_ctl2 = 0;
@@ -540,6 +537,11 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 	link->l1ss.ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
 }
 
+static void aspm_support(struct pci_dev *pdev)
+{
+	return (pdev->lnkcap & PCI_EXP_LNKCAP_ASPMS) >> 10;
+}
+
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
@@ -561,7 +563,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	 * If ASPM not supported, don't mess with the clocks and link,
 	 * bail out now.
 	 */
-	if (!(upreg.support & dwreg.support))
+	if (!(aspm_support(parent) & aspm_support(child)))
 		return;
 
 	/* Configure common clock before checking latencies */
@@ -581,8 +583,9 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	 * given link unless components on both sides of the link each
 	 * support L0s.
 	 */
-	if (dwreg.support & upreg.support & PCIE_LINK_STATE_L0S)
+	if (aspm_support(parent) & aspm_support(child) & PCIE_LINK_STATE_L0S)
 		link->aspm_support |= ASPM_STATE_L0S;
+
 	if (dwreg.enabled & PCIE_LINK_STATE_L0S)
 		link->aspm_enabled |= ASPM_STATE_L0S_UP;
 	if (upreg.enabled & PCIE_LINK_STATE_L0S)
@@ -591,8 +594,9 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	link->latency_dw.l0s = calc_l0s_latency(child);
 
 	/* Setup L1 state */
-	if (upreg.support & dwreg.support & PCIE_LINK_STATE_L1)
+	if (aspm_support(parent) & aspm_support(child) & PCIE_LINK_STATE_L1)
 		link->aspm_support |= ASPM_STATE_L1;
+
 	if (upreg.enabled & dwreg.enabled & PCIE_LINK_STATE_L1)
 		link->aspm_enabled |= ASPM_STATE_L1;
 	link->latency_up.l1 = calc_l1_latency(parent);
-- 
2.18.4

Re: [PATCH v2 3/7] PCI/ASPM: Compute the value of aspm_register_info.support directly

[Bjorn Helgaas]

On Thu, Sep 24, 2020 at 04:24:39PM +0200, Saheed O. Bolarinwa wrote:
>  - Calculate aspm_register_info.support inside aspm_support()
>  - Replace references to aspm_register_info.support with aspm_support().
>  - In pcie_get_aspm_reg() remove assignment to aspm_register_info.support
>  - Remove aspm_register_info.support
> 
> Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
> ---
>  drivers/pci/pcie/aspm.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 5f7cf47b6a40..321b328347c1 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -383,7 +383,6 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
>  }
>  
>  struct aspm_register_info {
> -	u32 support:2;
>  	u32 enabled:2;
>  
>  	/* L1 substates */
> @@ -396,12 +395,10 @@ struct aspm_register_info {
>  static void pcie_get_aspm_reg(struct pci_dev *pdev,
>  			      struct aspm_register_info *info)
>  {
> -	u16 reg16;
> -	u32 reg32 = pdev->lnkcap;
> +	u16 ctl;

Don't include unrelated changes.  The change from "reg16" to "ctl" is
gratuitous, and it makes this patch harder to read.  I think you
remove it later anyway.

> -	info->support = (reg32 & PCI_EXP_LNKCAP_ASPMS) >> 10;
> -	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &reg16);
> -	info->enabled = reg16 & PCI_EXP_LNKCTL_ASPMC;
> +	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl);
> +	info->enabled = ctl & PCI_EXP_LNKCTL_ASPMC;
>  
>  	/* Read L1 PM substate capabilities */
>  	info->l1ss_cap = info->l1ss_ctl1 = info->l1ss_ctl2 = 0;
> @@ -540,6 +537,11 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>  	link->l1ss.ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
>  }
>  
> +static void aspm_support(struct pci_dev *pdev)
> +{
> +	return (pdev->lnkcap & PCI_EXP_LNKCAP_ASPMS) >> 10;

Oops, this doesn't build!  Should return a u32.

> +}
> +
>  static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  {
>  	struct pci_dev *child = link->downstream, *parent = link->pdev;
> @@ -561,7 +563,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  	 * If ASPM not supported, don't mess with the clocks and link,
>  	 * bail out now.
>  	 */
> -	if (!(upreg.support & dwreg.support))
> +	if (!(aspm_support(parent) & aspm_support(child)))
>  		return;
>  
>  	/* Configure common clock before checking latencies */
> @@ -581,8 +583,9 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  	 * given link unless components on both sides of the link each
>  	 * support L0s.
>  	 */
> -	if (dwreg.support & upreg.support & PCIE_LINK_STATE_L0S)
> +	if (aspm_support(parent) & aspm_support(child) & PCIE_LINK_STATE_L0S)
>  		link->aspm_support |= ASPM_STATE_L0S;
> +
>  	if (dwreg.enabled & PCIE_LINK_STATE_L0S)
>  		link->aspm_enabled |= ASPM_STATE_L0S_UP;
>  	if (upreg.enabled & PCIE_LINK_STATE_L0S)
> @@ -591,8 +594,9 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  	link->latency_dw.l0s = calc_l0s_latency(child);
>  
>  	/* Setup L1 state */
> -	if (upreg.support & dwreg.support & PCIE_LINK_STATE_L1)
> +	if (aspm_support(parent) & aspm_support(child) & PCIE_LINK_STATE_L1)
>  		link->aspm_support |= ASPM_STATE_L1;
> +
>  	if (upreg.enabled & dwreg.enabled & PCIE_LINK_STATE_L1)
>  		link->aspm_enabled |= ASPM_STATE_L1;
>  	link->latency_up.l1 = calc_l1_latency(parent);
> -- 
> 2.18.4
> 

Re: [PATCH v2 3/7] PCI/ASPM: Compute the value of aspm_register_info.support directly

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 7441 bytes --]

Hi "Saheed,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pci/next]
[also build test ERROR on v5.9-rc6 next-20200924]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Saheed-O-Bolarinwa/PCI-ASPM-Cache-device-s-ASPM-link-capability-in-struct-pci_dev/20200924-232457
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: x86_64-randconfig-a005-20200923 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project c32e69b2ce7abfb151a87ba363ac9e25abf7d417)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/9cce0413425d28cbbb50a18bc01174c0f126632e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Saheed-O-Bolarinwa/PCI-ASPM-Cache-device-s-ASPM-link-capability-in-struct-pci_dev/20200924-232457
        git checkout 9cce0413425d28cbbb50a18bc01174c0f126632e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/pci/pcie/aspm.c:542:2: error: void function 'aspm_support' should not return a value [-Wreturn-type]
           return (pdev->lnkcap & PCI_EXP_LNKCAP_ASPMS) >> 10;
           ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/pci/pcie/aspm.c:566:29: error: invalid operands to binary expression ('void' and 'void')
           if (!(aspm_support(parent) & aspm_support(child)))
                 ~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~
   drivers/pci/pcie/aspm.c:586:27: error: invalid operands to binary expression ('void' and 'void')
           if (aspm_support(parent) & aspm_support(child) & PCIE_LINK_STATE_L0S)
               ~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~
   drivers/pci/pcie/aspm.c:597:27: error: invalid operands to binary expression ('void' and 'void')
           if (aspm_support(parent) & aspm_support(child) & PCIE_LINK_STATE_L1)
               ~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~
   4 errors generated.

vim +/aspm_support +542 drivers/pci/pcie/aspm.c

   539	
   540	static void aspm_support(struct pci_dev *pdev)
   541	{
 > 542		return (pdev->lnkcap & PCI_EXP_LNKCAP_ASPMS) >> 10;
   543	}
   544	
   545	static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
   546	{
   547		struct pci_dev *child = link->downstream, *parent = link->pdev;
   548		struct pci_bus *linkbus = parent->subordinate;
   549		struct aspm_register_info upreg, dwreg;
   550	
   551		if (blacklist) {
   552			/* Set enabled/disable so that we will disable ASPM later */
   553			link->aspm_enabled = ASPM_STATE_ALL;
   554			link->aspm_disable = ASPM_STATE_ALL;
   555			return;
   556		}
   557	
   558		/* Get upstream/downstream components' register state */
   559		pcie_get_aspm_reg(parent, &upreg);
   560		pcie_get_aspm_reg(child, &dwreg);
   561	
   562		/*
   563		 * If ASPM not supported, don't mess with the clocks and link,
   564		 * bail out now.
   565		 */
 > 566		if (!(aspm_support(parent) & aspm_support(child)))
   567			return;
   568	
   569		/* Configure common clock before checking latencies */
   570		pcie_aspm_configure_common_clock(link);
   571	
   572		/*
   573		 * Re-read upstream/downstream components' register state
   574		 * after clock configuration
   575		 */
   576		pcie_get_aspm_reg(parent, &upreg);
   577		pcie_get_aspm_reg(child, &dwreg);
   578	
   579		/*
   580		 * Setup L0s state
   581		 *
   582		 * Note that we must not enable L0s in either direction on a
   583		 * given link unless components on both sides of the link each
   584		 * support L0s.
   585		 */
   586		if (aspm_support(parent) & aspm_support(child) & PCIE_LINK_STATE_L0S)
   587			link->aspm_support |= ASPM_STATE_L0S;
   588	
   589		if (dwreg.enabled & PCIE_LINK_STATE_L0S)
   590			link->aspm_enabled |= ASPM_STATE_L0S_UP;
   591		if (upreg.enabled & PCIE_LINK_STATE_L0S)
   592			link->aspm_enabled |= ASPM_STATE_L0S_DW;
   593		link->latency_up.l0s = calc_l0s_latency(parent);
   594		link->latency_dw.l0s = calc_l0s_latency(child);
   595	
   596		/* Setup L1 state */
   597		if (aspm_support(parent) & aspm_support(child) & PCIE_LINK_STATE_L1)
   598			link->aspm_support |= ASPM_STATE_L1;
   599	
   600		if (upreg.enabled & dwreg.enabled & PCIE_LINK_STATE_L1)
   601			link->aspm_enabled |= ASPM_STATE_L1;
   602		link->latency_up.l1 = calc_l1_latency(parent);
   603		link->latency_dw.l1 = calc_l1_latency(child);
   604	
   605		/* Setup L1 substate */
   606		if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_ASPM_L1_1)
   607			link->aspm_support |= ASPM_STATE_L1_1;
   608		if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_ASPM_L1_2)
   609			link->aspm_support |= ASPM_STATE_L1_2;
   610		if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_1)
   611			link->aspm_support |= ASPM_STATE_L1_1_PCIPM;
   612		if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_2)
   613			link->aspm_support |= ASPM_STATE_L1_2_PCIPM;
   614	
   615		if (upreg.l1ss_ctl1 & dwreg.l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_1)
   616			link->aspm_enabled |= ASPM_STATE_L1_1;
   617		if (upreg.l1ss_ctl1 & dwreg.l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_2)
   618			link->aspm_enabled |= ASPM_STATE_L1_2;
   619		if (upreg.l1ss_ctl1 & dwreg.l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_1)
   620			link->aspm_enabled |= ASPM_STATE_L1_1_PCIPM;
   621		if (upreg.l1ss_ctl1 & dwreg.l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_2)
   622			link->aspm_enabled |= ASPM_STATE_L1_2_PCIPM;
   623	
   624		if (link->aspm_support & ASPM_STATE_L1SS)
   625			aspm_calc_l1ss_info(link, &upreg, &dwreg);
   626	
   627		/* Save default state */
   628		link->aspm_default = link->aspm_enabled;
   629	
   630		/* Setup initial capable state. Will be updated later */
   631		link->aspm_capable = link->aspm_support;
   632	
   633		/* Get and check endpoint acceptable latencies */
   634		list_for_each_entry(child, &linkbus->devices, bus_list) {
   635			u32 reg32, encoding;
   636			struct aspm_latency *acceptable =
   637				&link->acceptable[PCI_FUNC(child->devfn)];
   638	
   639			if (pci_pcie_type(child) != PCI_EXP_TYPE_ENDPOINT &&
   640			    pci_pcie_type(child) != PCI_EXP_TYPE_LEG_END)
   641				continue;
   642	
   643			pcie_capability_read_dword(child, PCI_EXP_DEVCAP, &reg32);
   644			/* Calculate endpoint L0s acceptable latency */
   645			encoding = (reg32 & PCI_EXP_DEVCAP_L0S) >> 6;
   646			acceptable->l0s = calc_l0s_acceptable(encoding);
   647			/* Calculate endpoint L1 acceptable latency */
   648			encoding = (reg32 & PCI_EXP_DEVCAP_L1) >> 9;
   649			acceptable->l1 = calc_l1_acceptable(encoding);
   650	
   651			pcie_aspm_check_latency(child);
   652		}
   653	}
   654	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36645 bytes --]

[PATCH v2 4/7] PCI/ASPM: Replace aspm_register_info.l1ss_cap*

[Saheed O. Bolarinwa]

 - Add l1ss_cap and l1ss_cap_ptr to struct pci_dev
 - In pci_configure_ltr(), compute the value of pci_dev.l1ss_cap
   and pci_dev.l1ss_cap_ptr
 - Replace all references to aspm_register_info.(l1ss_cap && l1ss_cap_ptr)
   with pci_dev.(l1ss_cap && l1ss_cap_ptr)
 - Remove the now redundant parameters of aspm_calc_l1ss_info()
 - Change callers of aspm_calc_l1ss_info() to use the new signature
 - In pcie_get_aspm_reg() remove reference to aspm_register_info.l1ss_cap*
 - In pcie_get_aspm_reg() remove reading of l1ss_cap_ptr and l1ss_cap
 - Remove aspm_register_info.(l1ss_cap && l1ss_cap_ptr)

Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
---
 drivers/pci/pcie/aspm.c | 62 +++++++++++++----------------------------
 drivers/pci/probe.c     |  6 ++++
 include/linux/pci.h     |  2 ++
 3 files changed, 28 insertions(+), 42 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 321b328347c1..e7bb7d069361 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -386,8 +386,6 @@ struct aspm_register_info {
 	u32 enabled:2;
 
 	/* L1 substates */
-	u32 l1ss_cap_ptr;
-	u32 l1ss_cap;
 	u32 l1ss_ctl1;
 	u32 l1ss_ctl2;
 };
@@ -400,26 +398,6 @@ static void pcie_get_aspm_reg(struct pci_dev *pdev,
 	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl);
 	info->enabled = ctl & PCI_EXP_LNKCTL_ASPMC;
 
-	/* Read L1 PM substate capabilities */
-	info->l1ss_cap = info->l1ss_ctl1 = info->l1ss_ctl2 = 0;
-	info->l1ss_cap_ptr = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
-	if (!info->l1ss_cap_ptr)
-		return;
-	pci_read_config_dword(pdev, info->l1ss_cap_ptr + PCI_L1SS_CAP,
-			      &info->l1ss_cap);
-	if (!(info->l1ss_cap & PCI_L1SS_CAP_L1_PM_SS)) {
-		info->l1ss_cap = 0;
-		return;
-	}
-
-	/*
-	 * If we don't have LTR for the entire path from the Root Complex
-	 * to this device, we can't use ASPM L1.2 because it relies on the
-	 * LTR_L1.2_THRESHOLD.  See PCIe r4.0, secs 5.5.4, 6.18.
-	 */
-	if (!pdev->ltr_path)
-		info->l1ss_cap &= ~PCI_L1SS_CAP_ASPM_L1_2;
-
 	pci_read_config_dword(pdev, info->l1ss_cap_ptr + PCI_L1SS_CTL1,
 			      &info->l1ss_ctl1);
 	pci_read_config_dword(pdev, info->l1ss_cap_ptr + PCI_L1SS_CTL2,
@@ -488,38 +466,38 @@ static struct pci_dev *pci_function_0(struct pci_bus *linkbus)
 }
 
 /* Calculate L1.2 PM substate timing parameters */
-static void aspm_calc_l1ss_info(struct pcie_link_state *link,
-				struct aspm_register_info *upreg,
-				struct aspm_register_info *dwreg)
+static void aspm_calc_l1ss_info(struct pcie_link_state *link)
 {
 	u32 val1, val2, scale1, scale2;
 	u32 t_common_mode, t_power_on, l1_2_threshold, scale, value;
+	struct pci_dev *dw_pdev = link->downstream;
+	struct pci_dev *up_pdev = link->pdev;
 
-	link->l1ss.up_cap_ptr = upreg->l1ss_cap_ptr;
-	link->l1ss.dw_cap_ptr = dwreg->l1ss_cap_ptr;
+	link->l1ss.up_cap_ptr = up_pdev->l1ss_cap_ptr;
+	link->l1ss.dw_cap_ptr = dw_pdev->l1ss_cap_ptr;
 	link->l1ss.ctl1 = link->l1ss.ctl2 = 0;
 
 	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
 		return;
 
 	/* 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;
+	val1 = (up_pdev->l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
+	val2 = (dw_pdev->l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
 	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;
-	scale1 = (upreg->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
-	val2   = (dwreg->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
-	scale2 = (dwreg->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
+	val1   = (up_pdev->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
+	scale1 = (up_pdev->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
+	val2   = (dw_pdev->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
+	scale2 = (dw_pdev->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)) {
+	if (calc_l1ss_pwron(up_pdev, scale1, val1) >
+	    calc_l1ss_pwron(dw_pdev, scale2, val2)) {
 		link->l1ss.ctl2 |= scale1 | (val1 << 3);
-		t_power_on = calc_l1ss_pwron(link->pdev, scale1, val1);
+		t_power_on = calc_l1ss_pwron(up_pdev, scale1, val1);
 	} else {
 		link->l1ss.ctl2 |= scale2 | (val2 << 3);
-		t_power_on = calc_l1ss_pwron(link->downstream, scale2, val2);
+		t_power_on = calc_l1ss_pwron(dw_pdev, scale2, val2);
 	}
 
 	/*
@@ -603,13 +581,13 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	link->latency_dw.l1 = calc_l1_latency(child);
 
 	/* Setup L1 substate */
-	if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_ASPM_L1_1)
+	if (parent->l1ss_cap & child->l1ss_cap & PCI_L1SS_CAP_ASPM_L1_1)
 		link->aspm_support |= ASPM_STATE_L1_1;
-	if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_ASPM_L1_2)
+	if (parent->l1ss_cap & child->l1ss_cap & PCI_L1SS_CAP_ASPM_L1_2)
 		link->aspm_support |= ASPM_STATE_L1_2;
-	if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_1)
+	if (parent->l1ss_cap & child->l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_1)
 		link->aspm_support |= ASPM_STATE_L1_1_PCIPM;
-	if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_2)
+	if (parent->l1ss_cap & child->l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_2)
 		link->aspm_support |= ASPM_STATE_L1_2_PCIPM;
 
 	if (upreg.l1ss_ctl1 & dwreg.l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_1)
@@ -622,7 +600,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 		link->aspm_enabled |= ASPM_STATE_L1_2_PCIPM;
 
 	if (link->aspm_support & ASPM_STATE_L1SS)
-		aspm_calc_l1ss_info(link, &upreg, &dwreg);
+		aspm_calc_l1ss_info(link);
 
 	/* Save default state */
 	link->aspm_default = link->aspm_enabled;
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 2d5898f05f89..71a714065e14 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2107,6 +2107,12 @@ static void pci_configure_ltr(struct pci_dev *dev)
 	if (!pci_is_pcie(dev))
 		return;
 
+	/* Read L1 PM substate capabilities */
+	dev->l1ss_cap_ptr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_L1SS);
+	if (dev->l1ss_cap_ptr)
+		pci_read_config_dword(dev, dev->l1ss_cap_ptr + PCI_L1SS_CAP,
+								&dev->l1ss_cap);
+
 	pcie_capability_read_dword(dev, PCI_EXP_DEVCAP2, &cap);
 	if (!(cap & PCI_EXP_DEVCAP2_LTR))
 		return;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5b305cfeb1dc..60b82e255738 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -381,6 +381,8 @@ struct pci_dev {
 	struct pcie_link_state	*link_state;	/* ASPM link state */
 	unsigned int	ltr_path:1;	/* Latency Tolerance Reporting
 					   supported from root to here */
+	int		l1ss_cap_ptr;	/* L1SS cap ptr, 0 if not supported */
+	u32		l1ss_cap;	/* L1 PM substate Capabilities */
 #endif
 	unsigned int	eetlp_prefix_path:1;	/* End-to-End TLP Prefix */
 
-- 
2.18.4

Re: [PATCH v2 4/7] PCI/ASPM: Replace aspm_register_info.l1ss_cap*

[Bjorn Helgaas]

On Thu, Sep 24, 2020 at 04:24:40PM +0200, Saheed O. Bolarinwa wrote:
>  - Add l1ss_cap and l1ss_cap_ptr to struct pci_dev
>  - In pci_configure_ltr(), compute the value of pci_dev.l1ss_cap
>    and pci_dev.l1ss_cap_ptr
>  - Replace all references to aspm_register_info.(l1ss_cap && l1ss_cap_ptr)
>    with pci_dev.(l1ss_cap && l1ss_cap_ptr)
>  - Remove the now redundant parameters of aspm_calc_l1ss_info()
>  - Change callers of aspm_calc_l1ss_info() to use the new signature
>  - In pcie_get_aspm_reg() remove reference to aspm_register_info.l1ss_cap*
>  - In pcie_get_aspm_reg() remove reading of l1ss_cap_ptr and l1ss_cap
>  - Remove aspm_register_info.(l1ss_cap && l1ss_cap_ptr)
> 
> Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
> ---
>  drivers/pci/pcie/aspm.c | 62 +++++++++++++----------------------------
>  drivers/pci/probe.c     |  6 ++++
>  include/linux/pci.h     |  2 ++
>  3 files changed, 28 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 321b328347c1..e7bb7d069361 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -386,8 +386,6 @@ struct aspm_register_info {
>  	u32 enabled:2;
>  
>  	/* L1 substates */
> -	u32 l1ss_cap_ptr;
> -	u32 l1ss_cap;
>  	u32 l1ss_ctl1;
>  	u32 l1ss_ctl2;
>  };
> @@ -400,26 +398,6 @@ static void pcie_get_aspm_reg(struct pci_dev *pdev,
>  	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl);
>  	info->enabled = ctl & PCI_EXP_LNKCTL_ASPMC;
>  
> -	/* Read L1 PM substate capabilities */
> -	info->l1ss_cap = info->l1ss_ctl1 = info->l1ss_ctl2 = 0;
> -	info->l1ss_cap_ptr = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
> -	if (!info->l1ss_cap_ptr)
> -		return;
> -	pci_read_config_dword(pdev, info->l1ss_cap_ptr + PCI_L1SS_CAP,
> -			      &info->l1ss_cap);
> -	if (!(info->l1ss_cap & PCI_L1SS_CAP_L1_PM_SS)) {

Where did this check for PCI_L1SS_CAP_L1_PM_SS go?  You removed it
from here, but the equivalent thing should happen somewhere else, or
you should explain why it's no longer needed.

> -		info->l1ss_cap = 0;
> -		return;
> -	}
> -
> -	/*
> -	 * If we don't have LTR for the entire path from the Root Complex
> -	 * to this device, we can't use ASPM L1.2 because it relies on the
> -	 * LTR_L1.2_THRESHOLD.  See PCIe r4.0, secs 5.5.4, 6.18.
> -	 */
> -	if (!pdev->ltr_path)
> -		info->l1ss_cap &= ~PCI_L1SS_CAP_ASPM_L1_2;

Where did this pdev->ltr_path check go?

I'm not sure, but this feels like this should be two separate patches:

  1) Move the PCI_EXT_CAP_ID_L1SS stuff from here to
  pci_configure_ltr(), and

  2) All the aspm_calc_l1ss_info() stuff below.

Maybe they can't be separated, but if they can be, they should be.

>  	pci_read_config_dword(pdev, info->l1ss_cap_ptr + PCI_L1SS_CTL1,
>  			      &info->l1ss_ctl1);
>  	pci_read_config_dword(pdev, info->l1ss_cap_ptr + PCI_L1SS_CTL2,
> @@ -488,38 +466,38 @@ static struct pci_dev *pci_function_0(struct pci_bus *linkbus)
>  }
>  
>  /* Calculate L1.2 PM substate timing parameters */
> -static void aspm_calc_l1ss_info(struct pcie_link_state *link,
> -				struct aspm_register_info *upreg,
> -				struct aspm_register_info *dwreg)
> +static void aspm_calc_l1ss_info(struct pcie_link_state *link)
>  {
>  	u32 val1, val2, scale1, scale2;
>  	u32 t_common_mode, t_power_on, l1_2_threshold, scale, value;
> +	struct pci_dev *dw_pdev = link->downstream;
> +	struct pci_dev *up_pdev = link->pdev;
>  
> -	link->l1ss.up_cap_ptr = upreg->l1ss_cap_ptr;
> -	link->l1ss.dw_cap_ptr = dwreg->l1ss_cap_ptr;
> +	link->l1ss.up_cap_ptr = up_pdev->l1ss_cap_ptr;
> +	link->l1ss.dw_cap_ptr = dw_pdev->l1ss_cap_ptr;
>  	link->l1ss.ctl1 = link->l1ss.ctl2 = 0;
>  
>  	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
>  		return;
>  
>  	/* 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;
> +	val1 = (up_pdev->l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
> +	val2 = (dw_pdev->l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
>  	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;
> -	scale1 = (upreg->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
> -	val2   = (dwreg->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
> -	scale2 = (dwreg->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
> +	val1   = (up_pdev->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
> +	scale1 = (up_pdev->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
> +	val2   = (dw_pdev->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
> +	scale2 = (dw_pdev->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)) {
> +	if (calc_l1ss_pwron(up_pdev, scale1, val1) >
> +	    calc_l1ss_pwron(dw_pdev, scale2, val2)) {
>  		link->l1ss.ctl2 |= scale1 | (val1 << 3);
> -		t_power_on = calc_l1ss_pwron(link->pdev, scale1, val1);
> +		t_power_on = calc_l1ss_pwron(up_pdev, scale1, val1);
>  	} else {
>  		link->l1ss.ctl2 |= scale2 | (val2 << 3);
> -		t_power_on = calc_l1ss_pwron(link->downstream, scale2, val2);
> +		t_power_on = calc_l1ss_pwron(dw_pdev, scale2, val2);
>  	}
>  
>  	/*
> @@ -603,13 +581,13 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  	link->latency_dw.l1 = calc_l1_latency(child);
>  
>  	/* Setup L1 substate */
> -	if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_ASPM_L1_1)
> +	if (parent->l1ss_cap & child->l1ss_cap & PCI_L1SS_CAP_ASPM_L1_1)
>  		link->aspm_support |= ASPM_STATE_L1_1;
> -	if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_ASPM_L1_2)
> +	if (parent->l1ss_cap & child->l1ss_cap & PCI_L1SS_CAP_ASPM_L1_2)
>  		link->aspm_support |= ASPM_STATE_L1_2;
> -	if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_1)
> +	if (parent->l1ss_cap & child->l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_1)
>  		link->aspm_support |= ASPM_STATE_L1_1_PCIPM;
> -	if (upreg.l1ss_cap & dwreg.l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_2)
> +	if (parent->l1ss_cap & child->l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_2)
>  		link->aspm_support |= ASPM_STATE_L1_2_PCIPM;
>  
>  	if (upreg.l1ss_ctl1 & dwreg.l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_1)
> @@ -622,7 +600,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  		link->aspm_enabled |= ASPM_STATE_L1_2_PCIPM;
>  
>  	if (link->aspm_support & ASPM_STATE_L1SS)
> -		aspm_calc_l1ss_info(link, &upreg, &dwreg);
> +		aspm_calc_l1ss_info(link);
>  
>  	/* Save default state */
>  	link->aspm_default = link->aspm_enabled;
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 2d5898f05f89..71a714065e14 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2107,6 +2107,12 @@ static void pci_configure_ltr(struct pci_dev *dev)
>  	if (!pci_is_pcie(dev))
>  		return;
>  
> +	/* Read L1 PM substate capabilities */
> +	dev->l1ss_cap_ptr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_L1SS);
> +	if (dev->l1ss_cap_ptr)
> +		pci_read_config_dword(dev, dev->l1ss_cap_ptr + PCI_L1SS_CAP,
> +								&dev->l1ss_cap);

Follow the usual indentation scheme.  &dev->l1ss_cap should line up
with the "dev" on the previous line.

>  	pcie_capability_read_dword(dev, PCI_EXP_DEVCAP2, &cap);
>  	if (!(cap & PCI_EXP_DEVCAP2_LTR))
>  		return;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 5b305cfeb1dc..60b82e255738 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -381,6 +381,8 @@ struct pci_dev {
>  	struct pcie_link_state	*link_state;	/* ASPM link state */
>  	unsigned int	ltr_path:1;	/* Latency Tolerance Reporting
>  					   supported from root to here */
> +	int		l1ss_cap_ptr;	/* L1SS cap ptr, 0 if not supported */
> +	u32		l1ss_cap;	/* L1 PM substate Capabilities */
>  #endif
>  	unsigned int	eetlp_prefix_path:1;	/* End-to-End TLP Prefix */
>  
> -- 
> 2.18.4
> 

[PATCH v2 5/7] PCI/ASPM: Remove aspm_register_info.l1ss_ctl*

[Saheed O. Bolarinwa]

 - Read the value of PCI_L1SS_CTL1 directly and cache in local variables.
 - Replace references to aspm_register_info.l1ss_ctl1 with the variables.
 - In pcie_get_aspm_reg() remove reference to aspm_register_info.l1ss_ctl*
 - In pcie_get_aspm_reg() remove reading PCI_L1SS_CTL1 and PCI_L1SS_CTL2
 - Remove aspm_register_info.(l1ss_ctl1 && l1ss_ctl2)

Note that aspm_register_info.l1ss_ctl2 is eliminated totally since it is
not used.

Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
---
 drivers/pci/pcie/aspm.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index e7bb7d069361..cec8acad6363 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -384,10 +384,6 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
 
 struct aspm_register_info {
 	u32 enabled:2;
-
-	/* L1 substates */
-	u32 l1ss_ctl1;
-	u32 l1ss_ctl2;
 };
 
 static void pcie_get_aspm_reg(struct pci_dev *pdev,
@@ -397,11 +393,6 @@ static void pcie_get_aspm_reg(struct pci_dev *pdev,
 
 	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl);
 	info->enabled = ctl & PCI_EXP_LNKCTL_ASPMC;
-
-	pci_read_config_dword(pdev, info->l1ss_cap_ptr + PCI_L1SS_CTL1,
-			      &info->l1ss_ctl1);
-	pci_read_config_dword(pdev, info->l1ss_cap_ptr + PCI_L1SS_CTL2,
-			      &info->l1ss_ctl2);
 }
 
 static void pcie_aspm_check_latency(struct pci_dev *endpoint)
@@ -525,6 +516,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
 	struct pci_bus *linkbus = parent->subordinate;
 	struct aspm_register_info upreg, dwreg;
+	u32 up_l1ss_ctl1, dw_l1ss_ctl1;
 
 	if (blacklist) {
 		/* Set enabled/disable so that we will disable ASPM later */
@@ -547,6 +539,11 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	/* Configure common clock before checking latencies */
 	pcie_aspm_configure_common_clock(link);
 
+	pci_read_config_dword(parent, parent->l1ss_cap_ptr + PCI_L1SS_CTL1,
+				&up_l1ss_ctl1);
+	pci_read_config_dword(child, child->l1ss_cap_ptr + PCI_L1SS_CTL1,
+				&dw_l1ss_ctl1);
+
 	/*
 	 * Re-read upstream/downstream components' register state
 	 * after clock configuration
@@ -590,13 +587,13 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	if (parent->l1ss_cap & child->l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_2)
 		link->aspm_support |= ASPM_STATE_L1_2_PCIPM;
 
-	if (upreg.l1ss_ctl1 & dwreg.l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_1)
+	if (up_l1ss_ctl1 & dw_l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_1)
 		link->aspm_enabled |= ASPM_STATE_L1_1;
-	if (upreg.l1ss_ctl1 & dwreg.l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_2)
+	if (up_l1ss_ctl1 & dw_l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_2)
 		link->aspm_enabled |= ASPM_STATE_L1_2;
-	if (upreg.l1ss_ctl1 & dwreg.l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_1)
+	if (up_l1ss_ctl1 & dw_l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_1)
 		link->aspm_enabled |= ASPM_STATE_L1_1_PCIPM;
-	if (upreg.l1ss_ctl1 & dwreg.l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_2)
+	if (up_l1ss_ctl1 & dw_l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_2)
 		link->aspm_enabled |= ASPM_STATE_L1_2_PCIPM;
 
 	if (link->aspm_support & ASPM_STATE_L1SS)
-- 
2.18.4

[PATCH v2 6/7] PCI/ASPM: Remove struct aspm_register_info and pcie_get_aspm_reg()

[Saheed O. Bolarinwa]

 - Create get_aspm_enable() to compute aspm_register_info.enable directly
 - Replace all aspm_register_info.enable references with get_aspm_enable()
 - Remove pcie_get_aspm_reg() and all calls to it. All the values are now
   calculated elsewhere.
 - Remove struct aspm_register_info and its references

Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
---
 drivers/pci/pcie/aspm.c | 40 ++++++++++++----------------------------
 1 file changed, 12 insertions(+), 28 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index cec8acad6363..f4fc2d65240c 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -382,19 +382,6 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
 	}
 }
 
-struct aspm_register_info {
-	u32 enabled:2;
-};
-
-static void pcie_get_aspm_reg(struct pci_dev *pdev,
-			      struct aspm_register_info *info)
-{
-	u16 ctl;
-
-	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl);
-	info->enabled = ctl & PCI_EXP_LNKCTL_ASPMC;
-}
-
 static void pcie_aspm_check_latency(struct pci_dev *endpoint)
 {
 	u32 latency, l1_switch_latency = 0;
@@ -511,11 +498,18 @@ static void aspm_support(struct pci_dev *pdev)
 	return (pdev->lnkcap & PCI_EXP_LNKCAP_ASPMS) >> 10;
 }
 
+static u32 get_aspm_enable(struct pci_dev *pdev)
+{
+	u16 ctl;
+
+	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl);
+	return (ctl & PCI_EXP_LNKCTL_ASPMC);
+}
+
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
 	struct pci_bus *linkbus = parent->subordinate;
-	struct aspm_register_info upreg, dwreg;
 	u32 up_l1ss_ctl1, dw_l1ss_ctl1;
 
 	if (blacklist) {
@@ -525,10 +519,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 		return;
 	}
 
-	/* Get upstream/downstream components' register state */
-	pcie_get_aspm_reg(parent, &upreg);
-	pcie_get_aspm_reg(child, &dwreg);
-
 	/*
 	 * If ASPM not supported, don't mess with the clocks and link,
 	 * bail out now.
@@ -544,13 +534,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	pci_read_config_dword(child, child->l1ss_cap_ptr + PCI_L1SS_CTL1,
 				&dw_l1ss_ctl1);
 
-	/*
-	 * Re-read upstream/downstream components' register state
-	 * after clock configuration
-	 */
-	pcie_get_aspm_reg(parent, &upreg);
-	pcie_get_aspm_reg(child, &dwreg);
-
 	/*
 	 * Setup L0s state
 	 *
@@ -561,9 +544,9 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	if (aspm_support(parent) & aspm_support(child) & PCIE_LINK_STATE_L0S)
 		link->aspm_support |= ASPM_STATE_L0S;
 
-	if (dwreg.enabled & PCIE_LINK_STATE_L0S)
+	if (get_aspm_enable(child) & PCIE_LINK_STATE_L0S)
 		link->aspm_enabled |= ASPM_STATE_L0S_UP;
-	if (upreg.enabled & PCIE_LINK_STATE_L0S)
+	if (get_aspm_enable(parent) & PCIE_LINK_STATE_L0S)
 		link->aspm_enabled |= ASPM_STATE_L0S_DW;
 	link->latency_up.l0s = calc_l0s_latency(parent);
 	link->latency_dw.l0s = calc_l0s_latency(child);
@@ -572,7 +555,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	if (aspm_support(parent) & aspm_support(child) & PCIE_LINK_STATE_L1)
 		link->aspm_support |= ASPM_STATE_L1;
 
-	if (upreg.enabled & dwreg.enabled & PCIE_LINK_STATE_L1)
+	if (get_aspm_enable(parent) & get_aspm_enable(child)
+				    & PCIE_LINK_STATE_L1)
 		link->aspm_enabled |= ASPM_STATE_L1;
 	link->latency_up.l1 = calc_l1_latency(parent);
 	link->latency_dw.l1 = calc_l1_latency(child);
-- 
2.18.4

Re: [PATCH v2 6/7] PCI/ASPM: Remove struct aspm_register_info and pcie_get_aspm_reg()

[Bjorn Helgaas]

On Thu, Sep 24, 2020 at 04:24:42PM +0200, Saheed O. Bolarinwa wrote:
>  - Create get_aspm_enable() to compute aspm_register_info.enable directly
>  - Replace all aspm_register_info.enable references with get_aspm_enable()
>  - Remove pcie_get_aspm_reg() and all calls to it. All the values are now
>    calculated elsewhere.
>  - Remove struct aspm_register_info and its references
> 
> Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
> ---
>  drivers/pci/pcie/aspm.c | 40 ++++++++++++----------------------------
>  1 file changed, 12 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index cec8acad6363..f4fc2d65240c 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -382,19 +382,6 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
>  	}
>  }
>  
> -struct aspm_register_info {
> -	u32 enabled:2;
> -};
> -
> -static void pcie_get_aspm_reg(struct pci_dev *pdev,
> -			      struct aspm_register_info *info)
> -{
> -	u16 ctl;
> -
> -	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl);
> -	info->enabled = ctl & PCI_EXP_LNKCTL_ASPMC;
> -}
> -
>  static void pcie_aspm_check_latency(struct pci_dev *endpoint)
>  {
>  	u32 latency, l1_switch_latency = 0;
> @@ -511,11 +498,18 @@ static void aspm_support(struct pci_dev *pdev)
>  	return (pdev->lnkcap & PCI_EXP_LNKCAP_ASPMS) >> 10;
>  }
>  
> +static u32 get_aspm_enable(struct pci_dev *pdev)

Shouldn't this return u16?

> +{
> +	u16 ctl;
> +
> +	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl);
> +	return (ctl & PCI_EXP_LNKCTL_ASPMC);
> +}
> +
>  static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  {
>  	struct pci_dev *child = link->downstream, *parent = link->pdev;
>  	struct pci_bus *linkbus = parent->subordinate;
> -	struct aspm_register_info upreg, dwreg;
>  	u32 up_l1ss_ctl1, dw_l1ss_ctl1;
>  
>  	if (blacklist) {
> @@ -525,10 +519,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  		return;
>  	}
>  
> -	/* Get upstream/downstream components' register state */
> -	pcie_get_aspm_reg(parent, &upreg);
> -	pcie_get_aspm_reg(child, &dwreg);
> -
>  	/*
>  	 * If ASPM not supported, don't mess with the clocks and link,
>  	 * bail out now.
> @@ -544,13 +534,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  	pci_read_config_dword(child, child->l1ss_cap_ptr + PCI_L1SS_CTL1,
>  				&dw_l1ss_ctl1);
>  
> -	/*
> -	 * Re-read upstream/downstream components' register state
> -	 * after clock configuration
> -	 */
> -	pcie_get_aspm_reg(parent, &upreg);
> -	pcie_get_aspm_reg(child, &dwreg);
> -
>  	/*
>  	 * Setup L0s state
>  	 *
> @@ -561,9 +544,9 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  	if (aspm_support(parent) & aspm_support(child) & PCIE_LINK_STATE_L0S)
>  		link->aspm_support |= ASPM_STATE_L0S;
>  
> -	if (dwreg.enabled & PCIE_LINK_STATE_L0S)
> +	if (get_aspm_enable(child) & PCIE_LINK_STATE_L0S)
>  		link->aspm_enabled |= ASPM_STATE_L0S_UP;
> -	if (upreg.enabled & PCIE_LINK_STATE_L0S)
> +	if (get_aspm_enable(parent) & PCIE_LINK_STATE_L0S)
>  		link->aspm_enabled |= ASPM_STATE_L0S_DW;
>  	link->latency_up.l0s = calc_l0s_latency(parent);
>  	link->latency_dw.l0s = calc_l0s_latency(child);
> @@ -572,7 +555,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  	if (aspm_support(parent) & aspm_support(child) & PCIE_LINK_STATE_L1)
>  		link->aspm_support |= ASPM_STATE_L1;
>  
> -	if (upreg.enabled & dwreg.enabled & PCIE_LINK_STATE_L1)
> +	if (get_aspm_enable(parent) & get_aspm_enable(child)
> +				    & PCIE_LINK_STATE_L1)

We just read these enable bits above, and I don't think they've
changed.  Can we just read them once?

>  		link->aspm_enabled |= ASPM_STATE_L1;
>  	link->latency_up.l1 = calc_l1_latency(parent);
>  	link->latency_dw.l1 = calc_l1_latency(child);
> -- 
> 2.18.4
> 

[PATCH v2 7/7] PCI/ASPM: Remove struct pcie_link_state.l1ss

[Saheed O. Bolarinwa]

pcie_link_state.l1ss.{up_cap_ptr, dw_cap_ptr} are used to cache the value
of l1ss_cap_ptr for upstream and downstream respectively. This value can
now be obtained directly from struct pci_dev, it is no longer useful to
cache it. So, aspm_calc_l1ss_info() will only be computing the values for
ctl1 and ctl2. The addresses of these can also be passed in.

Then if aspm_calc_l1ss_info() calculates pcie_link_state.l1ss.{ctl1, ctl2}
which are only used inside pcie_config_aspm_l1ss(). Calling the function
where it is needed will remove the need to cache the values in the struct.

 - Move call to aspm_calc_l1ss_info() from pcie_aspm_cap_init() to
   pcie_config_aspm_l1ss().
 - Rename aspm_calc_l1ss_info() to aspm_calc_l1ss_ctl_values().
 - Rework the function to take a pci_dev and pointers to ctl1 and ctl2.
 - Change calls to aspm_calc_l1ss_info() into new function.
 - Replace l1ss.{up,dw}_cap_ptr with pci_dev->l1ss_cap_ptr
 - Replace pcie_link_state.l1ss.{ctl1, ctl2} with local variables.
 - No more reference to struct pcie_link_state.l1ss, so remove it.
 - Remove pcie_link_state.l1ss

Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
---
 drivers/pci/pcie/aspm.c | 45 ++++++++++++++---------------------------
 1 file changed, 15 insertions(+), 30 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index f4fc2d65240c..b9bacdef8c80 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -74,14 +74,6 @@ struct pcie_link_state {
 	 * has one slot under it, so at most there are 8 functions.
 	 */
 	struct aspm_latency acceptable[8];
-
-	/* L1 PM Substate info */
-	struct {
-		u32 up_cap_ptr;		/* L1SS cap ptr in upstream dev */
-		u32 dw_cap_ptr;		/* L1SS cap ptr in downstream dev */
-		u32 ctl1;		/* value to be programmed in ctl1 */
-		u32 ctl2;		/* value to be programmed in ctl2 */
-	} l1ss;
 };
 
 static int aspm_disabled, aspm_force;
@@ -444,17 +436,15 @@ static struct pci_dev *pci_function_0(struct pci_bus *linkbus)
 }
 
 /* Calculate L1.2 PM substate timing parameters */
-static void aspm_calc_l1ss_info(struct pcie_link_state *link)
+static void aspm_calc_l1ss_ctl_values(struct pci_dev *pdev,
+					u32 *ctl1, u32 *ctl2)
 {
+	struct pcie_link_state *link = pdev->link_state;
 	u32 val1, val2, scale1, scale2;
 	u32 t_common_mode, t_power_on, l1_2_threshold, scale, value;
 	struct pci_dev *dw_pdev = link->downstream;
 	struct pci_dev *up_pdev = link->pdev;
 
-	link->l1ss.up_cap_ptr = up_pdev->l1ss_cap_ptr;
-	link->l1ss.dw_cap_ptr = dw_pdev->l1ss_cap_ptr;
-	link->l1ss.ctl1 = link->l1ss.ctl2 = 0;
-
 	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
 		return;
 
@@ -471,10 +461,10 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link)
 
 	if (calc_l1ss_pwron(up_pdev, scale1, val1) >
 	    calc_l1ss_pwron(dw_pdev, scale2, val2)) {
-		link->l1ss.ctl2 |= scale1 | (val1 << 3);
+		*ctl2 |= scale1 | (val1 << 3);
 		t_power_on = calc_l1ss_pwron(up_pdev, scale1, val1);
 	} else {
-		link->l1ss.ctl2 |= scale2 | (val2 << 3);
+		*ctl2 |= scale2 | (val2 << 3);
 		t_power_on = calc_l1ss_pwron(dw_pdev, scale2, val2);
 	}
 
@@ -490,7 +480,7 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link)
 	 */
 	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;
+	*ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
 }
 
 static void aspm_support(struct pci_dev *pdev)
@@ -580,9 +570,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	if (up_l1ss_ctl1 & dw_l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_2)
 		link->aspm_enabled |= ASPM_STATE_L1_2_PCIPM;
 
-	if (link->aspm_support & ASPM_STATE_L1SS)
-		aspm_calc_l1ss_info(link);
-
 	/* Save default state */
 	link->aspm_default = link->aspm_enabled;
 
@@ -625,12 +612,13 @@ static void pci_clear_and_set_dword(struct pci_dev *pdev, int pos,
 /* Configure the ASPM L1 substates */
 static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
 {
-	u32 val, enable_req;
+	u32 val, enable_req, ctl1, ctl2;
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
-	u32 up_cap_ptr = link->l1ss.up_cap_ptr;
-	u32 dw_cap_ptr = link->l1ss.dw_cap_ptr;
+	int up_cap_ptr = parent->l1ss_cap_ptr;
+	int dw_cap_ptr = child->l1ss_cap_ptr;
 
 	enable_req = (link->aspm_enabled ^ state) & state;
+	aspm_calc_l1ss_ctl_values(parent, &ctl1, &ctl2);
 
 	/*
 	 * Here are the rules specified in the PCIe spec for enabling L1SS:
@@ -665,24 +653,21 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
 
 		/* Program T_POWER_ON times in both ports */
 		pci_write_config_dword(parent, up_cap_ptr + PCI_L1SS_CTL2,
-				       link->l1ss.ctl2);
+								ctl2);
 		pci_write_config_dword(child, dw_cap_ptr + PCI_L1SS_CTL2,
-				       link->l1ss.ctl2);
+								ctl2);
 
 		/* Program Common_Mode_Restore_Time in upstream device */
 		pci_clear_and_set_dword(parent, up_cap_ptr + PCI_L1SS_CTL1,
-					PCI_L1SS_CTL1_CM_RESTORE_TIME,
-					link->l1ss.ctl1);
+					PCI_L1SS_CTL1_CM_RESTORE_TIME, ctl1);
 
 		/* Program LTR_L1.2_THRESHOLD time in both ports */
 		pci_clear_and_set_dword(parent,	up_cap_ptr + PCI_L1SS_CTL1,
 					PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
-					PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
-					link->l1ss.ctl1);
+					PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
 		pci_clear_and_set_dword(child, dw_cap_ptr + PCI_L1SS_CTL1,
 					PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
-					PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
-					link->l1ss.ctl1);
+					PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
 	}
 
 	val = 0;
-- 
2.18.4