On 6/2/2023 6:47 AM, Ilpo Järvinen wrote:
> IB/hfi1 driver adjusts ASPM state itself which leaves ASPM service
> driver in PCI core unaware of the link state changes the driver
> implemented.
>
> Call pci_disable_link_state() and pci_enable_link_state() instead of
> adjusting ASPMC field in LNKCTL directly in the driver and let PCI core
> handle the ASPM state management. Remove the functions that handled the
> ASPM changes that are now unnecessary.
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> ---
> drivers/infiniband/hw/hfi1/aspm.c | 38 +++----------------------------
> drivers/infiniband/hw/hfi1/pcie.c | 2 +-
> 2 files changed, 4 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/infiniband/hw/hfi1/aspm.c b/drivers/infiniband/hw/hfi1/aspm.c
> index a3c53be4072c..8e3fc1d4c9c6 100644
> --- a/drivers/infiniband/hw/hfi1/aspm.c
> +++ b/drivers/infiniband/hw/hfi1/aspm.c
> @@ -54,45 +54,13 @@ static void aspm_hw_set_l1_ent_latency(struct hfi1_devdata *dd)
> pci_write_config_dword(dd->pcidev, PCIE_CFG_REG_PL3, reg32);
> }
>
> -static void aspm_hw_enable_l1(struct hfi1_devdata *dd)
> -{
> - struct pci_dev *parent = dd->pcidev->bus->self;
> -
> - /*
> - * If the driver does not have access to the upstream component,
> - * it cannot support ASPM L1 at all.
> - */
> - if (!parent)
> - return;
> -
> - /* Enable ASPM L1 first in upstream component and then downstream */
> - pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
> - PCI_EXP_LNKCTL_ASPMC,
> - PCI_EXP_LNKCTL_ASPM_L1);
> - pcie_capability_clear_and_set_word(dd->pcidev, PCI_EXP_LNKCTL,
> - PCI_EXP_LNKCTL_ASPMC,
> - PCI_EXP_LNKCTL_ASPM_L1);
> -}
> -
> -void aspm_hw_disable_l1(struct hfi1_devdata *dd)
> -{
> - struct pci_dev *parent = dd->pcidev->bus->self;
> -
> - /* Disable ASPM L1 first in downstream component and then upstream */
> - pcie_capability_clear_and_set_word(dd->pcidev, PCI_EXP_LNKCTL,
> - PCI_EXP_LNKCTL_ASPMC, 0x0);
> - if (parent)
> - pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
> - PCI_EXP_LNKCTL_ASPMC, 0x0);
> -}
> -
> static void aspm_enable(struct hfi1_devdata *dd)
> {
> if (dd->aspm_enabled || aspm_mode == ASPM_MODE_DISABLED ||
> !dd->aspm_supported)
> return;
>
> - aspm_hw_enable_l1(dd);
> + pci_enable_link_state(dd->pcidev, PCI_EXP_LNKCTL_ASPM_L1);
> dd->aspm_enabled = true;
> }
>
> @@ -101,7 +69,7 @@ static void aspm_disable(struct hfi1_devdata *dd)
> if (!dd->aspm_enabled || aspm_mode == ASPM_MODE_ENABLED)
> return;
>
> - aspm_hw_disable_l1(dd);
> + pci_disable_link_state(dd->pcidev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
> dd->aspm_enabled = false;
> }
>
> @@ -254,7 +222,7 @@ void aspm_init(struct hfi1_devdata *dd)
> /* Start with ASPM disabled */
> aspm_hw_set_l1_ent_latency(dd);
> dd->aspm_enabled = false;
> - aspm_hw_disable_l1(dd);
> + pci_disable_link_state(dd->pcidev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
>
> /* Now turn on ASPM if configured */
> aspm_enable_all(dd);
> diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c
> index 08732e1ac966..767f6cb770b6 100644
> --- a/drivers/infiniband/hw/hfi1/pcie.c
> +++ b/drivers/infiniband/hw/hfi1/pcie.c
> @@ -1182,7 +1182,7 @@ int do_pcie_gen3_transition(struct hfi1_devdata *dd)
> * will be enabled if required later
> */
> dd_dev_info(dd, "%s: clearing ASPM\n", __func__);
> - aspm_hw_disable_l1(dd);
> + pci_disable_link_state(dd->pcidev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
>
> /*
> * step 5f: clear DirectSpeedChange
Looks good.
Reviewed-by: Dean Luick <dean.luick@xxxxxxxxxxxxxxxxxxxx>
-Dean
External recipient
