| Message ID | 1343125210-16720-3-git-send-email-B38951@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kumar Gala |
| Headers | show |
On 07/24/2012 05:20 AM, Jia Hongtao wrote: > PCI host bridge is primary bus if it contains an ISA node. But not all boards > fit this rule. Device tree should be updated for all these boards. > > Signed-off-by: Jia Hongtao <B38951@freescale.com> > Signed-off-by: Li Yang <leoli@freescale.com> > --- > arch/powerpc/include/asm/pci-bridge.h | 1 + > arch/powerpc/sysdev/fsl_pci.c | 31 ++++++++++++++++++++++++------- > arch/powerpc/sysdev/fsl_pci.h | 12 +++++++++++- > 3 files changed, 36 insertions(+), 8 deletions(-) > > diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h > index ac39e6a..b48fa7f 100644 > --- a/arch/powerpc/include/asm/pci-bridge.h > +++ b/arch/powerpc/include/asm/pci-bridge.h > @@ -20,6 +20,7 @@ struct device_node; > struct pci_controller { > struct pci_bus *bus; > char is_dynamic; > + int is_primary; > #ifdef CONFIG_PPC64 > int node; > #endif > diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c > index 99a3e78..2a369be 100644 > --- a/arch/powerpc/sysdev/fsl_pci.c > +++ b/arch/powerpc/sysdev/fsl_pci.c > @@ -453,6 +453,7 @@ int __init fsl_add_bridge(struct device_node *dev, int is_primary) > > hose->first_busno = bus_range ? bus_range[0] : 0x0; > hose->last_busno = bus_range ? bus_range[1] : 0xff; > + hose->is_primary = is_primary; > > setup_indirect_pci(hose, rsrc.start, rsrc.start + 0x4, > PPC_INDIRECT_TYPE_BIG_ENDIAN); > @@ -932,18 +933,34 @@ void pci_check_swiotlb(void) > } > #endif > > -int primary_phb_addr; > +/* > + * Recursively scan all the children nodes of parent and find out if there > + * is "isa" node. Return 1 if parent has isa node otherwise return 0. > + */ > +int has_isa_node(struct device_node *parent) > +{ > + static int result; > + struct device_node *cur_child; > + > + cur_child = NULL; > + result = 0; > + while (!result && (cur_child = of_get_next_child(parent, cur_child))) { > + /* Get "isa" node and return 1 */ > + if (of_node_cmp(cur_child->type, "isa") == 0) > + return result = 1; > + has_isa_node(cur_child); > + } > + > + return result; > +} Why are you reimplementing this? It's already in Linus's tree. See fsl_pci_init(). Plus, your version is recursive which is unacceptable in kernel code with a small stack (outside of a few rare examples where the depth has a small fixed upper bound), and once it finds an ISA node, it returns 1 forever, regardless of what node you pass in in the future. -Scott
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Wednesday, July 25, 2012 2:48 AM > To: Jia Hongtao-B38951 > Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org; Wood Scott- > B07421; Li Yang-R58472 > Subject: Re: [PATCH 3/6] powerpc/fsl-pci: Determine primary bus by > looking for ISA node > > On 07/24/2012 05:20 AM, Jia Hongtao wrote: > > PCI host bridge is primary bus if it contains an ISA node. But not all > boards > > fit this rule. Device tree should be updated for all these boards. > > > > Signed-off-by: Jia Hongtao <B38951@freescale.com> > > Signed-off-by: Li Yang <leoli@freescale.com> > > --- > > arch/powerpc/include/asm/pci-bridge.h | 1 + > > arch/powerpc/sysdev/fsl_pci.c | 31 ++++++++++++++++++++++++- > ------ > > arch/powerpc/sysdev/fsl_pci.h | 12 +++++++++++- > > 3 files changed, 36 insertions(+), 8 deletions(-) > > > > diff --git a/arch/powerpc/include/asm/pci-bridge.h > b/arch/powerpc/include/asm/pci-bridge.h > > index ac39e6a..b48fa7f 100644 > > --- a/arch/powerpc/include/asm/pci-bridge.h > > +++ b/arch/powerpc/include/asm/pci-bridge.h > > @@ -20,6 +20,7 @@ struct device_node; > > struct pci_controller { > > struct pci_bus *bus; > > char is_dynamic; > > + int is_primary; > > #ifdef CONFIG_PPC64 > > int node; > > #endif > > diff --git a/arch/powerpc/sysdev/fsl_pci.c > b/arch/powerpc/sysdev/fsl_pci.c > > index 99a3e78..2a369be 100644 > > --- a/arch/powerpc/sysdev/fsl_pci.c > > +++ b/arch/powerpc/sysdev/fsl_pci.c > > @@ -453,6 +453,7 @@ int __init fsl_add_bridge(struct device_node *dev, > int is_primary) > > > > hose->first_busno = bus_range ? bus_range[0] : 0x0; > > hose->last_busno = bus_range ? bus_range[1] : 0xff; > > + hose->is_primary = is_primary; > > > > setup_indirect_pci(hose, rsrc.start, rsrc.start + 0x4, > > PPC_INDIRECT_TYPE_BIG_ENDIAN); > > @@ -932,18 +933,34 @@ void pci_check_swiotlb(void) > > } > > #endif > > > > -int primary_phb_addr; > > +/* > > + * Recursively scan all the children nodes of parent and find out if > there > > + * is "isa" node. Return 1 if parent has isa node otherwise return 0. > > + */ > > +int has_isa_node(struct device_node *parent) > > +{ > > + static int result; > > + struct device_node *cur_child; > > + > > + cur_child = NULL; > > + result = 0; > > + while (!result && (cur_child = of_get_next_child(parent, > cur_child))) { > > + /* Get "isa" node and return 1 */ > > + if (of_node_cmp(cur_child->type, "isa") == 0) > > + return result = 1; > > + has_isa_node(cur_child); > > + } > > + > > + return result; > > +} > > Why are you reimplementing this? It's already in Linus's tree. See > fsl_pci_init(). > > Plus, your version is recursive which is unacceptable in kernel code > with a small stack (outside of a few rare examples where the depth has a > small fixed upper bound), and once it finds an ISA node, it returns 1 > forever, regardless of what node you pass in in the future. > > -Scott About recursion I will do some more investigation. If it finds ISA it returns 1. But for next PCI node it will return 0 if no ISA is found (note that I set result to 0 at the beginning). -Hongtao.
> > +/* > > + * Recursively scan all the children nodes of parent and find out if > there > > + * is "isa" node. Return 1 if parent has isa node otherwise return 0. > > + */ > > +int has_isa_node(struct device_node *parent) > > +{ > > + static int result; > > + struct device_node *cur_child; > > + > > + cur_child = NULL; > > + result = 0; > > + while (!result && (cur_child = of_get_next_child(parent, > cur_child))) { > > + /* Get "isa" node and return 1 */ > > + if (of_node_cmp(cur_child->type, "isa") == 0) > > + return result = 1; > > + has_isa_node(cur_child); > > + } > > + > > + return result; > > +} > > Why are you reimplementing this? It's already in Linus's tree. See > fsl_pci_init(). > > Plus, your version is recursive which is unacceptable in kernel code > with a small stack (outside of a few rare examples where the depth has a > small fixed upper bound), and once it finds an ISA node, it returns 1 > forever, regardless of what node you pass in in the future. > > -Scott Yes, recursive function is not recommended for kernel but maybe it's not unacceptable. This function is not so deep stacked and simple. In my opinion this is acceptable. Thanks. -Hongtao.
On 07/24/2012 09:42 PM, Jia Hongtao-B38951 wrote: > > >> -----Original Message----- >> From: Wood Scott-B07421 >> Sent: Wednesday, July 25, 2012 2:48 AM >> To: Jia Hongtao-B38951 >> Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org; Wood Scott- >> B07421; Li Yang-R58472 >> Subject: Re: [PATCH 3/6] powerpc/fsl-pci: Determine primary bus by >> looking for ISA node >> >> On 07/24/2012 05:20 AM, Jia Hongtao wrote: >>> PCI host bridge is primary bus if it contains an ISA node. But not all >> boards >>> fit this rule. Device tree should be updated for all these boards. >>> >>> Signed-off-by: Jia Hongtao <B38951@freescale.com> >>> Signed-off-by: Li Yang <leoli@freescale.com> >>> --- >>> arch/powerpc/include/asm/pci-bridge.h | 1 + >>> arch/powerpc/sysdev/fsl_pci.c | 31 ++++++++++++++++++++++++- >> ------ >>> arch/powerpc/sysdev/fsl_pci.h | 12 +++++++++++- >>> 3 files changed, 36 insertions(+), 8 deletions(-) >>> >>> diff --git a/arch/powerpc/include/asm/pci-bridge.h >> b/arch/powerpc/include/asm/pci-bridge.h >>> index ac39e6a..b48fa7f 100644 >>> --- a/arch/powerpc/include/asm/pci-bridge.h >>> +++ b/arch/powerpc/include/asm/pci-bridge.h >>> @@ -20,6 +20,7 @@ struct device_node; >>> struct pci_controller { >>> struct pci_bus *bus; >>> char is_dynamic; >>> + int is_primary; >>> #ifdef CONFIG_PPC64 >>> int node; >>> #endif >>> diff --git a/arch/powerpc/sysdev/fsl_pci.c >> b/arch/powerpc/sysdev/fsl_pci.c >>> index 99a3e78..2a369be 100644 >>> --- a/arch/powerpc/sysdev/fsl_pci.c >>> +++ b/arch/powerpc/sysdev/fsl_pci.c >>> @@ -453,6 +453,7 @@ int __init fsl_add_bridge(struct device_node *dev, >> int is_primary) >>> >>> hose->first_busno = bus_range ? bus_range[0] : 0x0; >>> hose->last_busno = bus_range ? bus_range[1] : 0xff; >>> + hose->is_primary = is_primary; >>> >>> setup_indirect_pci(hose, rsrc.start, rsrc.start + 0x4, >>> PPC_INDIRECT_TYPE_BIG_ENDIAN); >>> @@ -932,18 +933,34 @@ void pci_check_swiotlb(void) >>> } >>> #endif >>> >>> -int primary_phb_addr; >>> +/* >>> + * Recursively scan all the children nodes of parent and find out if >> there >>> + * is "isa" node. Return 1 if parent has isa node otherwise return 0. >>> + */ >>> +int has_isa_node(struct device_node *parent) >>> +{ >>> + static int result; >>> + struct device_node *cur_child; >>> + >>> + cur_child = NULL; >>> + result = 0; >>> + while (!result && (cur_child = of_get_next_child(parent, >> cur_child))) { >>> + /* Get "isa" node and return 1 */ >>> + if (of_node_cmp(cur_child->type, "isa") == 0) >>> + return result = 1; >>> + has_isa_node(cur_child); >>> + } >>> + >>> + return result; >>> +} >> >> Why are you reimplementing this? It's already in Linus's tree. See >> fsl_pci_init(). >> >> Plus, your version is recursive which is unacceptable in kernel code >> with a small stack (outside of a few rare examples where the depth has a >> small fixed upper bound), and once it finds an ISA node, it returns 1 >> forever, regardless of what node you pass in in the future. >> >> -Scott > > About recursion I will do some more investigation. > If it finds ISA it returns 1. But for next PCI node it will return 0 if > no ISA is found (note that I set result to 0 at the beginning). Sorry, I misread that as an initializer. Still, it's awkward (why not just use a return value?) and non-thread-safe (may not matter here, but it's a bad habit), and it's still an unacceptable use of recursion, and still a reimplementation of functionality that already exists. :-) -Scott
On 07/25/2012 04:01 AM, Jia Hongtao-B38951 wrote: >>> +/* >>> + * Recursively scan all the children nodes of parent and find out if >> there >>> + * is "isa" node. Return 1 if parent has isa node otherwise return 0. >>> + */ >>> +int has_isa_node(struct device_node *parent) >>> +{ >>> + static int result; >>> + struct device_node *cur_child; >>> + >>> + cur_child = NULL; >>> + result = 0; >>> + while (!result && (cur_child = of_get_next_child(parent, >> cur_child))) { >>> + /* Get "isa" node and return 1 */ >>> + if (of_node_cmp(cur_child->type, "isa") == 0) >>> + return result = 1; >>> + has_isa_node(cur_child); >>> + } >>> + >>> + return result; >>> +} >> >> Why are you reimplementing this? It's already in Linus's tree. See >> fsl_pci_init(). >> >> Plus, your version is recursive which is unacceptable in kernel code >> with a small stack (outside of a few rare examples where the depth has a >> small fixed upper bound), and once it finds an ISA node, it returns 1 >> forever, regardless of what node you pass in in the future. >> >> -Scott > > Yes, recursive function is not recommended for kernel but maybe it's not unacceptable. > This function is not so deep stacked and simple. In my opinion this is acceptable. The depth is limited not by code, but by externally provided data. Granted a bad device tree can mess the kernel up in far worse ways, but still it's a bad idea and totally unnecessary. -Scott
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Thursday, July 26, 2012 1:26 AM > To: Jia Hongtao-B38951 > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; > galak@kernel.crashing.org; Li Yang-R58472 > Subject: Re: [PATCH 3/6] powerpc/fsl-pci: Determine primary bus by looking > for ISA node > > >>> +/* > >>> + * Recursively scan all the children nodes of parent and find out if > >> there > >>> + * is "isa" node. Return 1 if parent has isa node otherwise return 0. > >>> + */ > >>> +int has_isa_node(struct device_node *parent) > >>> +{ > >>> + static int result; > >>> + struct device_node *cur_child; > >>> + > >>> + cur_child = NULL; > >>> + result = 0; > >>> + while (!result && (cur_child = of_get_next_child(parent, > >> cur_child))) { > >>> + /* Get "isa" node and return 1 */ > >>> + if (of_node_cmp(cur_child->type, "isa") == 0) > >>> + return result = 1; > >>> + has_isa_node(cur_child); > >>> + } > >>> + > >>> + return result; > >>> +} > >> > >> Why are you reimplementing this? It's already in Linus's tree. See > >> fsl_pci_init(). > >> > >> Plus, your version is recursive which is unacceptable in kernel code > >> with a small stack (outside of a few rare examples where the depth has > a > >> small fixed upper bound), and once it finds an ISA node, it returns 1 > >> forever, regardless of what node you pass in in the future. > >> > >> -Scott > > > > About recursion I will do some more investigation. > > If it finds ISA it returns 1. But for next PCI node it will return 0 if > > no ISA is found (note that I set result to 0 at the beginning). > > Sorry, I misread that as an initializer. Still, it's awkward (why not > just use a return value?) and non-thread-safe (may not matter here, but > it's a bad habit), and it's still an unacceptable use of recursion, and > still a reimplementation of functionality that already exists. :-) > > -Scott All this recursion thing I will try another way. But this is not the same as you did. If we use platform driver probe function will be called more than once. Your function is to find ISA node and check if the parent equal to this PCI controller. My function is to search ISA under each PCI controller. If the platform driver mechanism is used I think this function is necessary and reasonable. -Hongtao.
On 07/25/2012 09:20 PM, Jia Hongtao-B38951 wrote: > All this recursion thing I will try another way. > > But this is not the same as you did. If we use platform driver probe function > will be called more than once. Your function is to find ISA node and check if > the parent equal to this PCI controller. My function is to search ISA under > each PCI controller. The result is the same -- "does this PCI controller have an ISA node under it?" -Scott
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Friday, July 27, 2012 9:34 AM > To: Jia Hongtao-B38951 > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; > galak@kernel.crashing.org; Li Yang-R58472 > Subject: Re: [PATCH 3/6] powerpc/fsl-pci: Determine primary bus by > looking for ISA node > > On 07/25/2012 09:20 PM, Jia Hongtao-B38951 wrote: > > All this recursion thing I will try another way. > > > > But this is not the same as you did. If we use platform driver probe > function > > will be called more than once. Your function is to find ISA node and > check if > > the parent equal to this PCI controller. My function is to search ISA > under > > each PCI controller. > > The result is the same -- "does this PCI controller have an ISA node > under it?" > > -Scott The result is the same but as I said in platform driver probe function will be called for each PCI controller. We don't want ISA is searched for more than once. Also please refer to V3 of this patch set in which I rebase them on latest tree. I made a non-recursive version of function to search for ISA under PCI nodes. -Hongtao.
On 07/26/2012 09:07 PM, Jia Hongtao-B38951 wrote: > > >> -----Original Message----- >> From: Wood Scott-B07421 >> Sent: Friday, July 27, 2012 9:34 AM >> To: Jia Hongtao-B38951 >> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; >> galak@kernel.crashing.org; Li Yang-R58472 >> Subject: Re: [PATCH 3/6] powerpc/fsl-pci: Determine primary bus by >> looking for ISA node >> >> On 07/25/2012 09:20 PM, Jia Hongtao-B38951 wrote: >>> All this recursion thing I will try another way. >>> >>> But this is not the same as you did. If we use platform driver probe >> function >>> will be called more than once. Your function is to find ISA node and >> check if >>> the parent equal to this PCI controller. My function is to search ISA >> under >>> each PCI controller. >> >> The result is the same -- "does this PCI controller have an ISA node >> under it?" >> >> -Scott > > > The result is the same but as I said in platform driver probe function will > be called for each PCI controller. So? Just because you've got a platform device now doesn't mean you can't do any early global init. This has to be done globally, so we can choose a fallback primary bus if there's no ISA in the system. -Scott
diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h index ac39e6a..b48fa7f 100644 --- a/arch/powerpc/include/asm/pci-bridge.h +++ b/arch/powerpc/include/asm/pci-bridge.h @@ -20,6 +20,7 @@ struct device_node; struct pci_controller { struct pci_bus *bus; char is_dynamic; + int is_primary; #ifdef CONFIG_PPC64 int node; #endif diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c index 99a3e78..2a369be 100644 --- a/arch/powerpc/sysdev/fsl_pci.c +++ b/arch/powerpc/sysdev/fsl_pci.c @@ -453,6 +453,7 @@ int __init fsl_add_bridge(struct device_node *dev, int is_primary) hose->first_busno = bus_range ? bus_range[0] : 0x0; hose->last_busno = bus_range ? bus_range[1] : 0xff; + hose->is_primary = is_primary; setup_indirect_pci(hose, rsrc.start, rsrc.start + 0x4, PPC_INDIRECT_TYPE_BIG_ENDIAN); @@ -932,18 +933,34 @@ void pci_check_swiotlb(void) } #endif -int primary_phb_addr; +/* + * Recursively scan all the children nodes of parent and find out if there + * is "isa" node. Return 1 if parent has isa node otherwise return 0. + */ +int has_isa_node(struct device_node *parent) +{ + static int result; + struct device_node *cur_child; + + cur_child = NULL; + result = 0; + while (!result && (cur_child = of_get_next_child(parent, cur_child))) { + /* Get "isa" node and return 1 */ + if (of_node_cmp(cur_child->type, "isa") == 0) + return result = 1; + has_isa_node(cur_child); + } + + return result; +} + static int __devinit fsl_pci_probe(struct platform_device *pdev) { - struct pci_controller *hose; bool is_primary; + is_primary = has_isa_node(pdev->dev.of_node); - if (of_match_node(pci_ids, pdev->dev.of_node)) { - struct resource rsrc; - of_address_to_resource(pdev->dev.of_node, 0, &rsrc); - is_primary = ((rsrc.start & 0xfffff) == primary_phb_addr); + if (of_match_node(pci_ids, pdev->dev.of_node)) fsl_add_bridge(pdev->dev.of_node, is_primary); - } return 0; } diff --git a/arch/powerpc/sysdev/fsl_pci.h b/arch/powerpc/sysdev/fsl_pci.h index c2c1de5..abbc09d 100644 --- a/arch/powerpc/sysdev/fsl_pci.h +++ b/arch/powerpc/sysdev/fsl_pci.h @@ -88,7 +88,17 @@ struct ccsr_pci { __be32 pex_err_cap_r3; /* 0x.e34 - PCIE error capture register 0 */ }; -extern int primary_phb_addr; + +#ifdef CONFIG_SUSPEND +struct fsl_pci_private_data { + int inbound_num; + struct pci_outbound_window_regs __iomem *pci_pow; + struct pci_inbound_window_regs __iomem *pci_piw; + void *saved_regs; +}; +#endif + +extern int is_has_isa_node(struct device_node *parent); extern int fsl_add_bridge(struct device_node *dev, int is_primary); extern void fsl_pcibios_fixup_bus(struct pci_bus *bus); extern int mpc83xx_add_bridge(struct device_node *dev);