[Qemu-devel] [PATCH v3 0/3] Add 8-byte wide AMD flash support, partial interleaving

[Qemu-devel] [PATCH v3 0/3] Add 8-byte wide AMD flash support, partial interleaving

[Mike Nawrocki]

This patch set does a few things. First, it switches the AMD CFI flash MMIO
operations from the old MMIO API to the new one. Second, it enables 8-byte wide
flash arrays. Finally, it adds flash interleaving using the "device-width" and
"max-device-width" properties, using the same interface as pflash_cfi01.c. Much
of the code was taken and adapted from that file.

Version 3 of the patch splits the patch set into distinct patches based on the
modified functionality.

Mike Nawrocki (3):
  Switch AMD CFI flash to use new MMIO API
  Enable 8-byte wide access to AMD CFI devices
  Add partial flash interleaving to AMD CFI devices

 hw/block/pflash_cfi02.c | 478 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 326 insertions(+), 152 deletions(-)

-- 
2.14.2

[Qemu-devel] [PATCH v3 1/3] Switch AMD CFI flash to use new MMIO API

[Mike Nawrocki]

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

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index c81ddd3a99..a81df913f6 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -138,12 +138,12 @@ static void pflash_timer (void *opaque)
     pfl->cmd = 0;
 }
 
-static uint32_t pflash_read (pflash_t *pfl, hwaddr offset,
-                             int width, int be)
+static uint64_t pflash_read(pflash_t *pfl, hwaddr offset,
+                            int width, int be)
 {
     hwaddr boff;
-    uint32_t ret;
     uint8_t *p;
+    uint64_t ret;
 
     DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
     ret = -1;
@@ -261,7 +261,7 @@ static void pflash_update(pflash_t *pfl, int offset,
 }
 
 static void pflash_write (pflash_t *pfl, hwaddr offset,
-                          uint32_t value, int width, int be)
+                          uint64_t value, int width, int be)
 {
     hwaddr boff;
     uint8_t *p;
@@ -494,102 +494,41 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
     pfl->cmd = 0;
 }
 
-
-static uint32_t pflash_readb_be(void *opaque, hwaddr addr)
-{
-    return pflash_read(opaque, addr, 1, 1);
-}
-
-static uint32_t pflash_readb_le(void *opaque, hwaddr addr)
-{
-    return pflash_read(opaque, addr, 1, 0);
-}
-
-static uint32_t pflash_readw_be(void *opaque, hwaddr addr)
+static uint64_t pflash_read_le(void *opaque, hwaddr addr, unsigned size)
 {
     pflash_t *pfl = opaque;
-
-    return pflash_read(pfl, addr, 2, 1);
+    return pflash_read(pfl, addr, size, 0);
 }
 
-static uint32_t pflash_readw_le(void *opaque, hwaddr addr)
+static uint64_t pflash_read_be(void *opaque, hwaddr addr, unsigned size)
 {
     pflash_t *pfl = opaque;
-
-    return pflash_read(pfl, addr, 2, 0);
+    return pflash_read(pfl, addr, size, 1);
 }
 
-static uint32_t pflash_readl_be(void *opaque, hwaddr addr)
+static void pflash_write_le(void *opaque, hwaddr addr, uint64_t data,
+                            unsigned size)
 {
     pflash_t *pfl = opaque;
-
-    return pflash_read(pfl, addr, 4, 1);
+    pflash_write(pfl, addr, data, size, 0);
 }
 
