diff mbox

[1/7] tests: add a m25p80 test

Message ID 1467634738-28642-2-git-send-email-clg@kaod.org
State New
Headers show

Commit Message

Cédric Le Goater July 4, 2016, 12:18 p.m. UTC
This test uses the palmetto platform and the AST2400 SPI controller to
test the m25p80 flash module device model. The flash model is defined
by the platform (n25q256a) and it would be nice to find way to control
it, using a property probably.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---

 Changes since v5:

 - fixed BE host support by using mem{write,read} for flash accesses
 
 Changes since v4:

 - fixed Makefile targets
 - replaced -M with -m in qtest command line
 
 Changes since v2:

 - changed mkstemp() path prefix
 
 Changes since v1:

 - fixed guest args to use -drive and not -mtdblock

 tests/Makefile.include |   2 +
 tests/m25p80-test.c    | 256 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 258 insertions(+)
 create mode 100644 tests/m25p80-test.c

Comments

Peter Maydell July 4, 2016, 12:24 p.m. UTC | #1
On 4 July 2016 at 13:18, Cédric Le Goater <clg@kaod.org> wrote:
> This test uses the palmetto platform and the AST2400 SPI controller to
> test the m25p80 flash module device model. The flash model is defined
> by the platform (n25q256a) and it would be nice to find way to control
> it, using a property probably.


> +static inline void flash_writel(uint32_t data)
> +{
> +    data = cpu_to_be32(data);
> +    memwrite(AST2400_FLASH_BASE, &data, 4);
> +}
> +
> +static inline uint32_t flash_readl(void)
> +{
> +    uint32_t data;
> +
> +    memread(AST2400_FLASH_BASE, &data, 4);
> +    return be32_to_cpu(data);
> +}

These are weird. As per my question in the other thread, if you
were writing this as a piece of test code that ran natively
in the guest, how would you write it?

thanks
-- PMM
Cédric Le Goater July 4, 2016, 12:39 p.m. UTC | #2
On 07/04/2016 02:24 PM, Peter Maydell wrote:
> On 4 July 2016 at 13:18, Cédric Le Goater <clg@kaod.org> wrote:
>> This test uses the palmetto platform and the AST2400 SPI controller to
>> test the m25p80 flash module device model. The flash model is defined
>> by the platform (n25q256a) and it would be nice to find way to control
>> it, using a property probably.
> 
> 
>> +static inline void flash_writel(uint32_t data)
>> +{
>> +    data = cpu_to_be32(data);
>> +    memwrite(AST2400_FLASH_BASE, &data, 4);
>> +}
>> +
>> +static inline uint32_t flash_readl(void)
>> +{
>> +    uint32_t data;
>> +
>> +    memread(AST2400_FLASH_BASE, &data, 4);
>> +    return be32_to_cpu(data);
>> +}
> 
> These are weird. As per my question in the other thread, if you
> were writing this as a piece of test code that ran natively
> in the guest, how would you write it?

Here is the uboot flash code :

	https://github.com/openbmc/u-boot/blob/v2016.05-aspeed-openbmc/arch/arm/mach-aspeed/flash.c#L420

So these are doing byte per byte, but there is no reason not to do
long.


C.
Peter Maydell July 4, 2016, 12:51 p.m. UTC | #3
On 4 July 2016 at 13:39, Cédric Le Goater <clg@kaod.org> wrote:
> On 07/04/2016 02:24 PM, Peter Maydell wrote:
>> On 4 July 2016 at 13:18, Cédric Le Goater <clg@kaod.org> wrote:
>>> This test uses the palmetto platform and the AST2400 SPI controller to
>>> test the m25p80 flash module device model. The flash model is defined
>>> by the platform (n25q256a) and it would be nice to find way to control
>>> it, using a property probably.
>>
>>
>>> +static inline void flash_writel(uint32_t data)
>>> +{
>>> +    data = cpu_to_be32(data);
>>> +    memwrite(AST2400_FLASH_BASE, &data, 4);
>>> +}
>>> +
>>> +static inline uint32_t flash_readl(void)
>>> +{
>>> +    uint32_t data;
>>> +
>>> +    memread(AST2400_FLASH_BASE, &data, 4);
>>> +    return be32_to_cpu(data);
>>> +}
>>
>> These are weird. As per my question in the other thread, if you
>> were writing this as a piece of test code that ran natively
>> in the guest, how would you write it?
>
> Here is the uboot flash code :
>
>         https://github.com/openbmc/u-boot/blob/v2016.05-aspeed-openbmc/arch/arm/mach-aspeed/flash.c#L420
>
> So these are doing byte per byte, but there is no reason not to do
> long.

