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 <[email protected]>
---
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, ®16);
+ 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, ®16);
+ 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);
[Cc'ing [email protected] too]
On Tue, 3 Nov 2009 16:38:20 -0500
RDH <[email protected]> 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 <[email protected]>
> ---
> 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, ®16);
> + 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,
> ®16);
> + 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
On Wednesday 04 November 2009 12:03:21 pm Jesse Barnes wrote:
> [Cc'ing [email protected] too]
>
> On Tue, 3 Nov 2009 16:38:20 -0500
> RDH <[email protected]> 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 <[email protected]>
> > ---
> > 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, ®16);
> > + 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);
Some other places in pcie/aspm (e.g., pcie_set_clkpm_nocheck() and
pcie_clkpm_cap_init()) check cpos for NULL. Your code looks
superficially similar, so maybe you should check it, too, although
I do see many other place in aspm where we *don't* check it.
We look up this capability so often, I wonder if we should save it
in the struct pci_dev or struct pcie_link or something in such a
way that if we have a struct pcie_link, we are guaranteed that there
is a PCIe capability, and we don't have to search for it again.
Bjorn
Jesse Barnes wrote:
> [Cc'ing [email protected] too]
>
> On Tue, 3 Nov 2009 16:38:20 -0500
> RDH <[email protected]> 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 <[email protected]>
>> ---
>> 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, ®16);
>> + 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,
>> ®16);
>> + 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;
Bjorn Helgaas wrote:
> On Wednesday 04 November 2009 12:03:21 pm Jesse Barnes wrote:
>> [Cc'ing [email protected] too]
>>
>> On Tue, 3 Nov 2009 16:38:20 -0500
>> RDH <[email protected]> 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 <[email protected]>
>>> ---
>>> 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, ®16);
>>> + 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);
>
> Some other places in pcie/aspm (e.g., pcie_set_clkpm_nocheck() and
> pcie_clkpm_cap_init()) check cpos for NULL. Your code looks
> superficially similar, so maybe you should check it, too, although
> I do see many other place in aspm where we *don't* check it.
>
> We look up this capability so often, I wonder if we should save it
> in the struct pci_dev or struct pcie_link or something in such a
> way that if we have a struct pcie_link, we are guaranteed that there
> is a PCIe capability, and we don't have to search for it again.
>
I agree. There are a lot of codes using pci_find_capability() to
search PCIe capability offset in addition to ASPM driver. So I
think it's good idea to have PCIe cap offset in struct pci_dev.
To be honest, I have already made a following patch to add a new
field into struct pci_dev and some other patches to use this new
field a few months ago. But I have never posted them yet. If it
is a right direction, I would like to update and post them soon.
Thanks,
Kenji Kaneshige
There are a lot of codes that searches PCI express capability offset
in the PCI configuration space using pci_find_capability(). Caching it
in the struct pci_dev will reduce unncecessary search. This patch adds
an additional 'pcie_cap' fields into struct pci_dev, which is
initialized at pci device scan time (in set_pcie_port_type()).
Signed-off-by: Kenji Kaneshige <[email protected]>
---
drivers/pci/probe.c | 1 +
include/linux/pci.h | 1 +
2 files changed, 2 insertions(+)
Index: 20090825/drivers/pci/probe.c
===================================================================
--- 20090825.orig/drivers/pci/probe.c
+++ 20090825/drivers/pci/probe.c
@@ -693,6 +693,7 @@ static void set_pcie_port_type(struct pc
if (!pos)
return;
pdev->is_pcie = 1;
+ pdev->pcie_cap = pos;
pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, ®16);
pdev->pcie_type = (reg16 & PCI_EXP_FLAGS_TYPE) >> 4;
}
Index: 20090825/include/linux/pci.h
===================================================================
--- 20090825.orig/include/linux/pci.h
+++ 20090825/include/linux/pci.h
@@ -218,6 +218,7 @@ struct pci_dev {
unsigned int class; /* 3 bytes: (base,sub,prog-if) */
u8 revision; /* PCI revision, low byte of class word */
u8 hdr_type; /* PCI header type (`multi' flag masked out) */
+ u8 pcie_cap; /* PCI-E capability offset */
u8 pcie_type; /* PCI-E device/port type */
u8 rom_base_reg; /* which config register controls the ROM */
u8 pin; /* which interrupt pin this device uses */
On Wednesday 04 November 2009 08:05:11 pm Kenji Kaneshige wrote:
> Bjorn Helgaas wrote:
> > On Wednesday 04 November 2009 12:03:21 pm Jesse Barnes wrote:
> >> [Cc'ing [email protected] too]
> >>
> >> On Tue, 3 Nov 2009 16:38:20 -0500
> >> RDH <[email protected]> wrote:
> >>> + /* Check upstream component for Common Clock Config */
> >>> + pci_read_config_word(parent, ppos + PCI_EXP_LNKCTL, ®16);
> >>> + 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);
> >
> > Some other places in pcie/aspm (e.g., pcie_set_clkpm_nocheck() and
> > pcie_clkpm_cap_init()) check cpos for NULL. Your code looks
> > superficially similar, so maybe you should check it, too, although
> > I do see many other place in aspm where we *don't* check it.
> >
> > We look up this capability so often, I wonder if we should save it
> > in the struct pci_dev or struct pcie_link or something in such a
> > way that if we have a struct pcie_link, we are guaranteed that there
> > is a PCIe capability, and we don't have to search for it again.
> >
>
> I agree. There are a lot of codes using pci_find_capability() to
> search PCIe capability offset in addition to ASPM driver. So I
> think it's good idea to have PCIe cap offset in struct pci_dev.
>
> To be honest, I have already made a following patch to add a new
> field into struct pci_dev and some other patches to use this new
> field a few months ago. But I have never posted them yet. If it
> is a right direction, I would like to update and post them soon.
>
> Thanks,
> Kenji Kaneshige
>
>
> There are a lot of codes that searches PCI express capability offset
> in the PCI configuration space using pci_find_capability(). Caching it
> in the struct pci_dev will reduce unncecessary search. This patch adds
> an additional 'pcie_cap' fields into struct pci_dev, which is
> initialized at pci device scan time (in set_pcie_port_type()).
>
> Signed-off-by: Kenji Kaneshige <[email protected]>
>
> ---
> drivers/pci/probe.c | 1 +
> include/linux/pci.h | 1 +
> 2 files changed, 2 insertions(+)
>
> Index: 20090825/drivers/pci/probe.c
> ===================================================================
> --- 20090825.orig/drivers/pci/probe.c
> +++ 20090825/drivers/pci/probe.c
> @@ -693,6 +693,7 @@ static void set_pcie_port_type(struct pc
> if (!pos)
> return;
> pdev->is_pcie = 1;
> + pdev->pcie_cap = pos;
> pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, ®16);
> pdev->pcie_type = (reg16 & PCI_EXP_FLAGS_TYPE) >> 4;
> }
> Index: 20090825/include/linux/pci.h
> ===================================================================
> --- 20090825.orig/include/linux/pci.h
> +++ 20090825/include/linux/pci.h
> @@ -218,6 +218,7 @@ struct pci_dev {
> unsigned int class; /* 3 bytes: (base,sub,prog-if) */
> u8 revision; /* PCI revision, low byte of class word */
> u8 hdr_type; /* PCI header type (`multi' flag masked out) */
> + u8 pcie_cap; /* PCI-E capability offset */
> u8 pcie_type; /* PCI-E device/port type */
> u8 rom_base_reg; /* which config register controls the ROM */
> u8 pin; /* which interrupt pin this device uses */
>
Here's another possibility, the idea being to collect all the PCIe
stuff in one place. This would require a lot of changes in the PCIe
driver code, but most of them would be trivial.
diff --git a/include/linux/pci.h b/include/linux/pci.h
index f5c7cd3..29272ab 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -198,6 +198,14 @@ struct pci_vpd;
struct pci_sriov;
struct pci_ats;
+struct pcie_dev {
+#ifdef CONFIG_PCIEASPM
+ struct pcie_link_state *link_state; /* ASPM link state. */
+#endif
+ u8 cap; /* PCI-E capability offset */
+ u8 type; /* PCI-E device/port type */
+};
+
/*
* The pci_dev structure is used to describe PCI devices.
*/
@@ -218,7 +226,6 @@ struct pci_dev {
unsigned int class; /* 3 bytes: (base,sub,prog-if) */
u8 revision; /* PCI revision, low byte of class word */
u8 hdr_type; /* PCI header type (`multi' flag masked out) */
- u8 pcie_type; /* PCI-E device/port type */
u8 rom_base_reg; /* which config register controls the ROM */
u8 pin; /* which interrupt pin this device uses */
@@ -243,10 +250,6 @@ struct pci_dev {
unsigned int no_d1d2:1; /* Only allow D0 and D3 */
unsigned int wakeup_prepared:1;
-#ifdef CONFIG_PCIEASPM
- struct pcie_link_state *link_state; /* ASPM link state. */
-#endif
-
pci_channel_state_t error_state; /* current connectivity state */
struct device dev; /* Generic device interface */
@@ -273,7 +276,6 @@ struct pci_dev {
unsigned int msix_enabled:1;
unsigned int ari_enabled:1; /* ARI forwarding */
unsigned int is_managed:1;
- unsigned int is_pcie:1;
unsigned int needs_freset:1; /* Dev requires fundamental reset */
unsigned int state_saved:1;
unsigned int is_physfn:1;
@@ -293,6 +295,7 @@ struct pci_dev {
struct list_head msi_list;
#endif
struct pci_vpd *vpd;
+ struct pci_pcie *pcie;
#ifdef CONFIG_PCI_IOV
union {
struct pci_sriov *sriov; /* SR-IOV capability related */
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 5c4ce1b..f85f2e7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -684,13 +684,21 @@ static void set_pcie_port_type(struct pci_dev *pdev)
{
int pos;
u16 reg16;
+ struct pcie_dev *pcie;
pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
if (!pos)
return;
- pdev->is_pcie = 1;
+
+ pcie = kzalloc(sizeof(struct pci_dev), GFP_KERNEL);
+ if (!pcie)
+ return;
+
pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, ®16);
- pdev->pcie_type = (reg16 & PCI_EXP_FLAGS_TYPE) >> 4;
+
+ pcie->type = (reg16 & PCI_EXP_FLAGS_TYPE) >> 4;
+ pcie->cap = pos;
+ pdev->pcie = pcie;
}
static void set_pcie_hotplug_bridge(struct pci_dev *pdev)
On Thu, Nov 05, 2009 at 12:59:25PM -0600, Bjorn Helgaas wrote:
> Here's another possibility, the idea being to collect all the PCIe
> stuff in one place. This would require a lot of changes in the PCIe
> driver code, but most of them would be trivial.
I don't like the idea of kmallocing a 6- or 10-byte data structure
... better to keep it in the pci_dev. Maybe embedding a pcie_dev inside
the pci_dev would be a good idea, but unless there're more things to
move to it, this seems like a net loss to me.
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
On Thu, Nov 05, 2009 at 12:05:11PM +0900, Kenji Kaneshige wrote:
> There are a lot of codes that searches PCI express capability offset
> in the PCI configuration space using pci_find_capability(). Caching it
> in the struct pci_dev will reduce unncecessary search. This patch adds
> an additional 'pcie_cap' fields into struct pci_dev, which is
> initialized at pci device scan time (in set_pcie_port_type()).
I think adding this should imply the removal of ->is_pcie. pcie_cap == 0
means !is_pcie.
> Signed-off-by: Kenji Kaneshige <[email protected]>
>
> ---
> drivers/pci/probe.c | 1 +
> include/linux/pci.h | 1 +
> 2 files changed, 2 insertions(+)
>
> Index: 20090825/drivers/pci/probe.c
> ===================================================================
> --- 20090825.orig/drivers/pci/probe.c
> +++ 20090825/drivers/pci/probe.c
> @@ -693,6 +693,7 @@ static void set_pcie_port_type(struct pc
> if (!pos)
> return;
> pdev->is_pcie = 1;
> + pdev->pcie_cap = pos;
> pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, ®16);
> pdev->pcie_type = (reg16 & PCI_EXP_FLAGS_TYPE) >> 4;
> }
> Index: 20090825/include/linux/pci.h
> ===================================================================
> --- 20090825.orig/include/linux/pci.h
> +++ 20090825/include/linux/pci.h
> @@ -218,6 +218,7 @@ struct pci_dev {
> unsigned int class; /* 3 bytes: (base,sub,prog-if) */
> u8 revision; /* PCI revision, low byte of class word */
> u8 hdr_type; /* PCI header type (`multi' flag masked out) */
> + u8 pcie_cap; /* PCI-E capability offset */
> u8 pcie_type; /* PCI-E device/port type */
> u8 rom_base_reg; /* which config register controls the ROM */
> u8 pin; /* which interrupt pin this device uses */
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
On Thursday 05 November 2009 12:07:07 pm Matthew Wilcox wrote:
> On Thu, Nov 05, 2009 at 12:59:25PM -0600, Bjorn Helgaas wrote:
> > Here's another possibility, the idea being to collect all the PCIe
> > stuff in one place. This would require a lot of changes in the PCIe
> > driver code, but most of them would be trivial.
>
> I don't like the idea of kmallocing a 6- or 10-byte data structure
> ... better to keep it in the pci_dev. Maybe embedding a pcie_dev inside
> the pci_dev would be a good idea, but unless there're more things to
> move to it, this seems like a net loss to me.
That's true, it's not worth it for such a small structure. I figured
there would probably be more PCIe-related stuff that could go there,
e.g., embedding the link_state directly. But I haven't looked to
see whether there actually is enough PCIe stuff to make it worthwhile.
Bjorn
Bjorn Helgaas wrote:
> On Thursday 05 November 2009 12:07:07 pm Matthew Wilcox wrote:
>> On Thu, Nov 05, 2009 at 12:59:25PM -0600, Bjorn Helgaas wrote:
>>> Here's another possibility, the idea being to collect all the PCIe
>>> stuff in one place. This would require a lot of changes in the PCIe
>>> driver code, but most of them would be trivial.
>> I don't like the idea of kmallocing a 6- or 10-byte data structure
>> ... better to keep it in the pci_dev. Maybe embedding a pcie_dev inside
>> the pci_dev would be a good idea, but unless there're more things to
>> move to it, this seems like a net loss to me.
>
> That's true, it's not worth it for such a small structure. I figured
> there would probably be more PCIe-related stuff that could go there,
> e.g., embedding the link_state directly. But I haven't looked to
> see whether there actually is enough PCIe stuff to make it worthwhile.
>
In my understanding, there are the following PCIe specific data in
struct pci_dev. It is not many.
u8 pcie_cap; *I added this time
u8 pcie_type;
struct pcie_link_state *link_state;
unsigned int ari_enabled:1;
unsigned int is_pcie:1; *No longer needed and will be removed.
unsigned int aer_firmware_first:1;
How about keeping cap offset in pci_dev so far and introducing the
following wrapper function?
static inline pcie_cap(struct pci_dev *pdev)
{
return pdev->pcie_cap;
}
When we actually need a new data structure for PCIe-related stuff in the
future, all we need to do would be changing this wrapper like below.
static inline pcie_cap(struct pci_dev *pdev)
{
struct pcie_dev *pcie = pdev->pcie;
if (pcie)
return pcie->cap;
return 0;
}
Thanks,
Kenji Kaneshige
Matthew Wilcox wrote:
> On Thu, Nov 05, 2009 at 12:05:11PM +0900, Kenji Kaneshige wrote:
>> There are a lot of codes that searches PCI express capability offset
>> in the PCI configuration space using pci_find_capability(). Caching it
>> in the struct pci_dev will reduce unncecessary search. This patch adds
>> an additional 'pcie_cap' fields into struct pci_dev, which is
>> initialized at pci device scan time (in set_pcie_port_type()).
>
> I think adding this should imply the removal of ->is_pcie. pcie_cap == 0
> means !is_pcie.
Right.
But, as you know, we need to change users of is_pcie (including some
adapter card drivers) before removing it.
Thanks,
Kenji Kaneshige
On Thu, 05 Nov 2009 12:05:11 +0900
Kenji Kaneshige <[email protected]> wrote:
>
> There are a lot of codes that searches PCI express capability offset
> in the PCI configuration space using pci_find_capability(). Caching it
> in the struct pci_dev will reduce unncecessary search. This patch adds
> an additional 'pcie_cap' fields into struct pci_dev, which is
> initialized at pci device scan time (in set_pcie_port_type()).
>
> Signed-off-by: Kenji Kaneshige <[email protected]>
>
> ---
> drivers/pci/probe.c | 1 +
> include/linux/pci.h | 1 +
> 2 files changed, 2 insertions(+)
>
> Index: 20090825/drivers/pci/probe.c
> ===================================================================
> --- 20090825.orig/drivers/pci/probe.c
> +++ 20090825/drivers/pci/probe.c
> @@ -693,6 +693,7 @@ static void set_pcie_port_type(struct pc
> if (!pos)
> return;
> pdev->is_pcie = 1;
> + pdev->pcie_cap = pos;
> pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, ®16);
> pdev->pcie_type = (reg16 & PCI_EXP_FLAGS_TYPE) >> 4;
> }
> Index: 20090825/include/linux/pci.h
> ===================================================================
> --- 20090825.orig/include/linux/pci.h
> +++ 20090825/include/linux/pci.h
> @@ -218,6 +218,7 @@ struct pci_dev {
> unsigned int class; /* 3 bytes:
> (base,sub,prog-if) */ u8 revision; /* PCI
> revision, low byte of class word */ u8
> hdr_type; /* PCI header type (`multi' flag masked out) */
> + u8 pcie_cap; /* PCI-E capability
> offset */ u8 pcie_type; /* PCI-E device/port
> type */ u8 rom_base_reg; /* which config
> register controls the ROM */ u8 pin;
> /* which interrupt pin this device uses */
>
>
Applied this one to linux-next, thanks Kenji-san.
--
Jesse Barnes, Intel Open Source Technology Center