Re: [PATCH 2/3] [POWERPC] Add pci node to sequoia dts

[Vitaly Bordug <vitb@kernel.crashing.org>]

On Sat, 25 Aug 2007 11:49:58 +0200
Segher Boessenkool wrote:

> > +		pci {
> > +    			reg = <1 eec00000 40 1 ef400000
> > 40>;  /* phb cfg, phb reg */
> 
> First component of reg is the unit address, so: pci@1eec00000 .
> 
> "phb cfg" is how you access PCI configuration space?  It wouldn't
> hurt to document that, either in a little binding or just here in
> the code.
> 
mmm, that was what my poor upper comment about, exactly.
do you mean that "PCI configuration space xxxx_xxxx, PCI register at xxxx_xxxx" will look more
appropriate?

> > +			bus-range = <0 0>;
> 
> Can't you have subordinate PCI busses?  Or are there no slots :-)
> 
Even if there are (and I dunno - Stefan did the HW validation and updates, since he has actual target),
the performance of such a beast would be low, with one shared irq for everybody...
> > +			/*
> > +			 * mem is at 80000000 set up indirectly
> > +			 * io is at 0001_e800_0000
> > +			 */
> > +			ranges = <02000000 0 80000000 1 80000000 0
> > 10000000
> > +				01000000 0 00000000 1 e8000000 0
> > 00100000>;
> 
> Comment doesn't match code for the memory space.  What does "set
> up indirectly" mean here?  Oh wait, you want to say that the host
> addresses 1_8000_0000..1_8fff_ffff are translated to PCI addresses
> 8000_0000..8fff_ffff.
> 
Yes, exactly.
> What about PCI DMA, is that identity mapped?
> 
Not thinking about it atm, should "just work" /*though it never really does*/ :)
> > +			#interrupt-cells = <1>;
> > +			#size-cells = <2>;
> > +			#address-cells = <3>;
> 
> The reverse order of these is more conventional.  Not that it
> actually matters ;-)

ok, I'll reorder them
> 
> > +			compatible = "ibm, 440epx";
> 
> Stray space.  And you need to say it is the PCI host, so something
> like "ibm,440epx-pci".
> 
OK, will have "amcc,.440epx-pci" here

-- 
Sincerely, Vitaly