diff mbox series

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

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

Comments

Bjorn Helgaas Sept. 24, 2020, 10:51 p.m. UTC | #1 
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
>
diff mbox series

Patch

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