[Qemu-devel] [PATCH 0/7] ast2400: U-boot support

[Qemu-devel] [PATCH 0/7] ast2400: U-boot support

[Cédric Le Goater]

Hello,

Here is a serie adding the changes requires to boot a ast2400 guest
directly from a flash image. These images generally start with the
U-boot bootloader.

U-boot on the Aspeed SOCs makes use of the SPI controller in the
Command mode. In this mode, the flash content is mapped to a memory
region and accesses are treated like mmios. The proposal below adds a
routine which permits us to share the memory storage between the flash
object and the SPI controller handling the MMIOs. This is very close
to the approach taken by the pflash_cfi object.


The patchset starts with a test case for the m25p80 which now supports
BE host, followed with an addon for the mx25l25635f flash modules.
These modules are common in the OpenPower ecosystem. Then come the
changes to support the SPI controller Command mode and the mapping of
the first BMC flash at 0x0. Last, is initial support for the memory
controller which is used in some occasion to retrieve the size of the
RAM.

Thanks,

C.

Cédric Le Goater (7):
  tests: add a m25p80 test
  m25p80: add mx25l25635f chip
  ast2400: use a mx25l25635f chip
  m25p80: add a m25p80_set_rom_storage() routine
  ast2400: handle SPI flash Command mode (read only)
  ast2400: use contents of first SPI flash as a rom
  ast2400: add a memory controller device model

 hw/arm/ast2400.c              |  13 +++
 hw/arm/palmetto-bmc.c         |  22 +++-
 hw/block/m25p80.c             |  18 ++-
 hw/misc/Makefile.objs         |   2 +-
 hw/misc/aspeed_sdmc.c         | 126 +++++++++++++++++++++
 hw/ssi/aspeed_smc.c           |  60 +++++++++-
 include/hw/arm/ast2400.h      |   1 +
 include/hw/block/flash.h      |   2 +
 include/hw/misc/aspeed_sdmc.h |  49 ++++++++
 include/hw/ssi/aspeed_smc.h   |   1 +
 tests/Makefile.include        |   2 +
 tests/m25p80-test.c           | 256 ++++++++++++++++++++++++++++++++++++++++++
 12 files changed, 544 insertions(+), 8 deletions(-)
 create mode 100644 hw/misc/aspeed_sdmc.c
 create mode 100644 include/hw/misc/aspeed_sdmc.h
 create mode 100644 tests/m25p80-test.c

-- 
2.1.4

[Qemu-devel] [PATCH 1/7] tests: add a m25p80 test

[Cédric Le Goater]

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

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;
+}
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/7] tests: add a m25p80 test

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH 1/7] tests: add a m25p80 test

[Cédric Le Goater]

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.

Re: [Qemu-devel] [PATCH 1/7] tests: add a m25p80 test

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH 1/7] tests: add a m25p80 test

[Cédric Le Goater]

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.

[Qemu-devel] [PATCH 2/7] m25p80: add mx25l25635f chip

[Cédric Le Goater]

The Macronix chip mx25l25635f used on some OpenPower boxes is very
similar to the mx25l25635e. They share the same JEDEC identifier but
the WRSR instruction requires 2 bytes in the mx25l25635f case.

To prevent some warnings on guests, let's introduce a new chip
identifying JEDEC 0xc22019 as a MX25L25635F chip.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/block/m25p80.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index d9b27939dde6..aa964280beab 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -199,6 +199,7 @@ static const FlashPartInfo known_devices[] = {
     { INFO("mx25l12805d", 0xc22018,      0,  64 << 10, 256, 0) },
     { INFO("mx25l12855e", 0xc22618,      0,  64 << 10, 256, 0) },
     { INFO("mx25l25635e", 0xc22019,      0,  64 << 10, 512, 0) },
+    { INFO("mx25l25635f", 0xc22019,      0,  64 << 10, 512, 0) },
     { INFO("mx25l25655e", 0xc22619,      0,  64 << 10, 512, 0) },
     { INFO("mx66u51235f", 0xc2253a,      0,  64 << 10, 1024, ER_4K | ER_32K) },
     { INFO("mx66u1g45g",  0xc2253b,      0,  64 << 10, 2048, ER_4K | ER_32K) },
@@ -991,6 +992,9 @@ static void decode_new_cmd(Flash *s, uint32_t value)
             s->pos = 0;
             s->len = 0;
             s->state = STATE_COLLECTING_DATA;
+            if (!strcmp(s->pi->part_name, "mx25l25635f")) {
+                s->needed_bytes = 2;
+            }
         }
         break;
     case RNVCR:
-- 
2.1.4

Re: [Qemu-devel] [PATCH 2/7] m25p80: add mx25l25635f chip

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

> -----Original Message-----
> From: Qemu-devel [mailto:qemu-devel-
> bounces+marcin.krzeminski=nokia.com@nongnu.org] On Behalf Of Cédric Le
> Goater
> Sent: Monday, July 04, 2016 2:19 PM
> To: Peter Maydell <peter.maydell@linaro.org>; Peter Crosthwaite
> <crosthwaite.peter@gmail.com>
> Cc: Andrew Jeffery <andrew@aj.id.au>; qemu-arm@nongnu.org; qemu-
> devel@nongnu.org; mar.krzeminski@gmail.com; Cédric Le Goater
> <clg@kaod.org>
> Subject: [Qemu-devel] [PATCH 2/7] m25p80: add mx25l25635f chip
> 
> The Macronix chip mx25l25635f used on some OpenPower boxes is very
> similar to the mx25l25635e. They share the same JEDEC identifier but the
> WRSR instruction requires 2 bytes in the mx25l25635f case.
> 
> To prevent some warnings on guests, let's introduce a new chip identifying
> JEDEC 0xc22019 as a MX25L25635F chip.
> 
Hello,

Adding fake JEDEC id is not good idea at all, we should somehow handle this in the code.

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/block/m25p80.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index
> d9b27939dde6..aa964280beab 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -199,6 +199,7 @@ static const FlashPartInfo known_devices[] = {
>      { INFO("mx25l12805d", 0xc22018,      0,  64 << 10, 256, 0) },
>      { INFO("mx25l12855e", 0xc22618,      0,  64 << 10, 256, 0) },
>      { INFO("mx25l25635e", 0xc22019,      0,  64 << 10, 512, 0) },
> +    { INFO("mx25l25635f", 0xc22019,      0,  64 << 10, 512, 0) },
>      { INFO("mx25l25655e", 0xc22619,      0,  64 << 10, 512, 0) },
>      { INFO("mx66u51235f", 0xc2253a,      0,  64 << 10, 1024, ER_4K | ER_32K) },
>      { INFO("mx66u1g45g",  0xc2253b,      0,  64 << 10, 2048, ER_4K | ER_32K) },
> @@ -991,6 +992,9 @@ static void decode_new_cmd(Flash *s, uint32_t
> value)
>              s->pos = 0;
>              s->len = 0;
>              s->state = STATE_COLLECTING_DATA;
> +            if (!strcmp(s->pi->part_name, "mx25l25635f")) {
> +                s->needed_bytes = 2;
> +            }
>          }
>          break;
Is it based on current master?
In my last series (and current master) this case should be handled by this code:
            case MAN_MACRONIX:
                s->needed_bytes = 2;
                s->state = STATE_COLLECTING_VAR_LEN_DATA;
                break;
I do not know what quest you are using, but linux mainline (at least 4.4.0) is sending only one byte,
in WRSR so this will not work.
Could you rebase to master and check if it will boot without your this change?

Thanks,
Marcin

>      case RNVCR:
> --
> 2.1.4
> 

Re: [Qemu-devel] [PATCH 2/7] m25p80: add mx25l25635f chip

[Cédric Le Goater]

On 07/04/2016 02:57 PM, Krzeminski, Marcin (Nokia - PL/Wroclaw) wrote:
> 
> 
>> -----Original Message-----
>> From: Qemu-devel [mailto:qemu-devel-
>> bounces+marcin.krzeminski=nokia.com@nongnu.org] On Behalf Of Cédric Le
>> Goater
>> Sent: Monday, July 04, 2016 2:19 PM
>> To: Peter Maydell <peter.maydell@linaro.org>; Peter Crosthwaite
>> <crosthwaite.peter@gmail.com>
>> Cc: Andrew Jeffery <andrew@aj.id.au>; qemu-arm@nongnu.org; qemu-
>> devel@nongnu.org; mar.krzeminski@gmail.com; Cédric Le Goater
>> <clg@kaod.org>
>> Subject: [Qemu-devel] [PATCH 2/7] m25p80: add mx25l25635f chip
>>
>> The Macronix chip mx25l25635f used on some OpenPower boxes is very
>> similar to the mx25l25635e. They share the same JEDEC identifier but the
>> WRSR instruction requires 2 bytes in the mx25l25635f case.
>>
>> To prevent some warnings on guests, let's introduce a new chip identifying
>> JEDEC 0xc22019 as a MX25L25635F chip.
>>
> Hello,
> 
> Adding fake JEDEC id is not good idea at all, we should somehow handle this in the code.

It is not a fake JEDEC id. It is a real one from the datasheet :)

mx25l25635f and mx25l25635e share the same 0xc22019 JEDEC. 

>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/block/m25p80.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index
>> d9b27939dde6..aa964280beab 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -199,6 +199,7 @@ static const FlashPartInfo known_devices[] = {
>>      { INFO("mx25l12805d", 0xc22018,      0,  64 << 10, 256, 0) },
>>      { INFO("mx25l12855e", 0xc22618,      0,  64 << 10, 256, 0) },
>>      { INFO("mx25l25635e", 0xc22019,      0,  64 << 10, 512, 0) },
>> +    { INFO("mx25l25635f", 0xc22019,      0,  64 << 10, 512, 0) },
>>      { INFO("mx25l25655e", 0xc22619,      0,  64 << 10, 512, 0) },
>>      { INFO("mx66u51235f", 0xc2253a,      0,  64 << 10, 1024, ER_4K | ER_32K) },
>>      { INFO("mx66u1g45g",  0xc2253b,      0,  64 << 10, 2048, ER_4K | ER_32K) },
>> @@ -991,6 +992,9 @@ static void decode_new_cmd(Flash *s, uint32_t
>> value)
>>              s->pos = 0;
>>              s->len = 0;
>>              s->state = STATE_COLLECTING_DATA;
>> +            if (!strcmp(s->pi->part_name, "mx25l25635f")) {
>> +                s->needed_bytes = 2;
>> +            }
>>          }
>>          break;
> Is it based on current master?

You are right. The patch has bit rotted, it still applies on qemu's HEAD
but at the wrong place :/ But, I don't think it is needed anymore, see 
below.

> In my last series (and current master) this case should be handled by this code:
>             case MAN_MACRONIX:
>                 s->needed_bytes = 2;
>                 s->state = STATE_COLLECTING_VAR_LEN_DATA;
>                 break;
> I do not know what quest you are using, but linux mainline (at least 4.4.0) is 
> sending only one byte, in WRSR so this will not work.

It is not Linux, it is a user space tool :

	https://github.com/open-power/skiboot/blob/master/hw/ast-bmc/ast-sf-ctrl.c#L465

> Could you rebase to master and check if it will boot without your this change?

So the current code is fine for the mx25l25635f, which takes 2 bytes 
for the WRSR. But, now, how would the m25p80 object behave with the 
mx25l25635e ? Only one byte will be written.

Should we deprecate mx25l25635e ? It is not used in qemu.

Thanks,

C. 

[Qemu-devel] Odp.: [PATCH 2/7] m25p80: add mx25l25635f chip

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

W dniu 04.07.2016 o 15:41, Cédric Le Goater pisze:
> On 07/04/2016 02:57 PM, Krzeminski, Marcin (Nokia - PL/Wroclaw) wrote:
>>
>>
>>> -----Original Message-----
>>> From: Qemu-devel [mailto:qemu-devel-
>>> bounces+marcin.krzeminski=nokia.com@nongnu.org] On Behalf Of Cédric Le
>>> Goater
>>> Sent: Monday, July 04, 2016 2:19 PM
>>> To: Peter Maydell <peter.maydell@linaro.org>; Peter Crosthwaite
>>> <crosthwaite.peter@gmail.com>
>>> Cc: Andrew Jeffery <andrew@aj.id.au>; qemu-arm@nongnu.org; qemu-
>>> devel@nongnu.org; mar.krzeminski@gmail.com; Cédric Le Goater
>>> <clg@kaod.org>
>>> Subject: [Qemu-devel] [PATCH 2/7] m25p80: add mx25l25635f chip
>>>
>>> The Macronix chip mx25l25635f used on some OpenPower boxes is very
>>> similar to the mx25l25635e. They share the same JEDEC identifier but the
>>> WRSR instruction requires 2 bytes in the mx25l25635f case.
>>>
>>> To prevent some warnings on guests, let's introduce a new chip identifying
>>> JEDEC 0xc22019 as a MX25L25635F chip.
>>>
>> Hello,
>>
>> Adding fake JEDEC id is not good idea at all, we should somehow handle this in the code.
>
> It is not a fake JEDEC id. It is a real one from the datasheet :)
>
> mx25l25635f and mx25l25635e share the same 0xc22019 JEDEC. 
Sorry, I misunderstand and haven't check in data-sheet :)
>
>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>  hw/block/m25p80.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index
>>> d9b27939dde6..aa964280beab 100644
>>> --- a/hw/block/m25p80.c
>>> +++ b/hw/block/m25p80.c
>>> @@ -199,6 +199,7 @@ static const FlashPartInfo known_devices[] = {
>>>      { INFO("mx25l12805d", 0xc22018,      0,  64 << 10, 256, 0) },
>>>      { INFO("mx25l12855e", 0xc22618,      0,  64 << 10, 256, 0) },
>>>      { INFO("mx25l25635e", 0xc22019,      0,  64 << 10, 512, 0) },
>>> +    { INFO("mx25l25635f", 0xc22019,      0,  64 << 10, 512, 0) },
>>>      { INFO("mx25l25655e", 0xc22619,      0,  64 << 10, 512, 0) },
>>>      { INFO("mx66u51235f", 0xc2253a,      0,  64 << 10, 1024, ER_4K | ER_32K) },
>>>      { INFO("mx66u1g45g",  0xc2253b,      0,  64 << 10, 2048, ER_4K | ER_32K) },
>>> @@ -991,6 +992,9 @@ static void decode_new_cmd(Flash *s, uint32_t
>>> value)
>>>              s->pos = 0;
>>>              s->len = 0;
>>>              s->state = STATE_COLLECTING_DATA;
>>> +            if (!strcmp(s->pi->part_name, "mx25l25635f")) {
>>> +                s->needed_bytes = 2;
>>> +            }
>>>          }
>>>          break;
>> Is it based on current master?
>
> You are right. The patch has bit rotted, it still applies on qemu's HEAD
> but at the wrong place :/ But, I don't think it is needed anymore, see 
> below.
>
>> In my last series (and current master) this case should be handled by this code:
>>             case MAN_MACRONIX:
>>                 s->needed_bytes = 2;
>>                 s->state = STATE_COLLECTING_VAR_LEN_DATA;
>>                 break;
>> I do not know what quest you are using, but linux mainline (at least 4.4.0) is 
>> sending only one byte, in WRSR so this will not work.
>
> It is not Linux, it is a user space tool :
>
> 	https://github.com/open-power/skiboot/blob/master/hw/ast-bmc/ast-sf-ctrl.c#L465
I meant spi-nor framework in Linux, if you are using different tool it is different story,
but should work for both (if real hw works for both...).
>
>
>> Could you rebase to master and check if it will boot without your this change?
>
> So the current code is fine for the mx25l25635f, which takes 2 bytes 
> for the WRSR. But, now, how would the m25p80 object behave with the 
> mx25l25635e ? Only one byte will be written.
It should behave properly (as in my case spi-nor framework in Linux writes only
one byte for Macronix in WSRS). COLLECTING_VAR_LEN_DATA state apply changes
(call complete_collecting_data) when CS go high. If it takes one byte instead of two,
one byte will be written to SR.
>
>
> Should we deprecate mx25l25635e ? It is not used in qemu.
That is good question, I do not see any flash with same JEDEC in the code, so
this one could be the first if you leave both of them.
You mentioned about warnings when you configure Qemu to use mx25l25635e,
instead of mx25l25635f, in my it should not matter, what are those warnings?

Regards,
Marcin
>
>
> Thanks,
>
> C. 
>

Re: [Qemu-devel] Odp.: [PATCH 2/7] m25p80: add mx25l25635f chip

[Cédric Le Goater]

