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

[Milton Miller <miltonm@bga.com>]

On Apr 30, 2007, at 1:19 PM, Mark A. Greer wrote:

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

>>> +               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?
That would be ok but that could be precompiled, vs set in code.

I was thinking you wanted to expose the vpd determined specific
bridge to the kernel / user.

>>> +/* 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\
>>> +");
>>>
>
> 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'?

I think you will find that cpp doesn't replace the define inside
the string.   And by the time you stop the string, stringify the
define, and restart the string, you might as well pass it as a
parameter.  Also, in C we sometimes add things like ULL that
aren't valid in assembler, although that's more of a 64 bit issue.

milton