[Qemu-devel] [PATCH v2 0/6] QOMify pflash_cfi0x + PL353 for Xilinx Zynq

[Qemu-devel] [PATCH v2 0/6] 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.

Changlog:
Changed from v1:
Address PMM and Paolos Reviews (P3).
Fixed a compile error in in pflash when debug was turned on (P6)
Removed NAND READ_STATUS address reset patch (fomerly P6)

Peter Crosthwaite (6):
  pflash_cfi0x: remove unused base field
  pflash_cfi01: remove unused total_len field
  pflash_cfi0x: QOMified
  hw: Model of Primecell pl35x mem controller
  xilinx_zynq: add pl353
  pflash_cfi01: Fix debug mode printfery

 default-configs/arm-softmmu.mak |    1 +
 hw/Makefile.objs                |    1 +
 hw/pflash_cfi01.c               |  149 ++++++++++++++------
 hw/pflash_cfi02.c               |  162 +++++++++++++++------
 hw/pl35x.c                      |  299 +++++++++++++++++++++++++++++++++++++++
 hw/xilinx_zynq.c                |   50 ++++++-
 6 files changed, 560 insertions(+), 102 deletions(-)
 create mode 100644 hw/pl35x.c

[Qemu-devel] [PATCH v2 1/6] 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---

 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 v2 2/6] pflash_cfi01: remove unused total_len field

[Peter Crosthwaite]

This field is completely unused.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---

 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 v2 3/6] 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>
---
changed from v1:
Removed union usages (PMM review)
Changed target_phys_addr type for sector_len to uint64 (Pao + PMM review)
Made property names nicer (Pao + PMM review)
Set name strings properly

 hw/pflash_cfi01.c |  142 +++++++++++++++++++++++++++++++++--------------
 hw/pflash_cfi02.c |  160 +++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 215 insertions(+), 87 deletions(-)

diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
index ebc8a57..6164a97 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,29 @@ 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 */
+    uint64_t sector_len;
+    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 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;
 };
 
