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

[Jia Hongtao-B38951]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Monday, August 06, 2012 11:10 PM
> To: Jia Hongtao-B38951
> Cc: Wood Scott-B07421; linuxppc-dev at lists.ozlabs.org;
> galak at kernel.crashing.org; Li Yang-R58472
> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie
> initialization code
> 
> On 08/05/2012 10:07 PM, Jia Hongtao-B38951 wrote:
> >
> >
> >> -----Original Message-----
> >> From: Wood Scott-B07421
> >> Sent: Saturday, August 04, 2012 12:28 AM
> >> To: Jia Hongtao-B38951
> >> Cc: linuxppc-dev at lists.ozlabs.org; galak at 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:
> >>> -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.
> 
> > 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.
> 
> >>> +
> >>> +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.
> >> */
> >>> -		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.
> 
> > I read your code and found that if there is no ISA node you will
> > assign the first PCI bus scanned as primary. It's not all right. Take
> > ge_imp3a as an
> > example: The second PCI bus (9000) is primary not the first one.
> 
> Does that board have ISA on it, that isn't described by the device tree?
>  If so, before converting to the new init mechanism, the board code will
> need to set fsl_pci_primary based on its own knowledge of where that ISA
> is.  If it doesn't have ISA, it doesn't matter which one we designate as
> primary.
> 
> > I doubt that there are bugs if no primary assigned.
> 
> Yeah, I just implemented the fallback for fun.  Come on.
> 
> It was recently discussed on this list.  PCI under QEMU did not work
> without it.
> 
> > Like mpc85xx_rdb assigned
> > no primary at all. Some other boards has no primary ether like
> > p1022ds, p1021mds, p1010rdb, p1023rds, all corenet boards (p2041_rdb,
> > p3041_ds, p4080_ds, p5020_ds, p5040_ds). If no primary is a bug then
> > all these boards above are not correctly setting up.
> 
> Those boards are not being correctly set up.  On real hardware things
> work by chance, but not under QEMU.
> 
> -Scott

I am really not sure that all boards need primary bus. Could you give me
the link of discussion about primary that you mentioned?

-Hongtao.