On Tue, Feb 19, 2013 at 4:49 AM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 8 February 2013 04:03, Peter Crosthwaite
> <peter.crosthwa...@xilinx.com> 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
>
