[1/2] target-arm: make MPIDR a property

From: Rob Herring <rob.herring@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.

ARM_FEATURE_MPIDR is not used here because it is set too late.

Signed-off-by: Rob Herring <rob.herring@linaro.org>
---
 target-arm/cpu-qom.h |  1 +
 target-arm/cpu.c     |  8 ++++++++
 target-arm/helper.c  | 18 +++++++++++-------
 3 files changed, 20 insertions(+), 7 deletions(-)

On 24 February 2014 22:14, Rob Herring <robherring2@gmail.com> wrote:
> From: Rob Herring <rob.herring@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.
>
> ARM_FEATURE_MPIDR is not used here because it is set too late.
>
> Signed-off-by: Rob Herring <rob.herring@linaro.org>
> ---
>  target-arm/cpu-qom.h |  1 +
>  target-arm/cpu.c     |  8 ++++++++
>  target-arm/helper.c  | 18 +++++++++++-------
>  3 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> index afbd422..27a44a0 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -113,6 +113,7 @@ typedef struct ARMCPU {
>       * prefix means a constant register.
>       */
>      uint32_t midr;
> +    uint32_t mpidr;
>      uint32_t reset_fpsid;
>      uint32_t mvfr0;
>      uint32_t mvfr1;
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 45ad7f0..5ac150e 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -249,6 +249,9 @@ static Property arm_cpu_reset_cbar_property =
>  static Property arm_cpu_reset_hivecs_property =
>              DEFINE_PROP_BOOL("reset-hivecs", ARMCPU, reset_hivecs, false);
>
> +static Property arm_cpu_mpidr_property =
> +            DEFINE_PROP_UINT32("mpidr", ARMCPU, mpidr, 0);
> +
>  static void arm_cpu_post_init(Object *obj)
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
> @@ -262,6 +265,11 @@ static void arm_cpu_post_init(Object *obj)
>          qdev_property_add_static(DEVICE(obj), &arm_cpu_reset_hivecs_property,
>                                   &error_abort);
>      }
> +
> +    if (arm_feature(&cpu->env, ARM_FEATURE_V7MP)) {
> +        qdev_property_add_static(DEVICE(obj), &arm_cpu_mpidr_property,
> +                                 &error_abort);
> +    }

This fails to work for 11mpcore, which doesn't have FEATURE_V7MP
but does have FEATURE_MPIDR.

> index ca5b000..2590a05 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -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.)

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.

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.

>      /* We don't support setting cluster ID ([8..11])
>       * so these bits always RAZ.
>       */
> @@ -1457,12 +1462,6 @@ static int mpidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
>      return 0;
>  }
>
> -static const ARMCPRegInfo mpidr_cp_reginfo[] = {
> -    { .name = "MPIDR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 5,
> -      .access = PL1_R, .readfn = mpidr_read, .type = ARM_CP_NO_MIGRATE },
> -    REGINFO_SENTINEL
> -};
> -
>  static int par64_read(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t *value)
>  {
>      *value = ((uint64_t)env->cp15.c7_par_hi << 32) | env->cp15.c7_par;
> @@ -1836,7 +1835,12 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>      }
>
>      if (arm_feature(env, ARM_FEATURE_MPIDR)) {
> -        define_arm_cp_regs(cpu, mpidr_cp_reginfo);
> +        ARMCPRegInfo mpidr_cp_reginfo = {
> +            .name = "MPIDR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 5,
> +            .access = PL1_R, .readfn = mpidr_read, .type = ARM_CP_NO_MIGRATE,
> +            .resetvalue = cpu->mpidr
> +        };
> +        define_one_arm_cp_reg(cpu, &mpidr_cp_reginfo);
>      }
>
>      if (arm_feature(env, ARM_FEATURE_AUXCR)) {

thanks
-- PMM

On 24 February 2014 23:13, Rob Herring <robherring2@gmail.com> wrote:
> On Mon, Feb 24, 2014 at 4:28 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 24 February 2014 22:14, Rob Herring <robherring2@gmail.com> wrote:
>>> From: Rob Herring <rob.herring@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.

Looking at your patch 2 I think makes it really clear we don't want
the platform to have to set the cpu index. It should just set the cluster ID.
(Eventually the CPU creation will disappear into the a15mpcore_priv
device, and then cluster ID will be a property on that device.)

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

Yeah, but the h/w doesn't give the ability to tie off anything except
the cluster affinity fields -- you can't make the individual cores
report anything except 0..3 in the lowest part of the MPIDR,
and you can't prevent bit 31 being set.

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

If we ever need that I think we should implement it straightforwardly
(ie by supporting holding an arbitrary set of  CPUs in reset or powerdown
and booting on the first powered up core) and not by doing weird
rearrangements of the MPIDR to fake something we're not emulating
properly.

thanks
-- PMM