[+cc Nirmal, Jonathan, since vmd is the only caller of
pci_enable_link_state()]
On Tue, Apr 11, 2023 at 04:40:33PM +0530, Ajay Agarwal wrote:
> Currently the aspm driver does not set ASPM_STATE_L1 bit in
> aspm_default when the class driver requests L1SS ASPM state.
> This will lead to pcie_config_aspm_link() not enabling the
> requested L1SS state. Set ASPM_STATE_L1 when class driver
> enables L1ss.
Since vmd is currently the only caller of pci_enable_link_state(), and
it supplies PCIE_LINK_STATE_ALL:
#define PCIE_LINK_STATE_ALL (PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |\
PCIE_LINK_STATE_CLKPM | PCIE_LINK_STATE_L1_1 |\
PCIE_LINK_STATE_L1_2 | PCIE_LINK_STATE_L1_1_PCIPM |\
PCIE_LINK_STATE_L1_2_PCIPM)
I don't think this makes any functional difference at this point,
right?
> Signed-off-by: Ajay Agarwal <ajayagarwal@xxxxxxxxxx>
> ---
> drivers/pci/pcie/aspm.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 5765b226102a..7c9935f331f1 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1170,16 +1170,16 @@ int pci_enable_link_state(struct pci_dev *pdev, int state)
> if (state & PCIE_LINK_STATE_L0S)
> link->aspm_default |= ASPM_STATE_L0S;
> if (state & PCIE_LINK_STATE_L1)
> - /* L1 PM substates require L1 */
> - link->aspm_default |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
> + link->aspm_default |= ASPM_STATE_L1;
> + /* L1 PM substates require L1 */
> if (state & PCIE_LINK_STATE_L1_1)
> - link->aspm_default |= ASPM_STATE_L1_1;
> + link->aspm_default |= ASPM_STATE_L1_1 | ASPM_STATE_L1;
IIUC, this:
pci_enable_link_state(PCIE_LINK_STATE_L1_1)
currently doesn't actually enable L1.1 because the caller didn't
supply "PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L1_1".
I'm not sure that's a problem -- the driver can easily supply both if
it wants both.
For devices that support only L1,
"pci_enable_link_state(PCIE_LINK_STATE_L1_1)" would implicitly enable
L1 even though L1.1 is not supported, which seems a little bit weird.
> if (state & PCIE_LINK_STATE_L1_2)
> - link->aspm_default |= ASPM_STATE_L1_2;
> + link->aspm_default |= ASPM_STATE_L1_2 | ASPM_STATE_L1;
> if (state & PCIE_LINK_STATE_L1_1_PCIPM)
> - link->aspm_default |= ASPM_STATE_L1_1_PCIPM;
> + link->aspm_default |= ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1;
> if (state & PCIE_LINK_STATE_L1_2_PCIPM)
> - link->aspm_default |= ASPM_STATE_L1_2_PCIPM;
> + link->aspm_default |= ASPM_STATE_L1_2_PCIPM | ASPM_STATE_L1;
> pcie_config_aspm_link(link, policy_to_aspm_state(link));
>
> link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
> --
> 2.40.0.577.gac1e443424-goog
>
