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

[Qemu-devel] [PATCH 0/4 v4] 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 the interface width, 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.

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 (4):
  rename pflash_t member width to bank_width
  Add device-width property to pflash_cfi01
  return status for each device
  Set proper device-width for vexpress flash

 hw/arm/vexpress.c       |   43 +++++++++++++++++++++++++++++++++----------
 hw/block/pflash_cfi01.c |   37 +++++++++++++++++++++++++++----------
 2 files changed, 60 insertions(+), 20 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/4 v4] block: rename pflash_t member width to bank_width

[Roy Franz]

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

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

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 018a967..d5e366d 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,9 +126,9 @@ 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
@@ -665,7 +665,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 +706,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 +745,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 +756,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

Re: [Qemu-devel] [PATCH 1/4 v4] block: rename pflash_t member width to bank_width

[Peter Maydell]

On 22 October 2013 17:35, Roy Franz <roy.franz@linaro.org> wrote:
> Rename the 'width' member of the pflash_t structuer
> 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>

-- PMM

[Qemu-devel] [PATCH 2/4 v5] block: 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 |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index d5e366d..cda8289 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -72,6 +72,7 @@ struct pflash_t {
     uint32_t nb_blocs;
     uint64_t sector_len;
     uint8_t bank_width;
+    uint8_t device_width;
     uint8_t be;
     uint8_t wcycle; /* if 0, the flash is read normally */
     int ro;
@@ -378,6 +379,8 @@ static void pflash_write(pflash_t *pfl, hwaddr offset,
 
             break;
         case 0xe8:
+            /* Mask writeblock size based on device width */
+            value &= (1ULL << (pfl->device_width * 8)) - 1;
             DPRINTF("%s: block write of %x bytes\n", __func__, value);
             pfl->counter = value;
             pfl->wcycle++;
@@ -707,6 +710,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),
@@ -757,6 +761,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", bank_width);
+    qdev_prop_set_uint8(dev, "device-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

Re: [Qemu-devel] [PATCH 2/4 v5] block: Add device-width property to pflash_cfi01

[Peter Maydell]

On 22 October 2013 17: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>
> ---
>  hw/block/pflash_cfi01.c |    5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index d5e366d..cda8289 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -72,6 +72,7 @@ struct pflash_t {
>      uint32_t nb_blocs;
>      uint64_t sector_len;
>      uint8_t bank_width;
> +    uint8_t device_width;
>      uint8_t be;
>      uint8_t wcycle; /* if 0, the flash is read normally */
>      int ro;
> @@ -378,6 +379,8 @@ static void pflash_write(pflash_t *pfl, hwaddr offset,
>
>              break;
>          case 0xe8:
> +            /* Mask writeblock size based on device width */
> +            value &= (1ULL << (pfl->device_width * 8)) - 1;

  value = extract32(value, 0, pfl->device_width * 8);

(you'll need to #include "qemu/bitops.h".)

Other than this and the status (which you deal with in
the other patch) the accesses I wonder about whether we
have correct are:
 * manufacturer & device ID code read
 * cfi_table[] accesses ("query mode")

http://www.delorie.com/agenda/specs/29220404.pdf
table 1 only talks about using 2 8x devices for a 16
bit bus, but it definitely seems to imply that the QRY
reads from the cfi_table[] behave differently for
paired devices than for a single full width one
(and that's logically what you'd expect since a
single device will just return the one character but
a paired device will return that one character
mirrored up into each of the byte lanes).

Basically these two things, like status read, are
returning fixed-values which will be output by both
the parallel devices; it's only pure data accesses
that behave the same way regardless.

>              DPRINTF("%s: block write of %x bytes\n", __func__, value);
>              pfl->counter = value;
>              pfl->wcycle++;
> @@ -707,6 +710,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),
> @@ -757,6 +761,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", bank_width);
> +    qdev_prop_set_uint8(dev, "device-width", bank_width);

This doesn't look right. We should just leave the property
at its default value. (In pflash_cfi01_realize you can catch
the 'device_width == 0' case and set it to bank_width there.)

>      qdev_prop_set_uint8(dev, "big-endian", !!be);
>      qdev_prop_set_uint16(dev, "id0", id0);
>      qdev_prop_set_uint16(dev, "id1", id1);

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/4 v5] block: Add device-width property to pflash_cfi01

[Roy Franz]

On Thu, Nov 28, 2013 at 11:03 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 22 October 2013 17: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>
>> ---
>>  hw/block/pflash_cfi01.c |    5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index d5e366d..cda8289 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -72,6 +72,7 @@ struct pflash_t {
>>      uint32_t nb_blocs;
>>      uint64_t sector_len;
>>      uint8_t bank_width;
>> +    uint8_t device_width;
>>      uint8_t be;
>>      uint8_t wcycle; /* if 0, the flash is read normally */
>>      int ro;
>> @@ -378,6 +379,8 @@ static void pflash_write(pflash_t *pfl, hwaddr offset,
>>
>>              break;
>>          case 0xe8:
>> +            /* Mask writeblock size based on device width */
>> +            value &= (1ULL << (pfl->device_width * 8)) - 1;
>
>   value = extract32(value, 0, pfl->device_width * 8);
>
> (you'll need to #include "qemu/bitops.h".)
>
> Other than this and the status (which you deal with in
> the other patch) the accesses I wonder about whether we
> have correct are:
>  * manufacturer & device ID code read
>  * cfi_table[] accesses ("query mode")

I'm pretty sure these are not correct when device width
is not equal to bank width.  The manufacturer and device ID code
looks completely broken, doing:

ret = pfl->ident0 << 8 | pfl->ident1;

with ident0 and ident1 being 16 bit values.

I can update the  device ID and cfi_table accesses to take into account
device width, and test that with UEFI, but my concern is that this code is
tricky to get right, and won't be well tested.  The UEFI code doesn't
care that these
values are wrong, so my test case will likely continue to work whether the
updates I make are correct or not.  Also, all other platforms will
continue to see
the current behavior, as they don't set the device width, so they
won't be testing
the new code either.

Let me know how you would like me to proceed on this.  This is the last issue
for me to resolve and I will have another version of the patch series ready.

Thanks,
Roy




>
> http://www.delorie.com/agenda/specs/29220404.pdf
> table 1 only talks about using 2 8x devices for a 16
> bit bus, but it definitely seems to imply that the QRY
> reads from the cfi_table[] behave differently for
> paired devices than for a single full width one
> (and that's logically what you'd expect since a
> single device will just return the one character but
> a paired device will return that one character
> mirrored up into each of the byte lanes).
>
> Basically these two things, like status read, are
> returning fixed-values which will be output by both
> the parallel devices; it's only pure data accesses
> that behave the same way regardless.
>
>>              DPRINTF("%s: block write of %x bytes\n", __func__, value);
>>              pfl->counter = value;
>>              pfl->wcycle++;
>> @@ -707,6 +710,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),
>> @@ -757,6 +761,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", bank_width);
>> +    qdev_prop_set_uint8(dev, "device-width", bank_width);
>
> This doesn't look right. We should just leave the property
> at its default value. (In pflash_cfi01_realize you can catch
> the 'device_width == 0' case and set it to bank_width there.)
>
>>      qdev_prop_set_uint8(dev, "big-endian", !!be);
>>      qdev_prop_set_uint16(dev, "id0", id0);
>>      qdev_prop_set_uint16(dev, "id1", id1);
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 2/4 v5] block: Add device-width property to pflash_cfi01

[Peter Maydell]

On 3 December 2013 20:12, Roy Franz <roy.franz@linaro.org> wrote:
> On Thu, Nov 28, 2013 at 11:03 AM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> Other than this and the status (which you deal with in
>> the other patch) the accesses I wonder about whether we
>> have correct are:
>>  * manufacturer & device ID code read
>>  * cfi_table[] accesses ("query mode")
>
> I'm pretty sure these are not correct when device width
> is not equal to bank width.  The manufacturer and device ID code
> looks completely broken, doing:
>
> ret = pfl->ident0 << 8 | pfl->ident1;
>
> with ident0 and ident1 being 16 bit values.
>
> I can update the  device ID and cfi_table accesses to take into account
> device width, and test that with UEFI, but my concern is that this code is
> tricky to get right, and won't be well tested.  The UEFI code doesn't
> care that these
> values are wrong, so my test case will likely continue to work whether the
> updates I make are correct or not.  Also, all other platforms will
> continue to see
> the current behavior, as they don't set the device width, so they
> won't be testing
> the new code either.
>
> Let me know how you would like me to proceed on this.  This is the last issue
> for me to resolve and I will have another version of the patch series ready.

I'd prefer us to have some untested code which we believe to
be correct, rather than the current approach, which is to have
untested code which we know to be wrong :-)

Cross-checking against what the real hardware reads these
ident/ID accesses as would be nice, if you have the setup to
hand (ie stuff some temporary test code into a uefi build or
something). At least then we can be reasonably sure the 16:16 case
is correct.

-- PMM

Re: [Qemu-devel] [PATCH 2/4 v5] block: Add device-width property to pflash_cfi01

[Roy Franz]

On Tue, Dec 3, 2013 at 12:27 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 December 2013 20:12, Roy Franz <roy.franz@linaro.org> wrote:
>> On Thu, Nov 28, 2013 at 11:03 AM, Peter Maydell
>> <peter.maydell@linaro.org> wrote:
>>> Other than this and the status (which you deal with in
>>> the other patch) the accesses I wonder about whether we
>>> have correct are:
>>>  * manufacturer & device ID code read
>>>  * cfi_table[] accesses ("query mode")
>>
>> I'm pretty sure these are not correct when device width
>> is not equal to bank width.  The manufacturer and device ID code
>> looks completely broken, doing:
>>
>> ret = pfl->ident0 << 8 | pfl->ident1;
>>
>> with ident0 and ident1 being 16 bit values.
>>
>> I can update the  device ID and cfi_table accesses to take into account
>> device width, and test that with UEFI, but my concern is that this code is
>> tricky to get right, and won't be well tested.  The UEFI code doesn't
>> care that these
>> values are wrong, so my test case will likely continue to work whether the
>> updates I make are correct or not.  Also, all other platforms will
>> continue to see
>> the current behavior, as they don't set the device width, so they
>> won't be testing
>> the new code either.
>>
>> Let me know how you would like me to proceed on this.  This is the last issue
>> for me to resolve and I will have another version of the patch series ready.
>
> I'd prefer us to have some untested code which we believe to
> be correct, rather than the current approach, which is to have
> untested code which we know to be wrong :-)
>
> Cross-checking against what the real hardware reads these
> ident/ID accesses as would be nice, if you have the setup to
> hand (ie stuff some temporary test code into a uefi build or
> something). At least then we can be reasonably sure the 16:16 case
> is correct.
>
> -- PMM

OK,  I'll take a stab at this.  I don't have VExpress hardware, but
should be able to get someone
to run some test code on it to check how actual hardware behaves.

Roy

[Qemu-devel] [PATCH 3/4 v4] block: return status for each 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 |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index cda8289..aa2cbbc 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -191,9 +191,21 @@ 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 (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 (width > 2) {
+            /* Handle 32 bit flash cases where device width may be
+             * improperly set. (Existing behavior before device width
+             * added.)
+             */
             ret |= pfl->status << 16;
         }
         DPRINTF("%s: status %x\n", __func__, ret);
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 3/4 v4] block: return status for each device

[Peter Maydell]

On 22 October 2013 17: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>
> ---
>  hw/block/pflash_cfi01.c |   16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index cda8289..aa2cbbc 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -191,9 +191,21 @@ 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 (width > pfl->device_width) {
> +            int shift = pfl->device_width * 8;
> +            while (shift + pfl->device_width * 8 <= width * 8) {
> +                ret |= pfl->status << (shift);

The brackets here are superfluous.

> +                shift += pfl->device_width * 8;
> +            }

If we end up with several things that all want to get
this "replicate into all lanes" behaviour then a helper
function is probably going to be worthwhile (see comments
on other patch).

> +        } else if (width > 2) {
> +            /* Handle 32 bit flash cases where device width may be
> +             * improperly set. (Existing behavior before device width
> +             * added.)
> +             */
>              ret |= pfl->status << 16;

Maybe we should have 'device_width == 0' mean 'same as
bank width and with all the legacy workaround code', so
device_width can be specifically set same as bank_width
for new platforms to get the actual correct behaviour for
a full 32 bit wide flash device.

I don't care very much though: up to you.

>          }
>          DPRINTF("%s: status %x\n", __func__, ret);
> --
> 1.7.10.4
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/4 v4] block: return status for each device

[Roy Franz]

On Thu, Nov 28, 2013 at 11:10 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 22 October 2013 17: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>
>> ---
>>  hw/block/pflash_cfi01.c |   16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index cda8289..aa2cbbc 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -191,9 +191,21 @@ 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 (width > pfl->device_width) {
>> +            int shift = pfl->device_width * 8;
>> +            while (shift + pfl->device_width * 8 <= width * 8) {
>> +                ret |= pfl->status << (shift);
>
> The brackets here are superfluous.
fixed.
>
>> +                shift += pfl->device_width * 8;
>> +            }
>
> If we end up with several things that all want to get
> this "replicate into all lanes" behaviour then a helper
> function is probably going to be worthwhile (see comments
> on other patch).
>
>> +        } else if (width > 2) {
>> +            /* Handle 32 bit flash cases where device width may be
>> +             * improperly set. (Existing behavior before device width
>> +             * added.)
>> +             */
>>              ret |= pfl->status << 16;
>
> Maybe we should have 'device_width == 0' mean 'same as
> bank width and with all the legacy workaround code', so
> device_width can be specifically set same as bank_width
> for new platforms to get the actual correct behaviour for
> a full 32 bit wide flash device.
>
> I don't care very much though: up to you.

I changed it as you describe.  Even though I have never seen a 32 bit
wide NOR device in the wild,
it would be good to allow proper support for them.

>
>>          }
>>          DPRINTF("%s: status %x\n", __func__, ret);
>> --
>> 1.7.10.4
>>
>
> thanks
> -- PMM

[Qemu-devel] [PATCH 4/4 v4] block: 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 |   43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index f48de00..8eae73c 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -480,6 +480,35 @@ 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,
+                                          BlockDriverState *bs)
+{
+    DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
+
+    if (bs && qdev_prop_set_drive(dev, "drive", bs)) {
+        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 +590,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 ? dinfo->bdrv : NULL);
     if (!pflash0) {
         fprintf(stderr, "vexpress: error registering flash 0.\n");
         exit(1);
@@ -580,11 +606,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 ? dinfo->bdrv : NULL)) {
         fprintf(stderr, "vexpress: error registering flash 1.\n");
         exit(1);
     }
