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

[Segher Boessenkool <segher@kernel.crashing.org>]

>>> +		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?

I just mean you should document what the two "reg" regions for this
device are meant to represent.  "phb reg" isn't really verbose enough 
;-)

>>> +			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...

Sure, but this is about correctness, not performance.

>>> +			/*
>>> +			 * 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.

Great.  Could you please fix the comment to just say this, then?

>> What about PCI DMA, is that identity mapped?
>>
> Not thinking about it atm, should "just work" /*though it never really 
> does*/ :)

Okay, if it's identity mapped, you don't need any properties in the
device tree for it.  Good :-)


Segher