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

[Peter Crosthwaite]

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.

Regards,
Peter

> Andrew