[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 27/27] hw/block/pflash_cfi02: Reduce I/O accesses
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-devel] [PULL 27/27] hw/block/pflash_cfi02: Reduce I/O accesses to 16-bit |
Date: |
Wed, 3 Jul 2019 18:36:55 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 |
On 7/3/19 6:20 PM, Stephen Checkoway wrote:
>> 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.
We can revert it if it helps you.
I plan to merge the rest of your work (interleaving) during the next
release, if tested we can revert it there too.
My goal with the pflash devices is to have a white list of real devices
with proper CFI table, and boards/machines can only use a whitelisted
correctly behaving device.
This way we won't have to deal with broken configurations.
>>>> 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
>
- Re: [Qemu-devel] [PULL 24/27] hw/block/pflash_cfi02: Implement erase suspend/resume, (continued)
- [Qemu-devel] [PULL 19/27] hw/block/pflash_cfi02: Extract pflash_regions_count(), Philippe Mathieu-Daudé, 2019/07/01
- [Qemu-devel] [PULL 13/27] tests/pflash-cfi02: Refactor to support testing multiple configurations, Philippe Mathieu-Daudé, 2019/07/01
- [Qemu-devel] [PULL 09/27] hw/block/pflash_cfi02: Use the ldst API in pflash_read(), Philippe Mathieu-Daudé, 2019/07/01
- [Qemu-devel] [PULL 10/27] hw/block/pflash_cfi02: Extract the pflash_data_read() function, Philippe Mathieu-Daudé, 2019/07/01
- [Qemu-devel] [PULL 27/27] hw/block/pflash_cfi02: Reduce I/O accesses to 16-bit, Philippe Mathieu-Daudé, 2019/07/01
[Qemu-devel] [PULL 21/27] hw/block/pflash_cfi02: Fix CFI in autoselect mode, Philippe Mathieu-Daudé, 2019/07/01
[Qemu-devel] [PULL 16/27] hw/block/pflash_cfi02: Hold the PRI table offset in a variable, Philippe Mathieu-Daudé, 2019/07/01
[Qemu-devel] [PULL 17/27] hw/block/pflash_cfi02: Document 'Page Mode' operations are not supported, Philippe Mathieu-Daudé, 2019/07/01
[Qemu-devel] [PULL 26/27] hw/block/pflash_cfi02: Document commands, Philippe Mathieu-Daudé, 2019/07/01
[Qemu-devel] [PULL 12/27] hw/block/pflash_cfi02: Fix command address comparison, Philippe Mathieu-Daudé, 2019/07/01
Re: [Qemu-devel] [PULL 00/27] pflash-next patches, Peter Maydell, 2019/07/02