[Qemu-devel] [PATCH V5 0/7] block, arm: Fix buffered flash writes on VExpress

[Qemu-devel] [PATCH V5 0/7] block, arm: Fix buffered flash writes on VExpress

[Roy Franz]

This patchset fixes buffered flash writes on the VExpress
platform.  Buffered writes should now work properly on platforms whose
flash interface width is different from the device width.  The default
is for the device-width to be set to 0, so platforms
that can benefit from this change will need to be updated.  This patchset
updates the configuration for the VExpress platform which requires it.
UEFI firmware uses buffered writes for persistent variable storage,
and this patchset enables this usage on QEMU.

The CFI and device ID changes were tested against a real VExpress board
by dumping the data.  UEFI itself does not use the device ID or CFI
information.

I think that the current device ID handling code is broken.  I did not remove
it in this patchset as I am reluctant to make changes to platforms I can't test,
and do not know the actual flash device configuration for.


Changes from v4:
* Added device_max_width parameter to enable proper device id and CFI
  responses.
* Device width now set to 0 to indicate it is not set, and this is used
  to keep behavior unchanged for platforms not updated.
* Fixed device ID and CFI responses to properly take into account device
  width.  For the VExpress platform this mostly matches the hardware.
* Refactored VExpress flash init code based on feedback.
* now uses helper functions for many bitfield operations.
* Fixed checkpatch problems in v4.
* NOTE: patch 2/4 of v5 was mis-identified in that submission as v5.

Changes from v3:
* Broke out width->bank_width name change into separate patch.
* Added patch that returns status for each device in bank.
* Reviewed code for other uses of device_width.  The one remaining place
  this should be used is in the handling of returning the device ID.  The
  existing code looks a bit suspect, as it is combining 16 bit values by
  shifting 8 bits and oring them.  I have no good way to test various
  flash configurations, so I am reluctant to make major changes to that code.
Changes from v2:
(All changes in patch 2/2, 1/1 unchanged.)
* Set flash invariant properties directly in VExpress specific flash init
  routine rather than passing long argument list.
* Fix typo in comment.

Changes from v1:
* Add device-width property and use this to mask write length instead
  of devices mas write length
* Update vexpress init code to set device-width to proper value.

Roy Franz (7):
  rename pflash_t member width to bank_width
  Add device-width property to pflash_cfi01
  return status for each NOR flash device
  Set proper device-width for vexpress flash
  Add max device width parameter for NOR devices
  Fix CFI query responses for NOR flash
  Fix NOR flash device ID reading

 hw/arm/vexpress.c       |   44 +++++++--
 hw/block/pflash_cfi01.c |  244 ++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 246 insertions(+), 42 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH V5 1/7] rename pflash_t member width to bank_width

[Roy Franz]