@@ -166,11 +175,11 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
     case 0x90:
         switch (boff) {
         case 0:
-            ret = pfl->ident[0] << 8 | pfl->ident[1];
+            ret = pfl->ident0 << 8 | pfl->ident1;
             DPRINTF("%s: Manufacturer Code %04x\n", __func__, ret);
             break;
         case 1:
-            ret = pfl->ident[2] << 8 | pfl->ident[3];
+            ret = pfl->ident2 << 8 | pfl->ident3;
             DPRINTF("%s: Device ID Code %04x\n", __func__, ret);
             break;
         default:
@@ -277,9 +286,8 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
             p = pfl->storage;
             offset &= ~(pfl->sector_len - 1);
 
-            DPRINTF("%s: block erase at " TARGET_FMT_plx " bytes "
-                    TARGET_FMT_plx "\n",
-                    __func__, offset, pfl->sector_len);
+            DPRINTF("%s: block erase at " TARGET_FMT_plx " bytes %x\n",
+                    __func__, offset, (unsigned)pfl->sector_len);
 
             if (!pfl->ro) {
                 memset(p + offset, 0xff, pfl->sector_len);
@@ -541,19 +549,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;
-    target_phys_addr_t total_len;
+    pflash_t *pfl = FROM_SYSBUS(typeof(*pfl), dev);
+    uint64_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 +564,22 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base,
         return NULL;
 #endif
 
-    pfl = g_malloc0(sizeof(pflash_t));
-
     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 +589,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 +640,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 +651,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 +676,75 @@ 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("drive", struct pflash_t, bs),
+    DEFINE_PROP_UINT32("num-blocks", struct pflash_t, nb_blocs, 0),
+    DEFINE_PROP_UINT64("sector-length", struct pflash_t, sector_len, 0),
+    DEFINE_PROP_UINT8("width", struct pflash_t, width, 0),
+    DEFINE_PROP_UINT8("big-endian", 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, "drive", bs)) {
+        abort();
+    }
+    qdev_prop_set_uint32(dev, "num-blocks", nb_blocs);
+    qdev_prop_set_uint64(dev, "sector-length", sector_len);
+    qdev_prop_set_uint8(dev, "width", width);
+    qdev_prop_set_uint8(dev, "big-endian", !!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_string(dev, "name", name);
+    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..76f52d7 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,26 @@ 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 */
+    uint16_t ident0;
+    uint16_t ident1;
+    uint16_t ident2;
+    uint16_t ident3;
+    uint16_t unlock_addr0;
+    uint16_t unlock_addr1;
     uint8_t cfi_len;
     uint8_t cfi_table[0x52];
     QEMUTimer *timer;
@@ -79,6 +88,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;
 };
 
@@ -189,16 +199,17 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
         switch (boff) {
         case 0x00:
         case 0x01:
-            ret = pfl->ident[boff & 0x01];
+            ret = boff & 0x01 ? pfl->ident1 : pfl->ident0;
             break;
         case 0x02:
             ret = 0x00; /* Pretend all sectors are unprotected */
             break;
         case 0x0E:
         case 0x0F:
-            if (pfl->ident[2 + (boff & 0x01)] == (uint8_t)-1)
+            ret = boff & 0x01 ? pfl->ident3 : pfl->ident2;
+            if (ret == (uint8_t)-1) {
                 goto flash_read;
-            ret = pfl->ident[2 + (boff & 0x01)];
+            }
             break;
         default:
             goto flash_read;
@@ -282,9 +293,9 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
             pfl->cmd = 0x98;
             return;
         }
-        if (boff != pfl->unlock_addr[0] || cmd != 0xAA) {
+        if (boff != pfl->unlock_addr0 || cmd != 0xAA) {
             DPRINTF("%s: unlock0 failed " TARGET_FMT_plx " %02x %04x\n",
-                    __func__, boff, cmd, pfl->unlock_addr[0]);
+                    __func__, boff, cmd, pfl->unlock_addr0);
             goto reset_flash;
         }
         DPRINTF("%s: unlock sequence started\n", __func__);
@@ -292,7 +303,7 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
     case 1:
         /* We started an unlock sequence */
     check_unlock1:
-        if (boff != pfl->unlock_addr[1] || cmd != 0x55) {
+        if (boff != pfl->unlock_addr1 || cmd != 0x55) {
             DPRINTF("%s: unlock1 failed " TARGET_FMT_plx " %02x\n", __func__,
                     boff, cmd);
             goto reset_flash;
@@ -301,7 +312,7 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
         break;
     case 2:
         /* We finished an unlock sequence */
-        if (!pfl->bypass && boff != pfl->unlock_addr[0]) {
+        if (!pfl->bypass && boff != pfl->unlock_addr0) {
             DPRINTF("%s: command failed " TARGET_FMT_plx " %02x\n", __func__,
                     boff, cmd);
             goto reset_flash;
@@ -399,7 +410,7 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
     case 5:
         switch (cmd) {
         case 0x10:
-            if (boff != pfl->unlock_addr[0]) {
+            if (boff != pfl->unlock_addr0) {
                 DPRINTF("%s: chip erase: invalid address " TARGET_FMT_plx "\n",
                         __func__, offset);
                 goto reset_flash;
@@ -574,49 +585,38 @@ 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;
-    int32_t chip_len;
+    pflash_t *pfl = FROM_SYSBUS(typeof(*pfl), dev);
+    uint32_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);
+
+    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 +625,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 +683,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 +706,81 @@ 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("drive", struct pflash_t, bs),
+    DEFINE_PROP_UINT32("num-blocks", struct pflash_t, nb_blocs, 0),
+    DEFINE_PROP_UINT32("sector-length", 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("big-endian", 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, "drive", bs)) {
+        abort();
+    }
+    qdev_prop_set_uint32(dev, "num-blocks", nb_blocs);
+    qdev_prop_set_uint32(dev, "sector-length", sector_len);
+    qdev_prop_set_uint8(dev, "width", width);
+    qdev_prop_set_uint8(dev, "mappings", nb_mappings);
+    qdev_prop_set_uint8(dev, "big-endian", !!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_prop_set_string(dev, "name", name);
+    qdev_init_nofail(dev);
+
+    sysbus_mmio_map(busdev, 0, base);
     return pfl;
 }
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v2 3/6] pflash_cfi0x: QOMified

[Peter Maydell]

On 22 October 2012 08:19, 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>

> --- 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,29 @@ 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 */

This comment is no longer necessary. If you delete it then
you can mark the next version as
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

[Qemu-devel] [PATCH v2 4/6] 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>
---
changed since v1:
use sysbus_mmio_get_region() for SRAM mappings (PMM Review)
fixed header comment s/pl353/pl35x
fixed complie warnings in debug mode (-DPL35X_DEBUG)

 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..0f8c5ed
--- /dev/null
+++ b/hw/pl35x.c
@@ -0,0 +1,299 @@
+/*
+ * QEMU model of Primcell PL35X family of memory controllers
+ *
+ * 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", (unsigned)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", (unsigned)addr, (unsigned)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);
+    memory_region_add_subregion(&itf->mm, 0, sysbus_mmio_get_region(sbd, 0));
+    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

Re: [Qemu-devel] [PATCH v2 4/6] hw: Model of Primecell pl35x mem controller

[Peter Maydell]

On 22 October 2012 08:19, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> 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>
> ---
> changed since v1:
> use sysbus_mmio_get_region() for SRAM mappings (PMM Review)
> fixed header comment s/pl353/pl35x
> fixed complie warnings in debug mode (-DPL35X_DEBUG)
>
>  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..0f8c5ed
> --- /dev/null
> +++ b/hw/pl35x.c
> @@ -0,0 +1,299 @@
> +/*
> + * QEMU model of Primcell PL35X family of memory controllers

"Primecell"

> + *
> + * 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 */

This field needs a better name (and as a device property it
desperately needs a better name).

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

These are the raw interrupt status bits; this code will probably
need to be rewritten when interrupt support is added. (You aren't
implementing the non-raw status bits or the enables for example,
and we might not want to be doing a nand_getpins query only on the
register read.)

The dynamic cast seems a bit ugly.

> +        break;
> +    default:
> +        DB_PRINT("Unimplemented SMC read access reg=" TARGET_FMT_plx "\n",
> +                 addr * 4);

You can use qemu_log_mask's LOG_UNIMP and LOG_GUEST_ERROR for this now.

> +        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", (unsigned)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", (unsigned)addr, (unsigned)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

"d" ?

> +     * region
> +     */
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(itf->dev);
> +
> +    memory_region_init(&itf->mm, "pl35x.sram", 1 << 24);
> +    memory_region_add_subregion(&itf->mm, 0, sysbus_mmio_get_region(sbd, 0));
> +    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);
> +}

So we're only supporting a single chipselect on each interface at the
moment, is that right?

And we do not try to model the addr_mask/addr_match tieoffs but instead
let the board do the equivalent with memory region aliases/containers
to put things in the right place? (That makes sense to me -- it's not
guest visible so there's no need to implement the complication when we
can just use the qemu standard functions for the same purpose.)

> +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);

Are we going to want different implementations of any of these registers
depending on whether NAND or SRAM or neither? (I think we can get away
without, the chip config registers are just "data, reads back as written"
as far as we're concerned, I think).

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

I think it would be cute to make this table driven, something like
(untested uncompiled code):

typedef void interface_initfn(SysBusDevice *dev, PL35xItf *itf);

static void pl35x_init_no_if(SysBusDevice *dev, PL35xItf *itf)
{
    /* Dummy init function for variants with no second memory interface */
}

/* Different config variants of the PL35x support different combinations
 * of SRAM and NAND on their memory interfaces 0 and 1.
 */
static interface_initfn** variant_if_table[] = {
   { pl35x_init_nand, pl35x_init_no_if }, /* PL351 */
   { pl35x_init_sram, pl35x_init_no_if }, /* PL352 */
   { pl35x_init_sram, pl35x_init_nand },  /* PL353 */
   { pl35x_init_sram, pl35x_init_sram },  /* PL354 */
};

then in pl35x_init you can do:
    variant_if_table[var][0](dev, &s->itf[0]);
    variant_if_table[var][1](dev, &s->itf[1]);

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

We have a sysbus init function and an instance init function?
that's confusing :-)

> +
> +static Property pl35x_properties[] = {
> +    DEFINE_PROP_UINT8("x", PL35xState, x, 3),

Apart from the name, I'm not totally convinced we just want
to make this property the last digit of the model name.

> +    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",

All the other plxxx devices just name themselves
"pl110" or whatever, so "pl35x" here.

> +    .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)

The other approach to the various part variants of course would
be to just define four different QOM classes. I guess that would
let us avoid having dynamic properties. Would it be better? Dunno.

-- PMM

Re: [Qemu-devel] [PATCH v2 4/6] hw: Model of Primecell pl35x mem controller

[Peter Crosthwaite]

On Tue, Oct 23, 2012 at 2:12 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 22 October 2012 08:19, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> 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>
>> ---
>> changed since v1:
>> use sysbus_mmio_get_region() for SRAM mappings (PMM Review)
>> fixed header comment s/pl353/pl35x
>> fixed complie warnings in debug mode (-DPL35X_DEBUG)
>>
>>  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..0f8c5ed
>> --- /dev/null
>> +++ b/hw/pl35x.c
>> @@ -0,0 +1,299 @@
>> +/*
>> + * QEMU model of Primcell PL35X family of memory controllers
>
> "Primecell"
>
>> + *
>> + * 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 */
>
> This field needs a better name (and as a device property it
> desperately needs a better name).
>

Annoyingly X is the most descriptive name for what it is if you go by
the TRM. We could call it "variant" or some such?

>> +} 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;
>> +        }
>
> These are the raw interrupt status bits; this code will probably
> need to be rewritten when interrupt support is added. (You aren't
> implementing the non-raw status bits or the enables for example,
> and we might not want to be doing a nand_getpins query only on the
> register read.)
>

Im implementing register features as I discover my guest software
needs them. This was first cab off the rank. I think you are right in
that it should come from the other side - as nand io happens this is
captured in the device state at the time. Reads then just read the
state out with no side effects.

> The dynamic cast seems a bit ugly.
>

Will go away with above soln.

>> +        break;
>> +    default:
>> +        DB_PRINT("Unimplemented SMC read access reg=" TARGET_FMT_plx "\n",
>> +                 addr * 4);
>
> You can use qemu_log_mask's LOG_UNIMP and LOG_GUEST_ERROR for this now.
>
>> +        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", (unsigned)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", (unsigned)addr, (unsigned)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
>
> "d" ?
>

Hangover from an out of date implementation. Will fix.

>> +     * region
>> +     */
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(itf->dev);
>> +
>> +    memory_region_init(&itf->mm, "pl35x.sram", 1 << 24);
>> +    memory_region_add_subregion(&itf->mm, 0, sysbus_mmio_get_region(sbd, 0));
>> +    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);
>> +}
>
> So we're only supporting a single chipselect on each interface at the
> moment, is that right?
>

