Patchwork target-arm: Tidy up ARM1136 CPUID naming

login
register
mail settings
Submitter Andreas Färber
Date Oct. 3, 2011, 10:32 a.m.
Message ID <1317637971-19296-1-git-send-email-andreas.faerber@web.de>
Download mbox | patch
Permalink /patch/117432/
State New
Headers show

Comments

Andreas Färber - Oct. 3, 2011, 10:32 a.m.
-cpu arm1136-r2 is commented to in fact be ARM1136 r0p2, whereas
-cpu arm1136 seems to be ARM1136 r1p3 according to the MIDR value.

The CPUID values contain major and minor revision numbers (rnpn) and
are never used with a mask, so are specific to the chosen revision.
Rename the CPUID preprocessor defines to reflect this, but leave the
CPU model names unchanged for command line compatibility.

Cc: Andrzej Zaborowski <andrew@openedhand.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 target-arm/cpu.h    |    4 ++--
 target-arm/helper.c |   12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)
Andreas Färber - Oct. 22, 2011, 9:22 a.m.
Am 03.10.2011 12:32, schrieb Andreas Färber:
> -cpu arm1136-r2 is commented to in fact be ARM1136 r0p2, whereas
> -cpu arm1136 seems to be ARM1136 r1p3 according to the MIDR value.
> 
> The CPUID values contain major and minor revision numbers (rnpn) and
> are never used with a mask, so are specific to the chosen revision.
> Rename the CPUID preprocessor defines to reflect this, but leave the
> CPU model names unchanged for command line compatibility.
> 
> Cc: Andrzej Zaborowski <andrew@openedhand.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> ---

Ping?

Andreas

