diff mbox series

[v3,3/3] Add partial flash interleaving to AMD CFI devices

Message ID 20171113173120.3686-1-michael.nawrocki@gtri.gatech.edu
State New
Headers show
Series Add 8-byte wide AMD flash support, partial interleaving | expand

Commit Message

Michael Nawrocki Nov. 13, 2017, 5:31 p.m. UTC 
This mirrors the interleaving support already in place in
pflash_cfi01.c, using the max_device_width and device_width properties.

Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
---
 hw/block/pflash_cfi02.c | 244 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 209 insertions(+), 35 deletions(-)

Comments

Peter Maydell Nov. 23, 2017, 1:46 p.m. UTC | #1 
On 13 November 2017 at 17:31, Mike Nawrocki
<michael.nawrocki@gtri.gatech.edu> wrote:
> This mirrors the interleaving support already in place in
> pflash_cfi01.c, using the max_device_width and device_width properties.
>
> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
> ---
>  hw/block/pflash_cfi02.c | 244 +++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 209 insertions(+), 35 deletions(-)

Thanks for this patch. There is some code duplication from cfi01.c,
but I think it's ok not to worry about trying to share code.

>      case 0xA0:
>      case 0x10:
>      case 0x30:
>          /* Status register read */
>          ret = pfl->status;
> -        DPRINTF("%s: status %x\n", __func__, ret);
> +        for (i = pfl->device_width;
> +             i < pfl->bank_width; i += pfl->device_width) {
> +            ret = deposit64(ret, 8 * i, 8 * pfl->device_width, ret);
> +        }

This will loop forever in the legacy "device_width == 0" case,
won't it?

> +        DPRINTF("%s: status %" PRIx64 "\n", __func__, ret);
>          /* Toggle bit 6 */
>          pfl->status ^= 0x40;
>          break;
> @@ -745,7 +901,25 @@ static Property pflash_cfi02_properties[] = {
>      DEFINE_PROP_DRIVE("drive", struct pflash_t, blk),
>      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),
> +    /* 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.

Is this correct for pflash_cfi02 ? In cfi01 you can see that our
legacy behaviour does things like jamming two ID values
together into one response because it's doing this "bad emulation
of two 16 bit devices", but I don't see similar code in cfi02,
so maybe the legacy default is something different ?

In particular, if the legacy default is actually "just looks like
a single device" and the old code's behaviour is identical to
the new code with device_width 1, we may be able to simply set
the default width to 1 and avoid the nasty back-compat hacks.
(I had to put the hacks in for cfi01 because they really are
different behaviour, and we didn't want to potentially break
the use in the pc platform.)

thanks
-- PMM
Michael Nawrocki Nov. 28, 2017, 7:47 p.m. UTC | #2 
On 11/23/2017 08:46 AM, Peter Maydell wrote:
> On 13 November 2017 at 17:31, Mike Nawrocki
> <michael.nawrocki@gtri.gatech.edu> wrote:
>> This mirrors the interleaving support already in place in
>> pflash_cfi01.c, using the max_device_width and device_width properties.
>>
>> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
>> ---
>>   hw/block/pflash_cfi02.c | 244 +++++++++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 209 insertions(+), 35 deletions(-)
> 
> Thanks for this patch. There is some code duplication from cfi01.c,
> but I think it's ok not to worry about trying to share code.
> 
>>       case 0xA0:
>>       case 0x10:
>>       case 0x30:
>>           /* Status register read */
>>           ret = pfl->status;
>> -        DPRINTF("%s: status %x\n", __func__, ret);
>> +        for (i = pfl->device_width;
>> +             i < pfl->bank_width; i += pfl->device_width) {
>> +            ret = deposit64(ret, 8 * i, 8 * pfl->device_width, ret);
>> +        }
> 
> This will loop forever in the legacy "device_width == 0" case,
> won't it?
>

Ah, yeah looks like it. I'll add a check to fix this.

