Re: [Qemu-arm] [Qemu-devel] [PATCH 8/9] hw/arm/bcm2836: Hardcode correct CPU type

[Andrew Baumann]

> From: Qemu-devel <qemu-devel-
> address@hidden> On Behalf Of Peter
> Maydell
> Sent: Tuesday, 13 March 2018 08:35
> 
> Now we have separate types for BCM2386 and BCM2387, we might as well
> just hard-code the CPU type they use rather than having it passed
> through as an object property. This then lets us put the initialization
> of the CPU object in init rather than realize.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  hw/arm/bcm2836.c | 22 +++++++++++++---------
>  hw/arm/raspi.c   |  2 --
>  2 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
> index 7140257c98..12f75b55a7 100644
> --- a/hw/arm/bcm2836.c
> +++ b/hw/arm/bcm2836.c
> @@ -25,16 +25,19 @@
> 
>  struct BCM283XInfo {
>      const char *name;
> +    const char *cpu_type;
>      int clusterid;
>  };
> 
>  static const BCM283XInfo bcm283x_socs[] = {
>      {
>          .name = TYPE_BCM2836,
> +        .cpu_type = ARM_CPU_TYPE_NAME("cortex-a15"),

At some point I remember seeing a patch to change this to cortex-a7. Is there a 
reason we didn't make that change?

(Background: the real Pi2 has an A7. When I first implemented the machine model 
there was no A7 emulation in QEMU, so I used the A15 which was the closest 
equivalent.)

>          .clusterid = 0xf,
>      },
>      {
>          .name = TYPE_BCM2837,
> +        .cpu_type = ARM_CPU_TYPE_NAME("cortex-a53"),
>          .clusterid = 0x0,
>      },
>  };
> @@ -42,6 +45,16 @@ static const BCM283XInfo bcm283x_socs[] = {
>  static void bcm2836_init(Object *obj)
>  {
>      BCM283XState *s = BCM283X(obj);
> +    BCM283XClass *bc = BCM283X_GET_CLASS(obj);
> +    const BCM283XInfo *info = bc->info;
> +    int n;
> +
> +    for (n = 0; n < BCM283X_NCPUS; n++) {
> +        object_initialize(&s->cpus[n], sizeof(s->cpus[n]),
> +                          info->cpu_type);
> +        object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]),
> +                                  &error_abort);
> +    }
> 
>      object_initialize(&s->control, sizeof(s->control), TYPE_BCM2836_CONTROL);
>      object_property_add_child(obj, "control", OBJECT(&s->control), NULL);
> @@ -69,14 +82,6 @@ static void bcm2836_realize(DeviceState *dev, Error
> **errp)
> 
>      /* common peripherals from bcm2835 */
> 
> -    obj = OBJECT(dev);
> -    for (n = 0; n < BCM283X_NCPUS; n++) {
> -        object_initialize(&s->cpus[n], sizeof(s->cpus[n]),
> -                          s->cpu_type);
> -        object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]),
> -                                  &error_abort);
> -    }
> -
>      obj = object_property_get_link(OBJECT(dev), "ram", &err);
>      if (obj == NULL) {
>          error_setg(errp, "%s: required ram link not found: %s",
> @@ -168,7 +173,6 @@ static void bcm2836_realize(DeviceState *dev, Error
> **errp)
>  }
> 
>  static Property bcm2836_props[] = {
> -    DEFINE_PROP_STRING("cpu-type", BCM283XState, cpu_type),
>      DEFINE_PROP_UINT32("enabled-cpus", BCM283XState, enabled_cpus,
>                         BCM283X_NCPUS),
>      DEFINE_PROP_END_OF_LIST()
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index f588720138..ae15997669 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -150,8 +150,6 @@ static void raspi_init(MachineState *machine, int
> version)
>      /* Setup the SOC */
>      object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram),
>                                     &error_abort);
> -    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> -                            &error_abort);
>      object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus",
>                              &error_abort);
>      int board_rev = version == 3 ? 0xa02082 : 0xa21041;

What about the default_cpu_type field of MachineClass set in 
raspi[23]_machine_init? That seems irrelevant now... Also, if anyone cares (I 
don't), we also just lost the ability to override the CPU type of a raspi 
model. 

Otherwise,
Reviewed-by: Andrew Baumann <address@hidden>

Cheers,
Andrew