On 07/04/2016 05:23 PM, Krzeminski, Marcin (Nokia - PL/Wroclaw) wrote:
> 
> 
> W dniu 04.07.2016 o 15:41, Cédric Le Goater pisze:
>> On 07/04/2016 02:57 PM, Krzeminski, Marcin (Nokia - PL/Wroclaw) wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Qemu-devel [mailto:qemu-devel-
>>>> bounces+marcin.krzeminski=nokia.com@nongnu.org] On Behalf Of Cédric Le
>>>> Goater
>>>> Sent: Monday, July 04, 2016 2:19 PM
>>>> To: Peter Maydell <peter.maydell@linaro.org>; Peter Crosthwaite
>>>> <crosthwaite.peter@gmail.com>
>>>> Cc: Andrew Jeffery <andrew@aj.id.au>; qemu-arm@nongnu.org; qemu-
>>>> devel@nongnu.org; mar.krzeminski@gmail.com; Cédric Le Goater
>>>> <clg@kaod.org>
>>>> Subject: [Qemu-devel] [PATCH 2/7] m25p80: add mx25l25635f chip
>>>>
>>>> The Macronix chip mx25l25635f used on some OpenPower boxes is very
>>>> similar to the mx25l25635e. They share the same JEDEC identifier but the
>>>> WRSR instruction requires 2 bytes in the mx25l25635f case.
>>>>
>>>> To prevent some warnings on guests, let's introduce a new chip identifying
>>>> JEDEC 0xc22019 as a MX25L25635F chip.
>>>>
>>> Hello,
>>>
>>> Adding fake JEDEC id is not good idea at all, we should somehow handle this in the code.
>>
>> It is not a fake JEDEC id. It is a real one from the datasheet :)
>>
>> mx25l25635f and mx25l25635e share the same 0xc22019 JEDEC. 
> Sorry, I misunderstand and haven't check in data-sheet :)

np.


>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>  hw/block/m25p80.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index
>>>> d9b27939dde6..aa964280beab 100644
>>>> --- a/hw/block/m25p80.c
>>>> +++ b/hw/block/m25p80.c
>>>> @@ -199,6 +199,7 @@ static const FlashPartInfo known_devices[] = {
>>>>      { INFO("mx25l12805d", 0xc22018,      0,  64 << 10, 256, 0) },
>>>>      { INFO("mx25l12855e", 0xc22618,      0,  64 << 10, 256, 0) },
>>>>      { INFO("mx25l25635e", 0xc22019,      0,  64 << 10, 512, 0) },
>>>> +    { INFO("mx25l25635f", 0xc22019,      0,  64 << 10, 512, 0) },
>>>>      { INFO("mx25l25655e", 0xc22619,      0,  64 << 10, 512, 0) },
>>>>      { INFO("mx66u51235f", 0xc2253a,      0,  64 << 10, 1024, ER_4K | ER_32K) },
>>>>      { INFO("mx66u1g45g",  0xc2253b,      0,  64 << 10, 2048, ER_4K | ER_32K) },
>>>> @@ -991,6 +992,9 @@ static void decode_new_cmd(Flash *s, uint32_t
>>>> value)
>>>>              s->pos = 0;
>>>>              s->len = 0;
>>>>              s->state = STATE_COLLECTING_DATA;
>>>> +            if (!strcmp(s->pi->part_name, "mx25l25635f")) {
>>>> +                s->needed_bytes = 2;
>>>> +            }
>>>>          }
>>>>          break;
>>> Is it based on current master?
>>
>> You are right. The patch has bit rotted, it still applies on qemu's HEAD
>> but at the wrong place :/ But, I don't think it is needed anymore, see 
>> below.
>>
>>> In my last series (and current master) this case should be handled by this code:
>>>             case MAN_MACRONIX:
>>>                 s->needed_bytes = 2;
>>>                 s->state = STATE_COLLECTING_VAR_LEN_DATA;
>>>                 break;
>>> I do not know what quest you are using, but linux mainline (at least 4.4.0) is 
>>> sending only one byte, in WRSR so this will not work.
>>
>> It is not Linux, it is a user space tool :
>>
>> 	https://github.com/open-power/skiboot/blob/master/hw/ast-bmc/ast-sf-ctrl.c#L465
> I meant spi-nor framework in Linux, if you are using different tool it is different story,
> but should work for both (if real hw works for both...).

Yes.  

Linux still does not know about the mx25l25635f and it is behaving 
fine using the mx25l25635e definition. We should send a patch for 
it I guess. I have the HW, I will look into that when I have some 
time.

>>> Could you rebase to master and check if it will boot without your this change?
>>
>> So the current code is fine for the mx25l25635f, which takes 2 bytes 
>> for the WRSR. But, now, how would the m25p80 object behave with the 
>> mx25l25635e ? Only one byte will be written.
>
> It should behave properly (as in my case spi-nor framework in Linux writes only
> one byte for Macronix in WSRS). COLLECTING_VAR_LEN_DATA state apply changes
> (call complete_collecting_data) when CS go high. If it takes one byte instead of two,
> one byte will be written to SR.

ah yes, this is true. so we are fine on that side.

>> Should we deprecate mx25l25635e ? It is not used in qemu.
>
> That is good question, I do not see any flash with same JEDEC in the code, so
> this one could be the first if you leave both of them.
> You mentioned about warnings when you configure Qemu to use mx25l25635e,
> instead of mx25l25635f, in my it should not matter, what are those warnings?

Before your patchset, we could get these:

        qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n", value);

you have solved that. 

So I guess we could just add : 

 +    { INFO("mx25l25635f", 0xc22019,      0,  64 << 10, 512, 0) },

but as we don't have any errors without them, I will just drop 
those oneliner patches. 

Thanks,

C.

[Qemu-devel] Odp.: Odp.: [PATCH 2/7] m25p80: add mx25l25635f chip

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

W dniu 04.07.2016 o 17:48, Cédric Le Goater pisze:
> On 07/04/2016 05:23 PM, Krzeminski, Marcin (Nokia - PL/Wroclaw) wrote:
>>
>>
>> W dniu 04.07.2016 o 15:41, Cédric Le Goater pisze:
>>> On 07/04/2016 02:57 PM, Krzeminski, Marcin (Nokia - PL/Wroclaw) wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Qemu-devel [mailto:qemu-devel-
>>>>> bounces+marcin.krzeminski=nokia.com@nongnu.org] On Behalf Of Cédric Le
>>>>> Goater
>>>>> Sent: Monday, July 04, 2016 2:19 PM
>>>>> To: Peter Maydell <peter.maydell@linaro.org>; Peter Crosthwaite
>>>>> <crosthwaite.peter@gmail.com>
>>>>> Cc: Andrew Jeffery <andrew@aj.id.au>; qemu-arm@nongnu.org; qemu-
>>>>> devel@nongnu.org; mar.krzeminski@gmail.com; Cédric Le Goater
>>>>> <clg@kaod.org>
>>>>> Subject: [Qemu-devel] [PATCH 2/7] m25p80: add mx25l25635f chip
>>>>>
>>>>> The Macronix chip mx25l25635f used on some OpenPower boxes is very
>>>>> similar to the mx25l25635e. They share the same JEDEC identifier but the
>>>>> WRSR instruction requires 2 bytes in the mx25l25635f case.
>>>>>
>>>>> To prevent some warnings on guests, let's introduce a new chip identifying
>>>>> JEDEC 0xc22019 as a MX25L25635F chip.
>>>>>
>>>> Hello,
>>>>
>>>> Adding fake JEDEC id is not good idea at all, we should somehow handle this in the code.
>>>
>>> It is not a fake JEDEC id. It is a real one from the datasheet :)
>>>
>>> mx25l25635f and mx25l25635e share the same 0xc22019 JEDEC. 
>> Sorry, I misunderstand and haven't check in data-sheet :)
>
> np.
>
>
>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>> ---
>>>>>  hw/block/m25p80.c | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index
>>>>> d9b27939dde6..aa964280beab 100644
>>>>> --- a/hw/block/m25p80.c
>>>>> +++ b/hw/block/m25p80.c
>>>>> @@ -199,6 +199,7 @@ static const FlashPartInfo known_devices[] = {
>>>>>      { INFO("mx25l12805d", 0xc22018,      0,  64 << 10, 256, 0) },
>>>>>      { INFO("mx25l12855e", 0xc22618,      0,  64 << 10, 256, 0) },
>>>>>      { INFO("mx25l25635e", 0xc22019,      0,  64 << 10, 512, 0) },
>>>>> +    { INFO("mx25l25635f", 0xc22019,      0,  64 << 10, 512, 0) },
>>>>>      { INFO("mx25l25655e", 0xc22619,      0,  64 << 10, 512, 0) },
>>>>>      { INFO("mx66u51235f", 0xc2253a,      0,  64 << 10, 1024, ER_4K | ER_32K) },
>>>>>      { INFO("mx66u1g45g",  0xc2253b,      0,  64 << 10, 2048, ER_4K | ER_32K) },
>>>>> @@ -991,6 +992,9 @@ static void decode_new_cmd(Flash *s, uint32_t
>>>>> value)
>>>>>              s->pos = 0;
>>>>>              s->len = 0;
>>>>>              s->state = STATE_COLLECTING_DATA;
>>>>> +            if (!strcmp(s->pi->part_name, "mx25l25635f")) {
>>>>> +                s->needed_bytes = 2;
>>>>> +            }
>>>>>          }
>>>>>          break;
>>>> Is it based on current master?
>>>
>>> You are right. The patch has bit rotted, it still applies on qemu's HEAD
>>> but at the wrong place :/ But, I don't think it is needed anymore, see 
>>> below.
>>>
>>>> In my last series (and current master) this case should be handled by this code:
>>>>             case MAN_MACRONIX:
>>>>                 s->needed_bytes = 2;
>>>>                 s->state = STATE_COLLECTING_VAR_LEN_DATA;
>>>>                 break;
>>>> I do not know what quest you are using, but linux mainline (at least 4.4.0) is 
>>>> sending only one byte, in WRSR so this will not work.
>>>
>>> It is not Linux, it is a user space tool :
>>>
>>> 	https://github.com/open-power/skiboot/blob/master/hw/ast-bmc/ast-sf-ctrl.c#L465
>> I meant spi-nor framework in Linux, if you are using different tool it is different story,
>> but should work for both (if real hw works for both...).
>
> Yes.  
>
> Linux still does not know about the mx25l25635f and it is behaving 
> fine using the mx25l25635e definition. We should send a patch for 
> it I guess. I have the HW, I will look into that when I have some 
> time.
Linux detects flash type by JEDEC code, so if the JEDEC is the same
it will be hard to patch this. They are working on detection by JEDEC216,
and then probably it will be possible to properly identify the flash.
>
>>>> Could you rebase to master and check if it will boot without your this change?
>>>
>>> So the current code is fine for the mx25l25635f, which takes 2 bytes 
>>> for the WRSR. But, now, how would the m25p80 object behave with the 
>>> mx25l25635e ? Only one byte will be written.
>>
>> It should behave properly (as in my case spi-nor framework in Linux writes only
>> one byte for Macronix in WSRS). COLLECTING_VAR_LEN_DATA state apply changes
>> (call complete_collecting_data) when CS go high. If it takes one byte instead of two,
>> one byte will be written to SR.
>
> ah yes, this is true. so we are fine on that side.
>
>>> Should we deprecate mx25l25635e ? It is not used in qemu.
>>
>> That is good question, I do not see any flash with same JEDEC in the code, so
>> this one could be the first if you leave both of them.
>> You mentioned about warnings when you configure Qemu to use mx25l25635e,
>> instead of mx25l25635f, in my it should not matter, what are those warnings?
>
> Before your patchset, we could get these:
>
>         qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n", value);
>
> you have solved that. 
>
> So I guess we could just add : 
>
>  +    { INFO("mx25l25635f", 0xc22019,      0,  64 << 10, 512, 0) },
>
> but as we don't have any errors without them, I will just drop 
> those oneliner patches. 
I think you should add this line, since this model is supported.

Thanks,
Marcin
>
>
> Thanks,
>
> C.
>

Re: [Qemu-devel] Odp.: Odp.: [PATCH 2/7] m25p80: add mx25l25635f chip

[Cédric Le Goater]

On 07/04/2016 06:03 PM, Krzeminski, Marcin (Nokia - PL/Wroclaw) wrote:
> 
> 
> W dniu 04.07.2016 o 17:48, Cédric Le Goater pisze:
>> On 07/04/2016 05:23 PM, Krzeminski, Marcin (Nokia - PL/Wroclaw) wrote:
>>>
>>>
>>> W dniu 04.07.2016 o 15:41, Cédric Le Goater pisze:
>>>> On 07/04/2016 02:57 PM, Krzeminski, Marcin (Nokia - PL/Wroclaw) wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Qemu-devel [mailto:qemu-devel-
>>>>>> bounces+marcin.krzeminski=nokia.com@nongnu.org] On Behalf Of Cédric Le
>>>>>> Goater
>>>>>> Sent: Monday, July 04, 2016 2:19 PM
>>>>>> To: Peter Maydell <peter.maydell@linaro.org>; Peter Crosthwaite
>>>>>> <crosthwaite.peter@gmail.com>
>>>>>> Cc: Andrew Jeffery <andrew@aj.id.au>; qemu-arm@nongnu.org; qemu-
>>>>>> devel@nongnu.org; mar.krzeminski@gmail.com; Cédric Le Goater
>>>>>> <clg@kaod.org>
>>>>>> Subject: [Qemu-devel] [PATCH 2/7] m25p80: add mx25l25635f chip
>>>>>>
>>>>>> The Macronix chip mx25l25635f used on some OpenPower boxes is very
>>>>>> similar to the mx25l25635e. They share the same JEDEC identifier but the
>>>>>> WRSR instruction requires 2 bytes in the mx25l25635f case.
>>>>>>
>>>>>> To prevent some warnings on guests, let's introduce a new chip identifying
>>>>>> JEDEC 0xc22019 as a MX25L25635F chip.
>>>>>>
>>>>> Hello,
>>>>>
>>>>> Adding fake JEDEC id is not good idea at all, we should somehow handle this in the code.
>>>>
>>>> It is not a fake JEDEC id. It is a real one from the datasheet :)
>>>>
>>>> mx25l25635f and mx25l25635e share the same 0xc22019 JEDEC. 
>>> Sorry, I misunderstand and haven't check in data-sheet :)
>>
>> np.
>>
>>
>>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>>> ---
>>>>>>  hw/block/m25p80.c | 4 ++++
>>>>>>  1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index
>>>>>> d9b27939dde6..aa964280beab 100644
>>>>>> --- a/hw/block/m25p80.c
>>>>>> +++ b/hw/block/m25p80.c
>>>>>> @@ -199,6 +199,7 @@ static const FlashPartInfo known_devices[] = {
>>>>>>      { INFO("mx25l12805d", 0xc22018,      0,  64 << 10, 256, 0) },
>>>>>>      { INFO("mx25l12855e", 0xc22618,      0,  64 << 10, 256, 0) },
>>>>>>      { INFO("mx25l25635e", 0xc22019,      0,  64 << 10, 512, 0) },
>>>>>> +    { INFO("mx25l25635f", 0xc22019,      0,  64 << 10, 512, 0) },
>>>>>>      { INFO("mx25l25655e", 0xc22619,      0,  64 << 10, 512, 0) },
>>>>>>      { INFO("mx66u51235f", 0xc2253a,      0,  64 << 10, 1024, ER_4K | ER_32K) },
>>>>>>      { INFO("mx66u1g45g",  0xc2253b,      0,  64 << 10, 2048, ER_4K | ER_32K) },
>>>>>> @@ -991,6 +992,9 @@ static void decode_new_cmd(Flash *s, uint32_t
>>>>>> value)
>>>>>>              s->pos = 0;
>>>>>>              s->len = 0;
>>>>>>              s->state = STATE_COLLECTING_DATA;
>>>>>> +            if (!strcmp(s->pi->part_name, "mx25l25635f")) {
>>>>>> +                s->needed_bytes = 2;
>>>>>> +            }
>>>>>>          }
>>>>>>          break;
>>>>> Is it based on current master?
>>>>
>>>> You are right. The patch has bit rotted, it still applies on qemu's HEAD
>>>> but at the wrong place :/ But, I don't think it is needed anymore, see 
>>>> below.
>>>>
>>>>> In my last series (and current master) this case should be handled by this code:
>>>>>             case MAN_MACRONIX:
>>>>>                 s->needed_bytes = 2;
>>>>>                 s->state = STATE_COLLECTING_VAR_LEN_DATA;
>>>>>                 break;
>>>>> I do not know what quest you are using, but linux mainline (at least 4.4.0) is 
>>>>> sending only one byte, in WRSR so this will not work.
>>>>
>>>> It is not Linux, it is a user space tool :
>>>>
>>>> 	https://github.com/open-power/skiboot/blob/master/hw/ast-bmc/ast-sf-ctrl.c#L465
>>> I meant spi-nor framework in Linux, if you are using different tool it is different story,
>>> but should work for both (if real hw works for both...).
>>
>> Yes.  
>>
>> Linux still does not know about the mx25l25635f and it is behaving 
>> fine using the mx25l25635e definition. We should send a patch for 
>> it I guess. I have the HW, I will look into that when I have some 
>> time.
> Linux detects flash type by JEDEC code, so if the JEDEC is the same
> it will be hard to patch this. They are working on detection by JEDEC216,
> and then probably it will be possible to properly identify the flash.
>
>>>>> Could you rebase to master and check if it will boot without your this change?
>>>>
>>>> So the current code is fine for the mx25l25635f, which takes 2 bytes 
>>>> for the WRSR. But, now, how would the m25p80 object behave with the 
>>>> mx25l25635e ? Only one byte will be written.
>>>
>>> It should behave properly (as in my case spi-nor framework in Linux writes only
>>> one byte for Macronix in WSRS). COLLECTING_VAR_LEN_DATA state apply changes
>>> (call complete_collecting_data) when CS go high. If it takes one byte instead of two,
>>> one byte will be written to SR.
>>
>> ah yes, this is true. so we are fine on that side.
>>
>>>> Should we deprecate mx25l25635e ? It is not used in qemu.
>>>
>>> That is good question, I do not see any flash with same JEDEC in the code, so
>>> this one could be the first if you leave both of them.
>>> You mentioned about warnings when you configure Qemu to use mx25l25635e,
>>> instead of mx25l25635f, in my it should not matter, what are those warnings?
>>
>> Before your patchset, we could get these:
>>
>>         qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n", value);
>>
>> you have solved that. 
>>
>> So I guess we could just add : 
>>
>>  +    { INFO("mx25l25635f", 0xc22019,      0,  64 << 10, 512, 0) },
>>
>> but as we don't have any errors without them, I will just drop 
>> those oneliner patches. 
>
> I think you should add this line, since this model is supported.

