Re: [Qemu-block] [Qemu-devel] [PATCH 00/10] block/pflash_cfi02: Implement missing AMD pflash functionality

[Philippe Mathieu-Daudé]

On 4/9/19 5:55 PM, Stephen Checkoway wrote:
> Hi Phil,
> 
>> On Apr 9, 2019, at 06:34, Philippe Mathieu-Daudé <address@hidden> wrote:
>>
>> Hi Stephen,
>>
>> [Cc'ing Markus and Laszlo, we have similar interest in pflash01 testing]
>>
>> On 4/8/19 10:55 PM, Stephen Checkoway wrote:
>>> The goal of this patch series implement the following AMD command-set 
>>> parallel
>>> flash functionality:
>>> - flash interleaving;
>>> - nonuniform sector sizes;
>>> - erase suspend/resume commands; and
>>> - multi-sector erase.
>>
>> I am very glad you addressed these long overdue issues and very pleased
>> by your patches :)
>> I'll thoroughly review it during next week (this won't get merge for the
>> current 4.0 cycle anyway).
> 
> Great!
> 
>> I started a similar cleanup (mostly pflash01 focused) and converted
>> DPRINTF to trace events, added few constants. I'll see if I can rebase
>> your work on top of mine. So far only your patch 2 (refactor) would be
>> modified, simplified as the "pull out all of the code to modify the
>> status into simple helper functions" part (which else I'd ask you to
>> move in a separate patch).
>>
>> We should think of more intereleaved tests, I'll prepare a table of
>> current QEMU models using this device and how is their intereleave
>> mapping. Hopefully it would be enough to simply add the existing
>> machines to your current musicpal qtest.
>>
>> Also I'd like to see some Top/Bottom block configuration qtests, your
>> patch #5 doesn't seem tested.
> 
> I included four tests 
> <https://patchew.org/QEMU/address@hidden/address@hidden/>.
> 
> Patchew makes it hard to link to particular lines. Here's the same patch on 
> Github 
> <https://github.com/qemu/qemu/commit/a851d21347121a91ba9a39c133a8fbee6e84e557#diff-d2ed797ca8898de80768bdfb390781dfR498>.
> 
> Are those sufficient or would you like more tests?

Ah you don't mention the tests in the patch description, that's why I
missed them :) Sometimes I prefer to keep the test addition in a
separate patch, it eases rebases, however in your series it seems fine.

Since you did changes in the CFI table, I think we should add a tests
verifying the table is correctly generated for all you FlashConfig entries.

>>> During refactoring and implementation, I discovered several bugs that are
>>> fixed here as well:
>>> - flash commands use only 11-bits of the address in most cases, but the
>>>  current code uses all of them [1];
>>> - entering CFI mode from autoselect mode and then exiting CFI mode should
>>>  return the chip to autoselect mode, but the current code returns to read
>>>  array mode; and
>>> - reset command should be ignored during sector/chip erase, but the current
>>>  code performs the reset.
>>>
>>> The first patch in the series adds a test for the existing behavior. Tests 
>>> for
>>> additional behavior/bug fixes are added in the relevant patch.
>>>
>>> 1. I found firmware in the wild that relies on the 11-bit address behavior,
>>>   probably due to a bug in the firmware itself.
>>
>> Is it a musicpal firmware? Are you able to compare with real hardware?
> 
> No, it's not musicpal. I'm not even sure what musicpal is, it was just the 
> most convenient choice for testing.  The real hardware is an embedded system 
> using an old AMD processor (Am486) with three different AMD command set flash 
> chips. The unlock addresses the firmware uses don't actually match the part 
> sheets (in most cases). It took quite a while to realize that the flash 
> hardware only uses 11 bits of address. (It's clearly spelled out in the 
> various part sheets, but I guess I missed it on the first 15 or so readings.)

I suppose you can not share the firmware binary. Can you share these
unlock addresses? Maybe once I reviewed carefully your series I might
ask you the (pflash) trace event output.

>> I vaguely remember some issue regarding address bus width when trying to
>> implement the TopBlock small sectors, but I wasn't using the musicpal.
>> I'll see if I can find my old notes and test with your series.
> 
> The one boot chip I'm using is the AM29F080B-90SI with top boot blocks. It 
> has an 8-bit address bus.