Patchwork [v1,3/7] pflash_cfi0x: QOMified

login
register
mail settings
Submitter Peter Crosthwaite
Date Oct. 19, 2012, 6:40 a.m.
Message ID <ddebf66ebc765be0adc04fd6a5b979f543ed482a.1350619775.git.peter.crosthwaite@xilinx.com>
Download mbox | patch
Permalink /patch/192572/
State New
Headers show

Comments

Peter Crosthwaite - Oct. 19, 2012, 6:40 a.m.
QOMified the pflash_cfi0x so machine models can connect them up in custom ways.

Kept the pflash_cfi0x_register functions as is. They can still be used to
create a flash straight onto system memory.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/pflash_cfi01.c |  142 +++++++++++++++++++++++++++++++++++++------------
 hw/pflash_cfi02.c |  154 ++++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 224 insertions(+), 72 deletions(-)
Paolo Bonzini - Oct. 19, 2012, 10:08 a.m.
bdrvIl 19/10/2012 08:40, Peter Crosthwaite ha scritto:
> QOMified the pflash_cfi0x so machine models can connect them up in custom ways.
> 
> Kept the pflash_cfi0x_register functions as is. They can still be used to
> create a flash straight onto system memory.
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Great, thanks!

Can you just name the BlockDriverState property "drive" for consistency
with other device models?

Also:

>  struct pflash_t {
> +    SysBusDevice busdev;
>      BlockDriverState *bs;
> -    target_phys_addr_t sector_len;
> -    int width;
> +    uint32_t nb_blocs;
> +    /* FIXME: get rid of target_phys_addr_t usage */
> +    union {
> +        target_phys_addr_t sector_len;
> +        uint32_t sector_len_u32;

Isn't this broken on big-endian machines?  You can just make it a
uint64_t, since that's what target_phys_addr_t is now.

Then you can change it to uint32_t when you have more time.

Paolo

> +    };
> +    uint8_t width;
> +    uint8_t be;
>      int wcycle; /* if 0, the flash is read normally */
>      int bypass;
>      int ro;
>      uint8_t cmd;
>      uint8_t status;
> -    uint16_t ident[4];
> +    union {
> +        uint16_t ident[4];
> +        struct {
> +            uint16_t ident0;
> +            uint16_t ident1;
> +            uint16_t ident2;
> +            uint16_t ident3;
> +        };
> +    };
>      uint8_t cfi_len;
>      uint8_t cfi_table[0x52];
>      target_phys_addr_t counter;
>      unsigned int writeblock_size;
>      QEMUTimer *timer;
>      MemoryRegion mem;
> +    char *name;
>      void *storage;
>  };
>  
> @@ -541,19 +558,13 @@ static const MemoryRegionOps pflash_cfi01_ops_le = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -pflash_t *pflash_cfi01_register(target_phys_addr_t base,
> -                                DeviceState *qdev, const char *name,
> -                                target_phys_addr_t size,
> -                                BlockDriverState *bs, uint32_t sector_len,
> -                                int nb_blocs, int width,
> -                                uint16_t id0, uint16_t id1,
> -                                uint16_t id2, uint16_t id3, int be)
> +static int pflash_cfi01_init(SysBusDevice *dev)
>  {
> -    pflash_t *pfl;
> +    pflash_t *pfl = FROM_SYSBUS(typeof(*pfl), dev);
>      target_phys_addr_t total_len;
>      int ret;
>  
> -    total_len = sector_len * nb_blocs;
> +    total_len = pfl->sector_len * pfl->nb_blocs;
>  
>      /* XXX: to be fixed */
>  #if 0
> @@ -562,27 +573,26 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base,
>          return NULL;
>  #endif
>  
> -    pfl = g_malloc0(sizeof(pflash_t));
> -
> +    if (!pfl->name) {
> +        static int next;
> +        pfl->name = g_strdup_printf("pflash.cfi01.%d", next++);
> +    }
>      memory_region_init_rom_device(
> -        &pfl->mem, be ? &pflash_cfi01_ops_be : &pflash_cfi01_ops_le, pfl,
> -        name, size);
> -    vmstate_register_ram(&pfl->mem, qdev);
> +        &pfl->mem, pfl->be ? &pflash_cfi01_ops_be : &pflash_cfi01_ops_le, pfl,
> +        pfl->name, total_len);
> +    vmstate_register_ram(&pfl->mem, DEVICE(pfl));
>      pfl->storage = memory_region_get_ram_ptr(&pfl->mem);
> -    memory_region_add_subregion(get_system_memory(), base, &pfl->mem);
> +    sysbus_init_mmio(dev, &pfl->mem);
>  
> -    pfl->bs = bs;
>      if (pfl->bs) {
>          /* read the initial flash content */
>          ret = bdrv_read(pfl->bs, 0, pfl->storage, total_len >> 9);
> +
>          if (ret < 0) {
> -            memory_region_del_subregion(get_system_memory(), &pfl->mem);
> -            vmstate_unregister_ram(&pfl->mem, qdev);
> +            vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
>              memory_region_destroy(&pfl->mem);
> -            g_free(pfl);
> -            return NULL;
> +            return 1;
>          }
> -        bdrv_attach_dev_nofail(pfl->bs, pfl);
>      }
>  
>      if (pfl->bs) {
> @@ -592,15 +602,9 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base,
>      }
>  
>      pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
> -    pfl->sector_len = sector_len;
> -    pfl->width = width;
>      pfl->wcycle = 0;
>      pfl->cmd = 0;
>      pfl->status = 0;
> -    pfl->ident[0] = id0;
> -    pfl->ident[1] = id1;
> -    pfl->ident[2] = id2;
> -    pfl->ident[3] = id3;
>      /* Hardcoded CFI table */
>      pfl->cfi_len = 0x52;
>      /* Standard "QRY" string */
> @@ -649,7 +653,7 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base,
>      pfl->cfi_table[0x28] = 0x02;
>      pfl->cfi_table[0x29] = 0x00;
>      /* Max number of bytes in multi-bytes write */
> -    if (width == 1) {
> +    if (pfl->width == 1) {
>          pfl->cfi_table[0x2A] = 0x08;
>      } else {
>          pfl->cfi_table[0x2A] = 0x0B;
> @@ -660,10 +664,10 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base,
>      /* Number of erase block regions (uniform) */
>      pfl->cfi_table[0x2C] = 0x01;
>      /* Erase block region 1 */
> -    pfl->cfi_table[0x2D] = nb_blocs - 1;
> -    pfl->cfi_table[0x2E] = (nb_blocs - 1) >> 8;
> -    pfl->cfi_table[0x2F] = sector_len >> 8;
> -    pfl->cfi_table[0x30] = sector_len >> 16;
> +    pfl->cfi_table[0x2D] = pfl->nb_blocs - 1;
> +    pfl->cfi_table[0x2E] = (pfl->nb_blocs - 1) >> 8;
> +    pfl->cfi_table[0x2F] = pfl->sector_len >> 8;
> +    pfl->cfi_table[0x30] = pfl->sector_len >> 16;
>  
>      /* Extended */
>      pfl->cfi_table[0x31] = 'P';
> @@ -685,6 +689,74 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base,
>  
>      pfl->cfi_table[0x3f] = 0x01; /* Number of protection fields */
>  
> +    return 0;
> +}
> +
> +static Property pflash_cfi01_properties[] = {
> +    DEFINE_PROP_DRIVE("bdrv", struct pflash_t, bs),
> +    DEFINE_PROP_UINT32("nb_blocs", struct pflash_t, nb_blocs, 0),
> +    DEFINE_PROP_UINT32("sector_len", struct pflash_t, sector_len_u32, 0),
> +    DEFINE_PROP_UINT8("width", struct pflash_t, width, 0),
> +    DEFINE_PROP_UINT8("be", struct pflash_t, be, 0),
> +    DEFINE_PROP_UINT16("id0", struct pflash_t, ident0, 0),
> +    DEFINE_PROP_UINT16("id1", struct pflash_t, ident1, 0),
> +    DEFINE_PROP_UINT16("id2", struct pflash_t, ident2, 0),
> +    DEFINE_PROP_UINT16("id3", struct pflash_t, ident3, 0),
> +    DEFINE_PROP_STRING("name", struct pflash_t, name),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void pflash_cfi01_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    k->init = pflash_cfi01_init;
> +    dc->props = pflash_cfi01_properties;
> +}
> +
> +
> +static const TypeInfo pflash_cfi01_info = {
> +    .name           = "cfi.pflash01",
> +    .parent         = TYPE_SYS_BUS_DEVICE,
> +    .instance_size  = sizeof(struct pflash_t),
> +    .class_init     = pflash_cfi01_class_init,
> +};
> +
> +static void pflash_cfi01_register_types(void)
> +{
> +    type_register_static(&pflash_cfi01_info);
> +}
> +
> +type_init(pflash_cfi01_register_types)
> +
> +pflash_t *pflash_cfi01_register(target_phys_addr_t base,
> +                                DeviceState *qdev, const char *name,
> +                                target_phys_addr_t size,
> +                                BlockDriverState *bs,
> +                                uint32_t sector_len, int nb_blocs, int width,
> +                                uint16_t id0, uint16_t id1,
> +                                uint16_t id2, uint16_t id3, int be)
> +{
> +    DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
> +    SysBusDevice *busdev = sysbus_from_qdev(dev);
> +    pflash_t *pfl = (pflash_t *)object_dynamic_cast(OBJECT(dev),
> +                                                    "cfi.pflash01");
> +
> +    if (bs && qdev_prop_set_drive(dev, "bdrv", bs)) {
> +        abort();
> +    }
> +    qdev_prop_set_uint32(dev, "nb_blocs", nb_blocs);
> +    qdev_prop_set_uint32(dev, "sector_len", sector_len);
> +    qdev_prop_set_uint8(dev, "width", width);
> +    qdev_prop_set_uint8(dev, "be", !!be);
> +    qdev_prop_set_uint16(dev, "id0", id0);
> +    qdev_prop_set_uint16(dev, "id1", id1);
> +    qdev_prop_set_uint16(dev, "id2", id2);
> +    qdev_prop_set_uint16(dev, "id3", id3);
> +    qdev_init_nofail(dev);
> +
> +    sysbus_mmio_map(busdev, 0, base);
>      return pfl;
>  }
>  
> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
> index 43fb3a4..db05fe6 100644
> --- a/hw/pflash_cfi02.c
> +++ b/hw/pflash_cfi02.c
> @@ -41,6 +41,7 @@
>  #include "block.h"
>  #include "exec-memory.h"
>  #include "host-utils.h"
> +#include "sysbus.h"
>  
>  //#define PFLASH_DEBUG
>  #ifdef PFLASH_DEBUG
> @@ -55,18 +56,36 @@ do {                                               \
>  #define PFLASH_LAZY_ROMD_THRESHOLD 42
>  
>  struct pflash_t {
> +    SysBusDevice busdev;
>      BlockDriverState *bs;
>      uint32_t sector_len;
> +    uint32_t nb_blocs;
>      uint32_t chip_len;
> -    int mappings;
> -    int width;
> +    uint8_t mappings;
> +    uint8_t width;
> +    uint8_t be;
>      int wcycle; /* if 0, the flash is read normally */
>      int bypass;
>      int ro;
>      uint8_t cmd;
>      uint8_t status;
> -    uint16_t ident[4];
> -    uint16_t unlock_addr[2];
> +    /* FIXME: implement array device properties */
> +    union {
> +        uint16_t ident[4];
> +        struct {
> +            uint16_t ident0;
> +            uint16_t ident1;
> +            uint16_t ident2;
> +            uint16_t ident3;
> +        };
> +    };
> +    union {
> +        uint16_t unlock_addr[2];
> +        struct {
> +            uint16_t unlock_addr0;
> +            uint16_t unlock_addr1;
> +        };
> +    };
>      uint8_t cfi_len;
>      uint8_t cfi_table[0x52];
>      QEMUTimer *timer;
> @@ -79,6 +98,7 @@ struct pflash_t {
>      MemoryRegion orig_mem;
>      int rom_mode;
>      int read_counter; /* used for lazy switch-back to rom mode */
> +    char *name;
>      void *storage;
>  };
>  
> @@ -574,49 +594,42 @@ static const MemoryRegionOps pflash_cfi02_ops_le = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -pflash_t *pflash_cfi02_register(target_phys_addr_t base,
> -                                DeviceState *qdev, const char *name,
> -                                target_phys_addr_t size,
> -                                BlockDriverState *bs, uint32_t sector_len,
> -                                int nb_blocs, int nb_mappings, int width,
> -                                uint16_t id0, uint16_t id1,
> -                                uint16_t id2, uint16_t id3,
> -                                uint16_t unlock_addr0, uint16_t unlock_addr1,
> -                                int be)
> +static int pflash_cfi02_init(SysBusDevice *dev)
>  {
> -    pflash_t *pfl;
> +    pflash_t *pfl = FROM_SYSBUS(typeof(*pfl), dev);
>      int32_t chip_len;
>      int ret;
>  
> -    chip_len = sector_len * nb_blocs;
> +    chip_len = pfl->sector_len * pfl->nb_blocs;
>      /* XXX: to be fixed */
>  #if 0
>      if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) &&
>          total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024))
>          return NULL;
>  #endif
> -    pfl = g_malloc0(sizeof(pflash_t));
> -    memory_region_init_rom_device(
> -        &pfl->orig_mem, be ? &pflash_cfi02_ops_be : &pflash_cfi02_ops_le, pfl,
> -        name, size);
> -    vmstate_register_ram(&pfl->orig_mem, qdev);
> +
> +    if (!pfl->name) {
> +        static int next;
> +        pfl->name = g_strdup_printf("pflash.cfi01.%d", next++);
> +    }
> +    memory_region_init_rom_device(&pfl->orig_mem, pfl->be ?
> +                                  &pflash_cfi02_ops_be : &pflash_cfi02_ops_le,
> +                                  pfl, pfl->name, chip_len);
> +    vmstate_register_ram(&pfl->orig_mem, DEVICE(pfl));
>      pfl->storage = memory_region_get_ram_ptr(&pfl->orig_mem);
>      pfl->chip_len = chip_len;
> -    pfl->mappings = nb_mappings;
> -    pfl->bs = bs;
>      if (pfl->bs) {
>          /* read the initial flash content */
>          ret = bdrv_read(pfl->bs, 0, pfl->storage, chip_len >> 9);
>          if (ret < 0) {
>              g_free(pfl);
> -            return NULL;
> +            return 1;
>          }
> -        bdrv_attach_dev_nofail(pfl->bs, pfl);
>      }
>  
>      pflash_setup_mappings(pfl);
>      pfl->rom_mode = 1;
> -    memory_region_add_subregion(get_system_memory(), base, &pfl->mem);
> +    sysbus_init_mmio(dev, &pfl->mem);
>  
>      if (pfl->bs) {
>          pfl->ro = bdrv_is_read_only(pfl->bs);
> @@ -625,17 +638,9 @@ pflash_t *pflash_cfi02_register(target_phys_addr_t base,
>      }
>  
>      pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
> -    pfl->sector_len = sector_len;
> -    pfl->width = width;
>      pfl->wcycle = 0;
>      pfl->cmd = 0;
>      pfl->status = 0;
> -    pfl->ident[0] = id0;
> -    pfl->ident[1] = id1;
> -    pfl->ident[2] = id2;
> -    pfl->ident[3] = id3;
> -    pfl->unlock_addr[0] = unlock_addr0;
> -    pfl->unlock_addr[1] = unlock_addr1;
>      /* Hardcoded CFI table (mostly from SG29 Spansion flash) */
>      pfl->cfi_len = 0x52;
>      /* Standard "QRY" string */
> @@ -691,10 +696,10 @@ pflash_t *pflash_cfi02_register(target_phys_addr_t base,
>      /* Number of erase block regions (uniform) */
>      pfl->cfi_table[0x2C] = 0x01;
>      /* Erase block region 1 */
> -    pfl->cfi_table[0x2D] = nb_blocs - 1;
> -    pfl->cfi_table[0x2E] = (nb_blocs - 1) >> 8;
> -    pfl->cfi_table[0x2F] = sector_len >> 8;
> -    pfl->cfi_table[0x30] = sector_len >> 16;
> +    pfl->cfi_table[0x2D] = pfl->nb_blocs - 1;
> +    pfl->cfi_table[0x2E] = (pfl->nb_blocs - 1) >> 8;
> +    pfl->cfi_table[0x2F] = pfl->sector_len >> 8;
> +    pfl->cfi_table[0x30] = pfl->sector_len >> 16;
>  
>      /* Extended */
>      pfl->cfi_table[0x31] = 'P';
> @@ -714,5 +719,80 @@ pflash_t *pflash_cfi02_register(target_phys_addr_t base,
>      pfl->cfi_table[0x3b] = 0x00;
>      pfl->cfi_table[0x3c] = 0x00;
>  
> +    return 0;
> +}
> +
> +static Property pflash_cfi02_properties[] = {
> +    DEFINE_PROP_DRIVE("bdrv", struct pflash_t, bs),
> +    DEFINE_PROP_UINT32("nb_blocs", struct pflash_t, nb_blocs, 0),
> +    DEFINE_PROP_UINT32("sector_len", struct pflash_t, sector_len, 0),
> +    DEFINE_PROP_UINT8("width", struct pflash_t, width, 0),
> +    DEFINE_PROP_UINT8("mappings", struct pflash_t, mappings, 0),
> +    DEFINE_PROP_UINT8("be", struct pflash_t, be, 0),
> +    DEFINE_PROP_UINT16("id0", struct pflash_t, ident0, 0),
> +    DEFINE_PROP_UINT16("id1", struct pflash_t, ident1, 0),
> +    DEFINE_PROP_UINT16("id2", struct pflash_t, ident2, 0),
> +    DEFINE_PROP_UINT16("id3", struct pflash_t, ident3, 0),
> +    DEFINE_PROP_UINT16("unlock_addr0", struct pflash_t, unlock_addr0, 0),
> +    DEFINE_PROP_UINT16("unlock_addr1", struct pflash_t, unlock_addr1, 0),
> +    DEFINE_PROP_STRING("name", struct pflash_t, name),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void pflash_cfi02_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    k->init = pflash_cfi02_init;
> +    dc->props = pflash_cfi02_properties;
> +}
> +
> +static const TypeInfo pflash_cfi02_info = {
> +    .name           = "cfi.pflash02",
> +    .parent         = TYPE_SYS_BUS_DEVICE,
> +    .instance_size  = sizeof(struct pflash_t),
> +    .class_init     = pflash_cfi02_class_init,
> +};
> +
> +static void pflash_cfi02_register_types(void)
> +{
> +    type_register_static(&pflash_cfi02_info);
> +}
> +
> +type_init(pflash_cfi02_register_types)
> +
> +pflash_t *pflash_cfi02_register(target_phys_addr_t base,
> +                                DeviceState *qdev, const char *name,
> +                                target_phys_addr_t size,
> +                                BlockDriverState *bs, uint32_t sector_len,
> +                                int nb_blocs, int nb_mappings, int width,
> +                                uint16_t id0, uint16_t id1,
> +                                uint16_t id2, uint16_t id3,
> +                                uint16_t unlock_addr0, uint16_t unlock_addr1,
> +                                int be)
> +{
> +    DeviceState *dev = qdev_create(NULL, "cfi.pflash02");
> +    SysBusDevice *busdev = sysbus_from_qdev(dev);
> +    pflash_t *pfl = (pflash_t *)object_dynamic_cast(OBJECT(dev),
> +                                                    "cfi.pflash02");
> +
> +    if (bs && qdev_prop_set_drive(dev, "bdrv", bs)) {
> +        abort();
> +    }
> +    qdev_prop_set_uint32(dev, "nb_blocs", nb_blocs);
> +    qdev_prop_set_uint32(dev, "sector_len", sector_len);
> +    qdev_prop_set_uint8(dev, "width", width);
> +    qdev_prop_set_uint8(dev, "mappings", nb_mappings);
> +    qdev_prop_set_uint8(dev, "be", !!be);
> +    qdev_prop_set_uint16(dev, "id0", id0);
> +    qdev_prop_set_uint16(dev, "id1", id1);
> +    qdev_prop_set_uint16(dev, "id2", id2);
> +    qdev_prop_set_uint16(dev, "id3", id3);
> +    qdev_prop_set_uint16(dev, "unlock_addr0", unlock_addr0);
> +    qdev_prop_set_uint16(dev, "unlock_addr1", unlock_addr1);
> +    qdev_init_nofail(dev);
> +
> +    sysbus_mmio_map(busdev, 0, base);
>      return pfl;
>  }
>
Peter Maydell - Oct. 19, 2012, 10:24 a.m.
On 19 October 2012 07:40, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> QOMified the pflash_cfi0x so machine models can connect them up in custom ways.
>
> Kept the pflash_cfi0x_register functions as is. They can still be used to
> create a flash straight onto system memory.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Thanks -- more QOMification is always nice.

> ---
>
>  hw/pflash_cfi01.c |  142 +++++++++++++++++++++++++++++++++++++------------
>  hw/pflash_cfi02.c |  154 ++++++++++++++++++++++++++++++++++++++++-------------
>  2 files changed, 224 insertions(+), 72 deletions(-)
>
> diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
> index ebc8a57..65cd619 100644
> --- a/hw/pflash_cfi01.c
> +++ b/hw/pflash_cfi01.c
> @@ -42,6 +42,7 @@
>  #include "qemu-timer.h"
>  #include "exec-memory.h"
>  #include "host-utils.h"
> +#include "sysbus.h"
>
>  #define PFLASH_BUG(fmt, ...) \
>  do { \
> @@ -60,21 +61,37 @@ do {                                               \
>  #endif
>
>  struct pflash_t {
> +    SysBusDevice busdev;
>      BlockDriverState *bs;
> -    target_phys_addr_t sector_len;
> -    int width;
> +    uint32_t nb_blocs;
> +    /* FIXME: get rid of target_phys_addr_t usage */
> +    union {
> +        target_phys_addr_t sector_len;
> +        uint32_t sector_len_u32;
> +    };

I think we should just fix this not to use target_phys_addr_t.
Option 1:
 * declare sector_len as uint64_t
 * fix the printf format in the DPRINTFs of it
Option 2:
 * declare sector_len as uint32_t
 * fix the printf formats
 * add casts to ensure 64 bit arithmetic when it is used in these exprs:
   offset &= ~(pfl->sector_len - 1);
   total_len = pfl->sector_len * pfl->nb_blocs;

Option 1 is slightly easier and I don't see any particular disadvantage
in having the sector length be a 64 bit property.

> +    uint8_t width;
> +    uint8_t be;
>      int wcycle; /* if 0, the flash is read normally */
>      int bypass;
>      int ro;
>      uint8_t cmd;
>      uint8_t status;
> -    uint16_t ident[4];
> +    union {
> +        uint16_t ident[4];
> +        struct {
> +            uint16_t ident0;
> +            uint16_t ident1;
> +            uint16_t ident2;
> +            uint16_t ident3;
> +        };
> +    };

the ident[] array is only used in one or two places so I would
suggest just fixing those to use ident0..ident3 and dropping
the union.

>      uint8_t cfi_len;
>      uint8_t cfi_table[0x52];
>      target_phys_addr_t counter;
>      unsigned int writeblock_size;
>      QEMUTimer *timer;
>      MemoryRegion mem;
> +    char *name;

can this take a 'const' qualifier?

>      void *storage;
>  };
>
> @@ -541,19 +558,13 @@ static const MemoryRegionOps pflash_cfi01_ops_le = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>
> -pflash_t *pflash_cfi01_register(target_phys_addr_t base,
> -                                DeviceState *qdev, const char *name,
> -                                target_phys_addr_t size,
> -                                BlockDriverState *bs, uint32_t sector_len,
> -                                int nb_blocs, int width,
> -                                uint16_t id0, uint16_t id1,
> -                                uint16_t id2, uint16_t id3, int be)
> +static int pflash_cfi01_init(SysBusDevice *dev)
>  {
> -    pflash_t *pfl;
> +    pflash_t *pfl = FROM_SYSBUS(typeof(*pfl), dev);
>      target_phys_addr_t total_len;
>      int ret;
>
> -    total_len = sector_len * nb_blocs;
> +    total_len = pfl->sector_len * pfl->nb_blocs;
>
>      /* XXX: to be fixed */
>  #if 0
> @@ -562,27 +573,26 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base,
>          return NULL;
>  #endif
>
> -    pfl = g_malloc0(sizeof(pflash_t));
> -
> +    if (!pfl->name) {
> +        static int next;
> +        pfl->name = g_strdup_printf("pflash.cfi01.%d", next++);
> +    }

Since all the callers do actually pass in a non-NULL name, you could
just say it was mandatory, and avoid this bit of code. That would
save wondering when to free the name...

>      memory_region_init_rom_device(
> -        &pfl->mem, be ? &pflash_cfi01_ops_be : &pflash_cfi01_ops_le, pfl,
> -        name, size);
> -    vmstate_register_ram(&pfl->mem, qdev);
> +        &pfl->mem, pfl->be ? &pflash_cfi01_ops_be : &pflash_cfi01_ops_le, pfl,
> +        pfl->name, total_len);
> +    vmstate_register_ram(&pfl->mem, DEVICE(pfl));
>      pfl->storage = memory_region_get_ram_ptr(&pfl->mem);
> -    memory_region_add_subregion(get_system_memory(), base, &pfl->mem);
> +    sysbus_init_mmio(dev, &pfl->mem);
>
> -    pfl->bs = bs;
>      if (pfl->bs) {
>          /* read the initial flash content */
>          ret = bdrv_read(pfl->bs, 0, pfl->storage, total_len >> 9);
> +
>          if (ret < 0) {
> -            memory_region_del_subregion(get_system_memory(), &pfl->mem);
> -            vmstate_unregister_ram(&pfl->mem, qdev);
> +            vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
>              memory_region_destroy(&pfl->mem);
> -            g_free(pfl);
> -            return NULL;
> +            return 1;
>          }
> -        bdrv_attach_dev_nofail(pfl->bs, pfl);
>      }
>
>      if (pfl->bs) {
> @@ -592,15 +602,9 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base,
>      }
>
>      pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
> -    pfl->sector_len = sector_len;
> -    pfl->width = width;
>      pfl->wcycle = 0;
>      pfl->cmd = 0;
>      pfl->status = 0;
> -    pfl->ident[0] = id0;
> -    pfl->ident[1] = id1;
> -    pfl->ident[2] = id2;
> -    pfl->ident[3] = id3;
>      /* Hardcoded CFI table */
>      pfl->cfi_len = 0x52;
>      /* Standard "QRY" string */
> @@ -649,7 +653,7 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base,
>      pfl->cfi_table[0x28] = 0x02;
>      pfl->cfi_table[0x29] = 0x00;
>      /* Max number of bytes in multi-bytes write */
> -    if (width == 1) {
> +    if (pfl->width == 1) {
>          pfl->cfi_table[0x2A] = 0x08;
>      } else {
>          pfl->cfi_table[0x2A] = 0x0B;
> @@ -660,10 +664,10 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base,
>      /* Number of erase block regions (uniform) */
>      pfl->cfi_table[0x2C] = 0x01;
>      /* Erase block region 1 */
> -    pfl->cfi_table[0x2D] = nb_blocs - 1;
> -    pfl->cfi_table[0x2E] = (nb_blocs - 1) >> 8;
> -    pfl->cfi_table[0x2F] = sector_len >> 8;
> -    pfl->cfi_table[0x30] = sector_len >> 16;
> +    pfl->cfi_table[0x2D] = pfl->nb_blocs - 1;
> +    pfl->cfi_table[0x2E] = (pfl->nb_blocs - 1) >> 8;
> +    pfl->cfi_table[0x2F] = pfl->sector_len >> 8;
> +    pfl->cfi_table[0x30] = pfl->sector_len >> 16;
>
>      /* Extended */
>      pfl->cfi_table[0x31] = 'P';
> @@ -685,6 +689,74 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base,
>
>      pfl->cfi_table[0x3f] = 0x01; /* Number of protection fields */
>
> +    return 0;
> +}
> +
> +static Property pflash_cfi01_properties[] = {
> +    DEFINE_PROP_DRIVE("bdrv", struct pflash_t, bs),
> +    DEFINE_PROP_UINT32("nb_blocs", struct pflash_t, nb_blocs, 0),

Let's not propagate the typo into the property name. "num-blocks"
is probably in line with other property name conventions.

> +    DEFINE_PROP_UINT32("sector_len", struct pflash_t, sector_len_u32, 0),

"sector-length".

> +    DEFINE_PROP_UINT8("width", struct pflash_t, width, 0),
> +    DEFINE_PROP_UINT8("be", struct pflash_t, be, 0),

"big-endian"

> +    DEFINE_PROP_UINT16("id0", struct pflash_t, ident0, 0),
> +    DEFINE_PROP_UINT16("id1", struct pflash_t, ident1, 0),
> +    DEFINE_PROP_UINT16("id2", struct pflash_t, ident2, 0),
> +    DEFINE_PROP_UINT16("id3", struct pflash_t, ident3, 0),
> +    DEFINE_PROP_STRING("name", struct pflash_t, name),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void pflash_cfi01_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    k->init = pflash_cfi01_init;
> +    dc->props = pflash_cfi01_properties;
> +}
> +
> +
> +static const TypeInfo pflash_cfi01_info = {
> +    .name           = "cfi.pflash01",
> +    .parent         = TYPE_SYS_BUS_DEVICE,
> +    .instance_size  = sizeof(struct pflash_t),
> +    .class_init     = pflash_cfi01_class_init,
> +};
> +
> +static void pflash_cfi01_register_types(void)
> +{
> +    type_register_static(&pflash_cfi01_info);
> +}
> +
> +type_init(pflash_cfi01_register_types)
> +
> +pflash_t *pflash_cfi01_register(target_phys_addr_t base,
> +                                DeviceState *qdev, const char *name,
> +                                target_phys_addr_t size,
> +                                BlockDriverState *bs,
> +                                uint32_t sector_len, int nb_blocs, int width,
> +                                uint16_t id0, uint16_t id1,
> +                                uint16_t id2, uint16_t id3, int be)
> +{
> +    DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
> +    SysBusDevice *busdev = sysbus_from_qdev(dev);
> +    pflash_t *pfl = (pflash_t *)object_dynamic_cast(OBJECT(dev),
> +                                                    "cfi.pflash01");

A useful followup patch to this one would be to:
 * change this function to return a DeviceState *
   [getting rid of this dynamic cast in the process]
 * change the uses of pflash_cfi01_get_memory() to use
   sysbus_mmio_get_region(sysbus_from_qdev(dev), 0) instead
 * delete the now unused pflash_cfi01_get_memory()
 * remove the declaration of pflash_t from flash.h (so it's a
   purely private type to the device implementation)

> +
> +    if (bs && qdev_prop_set_drive(dev, "bdrv", bs)) {
> +        abort();
> +    }
> +    qdev_prop_set_uint32(dev, "nb_blocs", nb_blocs);
> +    qdev_prop_set_uint32(dev, "sector_len", sector_len);
> +    qdev_prop_set_uint8(dev, "width", width);
> +    qdev_prop_set_uint8(dev, "be", !!be);
> +    qdev_prop_set_uint16(dev, "id0", id0);
> +    qdev_prop_set_uint16(dev, "id1", id1);
> +    qdev_prop_set_uint16(dev, "id2", id2);
> +    qdev_prop_set_uint16(dev, "id3", id3);
> +    qdev_init_nofail(dev);
> +
> +    sysbus_mmio_map(busdev, 0, base);
>      return pfl;
>  }
>
> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
> index 43fb3a4..db05fe6 100644
> --- a/hw/pflash_cfi02.c
> +++ b/hw/pflash_cfi02.c
> @@ -41,6 +41,7 @@
>  #include "block.h"
>  #include "exec-memory.h"
>  #include "host-utils.h"
> +#include "sysbus.h"
>
>  //#define PFLASH_DEBUG
>  #ifdef PFLASH_DEBUG
> @@ -55,18 +56,36 @@ do {                                               \
>  #define PFLASH_LAZY_ROMD_THRESHOLD 42
>
>  struct pflash_t {
> +    SysBusDevice busdev;
>      BlockDriverState *bs;
>      uint32_t sector_len;
> +    uint32_t nb_blocs;
>      uint32_t chip_len;
> -    int mappings;
> -    int width;
> +    uint8_t mappings;
> +    uint8_t width;
> +    uint8_t be;
>      int wcycle; /* if 0, the flash is read normally */
>      int bypass;
>      int ro;
>      uint8_t cmd;
>      uint8_t status;
> -    uint16_t ident[4];
> -    uint16_t unlock_addr[2];
> +    /* FIXME: implement array device properties */
> +    union {
> +        uint16_t ident[4];
> +        struct {
> +            uint16_t ident0;
> +            uint16_t ident1;
> +            uint16_t ident2;
> +            uint16_t ident3;
> +        };
> +    };
> +    union {
> +        uint16_t unlock_addr[2];
> +        struct {
> +            uint16_t unlock_addr0;
> +            uint16_t unlock_addr1;
> +        };

Again, I would just drop the unions.

Most of the comments on the 01 device also apply to 02, so I haven't
repeated them explicitly.

thanks
-- PMM
Peter Crosthwaite - Oct. 22, 2012, 6:46 a.m.
On Fri, Oct 19, 2012 at 8:24 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 19 October 2012 07:40, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> QOMified the pflash_cfi0x so machine models can connect them up in custom ways.
>>
>> Kept the pflash_cfi0x_register functions as is. They can still be used to
>> create a flash straight onto system memory.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> Thanks -- more QOMification is always nice.
>
>> ---
>>
>>  hw/pflash_cfi01.c |  142 +++++++++++++++++++++++++++++++++++++------------
>>  hw/pflash_cfi02.c |  154 ++++++++++++++++++++++++++++++++++++++++-------------
>>  2 files changed, 224 insertions(+), 72 deletions(-)
>>
>> diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
>> index ebc8a57..65cd619 100644
>> --- a/hw/pflash_cfi01.c
>> +++ b/hw/pflash_cfi01.c
>> @@ -42,6 +42,7 @@
>>  #include "qemu-timer.h"
>>  #include "exec-memory.h"
>>  #include "host-utils.h"
>> +#include "sysbus.h"
>>
>>  #define PFLASH_BUG(fmt, ...) \
>>  do { \
>> @@ -60,21 +61,37 @@ do {                                               \
>>  #endif
>>
>>  struct pflash_t {
>> +    SysBusDevice busdev;
>>      BlockDriverState *bs;
>> -    target_phys_addr_t sector_len;
>> -    int width;
>> +    uint32_t nb_blocs;
>> +    /* FIXME: get rid of target_phys_addr_t usage */
>> +    union {
>> +        target_phys_addr_t sector_len;
>> +        uint32_t sector_len_u32;
>> +    };
>
> I think we should just fix this not to use target_phys_addr_t.
> Option 1:
>  * declare sector_len as uint64_t
>  * fix the printf format in the DPRINTFs of it

Done

> Option 2:
>  * declare sector_len as uint32_t
>  * fix the printf formats
>  * add casts to ensure 64 bit arithmetic when it is used in these exprs:
>    offset &= ~(pfl->sector_len - 1);
>    total_len = pfl->sector_len * pfl->nb_blocs;
>
> Option 1 is slightly easier and I don't see any particular disadvantage
> in having the sector length be a 64 bit property.
>
>> +    uint8_t width;
>> +    uint8_t be;
>>      int wcycle; /* if 0, the flash is read normally */
>>      int bypass;
>>      int ro;
>>      uint8_t cmd;
>>      uint8_t status;
>> -    uint16_t ident[4];
>> +    union {
>> +        uint16_t ident[4];
>> +        struct {
>> +            uint16_t ident0;
>> +            uint16_t ident1;
>> +            uint16_t ident2;
>> +            uint16_t ident3;
>> +        };
>> +    };
>
> the ident[] array is only used in one or two places so I would
> suggest just fixing those to use ident0..ident3 and dropping
> the union.
>

OK

>>      uint8_t cfi_len;
>>      uint8_t cfi_table[0x52];
>>      target_phys_addr_t counter;
>>      unsigned int writeblock_size;
>>      QEMUTimer *timer;
>>      MemoryRegion mem;
>> +    char *name;
>
> can this take a 'const' qualifier?
>

No because DEFINE_PROP_STRING expects it to be non-const.

>>      void *storage;
>>  };
>>
>> @@ -541,19 +558,13 @@ static const MemoryRegionOps pflash_cfi01_ops_le = {
>>      .endianness = DEVICE_NATIVE_ENDIAN,
>>  };
>>
>> -pflash_t *pflash_cfi01_register(target_phys_addr_t base,
>> -                                DeviceState *qdev, const char *name,
>> -                                target_phys_addr_t size,
>> -                                BlockDriverState *bs, uint32_t sector_len,
>> -                                int nb_blocs, int width,
>> -                                uint16_t id0, uint16_t id1,
>> -                                uint16_t id2, uint16_t id3, int be)
>> +static int pflash_cfi01_init(SysBusDevice *dev)
>>  {
>> -    pflash_t *pfl;
>> +    pflash_t *pfl = FROM_SYSBUS(typeof(*pfl), dev);
>>      target_phys_addr_t total_len;
>>      int ret;
>>
>> -    total_len = sector_len * nb_blocs;
>> +    total_len = pfl->sector_len * pfl->nb_blocs;
>>
>>      /* XXX: to be fixed */
>>  #if 0
>> @@ -562,27 +573,26 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base,
>>          return NULL;
>>  #endif
>>
>> -    pfl = g_malloc0(sizeof(pflash_t));
>> -
>> +    if (!pfl->name) {
>> +        static int next;
>> +        pfl->name = g_strdup_printf("pflash.cfi01.%d", next++);
>> +    }
>
> Since all the callers do actually pass in a non-NULL name, you could
> just say it was mandatory, and avoid this bit of code. That would
> save wondering when to free the name...
>

OK

>>      memory_region_init_rom_device(
>> -        &pfl->mem, be ? &pflash_cfi01_ops_be : &pflash_cfi01_ops_le, pfl,
>> -        name, size);
>> -    vmstate_register_ram(&pfl->mem, qdev);
>> +        &pfl->mem, pfl->be ? &pflash_cfi01_ops_be : &pflash_cfi01_ops_le, pfl,
>> +        pfl->name, total_len);
>> +    vmstate_register_ram(&pfl->mem, DEVICE(pfl));
>>      pfl->storage = memory_region_get_ram_ptr(&pfl->mem);
>> -    memory_region_add_subregion(get_system_memory(), base, &pfl->mem);
>> +    sysbus_init_mmio(dev, &pfl->mem);
>>
>> -    pfl->bs = bs;
>>      if (pfl->bs) {
>>          /* read the initial flash content */
>>          ret = bdrv_read(pfl->bs, 0, pfl->storage, total_len >> 9);
>> +
>>          if (ret < 0) {
>> -            memory_region_del_subregion(get_system_memory(), &pfl->mem);
>> -            vmstate_unregister_ram(&pfl->mem, qdev);
>> +            vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
>>              memory_region_destroy(&pfl->mem);
>> -            g_free(pfl);
>> -            return NULL;
>> +            return 1;
>>          }
>> -        bdrv_attach_dev_nofail(pfl->bs, pfl);
>>      }
>>
>>      if (pfl->bs) {
>> @@ -592,15 +602,9 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base,
>>      }
>>
>>      pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
>> -    pfl->sector_len = sector_len;
>> -    pfl->width = width;
>>      pfl->wcycle = 0;
>>      pfl->cmd = 0;
>>      pfl->status = 0;
>> -    pfl->ident[0] = id0;
>> -    pfl->ident[1] = id1;
>> -    pfl->ident[2] = id2;
>> -    pfl->ident[3] = id3;
>>      /* Hardcoded CFI table */
>>      pfl->cfi_len = 0x52;
>>      /* Standard "QRY" string */
>> @@ -649,7 +653,7 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base,
>>      pfl->cfi_table[0x28] = 0x02;
>>      pfl->cfi_table[0x29] = 0x00;
>>      /* Max number of bytes in multi-bytes write */
>> -    if (width == 1) {
>> +    if (pfl->width == 1) {
>>          pfl->cfi_table[0x2A] = 0x08;
>>      } else {
>>          pfl->cfi_table[0x2A] = 0x0B;
>> @@ -660,10 +664,10 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base,
>>      /* Number of erase block regions (uniform) */
>>      pfl->cfi_table[0x2C] = 0x01;
>>      /* Erase block region 1 */
>> -    pfl->cfi_table[0x2D] = nb_blocs - 1;
>> -    pfl->cfi_table[0x2E] = (nb_blocs - 1) >> 8;
>> -    pfl->cfi_table[0x2F] = sector_len >> 8;
>> -    pfl->cfi_table[0x30] = sector_len >> 16;
>> +    pfl->cfi_table[0x2D] = pfl->nb_blocs - 1;
>> +    pfl->cfi_table[0x2E] = (pfl->nb_blocs - 1) >> 8;
>> +    pfl->cfi_table[0x2F] = pfl->sector_len >> 8;
>> +    pfl->cfi_table[0x30] = pfl->sector_len >> 16;
>>
>>      /* Extended */
>>      pfl->cfi_table[0x31] = 'P';
>> @@ -685,6 +689,74 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base,
>>
>>      pfl->cfi_table[0x3f] = 0x01; /* Number of protection fields */
>>
>> +    return 0;
>> +}
>> +
>> +static Property pflash_cfi01_properties[] = {
>> +    DEFINE_PROP_DRIVE("bdrv", struct pflash_t, bs),
>> +    DEFINE_PROP_UINT32("nb_blocs", struct pflash_t, nb_blocs, 0),
>
> Let's not propagate the typo into the property name. "num-blocks"
> is probably in line with other property name conventions.
>
>> +    DEFINE_PROP_UINT32("sector_len", struct pflash_t, sector_len_u32, 0),
>
> "sector-length".
>
>> +    DEFINE_PROP_UINT8("width", struct pflash_t, width, 0),
>> +    DEFINE_PROP_UINT8("be", struct pflash_t, be, 0),
>
> "big-endian"
>
>> +    DEFINE_PROP_UINT16("id0", struct pflash_t, ident0, 0),
>> +    DEFINE_PROP_UINT16("id1", struct pflash_t, ident1, 0),
>> +    DEFINE_PROP_UINT16("id2", struct pflash_t, ident2, 0),
>> +    DEFINE_PROP_UINT16("id3", struct pflash_t, ident3, 0),
>> +    DEFINE_PROP_STRING("name", struct pflash_t, name),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void pflash_cfi01_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>> +
>> +    k->init = pflash_cfi01_init;
>> +    dc->props = pflash_cfi01_properties;
>> +}
>> +
>> +
>> +static const TypeInfo pflash_cfi01_info = {
>> +    .name           = "cfi.pflash01",
>> +    .parent         = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size  = sizeof(struct pflash_t),
>> +    .class_init     = pflash_cfi01_class_init,
>> +};
>> +
>> +static void pflash_cfi01_register_types(void)
>> +{
>> +    type_register_static(&pflash_cfi01_info);
>> +}
>> +
>> +type_init(pflash_cfi01_register_types)
>> +
>> +pflash_t *pflash_cfi01_register(target_phys_addr_t base,
>> +                                DeviceState *qdev, const char *name,
>> +                                target_phys_addr_t size,
>> +                                BlockDriverState *bs,
>> +                                uint32_t sector_len, int nb_blocs, int width,
>> +                                uint16_t id0, uint16_t id1,
>> +                                uint16_t id2, uint16_t id3, int be)
>> +{
>> +    DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
>> +    SysBusDevice *busdev = sysbus_from_qdev(dev);
>> +    pflash_t *pfl = (pflash_t *)object_dynamic_cast(OBJECT(dev),
>> +                                                    "cfi.pflash01");
>
> A useful followup patch to this one would be to:
>  * change this function to return a DeviceState *
>    [getting rid of this dynamic cast in the process]
>  * change the uses of pflash_cfi01_get_memory() to use
>    sysbus_mmio_get_region(sysbus_from_qdev(dev), 0) instead
>  * delete the now unused pflash_cfi01_get_memory()
>  * remove the declaration of pflash_t from flash.h (so it's a
>    purely private type to the device implementation)
>

Yes, thats the logical next step. Will leave for a future series for the moment.

>> +
>> +    if (bs && qdev_prop_set_drive(dev, "bdrv", bs)) {
>> +        abort();
>> +    }
>> +    qdev_prop_set_uint32(dev, "nb_blocs", nb_blocs);
>> +    qdev_prop_set_uint32(dev, "sector_len", sector_len);
>> +    qdev_prop_set_uint8(dev, "width", width);
>> +    qdev_prop_set_uint8(dev, "be", !!be);
>> +    qdev_prop_set_uint16(dev, "id0", id0);
>> +    qdev_prop_set_uint16(dev, "id1", id1);
>> +    qdev_prop_set_uint16(dev, "id2", id2);
>> +    qdev_prop_set_uint16(dev, "id3", id3);
>> +    qdev_init_nofail(dev);
>> +
>> +    sysbus_mmio_map(busdev, 0, base);
>>      return pfl;
>>  }
>>
>> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
>> index 43fb3a4..db05fe6 100644
>> --- a/hw/pflash_cfi02.c
>> +++ b/hw/pflash_cfi02.c
>> @@ -41,6 +41,7 @@
>>  #include "block.h"
>>  #include "exec-memory.h"
>>  #include "host-utils.h"
>> +#include "sysbus.h"
>>
>>  //#define PFLASH_DEBUG
>>  #ifdef PFLASH_DEBUG
>> @@ -55,18 +56,36 @@ do {                                               \
>>  #define PFLASH_LAZY_ROMD_THRESHOLD 42
>>
>>  struct pflash_t {
>> +    SysBusDevice busdev;
>>      BlockDriverState *bs;
>>      uint32_t sector_len;
>> +    uint32_t nb_blocs;
>>      uint32_t chip_len;
>> -    int mappings;
>> -    int width;
>> +    uint8_t mappings;
>> +    uint8_t width;
>> +    uint8_t be;
>>      int wcycle; /* if 0, the flash is read normally */
>>      int bypass;
>>      int ro;
>>      uint8_t cmd;
>>      uint8_t status;
>> -    uint16_t ident[4];
>> -    uint16_t unlock_addr[2];
>> +    /* FIXME: implement array device properties */
>> +    union {
>> +        uint16_t ident[4];
>> +        struct {
>> +            uint16_t ident0;
>> +            uint16_t ident1;
>> +            uint16_t ident2;
>> +            uint16_t ident3;
>> +        };
>> +    };
>> +    union {
>> +        uint16_t unlock_addr[2];
>> +        struct {
>> +            uint16_t unlock_addr0;
>> +            uint16_t unlock_addr1;
>> +        };
>
> Again, I would just drop the unions.
>
> Most of the comments on the 01 device also apply to 02, so I haven't
> repeated them explicitly.
>

All suggested changes apart from the const name made to v2.

Regards,
Peter

> thanks
> -- PMM
>

Patch

diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
index ebc8a57..65cd619 100644
--- a/hw/pflash_cfi01.c
+++ b/hw/pflash_cfi01.c
@@ -42,6 +42,7 @@ 
 #include "qemu-timer.h"
 #include "exec-memory.h"
 #include "host-utils.h"
+#include "sysbus.h"
 
 #define PFLASH_BUG(fmt, ...) \
 do { \
@@ -60,21 +61,37 @@  do {                                               \
 #endif
 
 struct pflash_t {
+    SysBusDevice busdev;
     BlockDriverState *bs;
-    target_phys_addr_t sector_len;
-    int width;
+    uint32_t nb_blocs;
+    /* FIXME: get rid of target_phys_addr_t usage */
+    union {
+        target_phys_addr_t sector_len;
+        uint32_t sector_len_u32;
+    };
+    uint8_t width;
+    uint8_t be;
     int wcycle; /* if 0, the flash is read normally */
     int bypass;
     int ro;
     uint8_t cmd;
     uint8_t status;
-    uint16_t ident[4];
+    union {
+        uint16_t ident[4];
+        struct {
+            uint16_t ident0;
+            uint16_t ident1;
+            uint16_t ident2;
+            uint16_t ident3;
+        };
+    };
     uint8_t cfi_len;
     uint8_t cfi_table[0x52];
     target_phys_addr_t counter;
     unsigned int writeblock_size;
     QEMUTimer *timer;
     MemoryRegion mem;
+    char *name;
     void *storage;
 };
 
@@ -541,19 +558,13 @@  static const MemoryRegionOps pflash_cfi01_ops_le = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-pflash_t *pflash_cfi01_register(target_phys_addr_t base,
-                                DeviceState *qdev, const char *name,
-                                target_phys_addr_t size,
-                                BlockDriverState *bs, uint32_t sector_len,
-                                int nb_blocs, int width,
-                                uint16_t id0, uint16_t id1,
-                                uint16_t id2, uint16_t id3, int be)
+static int pflash_cfi01_init(SysBusDevice *dev)
 {
-    pflash_t *pfl;
+    pflash_t *pfl = FROM_SYSBUS(typeof(*pfl), dev);
     target_phys_addr_t total_len;
     int ret;
 
-    total_len = sector_len * nb_blocs;
+    total_len = pfl->sector_len * pfl->nb_blocs;
 
     /* XXX: to be fixed */
 #if 0
@@ -562,27 +573,26 @@  pflash_t *pflash_cfi01_register(target_phys_addr_t base,
         return NULL;
 #endif
 
-    pfl = g_malloc0(sizeof(pflash_t));
-
+    if (!pfl->name) {
+        static int next;
+        pfl->name = g_strdup_printf("pflash.cfi01.%d", next++);
+    }
     memory_region_init_rom_device(
-        &pfl->mem, be ? &pflash_cfi01_ops_be : &pflash_cfi01_ops_le, pfl,
-        name, size);
-    vmstate_register_ram(&pfl->mem, qdev);
+        &pfl->mem, pfl->be ? &pflash_cfi01_ops_be : &pflash_cfi01_ops_le, pfl,
+        pfl->name, total_len);
+    vmstate_register_ram(&pfl->mem, DEVICE(pfl));
     pfl->storage = memory_region_get_ram_ptr(&pfl->mem);
-    memory_region_add_subregion(get_system_memory(), base, &pfl->mem);
+    sysbus_init_mmio(dev, &pfl->mem);
 
-    pfl->bs = bs;
     if (pfl->bs) {
         /* read the initial flash content */
         ret = bdrv_read(pfl->bs, 0, pfl->storage, total_len >> 9);
+
         if (ret < 0) {
-            memory_region_del_subregion(get_system_memory(), &pfl->mem);
-            vmstate_unregister_ram(&pfl->mem, qdev);
+            vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
             memory_region_destroy(&pfl->mem);
-            g_free(pfl);
-            return NULL;
+            return 1;
         }
-        bdrv_attach_dev_nofail(pfl->bs, pfl);
     }
 
     if (pfl->bs) {
@@ -592,15 +602,9 @@  pflash_t *pflash_cfi01_register(target_phys_addr_t base,
     }
 
     pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
-    pfl->sector_len = sector_len;
-    pfl->width = width;
     pfl->wcycle = 0;
     pfl->cmd = 0;
     pfl->status = 0;
-    pfl->ident[0] = id0;
-    pfl->ident[1] = id1;
-    pfl->ident[2] = id2;
-    pfl->ident[3] = id3;
     /* Hardcoded CFI table */
     pfl->cfi_len = 0x52;
     /* Standard "QRY" string */
@@ -649,7 +653,7 @@  pflash_t *pflash_cfi01_register(target_phys_addr_t base,
     pfl->cfi_table[0x28] = 0x02;
     pfl->cfi_table[0x29] = 0x00;
     /* Max number of bytes in multi-bytes write */
-    if (width == 1) {
+    if (pfl->width == 1) {
         pfl->cfi_table[0x2A] = 0x08;
     } else {
         pfl->cfi_table[0x2A] = 0x0B;
@@ -660,10 +664,10 @@  pflash_t *pflash_cfi01_register(target_phys_addr_t base,
     /* Number of erase block regions (uniform) */
     pfl->cfi_table[0x2C] = 0x01;
     /* Erase block region 1 */
-    pfl->cfi_table[0x2D] = nb_blocs - 1;
-    pfl->cfi_table[0x2E] = (nb_blocs - 1) >> 8;
-    pfl->cfi_table[0x2F] = sector_len >> 8;
-    pfl->cfi_table[0x30] = sector_len >> 16;
+    pfl->cfi_table[0x2D] = pfl->nb_blocs - 1;
+    pfl->cfi_table[0x2E] = (pfl->nb_blocs - 1) >> 8;
+    pfl->cfi_table[0x2F] = pfl->sector_len >> 8;
+    pfl->cfi_table[0x30] = pfl->sector_len >> 16;
 
     /* Extended */
     pfl->cfi_table[0x31] = 'P';
@@ -685,6 +689,74 @@  pflash_t *pflash_cfi01_register(target_phys_addr_t base,
 
     pfl->cfi_table[0x3f] = 0x01; /* Number of protection fields */
 
+    return 0;
+}
+
+static Property pflash_cfi01_properties[] = {
+    DEFINE_PROP_DRIVE("bdrv", struct pflash_t, bs),
+    DEFINE_PROP_UINT32("nb_blocs", struct pflash_t, nb_blocs, 0),
+    DEFINE_PROP_UINT32("sector_len", struct pflash_t, sector_len_u32, 0),
+    DEFINE_PROP_UINT8("width", struct pflash_t, width, 0),
+    DEFINE_PROP_UINT8("be", struct pflash_t, be, 0),
+    DEFINE_PROP_UINT16("id0", struct pflash_t, ident0, 0),
+    DEFINE_PROP_UINT16("id1", struct pflash_t, ident1, 0),
+    DEFINE_PROP_UINT16("id2", struct pflash_t, ident2, 0),
+    DEFINE_PROP_UINT16("id3", struct pflash_t, ident3, 0),
+    DEFINE_PROP_STRING("name", struct pflash_t, name),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void pflash_cfi01_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+    k->init = pflash_cfi01_init;
+    dc->props = pflash_cfi01_properties;
+}
+
+
+static const TypeInfo pflash_cfi01_info = {
+    .name           = "cfi.pflash01",
+    .parent         = TYPE_SYS_BUS_DEVICE,
+    .instance_size  = sizeof(struct pflash_t),
+    .class_init     = pflash_cfi01_class_init,
+};
+
+static void pflash_cfi01_register_types(void)
+{
+    type_register_static(&pflash_cfi01_info);
+}
+
+type_init(pflash_cfi01_register_types)
+
+pflash_t *pflash_cfi01_register(target_phys_addr_t base,
+                                DeviceState *qdev, const char *name,
+                                target_phys_addr_t size,
+                                BlockDriverState *bs,
+                                uint32_t sector_len, int nb_blocs, int width,
+                                uint16_t id0, uint16_t id1,
+                                uint16_t id2, uint16_t id3, int be)
+{
+    DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
+    SysBusDevice *busdev = sysbus_from_qdev(dev);
+    pflash_t *pfl = (pflash_t *)object_dynamic_cast(OBJECT(dev),
+                                                    "cfi.pflash01");
+
+    if (bs && qdev_prop_set_drive(dev, "bdrv", bs)) {
+        abort();
+    }
+    qdev_prop_set_uint32(dev, "nb_blocs", nb_blocs);
+    qdev_prop_set_uint32(dev, "sector_len", sector_len);
+    qdev_prop_set_uint8(dev, "width", width);
+    qdev_prop_set_uint8(dev, "be", !!be);
+    qdev_prop_set_uint16(dev, "id0", id0);
+    qdev_prop_set_uint16(dev, "id1", id1);
+    qdev_prop_set_uint16(dev, "id2", id2);
+    qdev_prop_set_uint16(dev, "id3", id3);
+    qdev_init_nofail(dev);
+
+    sysbus_mmio_map(busdev, 0, base);
     return pfl;
 }
 
diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
index 43fb3a4..db05fe6 100644
--- a/hw/pflash_cfi02.c
+++ b/hw/pflash_cfi02.c
@@ -41,6 +41,7 @@ 
 #include "block.h"
 #include "exec-memory.h"
 #include "host-utils.h"
+#include "sysbus.h"
 
 //#define PFLASH_DEBUG
 #ifdef PFLASH_DEBUG
@@ -55,18 +56,36 @@  do {                                               \
 #define PFLASH_LAZY_ROMD_THRESHOLD 42
 
 struct pflash_t {
+    SysBusDevice busdev;
     BlockDriverState *bs;
     uint32_t sector_len;
+    uint32_t nb_blocs;
     uint32_t chip_len;
-    int mappings;
-    int width;
+    uint8_t mappings;
+    uint8_t width;
+    uint8_t be;
     int wcycle; /* if 0, the flash is read normally */
     int bypass;
     int ro;
     uint8_t cmd;
     uint8_t status;
-    uint16_t ident[4];
-    uint16_t unlock_addr[2];
+    /* FIXME: implement array device properties */
+    union {
+        uint16_t ident[4];
+        struct {
+            uint16_t ident0;
+            uint16_t ident1;
+            uint16_t ident2;
+            uint16_t ident3;
+        };
+    };
+    union {
+        uint16_t unlock_addr[2];
+        struct {
+            uint16_t unlock_addr0;
+            uint16_t unlock_addr1;
+        };
+    };
     uint8_t cfi_len;
     uint8_t cfi_table[0x52];
     QEMUTimer *timer;
@@ -79,6 +98,7 @@  struct pflash_t {
     MemoryRegion orig_mem;
     int rom_mode;
     int read_counter; /* used for lazy switch-back to rom mode */
+    char *name;
     void *storage;
 };
 
@@ -574,49 +594,42 @@  static const MemoryRegionOps pflash_cfi02_ops_le = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-pflash_t *pflash_cfi02_register(target_phys_addr_t base,
-                                DeviceState *qdev, const char *name,
-                                target_phys_addr_t size,
-                                BlockDriverState *bs, uint32_t sector_len,
-                                int nb_blocs, int nb_mappings, int width,
-                                uint16_t id0, uint16_t id1,
-                                uint16_t id2, uint16_t id3,
-                                uint16_t unlock_addr0, uint16_t unlock_addr1,
-                                int be)
+static int pflash_cfi02_init(SysBusDevice *dev)
 {
-    pflash_t *pfl;
+    pflash_t *pfl = FROM_SYSBUS(typeof(*pfl), dev);
     int32_t chip_len;
     int ret;
 
-    chip_len = sector_len * nb_blocs;
+    chip_len = pfl->sector_len * pfl->nb_blocs;
     /* XXX: to be fixed */
 #if 0
     if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) &&
         total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024))
         return NULL;
 #endif
-    pfl = g_malloc0(sizeof(pflash_t));
-    memory_region_init_rom_device(
-        &pfl->orig_mem, be ? &pflash_cfi02_ops_be : &pflash_cfi02_ops_le, pfl,
-        name, size);
-    vmstate_register_ram(&pfl->orig_mem, qdev);
+
+    if (!pfl->name) {
+        static int next;
+        pfl->name = g_strdup_printf("pflash.cfi01.%d", next++);
+    }
+    memory_region_init_rom_device(&pfl->orig_mem, pfl->be ?
+                                  &pflash_cfi02_ops_be : &pflash_cfi02_ops_le,
+                                  pfl, pfl->name, chip_len);
+    vmstate_register_ram(&pfl->orig_mem, DEVICE(pfl));
     pfl->storage = memory_region_get_ram_ptr(&pfl->orig_mem);
     pfl->chip_len = chip_len;
-    pfl->mappings = nb_mappings;
-    pfl->bs = bs;
     if (pfl->bs) {
         /* read the initial flash content */
         ret = bdrv_read(pfl->bs, 0, pfl->storage, chip_len >> 9);
         if (ret < 0) {
             g_free(pfl);
-            return NULL;
+            return 1;
         }
-        bdrv_attach_dev_nofail(pfl->bs, pfl);
     }
 
     pflash_setup_mappings(pfl);
     pfl->rom_mode = 1;
-    memory_region_add_subregion(get_system_memory(), base, &pfl->mem);
+    sysbus_init_mmio(dev, &pfl->mem);
 
     if (pfl->bs) {
         pfl->ro = bdrv_is_read_only(pfl->bs);
@@ -625,17 +638,9 @@  pflash_t *pflash_cfi02_register(target_phys_addr_t base,
     }
 
     pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
-    pfl->sector_len = sector_len;
-    pfl->width = width;
     pfl->wcycle = 0;
     pfl->cmd = 0;
     pfl->status = 0;
-    pfl->ident[0] = id0;
-    pfl->ident[1] = id1;
-    pfl->ident[2] = id2;
-    pfl->ident[3] = id3;
-    pfl->unlock_addr[0] = unlock_addr0;
-    pfl->unlock_addr[1] = unlock_addr1;
     /* Hardcoded CFI table (mostly from SG29 Spansion flash) */
     pfl->cfi_len = 0x52;
     /* Standard "QRY" string */
@@ -691,10 +696,10 @@  pflash_t *pflash_cfi02_register(target_phys_addr_t base,
     /* Number of erase block regions (uniform) */
     pfl->cfi_table[0x2C] = 0x01;
     /* Erase block region 1 */
-    pfl->cfi_table[0x2D] = nb_blocs - 1;
-    pfl->cfi_table[0x2E] = (nb_blocs - 1) >> 8;
-    pfl->cfi_table[0x2F] = sector_len >> 8;
-    pfl->cfi_table[0x30] = sector_len >> 16;
+    pfl->cfi_table[0x2D] = pfl->nb_blocs - 1;
+    pfl->cfi_table[0x2E] = (pfl->nb_blocs - 1) >> 8;
+    pfl->cfi_table[0x2F] = pfl->sector_len >> 8;
+    pfl->cfi_table[0x30] = pfl->sector_len >> 16;
 
     /* Extended */
     pfl->cfi_table[0x31] = 'P';
@@ -714,5 +719,80 @@  pflash_t *pflash_cfi02_register(target_phys_addr_t base,
     pfl->cfi_table[0x3b] = 0x00;
     pfl->cfi_table[0x3c] = 0x00;
 
+    return 0;
+}
+
+static Property pflash_cfi02_properties[] = {
+    DEFINE_PROP_DRIVE("bdrv", struct pflash_t, bs),
+    DEFINE_PROP_UINT32("nb_blocs", struct pflash_t, nb_blocs, 0),
+    DEFINE_PROP_UINT32("sector_len", struct pflash_t, sector_len, 0),
+    DEFINE_PROP_UINT8("width", struct pflash_t, width, 0),
+    DEFINE_PROP_UINT8("mappings", struct pflash_t, mappings, 0),
+    DEFINE_PROP_UINT8("be", struct pflash_t, be, 0),
+    DEFINE_PROP_UINT16("id0", struct pflash_t, ident0, 0),
+    DEFINE_PROP_UINT16("id1", struct pflash_t, ident1, 0),
+    DEFINE_PROP_UINT16("id2", struct pflash_t, ident2, 0),
+    DEFINE_PROP_UINT16("id3", struct pflash_t, ident3, 0),
+    DEFINE_PROP_UINT16("unlock_addr0", struct pflash_t, unlock_addr0, 0),
+    DEFINE_PROP_UINT16("unlock_addr1", struct pflash_t, unlock_addr1, 0),
+    DEFINE_PROP_STRING("name", struct pflash_t, name),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void pflash_cfi02_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+    k->init = pflash_cfi02_init;
+    dc->props = pflash_cfi02_properties;
+}
+
+static const TypeInfo pflash_cfi02_info = {
+    .name           = "cfi.pflash02",
+    .parent         = TYPE_SYS_BUS_DEVICE,
+    .instance_size  = sizeof(struct pflash_t),
+    .class_init     = pflash_cfi02_class_init,
+};
+
+static void pflash_cfi02_register_types(void)
+{
+    type_register_static(&pflash_cfi02_info);
+}
+
+type_init(pflash_cfi02_register_types)
+
+pflash_t *pflash_cfi02_register(target_phys_addr_t base,
+                                DeviceState *qdev, const char *name,
+                                target_phys_addr_t size,
+                                BlockDriverState *bs, uint32_t sector_len,
+                                int nb_blocs, int nb_mappings, int width,
+                                uint16_t id0, uint16_t id1,
+                                uint16_t id2, uint16_t id3,
+                                uint16_t unlock_addr0, uint16_t unlock_addr1,
+                                int be)
+{
+    DeviceState *dev = qdev_create(NULL, "cfi.pflash02");
+    SysBusDevice *busdev = sysbus_from_qdev(dev);
+    pflash_t *pfl = (pflash_t *)object_dynamic_cast(OBJECT(dev),
+                                                    "cfi.pflash02");
+
+    if (bs && qdev_prop_set_drive(dev, "bdrv", bs)) {
+        abort();
+    }
+    qdev_prop_set_uint32(dev, "nb_blocs", nb_blocs);
+    qdev_prop_set_uint32(dev, "sector_len", sector_len);
+    qdev_prop_set_uint8(dev, "width", width);
+    qdev_prop_set_uint8(dev, "mappings", nb_mappings);
+    qdev_prop_set_uint8(dev, "be", !!be);
+    qdev_prop_set_uint16(dev, "id0", id0);
+    qdev_prop_set_uint16(dev, "id1", id1);
+    qdev_prop_set_uint16(dev, "id2", id2);
+    qdev_prop_set_uint16(dev, "id3", id3);
+    qdev_prop_set_uint16(dev, "unlock_addr0", unlock_addr0);
+    qdev_prop_set_uint16(dev, "unlock_addr1", unlock_addr1);
+    qdev_init_nofail(dev);
+
+    sysbus_mmio_map(busdev, 0, base);
     return pfl;
 }