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

[Roy Franz]

On Fri, Oct 18, 2013 at 6:36 AM, Peter Maydell <address@hidden> wrote:
> On 18 October 2013 12:38, Stefan Hajnoczi <address@hidden> 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