-- 
1.7.10.4

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

[Peter Maydell]

On 22 October 2013 17: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>
> ---
>  hw/arm/vexpress.c |   43 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index f48de00..8eae73c 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -480,6 +480,35 @@ 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,
> +                                          BlockDriverState *bs)
> +{
> +    DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
> +
> +    if (bs && qdev_prop_set_drive(dev, "drive", bs)) {
> +        abort();
> +    }
> +
> +    qdev_prop_set_uint32(dev, "num-blocks", VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE);

This line is too long and needs a linebreak.
(scripts/checkpatch.pl catches this kind of thing for you.)

> +    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 +590,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 ? dinfo->bdrv : NULL);

You might as well fold the "is this NULL?" check into
the helper and just pass in dinfo.

>      if (!pflash0) {
>          fprintf(stderr, "vexpress: error registering flash 0.\n");
>          exit(1);
> @@ -580,11 +606,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 ? dinfo->bdrv : NULL)) {
>          fprintf(stderr, "vexpress: error registering flash 1.\n");
>          exit(1);
>      }
> --
> 1.7.10.4

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/4 v4] block, arm: Fix buffered flash writes on VExpress

[Roy Franz]

Peter - is this series acceptable?  I have reviewed the patches based
on our conversation at Connect, and patch 3 already handles returning
the status
based on device width as we discussed.  If device and bank width are
different, it repeats the status based on device width.  Otherwise, if
both widths are > 2, it repeats the status assuming 16 bit device
width, which preserves the current behavior for 32 bit wide banks.
The existing behavior will be preserved for all platforms that don't
set different device and bank widths.