>> +        DPRINTF("%s: status %" PRIx64 "\n", __func__, ret);
>>           /* Toggle bit 6 */
>>           pfl->status ^= 0x40;
>>           break;
>> @@ -745,7 +901,25 @@ static Property pflash_cfi02_properties[] = {
>>       DEFINE_PROP_DRIVE("drive", struct pflash_t, blk),
>>       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),
>> +    /* 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.
> 
> Is this correct for pflash_cfi02 ? In cfi01 you can see that our
> legacy behaviour does things like jamming two ID values
> together into one response because it's doing this "bad emulation
> of two 16 bit devices", but I don't see similar code in cfi02,
> so maybe the legacy default is something different ?
> 
> In particular, if the legacy default is actually "just looks like
> a single device" and the old code's behaviour is identical to
> the new code with device_width 1, we may be able to simply set
> the default width to 1 and avoid the nasty back-compat hacks.
> (I had to put the hacks in for cfi01 because they really are
> different behaviour, and we didn't want to potentially break
> the use in the pc platform.)
> 
> thanks
> -- PMM
> 
I ran into this issue when emulating a PPMC7400 evaluation board. It has 
a 64-bit wide flash bus comprised of 4 16-bit AMD flash devices (in 
16-bit mode), and the drivers I have send commands to all 4 devices 
simultaneously (by packing the data into quadwords). So it expects 4 
responses packed into a quadword when it reads the response value.

As far as I can tell, the legacy default for pflash_cfi02 is "just looks 
like a single device". I think the legacy behavior is equivalent to 
setting device_width to bank_width in the new code; setting it to 1 in 
the new code would mean the response gets duplicated "bank_width" times 
while setting it to bank_width should result in a single response.

I was referencing pflash_cfi01, so I ended up duplicating the 
back-compat hacks, but as far as I can tell they are unnecessary here, 
and it'd be much cleaner to remove them.

I'll go ahead and remove the back-compat hacks and see if everything 
works as expected. I'll also incorporate your other suggestions and push 
a newer revision of the patch set as soon as possible.
Thanks!
diff mbox series

Patch

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 29eb0429a3..edf683bce5 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -28,11 +28,14 @@ 
  * - unlock bypass command
  * - CFI queries
  *
- * It does not support flash interleaving.
  * It does not implement boot blocs with reduced size
  * It does not implement software data protection as found in many real chips
  * It does not implement erase suspend/resume commands
  * It does not implement multiple sectors erase
+ *
+ * Flash interleaving is partially supported using the device_width and
+ * max_device_width properties, as in pflash_cfi01.c
+ * Issuing commands to individual members of the flash array is not supported.
  */
 
 #include "qemu/osdep.h"
@@ -40,6 +43,7 @@ 
 #include "hw/block/flash.h"
 #include "qapi/error.h"
 #include "qemu/timer.h"
+#include "qemu/bitops.h"
 #include "sysemu/block-backend.h"
 #include "exec/address-spaces.h"
 #include "qemu/host-utils.h"
@@ -69,7 +73,9 @@  struct pflash_t {
     uint32_t nb_blocs;
     uint32_t chip_len;
     uint8_t mappings;
-    uint8_t width;
+    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;
     int wcycle; /* if 0, the flash is read normally */
     int bypass;
@@ -138,6 +144,63 @@  static void pflash_timer (void *opaque)
     pfl->cmd = 0;
 }
 
