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
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. 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. 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 -- 2.20.1 (Apple Git-117)
Patchew URL: https://patchew.org/QEMU/cover.1554755001.git.stephen.checkoway@oberlin.edu/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: cover.1554755001.git.stephen.checkoway@oberlin.edu
Subject: [Qemu-devel] [PATCH 00/10] block/pflash_cfi02: Implement missing AMD pflash functionality
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/cover.1554755001.git.stephen.checkoway@oberlin.edu -> patchew/cover.1554755001.git.stephen.checkoway@oberlin.edu
Switched to a new branch 'test'
b0552d6d4b block/pflash_cfi02: Use the chip erase time specified in the CFI table
e8c5cb8285 block/pflash_cfi02: Implement erase suspend/resume
06ea827393 block/pflash_cfi02: Implement multi-sector erase
6cfd6f4112 block/pflash_cfi02: Fix reset command not ignored during erase
a2dd176ca2 block/pflash_cfi02: Fix CFI in autoselect mode
d6aac26497 block/pflash_cfi02: Implement nonuniform sector sizes
7823825a45 block/pflash_cfi02: Implement intereleaved flash devices
0e8320e578 block/pflash_cfi02: Fix command address comparison
2d86dbe9a5 block/pflash_cfi02: Refactor, NFC intended
ef5bc94219 block/pflash_cfi02: Add test for supported commands
=== OUTPUT BEGIN ===
1/10 Checking commit ef5bc9421964 (block/pflash_cfi02: Add test for supported commands)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#42:
new file mode 100644
ERROR: spaces required around that '*' (ctx:VxV)
#67: FILE: tests/pflash-cfi02-test.c:21:
+#define MP_FLASH_SIZE_MAX (32*1024*1024)
^
ERROR: spaces required around that '*' (ctx:VxV)
#67: FILE: tests/pflash-cfi02-test.c:21:
+#define MP_FLASH_SIZE_MAX (32*1024*1024)
^
ERROR: spaces required around that '-' (ctx:VxV)
#68: FILE: tests/pflash-cfi02-test.c:22:
+#define BASE_ADDR (0x100000000ULL-MP_FLASH_SIZE_MAX)
^
ERROR: spaces required around that '*' (ctx:VxV)
#71: FILE: tests/pflash-cfi02-test.c:25:
+#define CFI_ADDR (FLASH_WIDTH*0x55)
^
ERROR: spaces required around that '*' (ctx:VxV)
#72: FILE: tests/pflash-cfi02-test.c:26:
+#define UNLOCK0_ADDR (FLASH_WIDTH*0x5555)
^
ERROR: spaces required around that '*' (ctx:VxV)
#73: FILE: tests/pflash-cfi02-test.c:27:
+#define UNLOCK1_ADDR (FLASH_WIDTH*0x2AAA)
^
ERROR: spaces required around that '*' (ctx:VxV)
#162: FILE: tests/pflash-cfi02-test.c:116:
+ g_assert_cmpint(flash_read(FLASH_WIDTH*0x0000), ==, 0x00BF);
^
ERROR: spaces required around that '*' (ctx:VxV)
#163: FILE: tests/pflash-cfi02-test.c:117:
+ g_assert_cmpint(flash_read(FLASH_WIDTH*0x0001), ==, 0x236D);
^
ERROR: spaces required around that '*' (ctx:VxV)
#168: FILE: tests/pflash-cfi02-test.c:122:
+ g_assert_cmpint(flash_read(FLASH_WIDTH*0x10), ==, 'Q');
^
ERROR: spaces required around that '*' (ctx:VxV)
#169: FILE: tests/pflash-cfi02-test.c:123:
+ g_assert_cmpint(flash_read(FLASH_WIDTH*0x11), ==, 'R');
^
ERROR: spaces required around that '*' (ctx:VxV)
#170: FILE: tests/pflash-cfi02-test.c:124:
+ g_assert_cmpint(flash_read(FLASH_WIDTH*0x12), ==, 'Y');
^
WARNING: line over 80 characters
#171: FILE: tests/pflash-cfi02-test.c:125:
+ g_assert_cmpint(flash_read(FLASH_WIDTH*0x2C), >=, 1); /* Num erase regions. */
ERROR: spaces required around that '*' (ctx:VxV)
#171: FILE: tests/pflash-cfi02-test.c:125:
+ g_assert_cmpint(flash_read(FLASH_WIDTH*0x2C), >=, 1); /* Num erase regions. */
^
ERROR: spaces required around that '*' (ctx:VxV)
#172: FILE: tests/pflash-cfi02-test.c:126:
+ uint32_t nb_sectors = flash_read(FLASH_WIDTH*0x2D) +
^
ERROR: spaces required around that '*' (ctx:VxV)
#173: FILE: tests/pflash-cfi02-test.c:127:
+ (flash_read(FLASH_WIDTH*0x2E) << 8) + 1;
^
ERROR: spaces required around that '*' (ctx:VxV)
#174: FILE: tests/pflash-cfi02-test.c:128:
+ uint32_t sector_len = (flash_read(FLASH_WIDTH*0x2F) << 8) +
^
ERROR: spaces required around that '*' (ctx:VxV)
#175: FILE: tests/pflash-cfi02-test.c:129:
+ (flash_read(FLASH_WIDTH*0x30) << 16);
^
ERROR: spaces required around that '*' (ctx:VxV)
#254: FILE: tests/pflash-cfi02-test.c:208:
+ if (ftruncate(fd, 8*1024*1024) < 0) {
^
ERROR: spaces required around that '*' (ctx:VxV)
#254: FILE: tests/pflash-cfi02-test.c:208:
+ if (ftruncate(fd, 8*1024*1024) < 0) {
^
total: 18 errors, 2 warnings, 238 lines checked
Patch 1/10 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/10 Checking commit 2d86dbe9a56f (block/pflash_cfi02: Refactor, NFC intended)
ERROR: spaces required around that '?' (ctx:VxW)
#101: FILE: hw/block/pflash_cfi02.c:172:
+ uint64_t ret = pfl->be? ldn_be_p(p, width) : ldn_le_p(p, width);
^
total: 1 errors, 0 warnings, 340 lines checked
Patch 2/10 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/10 Checking commit 0e8320e578b1 (block/pflash_cfi02: Fix command address comparison)
ERROR: spaces required around that '*' (ctx:VxV)
#55: FILE: tests/pflash-cfi02-test.c:26:
+#define UNLOCK0_ADDR (FLASH_WIDTH*0x555)
^
ERROR: spaces required around that '*' (ctx:VxV)
#56: FILE: tests/pflash-cfi02-test.c:27:
+#define UNLOCK1_ADDR (FLASH_WIDTH*0x2AA)
^
ERROR: spaces required around that '*' (ctx:VxV)
#65: FILE: tests/pflash-cfi02-test.c:195:
+ flash_write(FLASH_WIDTH*0x5555, UNLOCK0_CMD);
^
ERROR: spaces required around that '*' (ctx:VxV)
#66: FILE: tests/pflash-cfi02-test.c:196:
+ flash_write(FLASH_WIDTH*0x2AAA, UNLOCK1_CMD);
^
ERROR: spaces required around that '*' (ctx:VxV)
#67: FILE: tests/pflash-cfi02-test.c:197:
+ flash_write(FLASH_WIDTH*0x5555, AUTOSELECT_CMD);
^
ERROR: spaces required around that '*' (ctx:VxV)
#68: FILE: tests/pflash-cfi02-test.c:198:
+ g_assert_cmpint(flash_read(FLASH_WIDTH*0x0000), ==, 0x00BF);
^
ERROR: spaces required around that '*' (ctx:VxV)
#69: FILE: tests/pflash-cfi02-test.c:199:
+ g_assert_cmpint(flash_read(FLASH_WIDTH*0x0001), ==, 0x236D);
^
total: 7 errors, 0 warnings, 48 lines checked
Patch 3/10 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/10 Checking commit 7823825a45cc (block/pflash_cfi02: Implement intereleaved flash devices)
ERROR: if this code is redundant consider removing it
#214: FILE: hw/block/pflash_cfi02.c:300:
+#if 0
total: 1 errors, 0 warnings, 995 lines checked
Patch 4/10 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/10 Checking commit d6aac264973d (block/pflash_cfi02: Implement nonuniform sector sizes)
6/10 Checking commit a2dd176ca2b6 (block/pflash_cfi02: Fix CFI in autoselect mode)
7/10 Checking commit 6cfd6f4112db (block/pflash_cfi02: Fix reset command not ignored during erase)
ERROR: Missing Signed-off-by: line(s)
total: 1 errors, 0 warnings, 9 lines checked
Patch 7/10 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
8/10 Checking commit 06ea82739330 (block/pflash_cfi02: Implement multi-sector erase)
9/10 Checking commit e8c5cb8285b4 (block/pflash_cfi02: Implement erase suspend/resume)
10/10 Checking commit b0552d6d4bf2 (block/pflash_cfi02: Use the chip erase time specified in the CFI table)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/cover.1554755001.git.stephen.checkoway@oberlin.edu/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
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 >
Hi Phil, > On Apr 9, 2019, at 06:34, Philippe Mathieu-Daudé <philmd@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.checkoway@oberlin.edu/bbe417c354dfa908d5e1bc8f8ad7121ec9451682.1554774454.git.stephen.checkoway@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? > >> 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 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. -- Stephen Checkoway
On 4/9/19 5:55 PM, Stephen Checkoway wrote: > Hi Phil, > >> On Apr 9, 2019, at 06:34, Philippe Mathieu-Daudé <philmd@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.checkoway@oberlin.edu/bbe417c354dfa908d5e1bc8f8ad7121ec9451682.1554774454.git.stephen.checkoway@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.
On Apr 9, 2019, at 12:15, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > 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. That's a good idea. Some of the values are essentially arbitrary (e.g., how long an erase typically takes) but the CFI values that indicate support for erase/suspend and erase regions at least should be tested. I'll take a closer look at what all I changed (it has been a few months since I wrote the code). Are there any in particular you think I should be sure to test? Do you want me to add those tests to the appropriate patches in the series or would you like an additional patch that adds those tests? (The later is easier to do as modifying the earlier patches in the series are likely to cause conflicts with later patches that I'd need to resolve, but I can do either.) > 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 probably cannot share the binary but the unlock addresses are 0x1554 and 0x2AA8. This is for two interleaved x8/x16 chips in x16 mode so shifting right by 2 (1 for the interleaving and 1 because it's an x16 chip) gives 0x555 and 0xAAA. The appropriate (word) unlock addresses for the chips are 0x555 and 0x2AA which is what you get if you restrict to 11 bits. My guess is that the firmware authors took the byte unlock addresses (0xAAA and 0x555), shifted left by 2, discovered that this didn't work, tried the unlock addresses in the opposite order, and found that that worked due to the chips only looking at 11 bits. Looking at hw/arm/musicpal.c, I see that it is using unlock addresses 0x5555 and 0x2AAA. My guess is this reflects a bug in some firmware and it should be using 0x555 and 0x2AA. I haven't seen any AMD pflash chips that used other unlock addresses, but I've only looked at about a dozen part sheets and I'm not sure what chips the musicpal is actually using. -- Stephen Checkoway
On Apr 9, 2019, at 12:15, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > 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. In v3 of the patch, I added tests for the CFI table values - device length - sector length and number of sectors - erase suspend supported -- Stephen Checkoway
© 2016 - 2024 Red Hat, Inc.