[Qemu-devel] [PATCH v1 0/7] QOMify pflash_cfi0x + PL353 for Xilinx Zynq

[Qemu-devel] [PATCH v1 0/7] QOMify pflash_cfi0x + PL353 for Xilinx Zynq

[Peter Crosthwaite]

This series adds the PL353 to Xilinx Zynq with both NAND and pflashes attached. Had to QOMify the pflash_cfi0x devices to get them working with PL35x in the least hackish way. Regression tested pflash_cfi_01 using petalogix-ml605 and pflash_cfi_02 tested using zynq. Further testing by clients of the pflash would be appreciated.

The pl35x is setup as a generalisation of all the pl35x family (i.e. it implements all of PL351-pl354). Once we get to actually implementing some of the register ops of this SRAM interface we could add this to vexpress for its PL354. The PL35x is incomplete (see the FIXME:s) at the moment but im pushing for this now as the more conterversial QOM-entangled aspects of this device model are encapsulated by this series. The device does also fully work for Linux.

Edgar E. Iglesias (1):
  nand: Reset addressing after READSTATUS.

Peter Crosthwaite (6):
  pflash_cfi0x: remove unused base field
  pflash_cfi01: remove unused total_len field
  pflash_cfi0x: QOMified
  sysbus/sysbus_mmio_map: parameterise mapped region
  hw: Model of Primecell pl35x mem controller
  xilinx_zynq: add pl353

 default-configs/arm-softmmu.mak |    1 +
 hw/Makefile.objs                |    1 +
 hw/nand.c                       |    6 +
 hw/pflash_cfi01.c               |  146 ++++++++++++++-----
 hw/pflash_cfi02.c               |  156 +++++++++++++++-----
 hw/pl35x.c                      |  299 +++++++++++++++++++++++++++++++++++++++
 hw/sysbus.c                     |   11 +-
 hw/sysbus.h                     |    2 +
 hw/xilinx_zynq.c                |   49 ++++++-
 9 files changed, 582 insertions(+), 89 deletions(-)
 create mode 100644 hw/pl35x.c

[Qemu-devel] [PATCH v1 1/7] pflash_cfi0x: remove unused base field

[Peter Crosthwaite]

This field is completely unused. The base address should also be abstracted
away from the device anyway. Removed.

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

 hw/pflash_cfi01.c |    2 --
 hw/pflash_cfi02.c |    4 +---
 2 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
