[PATCH v2 07/13] hw/arm/bcm2836: QOM'ify more by adding class_init() to each SoC type

Philippe Mathieu-Daudé posted 13 patches 5 years, 9 months ago
There is a newer version of this series
[PATCH v2 07/13] hw/arm/bcm2836: QOM'ify more by adding class_init() to each SoC type
Posted by Philippe Mathieu-Daudé 5 years, 9 months ago
Remove usage of TypeInfo::class_data. Instead fill the fields in
the corresponding class_init().

Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/bcm2836.c | 109 ++++++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 58 deletions(-)

diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index 24109fef1d..683d04d6ea 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -16,57 +16,30 @@
 #include "hw/arm/raspi_platform.h"
 #include "hw/sysbus.h"
 
-typedef struct BCM283XInfo BCM283XInfo;
-
 typedef struct BCM283XClass {
     /*< private >*/
     DeviceClass parent_class;
     /*< public >*/
-    const BCM283XInfo *info;
-} BCM283XClass;
-
-struct BCM283XInfo {
-    const char *name;
     const char *cpu_type;
     hwaddr peri_base; /* Peripheral base address seen by the CPU */
     hwaddr ctrl_base; /* Interrupt controller and mailboxes etc. */
     int clusterid;
-};
+} BCM283XClass;
 
 #define BCM283X_CLASS(klass) \
     OBJECT_CLASS_CHECK(BCM283XClass, (klass), TYPE_BCM283X)
 #define BCM283X_GET_CLASS(obj) \
     OBJECT_GET_CLASS(BCM283XClass, (obj), TYPE_BCM283X)
 
-static const BCM283XInfo bcm283x_socs[] = {
-    {
-        .name = TYPE_BCM2836,
-        .cpu_type = ARM_CPU_TYPE_NAME("cortex-a7"),
-        .peri_base = 0x3f000000,
-        .ctrl_base = 0x40000000,
-        .clusterid = 0xf,
-    },
-#ifdef TARGET_AARCH64
-    {
-        .name = TYPE_BCM2837,
-        .cpu_type = ARM_CPU_TYPE_NAME("cortex-a53"),
-        .peri_base = 0x3f000000,
-        .ctrl_base = 0x40000000,
-        .clusterid = 0x0,
-    },
-#endif
-};
-
 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_child(obj, "cpu[*]", &s->cpu[n].core,
-                                sizeof(s->cpu[n].core), info->cpu_type,
+                                sizeof(s->cpu[n].core), bc->cpu_type,
                                 &error_abort, NULL);
     }
 
@@ -85,7 +58,6 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
 {
     BCM283XState *s = BCM283X(dev);
     BCM283XClass *bc = BCM283X_GET_CLASS(dev);
-    const BCM283XInfo *info = bc->info;
     Object *obj;
     Error *err = NULL;
     int n;
@@ -119,7 +91,7 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
     }
 
     sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->peripherals), 0,
-                            info->peri_base, 1);
+                            bc->peri_base, 1);
 
     /* bcm2836 interrupt controller (and mailboxes, etc.) */
     object_property_set_bool(OBJECT(&s->control), true, "realized", &err);
@@ -128,7 +100,7 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    sysbus_mmio_map(SYS_BUS_DEVICE(&s->control), 0, info->ctrl_base);
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->control), 0, bc->ctrl_base);
 
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->peripherals), 0,
         qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-irq", 0));
