| Message ID | 1366844090-5492-2-git-send-email-lucaskt@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Wed, Apr 24, 2013 at 07:54:49PM -0300, lucaskt@linux.vnet.ibm.com wrote: > From: Lucas Kannebley Tavares <lucaskt@linux.vnet.ibm.com> > > On pseries machines the detection for max_bus_speed should be done > through an OpenFirmware property. This patch adds a function to perform > this detection and a hook to perform dynamic adding of the function only for > pseries. This is done by overwriting the weak > pcibios_root_bridge_prepare function which is called by pci_create_root_bus(). > > Signed-off-by: Lucas Kannebley Tavares <lucaskt@linux.vnet.ibm.com> > --- > arch/powerpc/include/asm/machdep.h | 2 ++ > arch/powerpc/kernel/pci-common.c | 8 +++++ > arch/powerpc/platforms/pseries/pci.c | 51 ++++++++++++++++++++++++++++++++ > arch/powerpc/platforms/pseries/pseries.h | 4 +++ > arch/powerpc/platforms/pseries/setup.c | 2 ++ > 5 files changed, 67 insertions(+) > > diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h > index 3d6b410..8f558bf 100644 > --- a/arch/powerpc/include/asm/machdep.h > +++ b/arch/powerpc/include/asm/machdep.h > @@ -107,6 +107,8 @@ struct machdep_calls { > void (*pcibios_fixup)(void); > int (*pci_probe_mode)(struct pci_bus *); > void (*pci_irq_fixup)(struct pci_dev *dev); > + int (*pcibios_root_bridge_prepare)(struct pci_host_bridge > + *bridge); > > /* To setup PHBs when using automatic OF platform driver for PCI */ > int (*pci_setup_phb)(struct pci_controller *host); > diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c > index fa12ae4..80986cf 100644 > --- a/arch/powerpc/kernel/pci-common.c > +++ b/arch/powerpc/kernel/pci-common.c > @@ -844,6 +844,14 @@ int pci_proc_domain(struct pci_bus *bus) > return 1; > } > > +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) > +{ > + if (ppc_md.pcibios_root_bridge_prepare) > + return ppc_md.pcibios_root_bridge_prepare(bridge); > + > + return 0; > +} > + > /* This header fixup will do the resource fixup for all devices as they are > * probed, but not for bridge ranges > */ > diff --git a/arch/powerpc/platforms/pseries/pci.c b/arch/powerpc/platforms/pseries/pci.c > index 0b580f4..7f9c956 100644 > --- a/arch/powerpc/platforms/pseries/pci.c > +++ b/arch/powerpc/platforms/pseries/pci.c > @@ -108,3 +108,54 @@ static void fixup_winbond_82c105(struct pci_dev* dev) > } > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_WINBOND, PCI_DEVICE_ID_WINBOND_82C105, > fixup_winbond_82c105); > + > +int pseries_root_bridge_prepare(struct pci_host_bridge *bridge) > +{ > + struct device_node *dn, *pdn; > + struct pci_bus *bus; > + const uint32_t *pcie_link_speed_stats; > + > + bus = bridge->bus; > + > + dn = pcibios_get_phb_of_node(bus); > + if (!dn) > + return 0; > + > + for (pdn = dn; pdn != NULL; pdn = pdn->parent) { > + pcie_link_speed_stats = (const uint32_t *) of_get_property(dn, > + "ibm,pcie-link-speed-stats", NULL); > + if (pcie_link_speed_stats) > + break; > + } Please use the helpers in include/linux/of.h rather than open coding this. Yours Tony
On 04/24/2013 08:48 PM, Tony Breeds wrote: > On Wed, Apr 24, 2013 at 07:54:49PM -0300, lucaskt@linux.vnet.ibm.com wrote: >> From: Lucas Kannebley Tavares<lucaskt@linux.vnet.ibm.com> >> >> On pseries machines the detection for max_bus_speed should be done >> through an OpenFirmware property. This patch adds a function to perform >> this detection and a hook to perform dynamic adding of the function only for >> pseries. This is done by overwriting the weak >> pcibios_root_bridge_prepare function which is called by pci_create_root_bus(). >> >> Signed-off-by: Lucas Kannebley Tavares<lucaskt@linux.vnet.ibm.com> >> --- >> arch/powerpc/include/asm/machdep.h | 2 ++ >> arch/powerpc/kernel/pci-common.c | 8 +++++ >> arch/powerpc/platforms/pseries/pci.c | 51 ++++++++++++++++++++++++++++++++ >> arch/powerpc/platforms/pseries/pseries.h | 4 +++ >> arch/powerpc/platforms/pseries/setup.c | 2 ++ >> 5 files changed, 67 insertions(+) >> >> diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h >> index 3d6b410..8f558bf 100644 >> --- a/arch/powerpc/include/asm/machdep.h >> +++ b/arch/powerpc/include/asm/machdep.h >> @@ -107,6 +107,8 @@ struct machdep_calls { >> void (*pcibios_fixup)(void); >> int (*pci_probe_mode)(struct pci_bus *); >> void (*pci_irq_fixup)(struct pci_dev *dev); >> + int (*pcibios_root_bridge_prepare)(struct pci_host_bridge >> + *bridge); >> >> /* To setup PHBs when using automatic OF platform driver for PCI */ >> int (*pci_setup_phb)(struct pci_controller *host); >> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c >> index fa12ae4..80986cf 100644 >> --- a/arch/powerpc/kernel/pci-common.c >> +++ b/arch/powerpc/kernel/pci-common.c >> @@ -844,6 +844,14 @@ int pci_proc_domain(struct pci_bus *bus) >> return 1; >> } >> >> +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) >> +{ >> + if (ppc_md.pcibios_root_bridge_prepare) >> + return ppc_md.pcibios_root_bridge_prepare(bridge); >> + >> + return 0; >> +} >> + >> /* This header fixup will do the resource fixup for all devices as they are >> * probed, but not for bridge ranges >> */ >> diff --git a/arch/powerpc/platforms/pseries/pci.c b/arch/powerpc/platforms/pseries/pci.c >> index 0b580f4..7f9c956 100644 >> --- a/arch/powerpc/platforms/pseries/pci.c >> +++ b/arch/powerpc/platforms/pseries/pci.c >> @@ -108,3 +108,54 @@ static void fixup_winbond_82c105(struct pci_dev* dev) >> } >> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_WINBOND, PCI_DEVICE_ID_WINBOND_82C105, >> fixup_winbond_82c105); >> + >> +int pseries_root_bridge_prepare(struct pci_host_bridge *bridge) >> +{ >> + struct device_node *dn, *pdn; >> + struct pci_bus *bus; >> + const uint32_t *pcie_link_speed_stats; >> + >> + bus = bridge->bus; >> + >> + dn = pcibios_get_phb_of_node(bus); >> + if (!dn) >> + return 0; >> + >> + for (pdn = dn; pdn != NULL; pdn = pdn->parent) { >> + pcie_link_speed_stats = (const uint32_t *) of_get_property(dn, >> + "ibm,pcie-link-speed-stats", NULL); >> + if (pcie_link_speed_stats) >> + break; >> + } > > Please use the helpers in include/linux/of.h rather than open coding > this. > > Yours Tony Hi Tony, This is what I can find as an equivalent code: for (pdn = dn; pdn != NULL; pdn = of_get_next_parent(pdn)) { pcie_link_speed_stats = (const uint32_t *) of_get_property(dn, "ibm,pcie-link-speed-stats", NULL); if (pcie_link_speed_stats) break; } is this your suggestion, or was it another approach that will have the same result? Thanks,
On 04/25/2013 02:34 PM, Lucas Kannebley Tavares wrote: > On 04/24/2013 08:48 PM, Tony Breeds wrote: >>> diff --git a/arch/powerpc/platforms/pseries/pci.c >>> b/arch/powerpc/platforms/pseries/pci.c >>> index 0b580f4..7f9c956 100644 >>> --- a/arch/powerpc/platforms/pseries/pci.c >>> +++ b/arch/powerpc/platforms/pseries/pci.c >>> @@ -108,3 +108,54 @@ static void fixup_winbond_82c105(struct pci_dev* >>> dev) >>> } >>> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_WINBOND, >>> PCI_DEVICE_ID_WINBOND_82C105, >>> fixup_winbond_82c105); >>> + >>> +int pseries_root_bridge_prepare(struct pci_host_bridge *bridge) >>> +{ >>> + struct device_node *dn, *pdn; >>> + struct pci_bus *bus; >>> + const uint32_t *pcie_link_speed_stats; >>> + >>> + bus = bridge->bus; >>> + >>> + dn = pcibios_get_phb_of_node(bus); >>> + if (!dn) >>> + return 0; >>> + >>> + for (pdn = dn; pdn != NULL; pdn = pdn->parent) { >>> + pcie_link_speed_stats = (const uint32_t *) of_get_property(dn, >>> + "ibm,pcie-link-speed-stats", NULL); >>> + if (pcie_link_speed_stats) >>> + break; >>> + } >> >> Please use the helpers in include/linux/of.h rather than open coding >> this. >> >> Yours Tony > > > Hi Tony, > > > This is what I can find as an equivalent code: > > for (pdn = dn; pdn != NULL; pdn = of_get_next_parent(pdn)) { > pcie_link_speed_stats = (const uint32_t *) > of_get_property(dn, > "ibm,pcie-link-speed-stats", NULL); > if (pcie_link_speed_stats) > break; > } > > is this your suggestion, or was it another approach that will have the > same result? > > Thanks, > Hi Tony, It seems Lucas' change is a bit incomplete and is not handling the reference counter to the device_node correctly. Is the following change what you had in mind? dn = pcibios_get_phb_of_node(bus); if (!dn) return 0; for (pdn = dn; pdn != NULL; pdn = of_get_next_parent(pdn)) { pcie_link_speed_stats = (const uint32_t *) of_get_property(pdn, "ibm,pcie-link-speed-stats", NULL); if (pcie_link_speed_stats) break; } of_node_put(pdn); Thanks,
On Thu, May 02, 2013 at 12:21:37PM -0300, Kleber Sacilotto de Souza wrote: > Hi Tony, > > It seems Lucas' change is a bit incomplete and is not handling the reference counter to > the device_node correctly. Is the following change what you had in mind? Ahh Sorry I expected there would be a for_each_parent_of_node macro. I did a quick grep and it seems that's not very common, so open coding it should be fine. > > dn = pcibios_get_phb_of_node(bus); > if (!dn) > return 0; > > for (pdn = dn; pdn != NULL; pdn = of_get_next_parent(pdn)) { > pcie_link_speed_stats = (const uint32_t *) of_get_property(pdn, > "ibm,pcie-link-speed-stats", NULL); > if (pcie_link_speed_stats) > break; > } > > of_node_put(pdn); I think you need the of_node_put() in the body of the loop, otherwise aren't you leaking refcounts? Yours Tony
On 05/03/2013 03:40 AM, Tony Breeds wrote: > On Thu, May 02, 2013 at 12:21:37PM -0300, Kleber Sacilotto de Souza wrote: > >> Hi Tony, >> >> It seems Lucas' change is a bit incomplete and is not handling the reference counter to >> the device_node correctly. Is the following change what you had in mind? > > Ahh Sorry I expected there would be a for_each_parent_of_node macro. > I did a quick grep and it seems that's not very common, so open coding > it should be fine. > >> >> dn = pcibios_get_phb_of_node(bus); >> if (!dn) >> return 0; >> >> for (pdn = dn; pdn != NULL; pdn = of_get_next_parent(pdn)) { >> pcie_link_speed_stats = (const uint32_t *) of_get_property(pdn, >> "ibm,pcie-link-speed-stats", NULL); >> if (pcie_link_speed_stats) >> break; >> } >> >> of_node_put(pdn); > > I think you need the of_node_put() in the body of the loop, otherwise > aren't you leaking refcounts? of_get_next_parent() takes care of that. It does of_node_put() on the current node after doing of_node_get() on the parent. Thanks,
diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h index 3d6b410..8f558bf 100644 --- a/arch/powerpc/include/asm/machdep.h +++ b/arch/powerpc/include/asm/machdep.h @@ -107,6 +107,8 @@ struct machdep_calls { void (*pcibios_fixup)(void); int (*pci_probe_mode)(struct pci_bus *); void (*pci_irq_fixup)(struct pci_dev *dev); + int (*pcibios_root_bridge_prepare)(struct pci_host_bridge + *bridge); /* To setup PHBs when using automatic OF platform driver for PCI */ int (*pci_setup_phb)(struct pci_controller *host); diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c index fa12ae4..80986cf 100644 --- a/arch/powerpc/kernel/pci-common.c +++ b/arch/powerpc/kernel/pci-common.c @@ -844,6 +844,14 @@ int pci_proc_domain(struct pci_bus *bus) return 1; } +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) +{ + if (ppc_md.pcibios_root_bridge_prepare) + return ppc_md.pcibios_root_bridge_prepare(bridge); + + return 0; +} + /* This header fixup will do the resource fixup for all devices as they are * probed, but not for bridge ranges */ diff --git a/arch/powerpc/platforms/pseries/pci.c b/arch/powerpc/platforms/pseries/pci.c index 0b580f4..7f9c956 100644 --- a/arch/powerpc/platforms/pseries/pci.c +++ b/arch/powerpc/platforms/pseries/pci.c @@ -108,3 +108,54 @@ static void fixup_winbond_82c105(struct pci_dev* dev) } DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_WINBOND, PCI_DEVICE_ID_WINBOND_82C105, fixup_winbond_82c105); + +int pseries_root_bridge_prepare(struct pci_host_bridge *bridge) +{ + struct device_node *dn, *pdn; + struct pci_bus *bus; + const uint32_t *pcie_link_speed_stats; + + bus = bridge->bus; + + dn = pcibios_get_phb_of_node(bus); + if (!dn) + return 0; + + for (pdn = dn; pdn != NULL; pdn = pdn->parent) { + pcie_link_speed_stats = (const uint32_t *) of_get_property(dn, + "ibm,pcie-link-speed-stats", NULL); + if (pcie_link_speed_stats) + break; + } + + if (!pcie_link_speed_stats) { + pr_err("no ibm,pcie-link-speed-stats property\n"); + return 0; + } + + switch (pcie_link_speed_stats[0]) { + case 0x01: + bus->max_bus_speed = PCIE_SPEED_2_5GT; + break; + case 0x02: + bus->max_bus_speed = PCIE_SPEED_5_0GT; + break; + default: + bus->max_bus_speed = PCI_SPEED_UNKNOWN; + break; + } + + switch (pcie_link_speed_stats[1]) { + case 0x01: + bus->cur_bus_speed = PCIE_SPEED_2_5GT; + break; + case 0x02: + bus->cur_bus_speed = PCIE_SPEED_5_0GT; + break; + default: + bus->cur_bus_speed = PCI_SPEED_UNKNOWN; + break; + } + + return 0; +} diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h index 9a3dda0..b79393d 100644 --- a/arch/powerpc/platforms/pseries/pseries.h +++ b/arch/powerpc/platforms/pseries/pseries.h @@ -60,4 +60,8 @@ extern int dlpar_detach_node(struct device_node *); /* Snooze Delay, pseries_idle */ DECLARE_PER_CPU(long, smt_snooze_delay); +/* PCI root bridge prepare function override for pseries */ +struct pci_host_bridge; +int pseries_root_bridge_prepare(struct pci_host_bridge *bridge); + #endif /* _PSERIES_PSERIES_H */ diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c index 8bcc9ca..bf34cc9 100644 --- a/arch/powerpc/platforms/pseries/setup.c +++ b/arch/powerpc/platforms/pseries/setup.c @@ -466,6 +466,8 @@ static void __init pSeries_setup_arch(void) else ppc_md.enable_pmcs = power4_enable_pmcs; + ppc_md.pcibios_root_bridge_prepare = pseries_root_bridge_prepare; + if (firmware_has_feature(FW_FEATURE_SET_MODE)) { long rc; if ((rc = pSeries_enable_reloc_on_exc()) != H_SUCCESS) {