Re: [PATCH 4/15] [POWERPC] pci32: Add flags modifying the PCI code behaviour

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

> On Fri, Dec 14, 2007 at 03:56:05PM +1100, Benjamin Herrenschmidt wrote:
> 
> > --- linux-merge.orig/arch/powerpc/kernel/pci_32.c	2007-12-14 15:49:28.000000000 +1100
> > +++ linux-merge/arch/powerpc/kernel/pci_32.c	2007-12-14 15:49:29.000000000 +1100
> > @@ -48,7 +51,7 @@ static u8* pci_to_OF_bus_map;
> >  /* By default, we don't re-assign bus numbers. We do this only on
> >   * some pmacs
> >   */
> > -int pci_assign_all_buses;
> > +static int pci_assign_all_buses;
> >  
> 
> Why not remove it alltogether, there's still the function to test the
> flag that's just as cheap (load + test + branch).

I don't remember... felt like I had a good reason back then but I don't
remember it :-) I'll have another look.

> > Index: linux-merge/include/asm-powerpc/pci-bridge.h
> > ===================================================================
> > --- linux-merge.orig/include/asm-powerpc/pci-bridge.h	2007-12-14 15:49:01.000000000 +1100
> > +++ linux-merge/include/asm-powerpc/pci-bridge.h	2007-12-14 15:49:29.000000000 +1100
> > @@ -13,6 +13,26 @@
> >  
> >  struct device_node;
> >  
> > +extern unsigned int ppc_pci_flags;
> 
> Should the below be a named enum and the type for the above be the same
> for nicer decode in gdb?
> 
> > +enum {
> > +	/* Force re-assigning all resources (ignore firmware
> > +	 * setup completely)
> > +	 */
> > +	PPC_PCI_REASSIGN_ALL_RSRC	= 0x00000001,
> 
> This should be ..._RSRCS (resources, not resource)
> 
> > +	/* Re-assign all bus numbers */
> > +	PPC_PCI_REASSIGN_ALL_BUS	= 0x00000002,
> 
> And this should be ..._BUSSES
> 
> > +	/* Do not try to assign, just use existing setup */
> > +	PPC_PCI_PROBE_ONLY		= 0x00000004,
> 
> My first reaction was "what's the difference between setting this and
> keeping the two other cleared". Looks like the difference is the
> assignment of unassigned resources. Not sure how to better represent
> that so it's obvious from the naming (without doing something as
> excessive as PPC_PCI_REASSIGN_UNASSIGNED_RSRCS :)

Well... hence "RE" in "REASSIGN" ...

By default, we assign only unassigned resources... (or the ones that
collide). I could add a comment to that effect.

> > +
> > +	/* Don't bother with ISA alignment unless the bridge has
> > +	 * ISA forwarding enabled
> > +	 */
> > +	PPC_PCI_CAN_SKIP_ISA_ALIGN	= 0x00000008,
> > +};

Ben.