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

[Peter Crosthwaite]

On Tue, Feb 19, 2013 at 4:49 AM, Peter Maydell <address@hidden> wrote:
> 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.
>

Done

>> diff --git a/hw/a9scu.c b/hw/a9scu.c
>> new file mode 100644
>> index 0000000..0a3d411
>> --- /dev/null
>> +++ b/hw/a9scu.c
>> @@ -0,0 +1,162 @@
>> +/*
>> + * Cortex-A9MPCore Snoop Control Unit (SCU) emulation.
>> + *
>> + * Copyright (c) 2009 CodeSourcery.
>> + * Copyright (c) 2011 Linaro Limited.
>> + * Written by Paul Brook, Peter Maydell.
>> + *
>> + * This code is licensed under the GPL.
>> + */
>> +
>> +#include "sysbus.h"
>> +
>> +/* A9MP private memory region.  */
>
> Stale comment (you could just delete it).
>

Done

>> +
>> +typedef struct A9SCUState {
>> +    SysBusDevice busdev;
>> +    MemoryRegion iomem;
>> +    uint32_t control;
>> +    uint32_t status;
>> +    uint32_t num_cpu;
>> +} A9SCUState;
>
>> +static const VMStateDescription vmstate_a9_scu = {
>> +    .name = "a9_scu",
>
> For new devices, hyphen is preferred, so "a9-scu".
>

Fixed globally

>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32(control, A9SCUState),
>> +        VMSTATE_UINT32(status, A9SCUState),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static Property a9_scu_properties[] = {
>> +    DEFINE_PROP_UINT32("num-cpu", A9SCUState, num_cpu, 1),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +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).
>

Fixed as per PL330 review.

Regards,
Peter

>> +    dc->props = a9_scu_properties;
>> +    dc->vmsd = &vmstate_a9_scu;
>> +    dc->reset = a9_scu_reset;
>> +}
>> +
>> +static TypeInfo a9_scu_info = {
>> +    .name          = "arm_a9_scu",
>
> Again, hyphens preferred.
>
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(A9SCUState),
>> +    .class_init    = a9_scu_class_init,
>> +};
>
> thanks
> -- PMM
>