On 16 June 2014 11:19, Andreas Färber <afaer...@suse.de> 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
