lkml.org 
[lkml]   [2009]   [Nov]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] pci/pcie: Avoid unnecessary PCIe link retrains
Jesse Barnes wrote:
> [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?
>

Hi Robert,

How about like below? IMHO, it is easier to understand.
(Please note that I don't test it yet)

Thanks,
Kenji Kaneshige


---
drivers/pci/pcie/aspm.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)

Index: 20091026/drivers/pci/pcie/aspm.c
===================================================================
--- 20091026.orig/drivers/pci/pcie/aspm.c
+++ 20091026/drivers/pci/pcie/aspm.c
@@ -186,6 +186,7 @@ static void pcie_aspm_configure_common_c
unsigned long start_jiffies;
struct pci_dev *child, *parent = link->pdev;
struct pci_bus *linkbus = parent->subordinate;
+ bool ccc_updated = false;
/*
* All functions of a slot should have the same Slot Clock
* Configuration, so just check one function
@@ -214,7 +215,10 @@ static void pcie_aspm_configure_common_c
reg16 |= PCI_EXP_LNKCTL_CCC;
else
reg16 &= ~PCI_EXP_LNKCTL_CCC;
+ if (reg16 == child_reg[PCI_FUNC(child->devfn)])
+ continue;
pci_write_config_word(child, cpos + PCI_EXP_LNKCTL, reg16);
+ ccc_updated = true;
}

/* Configure upstream component */
@@ -224,7 +228,14 @@ static void pcie_aspm_configure_common_c
reg16 |= PCI_EXP_LNKCTL_CCC;
else
reg16 &= ~PCI_EXP_LNKCTL_CCC;
- pci_write_config_word(parent, ppos + PCI_EXP_LNKCTL, reg16);
+ if (reg16 != parent_reg) {
+ pci_write_config_word(parent, ppos + PCI_EXP_LNKCTL, reg16);
+ ccc_updated = true;
+ }
+
+ /* Don't need to retrain link if there is no change in CCC */
+ if (!ccc_updated)
+ return;

/* Retrain link */
reg16 |= PCI_EXP_LNKCTL_RL;



\
 
 \ /
  Last update: 2009-11-05 02:41    [W:0.067 / U:0.500 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site