>  target-arm/cpu.h    |    4 ++--
>  target-arm/helper.c |   12 ++++++------
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 6ab780d..783989f 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -419,8 +419,8 @@ void cpu_arm_set_cp_io(CPUARMState *env, int cpnum,
>  #define ARM_CPUID_PXA270_B1   0x69054113
>  #define ARM_CPUID_PXA270_C0   0x69054114
>  #define ARM_CPUID_PXA270_C5   0x69054117
> -#define ARM_CPUID_ARM1136     0x4117b363
> -#define ARM_CPUID_ARM1136_R2  0x4107b362
> +#define ARM_CPUID_ARM1136_R1P3 0x4117b363
> +#define ARM_CPUID_ARM1136_R0P2 0x4107b362
>  #define ARM_CPUID_ARM1176     0x410fb767
>  #define ARM_CPUID_ARM11MPCORE 0x410fb022
>  #define ARM_CPUID_CORTEXA8    0x410fc080
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index e2428eb..0d342ba 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -76,11 +76,11 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
>          env->cp15.c0_cachetype = 0x1dd20d2;
>          env->cp15.c1_sys = 0x00090078;
>          break;
> -    case ARM_CPUID_ARM1136:
> +    case ARM_CPUID_ARM1136_R1P3:
>          /* This is the 1136 r1, which is a v6K core */
>          set_feature(env, ARM_FEATURE_V6K);
>          /* Fall through */
> -    case ARM_CPUID_ARM1136_R2:
> +    case ARM_CPUID_ARM1136_R0P2:
>          /* What qemu calls "arm1136_r2" is actually the 1136 r0p2, ie an
>           * older core than plain "arm1136". In particular this does not
>           * have the v6K features.
> @@ -417,8 +417,8 @@ static const struct arm_cpu_t arm_cpu_names[] = {
>      { ARM_CPUID_ARM926, "arm926"},
>      { ARM_CPUID_ARM946, "arm946"},
>      { ARM_CPUID_ARM1026, "arm1026"},
> -    { ARM_CPUID_ARM1136, "arm1136"},
> -    { ARM_CPUID_ARM1136_R2, "arm1136-r2"},
> +    { ARM_CPUID_ARM1136_R1P3, "arm1136" },
> +    { ARM_CPUID_ARM1136_R0P2, "arm1136-r2" },
>      { ARM_CPUID_ARM1176, "arm1176"},
>      { ARM_CPUID_ARM11MPCORE, "arm11mpcore"},
>      { ARM_CPUID_CORTEXM3, "cortex-m3"},
> @@ -1886,8 +1886,8 @@ uint32_t HELPER(get_cp15)(CPUState *env, uint32_t insn)
>              switch (ARM_CPUID(env)) {
>              case ARM_CPUID_ARM1026:
>                  return 1;
> -            case ARM_CPUID_ARM1136:
> -            case ARM_CPUID_ARM1136_R2:
> +            case ARM_CPUID_ARM1136_R1P3:
> +            case ARM_CPUID_ARM1136_R0P2:
>              case ARM_CPUID_ARM1176:
>                  return 7;
>              case ARM_CPUID_ARM11MPCORE:
Peter Maydell - Oct. 22, 2011, 10:20 a.m.
On 3 October 2011 11:32, Andreas Färber <andreas.faerber@web.de> wrote:
> -#define ARM_CPUID_ARM1136     0x4117b363
> -#define ARM_CPUID_ARM1136_R2  0x4107b362
> +#define ARM_CPUID_ARM1136_R1P3 0x4117b363
> +#define ARM_CPUID_ARM1136_R0P2 0x4107b362

I don't think the patchlevels are important enough to
memorialise in the constant names. The important
distinction in behaviour is between the r0 and r1, so
I think that ARM1136_R0 vs _R1 would be better.

-- PMM
Andreas Färber - Oct. 22, 2011, 10:33 a.m.
Am 22.10.2011 12:20, schrieb Peter Maydell:
> On 3 October 2011 11:32, Andreas Färber <andreas.faerber@web.de> wrote:
>> -#define ARM_CPUID_ARM1136     0x4117b363
>> -#define ARM_CPUID_ARM1136_R2  0x4107b362
>> +#define ARM_CPUID_ARM1136_R1P3 0x4117b363
>> +#define ARM_CPUID_ARM1136_R0P2 0x4107b362
> 
> I don't think the patchlevels are important enough to
> memorialise in the constant names. The important
> distinction in behaviour is between the r0 and r1, so
> I think that ARM1136_R0 vs _R1 would be better.

Would you be okay if we do the following?

#define ARM_CPUID_ARM1136_R0 ARM_CPUID_ARM1136_R0P2
#define ARM_CPUID_ARM1136_R1 ARM_CPUID_ARM1136_R1P3

My point is that the number is actually hardcoded in there, whatever we
name the constant. ARM1136 or ARM1136_R0 gives the impression of a more
generic value.

Masking would be the only way to have generic code there and I still
haven't figured out how to do that sensibly without breaking up the
whole switch. For now I have a patch cooking for CPUID preservation that
I'll submit shortly.

Andreas
Peter Maydell - Oct. 24, 2011, 11:15 a.m.
On 22 October 2011 11:33, Andreas Färber <andreas.faerber@web.de> wrote:
> Am 22.10.2011 12:20, schrieb Peter Maydell:
>> On 3 October 2011 11:32, Andreas Färber <andreas.faerber@web.de> wrote:
>>> -#define ARM_CPUID_ARM1136     0x4117b363
>>> -#define ARM_CPUID_ARM1136_R2  0x4107b362
>>> +#define ARM_CPUID_ARM1136_R1P3 0x4117b363
>>> +#define ARM_CPUID_ARM1136_R0P2 0x4107b362
>>
>> I don't think the patchlevels are important enough to
>> memorialise in the constant names. The important
>> distinction in behaviour is between the r0 and r1, so
>> I think that ARM1136_R0 vs _R1 would be better.
>
> Would you be okay if we do the following?
>
> #define ARM_CPUID_ARM1136_R0 ARM_CPUID_ARM1136_R0P2
> #define ARM_CPUID_ARM1136_R1 ARM_CPUID_ARM1136_R1P3

Not sure that makes it any better.

> My point is that the number is actually hardcoded in there, whatever we
> name the constant.

I think that's really the problem...

-- PMM

Patch

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 6ab780d..783989f 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -419,8 +419,8 @@  void cpu_arm_set_cp_io(CPUARMState *env, int cpnum,
 #define ARM_CPUID_PXA270_B1   0x69054113
 #define ARM_CPUID_PXA270_C0   0x69054114
 #define ARM_CPUID_PXA270_C5   0x69054117
-#define ARM_CPUID_ARM1136     0x4117b363
-#define ARM_CPUID_ARM1136_R2  0x4107b362
+#define ARM_CPUID_ARM1136_R1P3 0x4117b363
+#define ARM_CPUID_ARM1136_R0P2 0x4107b362
 #define ARM_CPUID_ARM1176     0x410fb767
 #define ARM_CPUID_ARM11MPCORE 0x410fb022
 #define ARM_CPUID_CORTEXA8    0x410fc080
diff --git a/target-arm/helper.c b/target-arm/helper.c
index e2428eb..0d342ba 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -76,11 +76,11 @@  static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
         env->cp15.c0_cachetype = 0x1dd20d2;
         env->cp15.c1_sys = 0x00090078;
         break;
-    case ARM_CPUID_ARM1136:
+    case ARM_CPUID_ARM1136_R1P3:
         /* This is the 1136 r1, which is a v6K core */
         set_feature(env, ARM_FEATURE_V6K);
         /* Fall through */
-    case ARM_CPUID_ARM1136_R2:
+    case ARM_CPUID_ARM1136_R0P2:
         /* What qemu calls "arm1136_r2" is actually the 1136 r0p2, ie an
          * older core than plain "arm1136". In particular this does not
          * have the v6K features.
@@ -417,8 +417,8 @@  static const struct arm_cpu_t arm_cpu_names[] = {
     { ARM_CPUID_ARM926, "arm926"},
     { ARM_CPUID_ARM946, "arm946"},
     { ARM_CPUID_ARM1026, "arm1026"},
-    { ARM_CPUID_ARM1136, "arm1136"},
-    { ARM_CPUID_ARM1136_R2, "arm1136-r2"},
+    { ARM_CPUID_ARM1136_R1P3, "arm1136" },
+    { ARM_CPUID_ARM1136_R0P2, "arm1136-r2" },
     { ARM_CPUID_ARM1176, "arm1176"},
     { ARM_CPUID_ARM11MPCORE, "arm11mpcore"},
     { ARM_CPUID_CORTEXM3, "cortex-m3"},
@@ -1886,8 +1886,8 @@  uint32_t HELPER(get_cp15)(CPUState *env, uint32_t insn)
             switch (ARM_CPUID(env)) {
             case ARM_CPUID_ARM1026:
                 return 1;
-            case ARM_CPUID_ARM1136:
-            case ARM_CPUID_ARM1136_R2:
+            case ARM_CPUID_ARM1136_R1P3:
+            case ARM_CPUID_ARM1136_R0P2:
             case ARM_CPUID_ARM1176:
                 return 7;
             case ARM_CPUID_ARM11MPCORE: