Re: [Qemu-devel] [PATCH v4 8/8] raspi: add raspberry pi 2 machine

[Andrew Baumann]

> From: Peter Crosthwaite [mailto:address@hidden
> Sent: Friday, 29 January 2016 14:41
> 
> On Fri, Jan 29, 2016 at 2:28 PM, Andrew Baumann
> <address@hidden> wrote:
> >> From: Peter Crosthwaite [mailto:address@hidden
> >> Sent: Friday, 29 January 2016 14:23
> >>
> >> On Fri, Jan 29, 2016 at 1:50 PM, Andrew Baumann
> >> <address@hidden> wrote:
> >> > Hi Peter,
> >> >
> >> > Thanks for all the reviews. I should have a respun version on the list
> shortly.
> >> There's one minor change to this last patch:
> >> >
> >> >> From: Peter Crosthwaite [mailto:address@hidden
> >> >> Sent: Thursday, 28 January 2016 23:31
> >> >> > On Fri, Jan 15, 2016 at 3:58 PM, Andrew Baumann
> >> <address@hidden> wrote:
> >> >  [...]
> >> >> > +typedef struct RaspiState {
> >> >>
> >> >> A quick google search, I see the camel case form for rpi is usually
> >> >> "RasPi". Should we follow?
> >> >
> >> > Ok.
> >> >
> >> >> > +    union {
> >> >>
> >> >> union not needed.
> >> >
> >> > I know it's not needed now, but it will be as soon as we add pi1, which I
> >> hope to address in the next patch series. It will make that diff cleaner if
> we
> >> keep this here now, so I'm going to leave it as-is. I hope that's ok with 
> >> you.
> >> >
> >>
> >> It sounds like you are implementing an inheritance outside QOM. I'm
> >> not sure about this, can we just drop the union and figure it out on
> >> the next series?
> >
> > It's nothing nearly that clever/complicated. See
> https://github.com/0xabu/qemu/blob/raspi/hw/arm/raspi.c#L116
> >
> >     switch (version) {
> >     case 1:
> >         object_initialize(&s->soc.pi1, sizeof(s->soc.pi1), TYPE_BCM2835);
> >         break;
> >     case 2:
> >         object_initialize(&s->soc.pi2, sizeof(s->soc.pi2), TYPE_BCM2836);
> >         break;
> >     }
> >
> > And then later, we always refer simply to OBJECT(&s->soc). I suppose I
> could use a generic Object pointer instead, and allocate the soc-specific type
> elsewhere, but it seemed simpler with the union.
> >
> 
> But this is effectively an upcast to an abstract type, here:
> 
> +
> +    /* Setup the SOC */
> +    object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s-
> >ram),
> +                                   &error_abort);
> +    object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus",
> +                            &error_abort);
> 
> Where that concrete type musts implement "ram" and "enabled-cpus". So
> I am unsure of having a split implementation of "ram", "enabled-cpus"
> etc from one SoC to the other as you are using a QOM property name as
> an abstract interface definition.

Ok, fair enough. Indeed, "enabled-cpus" is actually a bug there for pi1. I'll 
drop the union for now, and rework it for pi1.

Andrew