Hi Sinan,
On Wed, Mar 08, 2017 at 03:39:11PM -0500, Sinan Kaya wrote:
> When the operating system is booted with the default ASPM policy
> (POLICY_DEFAULT), current code is querying the enable/disable
> states from ASPM registers to determine the policy.
>
> For example, a BIOS could set the power saving state to performance
> and clear all ASPM control registers. A balanced ASPM policy could
> enable L0s and disable L1. A power conscious BIOS could enable both
> L0s and L1 to trade off latency and performance vs. power.
>
> After hotplug removal, pcie_aspm_exit_link_state() function clears
> the ASPM registers. An insertion following hotplug removal reads
> incorrect policy as ASPM disabled even though ASPM was enabled
> during boot.
>
> This is caused by the fact that same function is used for reconfiguring
> ASPM regardless of the of the power on state.
>
> This patch builds some state info into pcie_aspm_cap_init() function.
>
> Adding a flag and a counter to the struct pci_dev to save the
> power up policy in the bridge during boot. ASPM enable counter is
> used as a switch to determine when to use saved value.
>
> Signed-off-by: Sinan Kaya <okaya@xxxxxxxxxxxxxx>
> Signed-off-by: Mayurkumar Patel <mayurkumar.patel@xxxxxxxxx>
> ---
> drivers/pci/pcie/aspm.c | 21 +++++++++++++++++----
> include/linux/pci.h | 2 ++
> 2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 17ac1dc..63435b0 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -338,8 +338,9 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
> }
> }
>
> -static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
> +static void pcie_aspm_cap_init(struct pci_dev *pdev, int blacklist)
> {
> + struct pcie_link_state *link = pdev->link_state;
> struct pci_dev *child, *parent = link->pdev;
> struct pci_bus *linkbus = parent->subordinate;
> struct aspm_register_info upreg, dwreg;
> @@ -397,8 +398,20 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
> link->latency_up.l1 = calc_l1_latency(upreg.latency_encoding_l1);
> link->latency_dw.l1 = calc_l1_latency(dwreg.latency_encoding_l1);
>
> - /* Save default state */
> - link->aspm_default = link->aspm_enabled;
> + /*
> + * Save default state from FW when enabling ASPM for the first time
> + * during boot by looking at the calculated link->aspm_enabled bits
> + * above and aspm_enable_cnt will be zero.
> + *
> + * If this path is getting called for the second/third time
> + * (aspm_enable_cnt will be non-zero). Assume that the current state
> + * of the ASPM registers may not necessarily match what FW asked us to
> + * do as in the case of hotplug insertion/removal.
> + */
> + if (atomic_inc_return(&pdev->aspm_enable_cnt) == 1)
> + pdev->aspm_default = link->aspm_default = link->aspm_enabled;
> + else
> + link->aspm_default = pdev->aspm_default;
This is an aspect of the ASPM design that I don't like:
- pcie_aspm_init_link_state() is called on a *bridge* after we've
enumerated any devices below the bridge, and we allocate the
link_state.
- pcie_aspm_exit_link_state() is called on an *endpoint*, and if
we're removing the last endpoint below a bridge, we release the
*parent's* link_state.
This leads to the problem you're trying to solve here: we throw away
all the bridge's ASPM information when the child is removed, so we
lose the original settings from firmware.
Your solution is to add some state outside the link_state structure,
and initialize it the first time we enable ASPM for the link. With
the current design, you don't really have much choice, but I think it
ends up being more complicated than it should be.
How hard do you think it would be to rework this path slightly so we:
- call pcie_aspm_init_link_state() for every device, maybe from
pci_init_capabilities()
- for bridges, have pcie_aspm_init_link_state() allocate a
link_state, regardless of whether it currently has any children,
and save the ASPM settings done by firmware
- for endpoints, have pcie_aspm_init_link_state() do the actual ASPM
setup of the link as it currently does
- for endpoints, change pcie_aspm_exit_link_state() so it cleans up
the device's own state and disables ASPM if necessary, but doesn't
remove the parent's link_state
- for bridges, change pcie_aspm_exit_link_state() so it frees the
bridge's own link_state
?
> /* Setup initial capable state. Will be updated later */
> link->aspm_capable = link->aspm_support;
> @@ -599,7 +612,7 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
> * upstream links also because capable state of them can be
> * update through pcie_aspm_cap_init().
> */
> - pcie_aspm_cap_init(link, blacklist);
> + pcie_aspm_cap_init(pdev, blacklist);
>
> /* Setup initial Clock PM state */
> pcie_clkpm_cap_init(link, blacklist);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index e2d1a12..aa7bd7e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -321,6 +321,8 @@ struct pci_dev {
>
> #ifdef CONFIG_PCIEASPM
> struct pcie_link_state *link_state; /* ASPM link state */
> + unsigned int aspm_default; /* ASPM policy set by BIOS */
> + atomic_t aspm_enable_cnt; /* ASPM policy initialization */
> #endif
>
> pci_channel_state_t error_state; /* current connectivity state */
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
