> From: Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com] > Sent: Friday, 29 January 2016 14:41 > > On Fri, Jan 29, 2016 at 2:28 PM, Andrew Baumann > <andrew.baum...@microsoft.com> wrote: > >> From: Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com] > >> Sent: Friday, 29 January 2016 14:23 > >> > >> On Fri, Jan 29, 2016 at 1:50 PM, Andrew Baumann > >> <andrew.baum...@microsoft.com> 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:crosthwaitepe...@gmail.com] > >> >> Sent: Thursday, 28 January 2016 23:31 > >> >> > On Fri, Jan 15, 2016 at 3:58 PM, Andrew Baumann > >> <andrew.baum...@microsoft.com> 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