-static uint32_t pflash_readl_le(void *opaque, hwaddr addr)
+static void pflash_write_be(void *opaque, hwaddr addr, uint64_t data,
+                            unsigned size)
 {
     pflash_t *pfl = opaque;
-
-    return pflash_read(pfl, addr, 4, 0);
-}
-
-static void pflash_writeb_be(void *opaque, hwaddr addr,
-                             uint32_t value)
-{
-    pflash_write(opaque, addr, value, 1, 1);
-}
-
-static void pflash_writeb_le(void *opaque, hwaddr addr,
-                             uint32_t value)
-{
-    pflash_write(opaque, addr, value, 1, 0);
-}
-
-static void pflash_writew_be(void *opaque, hwaddr addr,
-                             uint32_t value)
-{
-    pflash_t *pfl = opaque;
-
-    pflash_write(pfl, addr, value, 2, 1);
-}
-
-static void pflash_writew_le(void *opaque, hwaddr addr,
-                             uint32_t value)
-{
-    pflash_t *pfl = opaque;
-
-    pflash_write(pfl, addr, value, 2, 0);
-}
-
-static void pflash_writel_be(void *opaque, hwaddr addr,
-                             uint32_t value)
-{
-    pflash_t *pfl = opaque;
-
-    pflash_write(pfl, addr, value, 4, 1);
-}
-
-static void pflash_writel_le(void *opaque, hwaddr addr,
-                             uint32_t value)
-{
-    pflash_t *pfl = opaque;
-
-    pflash_write(pfl, addr, value, 4, 0);
+    pflash_write(pfl, addr, data, size, 1);
 }
 
 static const MemoryRegionOps pflash_cfi02_ops_be = {
-    .old_mmio = {
-        .read = { pflash_readb_be, pflash_readw_be, pflash_readl_be, },
-        .write = { pflash_writeb_be, pflash_writew_be, pflash_writel_be, },
-    },
+    .read = pflash_read_be,
+    .write = pflash_write_be,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
 static const MemoryRegionOps pflash_cfi02_ops_le = {
-    .old_mmio = {
-        .read = { pflash_readb_le, pflash_readw_le, pflash_readl_le, },
-        .write = { pflash_writeb_le, pflash_writew_le, pflash_writel_le, },
-    },
+    .read = pflash_read_le,
+    .write = pflash_write_le,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-- 
2.14.2

Re: [Qemu-devel] [PATCH v3 1/3] Switch AMD CFI flash to use new MMIO API

[Peter Maydell]

On 13 November 2017 at 16:14, Mike Nawrocki
<michael.nawrocki@gtri.gatech.edu> wrote:
> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
> ---
>  hw/block/pflash_cfi02.c | 97 +++++++++----------------------------------------
>  1 file changed, 18 insertions(+), 79 deletions(-)
>
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index c81ddd3a99..a81df913f6 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -138,12 +138,12 @@ static void pflash_timer (void *opaque)
>      pfl->cmd = 0;
>  }
>
> -static uint32_t pflash_read (pflash_t *pfl, hwaddr offset,
> -                             int width, int be)
> +static uint64_t pflash_read(pflash_t *pfl, hwaddr offset,
> +                            int width, int be)
>  {
>      hwaddr boff;
> -    uint32_t ret;
>      uint8_t *p;
> +    uint64_t ret;

I suspect you'll find that the change of type for 'ret' here
and the 'value' argument to pflash_write() will break compilation
with PFLASH_DEBUG defined, because the type won't match the DPRINTF
format strings any more.

You could either fix up the format strings, or (since there's a
wrapper function here anyway) leave the types of pflash_read()
and pflash_write() alone and let the wrappers implicitly do
the conversion between uint64_t and uint32_t.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 1/3] Switch AMD CFI flash to use new MMIO API

[Peter Maydell]

On 23 November 2017 at 11:26, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 13 November 2017 at 16:14, Mike Nawrocki
> <michael.nawrocki@gtri.gatech.edu> wrote:
>> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
>> ---
>>  hw/block/pflash_cfi02.c | 97 +++++++++----------------------------------------
>>  1 file changed, 18 insertions(+), 79 deletions(-)
>>
>> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>> index c81ddd3a99..a81df913f6 100644
>> --- a/hw/block/pflash_cfi02.c
>> +++ b/hw/block/pflash_cfi02.c
>> @@ -138,12 +138,12 @@ static void pflash_timer (void *opaque)
>>      pfl->cmd = 0;
>>  }
>>
>> -static uint32_t pflash_read (pflash_t *pfl, hwaddr offset,
>> -                             int width, int be)
>> +static uint64_t pflash_read(pflash_t *pfl, hwaddr offset,
>> +                            int width, int be)
>>  {
>>      hwaddr boff;
>> -    uint32_t ret;
>>      uint8_t *p;
>> +    uint64_t ret;
>
> I suspect you'll find that the change of type for 'ret' here
> and the 'value' argument to pflash_write() will break compilation
> with PFLASH_DEBUG defined, because the type won't match the DPRINTF
> format strings any more.
>
> You could either fix up the format strings, or (since there's a
> wrapper function here anyway) leave the types of pflash_read()
> and pflash_write() alone and let the wrappers implicitly do
> the conversion between uint64_t and uint32_t.

...ah, just noticed that in patch 2 you want to add 8-byte
accesses. So you'll need to fix the format strings.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 1/3] Switch AMD CFI flash to use new MMIO API

[Michael Nawrocki]

On 11/23/2017 06:27 AM, Peter Maydell wrote:
> On 23 November 2017 at 11:26, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 13 November 2017 at 16:14, Mike Nawrocki
>> <michael.nawrocki@gtri.gatech.edu> wrote:
>>> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
>>> ---
>>>   hw/block/pflash_cfi02.c | 97 +++++++++----------------------------------------
>>>   1 file changed, 18 insertions(+), 79 deletions(-)
>>>
>>> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>>> index c81ddd3a99..a81df913f6 100644
>>> --- a/hw/block/pflash_cfi02.c
>>> +++ b/hw/block/pflash_cfi02.c
>>> @@ -138,12 +138,12 @@ static void pflash_timer (void *opaque)
>>>       pfl->cmd = 0;
>>>   }
>>>
>>> -static uint32_t pflash_read (pflash_t *pfl, hwaddr offset,
>>> -                             int width, int be)
>>> +static uint64_t pflash_read(pflash_t *pfl, hwaddr offset,
>>> +                            int width, int be)
>>>   {
>>>       hwaddr boff;
>>> -    uint32_t ret;
>>>       uint8_t *p;
>>> +    uint64_t ret;
>>
>> I suspect you'll find that the change of type for 'ret' here
>> and the 'value' argument to pflash_write() will break compilation
>> with PFLASH_DEBUG defined, because the type won't match the DPRINTF
>> format strings any more.
>>
>> You could either fix up the format strings, or (since there's a
>> wrapper function here anyway) leave the types of pflash_read()
>> and pflash_write() alone and let the wrappers implicitly do
>> the conversion between uint64_t and uint32_t.
> 
> ...ah, just noticed that in patch 2 you want to add 8-byte
> accesses. So you'll need to fix the format strings.
> 
> thanks
> -- PMM
> 

Yeah, it definitely doesn't compile with PFLASH_DEBUG defined. I'll 
update those format strings for the next revision.
Thanks!

Re: [Qemu-devel] [PATCH v3 1/3] Switch AMD CFI flash to use new MMIO API

[Eric Blake]

On 11/28/2017 01:47 PM, Michael Nawrocki wrote:
>>> I suspect you'll find that the change of type for 'ret' here
>>> and the 'value' argument to pflash_write() will break compilation
>>> with PFLASH_DEBUG defined, because the type won't match the DPRINTF
>>> format strings any more.
>>>
>>> You could either fix up the format strings, or (since there's a
>>> wrapper function here anyway) leave the types of pflash_read()
>>> and pflash_write() alone and let the wrappers implicitly do
>>> the conversion between uint64_t and uint32_t.
>>
>> ...ah, just noticed that in patch 2 you want to add 8-byte
>> accesses. So you'll need to fix the format strings.
>>
>> thanks
>> -- PMM
>>
> 
> Yeah, it definitely doesn't compile with PFLASH_DEBUG defined. I'll 
> update those format strings for the next revision.

Better yet, please fix the PFLASH_DEBUG code to avoid bitrot in the 
first place, by rewriting the bad pattern:

/* #define PFLASH_DEBUG */
#ifdef PFLASH_DEBUG
#define DPRINTF(fmt, ...)                                   \
do {                                                        \
     fprintf(stderr, "PFLASH: " fmt , ## __VA_ARGS__);       \
} while (0)
#else
#define DPRINTF(fmt, ...) do { } while (0)
#endif

into the good pattern:

#ifdef PFLASH_DEBUG
# define PFLASH_PRINT 1
#else
# define PFLASH_PRINT 0
#endif
#define DPRINTF(fmt, ...) \
do { \
   if (PFLASH_PRINT) { \
     fprintf(stderr, "PFLASH: " fmt, ## __VA_ARGS__); \
   }; \
} while (0)

or even better, using the trace infrastructure instead.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3 1/3] Switch AMD CFI flash to use new MMIO API

[Eric Blake]

On 11/28/2017 02:05 PM, Eric Blake wrote:

> Better yet, please fix the PFLASH_DEBUG code to avoid bitrot in the 
> first place, by rewriting the bad pattern:
> 

> into the good pattern:
> 
> #ifdef PFLASH_DEBUG
> # define PFLASH_PRINT 1
> #else
> # define PFLASH_PRINT 0
> #endif
> #define DPRINTF(fmt, ...) \
> do { \
>    if (PFLASH_PRINT) { \
>      fprintf(stderr, "PFLASH: " fmt, ## __VA_ARGS__); \
>    }; \

Serves me right for typing this without testing; the extra ; is spurious.

By the way, this is one of the Bite-Sized Tasks that never seems to get 
finished...
https://wiki.qemu.org/BiteSizedTasks#Bitrot_prevention

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[Qemu-devel] [PATCH v3 2/3] Enable 8-byte wide access to AMD CFI devices

[Mike Nawrocki]

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

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index a81df913f6..29eb0429a3 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -18,7 +18,7 @@
  */
 
 /*
- * For now, this code can emulate flashes of 1, 2 or 4 bytes width.
+ * For now, this code can emulate flashes of 1, 2, 4, or 8 bytes width.
  * Supported commands/modes are:
  * - flash read
  * - flash write
@@ -138,11 +138,72 @@ static void pflash_timer (void *opaque)
     pfl->cmd = 0;
 }
 
+static uint64_t _flash_read(pflash_t *pfl, hwaddr offset,
+                            int width, int be)
+{
+    /* Flash area read */
+    uint64_t ret = 0;
+    uint8_t *p = pfl->storage;
+
+    switch (width) {
+    case 1:
+        ret = p[offset];
+        /* DPRINTF("%s: data offset %08x %02x\n", __func__, offset, ret); */
+        break;
+    case 2:
+        if (be) {
+            ret = p[offset] << 8;
+            ret |= p[offset + 1];
+        } else {
+            ret = p[offset];
+            ret |= p[offset + 1] << 8;
+        }
+        /* DPRINTF("%s: data offset %08x %04x\n", __func__, offset, ret); */
+        break;
+    case 4:
+        if (be) {
+            ret = p[offset] << 24;
+            ret |= p[offset + 1] << 16;
+            ret |= p[offset + 2] << 8;
+            ret |= p[offset + 3];
+        } else {
+            ret = p[offset];
+            ret |= p[offset + 1] << 8;
+            ret |= p[offset + 2] << 16;
+            ret |= p[offset + 3] << 24;
+        }
+        /* DPRINTF("%s: data offset %08x %08x\n", __func__, offset, ret); */
+        break;
+    case 8:
+        if (be) {
+            ret = (uint64_t)p[offset] << 56;
+            ret |= (uint64_t)p[offset + 1] << 48;
+            ret |= (uint64_t)p[offset + 2] << 40;
+            ret |= (uint64_t)p[offset + 3] << 32;
+            ret |= (uint64_t)p[offset + 4] << 24;
+            ret |= (uint64_t)p[offset + 5] << 16;
+            ret |= (uint64_t)p[offset + 6] << 8;
+            ret |= (uint64_t)p[offset + 7];
+        } else {
+            ret = (uint64_t)p[offset];
+            ret |= (uint64_t)p[offset + 1] << 8;
+            ret |= (uint64_t)p[offset + 2] << 16;
+            ret |= (uint64_t)p[offset + 3] << 24;
+            ret |= (uint64_t)p[offset + 4] << 32;
+            ret |= (uint64_t)p[offset + 5] << 40;
+            ret |= (uint64_t)p[offset + 6] << 48;
+            ret |= (uint64_t)p[offset + 7] << 56;
+        }
+        break;
+    }
+
+    return ret;
+}
+
 static uint64_t pflash_read(pflash_t *pfl, hwaddr offset,
                             int width, int be)
 {
     hwaddr boff;
-    uint8_t *p;
     uint64_t ret;
 
     DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
@@ -158,6 +219,8 @@ static uint64_t pflash_read(pflash_t *pfl, hwaddr offset,
         boff = boff >> 1;
     else if (pfl->width == 4)
         boff = boff >> 2;
+    else if (pfl->width == 8)
+        boff = boff >> 3;
     switch (pfl->cmd) {
     default:
         /* This should never happen : reset state & treat it as a read*/
@@ -170,37 +233,7 @@ static uint64_t pflash_read(pflash_t *pfl, hwaddr offset,
     case 0x00:
     flash_read:
         /* Flash area read */
-        p = pfl->storage;
-        switch (width) {
-        case 1:
-            ret = p[offset];
-//            DPRINTF("%s: data offset %08x %02x\n", __func__, offset, ret);
-            break;
-        case 2:
-            if (be) {
-                ret = p[offset] << 8;
-                ret |= p[offset + 1];
-            } else {
-                ret = p[offset];
-                ret |= p[offset + 1] << 8;
-            }
-//            DPRINTF("%s: data offset %08x %04x\n", __func__, offset, ret);
-            break;
-        case 4:
-            if (be) {
-                ret = p[offset] << 24;
-                ret |= p[offset + 1] << 16;
-                ret |= p[offset + 2] << 8;
-                ret |= p[offset + 3];
-            } else {
-                ret = p[offset];
-                ret |= p[offset + 1] << 8;
-                ret |= p[offset + 2] << 16;
-                ret |= p[offset + 3] << 24;
-            }
-//            DPRINTF("%s: data offset %08x %08x\n", __func__, offset, ret);
-            break;
-        }
+        ret = _flash_read(pfl, offset, width, be);
         break;
     case 0x90:
         /* flash ID read */
@@ -260,8 +293,8 @@ static void pflash_update(pflash_t *pfl, int offset,
     }
 }
 
-static void pflash_write (pflash_t *pfl, hwaddr offset,
-                          uint64_t value, int width, int be)
+static void pflash_write(pflash_t *pfl, hwaddr offset,
+                         uint64_t value, int width, int be)
 {
     hwaddr boff;
     uint8_t *p;
@@ -275,17 +308,19 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
 #endif
         goto reset_flash;
     }
-    DPRINTF("%s: offset " TARGET_FMT_plx " %08x %d %d\n", __func__,
-            offset, value, width, pfl->wcycle);
+    DPRINTF("%s: offset " TARGET_FMT_plx " %08" PRIx64 " %d %d\n",
+            __func__, offset, value, width, pfl->wcycle);
     offset &= pfl->chip_len - 1;
 
-    DPRINTF("%s: offset " TARGET_FMT_plx " %08x %d\n", __func__,
-            offset, value, width);
+    DPRINTF("%s: offset " TARGET_FMT_plx " %08" PRIx64 " %d\n",
+            __func__, offset, value, width);
     boff = offset & (pfl->sector_len - 1);
     if (pfl->width == 2)
         boff = boff >> 1;
     else if (pfl->width == 4)
         boff = boff >> 2;
+    else if (pfl->width == 8)
+        boff = boff >> 3;
     switch (pfl->wcycle) {
     case 0:
         /* Set the device in I/O access mode if required */
@@ -346,8 +381,8 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
             /* We need another unlock sequence */
             goto check_unlock0;
         case 0xA0:
-            DPRINTF("%s: write data offset " TARGET_FMT_plx " %08x %d\n",
-                    __func__, offset, value, width);
+            DPRINTF("%s: write data offset " TARGET_FMT_plx
+                    " %08" PRIx64 " %d\n", __func__, offset, value, width);
             p = pfl->storage;
             if (!pfl->ro) {
                 switch (width) {
@@ -379,6 +414,28 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
                     }
                     pflash_update(pfl, offset, 4);
                     break;
+                case 8:
+                    if (be) {
+                        p[offset] &= value >> 56;
+                        p[offset + 1] &= value >> 48;
+                        p[offset + 2] &= value >> 40;
+                        p[offset + 3] &= value >> 32;
+                        p[offset + 4] &= value >> 24;
+                        p[offset + 5] &= value >> 16;
+                        p[offset + 6] &= value >> 8;
+                        p[offset + 7] &= value;
+                    } else {
+                        p[offset] &= value;
+                        p[offset + 1] &= value >> 8;
+                        p[offset + 2] &= value >> 16;
+                        p[offset + 3] &= value >> 24;
+                        p[offset + 4] &= value >> 32;
+                        p[offset + 5] &= value >> 40;
+                        p[offset + 6] &= value >> 48;
+                        p[offset + 7] &= value >> 56;
+                    }
+                    pflash_update(pfl, offset, 8);
+                    break;
                 }
             }
             pfl->status = 0x00 | ~(value & 0x80);
@@ -524,12 +581,16 @@ static const MemoryRegionOps pflash_cfi02_ops_be = {
     .read = pflash_read_be,
     .write = pflash_write_be,
     .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid.max_access_size = 8,
+    .impl.max_access_size = 8,
 };
 
 static const MemoryRegionOps pflash_cfi02_ops_le = {
     .read = pflash_read_le,
     .write = pflash_write_le,
     .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid.max_access_size = 8,
+    .impl.max_access_size = 8,
 };
 
 static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
-- 
2.14.2

Re: [Qemu-devel] [PATCH v3 2/3] Enable 8-byte wide access to AMD CFI devices

[Peter Maydell]

On 13 November 2017 at 16:14, Mike Nawrocki
<michael.nawrocki@gtri.gatech.edu> wrote:
> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
> ---
>  hw/block/pflash_cfi02.c | 143 ++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 102 insertions(+), 41 deletions(-)
>
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index a81df913f6..29eb0429a3 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -18,7 +18,7 @@
>   */
>
>  /*
> - * For now, this code can emulate flashes of 1, 2 or 4 bytes width.
> + * For now, this code can emulate flashes of 1, 2, 4, or 8 bytes width.
>   * Supported commands/modes are:
>   * - flash read
>   * - flash write
> @@ -138,11 +138,72 @@ static void pflash_timer (void *opaque)
>      pfl->cmd = 0;
>  }
>
> +static uint64_t _flash_read(pflash_t *pfl, hwaddr offset,
> +                            int width, int be)

Don't use leading underscores in function names, please --
they are reserved for the system (C library, etc).

> +{
> +    /* Flash area read */
> +    uint64_t ret = 0;
> +    uint8_t *p = pfl->storage;
> +
> +    switch (width) {
> +    case 1:
> +        ret = p[offset];
> +        /* DPRINTF("%s: data offset %08x %02x\n", __func__, offset, ret); */
> +        break;
> +    case 2:
> +        if (be) {
> +            ret = p[offset] << 8;
> +            ret |= p[offset + 1];
> +        } else {
> +            ret = p[offset];
> +            ret |= p[offset + 1] << 8;
> +        }
> +        /* DPRINTF("%s: data offset %08x %04x\n", __func__, offset, ret); */
> +        break;
> +    case 4:
> +        if (be) {
> +            ret = p[offset] << 24;
> +            ret |= p[offset + 1] << 16;
> +            ret |= p[offset + 2] << 8;
> +            ret |= p[offset + 3];
> +        } else {
> +            ret = p[offset];
> +            ret |= p[offset + 1] << 8;
> +            ret |= p[offset + 2] << 16;
> +            ret |= p[offset + 3] << 24;
> +        }
> +        /* DPRINTF("%s: data offset %08x %08x\n", __func__, offset, ret); */
> +        break;
> +    case 8:
> +        if (be) {
> +            ret = (uint64_t)p[offset] << 56;
> +            ret |= (uint64_t)p[offset + 1] << 48;
> +            ret |= (uint64_t)p[offset + 2] << 40;
> +            ret |= (uint64_t)p[offset + 3] << 32;
> +            ret |= (uint64_t)p[offset + 4] << 24;
> +            ret |= (uint64_t)p[offset + 5] << 16;
> +            ret |= (uint64_t)p[offset + 6] << 8;
> +            ret |= (uint64_t)p[offset + 7];
> +        } else {
> +            ret = (uint64_t)p[offset];
> +            ret |= (uint64_t)p[offset + 1] << 8;
> +            ret |= (uint64_t)p[offset + 2] << 16;
> +            ret |= (uint64_t)p[offset + 3] << 24;
> +            ret |= (uint64_t)p[offset + 4] << 32;
> +            ret |= (uint64_t)p[offset + 5] << 40;
> +            ret |= (uint64_t)p[offset + 6] << 48;
> +            ret |= (uint64_t)p[offset + 7] << 56;
> +        }
> +        break;
> +    }

This can be rewritten in a shorter and clearer way using
the ld*_le_p() and ld*_be_p() functions, eg case 4 is

    ret = be ? ldl_be_p(p + offset) : ldl_le_p(p + offset);


case 2 uses lduw_be_p() and lduw_le_p(), case 8 uses
ldq_be_p() and ldq_le_p().

case 1 is just
    ret = ldub_p(p + offset);
because there's no endianness issue for 1 byte.


> +
> +    return ret;
> +}
> +
>  static uint64_t pflash_read(pflash_t *pfl, hwaddr offset,
>                              int width, int be)
>  {
>      hwaddr boff;
> -    uint8_t *p;
>      uint64_t ret;
>
>      DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
> @@ -158,6 +219,8 @@ static uint64_t pflash_read(pflash_t *pfl, hwaddr offset,
>          boff = boff >> 1;
>      else if (pfl->width == 4)
>          boff = boff >> 2;
> +    else if (pfl->width == 8)
> +        boff = boff >> 3;

If you run this patch through scripts/checkpatch.pl it should tell
you that our coding style would like you to put braces on this if.
Our usual practice is that if you're touching an existing statement
that isn't in line with our coding style you should fix at least the
parts checkpatch complains about (ie add {} to all the arms of the
if, here).

>      switch (pfl->cmd) {
>      default:
>          /* This should never happen : reset state & treat it as a read*/
> @@ -170,37 +233,7 @@ static uint64_t pflash_read(pflash_t *pfl, hwaddr offset,
>      case 0x00:
>      flash_read:
>          /* Flash area read */
> -        p = pfl->storage;
> -        switch (width) {
> -        case 1:
> -            ret = p[offset];
> -//            DPRINTF("%s: data offset %08x %02x\n", __func__, offset, ret);
> -            break;
> -        case 2:
> -            if (be) {
> -                ret = p[offset] << 8;
> -                ret |= p[offset + 1];
> -            } else {
> -                ret = p[offset];
> -                ret |= p[offset + 1] << 8;
> -            }
> -//            DPRINTF("%s: data offset %08x %04x\n", __func__, offset, ret);
> -            break;
> -        case 4:
> -            if (be) {
> -                ret = p[offset] << 24;
> -                ret |= p[offset + 1] << 16;
> -                ret |= p[offset + 2] << 8;
> -                ret |= p[offset + 3];
> -            } else {
> -                ret = p[offset];
> -                ret |= p[offset + 1] << 8;
> -                ret |= p[offset + 2] << 16;
> -                ret |= p[offset + 3] << 24;
> -            }
> -//            DPRINTF("%s: data offset %08x %08x\n", __func__, offset, ret);
> -            break;
> -        }
> +        ret = _flash_read(pfl, offset, width, be);
>          break;
>      case 0x90:
>          /* flash ID read */
> @@ -260,8 +293,8 @@ static void pflash_update(pflash_t *pfl, int offset,
>      }
>  }
>
> -static void pflash_write (pflash_t *pfl, hwaddr offset,
> -                          uint64_t value, int width, int be)
> +static void pflash_write(pflash_t *pfl, hwaddr offset,
> +                         uint64_t value, int width, int be)
>  {
>      hwaddr boff;
>      uint8_t *p;
> @@ -275,17 +308,19 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
>  #endif
>          goto reset_flash;
>      }
> -    DPRINTF("%s: offset " TARGET_FMT_plx " %08x %d %d\n", __func__,
> -            offset, value, width, pfl->wcycle);
> +    DPRINTF("%s: offset " TARGET_FMT_plx " %08" PRIx64 " %d %d\n",
> +            __func__, offset, value, width, pfl->wcycle);
>      offset &= pfl->chip_len - 1;
>
> -    DPRINTF("%s: offset " TARGET_FMT_plx " %08x %d\n", __func__,
> -            offset, value, width);
> +    DPRINTF("%s: offset " TARGET_FMT_plx " %08" PRIx64 " %d\n",
> +            __func__, offset, value, width);
>      boff = offset & (pfl->sector_len - 1);
>      if (pfl->width == 2)
>          boff = boff >> 1;
>      else if (pfl->width == 4)
>          boff = boff >> 2;
> +    else if (pfl->width == 8)
> +        boff = boff >> 3;
>      switch (pfl->wcycle) {
>      case 0:
>          /* Set the device in I/O access mode if required */
> @@ -346,8 +381,8 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
>              /* We need another unlock sequence */
>              goto check_unlock0;
>          case 0xA0:
> -            DPRINTF("%s: write data offset " TARGET_FMT_plx " %08x %d\n",
> -                    __func__, offset, value, width);
> +            DPRINTF("%s: write data offset " TARGET_FMT_plx
> +                    " %08" PRIx64 " %d\n", __func__, offset, value, width);
>              p = pfl->storage;
>              if (!pfl->ro) {
>                  switch (width) {
> @@ -379,6 +414,28 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
>                      }
>                      pflash_update(pfl, offset, 4);
>                      break;
> +                case 8:
> +                    if (be) {
> +                        p[offset] &= value >> 56;
> +                        p[offset + 1] &= value >> 48;
> +                        p[offset + 2] &= value >> 40;
> +                        p[offset + 3] &= value >> 32;
> +                        p[offset + 4] &= value >> 24;
> +                        p[offset + 5] &= value >> 16;
> +                        p[offset + 6] &= value >> 8;
> +                        p[offset + 7] &= value;
> +                    } else {
> +                        p[offset] &= value;
> +                        p[offset + 1] &= value >> 8;
> +                        p[offset + 2] &= value >> 16;
> +                        p[offset + 3] &= value >> 24;
> +                        p[offset + 4] &= value >> 32;
> +                        p[offset + 5] &= value >> 40;
> +                        p[offset + 6] &= value >> 48;
> +                        p[offset + 7] &= value >> 56;
> +                    }
> +                    pflash_update(pfl, offset, 8);
> +                    break;
>                  }

Similarly here we can use
   if (be) {
       uint64_t oldv = ldq_be_p(p + offset);
       stq_be_p(p + offset, oldv & value);
   } else {
       ...ditto with _le_
   }

Other than those minor issues, patch looks good.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 2/3] Enable 8-byte wide access to AMD CFI devices

[Michael Nawrocki]

On 11/23/2017 06:49 AM, Peter Maydell wrote:
> On 13 November 2017 at 16:14, Mike Nawrocki
> <michael.nawrocki@gtri.gatech.edu> wrote:
>> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
>> ---
>>   hw/block/pflash_cfi02.c | 143 ++++++++++++++++++++++++++++++++++--------------
>>   1 file changed, 102 insertions(+), 41 deletions(-)
>>
>> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>> index a81df913f6..29eb0429a3 100644
>> --- a/hw/block/pflash_cfi02.c
>> +++ b/hw/block/pflash_cfi02.c
>> @@ -18,7 +18,7 @@
>>    */
>>
>>   /*
>> - * For now, this code can emulate flashes of 1, 2 or 4 bytes width.
>> + * For now, this code can emulate flashes of 1, 2, 4, or 8 bytes width.
>>    * Supported commands/modes are:
>>    * - flash read
>>    * - flash write
>> @@ -138,11 +138,72 @@ static void pflash_timer (void *opaque)
>>       pfl->cmd = 0;
>>   }
>>
>> +static uint64_t _flash_read(pflash_t *pfl, hwaddr offset,
>> +                            int width, int be)
> 
> Don't use leading underscores in function names, please --
> they are reserved for the system (C library, etc).

Gotcha, I'll update this.

> 
>> +{
>> +    /* Flash area read */
>> +    uint64_t ret = 0;
>> +    uint8_t *p = pfl->storage;
>> +
>> +    switch (width) {
>> +    case 1:
>> +        ret = p[offset];
>> +        /* DPRINTF("%s: data offset %08x %02x\n", __func__, offset, ret); */
>> +        break;
>> +    case 2:
>> +        if (be) {
>> +            ret = p[offset] << 8;
>> +            ret |= p[offset + 1];
>> +        } else {
>> +            ret = p[offset];
>> +            ret |= p[offset + 1] << 8;
>> +        }
>> +        /* DPRINTF("%s: data offset %08x %04x\n", __func__, offset, ret); */
>> +        break;
>> +    case 4:
>> +        if (be) {
>> +            ret = p[offset] << 24;
>> +            ret |= p[offset + 1] << 16;
>> +            ret |= p[offset + 2] << 8;
>> +            ret |= p[offset + 3];
>> +        } else {
>> +            ret = p[offset];
>> +            ret |= p[offset + 1] << 8;
>> +            ret |= p[offset + 2] << 16;
>> +            ret |= p[offset + 3] << 24;
>> +        }
>> +        /* DPRINTF("%s: data offset %08x %08x\n", __func__, offset, ret); */
>> +        break;
>> +    case 8:
>> +        if (be) {
>> +            ret = (uint64_t)p[offset] << 56;
>> +            ret |= (uint64_t)p[offset + 1] << 48;
>> +            ret |= (uint64_t)p[offset + 2] << 40;
>> +            ret |= (uint64_t)p[offset + 3] << 32;
>> +            ret |= (uint64_t)p[offset + 4] << 24;
>> +            ret |= (uint64_t)p[offset + 5] << 16;
>> +            ret |= (uint64_t)p[offset + 6] << 8;
>> +            ret |= (uint64_t)p[offset + 7];
>> +        } else {
>> +            ret = (uint64_t)p[offset];
>> +            ret |= (uint64_t)p[offset + 1] << 8;
>> +            ret |= (uint64_t)p[offset + 2] << 16;
>> +            ret |= (uint64_t)p[offset + 3] << 24;
>> +            ret |= (uint64_t)p[offset + 4] << 32;
>> +            ret |= (uint64_t)p[offset + 5] << 40;
>> +            ret |= (uint64_t)p[offset + 6] << 48;
>> +            ret |= (uint64_t)p[offset + 7] << 56;
>> +        }
>> +        break;
>> +    }
> 
> This can be rewritten in a shorter and clearer way using
> the ld*_le_p() and ld*_be_p() functions, eg case 4 is
> 
>      ret = be ? ldl_be_p(p + offset) : ldl_le_p(p + offset);
> 
> 
> case 2 uses lduw_be_p() and lduw_le_p(), case 8 uses
> ldq_be_p() and ldq_le_p().
> 
> case 1 is just
>      ret = ldub_p(p + offset);
> because there's no endianness issue for 1 byte.
> 
> 

Agreed, I'll switch these over.

>> +
>> +    return ret;
>> +}
>> +
>>   static uint64_t pflash_read(pflash_t *pfl, hwaddr offset,
>>                               int width, int be)
>>   {
>>       hwaddr boff;
>> -    uint8_t *p;
>>       uint64_t ret;
>>
>>       DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
>> @@ -158,6 +219,8 @@ static uint64_t pflash_read(pflash_t *pfl, hwaddr offset,
>>           boff = boff >> 1;
>>       else if (pfl->width == 4)
>>           boff = boff >> 2;
>> +    else if (pfl->width == 8)
>> +        boff = boff >> 3;
> 
> If you run this patch through scripts/checkpatch.pl it should tell
> you that our coding style would like you to put braces on this if.
> Our usual practice is that if you're touching an existing statement
> that isn't in line with our coding style you should fix at least the
> parts checkpatch complains about (ie add {} to all the arms of the
> if, here).
> 

Will do. Oddly, checkpatch isn't warning me about the brace elision...

>>       switch (pfl->cmd) {
>>       default:
>>           /* This should never happen : reset state & treat it as a read*/
>> @@ -170,37 +233,7 @@ static uint64_t pflash_read(pflash_t *pfl, hwaddr offset,
>>       case 0x00:
>>       flash_read:
>>           /* Flash area read */
>> -        p = pfl->storage;
>> -        switch (width) {
>> -        case 1:
>> -            ret = p[offset];
>> -//            DPRINTF("%s: data offset %08x %02x\n", __func__, offset, ret);
>> -            break;
>> -        case 2:
>> -            if (be) {
>> -                ret = p[offset] << 8;
>> -                ret |= p[offset + 1];
>> -            } else {
>> -                ret = p[offset];
>> -                ret |= p[offset + 1] << 8;
>> -            }
>> -//            DPRINTF("%s: data offset %08x %04x\n", __func__, offset, ret);
>> -            break;
>> -        case 4:
>> -            if (be) {
>> -                ret = p[offset] << 24;
>> -                ret |= p[offset + 1] << 16;
>> -                ret |= p[offset + 2] << 8;
>> -                ret |= p[offset + 3];
>> -            } else {
>> -                ret = p[offset];
>> -                ret |= p[offset + 1] << 8;
>> -                ret |= p[offset + 2] << 16;
>> -                ret |= p[offset + 3] << 24;
>> -            }
>> -//            DPRINTF("%s: data offset %08x %08x\n", __func__, offset, ret);
>> -            break;
>> -        }
>> +        ret = _flash_read(pfl, offset, width, be);
>>           break;
>>       case 0x90:
>>           /* flash ID read */
>> @@ -260,8 +293,8 @@ static void pflash_update(pflash_t *pfl, int offset,
>>       }
>>   }
>>
>> -static void pflash_write (pflash_t *pfl, hwaddr offset,
>> -                          uint64_t value, int width, int be)
>> +static void pflash_write(pflash_t *pfl, hwaddr offset,
>> +                         uint64_t value, int width, int be)
>>   {
>>       hwaddr boff;
>>       uint8_t *p;
>> @@ -275,17 +308,19 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
>>   #endif
>>           goto reset_flash;
>>       }
>> -    DPRINTF("%s: offset " TARGET_FMT_plx " %08x %d %d\n", __func__,
>> -            offset, value, width, pfl->wcycle);
>> +    DPRINTF("%s: offset " TARGET_FMT_plx " %08" PRIx64 " %d %d\n",
>> +            __func__, offset, value, width, pfl->wcycle);
>>       offset &= pfl->chip_len - 1;
>>
>> -    DPRINTF("%s: offset " TARGET_FMT_plx " %08x %d\n", __func__,
>> -            offset, value, width);
>> +    DPRINTF("%s: offset " TARGET_FMT_plx " %08" PRIx64 " %d\n",
>> +            __func__, offset, value, width);
>>       boff = offset & (pfl->sector_len - 1);
>>       if (pfl->width == 2)
>>           boff = boff >> 1;
>>       else if (pfl->width == 4)
>>           boff = boff >> 2;
>> +    else if (pfl->width == 8)
>> +        boff = boff >> 3;
>>       switch (pfl->wcycle) {
>>       case 0:
>>           /* Set the device in I/O access mode if required */
>> @@ -346,8 +381,8 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
>>               /* We need another unlock sequence */
>>               goto check_unlock0;
>>           case 0xA0:
>> -            DPRINTF("%s: write data offset " TARGET_FMT_plx " %08x %d\n",
>> -                    __func__, offset, value, width);
>> +            DPRINTF("%s: write data offset " TARGET_FMT_plx
>> +                    " %08" PRIx64 " %d\n", __func__, offset, value, width);
>>               p = pfl->storage;
>>               if (!pfl->ro) {
>>                   switch (width) {
>> @@ -379,6 +414,28 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
>>                       }
>>                       pflash_update(pfl, offset, 4);
>>                       break;
>> +                case 8:
>> +                    if (be) {
>> +                        p[offset] &= value >> 56;
>> +                        p[offset + 1] &= value >> 48;
>> +                        p[offset + 2] &= value >> 40;
>> +                        p[offset + 3] &= value >> 32;
>> +                        p[offset + 4] &= value >> 24;
>> +                        p[offset + 5] &= value >> 16;
>> +                        p[offset + 6] &= value >> 8;
>> +                        p[offset + 7] &= value;
>> +                    } else {
>> +                        p[offset] &= value;
>> +                        p[offset + 1] &= value >> 8;
>> +                        p[offset + 2] &= value >> 16;
>> +                        p[offset + 3] &= value >> 24;
>> +                        p[offset + 4] &= value >> 32;
>> +                        p[offset + 5] &= value >> 40;
>> +                        p[offset + 6] &= value >> 48;
>> +                        p[offset + 7] &= value >> 56;
>> +                    }
>> +                    pflash_update(pfl, offset, 8);
>> +                    break;
>>                   }
> 
> Similarly here we can use
>     if (be) {
>         uint64_t oldv = ldq_be_p(p + offset);
>         stq_be_p(p + offset, oldv & value);
>     } else {
>         ...ditto with _le_
>     }
> 
> Other than those minor issues, patch looks good.
> 
> thanks
> -- PMM
> 

I'll fix these as well.
Thanks!

[Qemu-devel] [PATCH v3 3/3] Add partial flash interleaving to AMD CFI devices

[Mike Nawrocki]

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

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

Re: [Qemu-devel] [PATCH v3 3/3] Add partial flash interleaving to AMD CFI devices

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH v3 3/3] Add partial flash interleaving to AMD CFI devices

[Michael Nawrocki]

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!