Re: [PATCH] powerpc: Add MPC837x PCIE controller RC mode support

[Li Li <r64360@freescale.com>]

On Wed, 2008-01-02 at 19:53 +0800, Arnd Bergmann wrote:
> On Wednesday 02 January 2008, Li Li wrote: 
> >  #ifdef CONFIG_PCI 
> > -       for_each_compatible_node(np, "pci", "fsl,mpc8349-pci") 
> > -               mpc83xx_add_bridge(np); 
> > +       for_each_compatible_node(np, "pci", "fsl,mpc8349-pci") { 
> > +               if (primary_pci_bus) { 
> > +                       mpc83xx_add_bridge(np, PPC_83XX_PCI |
> PPC_83XX_PCI_PRIMARY); 
> > +                       primary_pci_bus = 0; 
> > +               } else 
> > +                       mpc83xx_add_bridge(np, PPC_83XX_PCI); 
> > +       } 
> > + 
> > +       for_each_compatible_node(np, "pci", "fsl,mpc8377-pcie") { 
> > +               if (primary_pci_bus) { 
> > +                       mpc83xx_add_bridge(np, PPC_83XX_PCIE |
> PPC_83XX_PCI_PRIMARY); 
> > +                       primary_pci_bus = 0; 
> > +               } else 
> > +                       mpc83xx_add_bridge(np, PPC_83XX_PCIE); 
> > +       } 
> > + 
> > +       ppc_md.pci_exclude_device = mpc837x_exclude_device; 
> >  #endif
> 
> A few comments on how I think this can be improved:
> 
> * Instead of duplicating this code across all platforms, make a
> single 
> function that does the probing in one place (including the #ifdef 
> CONFIG_PCI).
> 

This code appears in 834x board in first and then copied from one board
to another. It is a legacy issue. :(
Yes. it is the time to remove this duplicates.

> * Better yet, get rid of mpc83xx_add_bridge and make it possible for
> the 
> mpc83xx code to use the of_pci_phb_driver from 
> arch/powerpc/kernel/of_platform.c. This used to be impossible because 
> of the differences  between 32 and 64 bit code for PCI probing, but 
> I think with the work than benh put into unifying the two, we should 
> be pretty close now.
> 

This is really a huge task and almost affects all exist boards. We need
more opinions in here. 


> * The detection method for the primary bus is somewhat fragile,
> because 
> we depend on the order of the nodes in the device tree, which is not 
> specified to have a meaning. /Maybe/ there could be a property in 
> (at most) one of the PCI host bridge nodes saying that this specific
> bus 
> is the primary one.
> 

Put this primary in code is more save than in dts.
The dts lacks error checking function. If provide error dts such as two
pci hosts both has primary property will occur chaos.

> * Since you are using exactly the same probing code for pci and pcie, 
> you may want to check for a more generic "compatible" property than 
> the specific model, so you can use the same code for both.
> 
>         Arnd <><
>