Re: [PATCH] pci/pcie: Avoid unnecessary PCIe link retrains

[Jesse Barnes <jbarnes@virtuousgeek.org>]

[Cc'ing linux-pci@vger.kernel.org too]

On Tue, 3 Nov 2009 16:38:20 -0500
RDH <rdh@east.sun.com> wrote:

> 
> This patch avoids unnecessary PCIe link retraining sequences within
> the PCIe ASPM common clock setup code by issuing a link retrain
> command only if we are actually changing the PCIe clock configuration.
> 
> Signed-off-by: Robert D. Houk <rdh@East.Sun.COM>
> ---
>  aspm.c |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> --- a/drivers/pci/pcie/aspm.c	2009-10-15 20:41:50.000000000
> -0400 +++ b/drivers/pci/pcie/aspm.c	2009-11-02
> 14:29:35.000000000 -0500 @@ -183,6 +183,7 @@ static void
> pcie_aspm_configure_common_c {
>  	int ppos, cpos, same_clock = 1;
>  	u16 reg16, parent_reg, child_reg[8];
> +	u16 lnkctl_ccc_or, lnkctl_ccc_and;
>  	unsigned long start_jiffies;
>  	struct pci_dev *child, *parent = link->pdev;
>  	struct pci_bus *linkbus = parent->subordinate;
> @@ -205,6 +206,25 @@ static void pcie_aspm_configure_common_c
>  	if (!(reg16 & PCI_EXP_LNKSTA_SLC))
>  		same_clock = 0;
>  
> +	/* Check upstream component for Common Clock Config */
> +	pci_read_config_word(parent, ppos + PCI_EXP_LNKCTL, &reg16);
> +	lnkctl_ccc_and = lnkctl_ccc_or = (reg16 &
> PCI_EXP_LNKCTL_CCC); +
> +	/* Scan downstream component for CCC, all functions */
> +	list_for_each_entry(child, &linkbus->devices, bus_list) {
> +		cpos = pci_find_capability(child, PCI_CAP_ID_EXP);
> +		pci_read_config_word(child, cpos + PCI_EXP_LNKCTL,
> &reg16);
> +		lnkctl_ccc_and &= (reg16 & PCI_EXP_LNKCTL_CCC);
> +		lnkctl_ccc_or |= (reg16 & PCI_EXP_LNKCTL_CCC);
> +	}
> +
> +	/* If we want Common Clock OFF and no device/function has it
> on */
> +	/* or we want Common Clock ON and every device/function has
> it on */
> +	/* then there is no need to retrain PCIe links */
> +	if (((same_clock == 0) && (lnkctl_ccc_or == 0))
> +            || ((same_clock == 1) && (lnkctl_ccc_and ==
> PCI_EXP_LNKCTL_CCC)))
> +		return;		/* Don't retrain link(s) */
> +
>  	/* Configure downstream component, all functions */
>  	list_for_each_entry(child, &linkbus->devices, bus_list) {
>  		cpos = pci_find_capability(child, PCI_CAP_ID_EXP);
> 

Seems ok to me, anyone have comments?

-- 
Jesse Barnes, Intel Open Source Technology Center