OK. Next round then.

Thanks,

C. 
 
> Thanks,
> Marcin
>>
>>
>> Thanks,
>>
>> C.
>>
> 

[Qemu-devel] [PATCH 3/7] ast2400: use a mx25l25635f chip

[Cédric Le Goater]

The pflash command on OpenBmc is used to update the flash and it
expects a mx25l25635f and not a mx25l25635e.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/palmetto-bmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
index 54e29a865d88..c0bb922f6406 100644
--- a/hw/arm/palmetto-bmc.c
+++ b/hw/arm/palmetto-bmc.c
@@ -78,7 +78,7 @@ static void palmetto_bmc_init(MachineState *machine)
                              &error_abort);
 
     palmetto_bmc_init_flashes(&bmc->soc.smc, "n25q256a", &error_abort);
-    palmetto_bmc_init_flashes(&bmc->soc.spi, "mx25l25635e", &error_abort);
+    palmetto_bmc_init_flashes(&bmc->soc.spi, "mx25l25635f", &error_abort);
 
     palmetto_bmc_binfo.kernel_filename = machine->kernel_filename;
     palmetto_bmc_binfo.initrd_filename = machine->initrd_filename;
-- 
2.1.4

[Qemu-devel] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[Cédric Le Goater]

Some SPI controllers, such as the Aspeed AST2400, have a mode in which
accesses to the flash content are no different than doing MMIOs. The
controller generates all the necessary commands to load (or store)
data in memory.

To emulate such a behavior, we need to have access to the underlying
storage of the flash object. The purpose of the routine is to set the
storage of a preinitialized SPI flash object, which can then be used
by the object itself but also by a SPI controller to handled the
MMIOs.

This is sufficient to support read only accesses. For writes, we would
certainly need to externalize flash_write8() routine.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/block/m25p80.c        | 14 +++++++++++++-
 include/hw/block/flash.h |  2 ++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index aa964280beab..fec0fd4c2228 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -29,6 +29,7 @@
 #include "qemu/bitops.h"
 #include "qemu/log.h"
 #include "qapi/error.h"
+#include "hw/block/flash.h"
 
 #ifndef M25P80_ERR_DEBUG
 #define M25P80_ERR_DEBUG 0
@@ -1152,7 +1153,11 @@ static void m25p80_realize(SSISlave *ss, Error **errp)
 
     if (s->blk) {
         DB_PRINT_L(0, "Binding to IF_MTD drive\n");
-        s->storage = blk_blockalign(s->blk, s->size);
+
+        /* using an external storage. see m25p80_create_rom() */
+        if (!s->storage) {
+            s->storage = blk_blockalign(s->blk, s->size);
+        }
 
         if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) {
             error_setg(errp, "failed to read the initial flash content");
@@ -1259,3 +1264,10 @@ static void m25p80_register_types(void)
 }
 
 type_init(m25p80_register_types)
+
+void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom)
+{
+    Flash *s = M25P80(dev);
+
+    s->storage = memory_region_get_ram_ptr(rom);
+}
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 50ccbbcf1352..9d780f4ae0c9 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -61,4 +61,6 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample);
 void ecc_reset(ECCState *s);
 extern VMStateDescription vmstate_ecc_state;
 
+void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom);
+
 #endif
-- 
2.1.4

Re: [Qemu-devel] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[mar.krzeminski]

W dniu 04.07.2016 o 14:18, Cédric Le Goater pisze:
> Some SPI controllers, such as the Aspeed AST2400, have a mode in which
> accesses to the flash content are no different than doing MMIOs. The
> controller generates all the necessary commands to load (or store)
> data in memory.
>
> To emulate such a behavior, we need to have access to the underlying
> storage of the flash object. The purpose of the routine is to set the
> storage of a preinitialized SPI flash object, which can then be used
> by the object itself but also by a SPI controller to handled the
> MMIOs.
Hi,

I am not sure if this approach is correct. I can not find any datasheet
to this SPI controller, but as you mentioned in first paragraph, controller
generates all commands (probably ones are somehow configurable).
In this series you hack this behaviour and you do direct access to file.
IMHO you should emulate sending such commands in SPI controller
model.

Thanks,
Marcin

> This is sufficient to support read only accesses. For writes, we would
> certainly need to externalize flash_write8() routine.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/block/m25p80.c        | 14 +++++++++++++-
>   include/hw/block/flash.h |  2 ++
>   2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index aa964280beab..fec0fd4c2228 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -29,6 +29,7 @@
>   #include "qemu/bitops.h"
>   #include "qemu/log.h"
>   #include "qapi/error.h"
> +#include "hw/block/flash.h"
>   
>   #ifndef M25P80_ERR_DEBUG
>   #define M25P80_ERR_DEBUG 0
> @@ -1152,7 +1153,11 @@ static void m25p80_realize(SSISlave *ss, Error **errp)
>   
>       if (s->blk) {
>           DB_PRINT_L(0, "Binding to IF_MTD drive\n");
> -        s->storage = blk_blockalign(s->blk, s->size);
> +
> +        /* using an external storage. see m25p80_create_rom() */
> +        if (!s->storage) {
> +            s->storage = blk_blockalign(s->blk, s->size);
> +        }
>   
>           if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) {
>               error_setg(errp, "failed to read the initial flash content");
> @@ -1259,3 +1264,10 @@ static void m25p80_register_types(void)
>   }
>   
>   type_init(m25p80_register_types)
> +
> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom)
> +{
> +    Flash *s = M25P80(dev);
> +
> +    s->storage = memory_region_get_ram_ptr(rom);
> +}
> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
> index 50ccbbcf1352..9d780f4ae0c9 100644
> --- a/include/hw/block/flash.h
> +++ b/include/hw/block/flash.h
> @@ -61,4 +61,6 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample);
>   void ecc_reset(ECCState *s);
>   extern VMStateDescription vmstate_ecc_state;
>   
> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom);
> +
>   #endif

Re: [Qemu-devel] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[Cédric Le Goater]

On 07/04/2016 07:57 PM, mar.krzeminski wrote:
> 
> 
> W dniu 04.07.2016 o 14:18, Cédric Le Goater pisze:
>> Some SPI controllers, such as the Aspeed AST2400, have a mode in which
>> accesses to the flash content are no different than doing MMIOs. The
>> controller generates all the necessary commands to load (or store)
>> data in memory.
>>
>> To emulate such a behavior, we need to have access to the underlying
>> storage of the flash object. The purpose of the routine is to set the
>> storage of a preinitialized SPI flash object, which can then be used
>> by the object itself but also by a SPI controller to handled the
>> MMIOs.
> Hi,
> 
> I am not sure if this approach is correct. I can not find any datasheet
> to this SPI controller, but as you mentioned in first paragraph, controller
> generates all commands (probably ones are somehow configurable).

yes. see this patch :

	http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg08229.html

	/* CEx Control Register */
	#define R_CTRL0           (0x10 / 4)
	#define   CTRL_CMD_SHIFT           16
	#define   CTRL_CMD_MASK            0xff


> In this series you hack this behaviour and you do direct access to file.

It is true it is not very respectful of the m25p80 interface.

> IMHO you should emulate sending such commands in SPI controller
> model.

I will give it a try. I don't think the alternative is a complex 
change anyhow.

Thanks,

C.

> Thanks,
> Marcin
> 
>> This is sufficient to support read only accesses. For writes, we would
>> certainly need to externalize flash_write8() routine.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/block/m25p80.c        | 14 +++++++++++++-
>>   include/hw/block/flash.h |  2 ++
>>   2 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index aa964280beab..fec0fd4c2228 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -29,6 +29,7 @@
>>   #include "qemu/bitops.h"
>>   #include "qemu/log.h"
>>   #include "qapi/error.h"
>> +#include "hw/block/flash.h"
>>     #ifndef M25P80_ERR_DEBUG
>>   #define M25P80_ERR_DEBUG 0
>> @@ -1152,7 +1153,11 @@ static void m25p80_realize(SSISlave *ss, Error **errp)
>>         if (s->blk) {
>>           DB_PRINT_L(0, "Binding to IF_MTD drive\n");
>> -        s->storage = blk_blockalign(s->blk, s->size);
>> +
>> +        /* using an external storage. see m25p80_create_rom() */
>> +        if (!s->storage) {
>> +            s->storage = blk_blockalign(s->blk, s->size);
>> +        }
>>             if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) {
>>               error_setg(errp, "failed to read the initial flash content");
>> @@ -1259,3 +1264,10 @@ static void m25p80_register_types(void)
>>   }
>>     type_init(m25p80_register_types)
>> +
>> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom)
>> +{
>> +    Flash *s = M25P80(dev);
>> +
>> +    s->storage = memory_region_get_ram_ptr(rom);
>> +}
>> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
>> index 50ccbbcf1352..9d780f4ae0c9 100644
>> --- a/include/hw/block/flash.h
>> +++ b/include/hw/block/flash.h
>> @@ -61,4 +61,6 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample);
>>   void ecc_reset(ECCState *s);
>>   extern VMStateDescription vmstate_ecc_state;
>>   +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom);
>> +
>>   #endif
> 

Re: [Qemu-devel] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[mar.krzeminski]

W dniu 04.07.2016 o 20:12, Cédric Le Goater pisze:
> On 07/04/2016 07:57 PM, mar.krzeminski wrote:
>>
>> W dniu 04.07.2016 o 14:18, Cédric Le Goater pisze:
>>> Some SPI controllers, such as the Aspeed AST2400, have a mode in which
>>> accesses to the flash content are no different than doing MMIOs. The
>>> controller generates all the necessary commands to load (or store)
>>> data in memory.
>>>
>>> To emulate such a behavior, we need to have access to the underlying
>>> storage of the flash object. The purpose of the routine is to set the
>>> storage of a preinitialized SPI flash object, which can then be used
>>> by the object itself but also by a SPI controller to handled the
>>> MMIOs.
>> Hi,
>>
>> I am not sure if this approach is correct. I can not find any datasheet
>> to this SPI controller, but as you mentioned in first paragraph, controller
>> generates all commands (probably ones are somehow configurable).
> yes. see this patch :
>
> 	http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg08229.html
>
> 	/* CEx Control Register */
> 	#define R_CTRL0           (0x10 / 4)
> 	#define   CTRL_CMD_SHIFT           16
> 	#define   CTRL_CMD_MASK            0xff
>
>
>> In this series you hack this behaviour and you do direct access to file.
> It is true it is not very respectful of the m25p80 interface.
>
>> IMHO you should emulate sending such commands in SPI controller
>> model.
> I will give it a try. I don't think the alternative is a complex
> change anyhow.
Register logic + few commands, then write is almost copy-paste.
Shouldn’t be complicated, but you have real HW modelling.

Thanks,
Marcin
> Thanks,
>
> C.
>
>> Thanks,
>> Marcin
>>
>>> This is sufficient to support read only accesses. For writes, we would
>>> certainly need to externalize flash_write8() routine.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>    hw/block/m25p80.c        | 14 +++++++++++++-
>>>    include/hw/block/flash.h |  2 ++
>>>    2 files changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>>> index aa964280beab..fec0fd4c2228 100644
>>> --- a/hw/block/m25p80.c
>>> +++ b/hw/block/m25p80.c
>>> @@ -29,6 +29,7 @@
>>>    #include "qemu/bitops.h"
>>>    #include "qemu/log.h"
>>>    #include "qapi/error.h"
>>> +#include "hw/block/flash.h"
>>>      #ifndef M25P80_ERR_DEBUG
>>>    #define M25P80_ERR_DEBUG 0
>>> @@ -1152,7 +1153,11 @@ static void m25p80_realize(SSISlave *ss, Error **errp)
>>>          if (s->blk) {
>>>            DB_PRINT_L(0, "Binding to IF_MTD drive\n");
>>> -        s->storage = blk_blockalign(s->blk, s->size);
>>> +
>>> +        /* using an external storage. see m25p80_create_rom() */
>>> +        if (!s->storage) {
>>> +            s->storage = blk_blockalign(s->blk, s->size);
>>> +        }
>>>              if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) {
>>>                error_setg(errp, "failed to read the initial flash content");
>>> @@ -1259,3 +1264,10 @@ static void m25p80_register_types(void)
>>>    }
>>>      type_init(m25p80_register_types)
>>> +
>>> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom)
>>> +{
>>> +    Flash *s = M25P80(dev);
>>> +
>>> +    s->storage = memory_region_get_ram_ptr(rom);
>>> +}
>>> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
>>> index 50ccbbcf1352..9d780f4ae0c9 100644
>>> --- a/include/hw/block/flash.h
>>> +++ b/include/hw/block/flash.h
>>> @@ -61,4 +61,6 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample);
>>>    void ecc_reset(ECCState *s);
>>>    extern VMStateDescription vmstate_ecc_state;
>>>    +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom);
>>> +
>>>    #endif
>

Re: [Qemu-devel] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[Cédric Le Goater]

On 07/04/2016 08:12 PM, Cédric Le Goater wrote:
> On 07/04/2016 07:57 PM, mar.krzeminski wrote:
>>
>>
>> W dniu 04.07.2016 o 14:18, Cédric Le Goater pisze:
>>> Some SPI controllers, such as the Aspeed AST2400, have a mode in which
>>> accesses to the flash content are no different than doing MMIOs. The
>>> controller generates all the necessary commands to load (or store)
>>> data in memory.
>>>
>>> To emulate such a behavior, we need to have access to the underlying
>>> storage of the flash object. The purpose of the routine is to set the
>>> storage of a preinitialized SPI flash object, which can then be used
>>> by the object itself but also by a SPI controller to handled the
>>> MMIOs.
>> Hi,
>>
>> I am not sure if this approach is correct. I can not find any datasheet
>> to this SPI controller, but as you mentioned in first paragraph, controller
>> generates all commands (probably ones are somehow configurable).
> 
> yes. see this patch :
> 
> 	http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg08229.html
> 
> 	/* CEx Control Register */
> 	#define R_CTRL0           (0x10 / 4)
> 	#define   CTRL_CMD_SHIFT           16
> 	#define   CTRL_CMD_MASK            0xff
> 
> 
>> In this series you hack this behaviour and you do direct access to file.
> 
> It is true it is not very respectful of the m25p80 interface.
> 
>> IMHO you should emulate sending such commands in SPI controller
>> model.
> 
> I will give it a try. I don't think the alternative is a complex 
> change anyhow.

So yes, that would work out pretty well. But there is another need 
that does not show up in the series (I am not splitting the patches 
correctly I guess) We would like to boot directly from a flash image. 
For this purpose, the pflash_cfi object uses a memory region of type 
rom and uses the storage behind the region.

m25p80_set_rom_storage() is probably not the right API to share the 
storage. I am looking for a way to handle this without changing too
much m25p80, which does not have a mem region currently. Suggestions 
welcomed ! :) 

Thanks,

C. 

Re: [Qemu-devel] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[mar.krzeminski]

