[PATCH 1/5] PowerPC 74xx: Katana Qp device tree

[Mark A. Greer]

On Mon, Dec 03, 2007 at 12:50:18PM +1100, David Gibson wrote:
> On Thu, Nov 29, 2007 at 06:28:36PM +0300, Andrei Dolnikov wrote:
> > Device tree source file for the Emerson Katana Qp board
> > 
> > Signed-off-by: Andrei Dolnikov <adolnikov at ru.mvisa.com>
> > 
> > ---
> >  katanaqp.dts |  360 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 360 insertions(+)
> > 
> > diff --git a/arch/powerpc/boot/dts/katanaqp.dts b/arch/powerpc/boot/dts/katanaqp.dts
> > new file mode 100644
> > index 0000000..98257a2
> > --- /dev/null
> > +++ b/arch/powerpc/boot/dts/katanaqp.dts
> > @@ -0,0 +1,360 @@
> > +/* Device Tree Source for Emerson Katana Qp
> > + *
> > + * Authors: Vladislav Buzov <vbuzov at ru.mvista.com>
> > + *	    Andrei Dolnikov <adolnikov at ru.mvista.com>
> > + * 
> > + * Based on prpmc8200.dts by Mark A. Greer <mgreer at mvista.com>
> > + *
> > + * 2007 (c) MontaVista, Software, Inc.  This file is licensed under
> > + * the terms of the GNU General Public License version 2.  This program
> > + * is licensed "as is" without any warranty of any kind, whether express
> > + * or implied.
> > + *
> > + */
> > +
> > +/ {
> > +	#address-cells = <1>;
> > +	#size-cells = <1>;
> > +	model = "Katana-Qp"; /* Default */
> > +	compatible = "emerson,Katana-Qp";
> > +	coherency-off;
> 
> What is this property for?

Its needed to tell the bootwrapper that the platform does not have
coherency enabled (since our policy is that you can't use a CONFIG_ option).
The bootwrapper needs to know that if/when it sets up the windows for
its I/O devices (e.g., enet, mpsc) to system memory.  I needed to do
this on the prpmc2800 because the firmware didn't set up those windows
correctly.  I don't know if the Katana Qp's firmware sets the up
correctly or not.

> > +	mv64x60 at f8100000 { /* Marvell Discovery */
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		model = "mv64460";			/* Default */
> > +		compatible = "marvell,mv64x60";
> 
> Compatible properties should not have "x" asn in 64x60 here.  If
> there's a suitable name for the general register interface use that,
> otherwise use the specific model number of the earliest device to
> implement this register interface.  Later models should have a
> compatible property which lists their specific model, followed by the
> earlier model number with which they're compatible.

This came from the prpmc2800's dts which has become out-of-date.
Both dts files need to be updated.

> > +		ethernet at 2000 {
> > +			reg = <2000 2000>;
> 
> Are the registers for the 3 ethernets really all together?  This bank
> can't be subdivided into seperate register blocks for each MAC?

Unfortunately there are some registers that are shared so you can't
divide them up nicely.

> > +			eth0 {
> > +				device_type = "network";
> > +				compatible = "marvell,mv64x60-eth";
> > +				block-index = <0>;
> 
> This block-index thing is crap.  If you really need to subindex nodes
> like this, use "reg", with an appropriate #address-cells in the
> parent, then the nodes will also get sensible unit addresses.

So how would that work for the "PHY Address Register 0x2000", say,
where bits 0-4 set the device addr for PHY 0; bits 5-9 set the device
addr for PHY 1; bts 10-14 set the devce addr for PHY 2?

> > +				interrupts = <20>;
> > +				interrupt-parent = <&/mv64x60/pic>;
> 
> You should use a label for the PIC to make things more readable.

More that needs to be updated in prpmc2800.  :(

> > +		sdma at 4000 {
> > +			compatible = "marvell,mv64x60-sdma";
> > +			reg = <4000 c18>;
> > +			virtual-reg = <f8104000>;
> 
> Why does this node have virtual-reg?

"virtual-reg" is a special property that doesn't get translated thru
the parent mappings.  It should never be used in the kernel.  Its
purpose is to give code in the bootwrapper the exact address that it
should use to access a register or block of registers or ...
Its needed here because the MPSC (serial) driver uses the SDMA unit
to perform console I/O in the bootwrapper (e.g., cmdline editing, printf's).

Yes, this needs to be documented.

> > +		mpsc at 8000 {
> > +			device_type = "serial";
> > +			compatible = "marvell,mpsc";
> > +			reg = <8000 38>;
> > +			virtual-reg = <f8108000>;
> > +			sdma = <&/mv64x60/sdma at 4000>;
> > +			brg = <&/mv64x60/brg at b200>;
> > +			cunit = <&/mv64x60/cunit at f200>;
> > +			mpscrouting = <&/mv64x60/mpscrouting at b400>;
> > +			mpscintr = <&/mv64x60/mpscintr at b800>;
> > +			block-index = <0>;
> 
> What is this block-index thing about here?  Since the devices are
> disambiguated by their register address, why do you need it?

This particular one is needed to access the correct MPSC interrupt reg.
Maybe it would be better to make a new property for this but it was only
one reg and block-index was already there and basically served that
purpose so I used it.  I'd be happy to use an alternative if you have
something you think is better.

> > +		pic {
> 
> Needs a unit address.

Okay.

> > +		cpu-error at 0070 {
> 
> The unit address should notr include leading zeroes.

Okay.

Mark