Thanks,
Roy




On Tue, Oct 22, 2013 at 9:35 AM, Roy Franz <roy.franz@linaro.org> wrote:
> 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 the interface width, 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.
>
> 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 (4):
>   rename pflash_t member width to bank_width
>   Add device-width property to pflash_cfi01
>   return status for each device
>   Set proper device-width for vexpress flash
>
>  hw/arm/vexpress.c       |   43 +++++++++++++++++++++++++++++++++----------
>  hw/block/pflash_cfi01.c |   37 +++++++++++++++++++++++++++----------
>  2 files changed, 60 insertions(+), 20 deletions(-)
>
> --
> 1.7.10.4
>

Re: [Qemu-devel] [PATCH 0/4 v4] block, arm: Fix buffered flash writes on VExpress

[Stefan Hajnoczi]

On Tue, Oct 22, 2013 at 09:35:52AM -0700, Roy Franz wrote:
> 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 the interface width, 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.
> 
> 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 (4):
>   rename pflash_t member width to bank_width
>   Add device-width property to pflash_cfi01
>   return status for each device
>   Set proper device-width for vexpress flash
> 
>  hw/arm/vexpress.c       |   43 +++++++++++++++++++++++++++++++++----------
>  hw/block/pflash_cfi01.c |   37 +++++++++++++++++++++++++++----------
>  2 files changed, 60 insertions(+), 20 deletions(-)

