| Message ID | ddebf66ebc765be0adc04fd6a5b979f543ed482a.1350619775.git.peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
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; > } >
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
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 >
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; }
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(-)