From: "Bolarinwa O. Saheed" <[email protected]>
To validate and set link latency capability, `struct aspm_latency` and
related members defined within `struct pcie_link_state` are used.
However, since there are not many access to theses values, it is possible
to directly access and compute these values.
Doing this will also reduce the dependency on `struct pcie_link_state`.
The series removes `struct aspm_latency` and related members within
`struct pcie_link_state`. All latencies are now calculated when needed.
Changes in this version:
- directly access downstream by calling `pci_function_0()` instead of
used the `struct pcie_link_state`
Saheed O. Bolarinwa (3):
PCI/ASPM: Remove link latencies cached within struct pcie_link_state
PCI/ASPM: Remove struct pcie_link_state.acceptable
PCI/ASPM: Remove struct aspm_latency
drivers/pci/pcie/aspm.c | 89 ++++++++++++++++++-----------------------
1 file changed, 38 insertions(+), 51 deletions(-)
--
2.20.1
The acceptable latencies for each device on the bus are calculated within
pcie_aspm_cap_init() and cached in struct pcie_link_state.acceptable. They
are only used in pcie_aspm_check_latency() to validate actual latencies.
Thus, it is possible to avoid caching these values.
This patch:
- removes `acceptable` from struct pcie_link_state
- calculates the acceptable latency for each device directly
- removes the calculations done within pcie_aspm_cap_init()
Signed-off-by: Saheed O. Bolarinwa <[email protected]>
---
drivers/pci/pcie/aspm.c | 27 ++++++++-------------------
1 file changed, 8 insertions(+), 19 deletions(-)
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 9e85dfc56657..0c0c055823f1 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -65,12 +65,6 @@ struct pcie_link_state {
u32 clkpm_enabled:1; /* Current Clock PM state */
u32 clkpm_default:1; /* Default Clock PM state by BIOS */
u32 clkpm_disable:1; /* Clock PM disabled */
-
- /*
- * Endpoint acceptable latencies. A pcie downstream port only
- * has one slot under it, so at most there are 8 functions.
- */
- struct aspm_latency acceptable[8];
};
static int aspm_disabled, aspm_force;
@@ -389,7 +383,7 @@ static struct pci_dev *pci_function_0(struct pci_bus *linkbus)
static void pcie_aspm_check_latency(struct pci_dev *endpoint)
{
- u32 latency, lnkcap_up, lnkcap_dw, l1_switch_latency = 0;
+ u32 reg32, latency, encoding, lnkcap_up, lnkcap_dw, l1_switch_latency = 0;
struct pci_dev *downstream;
struct aspm_latency latency_up, latency_dw;
struct aspm_latency *acceptable;
@@ -402,7 +396,13 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
link = endpoint->bus->self->link_state;
downstream = pci_function_0(link->pdev->subordinate);
- acceptable = &link->acceptable[PCI_FUNC(endpoint->devfn)];
+ pcie_capability_read_dword(endpoint, PCI_EXP_DEVCAP, ®32);
+ /* Calculate endpoint L0s acceptable latency */
+ encoding = (reg32 & PCI_EXP_DEVCAP_L0S) >> 6;
+ acceptable->l0s = calc_l0s_acceptable(encoding);
+ /* Calculate endpoint L1 acceptable latency */
+ encoding = (reg32 & PCI_EXP_DEVCAP_L1) >> 9;
+ acceptable->l1 = calc_l1_acceptable(encoding);
while (link) {
/* Read direction exit latencies */
@@ -664,22 +664,11 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
/* Get and check endpoint acceptable latencies */
list_for_each_entry(child, &linkbus->devices, bus_list) {
- u32 reg32, encoding;
- struct aspm_latency *acceptable =
- &link->acceptable[PCI_FUNC(child->devfn)];
if (pci_pcie_type(child) != PCI_EXP_TYPE_ENDPOINT &&
pci_pcie_type(child) != PCI_EXP_TYPE_LEG_END)
continue;
- pcie_capability_read_dword(child, PCI_EXP_DEVCAP, ®32);
- /* Calculate endpoint L0s acceptable latency */
- encoding = (reg32 & PCI_EXP_DEVCAP_L0S) >> 6;
- acceptable->l0s = calc_l0s_acceptable(encoding);
- /* Calculate endpoint L1 acceptable latency */
- encoding = (reg32 & PCI_EXP_DEVCAP_L1) >> 9;
- acceptable->l1 = calc_l1_acceptable(encoding);
-
pcie_aspm_check_latency(child);
}
}
--
2.20.1
On Thu, Sep 16, 2021 at 10:49:23AM +0200, Saheed O. Bolarinwa wrote:
> From: "Bolarinwa O. Saheed" <[email protected]>
>
> To validate and set link latency capability, `struct aspm_latency` and
> related members defined within `struct pcie_link_state` are used.
> However, since there are not many access to theses values, it is possible
> to directly access and compute these values.
> Doing this will also reduce the dependency on `struct pcie_link_state`.
>
> The series removes `struct aspm_latency` and related members within
> `struct pcie_link_state`. All latencies are now calculated when needed.
>
> Changes in this version:
> - directly access downstream by calling `pci_function_0()` instead of
> used the `struct pcie_link_state`
>
> Saheed O. Bolarinwa (3):
> PCI/ASPM: Remove link latencies cached within struct pcie_link_state
> PCI/ASPM: Remove struct pcie_link_state.acceptable
> PCI/ASPM: Remove struct aspm_latency
>
> drivers/pci/pcie/aspm.c | 89 ++++++++++++++++++-----------------------
> 1 file changed, 38 insertions(+), 51 deletions(-)
Hi Saheed, what is this series based on? The other series
(https://lore.kernel.org/r/[email protected])
aplies cleanly to my "main" branch (v5.15-rc2), but this one doesn't
apply either there or on top of the other.
Bjorn
On Thu, Sep 16, 2021 at 10:49:25AM +0200, Saheed O. Bolarinwa wrote:
> The acceptable latencies for each device on the bus are calculated within
> pcie_aspm_cap_init() and cached in struct pcie_link_state.acceptable. They
> are only used in pcie_aspm_check_latency() to validate actual latencies.
> Thus, it is possible to avoid caching these values.
>
> This patch:
> - removes `acceptable` from struct pcie_link_state
> - calculates the acceptable latency for each device directly
> - removes the calculations done within pcie_aspm_cap_init()
>
> Signed-off-by: Saheed O. Bolarinwa <[email protected]>
> ---
> drivers/pci/pcie/aspm.c | 27 ++++++++-------------------
> 1 file changed, 8 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 9e85dfc56657..0c0c055823f1 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -65,12 +65,6 @@ struct pcie_link_state {
> u32 clkpm_enabled:1; /* Current Clock PM state */
> u32 clkpm_default:1; /* Default Clock PM state by BIOS */
> u32 clkpm_disable:1; /* Clock PM disabled */
> -
> - /*
> - * Endpoint acceptable latencies. A pcie downstream port only
> - * has one slot under it, so at most there are 8 functions.
> - */
> - struct aspm_latency acceptable[8];
> };
>
> static int aspm_disabled, aspm_force;
> @@ -389,7 +383,7 @@ static struct pci_dev *pci_function_0(struct pci_bus *linkbus)
>
> static void pcie_aspm_check_latency(struct pci_dev *endpoint)
> {
> - u32 latency, lnkcap_up, lnkcap_dw, l1_switch_latency = 0;
> + u32 reg32, latency, encoding, lnkcap_up, lnkcap_dw, l1_switch_latency = 0;
> struct pci_dev *downstream;
> struct aspm_latency latency_up, latency_dw;
> struct aspm_latency *acceptable;
> @@ -402,7 +396,13 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
>
> link = endpoint->bus->self->link_state;
> downstream = pci_function_0(link->pdev->subordinate);
> - acceptable = &link->acceptable[PCI_FUNC(endpoint->devfn)];
> + pcie_capability_read_dword(endpoint, PCI_EXP_DEVCAP, ®32);
I think you can use endpoint->devcap here, can't you?
> + /* Calculate endpoint L0s acceptable latency */
> + encoding = (reg32 & PCI_EXP_DEVCAP_L0S) >> 6;
> + acceptable->l0s = calc_l0s_acceptable(encoding);
> + /* Calculate endpoint L1 acceptable latency */
> + encoding = (reg32 & PCI_EXP_DEVCAP_L1) >> 9;
> + acceptable->l1 = calc_l1_acceptable(encoding);
I think it's a little weird that we call pcie_aspm_check_latency() for
all the functions in a multi-function device. It's true that they can
all have different acceptable latencies, but they're all on the
downstream end of the same link, so they will all see the same exit
latencies.
I would think we could compute the minimum latency across all the
functions and do a single check to see whether the link can meet that.
But that would be material for a future patch, not this one.
> while (link) {
> /* Read direction exit latencies */
> @@ -664,22 +664,11 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>
> /* Get and check endpoint acceptable latencies */
> list_for_each_entry(child, &linkbus->devices, bus_list) {
> - u32 reg32, encoding;
> - struct aspm_latency *acceptable =
> - &link->acceptable[PCI_FUNC(child->devfn)];
>
> if (pci_pcie_type(child) != PCI_EXP_TYPE_ENDPOINT &&
> pci_pcie_type(child) != PCI_EXP_TYPE_LEG_END)
> continue;
>
> - pcie_capability_read_dword(child, PCI_EXP_DEVCAP, ®32);
> - /* Calculate endpoint L0s acceptable latency */
> - encoding = (reg32 & PCI_EXP_DEVCAP_L0S) >> 6;
> - acceptable->l0s = calc_l0s_acceptable(encoding);
> - /* Calculate endpoint L1 acceptable latency */
> - encoding = (reg32 & PCI_EXP_DEVCAP_L1) >> 9;
> - acceptable->l1 = calc_l1_acceptable(encoding);
> -
> pcie_aspm_check_latency(child);
> }
> }
> --
> 2.20.1
>