Re: [Qemu-devel] [PATCH 1/2] target-arm: make MPIDR a property

[Rob Herring]

On Mon, Feb 24, 2014 at 4:28 PM, Peter Maydell <address@hidden> wrote:
> On 24 February 2014 22:14, Rob Herring <address@hidden> wrote:
>> From: Rob Herring <address@hidden>
>> 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