index 3b437da..4f3f5f0 100644
--- a/hw/pflash_cfi01.c
+++ b/hw/pflash_cfi01.c
@@ -61,7 +61,6 @@ do {                                               \
 
 struct pflash_t {
     BlockDriverState *bs;
-    target_phys_addr_t base;
     target_phys_addr_t sector_len;
     target_phys_addr_t total_len;
     int width;
@@ -594,7 +593,6 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base,
     }
 
     pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
-    pfl->base = base;
     pfl->sector_len = sector_len;
     pfl->total_len = total_len;
     pfl->width = width;
diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
index 39337ec..43fb3a4 100644
--- a/hw/pflash_cfi02.c
+++ b/hw/pflash_cfi02.c
@@ -56,7 +56,6 @@ do {                                               \
 
 struct pflash_t {
     BlockDriverState *bs;
-    target_phys_addr_t base;
     uint32_t sector_len;
     uint32_t chip_len;
     int mappings;
@@ -602,7 +601,6 @@ pflash_t *pflash_cfi02_register(target_phys_addr_t base,
         name, size);
     vmstate_register_ram(&pfl->orig_mem, qdev);
     pfl->storage = memory_region_get_ram_ptr(&pfl->orig_mem);
-    pfl->base = base;
     pfl->chip_len = chip_len;
     pfl->mappings = nb_mappings;
     pfl->bs = bs;
@@ -618,7 +616,7 @@ pflash_t *pflash_cfi02_register(target_phys_addr_t base,
 
     pflash_setup_mappings(pfl);
     pfl->rom_mode = 1;
-    memory_region_add_subregion(get_system_memory(), pfl->base, &pfl->mem);
+    memory_region_add_subregion(get_system_memory(), base, &pfl->mem);
 
     if (pfl->bs) {
         pfl->ro = bdrv_is_read_only(pfl->bs);
-- 
1.7.0.4

[Qemu-devel] [PATCH v1 2/7] pflash_cfi01: remove unused total_len field

[Peter Crosthwaite]

This field is completely unused.

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

 hw/pflash_cfi01.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
index 4f3f5f0..ebc8a57 100644
--- a/hw/pflash_cfi01.c
+++ b/hw/pflash_cfi01.c
@@ -62,7 +62,6 @@ do {                                               \
 struct pflash_t {
     BlockDriverState *bs;
     target_phys_addr_t sector_len;
-    target_phys_addr_t total_len;
     int width;
     int wcycle; /* if 0, the flash is read normally */
     int bypass;
@@ -594,7 +593,6 @@ 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->total_len = total_len;
     pfl->width = width;
     pfl->wcycle = 0;
     pfl->cmd = 0;
-- 
1.7.0.4

[Qemu-devel] [PATCH v1 3/7] pflash_cfi0x: QOMified

[Peter Crosthwaite]

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(-)

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;
 }
-- 
1.7.0.4

[Qemu-devel] [PATCH v1 4/7] sysbus/sysbus_mmio_map: parameterise mapped region

[Peter Crosthwaite]

Add a variant to sysbus_mmio_map that allow specifying a target memory region.
The requested device memory region is mapped within the argument target memory
region rather than the default (get_system_memory()). Behaviour of original
sysbus_mmio_map remains unchanged.

The will probably go away or morph into something else with Anthony sysbus purge
so its intended to be a bridging patch until those refactorings go live.

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

 hw/sysbus.c |   11 ++++++++---
 hw/sysbus.h |    2 ++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/sysbus.c b/hw/sysbus.c
index c173840..bd45183 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -48,7 +48,8 @@ void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq)
     }
 }
 
-void sysbus_mmio_map(SysBusDevice *dev, int n, target_phys_addr_t addr)
+void sysbus_mmio_map_to_region(SysBusDevice *dev, int n,
+                               target_phys_addr_t addr, MemoryRegion *region)
 {
     assert(n >= 0 && n < dev->num_mmio);
 
@@ -58,14 +59,18 @@ void sysbus_mmio_map(SysBusDevice *dev, int n, target_phys_addr_t addr)
     }
     if (dev->mmio[n].addr != (target_phys_addr_t)-1) {
         /* Unregister previous mapping.  */
-        memory_region_del_subregion(get_system_memory(), dev->mmio[n].memory);
+        memory_region_del_subregion(region, dev->mmio[n].memory);
     }
     dev->mmio[n].addr = addr;
-    memory_region_add_subregion(get_system_memory(),
+    memory_region_add_subregion(region,
                                 addr,
                                 dev->mmio[n].memory);
 }
 
+void sysbus_mmio_map(SysBusDevice *dev, int n, target_phys_addr_t addr)
+{
+    sysbus_mmio_map_to_region(dev, n, addr, get_system_memory());
+}
 
 /* Request an IRQ source.  The actual IRQ object may be populated later.  */
 void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
diff --git a/hw/sysbus.h b/hw/sysbus.h
index acfbcfb..9cdb2b4 100644
--- a/hw/sysbus.h
+++ b/hw/sysbus.h
@@ -56,6 +56,8 @@ void sysbus_init_ioports(SysBusDevice *dev, pio_addr_t ioport, pio_addr_t size);
 
 
 void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq);
+void sysbus_mmio_map_to_region(SysBusDevice *dev, int n,
+                               target_phys_addr_t addr, MemoryRegion *region);
 void sysbus_mmio_map(SysBusDevice *dev, int n, target_phys_addr_t addr);
 void sysbus_add_memory(SysBusDevice *dev, target_phys_addr_t addr,
                        MemoryRegion *mem);
-- 
1.7.0.4

[Qemu-devel] [PATCH v1 5/7] hw: Model of Primecell pl35x mem controller

[Peter Crosthwaite]

Initial device model for the pl35x series of memory controllers. The SRAM
interface is just implemented as a passthrough using memory regions. NAND
interfaces are modelled.

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

 default-configs/arm-softmmu.mak |    1 +
 hw/Makefile.objs                |    1 +
 hw/pl35x.c                      |  299 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 301 insertions(+), 0 deletions(-)
 create mode 100644 hw/pl35x.c

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index 2f1a5c9..b24bf68 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -41,6 +41,7 @@ CONFIG_PL110=y
 CONFIG_PL181=y
 CONFIG_PL190=y
 CONFIG_PL310=y
+CONFIG_PL35X=y
 CONFIG_CADENCE=y
 CONFIG_XGMAC=y
 
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index 854faa9..502f139 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -88,6 +88,7 @@ common-obj-$(CONFIG_PL110) += pl110.o
 common-obj-$(CONFIG_PL181) += pl181.o
 common-obj-$(CONFIG_PL190) += pl190.o
 common-obj-$(CONFIG_PL310) += arm_l2x0.o
+common-obj-$(CONFIG_PL35X) += pl35x.o
 common-obj-$(CONFIG_VERSATILE_PCI) += versatile_pci.o
 common-obj-$(CONFIG_VERSATILE_I2C) += versatile_i2c.o
 common-obj-$(CONFIG_CADENCE) += cadence_uart.o
diff --git a/hw/pl35x.c b/hw/pl35x.c
new file mode 100644
index 0000000..ec3d194
--- /dev/null
+++ b/hw/pl35x.c
@@ -0,0 +1,299 @@
+/*
+ * QEMU model of Primcell PL353
+ *
+ * Copyright (c) 2012 Xilinx Inc.
+ * Copyright (c) 2012 Peter Crosthwaite <peter.crosthwaite@xilinx.com>.
+ * Copyright (c) 2011 Edgar E. Iglesias.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "hw.h"
+#include "qemu-timer.h"
+#include "sysbus.h"
+#include "sysemu.h"
+#include "flash.h"
+
+#ifdef PL35X_ERR_DEBUG
+#define DB_PRINT(...) do { \
+    fprintf(stderr,  ": %s: ", __func__); \
+    fprintf(stderr, ## __VA_ARGS__); \
+    } while (0);
+#else
+    #define DB_PRINT(...)
+#endif
+
+typedef struct PL35xItf {
+    MemoryRegion mm;
+    DeviceState *dev;
+    uint8_t nand_pending_addr_cycles;
+} PL35xItf;
+
+typedef struct PL35xState {
+    SysBusDevice busdev;
+    MemoryRegion mmio;
+
+    /* FIXME: add support for multiple chip selects/interface */
+
+    PL35xItf itf[2];
+
+    /* FIXME: add Interrupt support */
+
+    /* FIXME: add ECC support */
+
+    uint8_t x; /* the "x" in pl35x */
+} PL35xState;
+
+static uint64_t pl35x_read(void *opaque, target_phys_addr_t addr,
+                         unsigned int size)
+{
+    PL35xState *s = opaque;
+    uint32_t r = 0;
+    int rdy;
+
+    addr >>= 2;
+    switch (addr) {
+    case 0x0:
+        if (s->itf[0].dev && object_dynamic_cast(OBJECT(s->itf[0].dev),
+                                                      "nand")) {
+            nand_getpins(s->itf[0].dev, &rdy);
+            r |= (!!rdy) << 5;
+        }
+        if (s->itf[1].dev && object_dynamic_cast(OBJECT(s->itf[1].dev),
+                                                      "nand")) {
+            nand_getpins(s->itf[1].dev, &rdy);
+            r |= (!!rdy) << 6;
+        }
+        break;
+    default:
+        DB_PRINT("Unimplemented SMC read access reg=" TARGET_FMT_plx "\n",
+                 addr * 4);
+        break;
+    }
+    return r;
+}
+
+static void pl35x_write(void *opaque, target_phys_addr_t addr, uint64_t value64,
+                      unsigned int size)
+{
+    DB_PRINT("addr=%x v=%x\n", addr, (unsigned)value64);
+    addr >>= 2;
+    /* FIXME: implement */
+    DB_PRINT("Unimplemented SMC write access reg=" TARGET_FMT_plx "\n",
+                 addr * 4);
+}
+
+static const MemoryRegionOps pl35x_ops = {
+    .read = pl35x_read,
+    .write = pl35x_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4
+    }
+};
+
+static uint64_t nand_read(void *opaque, target_phys_addr_t addr,
+                           unsigned int size)
+{
+    PL35xItf *s = opaque;
+    unsigned int len = size;
+    int shift = 0;
+    uint32_t r = 0;
+
+    while (len--) {
+        uint8_t r8;
+
+        r8 = nand_getio(s->dev) & 0xff;
+        r |= r8 << shift;
+        shift += 8;
+    }
+    DB_PRINT("addr=%x r=%x size=%d\n", (unsigned)addr, r, size);
+    return r;
+}
+
+static void nand_write(void *opaque, target_phys_addr_t addr, uint64_t value64,
+                       unsigned int size)
+{
+    struct PL35xItf *s = opaque;
+    bool data_phase, ecmd_valid;
+    unsigned int addr_cycles = 0;
+    uint16_t start_cmd, end_cmd;
+    uint32_t value = value64;
+    uint32_t nandaddr = value;
+
+    DB_PRINT("addr=%x v=%x size=%d\n", addr, value, size);
+
+    /* Decode the various signals.  */
+    data_phase = (addr >> 19) & 1;
+    ecmd_valid = (addr >> 20) & 1;
+    start_cmd = (addr >> 3) & 0xff;
+    end_cmd = (addr >> 11) & 0xff;
+    if (!data_phase) {
+        addr_cycles = (addr >> 21) & 7;
+    }
+
+    if (!data_phase) {
+        DB_PRINT("start_cmd=%x end_cmd=%x (valid=%d) acycl=%d\n",
+                start_cmd, end_cmd, ecmd_valid, addr_cycles);
+    }
+
+    /* Writing data to the NAND.  */
+    if (data_phase) {
+        nand_setpins(s->dev, 0, 0, 0, 1, 0);
+        while (size--) {
+            nand_setio(s->dev, value & 0xff);
+            value >>= 8;
+        }
+    }
+
+    /* Writing Start cmd.  */
+    if (!data_phase && !s->nand_pending_addr_cycles) {
+        nand_setpins(s->dev, 1, 0, 0, 1, 0);
+        nand_setio(s->dev, start_cmd);
+    }
+
+    if (!addr_cycles) {
+        s->nand_pending_addr_cycles = 0;
+    }
+    if (s->nand_pending_addr_cycles) {
+        addr_cycles = s->nand_pending_addr_cycles;
+        s->nand_pending_addr_cycles = 0;
+    }
+    if (addr_cycles > 4) {
+        s->nand_pending_addr_cycles = addr_cycles - 4;
+        addr_cycles = 4;
+    }
+    while (addr_cycles) {
+        nand_setpins(s->dev, 0, 1, 0, 1, 0);
+        DB_PRINT("nand cycl=%d addr=%x\n", addr_cycles, nandaddr & 0xff);
+        nand_setio(s->dev, nandaddr & 0xff);
+        nandaddr >>= 8;
+        addr_cycles--;
+    }
+
+    /* Writing commands. One or two (Start and End).  */
+    if (ecmd_valid && !s->nand_pending_addr_cycles) {
+        nand_setpins(s->dev, 1, 0, 0, 1, 0);
+        nand_setio(s->dev, end_cmd);
+    }
+}
+
+static const MemoryRegionOps nand_ops = {
+    .read = nand_read,
+    .write = nand_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 4
+    }
+};
+
+static void pl35x_init_sram(SysBusDevice *dev, PL35xItf *itf)
+{
+    /* d Just needs to be a valid sysbus device with at least one memory
+     * region
+     */
+    SysBusDevice *sbd = SYS_BUS_DEVICE(itf->dev);
+
+    memory_region_init(&itf->mm, "pl35x.sram", 1 << 24);
+    sysbus_mmio_map_to_region(sbd, 0, 0, &itf->mm);
+    sysbus_init_mmio(dev, &itf->mm);
+}
+
+static void pl35x_init_nand(SysBusDevice *dev, PL35xItf *itf)
+{
+    /* d Must be a NAND flash */
+    object_dynamic_cast_assert(OBJECT(itf->dev), "nand");
+
+    memory_region_init_io(&itf->mm, &nand_ops, itf, "pl35x.nand", 1 << 24);
+    sysbus_init_mmio(dev, &itf->mm);
+}
+
+static int pl35x_init(SysBusDevice *dev)
+{
+    PL35xState *s = FROM_SYSBUS(typeof(*s), dev);
+    int itfn = 0;
+
+    memory_region_init_io(&s->mmio, &pl35x_ops, s, "pl35x_io", 0x1000);
+    sysbus_init_mmio(dev, &s->mmio);
+    if (s->x != 1) { /* everything cept PL351 has at least one SRAM */
+        pl35x_init_sram(dev, &s->itf[itfn]);
+        itfn++;
+    }
+    if (s->x & 0x1) { /* PL351 and PL353 have NAND */
+        pl35x_init_nand(dev, &s->itf[itfn]);
+    } else if (s->x == 4) { /* PL354 has a second SRAM */
+        pl35x_init_sram(dev, &s->itf[itfn]);
+    }
+    return 0;
+}
+static void pl35x_initfn(Object *obj)
+{
+    PL35xState *s = FROM_SYSBUS(typeof(*s), SYS_BUS_DEVICE(obj));
+    Error *errp = NULL;
+
+    object_property_add_link(obj, "dev0", TYPE_DEVICE,
+                             (Object **)&s->itf[0].dev, &errp);
+    assert_no_error(errp);
+    object_property_add_link(obj, "dev1", TYPE_DEVICE,
+                             (Object **)&s->itf[1].dev, &errp);
+    assert_no_error(errp);
+}
+
+static Property pl35x_properties[] = {
+    DEFINE_PROP_UINT8("x", PL35xState, x, 3),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static const VMStateDescription vmstate_pl35x = {
+    .name = "pl35x",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(itf[0].nand_pending_addr_cycles, PL35xState),
+        VMSTATE_UINT8(itf[1].nand_pending_addr_cycles, PL35xState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void pl35x_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+    k->init = pl35x_init;
+    dc->props = pl35x_properties;
+    dc->vmsd = &vmstate_pl35x;
+}
+
+static TypeInfo pl35x_info = {
+    .name           = "arm.pl35x",
+    .parent         = TYPE_SYS_BUS_DEVICE,
+    .instance_size  = sizeof(PL35xState),
+    .class_init     = pl35x_class_init,
+    .instance_init  = pl35x_initfn,
+};
+
+static void pl35x_register_types(void)
+{
+    type_register_static(&pl35x_info);
+}
+
+type_init(pl35x_register_types)
-- 
1.7.0.4

[Qemu-devel] [PATCH v1 6/7] xilinx_zynq: add pl353

[Peter Crosthwaite]

Add the pl353 memory controller with both NAND and parallel flashes
attached.

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

 hw/xilinx_zynq.c |   49 +++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/hw/xilinx_zynq.c b/hw/xilinx_zynq.c
index fd46ba2..7f6faf0 100644
--- a/hw/xilinx_zynq.c
+++ b/hw/xilinx_zynq.c
@@ -120,14 +120,47 @@ static void zynq_init(ram_addr_t ram_size, const char *boot_device,
     vmstate_register_ram_global(ocm_ram);
     memory_region_add_subregion(address_space_mem, 0xFFFC0000, ocm_ram);
 
-    DriveInfo *dinfo = drive_get(IF_PFLASH, 0, 0);
-
-    /* AMD */
-    pflash_cfi02_register(0xe2000000, NULL, "zynq.pflash", FLASH_SIZE,
-                          dinfo ? dinfo->bdrv : NULL, FLASH_SECTOR_SIZE,
-                          FLASH_SIZE/FLASH_SECTOR_SIZE, 1,
-                          1, 0x0066, 0x0022, 0x0000, 0x0000, 0x0555, 0x2aa,
-                              0);
+    /* pl353 */
+    dev = qdev_create(NULL, "arm.pl35x");
+    /* FIXME: handle this somewhere central */
+    object_property_add_child(container_get(qdev_get_machine(), "/unattached"),
+                              "pl353", OBJECT(dev), NULL);
+    qdev_prop_set_uint8(dev, "x", 3);
+    {
+        DriveInfo *dinfo = drive_get_next(IF_PFLASH);
+        BlockDriverState *bs =  dinfo ? dinfo->bdrv : NULL;
+        DeviceState *att_dev = qdev_create(NULL, "cfi.pflash02");
+        Error *errp = NULL;
+
+        if (bs && qdev_prop_set_drive(att_dev, "bdrv", bs)) {
+            abort();
+        }
+        qdev_prop_set_uint32(att_dev, "nb_blocs",
+                             FLASH_SIZE/FLASH_SECTOR_SIZE);
+        qdev_prop_set_uint32(att_dev, "sector_len", FLASH_SECTOR_SIZE);
+        qdev_prop_set_uint8(att_dev, "width", 1);
+        qdev_prop_set_uint8(att_dev, "mappings", 1);
+        qdev_prop_set_uint8(att_dev, "be", 0);
+        qdev_prop_set_uint16(att_dev, "id0", 0x0066);
+        qdev_prop_set_uint16(att_dev, "id1", 0x0022);
+        qdev_prop_set_uint16(att_dev, "id2", 0x0000);
+        qdev_prop_set_uint16(att_dev, "id3", 0x0000);
+        qdev_prop_set_uint16(att_dev, "unlock_addr0", 0x0aaa);
+        qdev_prop_set_uint16(att_dev, "unlock_addr1", 0x0555);
+        qdev_init_nofail(att_dev);
+        object_property_set_link(OBJECT(dev), OBJECT(att_dev), "dev0", &errp);
+        assert_no_error(errp);
+
+        dinfo = drive_get_next(IF_PFLASH);
+        att_dev = nand_init(dinfo ? dinfo->bdrv : NULL, NAND_MFR_STMICRO, 0xaa);
+        object_property_set_link(OBJECT(dev), OBJECT(att_dev), "dev1", &errp);
+        assert_no_error(errp);
+    }
+    qdev_init_nofail(dev);
+    busdev = sysbus_from_qdev(dev);
+    sysbus_mmio_map(busdev, 0, 0xe000e000);
+    sysbus_mmio_map(busdev, 1, 0xe2000000);
+    sysbus_mmio_map(busdev, 2, 0xe1000000);
 
     dev = qdev_create(NULL, "xilinx,zynq_slcr");
     qdev_init_nofail(dev);
-- 
1.7.0.4

[Qemu-devel] [PATCH v1 7/7] nand: Reset addressing after READSTATUS.

[Peter Crosthwaite]

From: Edgar E. Iglesias <edgar.iglesias@gmail.com>

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
---

 hw/nand.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/hw/nand.c b/hw/nand.c
index 01f3ada..f931d0c 100644
--- a/hw/nand.c
+++ b/hw/nand.c
@@ -478,6 +478,12 @@ void nand_setio(DeviceState *dev, uint32_t value)
     int i;
     NANDFlashState *s = (NANDFlashState *) dev;
     if (!s->ce && s->cle) {
+        if (s->cmd == NAND_CMD_READSTATUS) {
+            s->addr = 0;
+            s->addrlen = 0;
+            s->iolen = 0;
+        }
+
         if (nand_flash_ids[s->chip_id].options & NAND_SAMSUNG_LP) {
             if (s->cmd == NAND_CMD_READ0 && value == NAND_CMD_LPREAD2)
                 return;
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v1 1/7] pflash_cfi0x: remove unused base field

[Peter Maydell]

On 19 October 2012 07:40, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> This field is completely unused. The base address should also be abstracted
> away from the device anyway. Removed.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH v1 2/7] pflash_cfi01: remove unused total_len field

[Peter Maydell]

On 19 October 2012 07:40, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> This field is completely unused.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH v1 4/7] sysbus/sysbus_mmio_map: parameterise mapped region

[Peter Maydell]

On 19 October 2012 07:40, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Add a variant to sysbus_mmio_map that allow specifying a target memory region.
> The requested device memory region is mapped within the argument target memory
> region rather than the default (get_system_memory()). Behaviour of original
> sysbus_mmio_map remains unchanged.

If you want to take a sysbus device and map one of its mmio regions into
something other than the system memory space, the usual approach is to
use sysbus_mmio_get_region() to get the MemoryRegion* and then you can
do what you want with it.

-- PMM

Re: [Qemu-devel] [PATCH v1 4/7] sysbus/sysbus_mmio_map: parameterise mapped region

[Peter Crosthwaite]

On Fri, Oct 19, 2012 at 6:06 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 19 October 2012 07:40, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> Add a variant to sysbus_mmio_map that allow specifying a target memory region.
>> The requested device memory region is mapped within the argument target memory
>> region rather than the default (get_system_memory()). Behaviour of original
>> sysbus_mmio_map remains unchanged.
>
> If you want to take a sysbus device and map one of its mmio regions into
> something other than the system memory space, the usual approach is to
> use sysbus_mmio_get_region() to get the MemoryRegion* and then you can
> do what you want with it.
>

Fixed in v2. This patch will be removed from the series.

Regards,
Peter

> -- PMM
>

Re: [Qemu-devel] [PATCH v1 3/7] pflash_cfi0x: QOMified

[Paolo Bonzini]

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;
>  }
> 

Re: [Qemu-devel] [PATCH v1 3/7] pflash_cfi0x: QOMified

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH v1 6/7] xilinx_zynq: add pl353

