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.
>
> > + vi...@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: p...@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;
> > + };
> > +
> > + resetswi...@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 */
> > + m...@0d000000 {
>
> Although mini is awesome, it isn't hardware, and doesn't
> belong in the device tree.
So what is at d0000000 ?
> > + ser...@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.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
