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

[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.

> 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).

> +
> +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".

> +    .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).

> +    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