Re: [Qemu-devel] [PATCH v3 1/1] xlnx-zynqmp: Add support for high DDR memory regions

[Peter Crosthwaite]

On Mon, Jan 4, 2016 at 12:47 PM, Alistair Francis
<address@hidden> wrote:
> The Xilinx ZynqMP SoC and EP108 board supports three memory regions:
>  - A 2GB region starting at 0
>  - A 32GB region starting at 32GB
>  - A 256GB region starting at 768GB
>
> This patch adds support for the first two memory regions, which is
> automatically created based on the size specified by the QEMU memory
> command line argument.
>
> On hardware the physical memory region is one continuous region, it is then
> mapped into the three different regions by the DDRC. As we don't model the
> DDRC this is done at startup by QEMU. The board creates the memory region and
> then passes that memory region to the SoC. The SoC then maps the memory
> regions.
>
> Signed-off-by: Alistair Francis <address@hidden>
> ---
> V3:
>  - Assert on the RAM sizes
>  - Remove ram_size property
>  - General fixes
> V2:
>  - Create one continuous memory region and pass it to the SoC
>
> Also, the Xilinx ZynqMP TRM is avaliable at:
> http://www.xilinx.com/products/silicon-devices/soc/zynq-ultrascale-mpsoc.html?resultsTablePreSelect=documenttype:User%20Guides#documentation
>
>  hw/arm/xlnx-ep108.c          | 38 ++++++++++++++++++++------------------
>  hw/arm/xlnx-zynqmp.c         | 36 ++++++++++++++++++++++++++++++++++++
>  include/hw/arm/xlnx-zynqmp.h | 13 +++++++++++++
>  3 files changed, 69 insertions(+), 18 deletions(-)
>
> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
> index 85b978f..d55663b 100644
> --- a/hw/arm/xlnx-ep108.c
> +++ b/hw/arm/xlnx-ep108.c
> @@ -25,9 +25,6 @@ typedef struct XlnxEP108 {
>      MemoryRegion ddr_ram;
>  } XlnxEP108;
>
> -/* Max 2GB RAM */
> -#define EP108_MAX_RAM_SIZE 0x80000000ull
> -
>  static struct arm_boot_info xlnx_ep108_binfo;
>
>  static void xlnx_ep108_init(MachineState *machine)
> @@ -35,20 +32,12 @@ static void xlnx_ep108_init(MachineState *machine)
>      XlnxEP108 *s = g_new0(XlnxEP108, 1);
>      Error *err = NULL;
>
> -    object_initialize(&s->soc, sizeof(s->soc), TYPE_XLNX_ZYNQMP);
> -    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> -                              &error_abort);
> -
> -    object_property_set_bool(OBJECT(&s->soc), true, "realized", &err);
> -    if (err) {
> -        error_report("%s", error_get_pretty(err));
> -        exit(1);
> -    }
> -
> -    if (machine->ram_size > EP108_MAX_RAM_SIZE) {
> +    /* Create the memory region to pass to the SoC */
> +    if (machine->ram_size > XLNX_ZYNQMP_MAX_RAM_SIZE) {
>          error_report("WARNING: RAM size " RAM_ADDR_FMT " above max 
> supported, "
> -                     "reduced to %llx", machine->ram_size, 
> EP108_MAX_RAM_SIZE);
> -        machine->ram_size = EP108_MAX_RAM_SIZE;
> +                     "reduced to %llx", machine->ram_size,
> +                     XLNX_ZYNQMP_MAX_RAM_SIZE);
> +        machine->ram_size = XLNX_ZYNQMP_MAX_RAM_SIZE;
>      }
>
>      if (machine->ram_size < 0x08000000) {
> @@ -56,9 +45,22 @@ static void xlnx_ep108_init(MachineState *machine)
>                   machine->ram_size);
>      }
>
> -    memory_region_allocate_system_memory(&s->ddr_ram, NULL, "ddr-ram",
> +    memory_region_allocate_system_memory(&s->ddr_ram, NULL,
> +                                         "ddr-ram",

Whitespace change unneeded.

>                                           machine->ram_size);
> -    memory_region_add_subregion(get_system_memory(), 0, &s->ddr_ram);
> +
> +    object_initialize(&s->soc, sizeof(s->soc), TYPE_XLNX_ZYNQMP);
> +    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> +                              &error_abort);
> +
> +    object_property_set_link(OBJECT(&s->soc), OBJECT(&s->ddr_ram),
> +                         "ddr-ram", &error_abort);
> +
> +    object_property_set_bool(OBJECT(&s->soc), true, "realized", &err);
> +    if (err) {
> +        error_report("%s", error_get_pretty(err));
> +        exit(1);
> +    }
>
>      xlnx_ep108_binfo.ram_size = machine->ram_size;
>      xlnx_ep108_binfo.kernel_filename = machine->kernel_filename;
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 87553bb..e749fd0 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -90,6 +90,11 @@ static void xlnx_zynqmp_init(Object *obj)
>                                    &error_abort);
>      }
>
> +    object_property_add_link(obj, "ddr-ram", TYPE_MEMORY_REGION,
> +                             (Object **)&s->ddr_ram,
> +                             qdev_prop_allow_set_link_before_realize,
> +                             OBJ_PROP_LINK_UNREF_ON_RELEASE, &error_abort);
> +
>      object_initialize(&s->gic, sizeof(s->gic), TYPE_ARM_GIC);
>      qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
>
> @@ -120,9 +125,40 @@ 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]";
> +    ram_addr_t ddr_low_size, ddr_high_size;
>      qemu_irq gic_spi[GIC_NUM_SPI_INTR];
>      Error *err = NULL;
>
> +    s->ram_size = memory_region_size(s->ddr_ram);

