Re: [Qemu-devel] [RFC v1 2/2] zynq: Update Zynq to init the CPU in the a9mpcore device

[Alistair Francis]

On Mon, Jun 16, 2014 at 2:42 PM, Peter Crosthwaite
<address@hidden> wrote:
> On Tue, Jun 10, 2014 at 11:33 AM, Alistair Francis
> <address@hidden> wrote:
>> This patch removes the initialisation of the ARM Cortex-A9
>> in Zynq and instead allows the a9mpcore device to init the
>> CPU. This also updates components that rely on the CPU
>> and GIC, as they are now initialised in a slightly different
>> way
>>
>> Signed-off-by: Alistair Francis <address@hidden>
>> ---
>> All other Cortex-A9 machines can be updated a similar way
>>
>> This patch breaks the AArch64 make check tests. I get a:
>> 'Warning: "-global dynamic-prop-type-bad.prop3=103" not used'
>> followed by a broken pipe and failure.
>> Any hints on what would be causing this?
>>
>>  hw/arm/xilinx_zynq.c |   63 
>> +++++++++++++++++++++++--------------------------
>>  1 files changed, 30 insertions(+), 33 deletions(-)
>>
>> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
>> index ba5aa82..5a4ce5c 100644
>> --- a/hw/arm/xilinx_zynq.c
>> +++ b/hw/arm/xilinx_zynq.c
>> @@ -26,6 +26,7 @@
>>  #include "hw/loader.h"
>>  #include "hw/ssi.h"
>>  #include "qemu/error-report.h"
>> +#include "hw/cpu/a9mpcore.h"
>>
>>  #define NUM_SPI_FLASHES 4
>>  #define NUM_QSPI_FLASHES 2
>> @@ -104,12 +105,10 @@ static inline void zynq_init_spi_flashes(uint32_t 
>> base_addr, qemu_irq irq,
>>  static void zynq_init(MachineState *machine)
>>  {
>>      ram_addr_t ram_size = machine->ram_size;
>> -    const char *cpu_model = machine->cpu_model;
>>      const char *kernel_filename = machine->kernel_filename;
>>      const char *kernel_cmdline = machine->kernel_cmdline;
>>      const char *initrd_filename = machine->initrd_filename;
>> -    ObjectClass *cpu_oc;
>> -    ARMCPU *cpu;
>> +    A9MPPrivState *mpcore;
>>      MemoryRegion *address_space_mem = get_system_memory();
>>      MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
>>      MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
>> @@ -119,30 +118,6 @@ static void zynq_init(MachineState *machine)
>>      Error *err = NULL;
>>      int n;
>>
>> -    if (!cpu_model) {
>> -        cpu_model = "cortex-a9";
>> -    }
>
> So this defeatures the cpu_model override. That's a good thing, but
> it's worthwhile to leave a check behind explaining to the user that
> the feature no longer exists:
>
> if (machine->cpu_model) {
>     error_report("Zynq does not support CPU model override!\n";
>     exit(1);
> }
>

Good idea, added!

>> -    cpu_oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model);
>> -
>> -    cpu = ARM_CPU(object_new(object_class_get_name(cpu_oc)));
>> -
>> -    object_property_set_int(OBJECT(cpu), ZYNQ_BOARD_MIDR, "midr", &err);
>> -    if (err) {
>> -        error_report("%s", error_get_pretty(err));
>> -        exit(1);
>> -    }
>> -
>> -    object_property_set_int(OBJECT(cpu), MPCORE_PERIPHBASE, "reset-cbar", 
>> &err);
>> -    if (err) {
>> -        error_report("%s", error_get_pretty(err));
>> -        exit(1);
>> -    }
>> -    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
>> -    if (err) {
>> -        error_report("%s", error_get_pretty(err));
>> -        exit(1);
>> -    }
>> -
>>      /* max 2GB ram */
>>      if (ram_size > 0x80000000) {
>>          ram_size = 0x80000000;
>> @@ -171,16 +146,38 @@ static void zynq_init(MachineState *machine)
>>      qdev_init_nofail(dev);
>>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8000000);
>>
>> -    dev = qdev_create(NULL, "a9mpcore_priv");
>> -    qdev_prop_set_uint32(dev, "num-cpu", 1);
>> -    qdev_init_nofail(dev);
>> -    busdev = SYS_BUS_DEVICE(dev);
>> +    mpcore = A9MPCORE_PRIV(object_new("a9mpcore_priv"));
>> +    object_property_set_int(OBJECT(mpcore), 1, "num-cpu",
>> +                            &err);
>> +    if (err) {
>> +        error_report("%s", error_get_pretty(err));
>> +        exit(1);
>> +    }
>> +    object_property_set_int(OBJECT(mpcore), ZYNQ_BOARD_MIDR, "midr",
>> +                            &err);
>> +    if (err) {
>> +        error_report("%s", error_get_pretty(err));
>> +        exit(1);
>> +    }
>> +    object_property_set_int(OBJECT(mpcore), MPCORE_PERIPHBASE,
>> +                            "reset-cbar", &err);
>> +    if (err) {
>> +        error_report("%s", error_get_pretty(err));
>> +        exit(1);
>> +    }
>> +    object_property_set_bool(OBJECT(mpcore), true, "realized", &err);
>> +    if (err != NULL) {
>> +        error_report("Couldn't realize the Zynq A9MPCore: %s",
>> +                     error_get_pretty(err));
>> +        exit(1);
>> +    }
>
> Can we just use the qdev_prop setters to cut down on the error boilerplate?
>

Yep, fixed

>> +    busdev = SYS_BUS_DEVICE(DEVICE(mpcore));
>>      sysbus_mmio_map(busdev, 0, MPCORE_PERIPHBASE);
>>      sysbus_connect_irq(busdev, 0,
>> -                       qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ));
>> +                       qdev_get_gpio_in(DEVICE(mpcore->cpu), ARM_CPU_IRQ));
>>
>
> Mpcore should now be responsible for connecting GIC to CPU. This
> should go away for board that use MPCore driven CPU instantiation.
>

Ok, done

Also, something here causes check-qtest-aarch64 to fail. Any ideas what
that would be/how to fix it?

> Regards,
> Peter
>
>>      for (n = 0; n < 64; n++) {
>> -        pic[n] = qdev_get_gpio_in(dev, n);
>> +        pic[n] = qdev_get_gpio_in(DEVICE(mpcore), n);
>>      }
>>
>>      zynq_init_spi_flashes(0xE0006000, pic[58-IRQ_OFFSET], false);
>> --
>> 1.7.1
>>
>>
>