W dniu 06.07.2016 o 18:30, Cédric Le Goater pisze:
> On 07/04/2016 08:12 PM, Cédric Le Goater wrote:
>> On 07/04/2016 07:57 PM, mar.krzeminski wrote:
>>>
>>> W dniu 04.07.2016 o 14:18, Cédric Le Goater pisze:
>>>> Some SPI controllers, such as the Aspeed AST2400, have a mode in which
>>>> accesses to the flash content are no different than doing MMIOs. The
>>>> controller generates all the necessary commands to load (or store)
>>>> data in memory.
>>>>
>>>> To emulate such a behavior, we need to have access to the underlying
>>>> storage of the flash object. The purpose of the routine is to set the
>>>> storage of a preinitialized SPI flash object, which can then be used
>>>> by the object itself but also by a SPI controller to handled the
>>>> MMIOs.
>>> Hi,
>>>
>>> I am not sure if this approach is correct. I can not find any datasheet
>>> to this SPI controller, but as you mentioned in first paragraph, controller
>>> generates all commands (probably ones are somehow configurable).
>> yes. see this patch :
>>
>> 	http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg08229.html
>>
>> 	/* CEx Control Register */
>> 	#define R_CTRL0           (0x10 / 4)
>> 	#define   CTRL_CMD_SHIFT           16
>> 	#define   CTRL_CMD_MASK            0xff
>>
>>
>>> In this series you hack this behaviour and you do direct access to file.
>> It is true it is not very respectful of the m25p80 interface.
>>
>>> IMHO you should emulate sending such commands in SPI controller
>>> model.
>> I will give it a try. I don't think the alternative is a complex
>> change anyhow.
> So yes, that would work out pretty well. But there is another need
> that does not show up in the series (I am not splitting the patches
> correctly I guess) We would like to boot directly from a flash image.
> For this purpose, the pflash_cfi object uses a memory region of type
> rom and uses the storage behind the region.
>
> m25p80_set_rom_storage() is probably not the right API to share the
> storage. I am looking for a way to handle this without changing too
> much m25p80, which does not have a mem region currently. Suggestions
> welcomed ! :)
I do not know if this is a good idea - for sure yet another complication,
but we still should process like the HW. The flash is memory mapped
to some memory area, so maybe creating alias to this memory area
would help with the implementation?
Then read will be still done by SPI and m25p80. Some configuration
from the board level might be needed to initialise the controller
(or maybe defaults are ok - depends how it is done in hw).

Regards,
Marcin
> Thanks,
>
> C.
>
>

Re: [Qemu-devel] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[Peter Crosthwaite]

On Mon, Jul 4, 2016 at 10:57 AM, mar.krzeminski
<mar.krzeminski@gmail.com> wrote:
>
>
> W dniu 04.07.2016 o 14:18, Cédric Le Goater pisze:
>>
>> Some SPI controllers, such as the Aspeed AST2400, have a mode in which
>> accesses to the flash content are no different than doing MMIOs. The
>> controller generates all the necessary commands to load (or store)
>> data in memory.
>>
>> To emulate such a behavior, we need to have access to the underlying
>> storage of the flash object. The purpose of the routine is to set the
>> storage of a preinitialized SPI flash object, which can then be used
>> by the object itself but also by a SPI controller to handled the
>> MMIOs.
>
> Hi,
>
> I am not sure if this approach is correct. I can not find any datasheet
> to this SPI controller, but as you mentioned in first paragraph, controller
> generates all commands (probably ones are somehow configurable).
> In this series you hack this behaviour and you do direct access to file.
> IMHO you should emulate sending such commands in SPI controller
> model.
>

Actually I think this is a good approach for two reasons.

Firstly there is more than one SPI controller that does this, the
Xilinx Zynq QSPI controller does this for its LQSPI mode. See
lqspi_read() in hw/ssi/xilinx_spips.c, which already works the way
Marcin proposes. That one is semi-automatic, in that there are
software configurable registers that hold the actual command to do the
read etc. But once setup, the interface is linear-read-only and
software transparent. A lot of assumptions where made in writing that
code (the buffering scheme is completely made up) and I think the
direct approach might be cleaner. This approach can go beyond SPI, in
that it can work for other protocols and external storage devices.

The more interesting application of this approach though, is emulating
boot ROMs. Multiple SoCs in tree have pty bootroms that read SD cards
(or even SPI), mount file-systems and then blob+boot images. Rather
than emulators having to talk SDHCI through the controller to get
images, we should be able to go direct, and implement the boot-logic
off the raw block device. This means that there should be a general
approach to a machine fishing the backing block-dev out from an
external storage device, and linear SPI is another good application if
it.

Regards,
Peter

> Thanks,
> Marcin
>
>
>> This is sufficient to support read only accesses. For writes, we would
>> certainly need to externalize flash_write8() routine.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/block/m25p80.c        | 14 +++++++++++++-
>>   include/hw/block/flash.h |  2 ++
>>   2 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index aa964280beab..fec0fd4c2228 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -29,6 +29,7 @@
>>   #include "qemu/bitops.h"
>>   #include "qemu/log.h"
>>   #include "qapi/error.h"
>> +#include "hw/block/flash.h"
>>     #ifndef M25P80_ERR_DEBUG
>>   #define M25P80_ERR_DEBUG 0
>> @@ -1152,7 +1153,11 @@ static void m25p80_realize(SSISlave *ss, Error
>> **errp)
>>         if (s->blk) {
>>           DB_PRINT_L(0, "Binding to IF_MTD drive\n");
>> -        s->storage = blk_blockalign(s->blk, s->size);
>> +
>> +        /* using an external storage. see m25p80_create_rom() */
>> +        if (!s->storage) {
>> +            s->storage = blk_blockalign(s->blk, s->size);
>> +        }
>>             if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) {
>>               error_setg(errp, "failed to read the initial flash
>> content");
>> @@ -1259,3 +1264,10 @@ static void m25p80_register_types(void)
>>   }
>>     type_init(m25p80_register_types)
>> +
>> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom)
>> +{
>> +    Flash *s = M25P80(dev);
>> +
>> +    s->storage = memory_region_get_ram_ptr(rom);
>> +}
>> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
>> index 50ccbbcf1352..9d780f4ae0c9 100644
>> --- a/include/hw/block/flash.h
>> +++ b/include/hw/block/flash.h
>> @@ -61,4 +61,6 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample);
>>   void ecc_reset(ECCState *s);
>>   extern VMStateDescription vmstate_ecc_state;
>>   +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom);
>> +
>>   #endif
>
>

Re: [Qemu-devel] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[Cédric Le Goater]

Hello,

On 07/10/2016 01:38 AM, Peter Crosthwaite wrote:
> On Mon, Jul 4, 2016 at 10:57 AM, mar.krzeminski
> <mar.krzeminski@gmail.com> wrote:
>>
>>
>> W dniu 04.07.2016 o 14:18, Cédric Le Goater pisze:
>>>
>>> Some SPI controllers, such as the Aspeed AST2400, have a mode in which
>>> accesses to the flash content are no different than doing MMIOs. The
>>> controller generates all the necessary commands to load (or store)
>>> data in memory.
>>>
>>> To emulate such a behavior, we need to have access to the underlying
>>> storage of the flash object. The purpose of the routine is to set the
>>> storage of a preinitialized SPI flash object, which can then be used
>>> by the object itself but also by a SPI controller to handled the
>>> MMIOs.
>>
>> Hi,
>>
>> I am not sure if this approach is correct. I can not find any datasheet
>> to this SPI controller, but as you mentioned in first paragraph, controller
>> generates all commands (probably ones are somehow configurable).
>> In this series you hack this behaviour and you do direct access to file.
>> IMHO you should emulate sending such commands in SPI controller
>> model.
>>
> 
> Actually I think this is a good approach for two reasons.
> 
> Firstly there is more than one SPI controller that does this, the
> Xilinx Zynq QSPI controller does this for its LQSPI mode. See
> lqspi_read() in hw/ssi/xilinx_spips.c, which already works the way
> Marcin proposes. That one is semi-automatic, in that there are
> software configurable registers that hold the actual command to do the
> read etc. But once setup, the interface is linear-read-only and
> software transparent. A lot of assumptions where made in writing that
> code (the buffering scheme is completely made up) and I think the
> direct approach might be cleaner. This approach can go beyond SPI, in
> that it can work for other protocols and external storage devices.

ok.


The Aspeed SPI controller could follow the SPI approach and initiate 
SPI tranfers instead of accessing the underlying storage of the flash 
module. This is not strictly necessary for that case. 

But, being able to use the SPI flash module as a boot ROM (as you point
out below)  brings in extra needs. A rom memory region is required for 
that as you need to share the storage with the flash object. Sharing 
gives you direct access to the storage and then, generating SPI transfers 
becomes a bit superfluous. 

I tried a few other approaches, adding aliases, adding a region without
ops behind the flash object but they were not very convincing. 


> The more interesting application of this approach though, is emulating
> boot ROMs. Multiple SoCs in tree have pty bootroms that read SD cards
> (or even SPI), mount file-systems and then blob+boot images. Rather
> than emulators having to talk SDHCI through the controller to get
> images, we should be able to go direct, and implement the boot-logic
> off the raw block device. This means that there should be a general
> approach to a machine fishing the backing block-dev out from an
> external storage device, and linear SPI is another good application if
> it.


So if I read you well, the m25p80_set_rom_storage() approach is not the 
most hideous thing to do but it needs to be generalized.

Currently, it boils down to :

 * create a rom memory region for each flash module needing one :

        memory_region_init_rom_device(&fl->mmio, OBJECT(s), &flash_ops,
                                      fl, name, fl->size, &err);

 * if needed, storage can be saved for later usage with :

        flash->storage = memory_region_get_ram_ptr(&fl->mmio);


 * the memory region storage should be passed on to the flash object 
   using m25p80_set_rom_storage() before realization of the object :

        fl->flash = ssi_create_slave_no_init(s->spi, flashtype);
        if (dinfo) {
            qdev_prop_set_drive(fl->flash, "drive", blk_by_legacy_dinfo(dinfo),
                                errp);
        }
        m25p80_set_rom_storage(fl->flash, &fl->mmio);
        qdev_init_nofail(fl->flash);


This is very m25p80 centric. A common solution could be an object 
gathering the properties of a memory region rom device and a blk 
device. This is a bit what the pflash_cfi* objects are proposing 
but it is all bundled in a big specific object.


Thanks,

C. 

> Regards,
> Peter
> 
>> Thanks,
>> Marcin
>>
>>
>>> This is sufficient to support read only accesses. For writes, we would
>>> certainly need to externalize flash_write8() routine.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>   hw/block/m25p80.c        | 14 +++++++++++++-
>>>   include/hw/block/flash.h |  2 ++
>>>   2 files changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>>> index aa964280beab..fec0fd4c2228 100644
>>> --- a/hw/block/m25p80.c
>>> +++ b/hw/block/m25p80.c
>>> @@ -29,6 +29,7 @@
>>>   #include "qemu/bitops.h"
>>>   #include "qemu/log.h"
>>>   #include "qapi/error.h"
>>> +#include "hw/block/flash.h"
>>>     #ifndef M25P80_ERR_DEBUG
>>>   #define M25P80_ERR_DEBUG 0
>>> @@ -1152,7 +1153,11 @@ static void m25p80_realize(SSISlave *ss, Error
>>> **errp)
>>>         if (s->blk) {
>>>           DB_PRINT_L(0, "Binding to IF_MTD drive\n");
>>> -        s->storage = blk_blockalign(s->blk, s->size);
>>> +
>>> +        /* using an external storage. see m25p80_create_rom() */
>>> +        if (!s->storage) {
>>> +            s->storage = blk_blockalign(s->blk, s->size);
>>> +        }
>>>             if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) {
>>>               error_setg(errp, "failed to read the initial flash
>>> content");
>>> @@ -1259,3 +1264,10 @@ static void m25p80_register_types(void)
>>>   }
>>>     type_init(m25p80_register_types)
>>> +
>>> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom)
>>> +{
>>> +    Flash *s = M25P80(dev);
>>> +
>>> +    s->storage = memory_region_get_ram_ptr(rom);
>>> +}
>>> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
>>> index 50ccbbcf1352..9d780f4ae0c9 100644
>>> --- a/include/hw/block/flash.h
>>> +++ b/include/hw/block/flash.h
>>> @@ -61,4 +61,6 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample);
>>>   void ecc_reset(ECCState *s);
>>>   extern VMStateDescription vmstate_ecc_state;
>>>   +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom);
>>> +
>>>   #endif
>>
>>

Re: [Qemu-devel] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[Cédric Le Goater]

Hello,

On 07/04/2016 07:57 PM, mar.krzeminski wrote:
> 
> 
> W dniu 04.07.2016 o 14:18, Cédric Le Goater pisze:
>> Some SPI controllers, such as the Aspeed AST2400, have a mode in which
>> accesses to the flash content are no different than doing MMIOs. The
>> controller generates all the necessary commands to load (or store)
>> data in memory.
>>
>> To emulate such a behavior, we need to have access to the underlying
>> storage of the flash object. The purpose of the routine is to set the
>> storage of a preinitialized SPI flash object, which can then be used
>> by the object itself but also by a SPI controller to handled the
>> MMIOs.
> Hi,
> 
> I am not sure if this approach is correct. I can not find any datasheet
> to this SPI controller, but as you mentioned in first paragraph, controller
> generates all commands (probably ones are somehow configurable).
> In this series you hack this behaviour and you do direct access to file.
> IMHO you should emulate sending such commands in SPI controller
> model.

I took some time to implement what you proposed, that is to emulate all 
the SPI commands needed to handle the read access. This is easily tested 
through the monitor. Looks good.

But the goal is to boot from the device, so I added a memory region alias 
at 0 to trigger the flash module mmios at boot time, as this is where 
u-boot expects to be. 

and I fell in this trap :/

	aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0
	Bad ram pointer (nil)
	Aborted (core dumped)

There is a failure in get_page_addr_code(), possibly because qemu uses
byte per byte reads of the code (cpu_ldub_code). But this is beyond my 
understanding of qemu's internal.


It seems that we do need a ROM. This is how firmwares are handled, with 
a load_image_targphys(), and also the pflash devices, through a ROM device 
region.


The proposal below allows to modify the backing region of the flash device
model for this purpose. A ROM device region can be used to enable booting 
from the device. It also allows some shortcuts in the model, like reading 
directly from the storage for the benefit of speed. This is what Peter 
Crosthwaite mentioned in a previous email. But this is not the primary 
use. Being able to use a ROM memory region is.

Shall I resend ? or are there strong objections ? 

Thanks,

C. 



> Thanks,
> Marcin
> 
>> This is sufficient to support read only accesses. For writes, we would
>> certainly need to externalize flash_write8() routine.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/block/m25p80.c        | 14 +++++++++++++-
>>   include/hw/block/flash.h |  2 ++
>>   2 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index aa964280beab..fec0fd4c2228 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -29,6 +29,7 @@
>>   #include "qemu/bitops.h"
>>   #include "qemu/log.h"
>>   #include "qapi/error.h"
>> +#include "hw/block/flash.h"
>>     #ifndef M25P80_ERR_DEBUG
>>   #define M25P80_ERR_DEBUG 0
>> @@ -1152,7 +1153,11 @@ static void m25p80_realize(SSISlave *ss, Error **errp)
>>         if (s->blk) {
>>           DB_PRINT_L(0, "Binding to IF_MTD drive\n");
>> -        s->storage = blk_blockalign(s->blk, s->size);
>> +
>> +        /* using an external storage. see m25p80_create_rom() */
>> +        if (!s->storage) {
>> +            s->storage = blk_blockalign(s->blk, s->size);
>> +        }
>>             if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) {
>>               error_setg(errp, "failed to read the initial flash content");
>> @@ -1259,3 +1264,10 @@ static void m25p80_register_types(void)
>>   }
>>     type_init(m25p80_register_types)
>> +
>> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom)
>> +{
>> +    Flash *s = M25P80(dev);
>> +
>> +    s->storage = memory_region_get_ram_ptr(rom);
>> +}
>> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
>> index 50ccbbcf1352..9d780f4ae0c9 100644
>> --- a/include/hw/block/flash.h
>> +++ b/include/hw/block/flash.h
>> @@ -61,4 +61,6 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample);
>>   void ecc_reset(ECCState *s);
>>   extern VMStateDescription vmstate_ecc_state;
>>   +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom);
>> +
>>   #endif
> 

Re: [Qemu-devel] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[Peter Maydell]

On 23 September 2016 at 08:19, Cédric Le Goater <clg@kaod.org> wrote:
> But the goal is to boot from the device, so I added a memory region alias
> at 0 to trigger the flash module mmios at boot time, as this is where
> u-boot expects to be.
>
> and I fell in this trap :/
>
>         aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0
>         Bad ram pointer (nil)
>         Aborted (core dumped)
>
> There is a failure in get_page_addr_code(), possibly because qemu uses
> byte per byte reads of the code (cpu_ldub_code). But this is beyond my
> understanding of qemu's internal.

This is a bug in how we report the problem, but the underlying
issue here is attempting to execute from something that's not RAM
or ROM. You can't execute code out of something backed by MMIO.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[Cédric Le Goater]

On 09/23/2016 10:17 AM, Peter Maydell wrote:
> On 23 September 2016 at 08:19, Cédric Le Goater <clg@kaod.org> wrote:
>> But the goal is to boot from the device, so I added a memory region alias
>> at 0 to trigger the flash module mmios at boot time, as this is where
>> u-boot expects to be.
>>
>> and I fell in this trap :/
>>
>>         aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0
>>         Bad ram pointer (nil)
>>         Aborted (core dumped)
>>
>> There is a failure in get_page_addr_code(), possibly because qemu uses
>> byte per byte reads of the code (cpu_ldub_code). But this is beyond my
>> understanding of qemu's internal.
> 
> This is a bug in how we report the problem, but the underlying
> issue here is attempting to execute from something that's not RAM
> or ROM. You can't execute code out of something backed by MMIO.

