[RFC PATCH 04/19] powerpc: wii: device tree

[Benjamin Herrenschmidt]

On Wed, 2009-11-25 at 18:49 +0100, Segher Boessenkool wrote:
> > +/memreserve/ 0x01800000 0xe800000;	/* memory hole (includes I/O area) */
> 
> Like others have said already, don't do this.  If you need
> a workaround, put it in the platform code.
> 
> > +/memreserve/ 0x10000000 0x0004000;	/* DSP RAM */
> 
> This address is fixed in the DSP hardware, right?  If not,
> you shouldn't do a reserve here.
> 
> > +	chosen {
> > +		/* root filesystem on 2nd partition of SD card */
> > +		bootargs = "nobats root=/dev/mmcblk0p2 rootwait udbg-immortal";
> 
> Question: why do you need/want nobats?

Good point. I can't even guarantee that the kernel will work reliably
with nobats :-) At least you really want the kernel .text to be fully
covered by an IBAT.

> What does this clock freq mean?  Hollywood has lots of
> clocks, many of them configurable.  Bus frequency is in
> the cpu node already.  The binding doesn't say what this
> is either, since you didn't write a binding :-)
> 
> Just remove it?

BTW. If we want to play with clocks, maybe you should look at
my proposed binding for clocks and implementing the clk API :-)

> > +		ranges = <0x0c000000 0x0c000000 0x00010000
> > +			  0x0d000000 0x0d000000 0x00010000
> > +			  0x0d040000 0x0d040000 0x00050000
> > +			  0x0d800000 0x0d800000 0x00001000
> 
> All of 0cXXXXXX and 0dXXXXXX actually.
> 
> > +			  0x133e0000 0x133e0000 0x00c20000>;
> 
> This is just part of MEM2, don't treat it special here.
> 
> > +		video at 0c002000 {
> > +			compatible = "nintendo,hollywood-video";
> > +			reg = <0x0c002000 0x100>;
> > +			interrupts = <8>;
> > +			interrupt-parent = <&PIC0>;
> 
> If you say interrupt-parent = <..> in the root node, you can
> leave it out in most of the rest of the tree.  Using the Flipper
> PIC for this sounds like a good plan.

Well, for the rest of the tree except stuff whose interrupt parent
is PIC1. With two PICs I prefer being explicit.

> > +		PIC0: pic0 at 0c003000 {
> > +			#interrupt-cells = <1>;
> > +			compatible = "nintendo,flipper-pic";
> > +			reg = <0x0c003000 0x8>;
> 
> This register block is bigger actually.  It's not only PIC,
> but some other bus stuff as well, dunno what to do here.

Add nodes for the other things ?

> > +			interrupt-controller;
> > +		};
> > +
> > +		resetswitch at 0c003000 {
> > +			compatible = "nintendo,hollywood-resetswitch";
> > +			reg = <0x0c003000 0x4>;
> 
> That's the same address.  Don't do that.
> 
> Create a flipper-processor-interface node for the whole 3000
> block (size 100)?  You can hang the PIC as a subnode under it,
> without a "reg".

Yeah.

> > +		/* Team Twiizers' 'mini' firmware IPC */
> > +		mini at 0d000000 {
> 
> Although mini is awesome, it isn't hardware, and doesn't
> belong in the device tree.

So what is at d0000000 ?

> > +		serial at 0d006400 {
> > +			compatible = "nintendo,hollywood-serial";
> 
> You might want to pick a more descriptive name for this,
> "serial" is usually understaood to mean "RS232".  Which
> this isn't.

What is it then ? You definitely want some other name if it's not
classic rs232 style serial.

Cheers,
Ben.