+static uint64_t pflash_cfi_query(pflash_t *pfl, hwaddr offset)
+{
+    int i;
+    uint64_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 > 8) {
+            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 = deposit64(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 = deposit64(resp, 8 * i, 8 * pfl->device_width, resp);
+        }
+    }
+
+    return resp;
+}
+
 static uint64_t _flash_read(pflash_t *pfl, hwaddr offset,
                             int width, int be)
 {
@@ -200,9 +263,66 @@  static uint64_t _flash_read(pflash_t *pfl, hwaddr offset,
     return ret;
 }
 
+/* Perform a device id query based on the bank width of the flash. */
+static uint64_t pflash_devid_query(pflash_t *pfl, hwaddr offset,
+                                   int width, int be)
+{
+    int i;
+    uint64_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.
+     */
+    switch (boff & 0xFF) {
+    case 0:
+        resp = pfl->ident0;
+        DPRINTF("%s: Manufacturer Code %04x\n", __func__, resp);
+        break;
+    case 1:
+        resp = pfl->ident1;
+        DPRINTF("%s: Device ID Code %04x\n", __func__, resp);
+        break;
+    case 2:
+        resp = 0x00; /* Pretend all sectors are unprotected */
+        break;
+    case 0xE:
+    case 0xF:
+        resp = boff & 0x01 ? pfl->ident3 : pfl->ident2;
+        if (resp != (uint8_t)-1) {
+            break;
+        }
+    default:
+        return _flash_read(pfl, offset, width, be);
+    }
+    /* 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 = deposit64(resp, 8 * i, 8 * pfl->device_width, resp);
+        }
+    }
+
+    return resp;
+}
+
 static uint64_t pflash_read(pflash_t *pfl, hwaddr offset,
                             int width, int be)
 {
+    int i;
     hwaddr boff;
     uint64_t ret;
 
@@ -215,11 +335,11 @@  static uint64_t pflash_read(pflash_t *pfl, hwaddr offset,
     }
     offset &= pfl->chip_len - 1;
     boff = offset & 0xFF;
-    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;
-    else if (pfl->width == 8)
+    else if (pfl->bank_width == 8)
         boff = boff >> 3;
     switch (pfl->cmd) {
     default:
@@ -231,47 +351,76 @@  static uint64_t pflash_read(pflash_t *pfl, hwaddr offset,
     case 0x80:
         /* We accept reads during second unlock sequence... */
     case 0x00:
-    flash_read:
-        /* Flash area read */
         ret = _flash_read(pfl, offset, width, be);
         break;
     case 0x90:
         /* flash ID read */
-        switch (boff) {
-        case 0x00:
-        case 0x01:
-            ret = boff & 0x01 ? pfl->ident1 : pfl->ident0;
-            break;
-        case 0x02:
-            ret = 0x00; /* Pretend all sectors are unprotected */
-            break;
-        case 0x0E:
-        case 0x0F:
-            ret = boff & 0x01 ? pfl->ident3 : pfl->ident2;
-            if (ret == (uint8_t)-1) {
-                goto flash_read;
+        if (!pfl->device_width) {
+            /* Preserve old behavior if device width not specified */
+            switch (boff) {
+            case 0x00:
+            case 0x01:
+                ret = boff & 0x01 ? pfl->ident1 : pfl->ident0;
+                break;
+            case 0x02:
+                ret = 0x00; /* Pretend all sectors are unprotected */
+                break;
+            case 0x0E:
+            case 0x0F:
+                ret = boff & 0x01 ? pfl->ident3 : pfl->ident2;
+                if (ret != (uint8_t)-1) {
+                    break;
+                }
+            default:
+                ret = _flash_read(pfl, offset, width, be);
+            }
+        } 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 = deposit64(ret, i * 8, pfl->bank_width * 8,
+                                pflash_devid_query(pfl,
+                                                  offset + i * pfl->bank_width,
+                                                  width, be));
             }
-            break;
-        default:
-            goto flash_read;
         }
-        DPRINTF("%s: ID " TARGET_FMT_plx " %x\n", __func__, boff, ret);
         break;
     case 0xA0:
     case 0x10:
     case 0x30:
         /* Status register read */
         ret = pfl->status;
-        DPRINTF("%s: status %x\n", __func__, ret);
+        for (i = pfl->device_width;
+             i < pfl->bank_width; i += pfl->device_width) {
+            ret = deposit64(ret, 8 * i, 8 * pfl->device_width, ret);
+        }
+        DPRINTF("%s: status %" PRIx64 "\n", __func__, ret);
         /* Toggle bit 6 */
         pfl->status ^= 0x40;
         break;
     case 0x98:
         /* CFI 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 */
+            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 = deposit64(ret, i * 8, pfl->bank_width * 8,
+                                pflash_cfi_query(pfl,
+                                                 offset + i * pfl->bank_width));
+            }
+        }
+
         break;
     }
 
@@ -315,11 +464,11 @@  static void pflash_write(pflash_t *pfl, hwaddr offset,
     DPRINTF("%s: offset " TARGET_FMT_plx " %08" PRIx64 " %d\n",
             __func__, offset, value, width);
     boff = offset & (pfl->sector_len - 1);
-    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;
-    else if (pfl->width == 8)
+    else if (pfl->bank_width == 8)
         boff = boff >> 3;
     switch (pfl->wcycle) {
     case 0:
@@ -654,6 +803,13 @@  static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
         }
     }
 
+    /* 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;
+    }
+
     pflash_setup_mappings(pfl);
     pfl->rom_mode = 1;
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
@@ -745,7 +901,25 @@  static Property pflash_cfi02_properties[] = {
     DEFINE_PROP_DRIVE("drive", struct pflash_t, blk),
     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),
+    /* 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.
+     */
+    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("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),
@@ -785,7 +959,7 @@  pflash_t *pflash_cfi02_register(hwaddr base,
                                 DeviceState *qdev, const char *name,
                                 hwaddr size,
                                 BlockBackend *blk, uint32_t sector_len,
-                                int nb_blocs, int nb_mappings, int width,
+                                int nb_blocs, int nb_mappings, int bank_width,
                                 uint16_t id0, uint16_t id1,
                                 uint16_t id2, uint16_t id3,
                                 uint16_t unlock_addr0, uint16_t unlock_addr1,
@@ -798,7 +972,7 @@  pflash_t *pflash_cfi02_register(hwaddr base,
     }
     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, "width", bank_width);
     qdev_prop_set_uint8(dev, "mappings", nb_mappings);
     qdev_prop_set_uint8(dev, "big-endian", !!be);
     qdev_prop_set_uint16(dev, "id0", id0);