Re: [Qemu-devel] [PATCH] QEMU: Add pflash support for versatile and integrator

[Peter Maydell <peter.maydell@linaro.org>]

On 17 March 2012 02:43, Marek Vasut <marek.vasut@gmail.com> wrote:
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>

This patch breaks previously working command lines -- you can't
make providing the flash binary blob mandatory.

> ---
>  hw/integratorcp.c |   26 ++++++++++++++++++++++++++
>  hw/versatilepb.c  |   24 ++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 0 deletions(-)
>
> diff --git a/hw/integratorcp.c b/hw/integratorcp.c
> index 5b06c81..2703ea6 100644
> --- a/hw/integratorcp.c
> +++ b/hw/integratorcp.c
> @@ -14,6 +14,9 @@
>  #include "net.h"
>  #include "exec-memory.h"
>  #include "sysemu.h"
> +#include "blockdev.h"
> +#include "exec-memory.h"
> +#include "flash.h"
>
>  typedef struct {
>     SysBusDevice busdev;
> @@ -451,6 +454,13 @@ static void integratorcp_init(ram_addr_t ram_size,
>     qemu_irq *cpu_pic;
>     DeviceState *dev;
>     int i;
> +    DriveInfo *dinfo;
> +
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    const int be = 1;
> +#else
> +    const int be = 0;
> +#endif

I know this is how other boards do it but it's pretty ugly. We could
do with something that defines to 0/1 rather than defined/undefined
to clean this up...

>     if (!cpu_model)
>         cpu_model = "arm926";
> @@ -469,6 +479,22 @@ static void integratorcp_init(ram_addr_t ram_size,
>     memory_region_init_alias(ram_alias, "ram.alias", ram, 0, ram_size);
>     memory_region_add_subregion(address_space_mem, 0x80000000, ram_alias);
>
> +    dinfo = drive_get(IF_PFLASH, 0, 0);
> +    if (!dinfo) {
> +        fprintf(stderr, "Flash image must be given with the "
> +                "'pflash' parameter\n");
> +        exit(1);
> +    }
> +
> +    if (!pflash_cfi01_register(0x24000000,
> +                               NULL, "integrator.flash0", 0x01000000,
> +                               dinfo->bdrv, 0x40000,
> +                               0x01000000 / 0x40000, 4, 0, 0, 0, 0,
> +                               be)) {
> +        fprintf(stderr, "qemu: Error registering flash memory.\n");
> +        exit(1);
> +    }
> +

This doesn't look right. You have the main flash device but none
of the boot code aliasing and you haven't correctly replaced the
current fake flash memory region (registered in integratorcm_init())
with an alias into the real flash device. See the board documentation:
http://infocenter.arm.com/help/topic/com.arm.doc.dui0159b/Chdjdabb.html
for more detail of what you should be modelling.

>     dev = qdev_create(NULL, "integrator_core");
>     qdev_prop_set_uint32(dev, "memsz", ram_size >> 20);
>     qdev_init_nofail(dev);
> diff --git a/hw/versatilepb.c b/hw/versatilepb.c
> index b9102f4..ad8429e 100644
> --- a/hw/versatilepb.c
> +++ b/hw/versatilepb.c
> @@ -17,6 +17,7 @@
>  #include "boards.h"
>  #include "blockdev.h"
>  #include "exec-memory.h"
> +#include "flash.h"
>
>  /* Primary interrupt controller.  */
>
> @@ -181,6 +182,13 @@ static void versatile_init(ram_addr_t ram_size,
>     NICInfo *nd;
>     int n;
>     int done_smc = 0;
> +    DriveInfo *dinfo;
> +
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    const int be = 1;
> +#else
> +    const int be = 0;
> +#endif
>
>     if (!cpu_model)
>         cpu_model = "arm926";
> @@ -195,6 +203,22 @@ static void versatile_init(ram_addr_t ram_size,
>     /* SDRAM at address zero.  */
>     memory_region_add_subregion(sysmem, 0, ram);
>
> +    dinfo = drive_get(IF_PFLASH, 0, 0);
> +    if (!dinfo) {
> +        fprintf(stderr, "Flash image must be given with the "
> +                "'pflash' parameter\n");
> +        exit(1);
> +    }
> +
> +    if (!pflash_cfi01_register(0x34000000,
> +                               NULL, "versatile.flash0", 0x8000000,
> +                               dinfo->bdrv, 0x40000,
> +                               0x8000000 / 0x40000, 4, 0, 0, 0, 0,
> +                               be)) {
> +        fprintf(stderr, "qemu: Error registering flash memory.\n");
> +        exit(1);
> +    }
> +

Should we support the flash remapping to address 0 on versatilepb
as well? "I want to boot from a ROM image in flash" seems like the
most obvious use case for providing the flash...

-- PMM