[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 01/10] block/pflash_cfi02: Add test for suppo
From: |
Thomas Huth |
Subject: |
Re: [Qemu-devel] [PATCH v3 01/10] block/pflash_cfi02: Add test for supported commands |
Date: |
Thu, 18 Apr 2019 06:47:56 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 |
On 18/04/2019 00.01, Stephen Checkoway wrote:
> Test the AMD command set for parallel flash chips. This test uses an
> ARM musicpal board with a pflash drive to test the following list of
> currently-supported commands.
> - Autoselect
> - CFI
> - Sector erase
> - Chip erase
> - Program
> - Unlock bypass
> - Reset
>
> Signed-off-by: Stephen Checkoway <address@hidden>
> ---
> tests/Makefile.include | 2 +
> tests/pflash-cfi02-test.c | 227 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 229 insertions(+)
> create mode 100644 tests/pflash-cfi02-test.c
[...]
> new file mode 100644
> index 0000000000..b113fca5af
> --- /dev/null
> +++ b/tests/pflash-cfi02-test.c
> @@ -0,0 +1,227 @@
> +/*
> + * QTest testcase for parallel flash with AMD command set
> + *
> + * Copyright (c) 2018 Stephen Checkoway
Do you maybe want to update that to 2019?
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include <err.h>
We generally don't use err.h in QEMU ... could you please use the
standard functions from glib instead?
> +#include <unistd.h>
unistd.h is already provided by osdep.h, so you don't need to include it
here again. (and the scripts/clean-includes script will barf at this
later, so better fix it right from the start)
> +#include "libqtest.h"
> +
> +/*
> + * To test the pflash_cfi02 device, we run QEMU with the musicpal machine
> with
> + * a pflash drive. This enables us to test some flash configurations, but not
> + * all. In particular, we're limited to a 16-bit wide flash device.
> + */
> +
> +#define MP_FLASH_SIZE_MAX (32 * 1024 * 1024)
> +#define BASE_ADDR (0x100000000ULL - MP_FLASH_SIZE_MAX)
> +
> +#define FLASH_WIDTH 2
> +#define CFI_ADDR (FLASH_WIDTH * 0x55)
> +#define UNLOCK0_ADDR (FLASH_WIDTH * 0x5555)
> +#define UNLOCK1_ADDR (FLASH_WIDTH * 0x2AAA)
> +
> +#define CFI_CMD 0x98
> +#define UNLOCK0_CMD 0xAA
> +#define UNLOCK1_CMD 0x55
> +#define AUTOSELECT_CMD 0x90
> +#define RESET_CMD 0xF0
> +#define PROGRAM_CMD 0xA0
> +#define SECTOR_ERASE_CMD 0x30
> +#define CHIP_ERASE_CMD 0x10
> +#define UNLOCK_BYPASS_CMD 0x20
> +#define UNLOCK_BYPASS_RESET_CMD 0x00
> +
> +static char image_path[] = "/tmp/qtest.XXXXXX";
> +
> +static inline void flash_write(uint64_t byte_addr, uint16_t data)
> +{
> + qtest_writew(global_qtest, BASE_ADDR + byte_addr, data);
> +}
> +
> +static inline uint16_t flash_read(uint64_t byte_addr)
> +{
> + return qtest_readw(global_qtest, BASE_ADDR + byte_addr);
> +}
> +
> +static void unlock(void)
> +{
> + flash_write(UNLOCK0_ADDR, UNLOCK0_CMD);
> + flash_write(UNLOCK1_ADDR, UNLOCK1_CMD);
> +}
> +
> +static void reset(void)
> +{
> + flash_write(0, RESET_CMD);
> +}
> +
> +static void sector_erase(uint64_t byte_addr)
> +{
> + unlock();
> + flash_write(UNLOCK0_ADDR, 0x80);
> + unlock();
> + flash_write(byte_addr, SECTOR_ERASE_CMD);
> +}
> +
> +static void wait_for_completion(uint64_t byte_addr)
> +{
> + /* If DQ6 is toggling, step the clock and ensure the toggle stops. */
> + if ((flash_read(byte_addr) & 0x40) ^ (flash_read(byte_addr) & 0x40)) {
> + /* Wait for erase or program to finish. */
> + clock_step_next();
> + /* Ensure that DQ6 has stopped toggling. */
> + g_assert_cmpint(flash_read(byte_addr), ==, flash_read(byte_addr));
> + }
> +}
> +
> +static void bypass_program(uint64_t byte_addr, uint16_t data)
> +{
> + flash_write(UNLOCK0_ADDR, PROGRAM_CMD);
> + flash_write(byte_addr, data);
> + /*
> + * Data isn't valid until DQ6 stops toggling. We don't model this as
> + * writes are immediate, but if this changes in the future, we can wait
> + * until the program is complete.
> + */
> + wait_for_completion(byte_addr);
> +}
> +
> +static void program(uint64_t byte_addr, uint16_t data)
> +{
> + unlock();
> + bypass_program(byte_addr, data);
> +}
> +
> +static void chip_erase(void)
> +{
> + unlock();
> + flash_write(UNLOCK0_ADDR, 0x80);
> + unlock();
> + flash_write(UNLOCK0_ADDR, SECTOR_ERASE_CMD);
> +}
> +
> +static void test_flash(void)
> +{
> + global_qtest = qtest_initf("-M musicpal,accel=qtest "
> + "-drive
> if=pflash,file=%s,format=raw,copy-on-read",
> + image_path);
> + /* Check the IDs. */
> + unlock();
> + flash_write(UNLOCK0_ADDR, AUTOSELECT_CMD);
> + g_assert_cmpint(flash_read(FLASH_WIDTH * 0x0000), ==, 0x00BF);
> + g_assert_cmpint(flash_read(FLASH_WIDTH * 0x0001), ==, 0x236D);
> + reset();
> +
> + /* Check the erase blocks. */
> + flash_write(CFI_ADDR, CFI_CMD);
> + g_assert_cmpint(flash_read(FLASH_WIDTH * 0x10), ==, 'Q');
> + g_assert_cmpint(flash_read(FLASH_WIDTH * 0x11), ==, 'R');
> + g_assert_cmpint(flash_read(FLASH_WIDTH * 0x12), ==, 'Y');
> + /* Num erase regions. */
> + g_assert_cmpint(flash_read(FLASH_WIDTH * 0x2C), >=, 1);
> + uint32_t nb_sectors = flash_read(FLASH_WIDTH * 0x2D) +
> + (flash_read(FLASH_WIDTH * 0x2E) << 8) + 1;
> + uint32_t sector_len = (flash_read(FLASH_WIDTH * 0x2F) << 8) +
> + (flash_read(FLASH_WIDTH * 0x30) << 16);
> + reset();
> +
> + /* Erase and program sector. */
> + for (uint32_t i = 0; i < nb_sectors; ++i) {
> + uint64_t byte_addr = i * sector_len;
> + sector_erase(byte_addr);
> + /* Read toggle. */
> + uint16_t status0 = flash_read(byte_addr);
> + /* DQ7 is 0 during an erase. */
> + g_assert_cmpint(status0 & 0x80, ==, 0);
> + uint16_t status1 = flash_read(byte_addr);
> + /* DQ6 toggles during an erase. */
> + g_assert_cmpint(status0 & 0x40, !=, status1 & 0x40);
> + /* Wait for erase to complete. */
> + clock_step_next();
> + /* Ensure DQ6 has stopped toggling. */
> + g_assert_cmpint(flash_read(byte_addr), ==, flash_read(byte_addr));
> + /* Now the data should be valid. */
> + g_assert_cmpint(flash_read(byte_addr), ==, 0xFFFF);
> +
> + /* Program a bit pattern. */
> + program(byte_addr, 0x5555);
> + g_assert_cmpint(flash_read(byte_addr), ==, 0x5555);
> + program(byte_addr, 0xAA55);
> + g_assert_cmpint(flash_read(byte_addr), ==, 0x0055);
> + }
> +
> + /* Erase the chip. */
> + chip_erase();
> + /* Read toggle. */
> + uint16_t status0 = flash_read(0);
> + /* DQ7 is 0 during an erase. */
> + g_assert_cmpint(status0 & 0x80, ==, 0);
> + uint16_t status1 = flash_read(0);
> + /* DQ6 toggles during an erase. */
> + g_assert_cmpint(status0 & 0x40, !=, status1 & 0x40);
> + /* Wait for erase to complete. */
> + clock_step_next();
> + /* Ensure DQ6 has stopped toggling. */
> + g_assert_cmpint(flash_read(0), ==, flash_read(0));
> + /* Now the data should be valid. */
> + g_assert_cmpint(flash_read(0), ==, 0xFFFF);
> +
> + /* Unlock bypass */
> + unlock();
> + flash_write(UNLOCK0_ADDR, UNLOCK_BYPASS_CMD);
> + bypass_program(0, 0x0123);
> + bypass_program(2, 0x4567);
> + bypass_program(4, 0x89AB);
> + /*
> + * Test that bypass programming, unlike normal programming can use any
> + * address for the PROGRAM_CMD.
> + */
> + flash_write(6, PROGRAM_CMD);
> + flash_write(6, 0xCDEF);
> + wait_for_completion(6);
> + flash_write(0, UNLOCK_BYPASS_RESET_CMD);
> + bypass_program(8, 0x55AA); /* Should fail. */
> + g_assert_cmpint(flash_read(0), ==, 0x0123);
> + g_assert_cmpint(flash_read(2), ==, 0x4567);
> + g_assert_cmpint(flash_read(4), ==, 0x89AB);
> + g_assert_cmpint(flash_read(6), ==, 0xCDEF);
> + g_assert_cmpint(flash_read(8), ==, 0xFFFF);
> +
> + qtest_quit(global_qtest);
> +}
> +
> +static void cleanup(void *opaque)
> +{
> + unlink(image_path);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + int fd = mkstemp(image_path);
> + if (fd == -1) {
> + err(1, "Failed to create temporary file %s", image_path);
g_printerr() ?
or maybe simply use a g_assert_cmpint(ret, !=, -1) like we do it in a
couple of other places already.
> + }
> + if (ftruncate(fd, 8 * 1024 * 1024) < 0) {
> + int error_code = errno;
> + close(fd);
> + unlink(image_path);
> + g_printerr("Failed to truncate file %s to 8 MB: %s\n", image_path,
> + strerror(error_code));
> + exit(EXIT_FAILURE);
> + }
> + close(fd);
> +
> + qtest_add_abrt_handler(cleanup, NULL);
> + g_test_init(&argc, &argv, NULL);
> + qtest_add_func("pflash-cfi02", test_flash);
> + int result = g_test_run();
> + cleanup(NULL);
> + return result;
> +}
> +
> +/* vim: set sw=4 sts=4 ts=8 et: */
IMHO the vim line should not be necessary (and we hardly do that in any
of the other files).
Thomas
- [Qemu-devel] [PATCH v3 00/10] block/pflash_cfi02: Implement missing AMD pflash functionality, Stephen Checkoway, 2019/04/17
- [Qemu-devel] [PATCH v3 03/10] block/pflash_cfi02: Fix command address comparison, Stephen Checkoway, 2019/04/17
- [Qemu-devel] [PATCH v3 02/10] block/pflash_cfi02: Refactor, NFC intended, Stephen Checkoway, 2019/04/17
- [Qemu-devel] [PATCH v3 06/10] block/pflash_cfi02: Fix CFI in autoselect mode, Stephen Checkoway, 2019/04/17
- [Qemu-devel] [PATCH v3 07/10] block/pflash_cfi02: Fix reset command not ignored during erase, Stephen Checkoway, 2019/04/17
- [Qemu-devel] [PATCH v3 08/10] block/pflash_cfi02: Implement multi-sector erase, Stephen Checkoway, 2019/04/17