From: Igor Mammedov <imammedo@redhat.com>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: "Peter Maydell" <peter.maydell@linaro.org>,
"Philippe Mathieu-Daudé" <f4bug@amsat.org>,
"Andrew Baumann" <Andrew.Baumann@microsoft.com>,
qemu-devel@nongnu.org, qemu-arm@nongnu.org,
"Luc Michel" <luc.michel@greensocs.com>
Subject: Re: [PATCH v2 07/13] hw/arm/bcm2836: QOM'ify more by adding class_init() to each SoC type
Date: Wed, 19 Feb 2020 18:28:45 +0100 [thread overview]
Message-ID: <20200219182845.4541db2e@redhat.com> (raw)
In-Reply-To: <effdb318-dff9-e861-7bc6-8db65f15ad00@redhat.com>
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)
> >
>
next prev parent reply other threads:[~2020-02-19 17:29 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-17 11:45 [PATCH v2 00/13] hw/arm: Add raspi0 and raspi1 machines Philippe Mathieu-Daudé
2020-02-17 11:45 ` [PATCH v2 01/13] hw/arm/raspi: Remove ignore_memory_transaction_failures on the raspi2 Philippe Mathieu-Daudé
2020-02-18 9:07 ` Luc Michel
2020-02-17 11:45 ` [PATCH v2 02/13] hw/arm/raspi: Avoid using TypeInfo::class_data pointer Philippe Mathieu-Daudé
2020-02-18 16:50 ` Igor Mammedov
2020-02-17 11:45 ` [PATCH v2 03/13] hw/arm/raspi: Use more specific machine names Philippe Mathieu-Daudé
2020-02-18 9:07 ` Luc Michel
2020-02-17 11:45 ` [PATCH v2 04/13] hw/arm/raspi: Introduce RaspiProcessorId enum Philippe Mathieu-Daudé
2020-02-18 8:24 ` Luc Michel
2020-02-17 11:45 ` [PATCH v2 05/13] hw/arm/raspi: Remove use of the 'version' value in the board code Philippe Mathieu-Daudé
2020-02-18 8:35 ` Luc Michel
2020-02-17 11:45 ` [PATCH v2 06/13] hw/arm/bcm2836: Restrict BCM283XClass declaration to C source Philippe Mathieu-Daudé
2020-02-18 8:55 ` Luc Michel
2020-02-17 11:45 ` [PATCH v2 07/13] hw/arm/bcm2836: QOM'ify more by adding class_init() to each SoC type Philippe Mathieu-Daudé
2020-02-18 17:04 ` Igor Mammedov
2020-02-18 17:39 ` Philippe Mathieu-Daudé
2020-02-19 17:28 ` Igor Mammedov [this message]
2020-02-17 11:45 ` [PATCH v2 08/13] hw/arm/bcm2836: Introduce BCM283XClass::core_count Philippe Mathieu-Daudé
2020-02-18 9:00 ` Luc Michel
2020-02-17 11:45 ` [PATCH v2 09/13] hw/arm/bcm2836: Only provide "enabled-cpus" property to multicore SoCs Philippe Mathieu-Daudé
2020-02-18 9:01 ` Luc Michel
2020-02-17 11:45 ` [PATCH v2 10/13] hw/arm/bcm2836: Split out common realize() code Philippe Mathieu-Daudé
2020-02-18 9:03 ` Luc Michel
2020-02-17 11:45 ` [PATCH v2 11/13] hw/arm/bcm2836: Introduce the BCM2835 SoC Philippe Mathieu-Daudé
2020-02-18 9:04 ` Luc Michel
2020-02-17 11:45 ` [PATCH v2 12/13] hw/arm/raspi: Add the Raspberry Pi B+ machine Philippe Mathieu-Daudé
2020-02-18 8:48 ` Luc Michel
2020-02-18 9:35 ` Philippe Mathieu-Daudé
2020-02-18 16:48 ` Igor Mammedov
2020-02-21 18:30 ` Eduardo Habkost
2020-02-22 22:19 ` Niek Linnenbank
2020-02-24 8:55 ` Philippe Mathieu-Daudé
2020-09-21 16:45 ` Philippe Mathieu-Daudé
2020-02-17 11:45 ` [PATCH v2 13/13] hw/arm/raspi: Add the Raspberry Pi Zero machine Philippe Mathieu-Daudé
2020-02-18 8:49 ` Luc Michel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200219182845.4541db2e@redhat.com \
--to=imammedo@redhat.com \
--cc=Andrew.Baumann@microsoft.com \
--cc=f4bug@amsat.org \
--cc=luc.michel@greensocs.com \
--cc=peter.maydell@linaro.org \
--cc=philmd@redhat.com \
--cc=qemu-arm@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.