diff mbox series

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

Message ID 20200924142443.260861-4-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 
- 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(-)

Comments

Bjorn Helgaas Sept. 24, 2020, 10:36 p.m. UTC | #1 
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
>
kernel test robot Sept. 25, 2020, 4:22 a.m. UTC | #2 
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
diff mbox series

Patch

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