Re: [Qemu-devel] [PATCH v1 1/1] xlnx-zynqmp: Connect the four OCM banks

[Alistair Francis]

On Sat, Jul 11, 2015 at 1:40 AM, Peter Crosthwaite
<address@hidden> wrote:
> On Fri, Jul 10, 2015 at 11:11 AM, Alistair Francis
> <address@hidden> wrote:
>> The Xilinx EP108 has four separate OCM banks which are located
>> adjacent to each other. This patch adds the four banks to
>> the ZynqMP SoC.
>>
>> Signed-off-by: Alistair Francis <address@hidden>
>> ---
>>
>>  hw/arm/xlnx-zynqmp.c         |   13 +++++++++++++
>>  include/hw/arm/xlnx-zynqmp.h |    6 ++++++
>>  2 files changed, 19 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
>> index 5e72078..a8edff0 100644
>> --- a/hw/arm/xlnx-zynqmp.c
>> +++ b/hw/arm/xlnx-zynqmp.c
>> @@ -98,9 +98,22 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error 
>> **errp)
>>      MemoryRegion *system_memory = get_system_memory();
>>      uint8_t i;
>>      const char *boot_cpu = s->boot_cpu ? s->boot_cpu : "apu-cpu[0]";
>> +    char ocm_name[25];
>>      qemu_irq gic_spi[GIC_NUM_SPI_INTR];
>>      Error *err = NULL;
>>
>> +    /* Create the four OCM banks */
>> +    for (i = 0; i < XLNX_ZYNQMP_NUM_OCM_BANKS; i++) {
>
> ocm_name can be localised to this scope.

Ok, I wasn't sure if that was against the style guide, but I prefer
it in the loop as well.

>
>> +        sprintf(ocm_name, "zynqmp.ocm_ram_bank_%d", i);
>
> g_strdup_printf remove need to statically size the string (making the
> code more patchable).

Thanks, I was thinking there must be something better.

Thanks,

Alistair

>
> Regards,
> Peter
>
>> +        memory_region_init_ram(&s->ocm_ram[i], NULL, ocm_name,
>> +                               XLNX_ZYNQMP_OCM_RAM_SIZE, &error_abort);
>> +        vmstate_register_ram_global(&s->ocm_ram[i]);
>> +        memory_region_add_subregion(get_system_memory(),
>> +                                    XLNX_ZYNQMP_OCM_RAM_0_ADDRESS +
>> +                                        i * XLNX_ZYNQMP_OCM_RAM_SIZE,
>> +                                    &s->ocm_ram[i]);
>> +    }
>> +
>>      qdev_prop_set_uint32(DEVICE(&s->gic), "num-irq", GIC_NUM_SPI_INTR + 32);
>>      qdev_prop_set_uint32(DEVICE(&s->gic), "revision", 2);
>>      qdev_prop_set_uint32(DEVICE(&s->gic), "num-cpu", 
>> XLNX_ZYNQMP_NUM_APU_CPUS);
>> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
>> index c379632..6ccb57b 100644
>> --- a/include/hw/arm/xlnx-zynqmp.h
>> +++ b/include/hw/arm/xlnx-zynqmp.h
>> @@ -32,6 +32,10 @@
>>  #define XLNX_ZYNQMP_NUM_GEMS 4
>>  #define XLNX_ZYNQMP_NUM_UARTS 2
>>
>> +#define XLNX_ZYNQMP_NUM_OCM_BANKS 4
>> +#define XLNX_ZYNQMP_OCM_RAM_0_ADDRESS 0xFFFC0000
>> +#define XLNX_ZYNQMP_OCM_RAM_SIZE 0x10000
>> +
>>  #define XLNX_ZYNQMP_GIC_REGIONS 2
>>
>>  /* ZynqMP maps the ARM GIC regions (GICC, GICD ...) at consecutive 64k 
>> offsets
>> @@ -52,6 +56,8 @@ typedef struct XlnxZynqMPState {
>>      ARMCPU rpu_cpu[XLNX_ZYNQMP_NUM_RPU_CPUS];
>>      GICState gic;
>>      MemoryRegion gic_mr[XLNX_ZYNQMP_GIC_REGIONS][XLNX_ZYNQMP_GIC_ALIASES];
>> +    MemoryRegion ocm_ram[XLNX_ZYNQMP_NUM_OCM_BANKS];
>> +
>>      CadenceGEMState gem[XLNX_ZYNQMP_NUM_GEMS];
>>      CadenceUARTState uart[XLNX_ZYNQMP_NUM_UARTS];
>>
>> --
>> 1.7.1
>>
>>
>