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

[Peter Crosthwaite]

On Mon, Nov 23, 2015 at 9:00 PM, Alistair Francis
<address@hidden> wrote:
> The Xilinx EP108 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 middle memory region, which is
> automatically created based on the size specified by the QEMU memory
> command line argument.
>

Is that the hardware reset value or is it usually uninitialised?
Although I'm guessing it is possible that in real HW even the low
region could be uninitialised (unmapped) too.

> Signed-off-by: Alistair Francis <address@hidden>
> ---
> Also, the Xilinx ZynqMP TRM has been released, if anyone is interested
> it is avalibale at:
> http://www.xilinx.com/products/silicon-devices/soc/zynq-ultrascale-mpsoc.html?resultsTablePreSelect=documenttype:User%20Guides#documentation
>

I couldn't find much here on how runtime programmable this is (is
there more than on pg282?). Are the mappings software controllable?

>  hw/arm/xlnx-ep108.c | 47 +++++++++++++++++++++++++++++++++++------------
>  1 file changed, 35 insertions(+), 12 deletions(-)
>
> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
> index 2899698..8c59d6d 100644
> --- a/hw/arm/xlnx-ep108.c
> +++ b/hw/arm/xlnx-ep108.c
> @@ -22,17 +22,22 @@
>
>  typedef struct XlnxEP108 {
>      XlnxZynqMPState soc;
> -    MemoryRegion ddr_ram;
> +    MemoryRegion ddr_ram_low;
> +    MemoryRegion ddr_ram_high;

Looking at pg282 of the doc, the external RAM (as known to boards) is
still only a singleton. So the RAM as instantiated here should still
only be the one (corresponding to a single DIMM slot?). The mapping of
that single RAM to multiple windows is a SoC (or DDRC) implemented
feature. So instead, the machine should create the RAM but not attach
it to the system memory. The RAM is then handed over to the SoC (MRs
are QOM objects so it can be a QOM link) which can then create aliases
into the single RAM for the windows. Those aliases are then in turn
added to the system memory map by the SoC. This avoid having to dup
this code when more boards happen and also prepares support for
runtime remapping of the memory locations (assuming that is
possible?).

Ideally this would all be managed by a DDRC peripheral, but just
getting it on the SoC level would be a good step.

>  } XlnxEP108;
>
> -/* Max 2GB RAM */
> -#define EP108_MAX_RAM_SIZE 0x80000000ull
> +/* Maximum size of the low memory region */
> +#define EP108_MAX_LOW_RAM_SIZE 0x80000000ull
> +/* Total maximum size of the EP108 memory */
> +#define EP108_MAX_RAM_SIZE     0x880000000ull

This feels odd. I think both LOW and HIGH should have MAX defined and
the overall MAX is addition of the two.

Regards,
Peter

> +#define EP108_HIGH_RAM_START   0x800000000ull
>
>  static struct arm_boot_info xlnx_ep108_binfo;
>
>  static void xlnx_ep108_init(MachineState *machine)
>  {
>      XlnxEP108 *s = g_new0(XlnxEP108, 1);
> +    ram_addr_t ddr_low_size, ddr_high_size;
>      Error *err = NULL;
>