On Mon, Jun 16, 2014 at 2:43 PM, Peter Crosthwaite
<peter.crosthwa...@xilinx.com> wrote:
> On Tue, Jun 10, 2014 at 11:32 AM, Alistair Francis
> <alistair.fran...@xilinx.com> wrote:
>> This patch adds the Cortex-A9 ARM CPU to the A9MPCore. It
>> first does a check to make sure no other CPUs exist and if
>> they do the Cortex-A9 won't be added. This is implemented to
>> maintain compatibility and can be removed once all machines
>> have been updated
>>
>> This patch also allows the midr and reset-property to be set
>>
>> Signed-off-by: Alistair Francis <alistair.fran...@xilinx.com>
>> ---
>> There comments in the code explaining the reason that the CPU
>> is initiated in the realize function. This is because it relies
>> on the num_cpu property, which isn't yet set in the initfn
>> Is this an acceptable compromise?
>>
>> hw/cpu/a9mpcore.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>> include/hw/cpu/a9mpcore.h | 4 ++++
>> 2 files changed, 47 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
>> index c09358c..1159044 100644
>> --- a/hw/cpu/a9mpcore.c
>> +++ b/hw/cpu/a9mpcore.c
>> @@ -21,6 +21,12 @@ static void a9mp_priv_initfn(Object *obj)
>> {
>> A9MPPrivState *s = A9MPCORE_PRIV(obj);
>>
>> + /* Ideally would init the CPUs here, but the num_cpu property has not
>> been
>> + * set yet. So that only works if assuming a single CPU
>> + * object_initialize(&s->cpu, sizeof(s->cpu), "cortex-a9-"
>> TYPE_ARM_CPU);
>> + * object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
>> + */
>> +
>
> 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?
I'm open for ideas/opinions. The method used here seemed to be the easiest
to implement (and actually the only reliable method that I could think of).
>
>> 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.
>
>> + 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.
>
>> +
>> + 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?
I shouldn't have done that, I don't know how that slipped through
>
>> + }
>> +
>> 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.
>
> Regards,
> Peter
All comments were changed. I'll give it a few days and see if anyone else
has any comments, otherwise I might release a patch following the same
style as this RFC
>
>> +
>> A9SCUState scu;
>> GICState gic;
>> A9GTimerState gtimer;
>> --
>> 1.7.1
>>
>>
>
