Re: [Qemu-devel] [PATCH v12 17/19] i.MX: Add the i.MX25 PDK plateform

[Peter Maydell]

On 11 July 2015 at 00:31, Jean-Christophe Dubois <address@hidden> wrote:
> Tested by booting a minimal Linux system on the emulated platform
> Tested by booting the Xvisor hyprvisor on the emulated platform

Typo in subject: "platform".

This commit message could probably use a little elaboration.
What is a PDK board?
> +    /* 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_IMX25_SDRAM0_ADDR, FSL_IMX25_SDRAM0_SIZE },
> +            { FSL_IMX25_SDRAM1_ADDR, FSL_IMX25_SDRAM1_SIZE },
> +        };
> +
> +        if (ram_size > ram[i].size) {
> +            size = ram[i].size;
> +        } else {
> +            size = ram_size;
> +        }
> +
> +        sprintf(ram_name, "imx25.ram%d", i);
> +
> +        ram_size -= size;
> +
> +        memory_region_allocate_system_memory(&s->ram[i], NULL, ram_name, 
> size);

This has the same 'calls this function twice' issues as the other patch.

> +        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);
> +        }
> +    }
> +
> +    imx25_pdk_binfo.ram_size = machine->ram_size;
> +    imx25_pdk_binfo.kernel_filename = machine->kernel_filename;
> +    imx25_pdk_binfo.kernel_cmdline = machine->kernel_cmdline;
> +    imx25_pdk_binfo.initrd_filename = machine->initrd_filename;
> +    imx25_pdk_binfo.loader_start = IMX25_PDK_ADDRESS;
> +    imx25_pdk_binfo.board_id = 1771,
> +    imx25_pdk_binfo.nb_cpus = 1;
> +
> +    /*
> +     * We test explicitly for qtest here as it is not done (yet?) in
> +     * arm_load_kernel(). Without this the "make check" command would
> +     * fail.
> +     */
> +    if (!qtest_enabled()) {
> +        arm_load_kernel(&s->soc.cpu, &imx25_pdk_binfo);
> +    } else {
> +        /*
> +         * This I2C device doesn't exist on the real board.
> +         * We add it here (only on qtest usage) to be able to do a bit
> +         * of simple qtest. See "make check" for details.
> +         */
> +        i2c_create_slave((I2CBus *)qdev_get_child_bus(DEVICE(&s->soc.i2c[0]),
> +                                                      "i2c"),
> +                         "ds1338", 0x68);
> +    }

This is kind of ugly. It's a shame you can't add an I2C device
via -device...

thanks
-- PMM