Re: [Qemu-devel] [PATCH v13 13/19] i.MX: KZM now uses the standalone i.MX31 SOC support

[Peter Maydell]

On 16 July 2015 at 22:21, Jean-Christophe Dubois <address@hidden> wrote:
> Tested by booting a minimal Linux system on the emulated platform
>
> Signed-off-by: Jean-Christophe Dubois <address@hidden>
> ---

This said:

> -     * 0x80000000-0x87ffffff RAM                  EMULATED
> -     * 0x88000000-0x8fffffff RAM Aliasing         EMULATED

but your patch changes it:

> + * 0x00000000-0x7fffffff See i.MX31 SOC for support
> + * 0x80000000-0x8fffffff RAM                  EMULATED
> + * 0x90000000-0x9fffffff RAM                  EMULATED

> +    /* initialize our memory */
> +    for (i=0, ram_size = machine->ram_size; (i<2) && ram_size; i++) {
> +        unsigned int size;
> +        char ram_name[20];
> +        static const struct {
> +            hwaddr addr;
> +            unsigned int size;
> +        } ram[2] = {
> +            { FSL_IMX31_SDRAM0_ADDR, FSL_IMX31_SDRAM0_SIZE },
> +            { FSL_IMX31_SDRAM1_ADDR, FSL_IMX31_SDRAM1_SIZE },
> +        };
> +
> +       if (ram_size > ram[i].size) {
> +            size = ram[i].size;
> +       } else {
> +            size = ram_size;
> +       }
> +
> +        sprintf(ram_name, "kzm.ram%d", i);
> +
> +        ram_size -= size;
> +
> +        memory_region_allocate_system_memory(&s->ram[i], NULL, ram_name, 
> size);

memory_region_allocate_system_memory() needs to be called once and
only once by a board init. You're going to end up calling it twice here.

> +        memory_region_add_subregion(get_system_memory(), ram[i].addr,
> +                                    &s->ram[i]);
> +        if (size < ram[i].size) {
> +            memory_region_init_alias(&s->ram_alias, NULL, "ram.alias",
> +                                     &s->ram[i], 0, ram[i].size - size);
> +            memory_region_add_subregion(get_system_memory(),
> +                                        ram[i].addr + size, &s->ram_alias);
> +        }
> +    }

What is this code trying to do with that alias? It looks very odd.
(Are we trying to model under-decoded address bits?)

My instinct is to say that rather than modelling two separate lumps
of RAM we should just have one region -- they're contiguous, after
all.

thanks
-- PMM