Re: [PATCH v2 1/2] target/arm: Implement Cortex-A5

[Peter Maydell]

On Thu, 16 Dec 2021 at 06:48, Byron Lathi <bslathi19@gmail.com> wrote:
>
> Add support for the Cortex-A5. These changes are based off of the A7 and
> A9 Init functions, using the appropriate values from the technical
> reference manual for the A5.
>
> Signed-off-by: Byron Lathi <bslathi19@gmail.com>
> ---
>  target/arm/cpu_tcg.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
> index 13d0e9b195..2b54fb618b 100644
> --- a/target/arm/cpu_tcg.c
> +++ b/target/arm/cpu_tcg.c
> @@ -304,6 +304,41 @@ static void cortex_a8_initfn(Object *obj)
>      define_arm_cp_regs(cpu, cortexa8_cp_reginfo);
>  }
>
> +static void cortex_a5_initfn(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    cpu->dtb_compatible = "arm,cortex-a5";
> +    set_feature(&cpu->env, ARM_FEATURE_V7);
> +    set_feature(&cpu->env, ARM_FEATURE_NEON);
> +    set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
> +    set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);

We should avoid using the DUMMY_C15_REGS feature where we
can for new cores. It's a legacy thing where we just RAZ/WI
the whole cp15 crn=15 space. For new CPU definitions we
should define the actual coprocessor registers they have in
this space, typically just as NOP/RAZ registers. You can see
how that's done for the Cortex-A9 with the
cortexa9_cp_reginfo[] array. The A5's registers are slightly
different but they are documented in the TRM:
https://developer.arm.com/documentation/ddi0433/c/system-control/register-summary/c15-summary-table?lang=en

Of those, the CBAR we handle with a feature bit (see below);
the others you can make ARM_CP_NOP for the WO ones and
ARM_CP_CONST with value 0 for the RO and RW ones. Make
sure you get the .access field right: these are all
available only frome secure mode, so should use
.access = PL3_RW / PL3_R / PL3_W for RW/RO/WO.

> +    set_feature(&cpu->env, ARM_FEATURE_PMU);

Missing ARM_FEATURE_EL3.
Missing ARM_FEATURE_CBAR_RO.
Missing ARM_FEATURE_V7MP.

> +    cpu->midr = 0x410fc0f1;

As you note, this is the A9 MIDR. A5 is 0x410fc051.

> +    cpu->reset_fpsid = 0x41023051;
> +    cpu->isar.mvfr0 = 0x10110221;

0x10110222

> +    cpu->isar.mvfr1 = 0x11000011;

0x11111111

These numbers are in the Cortex-A5 NEON Media Processing
Engine TRM:
https://developer.arm.com/documentation/ddi0450/b/Programmers-Model/Register-summary

and indicate the A5's VFPv4 support, among other things.
(QEMU drives its insn enablement off these ID register
values, so if we get them wrong then some guest code that
works on a real A5 will UNDEF on QEMU.)

> +    cpu->ctr = 0x83338003;
> +    cpu->reset_sctlr = 0x00c50078;
> +    cpu->isar.id_pfr0 = 0x00001231;
> +    cpu->isar.id_pfr1 = 0x00000011;
> +    cpu->isar.id_dfr0 = 0x02010444;
> +    cpu->id_afr0 = 0x00000000;
> +    cpu->isar.id_mmfr0 = 0x00100103;

0x00100003

> +    cpu->isar.id_mmfr1 = 0x40000000;
> +    cpu->isar.id_mmfr2 = 0x01230000;
> +    cpu->isar.id_mmfr3 = 0x00102211;
> +    cpu->isar.id_isar0 = 0x00101111;
> +    cpu->isar.id_isar1 = 0x13112111;
> +    cpu->isar.id_isar2 = 0x21232041;
> +    cpu->isar.id_isar3 = 0x11112131;
> +    cpu->isar.id_isar4 = 0x00011142;
> +    cpu->isar.dbgdidr = 0x1203f001;
> +    cpu->clidr = 0x09200003;
> +    cpu->ccsidr[0] = 0x701fe00a;
> +    cpu->ccsidr[1] = 0x203fe00a;

These don't match what the A5 TRM says are valid values for the
CCSIDR fields: specifically the associativity and linesize
fields don't match what the TRM says are the mandated values.
Where did you get your values from?

Following the TRM fields and using 32KB for each would be:

  cpu->ccsidr[0] = 0x701fe019; /* dcache: 32KB 4-way, 8 words/line */
  cpu->ccsidr[1] = 0x203fe009; /* icache: 32KB 2-way, 8 words/line */

> +}
> +
>  static const ARMCPRegInfo cortexa9_cp_reginfo[] = {
>      /*
>       * power_control should be set to maximum latency. Again,
> @@ -1019,6 +1054,7 @@ static const ARMCPUInfo arm_tcg_cpus[] = {
>      { .name = "arm1136",     .initfn = arm1136_initfn },
>      { .name = "arm1176",     .initfn = arm1176_initfn },
>      { .name = "arm11mpcore", .initfn = arm11mpcore_initfn },
> +    { .name = "cortex-a5",  .initfn = cortex_a5_initfn },
>      { .name = "cortex-a7",   .initfn = cortex_a7_initfn },
>      { .name = "cortex-a8",   .initfn = cortex_a8_initfn },
>      { .name = "cortex-a9",   .initfn = cortex_a9_initfn },
> --
> 2.30.2
>

thanks
-- PMM