@@ -137,11 +109,11 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
 
     for (n = 0; n < BCM283X_NCPUS; n++) {
         /* TODO: this should be converted to a property of ARM_CPU */
-        s->cpu[n].core.mp_affinity = (info->clusterid << 8) | n;
+        s->cpu[n].core.mp_affinity = (bc->clusterid << 8) | n;
 
         /* set periphbase/CBAR value for CPU-local registers */
         object_property_set_int(OBJECT(&s->cpu[n].core),
-                                info->peri_base,
+                                bc->peri_base,
                                 "reset-cbar", &err);
         if (err) {
             error_propagate(errp, err);
@@ -190,38 +162,59 @@ static Property bcm2836_props[] = {
 static void bcm283x_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
-    BCM283XClass *bc = BCM283X_CLASS(oc);
 
-    bc->info = data;
-    dc->realize = bcm2836_realize;
-    device_class_set_props(dc, bcm2836_props);
     /* Reason: Must be wired up in code (see raspi_init() function) */
     dc->user_creatable = false;
 }
 
-static const TypeInfo bcm283x_type_info = {
-    .name = TYPE_BCM283X,
-    .parent = TYPE_DEVICE,
-    .instance_size = sizeof(BCM283XState),
-    .instance_init = bcm2836_init,
-    .class_size = sizeof(BCM283XClass),
-    .abstract = true,
+static void bcm2836_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    BCM283XClass *bc = BCM283X_CLASS(oc);
+
+    bc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a7");
+    bc->peri_base = 0x3f000000;
+    bc->ctrl_base = 0x40000000;
+    bc->clusterid = 0xf;
+    dc->realize = bcm2836_realize;
+    device_class_set_props(dc, bcm2836_props);
 };
 
-static void bcm2836_register_types(void)
+#ifdef TARGET_AARCH64
+static void bcm2837_class_init(ObjectClass *oc, void *data)
 {
-    int i;
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    BCM283XClass *bc = BCM283X_CLASS(oc);
 
-    type_register_static(&bcm283x_type_info);
-    for (i = 0; i < ARRAY_SIZE(bcm283x_socs); i++) {
-        TypeInfo ti = {
-            .name = bcm283x_socs[i].name,
-            .parent = TYPE_BCM283X,
-            .class_init = bcm283x_class_init,
-            .class_data = (void *) &bcm283x_socs[i],
-        };
-        type_register(&ti);
+    bc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
+    bc->peri_base = 0x3f000000;
+    bc->ctrl_base = 0x40000000;
+    bc->clusterid = 0x0;
+    dc->realize = bcm2836_realize;
+    device_class_set_props(dc, bcm2836_props);
+};
+#endif
+
+static const TypeInfo bcm283x_types[] = {
+    {
+        .name           = TYPE_BCM2836,
+        .parent         = TYPE_BCM283X,
+        .class_init     = bcm2836_class_init,
+#ifdef TARGET_AARCH64
+    }, {
+        .name           = TYPE_BCM2837,
+        .parent         = TYPE_BCM283X,
+        .class_init     = bcm2837_class_init,
+#endif
+    }, {
+        .name           = TYPE_BCM283X,
+        .parent         = TYPE_DEVICE,
+        .instance_size  = sizeof(BCM283XState),
+        .instance_init  = bcm2836_init,
+        .class_size     = sizeof(BCM283XClass),
+        .class_init     = bcm283x_class_init,
+        .abstract       = true,
     }
-}
+};
 
-type_init(bcm2836_register_types)
+DEFINE_TYPES(bcm283x_types)
-- 
2.21.1


Re: [PATCH v2 07/13] hw/arm/bcm2836: QOM'ify more by adding class_init() to each SoC type
Posted by Igor Mammedov 5 years, 8 months ago
On Mon, 17 Feb 2020 12:45:27 +0100
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> Remove usage of TypeInfo::class_data. Instead fill the fields in
> the corresponding class_init().
> 
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/bcm2836.c | 109 ++++++++++++++++++++++-------------------------
>  1 file changed, 51 insertions(+), 58 deletions(-)
> 
> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
> index 24109fef1d..683d04d6ea 100644
> --- a/hw/arm/bcm2836.c
> +++ b/hw/arm/bcm2836.c
> @@ -16,57 +16,30 @@
>  #include "hw/arm/raspi_platform.h"
>  #include "hw/sysbus.h"
>  
> -typedef struct BCM283XInfo BCM283XInfo;
> -
>  typedef struct BCM283XClass {
>      /*< private >*/
>      DeviceClass parent_class;
>      /*< public >*/
> -    const BCM283XInfo *info;
> -} BCM283XClass;
> -
> -struct BCM283XInfo {
> -    const char *name;

>      const char *cpu_type;

probably could be cleaned up by using machine->cpu_type/machine_class->default_cpu_type
(no need to change this patch as it's separate issue)


>      hwaddr peri_base; /* Peripheral base address seen by the CPU */
>      hwaddr ctrl_base; /* Interrupt controller and mailboxes etc. */
>      int clusterid;
> -};
> +} BCM283XClass;
>  
>  #define BCM283X_CLASS(klass) \
>      OBJECT_CLASS_CHECK(BCM283XClass, (klass), TYPE_BCM283X)
>  #define BCM283X_GET_CLASS(obj) \
>      OBJECT_GET_CLASS(BCM283XClass, (obj), TYPE_BCM283X)
>  
> -static const BCM283XInfo bcm283x_socs[] = {
> -    {
> -        .name = TYPE_BCM2836,
> -        .cpu_type = ARM_CPU_TYPE_NAME("cortex-a7"),
> -        .peri_base = 0x3f000000,
> -        .ctrl_base = 0x40000000,
> -        .clusterid = 0xf,
> -    },
> -#ifdef TARGET_AARCH64
> -    {
> -        .name = TYPE_BCM2837,
> -        .cpu_type = ARM_CPU_TYPE_NAME("cortex-a53"),
> -        .peri_base = 0x3f000000,
> -        .ctrl_base = 0x40000000,
> -        .clusterid = 0x0,
> -    },
> -#endif
> -};
> -
>  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_child(obj, "cpu[*]", &s->cpu[n].core,
> -                                sizeof(s->cpu[n].core), info->cpu_type,
> +                                sizeof(s->cpu[n].core), bc->cpu_type,
>                                  &error_abort, NULL);
>      }
>  
> @@ -85,7 +58,6 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
>  {
>      BCM283XState *s = BCM283X(dev);
>      BCM283XClass *bc = BCM283X_GET_CLASS(dev);
> -    const BCM283XInfo *info = bc->info;
>      Object *obj;
>      Error *err = NULL;
>      int n;
> @@ -119,7 +91,7 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
>      }
>  
>      sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->peripherals), 0,
> -                            info->peri_base, 1);
> +                            bc->peri_base, 1);
>  
>      /* bcm2836 interrupt controller (and mailboxes, etc.) */
>      object_property_set_bool(OBJECT(&s->control), true, "realized", &err);
> @@ -128,7 +100,7 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    sysbus_mmio_map(SYS_BUS_DEVICE(&s->control), 0, info->ctrl_base);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->control), 0, bc->ctrl_base);
>  
>      sysbus_connect_irq(SYS_BUS_DEVICE(&s->peripherals), 0,
>          qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-irq", 0));
> @@ -137,11 +109,11 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
>  
>      for (n = 0; n < BCM283X_NCPUS; n++) {
>          /* TODO: this should be converted to a property of ARM_CPU */
> -        s->cpu[n].core.mp_affinity = (info->clusterid << 8) | n;
> +        s->cpu[n].core.mp_affinity = (bc->clusterid << 8) | n;
>  
>          /* set periphbase/CBAR value for CPU-local registers */
>          object_property_set_int(OBJECT(&s->cpu[n].core),
> -                                info->peri_base,
> +                                bc->peri_base,
>                                  "reset-cbar", &err);
>          if (err) {
>              error_propagate(errp, err);
> @@ -190,38 +162,59 @@ static Property bcm2836_props[] = {
>  static void bcm283x_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
> -    BCM283XClass *bc = BCM283X_CLASS(oc);
>  
> -    bc->info = data;
> -    dc->realize = bcm2836_realize;
> -    device_class_set_props(dc, bcm2836_props);
>      /* Reason: Must be wired up in code (see raspi_init() function) */
>      dc->user_creatable = false;
>  }
>  
> -static const TypeInfo bcm283x_type_info = {
> -    .name = TYPE_BCM283X,
> -    .parent = TYPE_DEVICE,
> -    .instance_size = sizeof(BCM283XState),
> -    .instance_init = bcm2836_init,
> -    .class_size = sizeof(BCM283XClass),
> -    .abstract = true,
> +static void bcm2836_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    BCM283XClass *bc = BCM283X_CLASS(oc);
> +
> +    bc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a7");
> +    bc->peri_base = 0x3f000000;
> +    bc->ctrl_base = 0x40000000;
> +    bc->clusterid = 0xf;
> +    dc->realize = bcm2836_realize;
> +    device_class_set_props(dc, bcm2836_props);
>  };
>  
> -static void bcm2836_register_types(void)
> +#ifdef TARGET_AARCH64
> +static void bcm2837_class_init(ObjectClass *oc, void *data)
>  {
> -    int i;
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    BCM283XClass *bc = BCM283X_CLASS(oc);
>  
> -    type_register_static(&bcm283x_type_info);
> -    for (i = 0; i < ARRAY_SIZE(bcm283x_socs); i++) {
> -        TypeInfo ti = {
> -            .name = bcm283x_socs[i].name,
> -            .parent = TYPE_BCM283X,
> -            .class_init = bcm283x_class_init,
> -            .class_data = (void *) &bcm283x_socs[i],
> -        };
> -        type_register(&ti);
> +    bc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> +    bc->peri_base = 0x3f000000;
> +    bc->ctrl_base = 0x40000000;
> +    bc->clusterid = 0x0;
> +    dc->realize = bcm2836_realize;
> +    device_class_set_props(dc, bcm2836_props);
both children do use the same values for almost all fields.
I'd set in children class_init()s only cpu_type/clusterid
and keep common bits in base class.


> +};
> +#endif
> +
> +static const TypeInfo bcm283x_types[] = {
> +    {
> +        .name           = TYPE_BCM2836,
> +        .parent         = TYPE_BCM283X,
> +        .class_init     = bcm2836_class_init,
> +#ifdef TARGET_AARCH64
> +    }, {
> +        .name           = TYPE_BCM2837,
> +        .parent         = TYPE_BCM283X,
> +        .class_init     = bcm2837_class_init,
> +#endif
> +    }, {
> +        .name           = TYPE_BCM283X,
> +        .parent         = TYPE_DEVICE,
> +        .instance_size  = sizeof(BCM283XState),
> +        .instance_init  = bcm2836_init,
> +        .class_size     = sizeof(BCM283XClass),
> +        .class_init     = bcm283x_class_init,
> +        .abstract       = true,
>      }
> -}
> +};
>  
> -type_init(bcm2836_register_types)
> +DEFINE_TYPES(bcm283x_types)


Re: [PATCH v2 07/13] hw/arm/bcm2836: QOM'ify more by adding class_init() to each SoC type
Posted by Philippe Mathieu-Daudé 5 years, 8 months ago
On 2/18/20 6:04 PM, Igor Mammedov wrote:
> On Mon, 17 Feb 2020 12:45:27 +0100
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
>> Remove usage of TypeInfo::class_data. Instead fill the fields in
>> the corresponding class_init().
>>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/arm/bcm2836.c | 109 ++++++++++++++++++++++-------------------------
>>   1 file changed, 51 insertions(+), 58 deletions(-)
>>
>> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
>> index 24109fef1d..683d04d6ea 100644
>> --- a/hw/arm/bcm2836.c
>> +++ b/hw/arm/bcm2836.c
>> @@ -16,57 +16,30 @@
>>   #include "hw/arm/raspi_platform.h"
>>   #include "hw/sysbus.h"
>>   
>> -typedef struct BCM283XInfo BCM283XInfo;
>> -
>>   typedef struct BCM283XClass {
>>       /*< private >*/
>>       DeviceClass parent_class;
>>       /*< public >*/
>> -    const BCM283XInfo *info;
>> -} BCM283XClass;
>> -
>> -struct BCM283XInfo {
>> -    const char *name;
> 
>>       const char *cpu_type;
> 
> probably could be cleaned up by using machine->cpu_type/machine_class->default_cpu_type
> (no need to change this patch as it's separate issue)

Hmm the core_type is tied to the SoC, not to the machine. The machine 
only selects a SoC and gets the cpu cores that come with it. The problem 
is QEMU does not provide interface to select SoC from cmdline, only CPU.

> 
> 
>>       hwaddr peri_base; /* Peripheral base address seen by the CPU */
>>       hwaddr ctrl_base; /* Interrupt controller and mailboxes etc. */
>>       int clusterid;
>> -};
>> +} BCM283XClass;
>>   
>>   #define BCM283X_CLASS(klass) \
>>       OBJECT_CLASS_CHECK(BCM283XClass, (klass), TYPE_BCM283X)
>>   #define BCM283X_GET_CLASS(obj) \
>>       OBJECT_GET_CLASS(BCM283XClass, (obj), TYPE_BCM283X)
>>   
>> -static const BCM283XInfo bcm283x_socs[] = {
>> -    {
>> -        .name = TYPE_BCM2836,
>> -        .cpu_type = ARM_CPU_TYPE_NAME("cortex-a7"),
>> -        .peri_base = 0x3f000000,
>> -        .ctrl_base = 0x40000000,
>> -        .clusterid = 0xf,
>> -    },
>> -#ifdef TARGET_AARCH64
>> -    {
>> -        .name = TYPE_BCM2837,
>> -        .cpu_type = ARM_CPU_TYPE_NAME("cortex-a53"),
>> -        .peri_base = 0x3f000000,
>> -        .ctrl_base = 0x40000000,
>> -        .clusterid = 0x0,
>> -    },
>> -#endif
>> -};
>> -
>>   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_child(obj, "cpu[*]", &s->cpu[n].core,
>> -                                sizeof(s->cpu[n].core), info->cpu_type,
>> +                                sizeof(s->cpu[n].core), bc->cpu_type,
>>                                   &error_abort, NULL);
>>       }
>>   
>> @@ -85,7 +58,6 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
>>   {
>>       BCM283XState *s = BCM283X(dev);
>>       BCM283XClass *bc = BCM283X_GET_CLASS(dev);
>> -    const BCM283XInfo *info = bc->info;
>>       Object *obj;
>>       Error *err = NULL;
>>       int n;
>> @@ -119,7 +91,7 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
>>       }
>>   
>>       sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->peripherals), 0,
>> -                            info->peri_base, 1);
>> +                            bc->peri_base, 1);
>>   
>>       /* bcm2836 interrupt controller (and mailboxes, etc.) */
>>       object_property_set_bool(OBJECT(&s->control), true, "realized", &err);
>> @@ -128,7 +100,7 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
>>           return;
>>       }
>>   
>> -    sysbus_mmio_map(SYS_BUS_DEVICE(&s->control), 0, info->ctrl_base);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->control), 0, bc->ctrl_base);
>>   
>>       sysbus_connect_irq(SYS_BUS_DEVICE(&s->peripherals), 0,
>>           qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-irq", 0));
>> @@ -137,11 +109,11 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
>>   
>>       for (n = 0; n < BCM283X_NCPUS; n++) {
>>           /* TODO: this should be converted to a property of ARM_CPU */
>> -        s->cpu[n].core.mp_affinity = (info->clusterid << 8) | n;
>> +        s->cpu[n].core.mp_affinity = (bc->clusterid << 8) | n;
>>   
>>           /* set periphbase/CBAR value for CPU-local registers */
>>           object_property_set_int(OBJECT(&s->cpu[n].core),
>> -                                info->peri_base,
>> +                                bc->peri_base,
>>                                   "reset-cbar", &err);
>>           if (err) {
>>               error_propagate(errp, err);
>> @@ -190,38 +162,59 @@ static Property bcm2836_props[] = {
>>   static void bcm283x_class_init(ObjectClass *oc, void *data)
>>   {
>>       DeviceClass *dc = DEVICE_CLASS(oc);
>> -    BCM283XClass *bc = BCM283X_CLASS(oc);
>>   
>> -    bc->info = data;
>> -    dc->realize = bcm2836_realize;
>> -    device_class_set_props(dc, bcm2836_props);
>>       /* Reason: Must be wired up in code (see raspi_init() function) */
>>       dc->user_creatable = false;
>>   }
>>   
>> -static const TypeInfo bcm283x_type_info = {
>> -    .name = TYPE_BCM283X,
>> -    .parent = TYPE_DEVICE,
>> -    .instance_size = sizeof(BCM283XState),
>> -    .instance_init = bcm2836_init,
>> -    .class_size = sizeof(BCM283XClass),
>> -    .abstract = true,
>> +static void bcm2836_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +    BCM283XClass *bc = BCM283X_CLASS(oc);
>> +
>> +    bc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a7");
>> +    bc->peri_base = 0x3f000000;
>> +    bc->ctrl_base = 0x40000000;
>> +    bc->clusterid = 0xf;
>> +    dc->realize = bcm2836_realize;
>> +    device_class_set_props(dc, bcm2836_props);
>>   };
>>   
>> -static void bcm2836_register_types(void)
>> +#ifdef TARGET_AARCH64
>> +static void bcm2837_class_init(ObjectClass *oc, void *data)
>>   {
>> -    int i;
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +    BCM283XClass *bc = BCM283X_CLASS(oc);
>>   
>> -    type_register_static(&bcm283x_type_info);
>> -    for (i = 0; i < ARRAY_SIZE(bcm283x_socs); i++) {
>> -        TypeInfo ti = {
>> -            .name = bcm283x_socs[i].name,
>> -            .parent = TYPE_BCM283X,
>> -            .class_init = bcm283x_class_init,
>> -            .class_data = (void *) &bcm283x_socs[i],
>> -        };
>> -        type_register(&ti);
>> +    bc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
>> +    bc->peri_base = 0x3f000000;
>> +    bc->ctrl_base = 0x40000000;
>> +    bc->clusterid = 0x0;
>> +    dc->realize = bcm2836_realize;
>> +    device_class_set_props(dc, bcm2836_props);
> both children do use the same values for almost all fields.
> I'd set in children class_init()s only cpu_type/clusterid
> and keep common bits in base class.

Yes so far, but then the BCM2711/BCM2838 for raspi4 changes all fields.

> 
>> +};
>> +#endif
>> +
>> +static const TypeInfo bcm283x_types[] = {
>> +    {
>> +        .name           = TYPE_BCM2836,
>> +        .parent         = TYPE_BCM283X,
>> +        .class_init     = bcm2836_class_init,
>> +#ifdef TARGET_AARCH64
>> +    }, {
>> +        .name           = TYPE_BCM2837,
>> +        .parent         = TYPE_BCM283X,
>> +        .class_init     = bcm2837_class_init,
>> +#endif
>> +    }, {
>> +        .name           = TYPE_BCM283X,
>> +        .parent         = TYPE_DEVICE,
>> +        .instance_size  = sizeof(BCM283XState),
>> +        .instance_init  = bcm2836_init,
>> +        .class_size     = sizeof(BCM283XClass),
>> +        .class_init     = bcm283x_class_init,
>> +        .abstract       = true,
>>       }
>> -}
>> +};
>>   
>> -type_init(bcm2836_register_types)
>> +DEFINE_TYPES(bcm283x_types)
> 


Re: [PATCH v2 07/13] hw/arm/bcm2836: QOM'ify more by adding class_init() to each SoC type
Posted by Igor Mammedov 5 years, 8 months ago
On Tue, 18 Feb 2020 18:39:49 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 2/18/20 6:04 PM, Igor Mammedov wrote:
> > On Mon, 17 Feb 2020 12:45:27 +0100
> > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >   
> >> Remove usage of TypeInfo::class_data. Instead fill the fields in
> >> the corresponding class_init().
> >>
> >> Cc: Igor Mammedov <imammedo@redhat.com>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> ---
> >>   hw/arm/bcm2836.c | 109 ++++++++++++++++++++++-------------------------
> >>   1 file changed, 51 insertions(+), 58 deletions(-)
> >>
> >> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
> >> index 24109fef1d..683d04d6ea 100644
> >> --- a/hw/arm/bcm2836.c
> >> +++ b/hw/arm/bcm2836.c
> >> @@ -16,57 +16,30 @@
> >>   #include "hw/arm/raspi_platform.h"
> >>   #include "hw/sysbus.h"
> >>   
> >> -typedef struct BCM283XInfo BCM283XInfo;
> >> -
> >>   typedef struct BCM283XClass {
> >>       /*< private >*/
> >>       DeviceClass parent_class;
> >>       /*< public >*/
> >> -    const BCM283XInfo *info;
> >> -} BCM283XClass;
> >> -
> >> -struct BCM283XInfo {
> >> -    const char *name;  
> >   
> >>       const char *cpu_type;  
> > 
> > probably could be cleaned up by using machine->cpu_type/machine_class->default_cpu_type
> > (no need to change this patch as it's separate issue)  
> 
> Hmm the core_type is tied to the SoC, not to the machine. The machine 
> only selects a SoC and gets the cpu cores that come with it. The problem 
> is QEMU does not provide interface to select SoC from cmdline, only CPU.
> 
> > 
> >   
> >>       hwaddr peri_base; /* Peripheral base address seen by the CPU */
> >>       hwaddr ctrl_base; /* Interrupt controller and mailboxes etc. */
> >>       int clusterid;
> >> -};
> >> +} BCM283XClass;
> >>   
> >>   #define BCM283X_CLASS(klass) \
> >>       OBJECT_CLASS_CHECK(BCM283XClass, (klass), TYPE_BCM283X)
> >>   #define BCM283X_GET_CLASS(obj) \
> >>       OBJECT_GET_CLASS(BCM283XClass, (obj), TYPE_BCM283X)
> >>   
> >> -static const BCM283XInfo bcm283x_socs[] = {
> >> -    {
> >> -        .name = TYPE_BCM2836,
> >> -        .cpu_type = ARM_CPU_TYPE_NAME("cortex-a7"),
> >> -        .peri_base = 0x3f000000,
> >> -        .ctrl_base = 0x40000000,
> >> -        .clusterid = 0xf,
> >> -    },
> >> -#ifdef TARGET_AARCH64
> >> -    {
> >> -        .name = TYPE_BCM2837,
> >> -        .cpu_type = ARM_CPU_TYPE_NAME("cortex-a53"),
> >> -        .peri_base = 0x3f000000,
> >> -        .ctrl_base = 0x40000000,
> >> -        .clusterid = 0x0,
> >> -    },
> >> -#endif
> >> -};
> >> -
> >>   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_child(obj, "cpu[*]", &s->cpu[n].core,
> >> -                                sizeof(s->cpu[n].core), info->cpu_type,
> >> +                                sizeof(s->cpu[n].core), bc->cpu_type,
> >>                                   &error_abort, NULL);
> >>       }
> >>   
> >> @@ -85,7 +58,6 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
> >>   {
> >>       BCM283XState *s = BCM283X(dev);
> >>       BCM283XClass *bc = BCM283X_GET_CLASS(dev);
> >> -    const BCM283XInfo *info = bc->info;
> >>       Object *obj;
> >>       Error *err = NULL;
> >>       int n;
> >> @@ -119,7 +91,7 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
> >>       }
> >>   
> >>       sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->peripherals), 0,
> >> -                            info->peri_base, 1);
> >> +                            bc->peri_base, 1);
> >>   
> >>       /* bcm2836 interrupt controller (and mailboxes, etc.) */
> >>       object_property_set_bool(OBJECT(&s->control), true, "realized", &err);
> >> @@ -128,7 +100,7 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
> >>           return;
> >>       }
> >>   
> >> -    sysbus_mmio_map(SYS_BUS_DEVICE(&s->control), 0, info->ctrl_base);
> >> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->control), 0, bc->ctrl_base);
> >>   
> >>       sysbus_connect_irq(SYS_BUS_DEVICE(&s->peripherals), 0,
> >>           qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-irq", 0));
> >> @@ -137,11 +109,11 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
> >>   
> >>       for (n = 0; n < BCM283X_NCPUS; n++) {
> >>           /* TODO: this should be converted to a property of ARM_CPU */
> >> -        s->cpu[n].core.mp_affinity = (info->clusterid << 8) | n;
> >> +        s->cpu[n].core.mp_affinity = (bc->clusterid << 8) | n;
> >>   
> >>           /* set periphbase/CBAR value for CPU-local registers */
> >>           object_property_set_int(OBJECT(&s->cpu[n].core),
> >> -                                info->peri_base,
> >> +                                bc->peri_base,
> >>                                   "reset-cbar", &err);
> >>           if (err) {
> >>               error_propagate(errp, err);
> >> @@ -190,38 +162,59 @@ static Property bcm2836_props[] = {
> >>   static void bcm283x_class_init(ObjectClass *oc, void *data)
> >>   {
> >>       DeviceClass *dc = DEVICE_CLASS(oc);
> >> -    BCM283XClass *bc = BCM283X_CLASS(oc);
> >>   
> >> -    bc->info = data;
> >> -    dc->realize = bcm2836_realize;
> >> -    device_class_set_props(dc, bcm2836_props);
> >>       /* Reason: Must be wired up in code (see raspi_init() function) */
> >>       dc->user_creatable = false;
> >>   }
> >>   
> >> -static const TypeInfo bcm283x_type_info = {
> >> -    .name = TYPE_BCM283X,
> >> -    .parent = TYPE_DEVICE,
> >> -    .instance_size = sizeof(BCM283XState),
> >> -    .instance_init = bcm2836_init,
> >> -    .class_size = sizeof(BCM283XClass),
> >> -    .abstract = true,
> >> +static void bcm2836_class_init(ObjectClass *oc, void *data)
> >> +{
> >> +    DeviceClass *dc = DEVICE_CLASS(oc);
> >> +    BCM283XClass *bc = BCM283X_CLASS(oc);
> >> +
> >> +    bc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a7");
> >> +    bc->peri_base = 0x3f000000;
> >> +    bc->ctrl_base = 0x40000000;
> >> +    bc->clusterid = 0xf;
> >> +    dc->realize = bcm2836_realize;
> >> +    device_class_set_props(dc, bcm2836_props);
> >>   };
> >>   
> >> -static void bcm2836_register_types(void)
> >> +#ifdef TARGET_AARCH64
> >> +static void bcm2837_class_init(ObjectClass *oc, void *data)
> >>   {
> >> -    int i;
> >> +    DeviceClass *dc = DEVICE_CLASS(oc);
> >> +    BCM283XClass *bc = BCM283X_CLASS(oc);
> >>   
> >> -    type_register_static(&bcm283x_type_info);
> >> -    for (i = 0; i < ARRAY_SIZE(bcm283x_socs); i++) {
> >> -        TypeInfo ti = {
> >> -            .name = bcm283x_socs[i].name,
> >> -            .parent = TYPE_BCM283X,
> >> -            .class_init = bcm283x_class_init,
> >> -            .class_data = (void *) &bcm283x_socs[i],
> >> -        };
> >> -        type_register(&ti);
> >> +    bc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> >> +    bc->peri_base = 0x3f000000;
> >> +    bc->ctrl_base = 0x40000000;
> >> +    bc->clusterid = 0x0;
> >> +    dc->realize = bcm2836_realize;
> >> +    device_class_set_props(dc, bcm2836_props);  
> > both children do use the same values for almost all fields.
> > I'd set in children class_init()s only cpu_type/clusterid
> > and keep common bits in base class.  
> 
> Yes so far, but then the BCM2711/BCM2838 for raspi4 changes all fields.

I this case, maybe add this as justification to commit message.

With that change
  Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> 
> >   
> >> +};
> >> +#endif
> >> +
> >> +static const TypeInfo bcm283x_types[] = {
> >> +    {
> >> +        .name           = TYPE_BCM2836,
> >> +        .parent         = TYPE_BCM283X,
> >> +        .class_init     = bcm2836_class_init,
> >> +#ifdef TARGET_AARCH64
> >> +    }, {
> >> +        .name           = TYPE_BCM2837,
> >> +        .parent         = TYPE_BCM283X,
> >> +        .class_init     = bcm2837_class_init,
> >> +#endif
> >> +    }, {
> >> +        .name           = TYPE_BCM283X,
> >> +        .parent         = TYPE_DEVICE,
> >> +        .instance_size  = sizeof(BCM283XState),
> >> +        .instance_init  = bcm2836_init,
> >> +        .class_size     = sizeof(BCM283XClass),
> >> +        .class_init     = bcm283x_class_init,
> >> +        .abstract       = true,
> >>       }
> >> -}
> >> +};
> >>   
> >> -type_init(bcm2836_register_types)
> >> +DEFINE_TYPES(bcm283x_types)  
> >   
>