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

On 07/24/2012 09:42 PM, Jia Hongtao-B38951 wrote:

quoted

-----Original Message-----
From: Wood Scott-B07421
Sent: Wednesday, July 25, 2012 2:48 AM
To: Jia Hongtao-B38951
Cc: <redacted>; <redacted>; 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:

quoted

PCI host bridge is primary bus if it contains an ISA node. But not all

boards

quoted

fit this rule. Device tree should be updated for all these boards.

Signed-off-by: Jia Hongtao <redacted>
Signed-off-by: Li Yang <redacted>
---
 arch/powerpc/include/asm/pci-bridge.h |    1 +
 arch/powerpc/sysdev/fsl_pci.c         |   31 ++++++++++++++++++++++++-

------

quoted

 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

quoted

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

quoted

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)

quoted

 	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

quoted

+ * 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))) {

quoted

+		/* 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