Patchwork [3/6] powerpc/fsl-pci: Determine primary bus by looking for ISA node

login
register
mail settings
Submitter Hongtao Jia
Date July 24, 2012, 10:20 a.m.
Message ID <1343125210-16720-3-git-send-email-B38951@freescale.com>
Download mbox | patch
Permalink /patch/172824/
State Superseded
Delegated to: Kumar Gala
Headers show

Comments

Hongtao Jia - July 24, 2012, 10:20 a.m.
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(-)
Scott Wood - July 24, 2012, 6:47 p.m.
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
Hongtao Jia - July 25, 2012, 2:42 a.m.
> -----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.
Hongtao Jia - July 25, 2012, 9:01 a.m.
> > +/*
> > + * 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.
Scott Wood - July 25, 2012, 5:25 p.m.
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
Scott Wood - July 25, 2012, 5:27 p.m.
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
Hongtao Jia - July 26, 2012, 2:20 a.m.
> -----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.
Scott Wood - July 27, 2012, 1:34 a.m.
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
Hongtao Jia - July 27, 2012, 2:07 a.m.
> -----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.
Scott Wood - July 27, 2012, 4:37 p.m.
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

Patch

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);