Re: [Qemu-devel] [RFC v1 1/2] arm: Add the cortex-a9 CPU to the a9mpcore device

[Peter Maydell]

On 16 June 2014 11:19, Andreas Färber <address@hidden> wrote:
> Am 16.06.2014 06:43, schrieb Peter Crosthwaite:
>> So you could add an integer property listener to init them earlier (or
>> even do dynamic extending/freeing or the allocated CPUs). I'm not sure
>> exactly what we are really supposed to do though, when the number of
>> child object depends on a prop like this? Andreas?
>
> PMM had added or looked into a property creating an array of properties.
> However a more fundamental issue that PMM was unsure about is whether
> the CPUs should be child<> of MPCore as done here or a sibling of the
> MPCore container.

Was I? I can't currently see any reason you'd want them to be
siblings of the container rather than children...

I think property-listeners was the mechanism we talked
about for when you need to do something before realize
but it depends on some property, yes.

>>>      memory_region_init(&s->container, obj, "a9mp-priv-container", 0x2000);
>>>      sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container);
>>>
>>> @@ -50,6 +56,40 @@ static void a9mp_priv_realize(DeviceState *dev, Error 
>>> **errp)
>>>      Error *err = NULL;
>>>      int i;
>>>
>>> +    /* Just a temporary measure to not break machines that init the CPU
>>> +     * seperatly */
>>
>> "separately"
>>
>>> +    if (!first_cpu) {
>>> +        s->cpu = g_malloc(sizeof(ARMCPU) * s->num_cpu);
>>
>> g_new should be use to allocate arrays.
>
> Whether malloc or new, more important is to 0-initialize the memory
> before running object_initialize() on it! :)
>
>>
>>> +        for (i = 0; i < s->num_cpu; i++) {
>>> +            object_initialize((s->cpu + i), sizeof(*(s->cpu + i)),
>>
>> &s->cpu[i] is more common and easier to read.
>>
>> sizeof(*s->cpu) is fine.
>>
>>> +                              "cortex-a9-" TYPE_ARM_CPU);
>>
>> Use cpu_class_by_name logic like in some of the boards, rather than
>> the string concatenation. The specifics of the concatenation system is
>> (supposed to be) private to target-arm code.
>
> +1
>
>>
>>> +
>>> +            if (s->midr) {
>>> +                object_property_set_int(OBJECT((s->cpu + i)), s->midr,
>>> +                                        "midr", &err);
>>> +                if (err) {
>>> +                    error_propagate(errp, err);
>>> +                    exit(1);
>>> +                }
>>> +            }
>>> +            if (s->reset_cbar) {
>>> +                object_property_set_int(OBJECT((s->cpu + i)), 
>>> s->reset_cbar,
>>> +                                        "reset-cbar", &err);
>>> +                if (err) {
>>> +                    error_propagate(errp, err);
>>> +                    exit(1);
>>> +                }
>>> +            }
>>> +            object_property_set_bool(OBJECT((s->cpu + i)), true,
>>> +                                     "realized", &err);
>>> +            if (err) {
>>> +                error_propagate(errp, err);
>>> +                return;
>>> +            }
>>> +        }
>>> +        g_free(s->cpu);
>>
>> Why free the just-initialized CPUs?
>>
>>> +    }
>>> +
>>>      scudev = DEVICE(&s->scu);
>>>      qdev_prop_set_uint32(scudev, "num-cpu", s->num_cpu);
>>>      object_property_set_bool(OBJECT(&s->scu), true, "realized", &err);
>>> @@ -152,6 +192,9 @@ static Property a9mp_priv_properties[] = {
>>>       * Other boards may differ and should set this property appropriately.
>>>       */
>>>      DEFINE_PROP_UINT32("num-irq", A9MPPrivState, num_irq, 96),
>>> +    /* Properties for the A9 CPU */
>>> +    DEFINE_PROP_UINT32("midr", A9MPPrivState, midr, 0),
>>> +    DEFINE_PROP_UINT64("reset-cbar", A9MPPrivState, reset_cbar, 0),
>>>      DEFINE_PROP_END_OF_LIST(),
>>>  };
>>>
>>> diff --git a/include/hw/cpu/a9mpcore.h b/include/hw/cpu/a9mpcore.h
>>> index 5d67ca2..8e395a4 100644
>>> --- a/include/hw/cpu/a9mpcore.h
>>> +++ b/include/hw/cpu/a9mpcore.h
>>> @@ -29,6 +29,10 @@ typedef struct A9MPPrivState {
>>>      MemoryRegion container;
>>>      uint32_t num_irq;
>>>
>>> +    ARMCPU *cpu;
>>> +    uint32_t midr;
>>
>> I'd preface this as "cpu_midr".
>>
>>> +    uint64_t reset_cbar;
>>
>> MPCores refer to this as PERIPHBASE in their documentation.

Well, generally the external config signal is PERIPHBASE
and the register it affects is CBAR. For QEMU I think
we seem to be generally using CBAR in all contexts.

thanks
-- PMM