Yes

> And we do not try to model the addr_mask/addr_match tieoffs but instead
> let the board do the equivalent with memory region aliases/containers
> to put things in the right place?

Prefer not, rather say its not supported at all. The board just
attaches devices via QOM and qdev_init for this device figures it out.
Im trying to abstract away to need for the board model to manage
memory mappings that are device specific. But until this model gets
some more attention that approach could be made to work.

(That makes sense to me -- it's not
> guest visible so there's no need to implement the complication when we
> can just use the qemu standard functions for the same purpose.)
>
>> +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);
>
> Are we going to want different implementations of any of these registers
> depending on whether NAND or SRAM or neither? (I think we can get away
> without, the chip config registers are just "data, reads back as written"
> as far as we're concerned, I think).
>

I dont think so. The register interface does not radically change with
x, so I think the discrepancies can be handled by ifery on x rather
than having seperate implementation for the control region for
different x.

>> +    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;
>> +}
>
> I think it would be cute to make this table driven, something like
> (untested uncompiled code):
>
> typedef void interface_initfn(SysBusDevice *dev, PL35xItf *itf);
>
> static void pl35x_init_no_if(SysBusDevice *dev, PL35xItf *itf)
> {
>     /* Dummy init function for variants with no second memory interface */
> }
>
> /* Different config variants of the PL35x support different combinations
>  * of SRAM and NAND on their memory interfaces 0 and 1.
>  */
> static interface_initfn** variant_if_table[] = {
>    { pl35x_init_nand, pl35x_init_no_if }, /* PL351 */
>    { pl35x_init_sram, pl35x_init_no_if }, /* PL352 */
>    { pl35x_init_sram, pl35x_init_nand },  /* PL353 */
>    { pl35x_init_sram, pl35x_init_sram },  /* PL354 */
> };
>
> then in pl35x_init you can do:
>     variant_if_table[var][0](dev, &s->itf[0]);
>     variant_if_table[var][1](dev, &s->itf[1]);
>
>> +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);
>> +}
>
> We have a sysbus init function and an instance init function?
> that's confusing :-)
>