If you want "write a long", that is what writel does,
and your test code should be able to use it in exactly
the same way native guest code would write a register
value with a str instruction.

It's very easy to work around endianness issues by
randomly changing things until the tests pass, but it's
much better to actually identify what you need to do
and where the code is diverging from that.

thanks
-- PMM
Cédric Le Goater July 4, 2016, 1:08 p.m. UTC | #4
On 07/04/2016 02:51 PM, Peter Maydell wrote:
> On 4 July 2016 at 13:39, Cédric Le Goater <clg@kaod.org> wrote:
>> On 07/04/2016 02:24 PM, Peter Maydell wrote:
>>> On 4 July 2016 at 13:18, Cédric Le Goater <clg@kaod.org> wrote:
>>>> This test uses the palmetto platform and the AST2400 SPI controller to
>>>> test the m25p80 flash module device model. The flash model is defined
>>>> by the platform (n25q256a) and it would be nice to find way to control
>>>> it, using a property probably.
>>>
>>>
>>>> +static inline void flash_writel(uint32_t data)
>>>> +{
>>>> +    data = cpu_to_be32(data);
>>>> +    memwrite(AST2400_FLASH_BASE, &data, 4);
>>>> +}
>>>> +
>>>> +static inline uint32_t flash_readl(void)
>>>> +{
>>>> +    uint32_t data;
>>>> +
>>>> +    memread(AST2400_FLASH_BASE, &data, 4);
>>>> +    return be32_to_cpu(data);
>>>> +}
>>>
>>> These are weird. As per my question in the other thread, if you
>>> were writing this as a piece of test code that ran natively
>>> in the guest, how would you write it?
>>
>> Here is the uboot flash code :
>>
>>         https://github.com/openbmc/u-boot/blob/v2016.05-aspeed-openbmc/arch/arm/mach-aspeed/flash.c#L420
>>
>> So these are doing byte per byte, but there is no reason not to do
>> long.
> 
> If you want "write a long", that is what writel does,
> and your test code should be able to use it in exactly
> the same way native guest code would write a register
> value with a str instruction.
> 
> It's very easy to work around endianness issues by
> randomly changing things until the tests pass, but it's
> much better to actually identify what you need to do
> and where the code is diverging from that.

I agree. Let's forget that patch until I have clarified
how it should be done.

Thanks,

C.
diff mbox

Patch

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 6c09962f7581..149f92585c1b 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -252,6 +252,7 @@  gcov-files-sparc-y += hw/timer/m48t59.c
 gcov-files-sparc64-y += hw/timer/m48t59.c
 check-qtest-arm-y = tests/tmp105-test$(EXESUF)
 check-qtest-arm-y += tests/ds1338-test$(EXESUF)
+check-qtest-arm-y += tests/m25p80-test$(EXESUF)
 gcov-files-arm-y += hw/misc/tmp105.c
 check-qtest-arm-y += tests/virtio-blk-test$(EXESUF)
 gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c
@@ -568,6 +569,7 @@  tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o \
 tests/pxe-test$(EXESUF): tests/pxe-test.o tests/boot-sector.o $(libqos-obj-y)
 tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
 tests/ds1338-test$(EXESUF): tests/ds1338-test.o $(libqos-imx-obj-y)
+tests/m25p80-test$(EXESUF): tests/m25p80-test.o
 tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
 tests/q35-test$(EXESUF): tests/q35-test.o $(libqos-pc-obj-y)
 tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
