Re: [Qemu-devel] [PATCH 4/4 v4] block: Set proper device-width for vexpress flash

[Peter Maydell]

On 22 October 2013 17:35, Roy Franz <address@hidden> wrote:
> Create vexpress specific pflash registration
> function which properly configures the device-width
> of 16 bits (2 bytes) for the NOR flash on the
> vexpress platform.  This change is required for
> buffered flash writes to work properly.
>
> Signed-off-by: Roy Franz <address@hidden>
> ---
>  hw/arm/vexpress.c |   43 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index f48de00..8eae73c 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -480,6 +480,35 @@ static void vexpress_modify_dtb(const struct 
> arm_boot_info *info, void *fdt)
>      }
>  }
>
> +
> +/* Open code a private version of pflash registration since we
> + * need to set non-default device width for VExpress platform.
> + */
> +static pflash_t *ve_pflash_cfi01_register(hwaddr base, const char *name,
> +                                          BlockDriverState *bs)
> +{
> +    DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
> +
> +    if (bs && qdev_prop_set_drive(dev, "drive", bs)) {
> +        abort();
> +    }
> +
> +    qdev_prop_set_uint32(dev, "num-blocks", VEXPRESS_FLASH_SIZE / 
> VEXPRESS_FLASH_SECT_SIZE);

This line is too long and needs a linebreak.
(scripts/checkpatch.pl catches this kind of thing for you.)

> +    qdev_prop_set_uint64(dev, "sector-length", VEXPRESS_FLASH_SECT_SIZE);
> +    qdev_prop_set_uint8(dev, "width", 4);
> +    qdev_prop_set_uint8(dev, "device-width", 2);
> +    qdev_prop_set_uint8(dev, "big-endian", 0);
> +    qdev_prop_set_uint16(dev, "id0", 0x00);
> +    qdev_prop_set_uint16(dev, "id1", 0x89);
> +    qdev_prop_set_uint16(dev, "id2", 0x00);
> +    qdev_prop_set_uint16(dev, "id3", 0x18);
> +    qdev_prop_set_string(dev, "name", name);
> +    qdev_init_nofail(dev);
> +
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> +    return OBJECT_CHECK(pflash_t, (dev), "cfi.pflash01");
> +}
> +
>  static void vexpress_common_init(VEDBoardInfo *daughterboard,
>                                   QEMUMachineInitArgs *args)
>  {
> @@ -561,11 +590,8 @@ static void vexpress_common_init(VEDBoardInfo 
> *daughterboard,
>      sysbus_create_simple("pl111", map[VE_CLCD], pic[14]);
>
>      dinfo = drive_get_next(IF_PFLASH);
> -    pflash0 = pflash_cfi01_register(map[VE_NORFLASH0], NULL, 
> "vexpress.flash0",
> -            VEXPRESS_FLASH_SIZE, dinfo ? dinfo->bdrv : NULL,
> -            VEXPRESS_FLASH_SECT_SIZE,
> -            VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE, 4,
> -            0x00, 0x89, 0x00, 0x18, 0);
> +    pflash0 = ve_pflash_cfi01_register(map[VE_NORFLASH0], "vexpress.flash0",
> +                                       dinfo ? dinfo->bdrv : NULL);

You might as well fold the "is this NULL?" check into
the helper and just pass in dinfo.

>      if (!pflash0) {
>          fprintf(stderr, "vexpress: error registering flash 0.\n");
>          exit(1);
> @@ -580,11 +606,8 @@ static void vexpress_common_init(VEDBoardInfo 
> *daughterboard,
>      }
>
>      dinfo = drive_get_next(IF_PFLASH);
> -    if (!pflash_cfi01_register(map[VE_NORFLASH1], NULL, "vexpress.flash1",
> -            VEXPRESS_FLASH_SIZE, dinfo ? dinfo->bdrv : NULL,
> -            VEXPRESS_FLASH_SECT_SIZE,
> -            VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE, 4,
> -            0x00, 0x89, 0x00, 0x18, 0)) {
> +    if (!ve_pflash_cfi01_register(map[VE_NORFLASH1], "vexpress.flash1",
> +                                  dinfo ? dinfo->bdrv : NULL)) {
>          fprintf(stderr, "vexpress: error registering flash 1.\n");
>          exit(1);
>      }
> --
> 1.7.10.4

thanks
-- PMM