Rename the 'width' member of the pflash_t structure
in preparation for adding a bank_width member.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/block/pflash_cfi01.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 018a967..a0d7a16 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -71,7 +71,7 @@ struct pflash_t {
     BlockDriverState *bs;
     uint32_t nb_blocs;
     uint64_t sector_len;
-    uint8_t width;
+    uint8_t bank_width;
     uint8_t be;
     uint8_t wcycle; /* if 0, the flash is read normally */
     int ro;
@@ -126,10 +126,11 @@ static uint32_t pflash_read (pflash_t *pfl, hwaddr offset,
     ret = -1;
     boff = offset & 0xFF; /* why this here ?? */
 
-    if (pfl->width == 2)
+    if (pfl->bank_width == 2) {
         boff = boff >> 1;
-    else if (pfl->width == 4)
+    } else if (pfl->bank_width == 4) {
         boff = boff >> 2;
+    }
 
 #if 0
     DPRINTF("%s: reading offset " TARGET_FMT_plx " under cmd %02x width %d\n",
@@ -665,7 +666,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     pfl->cfi_table[0x28] = 0x02;
     pfl->cfi_table[0x29] = 0x00;
     /* Max number of bytes in multi-bytes write */
-    if (pfl->width == 1) {
+    if (pfl->bank_width == 1) {
         pfl->cfi_table[0x2A] = 0x08;
     } else {
         pfl->cfi_table[0x2A] = 0x0B;
@@ -706,7 +707,7 @@ 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("width", struct pflash_t, bank_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),
@@ -745,8 +746,8 @@ pflash_t *pflash_cfi01_register(hwaddr base,
                                 DeviceState *qdev, const char *name,
                                 hwaddr size,
                                 BlockDriverState *bs,
-                                uint32_t sector_len, int nb_blocs, int width,
-                                uint16_t id0, uint16_t id1,
+                                uint32_t sector_len, int nb_blocs,
+                                int bank_width, uint16_t id0, uint16_t id1,
                                 uint16_t id2, uint16_t id3, int be)
 {
     DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH01);
@@ -756,7 +757,7 @@ pflash_t *pflash_cfi01_register(hwaddr base,
     }
     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, "width", bank_width);
     qdev_prop_set_uint8(dev, "big-endian", !!be);
     qdev_prop_set_uint16(dev, "id0", id0);
     qdev_prop_set_uint16(dev, "id1", id1);
-- 
1.7.10.4

[Qemu-devel] [PATCH V5 2/7] Add device-width property to pflash_cfi01

[Roy Franz]

The width of the devices that make up the flash interface
is required to mask certain commands, in particular the
write length for buffered writes.  This length will be presented
to each device on the interface by the program writing the flash,
and the flash emulation code needs to be able to determine
the length of the write as recieved by each flash device.
The device-width defaults to the bank width which should
maintain existing behavior for platforms that don't need
this change.
This change is required to support buffered writes on the
vexpress platform that has a 32 bit flash interface with 2
16 bit devices on it.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 hw/block/pflash_cfi01.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index a0d7a16..a458ad6 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -40,6 +40,7 @@
 #include "hw/block/flash.h"
 #include "block/block.h"
 #include "qemu/timer.h"
+#include "qemu/bitops.h"
 #include "exec/address-spaces.h"
 #include "qemu/host-utils.h"
 #include "hw/sysbus.h"
@@ -72,6 +73,7 @@ struct pflash_t {
     uint32_t nb_blocs;
     uint64_t sector_len;
     uint8_t bank_width;
+    uint8_t device_width; /* If 0, device width not specified. */
     uint8_t be;
     uint8_t wcycle; /* if 0, the flash is read normally */
     int ro;
@@ -379,6 +381,14 @@ static void pflash_write(pflash_t *pfl, hwaddr offset,
 
             break;
         case 0xe8:
+            /* Mask writeblock size based on device width, or bank width if
+             * device width not specified.
+             */
+            if (pfl->device_width) {
+                value = extract32(value, 0, pfl->device_width * 8);
+            } else {
+                value = extract32(value, 0, pfl->bank_width * 8);
+            }
             DPRINTF("%s: block write of %x bytes\n", __func__, value);
             pfl->counter = value;
             pfl->wcycle++;
@@ -708,6 +718,7 @@ static Property pflash_cfi01_properties[] = {
     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, bank_width, 0),
+    DEFINE_PROP_UINT8("device-width", struct pflash_t, device_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),
-- 
1.7.10.4

[Qemu-devel] [PATCH V5 3/7] return status for each NOR flash device

[Roy Franz]

Now that we know how wide each flash device that makes up the bank is,
return status for each device in the bank.  Leave existing code
that treats 32 bit wide banks as composed of two 16 bit devices as otherwise
we may break configurations that do not set the device_width propery.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 hw/block/pflash_cfi01.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index a458ad6..82a2519 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -193,9 +193,20 @@ static uint32_t pflash_read (pflash_t *pfl, hwaddr offset,
     case 0x60: /* Block /un)lock */
     case 0x70: /* Status Register */
     case 0xe8: /* Write block */
-        /* Status register read */
+        /* Status register read.  Return status from each device in
+         * bank.
+         */
         ret = pfl->status;
-        if (width > 2) {
+        if (pfl->device_width && width > pfl->device_width) {
+            int shift = pfl->device_width * 8;
+            while (shift + pfl->device_width * 8 <= width * 8) {
+                ret |= pfl->status << shift;
+                shift += pfl->device_width * 8;
+            }
+        } else if (!pfl->device_width && width > 2) {
+            /* Handle 32 bit flash cases where device width is not
+             * set. (Existing behavior before device width added.)
+             */
             ret |= pfl->status << 16;
         }
         DPRINTF("%s: status %x\n", __func__, ret);
-- 
1.7.10.4

[Qemu-devel] [PATCH V5 4/7] Set proper device-width for vexpress flash

[Roy Franz]

Create vexpress specific pflash registration
function which properly configures the device-width
of 16 bits (2 bytes) for the NOR flash on the
vexpress platform.  This change is required for
buffered flash writes to work properly.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 hw/arm/vexpress.c |   44 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 10 deletions(-)

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index f48de00..939b468 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -480,6 +480,36 @@ static void vexpress_modify_dtb(const struct arm_boot_info *info, void *fdt)
     }
 }
 
+
+/* Open code a private version of pflash registration since we
+ * need to set non-default device width for VExpress platform.
+ */
+static pflash_t *ve_pflash_cfi01_register(hwaddr base, const char *name,
+                                          DriveInfo *di)
+{
+    DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
+
+    if (di && qdev_prop_set_drive(dev, "drive", di->bdrv)) {
+        abort();
+    }
+
+    qdev_prop_set_uint32(dev, "num-blocks",
+                         VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE);
+    qdev_prop_set_uint64(dev, "sector-length", VEXPRESS_FLASH_SECT_SIZE);
+    qdev_prop_set_uint8(dev, "width", 4);
+    qdev_prop_set_uint8(dev, "device-width", 2);
+    qdev_prop_set_uint8(dev, "big-endian", 0);
+    qdev_prop_set_uint16(dev, "id0", 0x00);
+    qdev_prop_set_uint16(dev, "id1", 0x89);
+    qdev_prop_set_uint16(dev, "id2", 0x00);
+    qdev_prop_set_uint16(dev, "id3", 0x18);
+    qdev_prop_set_string(dev, "name", name);
+    qdev_init_nofail(dev);
+
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
+    return OBJECT_CHECK(pflash_t, (dev), "cfi.pflash01");
+}
+
 static void vexpress_common_init(VEDBoardInfo *daughterboard,
                                  QEMUMachineInitArgs *args)
 {
@@ -561,11 +591,8 @@ static void vexpress_common_init(VEDBoardInfo *daughterboard,
     sysbus_create_simple("pl111", map[VE_CLCD], pic[14]);
 
     dinfo = drive_get_next(IF_PFLASH);
-    pflash0 = pflash_cfi01_register(map[VE_NORFLASH0], NULL, "vexpress.flash0",
-            VEXPRESS_FLASH_SIZE, dinfo ? dinfo->bdrv : NULL,
-            VEXPRESS_FLASH_SECT_SIZE,
-            VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE, 4,
-            0x00, 0x89, 0x00, 0x18, 0);
+    pflash0 = ve_pflash_cfi01_register(map[VE_NORFLASH0], "vexpress.flash0",
+                                       dinfo);
     if (!pflash0) {
         fprintf(stderr, "vexpress: error registering flash 0.\n");
         exit(1);
@@ -580,11 +607,8 @@ static void vexpress_common_init(VEDBoardInfo *daughterboard,
     }
 
     dinfo = drive_get_next(IF_PFLASH);
-    if (!pflash_cfi01_register(map[VE_NORFLASH1], NULL, "vexpress.flash1",
-            VEXPRESS_FLASH_SIZE, dinfo ? dinfo->bdrv : NULL,
-            VEXPRESS_FLASH_SECT_SIZE,
-            VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE, 4,
-            0x00, 0x89, 0x00, 0x18, 0)) {
+    if (!ve_pflash_cfi01_register(map[VE_NORFLASH1], "vexpress.flash1",
+                                  dinfo)) {
         fprintf(stderr, "vexpress: error registering flash 1.\n");
         exit(1);
     }
-- 
1.7.10.4

[Qemu-devel] [PATCH V5 5/7] Add max device width parameter for NOR devices

[Roy Franz]

For handling CFI and device ID reads, we need to not only know the
width that a NOR flash device is configured for, but also its maximum
width.  The maximum width addressing mode is used for multi-width
parts no matter which width they are configured for.  The most common
case is x16 parts that also support x8 mode.  When configured for x8
operation these devices respond to CFI and device ID requests differently
than native x8 NOR parts.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 hw/block/pflash_cfi01.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 82a2519..8f81341 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -74,6 +74,7 @@ struct pflash_t {
     uint64_t sector_len;
     uint8_t bank_width;
     uint8_t device_width; /* If 0, device width not specified. */
+    uint8_t max_device_width;  /* max device width in bytes */
     uint8_t be;
     uint8_t wcycle; /* if 0, the flash is read normally */
     int ro;
@@ -635,6 +636,13 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
         pfl->ro = 0;
     }
 
+    /* Default to devices being used at their maximum device width. This was
+     * assumed before the device_width support was added.
+     */
+    if (!pfl->max_device_width) {
+        pfl->max_device_width = pfl->device_width;
+    }
+
     pfl->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
     pfl->wcycle = 0;
     pfl->cmd = 0;
@@ -730,6 +738,7 @@ static Property pflash_cfi01_properties[] = {
     DEFINE_PROP_UINT64("sector-length", struct pflash_t, sector_len, 0),
     DEFINE_PROP_UINT8("width", struct pflash_t, bank_width, 0),
     DEFINE_PROP_UINT8("device-width", struct pflash_t, device_width, 0),
+    DEFINE_PROP_UINT8("max-device-width", struct pflash_t, max_device_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),
-- 
1.7.10.4

[Qemu-devel] [PATCH V5 6/7] Fix CFI query responses for NOR flash

[Roy Franz]

This change fixes the CFI query responses to handle NOR device
widths that are different from the bank width.  Support is also
added for multi-width devices in a x8 configuration.  This is
typically x8/x16 devices, but the CFI specification mentions
x8/x32 devices so those should be supported as well if they
exist.
The query response data is now replicated per-device in the bank,
and is adjusted for x16 or x32 parts configured in x8 mode.

The existing code is left in place for boards that have not
been updated to specify an explicit device_width.  The VExpress
board has been updated in an earlier patch in this series so
this is the only board currently affected.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 hw/block/pflash_cfi01.c |  103 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 92 insertions(+), 11 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 8f81341..564e6ee 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -119,6 +119,66 @@ static void pflash_timer (void *opaque)
     pfl->cmd = 0;
 }
 
+/* Perform a CFI query based on the bank width of the flash.
+ * If this code is called we know we have a device_width set for
+ * this flash.
+ */
+static uint32_t pflash_cfi_query(pflash_t *pfl, hwaddr offset)
+{
+    int i;
+    uint32_t resp = 0;
+    hwaddr boff;
+
+    /* Adjust incoming offset to match expected device-width
+     * addressing. CFI query addresses are always specified in terms of
+     * the maximum supported width of the device.  This means that x8
+     * devices and x8/x16 devices in x8 mode behave differently.  For
+     * devices that are not used at their max width, we will be
+     * provided with addresses that use higher address bits than
+     * expected (based on the max width), so we will shift them lower
+     * so that they will match the addresses used when
+     * device_width==max_device_width.
+     */
+    boff = offset >> (ctz32(pfl->bank_width) +
+                      ctz32(pfl->max_device_width) - ctz32(pfl->device_width));
+
+    if (boff > pfl->cfi_len) {
+        return 0;
+    }
+    /* Now we will construct the CFI response generated by a single
+     * device, then replicate that for all devices that make up the
+     * bus.  For wide parts used in x8 mode, CFI query responses
+     * are different than native byte-wide parts.
+     */
+    resp = pfl->cfi_table[boff];
+    if (pfl->device_width != pfl->max_device_width) {
+        /* The only case currently supported is x8 mode for a
+         * wider part.
+         */
+        if (pfl->device_width != 1 || pfl->bank_width > 4) {
+            DPRINTF("%s: Unsupported device configuration: device_width=%d, max_device_width=%d\n",
+                    __func__, pfl->device_width,
+                    pfl->max_device_width);
+            return 0;
+        }
+        /* CFI query data is repeated, rather than zero padded for
+         * wide devices used in x8 mode.
+         */
+        for (i = 1; i < pfl->max_device_width; i++) {
+            resp = deposit32(resp, 8 * i, 8, pfl->cfi_table[boff]);
+        }
+    }
+    /* Replicate responses for each device in bank. */
+    if (pfl->device_width < pfl->bank_width) {
+        for (i = pfl->device_width;
+             i < pfl->bank_width; i += pfl->device_width) {
+            resp = deposit32(resp, 8 * i, 8 * pfl->device_width, resp);
+        }
+    }
+
+    return resp;
+}
+
 static uint32_t pflash_read (pflash_t *pfl, hwaddr offset,
                              int width, int be)
 {
@@ -127,13 +187,6 @@ static uint32_t pflash_read (pflash_t *pfl, hwaddr offset,
     uint8_t *p;
 
     ret = -1;
-    boff = offset & 0xFF; /* why this here ?? */
-
-    if (pfl->bank_width == 2) {
-        boff = boff >> 1;
-    } else if (pfl->bank_width == 4) {
-        boff = boff >> 2;
-    }
 
 #if 0
     DPRINTF("%s: reading offset " TARGET_FMT_plx " under cmd %02x width %d\n",
@@ -213,6 +266,12 @@ static uint32_t pflash_read (pflash_t *pfl, hwaddr offset,
         DPRINTF("%s: status %x\n", __func__, ret);
         break;
     case 0x90:
+        boff = offset & 0xFF;
+        if (pfl->bank_width == 2)
+            boff = boff >> 1;
+        else if (pfl->bank_width == 4)
+            boff = boff >> 2;
+
         switch (boff) {
         case 0:
             ret = pfl->ident0 << 8 | pfl->ident1;
@@ -230,10 +289,32 @@ static uint32_t pflash_read (pflash_t *pfl, hwaddr offset,
         }
         break;
     case 0x98: /* Query mode */
-        if (boff > pfl->cfi_len)
-            ret = 0;
-        else
-            ret = pfl->cfi_table[boff];
+        if (!pfl->device_width) {
+            /* Preserve old behavior if device width not specified */
+            boff = offset & 0xFF;
+            if (pfl->bank_width == 2) {
+                boff = boff >> 1;
+            } else if (pfl->bank_width == 4) {
+                boff = boff >> 2;
+            }
+
+            if (boff > pfl->cfi_len) {
+                ret = 0;
+            } else {
+                ret = pfl->cfi_table[boff];
+            }
+        } else {
+            /* If we have a read larger than the bank_width, combine multiple
+             * CFI queries into a single response.
+             */
+            int i;
+            for (i = 0; i < width; i += pfl->bank_width) {
+                ret = deposit32(ret, i * 8, pfl->bank_width * 8,
+                                pflash_cfi_query(pfl,
+                                                 offset + i * pfl->bank_width));
+            }
+        }
+
         break;
     }
     return ret;
-- 
1.7.10.4

[Qemu-devel] [PATCH V5 7/7] Fix NOR flash device ID reading

[Roy Franz]

Fix NOR flash manufacturer and device ID reading.  This now
properly takes into account device widths and device max widths
as required.  The reading of these IDs uses the same max_width
dependent addressing as CFI queries.

The old code remains for chips that don't specify a device width,
as the new code relies on a device width being set in order to
properly operate.  The existing code seems very broken.

Only ident0 and ident1 are used in the new code, as other fields
relate to the lock state of blocks in flash.

The VExpress flash configuration has been updated to match
the new code, as the existing definition was 'wrong' in order
to return the expected results with the broken device ID code.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 hw/arm/vexpress.c       |    6 +--
 hw/block/pflash_cfi01.c |  107 ++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 90 insertions(+), 23 deletions(-)

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 939b468..aaa863e 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -499,10 +499,10 @@ static pflash_t *ve_pflash_cfi01_register(hwaddr base, const char *name,
     qdev_prop_set_uint8(dev, "width", 4);
     qdev_prop_set_uint8(dev, "device-width", 2);
     qdev_prop_set_uint8(dev, "big-endian", 0);
-    qdev_prop_set_uint16(dev, "id0", 0x00);
-    qdev_prop_set_uint16(dev, "id1", 0x89);
+    qdev_prop_set_uint16(dev, "id0", 0x89);
+    qdev_prop_set_uint16(dev, "id1", 0x18);
     qdev_prop_set_uint16(dev, "id2", 0x00);
-    qdev_prop_set_uint16(dev, "id3", 0x18);
+    qdev_prop_set_uint16(dev, "id3", 0x00);
     qdev_prop_set_string(dev, "name", name);
     qdev_init_nofail(dev);
 
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 564e6ee..15e4a64 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -179,6 +179,58 @@ static uint32_t pflash_cfi_query(pflash_t *pfl, hwaddr offset)
     return resp;
 }
 
+
+
+/* Perform a device id query based on the bank width of the flash. */
+static uint32_t pflash_devid_query(pflash_t *pfl, hwaddr offset)
+{
+    int i;
+    uint32_t resp;
+    hwaddr boff;
+
+    /* Adjust incoming offset to match expected device-width
+     * addressing. Device ID read addresses are always specified in
+     * terms of the maximum supported width of the device.  This means
+     * that x8 devices and x8/x16 devices in x8 mode behave
+     * differently. For devices that are not used at their max width,
+     * we will be provided with addresses that use higher address bits
+     * than expected (based on the max width), so we will shift them
+     * lower so that they will match the addresses used when
+     * device_width==max_device_width.
+     */
+    boff = offset >> (ctz32(pfl->bank_width) +
+                      ctz32(pfl->max_device_width) - ctz32(pfl->device_width));
+
+    /* Mask off upper bits which may be used in to query block
+     * or sector lock status at other addresses.
+     * Offsets 2/3 are block lock status, is not emulated.
+     */
+    switch (boff & 0xFF) {
+    case 0:
+        resp = pfl->ident0;
+        DPRINTF("%s: Manufacturer Code %04x\n", __func__, ret);
+        break;
+    case 1:
+        resp = pfl->ident1;
+        DPRINTF("%s: Device ID Code %04x\n", __func__, ret);
+        break;
+    default:
+        DPRINTF("%s: Read Device Information offset=%x\n", __func__,
+                (unsigned)offset);
+        return 0;
+        break;
+    }
+    /* Replicate responses for each device in bank. */
+    if (pfl->device_width < pfl->bank_width) {
+        for (i = pfl->device_width;
+              i < pfl->bank_width; i += pfl->device_width) {
+            resp = deposit32(resp, 8 * i, 8 * pfl->device_width, resp);
+        }
+    }
+
+    return resp;
+}
+
 static uint32_t pflash_read (pflash_t *pfl, hwaddr offset,
                              int width, int be)
 {
@@ -266,26 +318,41 @@ static uint32_t pflash_read (pflash_t *pfl, hwaddr offset,
         DPRINTF("%s: status %x\n", __func__, ret);
         break;
     case 0x90:
-        boff = offset & 0xFF;
-        if (pfl->bank_width == 2)
-            boff = boff >> 1;
-        else if (pfl->bank_width == 4)
-            boff = boff >> 2;
-
-        switch (boff) {
-        case 0:
-            ret = pfl->ident0 << 8 | pfl->ident1;
-            DPRINTF("%s: Manufacturer Code %04x\n", __func__, ret);
-            break;
-        case 1:
-            ret = pfl->ident2 << 8 | pfl->ident3;
-            DPRINTF("%s: Device ID Code %04x\n", __func__, ret);
-            break;
-        default:
-            DPRINTF("%s: Read Device Information boff=%x\n", __func__,
-                    (unsigned)boff);
-            ret = 0;
-            break;
+
+        if (!pfl->device_width) {
+            /* Preserve old behavior if device width not specified */
+            boff = offset & 0xFF;
+            if (pfl->bank_width == 2) {
+                boff = boff >> 1;
+            } else if (pfl->bank_width == 4) {
+                boff = boff >> 2;
+            }
+
+            switch (boff) {
+            case 0:
+                ret = pfl->ident0 << 8 | pfl->ident1;
+                DPRINTF("%s: Manufacturer Code %04x\n", __func__, ret);
+                break;
+            case 1:
+                ret = pfl->ident2 << 8 | pfl->ident3;
+                DPRINTF("%s: Device ID Code %04x\n", __func__, ret);
+                break;
+            default:
+                DPRINTF("%s: Read Device Information boff=%x\n", __func__,
+                        (unsigned)boff);
+                ret = 0;
+                break;
+            }
+        } else {
+            /* If we have a read larger than the bank_width, combine multiple
+             * manufacturer/device ID queries into a single response.
+             */
+            int i;
+            for (i = 0; i < width; i += pfl->bank_width) {
+                ret = deposit32(ret, i * 8, pfl->bank_width * 8,
+                                pflash_devid_query(pfl,
+                                                 offset + i * pfl->bank_width));
+            }
         }
         break;
     case 0x98: /* Query mode */
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH V5 5/7] Add max device width parameter for NOR devices

[Peter Maydell]

On 5 December 2013 21:35, Roy Franz <roy.franz@linaro.org> wrote:
> For handling CFI and device ID reads, we need to not only know the
> width that a NOR flash device is configured for, but also its maximum
> width.  The maximum width addressing mode is used for multi-width
> parts no matter which width they are configured for.  The most common
> case is x16 parts that also support x8 mode.  When configured for x8
> operation these devices respond to CFI and device ID requests differently
> than native x8 NOR parts.

>      DEFINE_PROP_UINT64("sector-length", struct pflash_t, sector_len, 0),
>      DEFINE_PROP_UINT8("width", struct pflash_t, bank_width, 0),
>      DEFINE_PROP_UINT8("device-width", struct pflash_t, device_width, 0),
> +    DEFINE_PROP_UINT8("max-device-width", struct pflash_t, max_device_width, 0),

So I think that given we now have three width related properties
we could use a comment here about what they mean. Do I have
this right?

/* width here is the overall width of this QEMU device in bytes.
 * The QEMU device may be emulating a number of flash devices
 * wired up in parallel; the width of each individual flash
 * device should be specified via device-width. If the individual
 * devices have a maximum width which is greater than the width
 * they are being used for, this maximum width should be set via
 * max-device-width (which otherwise defaults to device-width).
 * So for instance a 32-bit wide QEMU flash device made from four
 * 16-bit flash devices used in 8-bit wide mode would be configured
 * with width = 4, device-width = 1, max-device-width = 2.
 *
 * If device-width is not specified we default to backwards
 * compatible behaviour which is a bad emulation of two
 * 16 bit devices making up a 32 bit wide QEMU device. This
 * is deprecated for new uses of this device.
 */

thanks
-- PMM

Re: [Qemu-devel] [PATCH V5 2/7] Add device-width property to pflash_cfi01

[Peter Maydell]

On 5 December 2013 21:35, Roy Franz <roy.franz@linaro.org> wrote:
> The width of the devices that make up the flash interface
> is required to mask certain commands, in particular the
> write length for buffered writes.  This length will be presented
> to each device on the interface by the program writing the flash,
> and the flash emulation code needs to be able to determine
> the length of the write as recieved by each flash device.
> The device-width defaults to the bank width which should
> maintain existing behavior for platforms that don't need
> this change.
> This change is required to support buffered writes on the
> vexpress platform that has a 32 bit flash interface with 2
> 16 bit devices on it.
>
> Signed-off-by: Roy Franz <roy.franz@linaro.org

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH V5 3/7] return status for each NOR flash device

[Peter Maydell]

On 5 December 2013 21:35, Roy Franz <roy.franz@linaro.org> wrote:
> Now that we know how wide each flash device that makes up the bank is,
> return status for each device in the bank.  Leave existing code
> that treats 32 bit wide banks as composed of two 16 bit devices as otherwise
> we may break configurations that do not set the device_width propery.
>
> Signed-off-by: Roy Franz <roy.franz@linaro.org>

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

-- PMM

Re: [Qemu-devel] [PATCH V5 4/7] Set proper device-width for vexpress flash

[Peter Maydell]

On 5 December 2013 21:35, Roy Franz <roy.franz@linaro.org> wrote:
> Create vexpress specific pflash registration
> function which properly configures the device-width
> of 16 bits (2 bytes) for the NOR flash on the
> vexpress platform.  This change is required for
> buffered flash writes to work properly.
>
> Signed-off-by: Roy Franz <roy.franz@linaro.org>

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH V5 6/7] Fix CFI query responses for NOR flash

[Peter Maydell]

On 5 December 2013 21:35, Roy Franz <roy.franz@linaro.org> wrote:
> This change fixes the CFI query responses to handle NOR device
> widths that are different from the bank width.  Support is also
> added for multi-width devices in a x8 configuration.  This is
> typically x8/x16 devices, but the CFI specification mentions
> x8/x32 devices so those should be supported as well if they
> exist.
> The query response data is now replicated per-device in the bank,
> and is adjusted for x16 or x32 parts configured in x8 mode.
>
> The existing code is left in place for boards that have not
> been updated to specify an explicit device_width.  The VExpress
> board has been updated in an earlier patch in this series so
> this is the only board currently affected.
>
> Signed-off-by: Roy Franz <roy.franz@linaro.org>
> ---
>  hw/block/pflash_cfi01.c |  103 ++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 92 insertions(+), 11 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 8f81341..564e6ee 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -119,6 +119,66 @@ static void pflash_timer (void *opaque)
>      pfl->cmd = 0;
>  }
>
> +/* Perform a CFI query based on the bank width of the flash.
> + * If this code is called we know we have a device_width set for
> + * this flash.
> + */
> +static uint32_t pflash_cfi_query(pflash_t *pfl, hwaddr offset)
> +{
> +    int i;
> +    uint32_t resp = 0;
> +    hwaddr boff;
> +
> +    /* Adjust incoming offset to match expected device-width
> +     * addressing. CFI query addresses are always specified in terms of
> +     * the maximum supported width of the device.  This means that x8
> +     * devices and x8/x16 devices in x8 mode behave differently.  For
> +     * devices that are not used at their max width, we will be
> +     * provided with addresses that use higher address bits than
> +     * expected (based on the max width), so we will shift them lower
> +     * so that they will match the addresses used when
> +     * device_width==max_device_width.
> +     */
> +    boff = offset >> (ctz32(pfl->bank_width) +
> +                      ctz32(pfl->max_device_width) - ctz32(pfl->device_width));
> +
> +    if (boff > pfl->cfi_len) {
> +        return 0;
> +    }
> +    /* Now we will construct the CFI response generated by a single
> +     * device, then replicate that for all devices that make up the
> +     * bus.  For wide parts used in x8 mode, CFI query responses
> +     * are different than native byte-wide parts.
> +     */
> +    resp = pfl->cfi_table[boff];
> +    if (pfl->device_width != pfl->max_device_width) {
> +        /* The only case currently supported is x8 mode for a
> +         * wider part.
> +         */
> +        if (pfl->device_width != 1 || pfl->bank_width > 4) {
> +            DPRINTF("%s: Unsupported device configuration: device_width=%d, max_device_width=%d\n",

This line is overlong and needs a linebreak.


> +        boff = offset & 0xFF;
> +        if (pfl->bank_width == 2)
> +            boff = boff >> 1;
> +        else if (pfl->bank_width == 4)
> +            boff = boff >> 2;
> +

Missing braces.

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH V5 7/7] Fix NOR flash device ID reading

[Peter Maydell]

On 5 December 2013 21:35, Roy Franz <roy.franz@linaro.org> wrote:
> Fix NOR flash manufacturer and device ID reading.  This now
> properly takes into account device widths and device max widths
> as required.  The reading of these IDs uses the same max_width
> dependent addressing as CFI queries.
>
> The old code remains for chips that don't specify a device width,
> as the new code relies on a device width being set in order to
> properly operate.  The existing code seems very broken.
>
> Only ident0 and ident1 are used in the new code, as other fields
> relate to the lock state of blocks in flash.
>
> The VExpress flash configuration has been updated to match
> the new code, as the existing definition was 'wrong' in order
> to return the expected results with the broken device ID code.
>
> Signed-off-by: Roy Franz <roy.franz@linaro.org>

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

-- PMM

Re: [Qemu-devel] [PATCH V5 5/7] Add max device width parameter for NOR devices

[Peter Maydell]

On 12 December 2013 17:26, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 December 2013 21:35, Roy Franz <roy.franz@linaro.org> wrote:
>> For handling CFI and device ID reads, we need to not only know the
>> width that a NOR flash device is configured for, but also its maximum
>> width.  The maximum width addressing mode is used for multi-width
>> parts no matter which width they are configured for.  The most common
>> case is x16 parts that also support x8 mode.  When configured for x8
>> operation these devices respond to CFI and device ID requests differently
>> than native x8 NOR parts.
>
>>      DEFINE_PROP_UINT64("sector-length", struct pflash_t, sector_len, 0),
>>      DEFINE_PROP_UINT8("width", struct pflash_t, bank_width, 0),
>>      DEFINE_PROP_UINT8("device-width", struct pflash_t, device_width, 0),
>> +    DEFINE_PROP_UINT8("max-device-width", struct pflash_t, max_device_width, 0),
>
> So I think that given we now have three width related properties
> we could use a comment here about what they mean. Do I have
> this right?
>
> /* width here is the overall width of this QEMU device in bytes.
>  * The QEMU device may be emulating a number of flash devices
>  * wired up in parallel; the width of each individual flash
>  * device should be specified via device-width. If the individual
>  * devices have a maximum width which is greater than the width
>  * they are being used for, this maximum width should be set via
>  * max-device-width (which otherwise defaults to device-width).
>  * So for instance a 32-bit wide QEMU flash device made from four
>  * 16-bit flash devices used in 8-bit wide mode would be configured
>  * with width = 4, device-width = 1, max-device-width = 2.
>  *
>  * If device-width is not specified we default to backwards
>  * compatible behaviour which is a bad emulation of two
>  * 16 bit devices making up a 32 bit wide QEMU device. This
>  * is deprecated for new uses of this device.
>  */

PS: if you're happy that the comment above is correct, I
can just add it locally (and fix up the format nits in
the other patch), to save you having to respin the series,
and stick it in the target-arm.next queue.

thanks
-- PMM

Re: [Qemu-devel] [PATCH V5 5/7] Add max device width parameter for NOR devices

[Roy Franz]

On Thu, Dec 12, 2013 at 9:37 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 December 2013 17:26, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 5 December 2013 21:35, Roy Franz <roy.franz@linaro.org> wrote:
>>> For handling CFI and device ID reads, we need to not only know the
>>> width that a NOR flash device is configured for, but also its maximum
>>> width.  The maximum width addressing mode is used for multi-width
>>> parts no matter which width they are configured for.  The most common
>>> case is x16 parts that also support x8 mode.  When configured for x8
>>> operation these devices respond to CFI and device ID requests differently
>>> than native x8 NOR parts.
>>
>>>      DEFINE_PROP_UINT64("sector-length", struct pflash_t, sector_len, 0),
>>>      DEFINE_PROP_UINT8("width", struct pflash_t, bank_width, 0),
>>>      DEFINE_PROP_UINT8("device-width", struct pflash_t, device_width, 0),
>>> +    DEFINE_PROP_UINT8("max-device-width", struct pflash_t, max_device_width, 0),
>>
>> So I think that given we now have three width related properties
>> we could use a comment here about what they mean. Do I have
>> this right?
>>
>> /* width here is the overall width of this QEMU device in bytes.
>>  * The QEMU device may be emulating a number of flash devices
>>  * wired up in parallel; the width of each individual flash
>>  * device should be specified via device-width. If the individual
>>  * devices have a maximum width which is greater than the width
>>  * they are being used for, this maximum width should be set via
>>  * max-device-width (which otherwise defaults to device-width).
>>  * So for instance a 32-bit wide QEMU flash device made from four
>>  * 16-bit flash devices used in 8-bit wide mode would be configured
>>  * with width = 4, device-width = 1, max-device-width = 2.
>>  *
>>  * If device-width is not specified we default to backwards
>>  * compatible behaviour which is a bad emulation of two
>>  * 16 bit devices making up a 32 bit wide QEMU device. This
>>  * is deprecated for new uses of this device.
>>  */
>
> PS: if you're happy that the comment above is correct, I
> can just add it locally (and fix up the format nits in
> the other patch), to save you having to respin the series,
> and stick it in the target-arm.next queue.
>
> thanks
> -- PMM

Hi Peter,

   Your comment explains it very nicely, and please go ahead
and fix the formatting issues.  This is the last QEMU patchset
I have for UEFI support, so once this is in UEFI should boot on the A15
QEMU platforms.

Thanks,
Roy