ram_size variable should be localised.

> +
> +    /* Create the DDR Memory Regions. User friendly checks shoulud happen at

"should"

> +     * the board level
> +     */
> +    if (s->ram_size > XLNX_ZYNQMP_MAX_LOW_RAM_SIZE) {
> +        /* The RAM size is above the maximum available for the low DDR.
> +         * Create the high DDR memory region as well.
> +         */
> +        assert(s->ram_size <= XLNX_ZYNQMP_MAX_RAM_SIZE);
> +        ddr_low_size = XLNX_ZYNQMP_MAX_LOW_RAM_SIZE;
> +        ddr_high_size = s->ram_size - XLNX_ZYNQMP_MAX_LOW_RAM_SIZE;
> +
> +        memory_region_init_alias(&s->ddr_ram_high, NULL,
> +                                 "ddr-ram-high", s->ddr_ram,
> +                                  ddr_low_size, ddr_high_size);
> +        memory_region_add_subregion(get_system_memory(),
> +                                    XLNX_ZYNQMP_HIGH_RAM_START,
> +                                    &s->ddr_ram_high);
> +    } else {
> +        /* RAM must be non-zero */
> +        assert(s->ram_size);
> +        ddr_low_size = s->ram_size;
> +    }
> +
> +    memory_region_init_alias(&s->ddr_ram_low, NULL,
> +                             "ddr-ram-low", s->ddr_ram,
> +                              0, ddr_low_size);
> +    memory_region_add_subregion(get_system_memory(), 0, &s->ddr_ram_low);
> +
>      /* Create the four OCM banks */
>      for (i = 0; i < XLNX_ZYNQMP_NUM_OCM_BANKS; i++) {
>          char *ocm_name = g_strdup_printf("zynqmp.ocm_ram_bank_%d", i);
> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> index d116092..aaea0d8 100644
> --- a/include/hw/arm/xlnx-zynqmp.h
> +++ b/include/hw/arm/xlnx-zynqmp.h
> @@ -51,6 +51,14 @@
>  #define XLNX_ZYNQMP_GIC_REGION_SIZE 0x1000
>  #define XLNX_ZYNQMP_GIC_ALIASES     (0x10000 / XLNX_ZYNQMP_GIC_REGION_SIZE - 
> 1)
>
> +#define XLNX_ZYNQMP_MAX_LOW_RAM_SIZE    0x80000000ull
> +
> +#define XLNX_ZYNQMP_MAX_HIGH_RAM_SIZE   0x800000000ull
> +#define XLNX_ZYNQMP_HIGH_RAM_START      0x800000000ull
> +
> +#define XLNX_ZYNQMP_MAX_RAM_SIZE (XLNX_ZYNQMP_MAX_LOW_RAM_SIZE + \
> +                                  XLNX_ZYNQMP_MAX_HIGH_RAM_SIZE)
> +
>  typedef struct XlnxZynqMPState {
>      /*< private >*/
>      DeviceState parent_obj;
> @@ -60,8 +68,13 @@ 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];
>
> +    uint64 ram_size;

This field should be deleted after localisation of ram_size in realize().

Otherwise

Reviewed-by: Peter Crosthwaite <address@hidden>

Regards,
Peter

> +    MemoryRegion *ddr_ram;
> +    MemoryRegion ddr_ram_low, ddr_ram_high;
> +
>      CadenceGEMState gem[XLNX_ZYNQMP_NUM_GEMS];
>      CadenceUARTState uart[XLNX_ZYNQMP_NUM_UARTS];
>      SysbusAHCIState sata;
> --
> 2.5.0
>