[PATCH 12/13] powerpc: Add bootwrapper support for Motorola PrPMC2800 platform

[Mark A. Greer]

On Mon, Apr 30, 2007 at 11:15:23AM -0500, Milton Miller wrote:
> On Thu Apr 26 10:02:27 EST 2007, Mark A. Greer wrote:
> >Add support for Motorola ECC PrPMC280/PrPMC2800 Platform.
> >The PrPMC280 sits on an F101 baseboard and the PrPMC2800 sits on a
> >F101e baseboard.  Logic has been added to determine which board
> >(and variant thereof) the code is being run on.
> 
> >
> >+       /* Update /mv64x60/device_type, if this is a mv64362 */
> >+       if (bip->bridge_type == BRIDGE_TYPE_MV64362) {
> >+               devp = finddevice("/mv64x60");
> >+               if (devp == NULL)
> >+                       fatal("Error: Missing /mv64x60 device tree 
> >node\n\r");
> >+               setprop(devp, "device_type", "mv64362", 
> >strlen("mv64362") + 1);
> >+       }
> >+
> 
> That is not a device_type.   It might be a model,   Or even a 
> compatable.
> but not a type.

How about "host-bridge" or is that too generic?

> >+       platform_ops.exit = prpmc2800_reset;
> 
> No delay to let the user know see what is wrong?

I'll add a delay.

> >+
> >+/* Following code is put at very beginning of zImage (64KB into ELF 
> >file) */
> >+asm (" .globl _zimage_start\n\
> >+       _zimage_start:\n\
> >+               mfmsr   10\n\
> >+               rlwinm  10,10,0,~(1<<15)        /* Clear MSR_EE */\n\
> >+               sync\n\
> >+               mtmsr   10\n\
> >+               isync\n\
> >+               b _zimage_start_lib\n\
> >+");
> >
> 
> That comment is very wrong.   This object might be the first to be 
> linked,
> and the text will be near the top.  But there is nothing in that 
> fragment
> to make it be the beginning of the image.  Not even in the text section.
> 
> That label will be the entrypoiint, so make the comment to that effect.

Yes, its a poorly worded comment.

> Also, the 1<<15 seems magic.  I would like to see it as a constant in
> reg.h,

It probably makes sense to add some MSR definitions to reg.h.

> but to use that you would have to pass it as an i parameter, so

I don't follow.  What's wrong with '#define MSR_EE (1<<15)'
in reg.h and 'rlwinm  10,10,0,~MSR_EE'?

Mark