Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code

On Mon, Aug 6, 2012 at 11:09 PM, Scott Wood [off-list ref] wrote:

On 08/05/2012 10:07 PM, Jia Hongtao-B38951 wrote:

quoted

quoted

-----Original Message-----
From: Wood Scott-B07421
Sent: Saturday, August 04, 2012 12:28 AM
To: Jia Hongtao-B38951
Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org; Li Yang-
R58472; Wood Scott-B07421
Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie
initialization code

On 08/03/2012 05:14 AM, Jia Hongtao wrote:

quoted

-void __devinit fsl_pci_init(void)
+/* Checkout if PCI contains ISA node */
+static int of_pci_has_isa(struct device_node *pci_node)
+{
+   struct device_node *np;
+   int ret = 0;
+
+   if (!pci_node)
+           return 0;
+
+   read_lock(&devtree_lock);
+   np = pci_node->allnext;
+
+   /* Only scan the children of PCI node */
+   for (; np != pci_node->sibling; np = np->allnext) {
+           if (np->type && (of_node_cmp(np->type, "isa") == 0)
+               && of_node_get(np)) {
+                   ret = 1;
+                   break;
+           }
+   }
+
+   of_node_put(pci_node);
+   read_unlock(&devtree_lock);
+
+   return ret;
+}

Why do you keep insisting on substituting your ISA search code here?
What advantages does it have over the code that is already there?  It
unnecessarily digs into the internals of the tree representation.

I want ISA search is done from probe.

Too bad.  You're breaking the case where there's no ISA node.

We can also take care of special cases with our approach if needed.
But it's not correct to assume the first PCI controller is the primary
one if there is no ISA node.  Your approach is still a band-aid to me.
 We can come back to this issue when we do find a proper solution.

quoted

Also this way is more efficient due
to we just search the children of PCI.

It is not more efficient, because you're doing the search for every PCIe
bus rather than once.  Not that it matters in this context.

We end up scanning at most a few PCI nodes instead of the whole device
tree for the primary.

quoted

quoted

quoted

+
+static int __devinit fsl_pci_probe(struct platform_device *pdev)
 {
    int ret;
-   struct device_node *node;
    struct pci_controller *hose;
-   dma_addr_t max = 0xffffffff;
+   int is_primary = 0;

-   /* Callers can specify the primary bus using other means. */
    if (!fsl_pci_primary) {
-           /* If a PCI host bridge contains an ISA node, it's primary.

*/

quoted

-           node = of_find_node_by_type(NULL, "isa");
-           while ((fsl_pci_primary = of_get_parent(node))) {
-                   of_node_put(node);
-                   node = fsl_pci_primary;
-
-                   if (of_match_node(pci_ids, node))
-                           break;
-           }
+           is_primary = of_pci_has_isa(pdev->dev.of_node);
+           if (is_primary)
+                   fsl_pci_primary = pdev->dev.of_node;
    }

As I explained before, this has to be done globally, not from the probe
function, so we can assign a default primary bus if there isn't any ISA.
 There are bugs in the Linux PPC PCI code relating to not having any
primary bus.

-Scott

In my way of searching ISA you can also assign a default primary bus in board
specific files.

That was meant for when the board file had an alternate way of searching
for the primary bus (e.g. look for i8259), not as a replacement for the
mechanism that guarantees there's a primary bus.

You are causing a regression in the qemu_e500.c platform.

Can we fix the qemu device tree to address the problem if we do make
it a rule to use the ISA node to indicate the primary bus?

- Leo