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@xxxxxxxxx>
> ---
> 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
>
