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@xxxxxxxxx>
> ---
> 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, ®16);
> - 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
>
