Re: [Qemu-devel] [PATCH v1 6/6] arm: a9mpcore: Coreify the SCU

[Peter Crosthwaite]

Hi Andreas,

On Tue, Feb 19, 2013 at 6:19 AM, Andreas Färber <address@hidden> wrote:
> Am 18.02.2013 19:49, schrieb Peter Maydell:
>> On 8 February 2013 04:03, Peter Crosthwaite
>> <address@hidden> wrote:
>>> Split the SCU in a9mpcore out into its own object definition. mpcore is now
>>> just a container for the mpcore components.
>>
>> Good idea.
>>
>>> --- a/hw/a9mpcore.c
>>> +++ b/hw/a9mpcore.c
>>> @@ -14,107 +14,12 @@
>>>
>>>  typedef struct A9MPPrivState {
>>>      SysBusDevice busdev;
>>> -    uint32_t scu_control;
>>> -    uint32_t scu_status;
>>>      uint32_t num_cpu;
>>> -    MemoryRegion scu_iomem;
>>>      MemoryRegion container;
>>>      DeviceState *gic;
>>>      uint32_t num_irq;
>>>  } A9MPPrivState;
>>
>> You need to add a DeviceState* for the scu.
>
> No, not a DeviceState*, an A9SCUState. With object_initialize() and
> qdev_set_parent_bus(NULL) instead of qdev_create() to be exact and some
> child<A9SCUState> property for ownership transfer. 2/7 and commit
> message say why.
>

Hi Andreas, what you are proposing is a major change pattern to pretty
much the entire tree - every device that is part of a container object
needs to inlined (thus creating public headers). Despite the fact that
I disagree with that approach, this change is way out of scope of this
series and changing just the SCU to be like this will make it
inconsistent with its peer devices GIC and MPTimer. This is cut and
paste re-organisation of existing code that is groundwork for what you
are talking about and the patch still stands in its own right in that
this scheme is better than what we have today.

So I'd like to take a crawl before we walk approach to this patch. For
the next revision i'm going to do it Peters way and ask that we sort
out the big questions about QOM containers and inline-structs for
MPCore in another patch series. Then we can fix GIC and MPTimer at the
same time and everything is consistent. Too often when contributors
submit patches some minor issue get tangled in large out of scope
discussions about QOM that relate to the entire tree. The whole series
then ends up bitrotting on list or living out of tree forever.

Regards,
Peter

>>> diff --git a/hw/a9scu.c b/hw/a9scu.c
>>> new file mode 100644
>>> index 0000000..0a3d411
>>> --- /dev/null
>>> +++ b/hw/a9scu.c
> [...]
>>> +static void a9_scu_class_init(ObjectClass *klass, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>>> +
>>> +    k->init = a9_scu_init;
>>
>> This should have an instance_init and/or realize method,
>> not a SysBusDeviceClass::init (see comments on PL330 patch).
>>
>>> +    dc->props = a9_scu_properties;
>>> +    dc->vmsd = &vmstate_a9_scu;
>>> +    dc->reset = a9_scu_reset;
>>> +}
>>> +
>>> +static TypeInfo a9_scu_info = {
>
> static const
>

Fixed

Regards,
Peter

> Regards,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>