diff --git a/tests/m25p80-test.c b/tests/m25p80-test.c
new file mode 100644
index 000000000000..f8a99b9a387f
--- /dev/null
+++ b/tests/m25p80-test.c
@@ -0,0 +1,256 @@ 
+/*
+ * QTest testcase for the M25P80 Flash (Using the AST2400 SPI Controller)
+ *
+ * Copyright (C) 2016 IBM Corp.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/bswap.h"
+#include "libqtest.h"
+
+/*
+ * AST2400 SPI Controller registers
+ */
+#define R_CONF              0x00
+#define   CONF_ENABLE_W0       (1 << 16)
+#define R_CE_CTRL           0x04
+#define   CRTL_EXTENDED0       0  /* 32 bit addressing for SPI */
+#define R_CTRL0             0x10
+#define   CTRL_CE_STOP_ACTIVE  (1 << 2)
+#define   CTRL_USERMODE        0x3
+
+#define AST2400_FMC_BASE    0x1E620000
+#define AST2400_FLASH_BASE  0x20000000
+
+/*
+ * Flash commands
+ */
+enum {
+    JEDEC_READ = 0x9f,
+    BULK_ERASE = 0xc7,
+    READ = 0x03,
+    PP = 0x02,
+    WREN = 0x6,
+    EN_4BYTE_ADDR = 0xB7,
+    ERASE_SECTOR = 0xd8,
+};
+
+#define FLASH_JEDEC         0x20ba19  /* n25q256a */
+#define FLASH_SIZE          (32 * 1024 * 1024)
+
+#define PAGE_SIZE           256
+
+static inline void flash_writel(uint32_t data)
+{
+    data = cpu_to_be32(data);
+    memwrite(AST2400_FLASH_BASE, &data, 4);
+}
+
+static inline uint32_t flash_readl(void)
+{
+    uint32_t data;
+
+    memread(AST2400_FLASH_BASE, &data, 4);
+    return be32_to_cpu(data);
+}
+
+static void spi_conf(uint32_t value)
+{
+    uint32_t conf = readl(AST2400_FMC_BASE + R_CONF);
+
+    conf |= value;
+    writel(AST2400_FMC_BASE + R_CONF, conf);
+}
+
+static void spi_ctrl_start_user(void)
+{
+    uint32_t ctrl = readl(AST2400_FMC_BASE + R_CTRL0);
+
+    ctrl |= CTRL_USERMODE | CTRL_CE_STOP_ACTIVE;
+    writel(AST2400_FMC_BASE + R_CTRL0, ctrl);
+
+    ctrl &= ~CTRL_CE_STOP_ACTIVE;
+    writel(AST2400_FMC_BASE + R_CTRL0, ctrl);
+}
+
+static void spi_ctrl_stop_user(void)
+{
+    uint32_t ctrl = readl(AST2400_FMC_BASE + R_CTRL0);
+
+    ctrl |= CTRL_USERMODE | CTRL_CE_STOP_ACTIVE;
+    writel(AST2400_FMC_BASE + R_CTRL0, ctrl);
+}
+
+static void test_read_jedec(void)
+{
+    uint32_t jedec = 0x0;
+
+    spi_conf(CONF_ENABLE_W0);
+
+    spi_ctrl_start_user();
+    writeb(AST2400_FLASH_BASE, JEDEC_READ);
+    jedec |= readb(AST2400_FLASH_BASE) << 16;
+    jedec |= readb(AST2400_FLASH_BASE) << 8;
+    jedec |= readb(AST2400_FLASH_BASE);
+    spi_ctrl_stop_user();
+
+    g_assert_cmphex(jedec, ==, FLASH_JEDEC);
+}
+
+static void read_page(uint32_t addr, uint32_t *page)
+{
+    int i;
+
+    spi_ctrl_start_user();
+
+    writeb(AST2400_FLASH_BASE, EN_4BYTE_ADDR);
+    writeb(AST2400_FLASH_BASE, READ);
+    flash_writel(addr);
+
+    /* Continuous read are supported */
+    for (i = 0; i < PAGE_SIZE / 4; i++) {
+        page[i] = flash_readl();
+    }
+    spi_ctrl_stop_user();
+}
+
+static void test_erase_sector(void)
+{
+    uint32_t some_page_addr = 0x600 * PAGE_SIZE;
+    uint32_t page[PAGE_SIZE / 4];
+    int i;
+
+    spi_conf(CONF_ENABLE_W0);
+
+    spi_ctrl_start_user();
+    writeb(AST2400_FLASH_BASE, WREN);
+    writeb(AST2400_FLASH_BASE, EN_4BYTE_ADDR);
+    writeb(AST2400_FLASH_BASE, ERASE_SECTOR);
+    flash_writel(some_page_addr);
+    spi_ctrl_stop_user();
+
+    /* Previous page should be full of zeroes as backend is not
+     * initialized */
+    read_page(some_page_addr - PAGE_SIZE, page);
+    for (i = 0; i < PAGE_SIZE / 4; i++) {
+        g_assert_cmphex(page[i], ==, 0x0);
+    }
+
+    /* But this one was erased */
+    read_page(some_page_addr, page);
+    for (i = 0; i < PAGE_SIZE / 4; i++) {
+        g_assert_cmphex(page[i], ==, 0xffffffff);
+    }
+}
+
+static void test_erase_all(void)
+{
+    uint32_t some_page_addr = 0x15000 * PAGE_SIZE;
+    uint32_t page[PAGE_SIZE / 4];
+    int i;
+
+    spi_conf(CONF_ENABLE_W0);
+
+    /* Check some random page. Should be full of zeroes as backend is
+     * not initialized */
+    read_page(some_page_addr, page);
+    for (i = 0; i < PAGE_SIZE / 4; i++) {
+        g_assert_cmphex(page[i], ==, 0x0);
+    }
+
+    spi_ctrl_start_user();
+    writeb(AST2400_FLASH_BASE, WREN);
+    writeb(AST2400_FLASH_BASE, BULK_ERASE);
+    spi_ctrl_stop_user();
+
+    /* Recheck that some random page */
+    read_page(some_page_addr, page);
+    for (i = 0; i < PAGE_SIZE / 4; i++) {
+        g_assert_cmphex(page[i], ==, 0xffffffff);
+    }
+}
+
+static void test_write_page(void)
+{
+    uint32_t my_page_addr = 0x14000 * PAGE_SIZE; /* beyond 16MB */
+    uint32_t some_page_addr = 0x15000 * PAGE_SIZE;
+    uint32_t page[PAGE_SIZE / 4];
+    int i;
+
+    spi_conf(CONF_ENABLE_W0);
+
+    spi_ctrl_start_user();
+    writeb(AST2400_FLASH_BASE, EN_4BYTE_ADDR);
+    writeb(AST2400_FLASH_BASE, PP);
+    flash_writel(my_page_addr);
+
+    /* Fill the page with its own addresses */
+    for (i = 0; i < PAGE_SIZE / 4; i++) {
+        flash_writel(my_page_addr + i * 4);
+    }
+    spi_ctrl_stop_user();
+
+    /* Check what was written */
+    read_page(my_page_addr, page);
+    for (i = 0; i < PAGE_SIZE / 4; i++) {
+        g_assert_cmphex(page[i], ==, my_page_addr + i * 4);
+    }
+
+    /* Check some other page. It should be full of 0xff */
+    read_page(some_page_addr, page);
+    for (i = 0; i < PAGE_SIZE / 4; i++) {
+        g_assert_cmphex(page[i], ==, 0xffffffff);
+    }
+}
+
+static char tmp_path[] = "/tmp/qtest.m25p80.XXXXXX";
+
+int main(int argc, char **argv)
+{
+    int ret;
+    int fd;
+    char *args;
+
+    g_test_init(&argc, &argv, NULL);
+
+    fd = mkstemp(tmp_path);
+    g_assert(fd >= 0);
+    ret = ftruncate(fd, FLASH_SIZE);
+    g_assert(ret == 0);
+    close(fd);
+
+    args = g_strdup_printf("-m 256 -machine palmetto-bmc "
+                           "-drive file=%s,format=raw,if=mtd",
+                           tmp_path);
+    qtest_start(args);
+
+    qtest_add_func("/m25p80/read_jedec", test_read_jedec);
+    qtest_add_func("/m25p80/erase_sector", test_erase_sector);
+    qtest_add_func("/m25p80/erase_all",  test_erase_all);
+    qtest_add_func("/m25p80/write_page", test_write_page);
+
+    ret = g_test_run();
+
+    qtest_quit(global_qtest);
+    unlink(tmp_path);
+    g_free(args);
+    return ret;
+}