[Peter Maydell]

On 19 October 2012 07:40, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Add the pl353 memory controller with both NAND and parallel flashes
> attached.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/xilinx_zynq.c |   49 +++++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 41 insertions(+), 8 deletions(-)
>
> diff --git a/hw/xilinx_zynq.c b/hw/xilinx_zynq.c
> index fd46ba2..7f6faf0 100644
> --- a/hw/xilinx_zynq.c
> +++ b/hw/xilinx_zynq.c
> @@ -120,14 +120,47 @@ static void zynq_init(ram_addr_t ram_size, const char *boot_device,
>      vmstate_register_ram_global(ocm_ram);
>      memory_region_add_subregion(address_space_mem, 0xFFFC0000, ocm_ram);
>
> -    DriveInfo *dinfo = drive_get(IF_PFLASH, 0, 0);
> -
> -    /* AMD */
> -    pflash_cfi02_register(0xe2000000, NULL, "zynq.pflash", FLASH_SIZE,
> -                          dinfo ? dinfo->bdrv : NULL, FLASH_SECTOR_SIZE,
> -                          FLASH_SIZE/FLASH_SECTOR_SIZE, 1,
> -                          1, 0x0066, 0x0022, 0x0000, 0x0000, 0x0555, 0x2aa,
> -                              0);
> +    /* pl353 */
> +    dev = qdev_create(NULL, "arm.pl35x");
> +    /* FIXME: handle this somewhere central */
> +    object_property_add_child(container_get(qdev_get_machine(), "/unattached"),
> +                              "pl353", OBJECT(dev), NULL);

So, er, what's this for? Whatever it is, as you say it needs to be
done properly, not randomly in the board model...

-- PMM

Re: [Qemu-devel] [PATCH v1 7/7] nand: Reset addressing after READSTATUS.

[Peter Maydell]

On 19 October 2012 07:40, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> From: Edgar E. Iglesias <edgar.iglesias@gmail.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> ---
>
>  hw/nand.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/hw/nand.c b/hw/nand.c
> index 01f3ada..f931d0c 100644
> --- a/hw/nand.c
> +++ b/hw/nand.c
> @@ -478,6 +478,12 @@ void nand_setio(DeviceState *dev, uint32_t value)
>      int i;
>      NANDFlashState *s = (NANDFlashState *) dev;
>      if (!s->ce && s->cle) {
> +        if (s->cmd == NAND_CMD_READSTATUS) {
> +            s->addr = 0;
> +            s->addrlen = 0;
> +            s->iolen = 0;
> +        }
> +

I find the NAND chip datasheets remarkably hard to interpret, but
I'm not convinced this patch is the right thing. Can you provide
some rationale/justification, please? (ideally with reference to
datasheets...)

Some of the code in the nand device definitely looks suspect though;
and I'm having trouble making a consistent mapping between how the
data sheets describe things and what our code does.  Why do
we handle some of the NAND commands directly in nand_setio but
delegate the rest to nand_command()? Should we really allow a
RANDOMREAD2 without checking that the previous command was
RANDOMREAD1? Can we safely just return after the command-handling
bit of nand_setio() or do we really need to allow the caller to
tell us to treat the data value as both a command and address
simultaneously if it wants to assert cle and ale at once? etc.

Basically I don't really understand this code, so I'm afraid
patches to it have to come with more rationale than usual attached
to them, because I can't fill in the gaps myself :-(

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 7/7] nand: Reset addressing after READSTATUS.

[Edgar E. Iglesias]

On Fri, Oct 19, 2012 at 12:59:49PM +0100, Peter Maydell wrote:
> On 19 October 2012 07:40, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
> > From: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > ---
> >
> >  hw/nand.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/nand.c b/hw/nand.c
> > index 01f3ada..f931d0c 100644
> > --- a/hw/nand.c
> > +++ b/hw/nand.c
> > @@ -478,6 +478,12 @@ void nand_setio(DeviceState *dev, uint32_t value)
> >      int i;
> >      NANDFlashState *s = (NANDFlashState *) dev;
> >      if (!s->ce && s->cle) {
> > +        if (s->cmd == NAND_CMD_READSTATUS) {
> > +            s->addr = 0;
> > +            s->addrlen = 0;
> > +            s->iolen = 0;
> > +        }
> > +
> 
> I find the NAND chip datasheets remarkably hard to interpret, but
> I'm not convinced this patch is the right thing. Can you provide
> some rationale/justification, please? (ideally with reference to
> datasheets...)

This is patch is quite old (several years). At the time modern linux kernels
stopped working with our nand model in some cases. Some patch to our
nand model broke something. I recall trying to make some sense out of
it and this was the closest I got..

I don't know what the state it is today nor do I remember the exact
circumstances on which the bug was trigged. Maybe Peter C has more
info?

Cheers,
Edgar

Re: [Qemu-devel] [PATCH v1 6/7] xilinx_zynq: add pl353

[Peter Crosthwaite]

On Fri, Oct 19, 2012 at 8:32 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 19 October 2012 07:40, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> Add the pl353 memory controller with both NAND and parallel flashes
>> attached.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/xilinx_zynq.c |   49 +++++++++++++++++++++++++++++++++++++++++--------
>>  1 files changed, 41 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/xilinx_zynq.c b/hw/xilinx_zynq.c
>> index fd46ba2..7f6faf0 100644
>> --- a/hw/xilinx_zynq.c
>> +++ b/hw/xilinx_zynq.c
>> @@ -120,14 +120,47 @@ static void zynq_init(ram_addr_t ram_size, const char *boot_device,
>>      vmstate_register_ram_global(ocm_ram);
>>      memory_region_add_subregion(address_space_mem, 0xFFFC0000, ocm_ram);
>>
>> -    DriveInfo *dinfo = drive_get(IF_PFLASH, 0, 0);
>> -
>> -    /* AMD */
>> -    pflash_cfi02_register(0xe2000000, NULL, "zynq.pflash", FLASH_SIZE,
>> -                          dinfo ? dinfo->bdrv : NULL, FLASH_SECTOR_SIZE,
>> -                          FLASH_SIZE/FLASH_SECTOR_SIZE, 1,
>> -                          1, 0x0066, 0x0022, 0x0000, 0x0000, 0x0555, 0x2aa,
>> -                              0);
>> +    /* pl353 */
>> +    dev = qdev_create(NULL, "arm.pl35x");
>> +    /* FIXME: handle this somewhere central */
>> +    object_property_add_child(container_get(qdev_get_machine(), "/unattached"),
>> +                              "pl353", OBJECT(dev), NULL);
>
> So, er, what's this for? Whatever it is, as you say it needs to be
> done properly, not randomly in the board model...
>

This is the well known link with no canonical path issue, that has
already been discussed on list a few times now. Im under the imression
that sone of the Qdev/QOM people are already on the case here. Andreas
in all the qdev QOM re-factorings going on is there a clear resolution
to this planned?

Regards,
Peter


> -- PMM
>

Re: [Qemu-devel] [PATCH v1 3/7] pflash_cfi0x: QOMified

[Peter Crosthwaite]

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
>

Re: [Qemu-devel] [PATCH v1 7/7] nand: Reset addressing after READSTATUS.

[Peter Crosthwaite]

On Fri, Oct 19, 2012 at 10:18 PM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> On Fri, Oct 19, 2012 at 12:59:49PM +0100, Peter Maydell wrote:
>> On 19 October 2012 07:40, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>> > From: Edgar E. Iglesias <edgar.iglesias@gmail.com>
>> >
>> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
>> > ---
>> >
>> >  hw/nand.c |    6 ++++++
>> >  1 files changed, 6 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/hw/nand.c b/hw/nand.c
>> > index 01f3ada..f931d0c 100644
>> > --- a/hw/nand.c
>> > +++ b/hw/nand.c
>> > @@ -478,6 +478,12 @@ void nand_setio(DeviceState *dev, uint32_t value)
>> >      int i;
>> >      NANDFlashState *s = (NANDFlashState *) dev;
>> >      if (!s->ce && s->cle) {
>> > +        if (s->cmd == NAND_CMD_READSTATUS) {
>> > +            s->addr = 0;
>> > +            s->addrlen = 0;
>> > +            s->iolen = 0;
>> > +        }
>> > +
>>
>> I find the NAND chip datasheets remarkably hard to interpret, but
>> I'm not convinced this patch is the right thing. Can you provide
>> some rationale/justification, please? (ideally with reference to
>> datasheets...)
>
> This is patch is quite old (several years). At the time modern linux kernels
> stopped working with our nand model in some cases. Some patch to our
> nand model broke something. I recall trying to make some sense out of
> it and this was the closest I got..
>
> I don't know what the state it is today nor do I remember the exact
> circumstances on which the bug was trigged. Maybe Peter C has more
> info?
>

Not really. Im fairly lost as well on the data-sheet front but AFAICT
what actually happens here is an undefined behaviour. Ill have to dig
deeper on my tests to see if its a problem. Could just be a hangover
from an ancient kernel bug and this patch is unneeded.

Regards,
Peter

> Cheers,
> Edgar
>