Yeh links have to exist before qdev_init(). this gets called at
qdev_create(), machine models get a chance to set links then when
qdev_init() happens the links are there.

>> +
>> +static Property pl35x_properties[] = {
>> +    DEFINE_PROP_UINT8("x", PL35xState, x, 3),
>
> Apart from the name, I'm not totally convinced we just want
> to make this property the last digit of the model name.
>
>> +    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",
>
> All the other plxxx devices just name themselves
> "pl110" or whatever, so "pl35x" here.
>
>> +    .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)
>
> The other approach to the various part variants of course would
> be to just define four different QOM classes. I guess that would
> let us avoid having dynamic properties. Would it be better? Dunno.
>

Dont think so. too much annoying to maintain copy-pasted code. (Four
VMSDs etc). I think parametrisation of x or "variant" can be made to
work with some polish and QOMs RTTI can be used to cast and talk to
devices.

Ill action the other review comments that ive no-replied too.

Regards,
Peter
> -- PMM
>

[Qemu-devel] [PATCH v2 5/6] 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>
---
changed from v1:
fixed property names (see patch 3)

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

diff --git a/hw/xilinx_zynq.c b/hw/xilinx_zynq.c
index c55dafb..7261693 100644
--- a/hw/xilinx_zynq.c
+++ b/hw/xilinx_zynq.c
@@ -123,14 +123,48 @@ static void zynq_init(QEMUMachineInitArgs *args)
     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, "drive", bs)) {
+            abort();
+        }
+        qdev_prop_set_uint32(att_dev, "num-blocks",
+                             FLASH_SIZE/FLASH_SECTOR_SIZE);
+        qdev_prop_set_uint32(att_dev, "sector-length", 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, "big-endian", 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_prop_set_string(att_dev, "name", "pl353.pflash");
+        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

Re: [Qemu-devel] [PATCH v2 5/6] xilinx_zynq: add pl353

[Peter Maydell]

On 22 October 2012 08:19, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> +    /* 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);

Spoke briefly to Anthony on IRC about this and he said he didn't
think it was necessary (though it might have been in the past).

-- PMM

Re: [Qemu-devel] [PATCH v2 5/6] xilinx_zynq: add pl353

[vbalaji]

Hi,

I want to emulate PL35x SMC controller with Nand flash as a device. Though I
got the patch from the following link.

http://qemu.11.n7.nabble.com/Qemu-devel-PATCH-v1-0-7-QOMify-pflash-cfi0x-PL353-for-Xilinx-Zynq-td167995.html.

I dont know how to test the nand flash with the SMC controller. Please can
you guide me if it is possible.

Thanks in advance
Balaji.



--
View this message in context: http://qemu.11.n7.nabble.com/Qemu-devel-PATCH-v2-0-6-QOMify-pflash-cfi0x-PL353-for-Xilinx-Zynq-tp168323p232520.html
Sent from the Developer mailing list archive at Nabble.com.

[Qemu-devel] [PATCH v2 6/6] pflash_cfi01: Fix debug mode printfery

[Peter Crosthwaite]

This DPRINTF was throwing a warning due to a missing cast.

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

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

diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
index 6164a97..90c111d 100644
--- a/hw/pflash_cfi01.c
+++ b/hw/pflash_cfi01.c
@@ -183,7 +183,8 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
             DPRINTF("%s: Device ID Code %04x\n", __func__, ret);
             break;
         default:
-            DPRINTF("%s: Read Device Information boff=%x\n", __func__, boff);
+            DPRINTF("%s: Read Device Information boff=%x\n", __func__,
+                    (unsigned)boff);
             ret = 0;
             break;
         }
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v2 6/6] pflash_cfi01: Fix debug mode printfery

[Peter Maydell]

On 22 October 2012 08:19, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> This DPRINTF was throwing a warning due to a missing cast.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

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

If you stick this earlier in the series with the other pflash
patches they can easily be applied even if we're still arguing
about the pl35x bits.

-- PMM