Looks okay on quick review.

I want to confirm: live migration is not supported?

Re: [Qemu-devel] [PATCH 0/4 v4] block, arm: Fix buffered flash writes on VExpress

[Peter Maydell]

On 20 November 2013 09:22, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Oct 22, 2013 at 09:35:52AM -0700, Roy Franz wrote:
>> 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 the interface width, 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.

> Looks okay on quick review.
>
> I want to confirm: live migration is not supported?

I don't know if you meant anything special by putting "live"
in there, but migration and save/restore is absolutely supposed
to be supported on the vexpress platform and the pflash
devices (they're in the x86 PC model too...)

-- PMM

Re: [Qemu-devel] [PATCH 0/4 v4] block, arm: Fix buffered flash writes on VExpress

[Stefan Hajnoczi]

On Wed, Nov 20, 2013 at 09:27:22AM +0000, Peter Maydell wrote:
> On 20 November 2013 09:22, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Tue, Oct 22, 2013 at 09:35:52AM -0700, Roy Franz wrote:
> >> 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 the interface width, 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.
> 
> > Looks okay on quick review.
> >
> > I want to confirm: live migration is not supported?
> 
> I don't know if you meant anything special by putting "live"
> in there, but migration and save/restore is absolutely supposed
> to be supported on the vexpress platform and the pflash
> devices (they're in the x86 PC model too...)

I looked at the patches again and there is no problem.

Originally I thought the device_width field would need to be migrated
but it's purely configuration state - the destination QEMU will receive
it on the command-line.  Therefore we don't need to transfer it during
live migration.

Stefan