OK. So I see two solutions. T

The "brutal" one which is to copy the flash contents in a rom blob 
at 0, but there is still an issue in getting access to the storage 
anyhow, as it is internal to m25p80. Or we should get the name of the 
backing file of the drive but I am not sure we are expected to do 
that as I don't see any API for it.

The other solution is something like this patch which lets the storage 
of the flash device be assigned externally. 


Thanks,

C.

Re: [Qemu-devel] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[mar.krzeminski]

Hi Cedric,

W dniu 23.09.2016 o 10:28, Cédric Le Goater pisze:
> On 09/23/2016 10:17 AM, Peter Maydell wrote:
>> On 23 September 2016 at 08:19, Cédric Le Goater <clg@kaod.org> wrote:
>>> But the goal is to boot from the device, so I added a memory region alias
>>> at 0 to trigger the flash module mmios at boot time, as this is where
>>> u-boot expects to be.
>>>
>>> and I fell in this trap :/
>>>
>>>          aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0
>>>          Bad ram pointer (nil)
>>>          Aborted (core dumped)
>>>
>>> There is a failure in get_page_addr_code(), possibly because qemu uses
>>> byte per byte reads of the code (cpu_ldub_code). But this is beyond my
>>> understanding of qemu's internal.
>> This is a bug in how we report the problem, but the underlying
>> issue here is attempting to execute from something that's not RAM
>> or ROM. You can't execute code out of something backed by MMIO.
> OK. So I see two solutions. T
>
> The "brutal" one which is to copy the flash contents in a rom blob
> at 0, but there is still an issue in getting access to the storage
> anyhow, as it is internal to m25p80. Or we should get the name of the
> backing file of the drive but I am not sure we are expected to do
> that as I don't see any API for it.
>
> The other solution is something like this patch which lets the storage
> of the flash device be assigned externally.
Since I do not like dirty hacks in the code, I want just to suggest a 
workaround,
that probably you will not like ;]

As Qemu expects that first running code will be in ROM or RAM memory,
you can implement in your board -bios option that you will use to
pass u-boot binary to rom memory, or even use generic loader functionality
when it reach master.

Thanks,
Marcin

> Thanks,
>
> C.
>
>

Re: [Qemu-devel] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[Cédric Le Goater]

On 09/23/2016 08:26 PM, mar.krzeminski wrote:
> Hi Cedric,
> 
> W dniu 23.09.2016 o 10:28, Cédric Le Goater pisze:
>> On 09/23/2016 10:17 AM, Peter Maydell wrote:
>>> On 23 September 2016 at 08:19, Cédric Le Goater <clg@kaod.org> wrote:
>>>> But the goal is to boot from the device, so I added a memory region alias
>>>> at 0 to trigger the flash module mmios at boot time, as this is where
>>>> u-boot expects to be.
>>>>
>>>> and I fell in this trap :/
>>>>
>>>>          aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0
>>>>          Bad ram pointer (nil)
>>>>          Aborted (core dumped)
>>>>
>>>> There is a failure in get_page_addr_code(), possibly because qemu uses
>>>> byte per byte reads of the code (cpu_ldub_code). But this is beyond my
>>>> understanding of qemu's internal.
>>> This is a bug in how we report the problem, but the underlying
>>> issue here is attempting to execute from something that's not RAM
>>> or ROM. You can't execute code out of something backed by MMIO.
>> OK. So I see two solutions. T
>>
>> The "brutal" one which is to copy the flash contents in a rom blob
>> at 0, but there is still an issue in getting access to the storage
>> anyhow, as it is internal to m25p80. Or we should get the name of the
>> backing file of the drive but I am not sure we are expected to do
>> that as I don't see any API for it.
>>
>> The other solution is something like this patch which lets the storage
>> of the flash device be assigned externally.
> Since I do not like dirty hacks in the code, I want just to suggest a 
> workaround, that probably you will not like ;]

It's a feature ! :) 

I am just trying to find a solution to this issue. So, we could also 
introduce a routine :

+uint8_t *m25p80_get_storage(DeviceState *dev, uint32_t *size)
+{
+    Flash *s = M25P80(dev);
+
+    *size = s->size;
+    return s->storage;
+}

and use rom_add_blob_fixed() with the return values. Maybe this is less 
intrusive in the m25p80 device model flow. Thoughts ? 

> As Qemu expects that first running code will be in ROM or RAM memory,
> you can implement in your board -bios option that you will use to
> pass u-boot binary to rom memory, or even use generic loader functionality
> when it reach master.

but if we use -bios <file> to have a ROM, we will need to pass a second 
time <file> as a drive to have a CS0 flash slave: 

	-bios "flash-palmetto-test"  \
	-drive file="flash-palmetto-test",format=raw,if=mtd \
	-drive file="palmetto.pnor",format=raw,if=mtd

This feels awkward. The virt platform and vexpress forbid that for 
instance.

Are there any other platform with similar need ? 

Thanks,
C. 

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[Edgar E. Iglesias]

On Sat, Sep 24, 2016 at 10:25:39AM +0200, Cédric Le Goater wrote:
> On 09/23/2016 08:26 PM, mar.krzeminski wrote:
> > Hi Cedric,
> > 
> > W dniu 23.09.2016 o 10:28, Cédric Le Goater pisze:
> >> On 09/23/2016 10:17 AM, Peter Maydell wrote:
> >>> On 23 September 2016 at 08:19, Cédric Le Goater <clg@kaod.org> wrote:
> >>>> But the goal is to boot from the device, so I added a memory region alias
> >>>> at 0 to trigger the flash module mmios at boot time, as this is where
> >>>> u-boot expects to be.
> >>>>
> >>>> and I fell in this trap :/
> >>>>
> >>>>          aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0
> >>>>          Bad ram pointer (nil)
> >>>>          Aborted (core dumped)
> >>>>
> >>>> There is a failure in get_page_addr_code(), possibly because qemu uses
> >>>> byte per byte reads of the code (cpu_ldub_code). But this is beyond my
> >>>> understanding of qemu's internal.
> >>> This is a bug in how we report the problem, but the underlying
> >>> issue here is attempting to execute from something that's not RAM
> >>> or ROM. You can't execute code out of something backed by MMIO.
> >> OK. So I see two solutions. T
> >>
> >> The "brutal" one which is to copy the flash contents in a rom blob
> >> at 0, but there is still an issue in getting access to the storage
> >> anyhow, as it is internal to m25p80. Or we should get the name of the
> >> backing file of the drive but I am not sure we are expected to do
> >> that as I don't see any API for it.
> >>
> >> The other solution is something like this patch which lets the storage
> >> of the flash device be assigned externally.
> > Since I do not like dirty hacks in the code, I want just to suggest a 
> > workaround, that probably you will not like ;]
> 
> It's a feature ! :)

CC: Mark and Fred

I agree, I think these are fair attempts to try to solve a real problem.


> I am just trying to find a solution to this issue. So, we could also 
> introduce a routine :
> 
> +uint8_t *m25p80_get_storage(DeviceState *dev, uint32_t *size)
> +{
> +    Flash *s = M25P80(dev);
> +
> +    *size = s->size;
> +    return s->storage;
> +}
> 
> and use rom_add_blob_fixed() with the return values. Maybe this is less 
> intrusive in the m25p80 device model flow. Thoughts ? 
> 
> > As Qemu expects that first running code will be in ROM or RAM memory,
> > you can implement in your board -bios option that you will use to
> > pass u-boot binary to rom memory, or even use generic loader functionality
> > when it reach master.
> 
> but if we use -bios <file> to have a ROM, we will need to pass a second 
> time <file> as a drive to have a CS0 flash slave: 
> 
> 	-bios "flash-palmetto-test"  \
> 	-drive file="flash-palmetto-test",format=raw,if=mtd \
> 	-drive file="palmetto.pnor",format=raw,if=mtd
> 
> This feels awkward. The virt platform and vexpress forbid that for 
> instance.
> 
> Are there any other platform with similar need ? 

Yes, ZynqMP has a linear memory mapping of SPI memory contents. It gets
slightly more complicated there as the mapping supports various modes
including parallel SPIs with striping done by the controller (i.e
bytes as seen via the linearly mapped area are interleaved from multiple
SPI memories).

So a plain RAM mapping of m25p80_get_storage won't work.

A ROM version of the linear data might work at controller level. The controller
would have to populate the ROM area at startup (slow) and keep it in sync for all
writes. We would also need to signal write events so that TCG can flush it's
caches. TCG only keeps track of modifications being made to cached code if changes
are done with writes to RAM area (not if they happen indirectly via other registers).

Another solution is to implement support in TCG to execute from MMIO mapped areas.
We'd also need to solve the problem of signalling content changes to TCG.

I think these approaches have some overlap. Personally, I think TCG executing from
MMIO areas would be quite useful. It's also useful for QEMU & SystemC integration.

Best regards,
Edgar

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[KONRAD Frederic]

Le 24/09/2016 à 10:55, Edgar E. Iglesias a écrit :
> On Sat, Sep 24, 2016 at 10:25:39AM +0200, Cédric Le Goater wrote:
>> On 09/23/2016 08:26 PM, mar.krzeminski wrote:
>>> Hi Cedric,
>>>
>>> W dniu 23.09.2016 o 10:28, Cédric Le Goater pisze:
>>>> On 09/23/2016 10:17 AM, Peter Maydell wrote:
>>>>> On 23 September 2016 at 08:19, Cédric Le Goater <clg@kaod.org> wrote:
>>>>>> But the goal is to boot from the device, so I added a memory region alias
>>>>>> at 0 to trigger the flash module mmios at boot time, as this is where
>>>>>> u-boot expects to be.
>>>>>>
>>>>>> and I fell in this trap :/
>>>>>>
>>>>>>          aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0
>>>>>>          Bad ram pointer (nil)
>>>>>>          Aborted (core dumped)
>>>>>>
>>>>>> There is a failure in get_page_addr_code(), possibly because qemu uses
>>>>>> byte per byte reads of the code (cpu_ldub_code). But this is beyond my
>>>>>> understanding of qemu's internal.
>>>>> This is a bug in how we report the problem, but the underlying
>>>>> issue here is attempting to execute from something that's not RAM
>>>>> or ROM. You can't execute code out of something backed by MMIO.
>>>> OK. So I see two solutions. T
>>>>
>>>> The "brutal" one which is to copy the flash contents in a rom blob
>>>> at 0, but there is still an issue in getting access to the storage
>>>> anyhow, as it is internal to m25p80. Or we should get the name of the
>>>> backing file of the drive but I am not sure we are expected to do
>>>> that as I don't see any API for it.
>>>>
>>>> The other solution is something like this patch which lets the storage
>>>> of the flash device be assigned externally.
>>> Since I do not like dirty hacks in the code, I want just to suggest a
>>> workaround, that probably you will not like ;]
>>
>> It's a feature ! :)
>
> CC: Mark and Fred
>
> I agree, I think these are fair attempts to try to solve a real problem.
>
>
>> I am just trying to find a solution to this issue. So, we could also
>> introduce a routine :
>>
>> +uint8_t *m25p80_get_storage(DeviceState *dev, uint32_t *size)
>> +{
>> +    Flash *s = M25P80(dev);
>> +
>> +    *size = s->size;
>> +    return s->storage;
>> +}
>>
>> and use rom_add_blob_fixed() with the return values. Maybe this is less
>> intrusive in the m25p80 device model flow. Thoughts ?
>>
>>> As Qemu expects that first running code will be in ROM or RAM memory,
>>> you can implement in your board -bios option that you will use to
>>> pass u-boot binary to rom memory, or even use generic loader functionality
>>> when it reach master.
>>
>> but if we use -bios <file> to have a ROM, we will need to pass a second
>> time <file> as a drive to have a CS0 flash slave:
>>
>> 	-bios "flash-palmetto-test"  \
>> 	-drive file="flash-palmetto-test",format=raw,if=mtd \
>> 	-drive file="palmetto.pnor",format=raw,if=mtd
>>
>> This feels awkward. The virt platform and vexpress forbid that for
>> instance.
>>
>> Are there any other platform with similar need ?
>
> Yes, ZynqMP has a linear memory mapping of SPI memory contents. It gets
> slightly more complicated there as the mapping supports various modes
> including parallel SPIs with striping done by the controller (i.e
> bytes as seen via the linearly mapped area are interleaved from multiple
> SPI memories).
>
> So a plain RAM mapping of m25p80_get_storage won't work.
>
> A ROM version of the linear data might work at controller level. The controller
> would have to populate the ROM area at startup (slow) and keep it in sync for all
> writes. We would also need to signal write events so that TCG can flush it's
> caches. TCG only keeps track of modifications being made to cached code if changes
> are done with writes to RAM area (not if they happen indirectly via other registers).
>
> Another solution is to implement support in TCG to execute from MMIO mapped areas.
> We'd also need to solve the problem of signalling content changes to TCG.
>
> I think these approaches have some overlap. Personally, I think TCG executing from
> MMIO areas would be quite useful. It's also useful for QEMU & SystemC integration.

Hi,

The solution we are preparing for this is to allow a MMIO region to 
execute code as Edgar just suggested by adding two callbacks:
   - one which allows to get a pointer from an MMIO region nothing if
     execution is not possible from it and some hint to know if it's
     readable, writable.
   - one function to "invalidate" the pointer which will flush the tb,
     etc.

We have fixed a second issue fairly linked to this: if we do a DMA 
access to the MMIO areas it is split in 1,2,4 bytes transaction which is 
really slow.

Thanks,
Fred
>
> Best regards,
> Edgar
>

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[Cédric Le Goater]

On 09/26/2016 10:25 AM, KONRAD Frederic wrote:
> 
> 
> Le 24/09/2016 à 10:55, Edgar E. Iglesias a écrit :
>> On Sat, Sep 24, 2016 at 10:25:39AM +0200, Cédric Le Goater wrote:
>>> On 09/23/2016 08:26 PM, mar.krzeminski wrote:
>>>> Hi Cedric,
>>>>
>>>> W dniu 23.09.2016 o 10:28, Cédric Le Goater pisze:
>>>>> On 09/23/2016 10:17 AM, Peter Maydell wrote:
>>>>>> On 23 September 2016 at 08:19, Cédric Le Goater <clg@kaod.org> wrote:
>>>>>>> But the goal is to boot from the device, so I added a memory region alias
>>>>>>> at 0 to trigger the flash module mmios at boot time, as this is where
>>>>>>> u-boot expects to be.
>>>>>>>
>>>>>>> and I fell in this trap :/
>>>>>>>
>>>>>>>          aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0
>>>>>>>          Bad ram pointer (nil)
>>>>>>>          Aborted (core dumped)
>>>>>>>
>>>>>>> There is a failure in get_page_addr_code(), possibly because qemu uses
>>>>>>> byte per byte reads of the code (cpu_ldub_code). But this is beyond my
>>>>>>> understanding of qemu's internal.
>>>>>> This is a bug in how we report the problem, but the underlying
>>>>>> issue here is attempting to execute from something that's not RAM
>>>>>> or ROM. You can't execute code out of something backed by MMIO.
>>>>> OK. So I see two solutions. T
>>>>>
>>>>> The "brutal" one which is to copy the flash contents in a rom blob
>>>>> at 0, but there is still an issue in getting access to the storage
>>>>> anyhow, as it is internal to m25p80. Or we should get the name of the
>>>>> backing file of the drive but I am not sure we are expected to do
>>>>> that as I don't see any API for it.
>>>>>
>>>>> The other solution is something like this patch which lets the storage
>>>>> of the flash device be assigned externally.
>>>> Since I do not like dirty hacks in the code, I want just to suggest a
>>>> workaround, that probably you will not like ;]
>>>
>>> It's a feature ! :)
>>
>> CC: Mark and Fred
>>
>> I agree, I think these are fair attempts to try to solve a real problem.
>>
>>
>>> I am just trying to find a solution to this issue. So, we could also
>>> introduce a routine :
>>>
>>> +uint8_t *m25p80_get_storage(DeviceState *dev, uint32_t *size)
>>> +{
>>> +    Flash *s = M25P80(dev);
>>> +
>>> +    *size = s->size;
>>> +    return s->storage;
>>> +}
>>>
>>> and use rom_add_blob_fixed() with the return values. Maybe this is less
>>> intrusive in the m25p80 device model flow. Thoughts ?
>>>
>>>> As Qemu expects that first running code will be in ROM or RAM memory,
>>>> you can implement in your board -bios option that you will use to
>>>> pass u-boot binary to rom memory, or even use generic loader functionality
>>>> when it reach master.
>>>
>>> but if we use -bios <file> to have a ROM, we will need to pass a second
>>> time <file> as a drive to have a CS0 flash slave:
>>>
>>>     -bios "flash-palmetto-test"  \
>>>     -drive file="flash-palmetto-test",format=raw,if=mtd \
>>>     -drive file="palmetto.pnor",format=raw,if=mtd
>>>
>>> This feels awkward. The virt platform and vexpress forbid that for
>>> instance.
>>>
>>> Are there any other platform with similar need ?
>>
>> Yes, ZynqMP has a linear memory mapping of SPI memory contents. It gets
>> slightly more complicated there as the mapping supports various modes
>> including parallel SPIs with striping done by the controller (i.e
>> bytes as seen via the linearly mapped area are interleaved from multiple
>> SPI memories).
>>
>> So a plain RAM mapping of m25p80_get_storage won't work.
>>
>> A ROM version of the linear data might work at controller level. The controller
>> would have to populate the ROM area at startup (slow) and keep it in sync for all
>> writes. We would also need to signal write events so that TCG can flush it's
>> caches. TCG only keeps track of modifications being made to cached code if changes
>> are done with writes to RAM area (not if they happen indirectly via other registers).
>>
>> Another solution is to implement support in TCG to execute from MMIO mapped areas.
>> We'd also need to solve the problem of signalling content changes to TCG.
>>
>> I think these approaches have some overlap. Personally, I think TCG executing from
>> MMIO areas would be quite useful. It's also useful for QEMU & SystemC integration.
> 
> Hi,
> 
> The solution we are preparing for this is to allow a MMIO region to execute code 
> as Edgar just suggested by adding two callbacks:

Yes it seems the best option to boot. 

The SPI controller I am working on maintains a set of mapping windows for 
each flash slave. But it's up to software to make sure the window size and 
the flash size are in sync. So, in a qemu world, we will need some flexibility 
if we want to access directly the flash storage. I don't think this patch is 
the right approach though. Let's boot first. 

>   - one which allows to get a pointer from an MMIO region nothing if
>     execution is not possible from it and some hint to know if it's
>     readable, writable.
>   - one function to "invalidate" the pointer which will flush the tb,
>     etc.

I don't know enough about TCG to comment, I am very willing to give it 
a try when ever you have something ready.

Thanks,

C.

> We have fixed a second issue fairly linked to this: if we do a DMA access to the 
> MMIO areas it is split in 1,2,4 bytes transaction which is really slow.

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[mar.krzeminski]

W dniu 26.09.2016 o 10:56, Cédric Le Goater pisze:
> On 09/26/2016 10:25 AM, KONRAD Frederic wrote:
>>
>> Le 24/09/2016 à 10:55, Edgar E. Iglesias a écrit :
>>> On Sat, Sep 24, 2016 at 10:25:39AM +0200, Cédric Le Goater wrote:
>>>> On 09/23/2016 08:26 PM, mar.krzeminski wrote:
>>>>> Hi Cedric,
>>>>>
>>>>> W dniu 23.09.2016 o 10:28, Cédric Le Goater pisze:
>>>>>> On 09/23/2016 10:17 AM, Peter Maydell wrote:
>>>>>>> On 23 September 2016 at 08:19, Cédric Le Goater <clg@kaod.org> wrote:
>>>>>>>> But the goal is to boot from the device, so I added a memory region alias
>>>>>>>> at 0 to trigger the flash module mmios at boot time, as this is where
>>>>>>>> u-boot expects to be.
>>>>>>>>
>>>>>>>> and I fell in this trap :/
>>>>>>>>
>>>>>>>>           aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0
>>>>>>>>           Bad ram pointer (nil)
>>>>>>>>           Aborted (core dumped)
>>>>>>>>
>>>>>>>> There is a failure in get_page_addr_code(), possibly because qemu uses
>>>>>>>> byte per byte reads of the code (cpu_ldub_code). But this is beyond my
>>>>>>>> understanding of qemu's internal.
>>>>>>> This is a bug in how we report the problem, but the underlying
>>>>>>> issue here is attempting to execute from something that's not RAM
>>>>>>> or ROM. You can't execute code out of something backed by MMIO.
>>>>>> OK. So I see two solutions. T
>>>>>>
>>>>>> The "brutal" one which is to copy the flash contents in a rom blob
>>>>>> at 0, but there is still an issue in getting access to the storage
>>>>>> anyhow, as it is internal to m25p80. Or we should get the name of the
>>>>>> backing file of the drive but I am not sure we are expected to do
>>>>>> that as I don't see any API for it.
>>>>>>
>>>>>> The other solution is something like this patch which lets the storage
>>>>>> of the flash device be assigned externally.
>>>>> Since I do not like dirty hacks in the code, I want just to suggest a
>>>>> workaround, that probably you will not like ;]
>>>> It's a feature ! :)
>>> CC: Mark and Fred
>>>
>>> I agree, I think these are fair attempts to try to solve a real problem.
>>>
>>>
>>>> I am just trying to find a solution to this issue. So, we could also
>>>> introduce a routine :
>>>>
>>>> +uint8_t *m25p80_get_storage(DeviceState *dev, uint32_t *size)
>>>> +{
>>>> +    Flash *s = M25P80(dev);
>>>> +
>>>> +    *size = s->size;
>>>> +    return s->storage;
>>>> +}
>>>>
>>>> and use rom_add_blob_fixed() with the return values. Maybe this is less
>>>> intrusive in the m25p80 device model flow. Thoughts ?
>>>>
>>>>> As Qemu expects that first running code will be in ROM or RAM memory,
>>>>> you can implement in your board -bios option that you will use to
>>>>> pass u-boot binary to rom memory, or even use generic loader functionality
>>>>> when it reach master.
>>>> but if we use -bios <file> to have a ROM, we will need to pass a second
>>>> time <file> as a drive to have a CS0 flash slave:
>>>>
>>>>      -bios "flash-palmetto-test"  \
>>>>      -drive file="flash-palmetto-test",format=raw,if=mtd \
>>>>      -drive file="palmetto.pnor",format=raw,if=mtd
>>>>
>>>> This feels awkward. The virt platform and vexpress forbid that for
>>>> instance.
Just to clarify, I mean a use of -bios option or generic loader just to 
allow this board to boot from u-boot
by directly potting u-boot in the memory where flash should be mapped.
I will not solve memory mapped flash problem at all..
>>>>
>>>> Are there any other platform with similar need ?
>>> Yes, ZynqMP has a linear memory mapping of SPI memory contents. It gets
>>> slightly more complicated there as the mapping supports various modes
>>> including parallel SPIs with striping done by the controller (i.e
>>> bytes as seen via the linearly mapped area are interleaved from multiple
>>> SPI memories).
>>>
>>> So a plain RAM mapping of m25p80_get_storage won't work.
>>>
>>> A ROM version of the linear data might work at controller level. The controller
>>> would have to populate the ROM area at startup (slow) and keep it in sync for all
>>> writes. We would also need to signal write events so that TCG can flush it's
>>> caches. TCG only keeps track of modifications being made to cached code if changes
>>> are done with writes to RAM area (not if they happen indirectly via other registers).
I was trying that in a very simple way, I stopped because I used 128MiB 
Flash (memory consumption
and speed).
>>>
>>> Another solution is to implement support in TCG to execute from MMIO mapped areas.
>>> We'd also need to solve the problem of signalling content changes to TCG.
>>>
>>> I think these approaches have some overlap. Personally, I think TCG executing from
>>> MMIO areas would be quite useful. It's also useful for QEMU & SystemC integration.
I can agree here, this could solve this problem, and allow to do more.
>> Hi,
>>
>> The solution we are preparing for this is to allow a MMIO region to execute code
>> as Edgar just suggested by adding two callbacks:
> Yes it seems the best option to boot.
>
> The SPI controller I am working on maintains a set of mapping windows for
> each flash slave. But it's up to software to make sure the window size and
> the flash size are in sync. So, in a qemu world, we will need some flexibility
> if we want to access directly the flash storage. I don't think this patch is
> the right approach though. Let's boot first.
I believe we agree here. Exposing underlying file is to big shortcut,
from the other hand it will allow to boot from flash.
>
>>    - one which allows to get a pointer from an MMIO region nothing if
>>      execution is not possible from it and some hint to know if it's
>>      readable, writable.
>>    - one function to "invalidate" the pointer which will flush the tb,
>>      etc.
> I don't know enough about TCG to comment, I am very willing to give it
> a try when ever you have something ready.
I am also interested in testing that.

Thanks,
Marcin
> Thanks,
>
> C.
>
>> We have fixed a second issue fairly linked to this: if we do a DMA access to the
>> MMIO areas it is split in 1,2,4 bytes transaction which is really slow.
>

[Qemu-devel] [PATCH 5/7] ast2400: handle SPI flash Command mode (read only)

[Cédric Le Goater]

To handle memory accesses when the SPI flash slave is configured in
Command mode, let's change the memory region of the SPI flash object
to a ROM memory region (like the pflash_cfi* object). The m25p80 flash
object creation is changed accordingly to use the new memory region as
a storage.

Only read only accesses are handled. Supporting write accesses would
demand using internal routines of the m25p80 flash model. This is not
required for the moment.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/palmetto-bmc.c       |  2 ++
 hw/ssi/aspeed_smc.c         | 60 +++++++++++++++++++++++++++++++++++++++++----
 include/hw/ssi/aspeed_smc.h |  1 +
 3 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
index c0bb922f6406..b00757dcbc69 100644
--- a/hw/arm/palmetto-bmc.c
+++ b/hw/arm/palmetto-bmc.c
@@ -20,6 +20,7 @@
 #include "qemu/log.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
+#include "hw/block/flash.h"
 
 static struct arm_boot_info palmetto_bmc_binfo = {
     .loader_start = AST2400_SDRAM_BASE,
@@ -51,6 +52,7 @@ static void palmetto_bmc_init_flashes(AspeedSMCState *s, const char *flashtype,
             qdev_prop_set_drive(fl->flash, "drive", blk_by_legacy_dinfo(dinfo),
                                 errp);
         }
+        m25p80_set_rom_storage(fl->flash, &fl->mmio);
         qdev_init_nofail(fl->flash);
 
         cs_line = qdev_get_gpio_in_named(fl->flash, SSI_GPIO_CS, 0);
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index cb0b23750bcf..f88edf953dee 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -23,11 +23,13 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "hw/sysbus.h"
 #include "sysemu/sysemu.h"
 #include "qemu/log.h"
 #include "include/qemu/error-report.h"
 #include "exec/address-spaces.h"
+#include "hw/block/flash.h"
 
 #include "hw/ssi/aspeed_smc.h"
 
@@ -198,6 +200,37 @@ static inline bool aspeed_smc_is_writable(const AspeedSMCState *s, int cs)
     return s->regs[s->r_conf] & (1 << (s->conf_enable_w0 + cs));
 }
 
+/*
+ * Sanity checks on the command mode and the SPI flash command being
+ * used
+ */
+static inline bool aspeed_smc_check_mode(const AspeedSMCState *s, int cs)
+{
+    uint8_t mode = aspeed_smc_flash_mode(s, cs);
+    uint8_t cmd = (s->regs[s->r_ctrl0 + cs] >> CTRL_CMD_SHIFT) && CTRL_CMD_MASK;
+    bool ret;
+
+    switch (mode) {
+    case CTRL_READMODE:
+        ret = (cmd == 0x3 || cmd == 0x0);
+        break;
+    case CTRL_FREADMODE:
+        ret = true;
+        break;
+    case CTRL_WRITEMODE:
+    default:
+        ret = false;
+        break;
+    }
+
+    if (!ret) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid mode/command: %d/%d\n",
+                      __func__, cmd, mode);
+    }
+
+    return ret;
+}
+
 static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
 {
     AspeedSMCFlash *fl = opaque;
@@ -210,9 +243,11 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
             ret |= ssi_transfer(s->spi, 0x0) << (8 * i);
         }
     } else {
-        qemu_log_mask(LOG_UNIMP, "%s: usermode not implemented\n",
-                      __func__);
-        ret = -1;
+        if (aspeed_smc_check_mode(s, fl->id)) {
+            for (i = 0; i < size; i++) {
+                ret = fl->storage[addr + i] << (8 * i);
+            }
+        }
     }
 
     return ret;
@@ -336,6 +371,12 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
      */
     s->regs[addr] = value;
     if (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->num_cs) {
+        int i;
+
+        for (i = 0; i < s->num_cs; ++i) {
+            memory_region_rom_device_set_romd(&s->flashes[i].mmio,
+                                              !aspeed_smc_is_usermode(s, i));
+        }
         aspeed_smc_update_cs(s);
     }
 }
@@ -355,6 +396,7 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
     int i;
     char name[32];
     hwaddr offset = 0;
+    Error *err = NULL;
 
     s->ctrl = mc->ctrl;
 
@@ -409,8 +451,16 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
         fl->id = i;
         fl->controller = s;
         fl->size = s->ctrl->segments[i].size;
-        memory_region_init_io(&fl->mmio, OBJECT(s), &aspeed_smc_flash_ops,
-                              fl, name, fl->size);
+        memory_region_init_rom_device(&fl->mmio, OBJECT(s),
+                                      &aspeed_smc_flash_ops,
+                                      fl, name, fl->size, &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
+        vmstate_register_ram(&fl->mmio, DEVICE(s));
+        fl->storage = memory_region_get_ram_ptr(&fl->mmio);
+
         memory_region_add_subregion(&s->mmio_flash, offset, &fl->mmio);
         offset += fl->size;
     }
diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index def3b4507e75..4796b4ca0138 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -47,6 +47,7 @@ typedef struct AspeedSMCController {
 
 typedef struct AspeedSMCFlash {
     const struct AspeedSMCState *controller;
+    uint8_t *storage;
 
     uint8_t id;
     uint32_t size;
-- 
2.1.4

[Qemu-devel] [PATCH 6/7] ast2400: use contents of first SPI flash as a rom

[Cédric Le Goater]

This provides support for U-Boot images which are loaded at 0x0.

A Palmetto BMC guest can now be simply booted with :

  $ qemu-system-arm -m 256 -M palmetto-bmc -nographic -nodefaults  \
	-mtdblock ./flash-palmetto-20160512040959  \
	-mtdblock ./palmetto.pnor

The first block device uses the file './flash-palmetto-20160512040959'
which will act as a SPI flash module for the BMC, handled by the
SMC/FMC controller. The second block device uses the file
'./palmetto.pnor' which is an OpenPower firmware image for a palmetto
OpenPower system. This one will be handled by the SMC/SPI controller.

The flash images can be grabbed here :

    https://openpower.xyz/job/openbmc-build/distro=ubuntu,target=palmetto/lastSuccessfulBuild/artifact/images/palmetto/flash-palmetto
    https://openpower.xyz/job/openpower-op-build/distro=ubuntu,target=palmetto/lastSuccessfulBuild/artifact/images/palmetto.pnor

We could add a second BMC SPI flash by changing the 'num-cs' property
of the controller and emulate a golden image flash module.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/palmetto-bmc.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
index b00757dcbc69..fcbb4f197162 100644
--- a/hw/arm/palmetto-bmc.c
+++ b/hw/arm/palmetto-bmc.c
@@ -33,6 +33,8 @@ typedef struct PalmettoBMCState {
     MemoryRegion ram;
 } PalmettoBMCState;
 
+static bool palmetto_bmc_has_flash0;
+
 static void palmetto_bmc_init_flashes(AspeedSMCState *s, const char *flashtype,
                                       Error **errp)
 {
@@ -51,6 +53,7 @@ static void palmetto_bmc_init_flashes(AspeedSMCState *s, const char *flashtype,
         if (dinfo) {
             qdev_prop_set_drive(fl->flash, "drive", blk_by_legacy_dinfo(dinfo),
                                 errp);
+            palmetto_bmc_has_flash0 = true;
         }
         m25p80_set_rom_storage(fl->flash, &fl->mmio);
         qdev_init_nofail(fl->flash);
@@ -82,6 +85,21 @@ static void palmetto_bmc_init(MachineState *machine)
     palmetto_bmc_init_flashes(&bmc->soc.smc, "n25q256a", &error_abort);
     palmetto_bmc_init_flashes(&bmc->soc.spi, "mx25l25635f", &error_abort);
 
+    /*
+     * Install first SMC/FMC flash content as a rom.
+     */
+    if (palmetto_bmc_has_flash0) {
+        AspeedSMCFlash *flash0 = &bmc->soc.smc.flashes[0];
+        MemoryRegion *flash0alias = g_new(MemoryRegion, 1);
+
+        memory_region_init_alias(flash0alias, OBJECT(&bmc->soc.smc),
+                                 "flash0alias", &flash0->mmio, 0,
+                                 flash0->size);
+
+        memory_region_add_subregion(get_system_memory(), 0, flash0alias);
+        palmetto_bmc_binfo.firmware_loaded = true;
+    }
+
     palmetto_bmc_binfo.kernel_filename = machine->kernel_filename;
     palmetto_bmc_binfo.initrd_filename = machine->initrd_filename;
     palmetto_bmc_binfo.kernel_cmdline = machine->kernel_cmdline;
-- 
2.1.4

Re: [Qemu-devel] [PATCH 6/7] ast2400: use contents of first SPI flash as a rom

[Cédric Le Goater]

On 07/04/2016 02:18 PM, Cédric Le Goater wrote:
> This provides support for U-Boot images which are loaded at 0x0.
> 
> A Palmetto BMC guest can now be simply booted with :
> 
>   $ qemu-system-arm -m 256 -M palmetto-bmc -nographic -nodefaults  \
> 	-mtdblock ./flash-palmetto-20160512040959  \
> 	-mtdblock ./palmetto.pnor
> 
> The first block device uses the file './flash-palmetto-20160512040959'
> which will act as a SPI flash module for the BMC, handled by the
> SMC/FMC controller. The second block device uses the file
> './palmetto.pnor' which is an OpenPower firmware image for a palmetto
> OpenPower system. This one will be handled by the SMC/SPI controller.
> 
> The flash images can be grabbed here :
> 
>     https://openpower.xyz/job/openbmc-build/distro=ubuntu,target=palmetto/lastSuccessfulBuild/artifact/images/palmetto/flash-palmetto
>     https://openpower.xyz/job/openpower-op-build/distro=ubuntu,target=palmetto/lastSuccessfulBuild/artifact/images/palmetto.pnor
> 
> We could add a second BMC SPI flash by changing the 'num-cs' property
> of the controller and emulate a golden image flash module.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/arm/palmetto-bmc.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
> index b00757dcbc69..fcbb4f197162 100644
> --- a/hw/arm/palmetto-bmc.c
> +++ b/hw/arm/palmetto-bmc.c
> @@ -33,6 +33,8 @@ typedef struct PalmettoBMCState {
>      MemoryRegion ram;
>  } PalmettoBMCState;
>  
> +static bool palmetto_bmc_has_flash0;
> +
>  static void palmetto_bmc_init_flashes(AspeedSMCState *s, const char *flashtype,
>                                        Error **errp)
>  {
> @@ -51,6 +53,7 @@ static void palmetto_bmc_init_flashes(AspeedSMCState *s, const char *flashtype,
>          if (dinfo) {
>              qdev_prop_set_drive(fl->flash, "drive", blk_by_legacy_dinfo(dinfo),
>                                  errp);
> +            palmetto_bmc_has_flash0 = true;
>          }
>          m25p80_set_rom_storage(fl->flash, &fl->mmio);
>          qdev_init_nofail(fl->flash);
> @@ -82,6 +85,21 @@ static void palmetto_bmc_init(MachineState *machine)
>      palmetto_bmc_init_flashes(&bmc->soc.smc, "n25q256a", &error_abort);
>      palmetto_bmc_init_flashes(&bmc->soc.spi, "mx25l25635f", &error_abort);
>  
> +    /*
> +     * Install first SMC/FMC flash content as a rom.
> +     */
> +    if (palmetto_bmc_has_flash0) {
> +        AspeedSMCFlash *flash0 = &bmc->soc.smc.flashes[0];
> +        MemoryRegion *flash0alias = g_new(MemoryRegion, 1);
> +
> +        memory_region_init_alias(flash0alias, OBJECT(&bmc->soc.smc),
> +                                 "flash0alias", &flash0->mmio, 0,
> +                                 flash0->size);
> +
> +        memory_region_add_subregion(get_system_memory(), 0, flash0alias);
> +        palmetto_bmc_binfo.firmware_loaded = true;


This should not be there. It will break qemu when a kernel is specified on 
the command line.

I will drop it in the next round.

Thanks,

C.

> +    }
> +
>      palmetto_bmc_binfo.kernel_filename = machine->kernel_filename;
>      palmetto_bmc_binfo.initrd_filename = machine->initrd_filename;
>      palmetto_bmc_binfo.kernel_cmdline = machine->kernel_cmdline;
> 

[Qemu-devel] [PATCH 7/7] ast2400: add a memory controller device model

[Cédric Le Goater]

The uboot in the previous release of the SDK was using a hardcoded
value for memory size. This is not true anymore, the value is now
retrieved from the memory controller.

Below is a model for this device, only supporting unlock and
configuration. Without it, we endup running a guest with 64MB, which
is a bit low for Linux nowdays.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/ast2400.c              |  13 +++++
 hw/misc/Makefile.objs         |   2 +-
 hw/misc/aspeed_sdmc.c         | 126 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/arm/ast2400.h      |   1 +
 include/hw/misc/aspeed_sdmc.h |  49 ++++++++++++++++
 5 files changed, 190 insertions(+), 1 deletion(-)
 create mode 100644 hw/misc/aspeed_sdmc.c
 create mode 100644 include/hw/misc/aspeed_sdmc.h

diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c
index 9c30b45f87a9..fb0156ba8bc1 100644
--- a/hw/arm/ast2400.c
+++ b/hw/arm/ast2400.c
@@ -27,6 +27,7 @@
 #define AST2400_FMC_BASE         0X1E620000
 #define AST2400_SPI_BASE         0X1E630000
 #define AST2400_VIC_BASE         0x1E6C0000
+#define AST2400_SDMC_BASE        0x1E6E0000
 #define AST2400_SCU_BASE         0x1E6E2000
 #define AST2400_TIMER_BASE       0x1E782000
 #define AST2400_I2C_BASE         0x1E78A000
@@ -99,6 +100,10 @@ static void ast2400_init(Object *obj)
     object_initialize(&s->spi, sizeof(s->spi), "aspeed.smc.spi");
     object_property_add_child(obj, "spi", OBJECT(&s->spi), NULL);
     qdev_set_parent_bus(DEVICE(&s->spi), sysbus_get_default());
+
+    object_initialize(&s->sdmc, sizeof(s->sdmc), TYPE_ASPEED_SDMC);
+    object_property_add_child(obj, "sdmc", OBJECT(&s->sdmc), NULL);
+    qdev_set_parent_bus(DEVICE(&s->sdmc), sysbus_get_default());
 }
 
 static void ast2400_realize(DeviceState *dev, Error **errp)
@@ -184,6 +189,14 @@ static void ast2400_realize(DeviceState *dev, Error **errp)
     }
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 0, AST2400_SPI_BASE);
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 1, AST2400_SPI_FLASH_BASE);
+
+    /* SDMC - SDRAM Memory Controller */
+    object_property_set_bool(OBJECT(&s->sdmc), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdmc), 0, AST2400_SDMC_BASE);
 }
 
 static void ast2400_class_init(ObjectClass *oc, void *data)
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 54020aa06c00..d488b748c789 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -52,4 +52,4 @@ obj-$(CONFIG_PVPANIC) += pvpanic.o
 obj-$(CONFIG_EDU) += edu.o
 obj-$(CONFIG_HYPERV_TESTDEV) += hyperv_testdev.o
 obj-$(CONFIG_AUX) += aux.o
-obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o
+obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o aspeed_sdmc.o
diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
new file mode 100644
index 000000000000..52b6393b3f0b
--- /dev/null
+++ b/hw/misc/aspeed_sdmc.c
@@ -0,0 +1,126 @@
+/*
+ * ASPEED SDRAM Memory Controller
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/misc/aspeed_sdmc.h"
+#include "hw/qdev-properties.h"
+#include "qapi/error.h"
+#include "trace.h"
+
+/* Protection Key Register */
+#define R_PROT            (0x00 / 4)
+#define   PROT_KEY_UNLOCK     0xFC600309
+
+/* Configuration Register */
+#define R_CONF            (0x04 / 4)
+
+static uint64_t aspeed_sdmc_read(void *opaque, hwaddr addr, unsigned size)
+{
+    AspeedSDMCState *s = ASPEED_SDMC(opaque);
+
+    addr >>= 2;
+
+    if (addr >= ARRAY_SIZE(s->regs)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
+                      __func__, addr);
+        return 0;
+    }
+
+    return s->regs[addr];
+}
+
+static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,
+                             unsigned int size)
+{
+    AspeedSDMCState *s = ASPEED_SDMC(opaque);
+
+    addr >>= 2;
+
+    if (addr >= ARRAY_SIZE(s->regs)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx "\n",
+                      __func__, addr);
+        return;
+    }
+
+    if (addr != R_PROT && s->regs[R_PROT] != PROT_KEY_UNLOCK) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked!\n", __func__);
+        return;
+    }
+
+    s->regs[addr] = data;
+}
+
+static const MemoryRegionOps aspeed_sdmc_ops = {
+    .read = aspeed_sdmc_read,
+    .write = aspeed_sdmc_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid.min_access_size = 4,
+    .valid.max_access_size = 4,
+    .valid.unaligned = false,
+};
+
+static void aspeed_sdmc_reset(DeviceState *dev)
+{
+    AspeedSDMCState *s = ASPEED_SDMC(dev);
+
+    memset(s->regs, 0, sizeof(s->regs));
+    s->regs[R_CONF] = s->config;
+}
+
+static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    AspeedSDMCState *s = ASPEED_SDMC(dev);
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_sdmc_ops, s,
+                          TYPE_ASPEED_SDMC, 0x1000);
+    sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static const VMStateDescription vmstate_aspeed_sdmc = {
+    .name = "aspeed.sdmc",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(regs, AspeedSDMCState, ASPEED_SDMC_NR_REGS),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static Property aspeed_sdmc_properties[] = {
+    DEFINE_PROP_UINT32("configuration", AspeedSDMCState, config,
+                       ASPEED_SDMC_256MB),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void aspeed_sdmc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    dc->realize = aspeed_sdmc_realize;
+    dc->reset = aspeed_sdmc_reset;
+    dc->desc = "ASPEED SDRAM Memory Controller";
+    dc->vmsd = &vmstate_aspeed_sdmc;
+    dc->props = aspeed_sdmc_properties;
+}
+
+static const TypeInfo aspeed_sdmc_info = {
+    .name = TYPE_ASPEED_SDMC,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AspeedSDMCState),
+    .class_init = aspeed_sdmc_class_init,
+};
+
+static void aspeed_sdmc_register_types(void)
+{
+    type_register_static(&aspeed_sdmc_info);
+}
+
+type_init(aspeed_sdmc_register_types);
diff --git a/include/hw/arm/ast2400.h b/include/hw/arm/ast2400.h
index 7833bc716cd8..d549cceb121c 100644
--- a/include/hw/arm/ast2400.h
+++ b/include/hw/arm/ast2400.h
@@ -32,6 +32,7 @@ typedef struct AST2400State {
     AspeedSCUState scu;
     AspeedSMCState smc;
     AspeedSMCState spi;
+    AspeedSDMCState sdmc;
 } AST2400State;
 
 #define TYPE_AST2400 "ast2400"
diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
new file mode 100644
index 000000000000..65fefebdb2cd
--- /dev/null
+++ b/include/hw/misc/aspeed_sdmc.h
@@ -0,0 +1,49 @@
+/*
+ * ASPEED SDRAM Memory Controller
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later.  See
+ * the COPYING file in the top-level directory.
+ */
+#ifndef ASPEED_SDMC_H
+#define ASPEED_SDMC_H
+
+#include "hw/sysbus.h"
+
+#define TYPE_ASPEED_SDMC "aspeed.sdmc"
+#define ASPEED_SDMC(obj) OBJECT_CHECK(AspeedSDMCState, (obj), TYPE_ASPEED_SDMC)
+
+#define ASPEED_SDMC_NR_REGS (0x8 >> 2)
+
+typedef struct AspeedSDMCState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+
+    /*< public >*/
+    MemoryRegion iomem;
+
+    uint32_t regs[ASPEED_SDMC_NR_REGS];
+    uint32_t config;
+
+} AspeedSDMCState;
+
+/*
+ * For Aspeed AST2400 SOC
+ */
+#define ASPEED_SDMC_64MB    0x0
+#define ASPEED_SDMC_128MB   0x1
+#define ASPEED_SDMC_256MB   0x2
+#define ASPEED_SDMC_512MB   0x3
+
+/*
+ * For Aspeed AST2500 SOC and higher revision number
+ */
+#define ASPEED_SDMC_NEW_VERSION  (1 << 28)
+
+#define ASPEED_SDMC_NEW_128MB   (0x0 | ASPEED_SDMC_NEW_VERSION)
+#define ASPEED_SDMC_NEW_256MB   (0x1 | ASPEED_SDMC_NEW_VERSION)
+#define ASPEED_SDMC_NEW_512MB   (0x2 | ASPEED_SDMC_NEW_VERSION)
+#define ASPEED_SDMC_NEW_1024MB  (0x3 | ASPEED_SDMC_NEW_VERSION)
+
+#endif /* ASPEED_SDMC_H */
-- 
2.1.4

Re: [Qemu-devel] [PATCH 7/7] ast2400: add a memory controller device model

[Andrew Jeffery]

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

On Mon, 2016-07-04 at 14:18 +0200, Cédric Le Goater wrote:
> The uboot in the previous release of the SDK was using a hardcoded
> value for memory size. This is not true anymore, the value is now
> retrieved from the memory controller.
> 
> Below is a model for this device, only supporting unlock and
> configuration. Without it, we endup running a guest with 64MB, which
> is a bit low for Linux nowdays.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/arm/ast2400.c              |  13 +++++
>  hw/misc/Makefile.objs         |   2 +-
>  hw/misc/aspeed_sdmc.c         | 126 ++++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/ast2400.h      |   1 +
>  include/hw/misc/aspeed_sdmc.h |  49 ++++++++++++++++
>  5 files changed, 190 insertions(+), 1 deletion(-)
>  create mode 100644 hw/misc/aspeed_sdmc.c
>  create mode 100644 include/hw/misc/aspeed_sdmc.h
> 
> diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c
> index 9c30b45f87a9..fb0156ba8bc1 100644
> --- a/hw/arm/ast2400.c
> +++ b/hw/arm/ast2400.c
> @@ -27,6 +27,7 @@
>  #define AST2400_FMC_BASE         0X1E620000
>  #define AST2400_SPI_BASE         0X1E630000
>  #define AST2400_VIC_BASE         0x1E6C0000
> +#define AST2400_SDMC_BASE        0x1E6E0000
>  #define AST2400_SCU_BASE         0x1E6E2000
>  #define AST2400_TIMER_BASE       0x1E782000
>  #define AST2400_I2C_BASE         0x1E78A000
> @@ -99,6 +100,10 @@ static void ast2400_init(Object *obj)
>      object_initialize(&s->spi, sizeof(s->spi), "aspeed.smc.spi");
>      object_property_add_child(obj, "spi", OBJECT(&s->spi), NULL);
>      qdev_set_parent_bus(DEVICE(&s->spi), sysbus_get_default());
> +
> +    object_initialize(&s->sdmc, sizeof(s->sdmc), TYPE_ASPEED_SDMC);
> +    object_property_add_child(obj, "sdmc", OBJECT(&s->sdmc), NULL);
> +    qdev_set_parent_bus(DEVICE(&s->sdmc), sysbus_get_default());
>  }
>  
>  static void ast2400_realize(DeviceState *dev, Error **errp)
> @@ -184,6 +189,14 @@ static void ast2400_realize(DeviceState *dev, Error **errp)
>      }
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 0, AST2400_SPI_BASE);
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 1, AST2400_SPI_FLASH_BASE);
> +
> +    /* SDMC - SDRAM Memory Controller */
> +    object_property_set_bool(OBJECT(&s->sdmc), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdmc), 0, AST2400_SDMC_BASE);
>  }
>  
>  static void ast2400_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 54020aa06c00..d488b748c789 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -52,4 +52,4 @@ obj-$(CONFIG_PVPANIC) += pvpanic.o
>  obj-$(CONFIG_EDU) += edu.o
>  obj-$(CONFIG_HYPERV_TESTDEV) += hyperv_testdev.o
>  obj-$(CONFIG_AUX) += aux.o
> -obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o
> +obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o aspeed_sdmc.o
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> new file mode 100644
> index 000000000000..52b6393b3f0b
> --- /dev/null
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -0,0 +1,126 @@
> +/*
> + * ASPEED SDRAM Memory Controller
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/misc/aspeed_sdmc.h"
> +#include "hw/qdev-properties.h"
> +#include "qapi/error.h"
> +#include "trace.h"

We need to #include "qemu/log.h" here to avoid build failures when
alternative tracing backends are enabled. I have forgotten it a couple
of times recently.

> +
> +/* Protection Key Register */
> +#define R_PROT            (0x00 / 4)
> +#define   PROT_KEY_UNLOCK     0xFC600309
> +
> +/* Configuration Register */
> +#define R_CONF            (0x04 / 4)
> +
> +static uint64_t aspeed_sdmc_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    AspeedSDMCState *s = ASPEED_SDMC(opaque);
> +
> +    addr >>= 2;
> +
> +    if (addr >= ARRAY_SIZE(s->regs)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
> +                      __func__, addr);
> +        return 0;
> +    }
> +
> +    return s->regs[addr];
> +}
> +
> +static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,
> +                             unsigned int size)
> +{
> +    AspeedSDMCState *s = ASPEED_SDMC(opaque);
> +
> +    addr >>= 2;
> +
> +    if (addr >= ARRAY_SIZE(s->regs)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx "\n",
> +                      __func__, addr);
> +        return;
> +    }
> +
> +    if (addr != R_PROT && s->regs[R_PROT] != PROT_KEY_UNLOCK) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked!\n", __func__);
> +        return;
> +    }
> +
> +    s->regs[addr] = data;
> +}
> +
> +static const MemoryRegionOps aspeed_sdmc_ops = {
> +    .read = aspeed_sdmc_read,
> +    .write = aspeed_sdmc_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid.min_access_size = 4,
> +    .valid.max_access_size = 4,
> +    .valid.unaligned = false,
> +};
> +
> +static void aspeed_sdmc_reset(DeviceState *dev)
> +{
> +    AspeedSDMCState *s = ASPEED_SDMC(dev);
> +
> +    memset(s->regs, 0, sizeof(s->regs));
> +    s->regs[R_CONF] = s->config;
> +}
> +
> +static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    AspeedSDMCState *s = ASPEED_SDMC(dev);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_sdmc_ops, s,
> +                          TYPE_ASPEED_SDMC, 0x1000);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +}
> +
> +static const VMStateDescription vmstate_aspeed_sdmc = {
> +    .name = "aspeed.sdmc",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(regs, AspeedSDMCState, ASPEED_SDMC_NR_REGS),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static Property aspeed_sdmc_properties[] = {
> +    DEFINE_PROP_UINT32("configuration", AspeedSDMCState, config,
> +                       ASPEED_SDMC_256MB),

The datasheet's default value for configuration is 0x40, which
configures a read-only, backwards-compatible 16-bit VGA mode bit.
Should we also set this bit? Do we care?

> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void aspeed_sdmc_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    dc->realize = aspeed_sdmc_realize;
> +    dc->reset = aspeed_sdmc_reset;
> +    dc->desc = "ASPEED SDRAM Memory Controller";
> +    dc->vmsd = &vmstate_aspeed_sdmc;
> +    dc->props = aspeed_sdmc_properties;
> +}
> +
> +static const TypeInfo aspeed_sdmc_info = {
> +    .name = TYPE_ASPEED_SDMC,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(AspeedSDMCState),
> +    .class_init = aspeed_sdmc_class_init,
> +};
> +
> +static void aspeed_sdmc_register_types(void)
> +{
> +    type_register_static(&aspeed_sdmc_info);
> +}
> +
> +type_init(aspeed_sdmc_register_types);
> diff --git a/include/hw/arm/ast2400.h b/include/hw/arm/ast2400.h
> index 7833bc716cd8..d549cceb121c 100644
> --- a/include/hw/arm/ast2400.h
> +++ b/include/hw/arm/ast2400.h
> @@ -32,6 +32,7 @@ typedef struct AST2400State {
>      AspeedSCUState scu;
>      AspeedSMCState smc;
>      AspeedSMCState spi;
> +    AspeedSDMCState sdmc;

You need to #include "hw/misc/aspeed_sdmc.h" for the AspeedSDMCState
type.

>  } AST2400State;
>  
>  #define TYPE_AST2400 "ast2400"
> diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
> new file mode 100644
> index 000000000000..65fefebdb2cd
> --- /dev/null
> +++ b/include/hw/misc/aspeed_sdmc.h
> @@ -0,0 +1,49 @@
> +/*
> + * ASPEED SDRAM Memory Controller
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + */
> +#ifndef ASPEED_SDMC_H
> +#define ASPEED_SDMC_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_ASPEED_SDMC "aspeed.sdmc"
> +#define ASPEED_SDMC(obj) OBJECT_CHECK(AspeedSDMCState, (obj), TYPE_ASPEED_SDMC)
> +
> +#define ASPEED_SDMC_NR_REGS (0x8 >> 2)
> +
> +typedef struct AspeedSDMCState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
> +    MemoryRegion iomem;
> +
> +    uint32_t regs[ASPEED_SDMC_NR_REGS];
> +    uint32_t config;
> +
> +} AspeedSDMCState;
> +
> +/*
> + * For Aspeed AST2400 SOC
> + */
> +#define ASPEED_SDMC_64MB    0x0
> +#define ASPEED_SDMC_128MB   0x1
> +#define ASPEED_SDMC_256MB   0x2
> +#define ASPEED_SDMC_512MB   0x3
> +
> +/*
> + * For Aspeed AST2500 SOC and higher revision number
> + */

