On 4/9/19 5:55 PM, Stephen Checkoway wrote: > Hi Phil, > >> On Apr 9, 2019, at 06:34, Philippe Mathieu-Daudé <phi...@redhat.com> 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/cover.1554774454.git.stephen.checko...@oberlin.edu/bbe417c354dfa908d5e1bc8f8ad7121ec9451682.1554774454.git.stephen.checko...@oberlin.edu/>. > > 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.
