[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v1 3/7] pflash_cfi0x: QOMified
From: |
Peter Crosthwaite |
Subject: |
Re: [Qemu-devel] [PATCH v1 3/7] pflash_cfi0x: QOMified |
Date: |
Mon, 22 Oct 2012 16:46:41 +1000 |
On Fri, Oct 19, 2012 at 8:24 PM, Peter Maydell <address@hidden> wrote:
> On 19 October 2012 07:40, Peter Crosthwaite
> <address@hidden> 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 <address@hidden>
>
> 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
>
- [Qemu-devel] [PATCH v1 0/7] QOMify pflash_cfi0x + PL353 for Xilinx Zynq, Peter Crosthwaite, 2012/10/19
- [Qemu-devel] [PATCH v1 1/7] pflash_cfi0x: remove unused base field, Peter Crosthwaite, 2012/10/19
- [Qemu-devel] [PATCH v1 2/7] pflash_cfi01: remove unused total_len field, Peter Crosthwaite, 2012/10/19
- [Qemu-devel] [PATCH v1 3/7] pflash_cfi0x: QOMified, Peter Crosthwaite, 2012/10/19
- [Qemu-devel] [PATCH v1 4/7] sysbus/sysbus_mmio_map: parameterise mapped region, Peter Crosthwaite, 2012/10/19
- [Qemu-devel] [PATCH v1 5/7] hw: Model of Primecell pl35x mem controller, Peter Crosthwaite, 2012/10/19
- [Qemu-devel] [PATCH v1 6/7] xilinx_zynq: add pl353, Peter Crosthwaite, 2012/10/19
- [Qemu-devel] [PATCH v1 7/7] nand: Reset addressing after READSTATUS., Peter Crosthwaite, 2012/10/19