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
<address@hidden> wrote:
> On 22 October 2013 17:35, Roy Franz <address@hidden> 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 <address@hidden>
>> ---
>>  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