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

[Bjorn Helgaas <helgaas@kernel.org>]

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
>