Re: [Qemu-devel] [PATCH V3] Target-arm: Add the Cortex-M4 CPU

[Peter Maydell]

On 15 June 2015 at 21:31, Aurelio C. Remonda <address@hidden> wrote:
> This patch adds the Cortex-M4 CPU. The M4 is basically the same as the M3,
> the main differences being the DSP instructions and an optional FPU.

OK, I've checked through the v7M ARM ARM, and I'm happy that
we don't need any further changes to the core code to support
a no-FPU Cortex-M4. so this patch is functionally good, and just
needs a few tweaks.

> Created an ARM_FEATURE_THUMB_DSP to be added to any non-M thumb2-compatible
> CPU that uses DSP instructions, and manually added it to the M4 in its initfn.

This commit message could use updating:
 * the THUMB_DSP feature was created in a different patch
 * it's worth mentioning that we're implementing the no-FPU M4
   here, and why (because the core target-arm code doesn't support
   the M-profile FPU model yet)

> Signed-off-by: Aurelio C. Remonda <address@hidden>
> ---
> * Changes in V3: Add the ARM_FEATURE_THUMB_DSP in a separate patch and create
> just the CPU init on this one.
>  target-arm/cpu.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 4a888ab..4950897 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -783,6 +783,21 @@ static void cortex_m3_initfn(Object *obj)
>      cpu->midr = 0x410fc231;
>  }
>
> +static void cortex_m4_initfn(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);

You want a blank line here. Compare the other CPU initfns.

> +    set_feature(&cpu->env, ARM_FEATURE_V7);
> +    set_feature(&cpu->env, ARM_FEATURE_M);
> +    set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
> +    cpu->midr = 0x410fc240;
> +    /*main id register CPUID bit assignments
> +    Bits    NAME        Function
> +    [31:24] IMPLEMENTER Indicates implementor: 0x41 = ARM
> +    [23:20] VARIANT     Indicates processor revision: 0x0 = Revision 0
> +    [19:16] (Constant)  Reads as 0xF
> +    [15:4]  PARTNO      Indicates part number: 0xC24 = Cortex-M4
> +    [3:0]   REVISION    Indicates patch release: 0x0 = Patch 0.*/

This whole comment is unnecessary -- the MIDR is the standard
format for all ARM CPUs, and no other initfn spells out the
various fields of it. You can say
    cpu->midr = 0x410fc240; /* r0p0 */
if you like.

> +}
>  static void arm_v7m_class_init(ObjectClass *oc, void *data)
>  {
>      CPUClass *cc = CPU_CLASS(oc);
> @@ -1185,6 +1200,8 @@ static const ARMCPUInfo arm_cpus[] = {
>      { .name = "arm11mpcore", .initfn = arm11mpcore_initfn },
>      { .name = "cortex-m3",   .initfn = cortex_m3_initfn,
>                               .class_init = arm_v7m_class_init },
> +    { .name = "cortex-m4",   .initfn = cortex_m4_initfn,
> +                             .class_init = arm_v7m_class_init },
>      { .name = "cortex-a8",   .initfn = cortex_a8_initfn },
>      { .name = "cortex-a9",   .initfn = cortex_a9_initfn },
>      { .name = "cortex-a15",  .initfn = cortex_a15_initfn },

If you can make those tweaks and resend I think we can
get this into QEMU for 2.4.

thanks
-- PMM