It might be worth noting somewhere in a comment that the AST2500
datasheet explicitly states the DRAM controller for the AST2500 is not
backwards compatible with the AST2400, but given the current
implementation it doesn't matter. If we start caring about the values
in any of the registers MCR10-MCR24, MCR34, MCR38, MCR54, MCR5C-MCR6C,
MCR80-MCR8C, then we need to be careful, i.e. test DRAM Controller
Hardware Version field in R_CONF (MCR04) to dispatch. That check needs
care as well, as the version field is 4 bits but only 2 values are
defined in the AST2500 datasheet.

> +#define ASPEED_SDMC_NEW_VERSION  (1 << 28)
> +
> +#define ASPEED_SDMC_NEW_128MB   (0x0 | ASPEED_SDMC_NEW_VERSION)
> +#define ASPEED_SDMC_NEW_256MB   (0x1 | ASPEED_SDMC_NEW_VERSION)
> +#define ASPEED_SDMC_NEW_512MB   (0x2 | ASPEED_SDMC_NEW_VERSION)
> +#define ASPEED_SDMC_NEW_1024MB  (0x3 | ASPEED_SDMC_NEW_VERSION)
> +
> +#endif /* ASPEED_SDMC_H */

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 7/7] ast2400: add a memory controller device model

[Cédric Le Goater]

On 07/06/2016 06:34 AM, Andrew Jeffery wrote:
> On Mon, 2016-07-04 at 14:18 +0200, Cédric Le Goater wrote:
>> The uboot in the previous release of the SDK was using a hardcoded
>> value for memory size. This is not true anymore, the value is now
>> retrieved from the memory controller.
>>
>> Below is a model for this device, only supporting unlock and
>> configuration. Without it, we endup running a guest with 64MB, which
>> is a bit low for Linux nowdays.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/arm/ast2400.c              |  13 +++++
>>  hw/misc/Makefile.objs         |   2 +-
>>  hw/misc/aspeed_sdmc.c         | 126 ++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/arm/ast2400.h      |   1 +
>>  include/hw/misc/aspeed_sdmc.h |  49 ++++++++++++++++
>>  5 files changed, 190 insertions(+), 1 deletion(-)
>>  create mode 100644 hw/misc/aspeed_sdmc.c
>>  create mode 100644 include/hw/misc/aspeed_sdmc.h
>>
>> diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c
>> index 9c30b45f87a9..fb0156ba8bc1 100644
>> --- a/hw/arm/ast2400.c
>> +++ b/hw/arm/ast2400.c
>> @@ -27,6 +27,7 @@
>>  #define AST2400_FMC_BASE         0X1E620000
>>  #define AST2400_SPI_BASE         0X1E630000
>>  #define AST2400_VIC_BASE         0x1E6C0000
>> +#define AST2400_SDMC_BASE        0x1E6E0000
>>  #define AST2400_SCU_BASE         0x1E6E2000
>>  #define AST2400_TIMER_BASE       0x1E782000
>>  #define AST2400_I2C_BASE         0x1E78A000
>> @@ -99,6 +100,10 @@ static void ast2400_init(Object *obj)
>>      object_initialize(&s->spi, sizeof(s->spi), "aspeed.smc.spi");
>>      object_property_add_child(obj, "spi", OBJECT(&s->spi), NULL);
>>      qdev_set_parent_bus(DEVICE(&s->spi), sysbus_get_default());
>> +
>> +    object_initialize(&s->sdmc, sizeof(s->sdmc), TYPE_ASPEED_SDMC);
>> +    object_property_add_child(obj, "sdmc", OBJECT(&s->sdmc), NULL);
>> +    qdev_set_parent_bus(DEVICE(&s->sdmc), sysbus_get_default());
>>  }
>>  
>>  static void ast2400_realize(DeviceState *dev, Error **errp)
>> @@ -184,6 +189,14 @@ static void ast2400_realize(DeviceState *dev, Error **errp)
>>      }
>>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 0, AST2400_SPI_BASE);
>>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 1, AST2400_SPI_FLASH_BASE);
>> +
>> +    /* SDMC - SDRAM Memory Controller */
>> +    object_property_set_bool(OBJECT(&s->sdmc), true, "realized", &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdmc), 0, AST2400_SDMC_BASE);
>>  }
>>  
>>  static void ast2400_class_init(ObjectClass *oc, void *data)
>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> index 54020aa06c00..d488b748c789 100644
>> --- a/hw/misc/Makefile.objs
>> +++ b/hw/misc/Makefile.objs
>> @@ -52,4 +52,4 @@ obj-$(CONFIG_PVPANIC) += pvpanic.o
>>  obj-$(CONFIG_EDU) += edu.o
>>  obj-$(CONFIG_HYPERV_TESTDEV) += hyperv_testdev.o
>>  obj-$(CONFIG_AUX) += aux.o
>> -obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o
>> +obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o aspeed_sdmc.o
>> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
>> new file mode 100644
>> index 000000000000..52b6393b3f0b
>> --- /dev/null
>> +++ b/hw/misc/aspeed_sdmc.c
>> @@ -0,0 +1,126 @@
>> +/*
>> + * ASPEED SDRAM Memory Controller
>> + *
>> + * Copyright 2016 IBM Corp.
>> + *
>> + * This code is licensed under the GPL version 2 or later.  See
>> + * the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/misc/aspeed_sdmc.h"
>> +#include "hw/qdev-properties.h"
>> +#include "qapi/error.h"
>> +#include "trace.h"
> 
> We need to #include "qemu/log.h" here to avoid build failures when
> alternative tracing backends are enabled. I have forgotten it a couple
> of times recently.

yes. Thanks.

>> +
>> +/* Protection Key Register */
>> +#define R_PROT            (0x00 / 4)
>> +#define   PROT_KEY_UNLOCK     0xFC600309
>> +
>> +/* Configuration Register */
>> +#define R_CONF            (0x04 / 4)
>> +
>> +static uint64_t aspeed_sdmc_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    AspeedSDMCState *s = ASPEED_SDMC(opaque);
>> +
>> +    addr >>= 2;
>> +
>> +    if (addr >= ARRAY_SIZE(s->regs)) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
>> +                      __func__, addr);
>> +        return 0;
>> +    }
>> +
>> +    return s->regs[addr];
>> +}
>> +
>> +static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,
>> +                             unsigned int size)
>> +{
>> +    AspeedSDMCState *s = ASPEED_SDMC(opaque);
>> +
>> +    addr >>= 2;
>> +
>> +    if (addr >= ARRAY_SIZE(s->regs)) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx "\n",
>> +                      __func__, addr);
>> +        return;
>> +    }
>> +
>> +    if (addr != R_PROT && s->regs[R_PROT] != PROT_KEY_UNLOCK) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked!\n", __func__);
>> +        return;
>> +    }
>> +
>> +    s->regs[addr] = data;
>> +}
>> +
>> +static const MemoryRegionOps aspeed_sdmc_ops = {
>> +    .read = aspeed_sdmc_read,
>> +    .write = aspeed_sdmc_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .valid.min_access_size = 4,
>> +    .valid.max_access_size = 4,
>> +    .valid.unaligned = false,
>> +};
>> +
>> +static void aspeed_sdmc_reset(DeviceState *dev)
>> +{
>> +    AspeedSDMCState *s = ASPEED_SDMC(dev);
>> +
>> +    memset(s->regs, 0, sizeof(s->regs));
>> +    s->regs[R_CONF] = s->config;
>> +}
>> +
>> +static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
>> +{
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> +    AspeedSDMCState *s = ASPEED_SDMC(dev);
>> +
>> +    memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_sdmc_ops, s,
>> +                          TYPE_ASPEED_SDMC, 0x1000);
>> +    sysbus_init_mmio(sbd, &s->iomem);
>> +}
>> +
>> +static const VMStateDescription vmstate_aspeed_sdmc = {
>> +    .name = "aspeed.sdmc",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32_ARRAY(regs, AspeedSDMCState, ASPEED_SDMC_NR_REGS),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static Property aspeed_sdmc_properties[] = {
>> +    DEFINE_PROP_UINT32("configuration", AspeedSDMCState, config,
>> +                       ASPEED_SDMC_256MB),
> 
> The datasheet's default value for configuration is 0x40, which
> configures a read-only, backwards-compatible 16-bit VGA mode bit.
> Should we also set this bit? Do we care?

I think we should add the definitions of all the bits of the 
registers we use. I will add the 16-bit VGA mode bit in the 
next version.

>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void aspeed_sdmc_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    dc->realize = aspeed_sdmc_realize;
>> +    dc->reset = aspeed_sdmc_reset;
>> +    dc->desc = "ASPEED SDRAM Memory Controller";
>> +    dc->vmsd = &vmstate_aspeed_sdmc;
>> +    dc->props = aspeed_sdmc_properties;
>> +}
>> +
>> +static const TypeInfo aspeed_sdmc_info = {
>> +    .name = TYPE_ASPEED_SDMC,
>> +    .parent = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(AspeedSDMCState),
>> +    .class_init = aspeed_sdmc_class_init,
>> +};
>> +
>> +static void aspeed_sdmc_register_types(void)
>> +{
>> +    type_register_static(&aspeed_sdmc_info);
>> +}
>> +
>> +type_init(aspeed_sdmc_register_types);
>> diff --git a/include/hw/arm/ast2400.h b/include/hw/arm/ast2400.h
>> index 7833bc716cd8..d549cceb121c 100644
>> --- a/include/hw/arm/ast2400.h
>> +++ b/include/hw/arm/ast2400.h
>> @@ -32,6 +32,7 @@ typedef struct AST2400State {
>>      AspeedSCUState scu;
>>      AspeedSMCState smc;
>>      AspeedSMCState spi;
>> +    AspeedSDMCState sdmc;
> 
> You need to #include "hw/misc/aspeed_sdmc.h" for the AspeedSDMCState
> type.

yes ... that was a bad refresh of the patch before sending.

>>  } AST2400State;
>>  
>>  #define TYPE_AST2400 "ast2400"
>> diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
>> new file mode 100644
>> index 000000000000..65fefebdb2cd
>> --- /dev/null
>> +++ b/include/hw/misc/aspeed_sdmc.h
>> @@ -0,0 +1,49 @@
>> +/*
>> + * ASPEED SDRAM Memory Controller
>> + *
>> + * Copyright 2016 IBM Corp.
>> + *
>> + * This code is licensed under the GPL version 2 or later.  See
>> + * the COPYING file in the top-level directory.
>> + */
>> +#ifndef ASPEED_SDMC_H
>> +#define ASPEED_SDMC_H
>> +
>> +#include "hw/sysbus.h"
>> +
>> +#define TYPE_ASPEED_SDMC "aspeed.sdmc"
>> +#define ASPEED_SDMC(obj) OBJECT_CHECK(AspeedSDMCState, (obj), TYPE_ASPEED_SDMC)
>> +
>> +#define ASPEED_SDMC_NR_REGS (0x8 >> 2)
>> +
>> +typedef struct AspeedSDMCState {
>> +    /*< private >*/
>> +    SysBusDevice parent_obj;
>> +
>> +    /*< public >*/
>> +    MemoryRegion iomem;
>> +
>> +    uint32_t regs[ASPEED_SDMC_NR_REGS];
>> +    uint32_t config;
>> +
>> +} AspeedSDMCState;
>> +
>> +/*
>> + * For Aspeed AST2400 SOC
>> + */
>> +#define ASPEED_SDMC_64MB    0x0
>> +#define ASPEED_SDMC_128MB   0x1
>> +#define ASPEED_SDMC_256MB   0x2
>> +#define ASPEED_SDMC_512MB   0x3
>> +
>> +/*
>> + * For Aspeed AST2500 SOC and higher revision number
>> + */
> 
> It might be worth noting somewhere in a comment that the AST2500
> datasheet explicitly states the DRAM controller for the AST2500 is not
> backwards compatible with the AST2400, but given the current
> implementation it doesn't matter. 

yes. I will add that.

> If we start caring about the values
> in any of the registers MCR10-MCR24, MCR34, MCR38, MCR54, MCR5C-MCR6C,
> MCR80-MCR8C, then we need to be careful, i.e. test DRAM Controller
> Hardware Version field in R_CONF (MCR04) to dispatch. That check needs
> care as well, as the version field is 4 bits but only 2 values are
> defined in the AST2500 datasheet.
> 
>> +#define ASPEED_SDMC_NEW_VERSION  (1 << 28)
>> +
>> +#define ASPEED_SDMC_NEW_128MB   (0x0 | ASPEED_SDMC_NEW_VERSION)
>> +#define ASPEED_SDMC_NEW_256MB   (0x1 | ASPEED_SDMC_NEW_VERSION)
>> +#define ASPEED_SDMC_NEW_512MB   (0x2 | ASPEED_SDMC_NEW_VERSION)
>> +#define ASPEED_SDMC_NEW_1024MB  (0x3 | ASPEED_SDMC_NEW_VERSION)
>> +
>> +#endif /* ASPEED_SDMC_H */
> 
> Cheers,

Thanks for the review, this patch needs another spin.

C.