Re: [PATCH 01/44] Split out common part of BCM283X classes

[Peter Maydell]

On Thu, 3 Aug 2023 at 16:48, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 26 Jul 2023 at 14:43, Sergey Kambalin <serg.oker@gmail.com> wrote:
> >
> > Signed-off-by: Sergey Kambalin <sergey.kambalin@auriga.com>
> > ---
> >  hw/arm/bcm2836.c         | 102 ++++++++++++++++++++++-----------------
> >  hw/arm/raspi.c           |   2 +-
> >  include/hw/arm/bcm2836.h |  26 +++++++++-
> >  3 files changed, 83 insertions(+), 47 deletions(-)
>
> > @@ -230,11 +238,17 @@ static const TypeInfo bcm283x_types[] = {
> >  #endif
> >      }, {
> >          .name           = TYPE_BCM283X,
> > -        .parent         = TYPE_DEVICE,
> > +        .parent         = TYPE_BCM283X_BASE,
> >          .instance_size  = sizeof(BCM283XState),
> > -        .instance_init  = bcm2836_init,
> > -        .class_size     = sizeof(BCM283XClass),
> > -        .class_init     = bcm283x_class_init,
> > +        .instance_init  = bcm283x_init,
> > +        .abstract       = true,
> > +    }, {
> > +        .name           = TYPE_BCM283X_BASE,
> > +        .parent         = TYPE_DEVICE,
> > +        .instance_size  = sizeof(BCM283XBaseState),
> > +        .instance_init  = bcm283x_base_init,
> > +        .class_size     = sizeof(BCM283XBaseClass),
> > +        .class_init     = bcm283x_base_class_init,
> >          .abstract       = true,
> >      }
> >  };
> > diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> > +
> > +struct BCM283XBaseClass {
> > +    /*< private >*/
> > +    DeviceClass parent_class;
> > +    /*< public >*/
> > +    const char *name;
> > +    const char *cpu_type;
> > +    unsigned core_count;
> > +    hwaddr peri_base; /* Peripheral base address seen by the CPU */
> > +    hwaddr ctrl_base; /* Interrupt controller and mailboxes etc. */
> > +    int clusterid;
> > +};
> > +
> > +struct BCM283XState {
> > +    /*< private >*/
> > +    BCM283XBaseState parent_obj;
> > +    /*< public >*/
> >      BCM2835PeripheralState peripherals;
> >  };
> >
>
> This gives us a slightly odd class hierarchy where we have
> two "common between bcmxxxx SoCs" types:
>
>    TYPE_BCM283X_BASE --> TYPE_BCM283X --> TYPE_BCM2835
>                      |                |-> TYPE_BCM2836
>                      |                \-> TYPE_BCM2837
>                      \-> TYPE_BCM2838
>
> The only thing TYPE_BCM283X seems to be doing here that
> TYPE_BCM283X_BASE is not is handling the BCM2835PeripheralState
> object. Would it be clearer to keep the existing
> class hierarchy where everything inherits from
> TYPE_BCM283X, and accept a little code duplication for
> the 3 subclasses that use the same BCM2835PeripheralState?
> I'm not sure...

Ah, looking at the later parts of the patchset I think I
see the issue -- because the board code wants to
embed the SoC object in the machine struct, not having
a common type for the BCM283[567] which is the same size
for all of them would mean that the board code would
also have to split rather than being common for those
SoC types. This is the downside of our "embed the structs"
style of board and SoC code, I guess.

So it's a bit swings-and-roundabouts, and since you've
written the code this way, let's go with doing it this
way. I can always come back and try a refactoring
later if it bothers me too much :-)

-- PMM