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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3307 bytes --]

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).

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.

> 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?

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.

Regards,

Phil.

> Stephen Checkoway (10):
>   block/pflash_cfi02: Add test for supported commands
>   block/pflash_cfi02: Refactor, NFC intended
>   block/pflash_cfi02: Fix command address comparison
>   block/pflash_cfi02: Implement intereleaved flash devices
>   block/pflash_cfi02: Implement nonuniform sector sizes
>   block/pflash_cfi02: Fix CFI in autoselect mode
>   block/pflash_cfi02: Fix reset command not ignored during erase
>   block/pflash_cfi02: Implement multi-sector erase
>   block/pflash_cfi02: Implement erase suspend/resume
>   block/pflash_cfi02: Use the chip erase time specified in the CFI table
> 
>  hw/block/pflash_cfi02.c   | 843 +++++++++++++++++++++++++++-----------
>  tests/Makefile.include    |   2 +
>  tests/pflash-cfi02-test.c | 757 ++++++++++++++++++++++++++++++++++
>  3 files changed, 1367 insertions(+), 235 deletions(-)
>  create mode 100644 tests/pflash-cfi02-test.c
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]