Re: [Qemu-block] [PULL 27/27] hw/block/pflash_cfi02: Reduce I/O accesses to 16-bit

[Stephen Checkoway]

> On Jul 3, 2019, at 12:02, Philippe Mathieu-Daudé <address@hidden> wrote:
> 
> On 7/3/19 5:52 PM, Stephen Checkoway wrote:
>> 
>> 
>>> On Jul 1, 2019, at 20:59, Philippe Mathieu-Daudé <address@hidden> wrote:
>>> 
>>> Parallel NOR flashes are limited to 16-bit bus accesses.
>> 
>> I don't think this is correct. The CFI spec defines an x32 interface for 
>> parallel NOR. CFI addresses 0x28 and 0x29 specify the interface and value 3 
>> is x32 and value 5 is x16/x32.
>> 
>> Here's an example of an x32 device 
>> <https://www.mouser.com/datasheet/2/100/002-00948_29CD032J_S29CD016J_S29CL032J_S29CL016J_3-1316792.pdf>.
> 
> OK, I was not aware of these.
> 
> QEMU never CFI-announced itself as x32 capable:
> 
>    /* Flash device interface (8 & 16 bits) */
>    pfl->cfi_table[0x28] = 0x02;
>    pfl->cfi_table[0x29] = 0x00;
> 
> So while the commit description is incorrect, the code is safe with the
> current device model.
> 
> I am not comfortable keeping untested 32-bit mode.
> Were you using it?

I'm not using it. I did have some code to set these CFI values based on the 
parameters used to control the interleaving 
<https://github.com/stevecheckoway/qemu/commit/f9a79a6e18b2c7c5a05e344ff554a7d980a56042#diff-d33881bd0ef099e2f46ebd4797c653bcR599>.

In general, a better testing harness would be nice though.

Steve

> 
> Regards,
> 
> Phil.
> 
>> Cheers,
>> 
>> Steve
>> 
>>> Remove the 32-bit dead code.
>>> 
>>> Reviewed-by: Alistair Francis <address@hidden>
>>> Message-Id: <address@hidden>
>>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>>> ---
>>> hw/block/pflash_cfi02.c | 5 +----
>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>> 
>>> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>>> index 83084b9d72..5392290c72 100644
>>> --- a/hw/block/pflash_cfi02.c
>>> +++ b/hw/block/pflash_cfi02.c
>>> @@ -317,8 +317,6 @@ static uint64_t pflash_read(void *opaque, hwaddr 
>>> offset, unsigned int width)
>>>    boff = offset & 0xFF;
>>>    if (pfl->width == 2) {
>>>        boff = boff >> 1;
>>> -    } else if (pfl->width == 4) {
>>> -        boff = boff >> 2;
>>>    }
>>>    switch (pfl->cmd) {
>>>    default:
>>> @@ -449,8 +447,6 @@ static void pflash_write(void *opaque, hwaddr offset, 
>>> uint64_t value,
>>>    boff = offset;
>>>    if (pfl->width == 2) {
>>>        boff = boff >> 1;
>>> -    } else if (pfl->width == 4) {
>>> -        boff = boff >> 2;
>>>    }
>>>    /* Only the least-significant 11 bits are used in most cases. */
>>>    boff &= 0x7FF;
>>> @@ -710,6 +706,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
>>> uint64_t value,
>>> static const MemoryRegionOps pflash_cfi02_ops = {
>>>    .read = pflash_read,
>>>    .write = pflash_write,
>>> +    .impl.max_access_size = 2,
>>>    .valid.min_access_size = 1,
>>>    .valid.max_access_size = 4,
>>>    .endianness = DEVICE_NATIVE_ENDIAN,
>>> -- 
>>> 2.20.1

-- 
Stephen Checkoway