[Qemu-devel] [PATCH] block: mask NOR flash buffered write length

[Qemu-devel] [PATCH] block: mask NOR flash buffered write length

[Roy Franz]

For buffered writes, mask the length with the maximum supported
length.  This is required for block writes to work on the ARM vexpress
platform, where the flash interface is 32 bits wide.  For buffered writes
to the 2 16 bit flashes on the interface, the length is repeated in each
16 bit word, and without this mask the two lengths are interpreted 
as a single 32 bit value that is very large.

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

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 018a967..a364cca 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -378,6 +378,7 @@ static void pflash_write(pflash_t *pfl, hwaddr offset,
 
             break;
         case 0xe8:
+            value &= pfl->writeblock_size - 1;
             DPRINTF("%s: block write of %x bytes\n", __func__, value);
             pfl->counter = value;
             pfl->wcycle++;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] block: mask NOR flash buffered write length

[Stefan Hajnoczi]

On Thu, Oct 17, 2013 at 07:30:02PM -0700, Roy Franz wrote:
> For buffered writes, mask the length with the maximum supported
> length.  This is required for block writes to work on the ARM vexpress
> platform, where the flash interface is 32 bits wide.  For buffered writes
> to the 2 16 bit flashes on the interface, the length is repeated in each
> 16 bit word, and without this mask the two lengths are interpreted 
> as a single 32 bit value that is very large.
> 
> Signed-off-by: Roy Franz <roy.franz@linaro.org>
> ---
>  hw/block/pflash_cfi01.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 018a967..a364cca 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -378,6 +378,7 @@ static void pflash_write(pflash_t *pfl, hwaddr offset,
>  
>              break;
>          case 0xe8:
> +            value &= pfl->writeblock_size - 1;

This patch feels weird.   Should the 32-bit interface width be truncated
down to 16 bits before dispatching pflash_write()?

It's not clear to me that truncating just in this specific case is
correct.  But then I don't know the hardware :).

Stefan

Re: [Qemu-devel] [PATCH] block: mask NOR flash buffered write length

[Peter Maydell]

On 18 October 2013 12:38, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Oct 17, 2013 at 07:30:02PM -0700, Roy Franz wrote:
>> For buffered writes, mask the length with the maximum supported
>> length.  This is required for block writes to work on the ARM vexpress
>> platform, where the flash interface is 32 bits wide.  For buffered writes
>> to the 2 16 bit flashes on the interface, the length is repeated in each
>> 16 bit word, and without this mask the two lengths are interpreted
>> as a single 32 bit value that is very large.

>> @@ -378,6 +378,7 @@ static void pflash_write(pflash_t *pfl, hwaddr offset,
>>
>>              break;
>>          case 0xe8:
>> +            value &= pfl->writeblock_size - 1;
>
> This patch feels weird.   Should the 32-bit interface width be truncated
> down to 16 bits before dispatching pflash_write()?
>
> It's not clear to me that truncating just in this specific case is
> correct.  But then I don't know the hardware :).

I think the problem may be that we're incorrectly modelling the
hardware's "2 side-by-side 16 bit wide flash chips" as "one 32 bit
wide flash chip", because our flash device code doesn't support
doing the former.

Probably instead of a single "width" property we should have two,
similar to the device tree binding's pair:
 - bank-width : Width (in bytes) of the bank.  Equal to the
   device width times the number of interleaved chips.
 - device-width : (optional) Width of a single mtd chip.  If
   omitted, assumed to be equal to 'bank-width'.

However I'm not very familiar with how flash hardware works...

-- PMM

Re: [Qemu-devel] [PATCH] block: mask NOR flash buffered write length

[Roy Franz]

On Fri, Oct 18, 2013 at 6:36 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 October 2013 12:38, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Thu, Oct 17, 2013 at 07:30:02PM -0700, Roy Franz wrote:
>>> For buffered writes, mask the length with the maximum supported
>>> length.  This is required for block writes to work on the ARM vexpress
>>> platform, where the flash interface is 32 bits wide.  For buffered writes
>>> to the 2 16 bit flashes on the interface, the length is repeated in each
>>> 16 bit word, and without this mask the two lengths are interpreted
>>> as a single 32 bit value that is very large.
>
>>> @@ -378,6 +378,7 @@ static void pflash_write(pflash_t *pfl, hwaddr offset,
>>>
>>>              break;
>>>          case 0xe8:
>>> +            value &= pfl->writeblock_size - 1;
>>
>> This patch feels weird.   Should the 32-bit interface width be truncated
>> down to 16 bits before dispatching pflash_write()?
>>
>> It's not clear to me that truncating just in this specific case is
>> correct.  But then I don't know the hardware :).
>
> I think the problem may be that we're incorrectly modelling the
> hardware's "2 side-by-side 16 bit wide flash chips" as "one 32 bit
> wide flash chip", because our flash device code doesn't support
> doing the former.
>
> Probably instead of a single "width" property we should have two,
> similar to the device tree binding's pair:
>  - bank-width : Width (in bytes) of the bank.  Equal to the
>    device width times the number of interleaved chips.
>  - device-width : (optional) Width of a single mtd chip.  If
>    omitted, assumed to be equal to 'bank-width'.
>
> However I'm not very familiar with how flash hardware works...
>
> -- PMM

Hi Peter,

    You are correct - we really do want to mask based on the device
width, as that is what the
actual flash chips will see.  Lacking the device width I used the
writeblock size.  Thinking about this more,
this will not work for 8 bit devices used together, as the mask size
will be greater than 8 bits and the writeblock size
will be mis-interpreted like it is now.
I'll work on adding a device-size property to the pflash*
implementations.  It looks like this will affect about 20 platforms.
For the platforms that I am not familiar with I plan just set
bank-width==device-width as that should result in the unchanged
behavior.

Thanks,
Roy

Re: [Qemu-devel] [PATCH] block: mask NOR flash buffered write length

[Peter Maydell]

On 18 October 2013 14:54, Roy Franz <roy.franz@linaro.org> wrote:
> On Fri, Oct 18, 2013 at 6:36 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Probably instead of a single "width" property we should have two,
>> similar to the device tree binding's pair:
>>  - bank-width : Width (in bytes) of the bank.  Equal to the
>>    device width times the number of interleaved chips.
>>  - device-width : (optional) Width of a single mtd chip.  If
>>    omitted, assumed to be equal to 'bank-width'.

>> However I'm not very familiar with how flash hardware works...
>     You are correct - we really do want to mask based on the device
> width, as that is what the
> actual flash chips will see.  Lacking the device width I used the
> writeblock size.  Thinking about this more,
> this will not work for 8 bit devices used together, as the mask size
> will be greater than 8 bits and the writeblock size
> will be mis-interpreted like it is now.
> I'll work on adding a device-size property to the pflash*
> implementations.  It looks like this will affect about 20 platforms.
> For the platforms that I am not familiar with I plan just set
> bank-width==device-width as that should result in the unchanged
> behavior.

Yes, you should make the default for the device-width property
be to be the same as the bank-width, since that's what we
currently implement; then we can just change the platforms
where we know that's wrong.

NB: probably best to leave the existing 'width' property with
the name it has, rather than renaming it to 'bank-width'.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] block: mask NOR flash buffered write length

[Roy Franz]

On Fri, Oct 18, 2013 at 7:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 October 2013 14:54, Roy Franz <roy.franz@linaro.org> wrote:
>> On Fri, Oct 18, 2013 at 6:36 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Probably instead of a single "width" property we should have two,
>>> similar to the device tree binding's pair:
>>>  - bank-width : Width (in bytes) of the bank.  Equal to the
>>>    device width times the number of interleaved chips.
>>>  - device-width : (optional) Width of a single mtd chip.  If
>>>    omitted, assumed to be equal to 'bank-width'.
>
>>> However I'm not very familiar with how flash hardware works...
>>     You are correct - we really do want to mask based on the device
>> width, as that is what the
>> actual flash chips will see.  Lacking the device width I used the
>> writeblock size.  Thinking about this more,
>> this will not work for 8 bit devices used together, as the mask size
>> will be greater than 8 bits and the writeblock size
>> will be mis-interpreted like it is now.
>> I'll work on adding a device-size property to the pflash*
>> implementations.  It looks like this will affect about 20 platforms.
>> For the platforms that I am not familiar with I plan just set
>> bank-width==device-width as that should result in the unchanged
>> behavior.
>
> Yes, you should make the default for the device-width property
> be to be the same as the bank-width, since that's what we
> currently implement; then we can just change the platforms
> where we know that's wrong.
>
> NB: probably best to leave the existing 'width' property with
> the name it has, rather than renaming it to 'bank-width'.
>
> thanks
> -- PMM

Thanks Peter.  I'm not familiar with the "properties" and how they are
used.  I think that
the device width is likely only of interest internally, so I won't add
a device-width property.

Roy

Re: [Qemu-devel] [PATCH] block: mask NOR flash buffered write length

[Peter Maydell]

On 18 October 2013 15:05, Roy Franz <roy.franz@linaro.org> wrote:
> On Fri, Oct 18, 2013 at 7:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Yes, you should make the default for the device-width property
>> be to be the same as the bank-width, since that's what we
>> currently implement; then we can just change the platforms
>> where we know that's wrong.
>>
>> NB: probably best to leave the existing 'width' property with
>> the name it has, rather than renaming it to 'bank-width'.

> Thanks Peter.  I'm not familiar with the "properties" and how they are
> used.  I think that
> the device width is likely only of interest internally, so I won't add
> a device-width property.

No, you need to add a property. Properties are how all configurable
device parameters are set: if you look at the implementation
of the pflash_cfi01_register() function you'll see that it's
just a convenience wrapper that creates the device and sets a
lot of properties on it. device-width should be one of those,
in the same way as the existing width property.

In fact I would suggest that you don't change the utility
pflash_cfi01_register() function to add a new parameter to
it (that would involve editing every caller). Instead just
opencode the "create / set properties / init / map" sequence
in vexpress.c. I think that will actually be easier to read
since you don't have to match up each unnamed argument in
a long function call with what it actually does.

-- PMM

Re: [Qemu-devel] [PATCH] block: mask NOR flash buffered write length

[Roy Franz]

On Fri, Oct 18, 2013 at 7:11 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 October 2013 15:05, Roy Franz <roy.franz@linaro.org> wrote:
>> On Fri, Oct 18, 2013 at 7:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Yes, you should make the default for the device-width property
>>> be to be the same as the bank-width, since that's what we
>>> currently implement; then we can just change the platforms
>>> where we know that's wrong.
>>>
>>> NB: probably best to leave the existing 'width' property with
>>> the name it has, rather than renaming it to 'bank-width'.
>
>> Thanks Peter.  I'm not familiar with the "properties" and how they are
>> used.  I think that
>> the device width is likely only of interest internally, so I won't add
>> a device-width property.
>
> No, you need to add a property. Properties are how all configurable
> device parameters are set: if you look at the implementation
> of the pflash_cfi01_register() function you'll see that it's
> just a convenience wrapper that creates the device and sets a
> lot of properties on it. device-width should be one of those,
> in the same way as the existing width property.
>
> In fact I would suggest that you don't change the utility
> pflash_cfi01_register() function to add a new parameter to
> it (that would involve editing every caller). Instead just
> opencode the "create / set properties / init / map" sequence
> in vexpress.c. I think that will actually be easier to read
> since you don't have to match up each unnamed argument in
> a long function call with what it actually does.
>
> -- PMM

Yup, just noticed that myself :)  Avoiding updating every caller for this change
will be good.

Thanks,
Roy