On Mon, Feb 24, 2014 at 4:28 PM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 24 February 2014 22:14, Rob Herring <robherri...@gmail.com> wrote:
>> From: Rob Herring <rob.herr...@linaro.org>
>> MPIDR register is a machine configurable option and current kernels require
>> the value to match with DT cpu reg properties. So add a property for MPIDR
>> value and allow platforms to override.
[snip]
>> @@ -1442,6 +1442,11 @@ static int mpidr_read(CPUARMState *env, const
>> ARMCPRegInfo *ri,
>> {
>> CPUState *cs = CPU(arm_env_get_cpu(env));
>> uint32_t mpidr = cs->cpu_index;
>> +
>> + if (ri->resetvalue) {
>> + *value = ri->resetvalue;
>> + return 0;
>> + }
>
> This looks weird, because it's overriding the CPU index. (Also, you have
> access to the CPU object here, you might as well just say "if (cpu->mpidr)"
> rather than feeding it in through the ri->resetvalue when we don't actually
> care about it at reset.)
The cpu index has already been set by the platform, but yes I agree
just using cpu->mpidr will be simpler.
> Should the property actually be "cluster number" or something
> similar? This is how the hardware does it, there are CLUSTERID
> signals to set bits [11:8] of the MPIDR for A9/A15; A57 has
> CLUSTERIDAFF1 and CLUSTERIDAFF2.
The whole concept of cluster isn't architecturally defined beyond
"affinity level" and is specific to those cores. I think it is better
to leave the flexibility to override MPIDR to anything.
Having been asked before, what the h/w folks tie these off to will be
all over the map if left up to their own imaginations. :)
> It might be easier to leave as a single uint32_t property, but I'm
> pretty sure we should be ORing it in with the CPU index and
> bit 31.
Some platform may want to do something like boot on cpu other than
mpidr[7:0] == 0 and define a different mapping. There would probably
be some other issues like GIC cpu interfaces before that would really
work though.
Rob
