[Qemu-devel] [RFC v3 0/2] Add NRF51 SOC non-volatile memory controller

[Qemu-devel] [RFC v3 0/2] Add NRF51 SOC non-volatile memory controller

[Steffen Görtz]

Add some non-volatile memories and a non-volatile
memory controller for the nRF51.
Furthermore, a testsuite for the bbc:microbit and
nrf51 soc was added.

Examination of the real device showed that
NVMs remained unchanged when the write/erase enabled
bits are not set in the controller, so we can
safely ignore all writes.
More: https://github.com/douzepouze/gsoc18-qemu/blob/master/notes.md#test-nvmc-behavior-out-of-micropython-repl

The CODE/FLASH NVM is not currently included in this
peripheral. It is hosted in the SOC and must be read-only
to provide an accurate model.

Steffen Görtz (2):
  arm: Add NRF51 SOC non-volatile memory controller
  tests: Add bbc:microbit / nRF51 test suite

 hw/nvram/Makefile.objs       |   1 +
 hw/nvram/nrf51_nvm.c         | 401 +++++++++++++++++++++++++++++++++++
 include/hw/arm/nrf51_soc.h   |   2 +-
 include/hw/nvram/nrf51_nvm.h |  56 +++++
 tests/Makefile.include       |   2 +
 tests/microbit-test.c        | 118 +++++++++++
 6 files changed, 579 insertions(+), 1 deletion(-)
 create mode 100644 hw/nvram/nrf51_nvm.c
 create mode 100644 include/hw/nvram/nrf51_nvm.h
 create mode 100644 tests/microbit-test.c

-- 
2.18.0

[Qemu-devel] [RFC v3 1/2] arm: Add NRF51 SOC non-volatile memory controller

[Steffen Görtz]

Changes in V3:
- NVMs consolidated in one file
- Removed bitfields
- Add VMState
- Add reset

Changes in V1/V2:
- Code style changes

Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
---
 hw/nvram/Makefile.objs       |   1 +
 hw/nvram/nrf51_nvm.c         | 401 +++++++++++++++++++++++++++++++++++
 include/hw/arm/nrf51_soc.h   |   2 +-
 include/hw/nvram/nrf51_nvm.h |  56 +++++
 4 files changed, 459 insertions(+), 1 deletion(-)
 create mode 100644 hw/nvram/nrf51_nvm.c
 create mode 100644 include/hw/nvram/nrf51_nvm.h

diff --git a/hw/nvram/Makefile.objs b/hw/nvram/Makefile.objs
index a912d25391..3f978e6212 100644
--- a/hw/nvram/Makefile.objs
+++ b/hw/nvram/Makefile.objs
@@ -5,3 +5,4 @@ common-obj-y += fw_cfg.o
 common-obj-y += chrp_nvram.o
 common-obj-$(CONFIG_MAC_NVRAM) += mac_nvram.o
 obj-$(CONFIG_PSERIES) += spapr_nvram.o
+obj-$(CONFIG_NRF51_SOC) += nrf51_nvm.o
diff --git a/hw/nvram/nrf51_nvm.c b/hw/nvram/nrf51_nvm.c
new file mode 100644
index 0000000000..285c70adee
--- /dev/null
+++ b/hw/nvram/nrf51_nvm.c
@@ -0,0 +1,401 @@
+/*
+ * Nordic Semiconductor nRF51 non-volatile memory
+ *
+ * It provides an interface to erase regions in flash memory.
+ * Furthermore it provides the user and factory information registers.
+ *
+ * Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
+ *
+ * See nRF51 reference manual and product sheet sections:
+ * + Non-Volatile Memory Controller (NVMC)
+ * + Factory Information Configuration Registers (FICR)
+ * + User Information Configuration Registers (UICR)
+ *
+ * Copyright 2018 Steffen Görtz <contrib@steffen-goertz.de>
+ *
+ * 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 "qapi/error.h"
+#include "qemu/log.h"
+#include "exec/address-spaces.h"
+#include "hw/nvram/nrf51_nvm.h"
+
+#define NRF51_NVMC_SIZE         0x1000
+
+#define NRF51_NVMC_READY        0x400
+#define NRF51_NVMC_READY_READY  0x01
+#define NRF51_NVMC_CONFIG       0x504
+#define NRF51_NVMC_CONFIG_MASK  0x03
+#define NRF51_NVMC_CONFIG_WEN   0x01
+#define NRF51_NVMC_CONFIG_EEN   0x02
+#define NRF51_NVMC_ERASEPCR1    0x508
+#define NRF51_NVMC_ERASEPCR0    0x510
+#define NRF51_NVMC_ERASEALL     0x50C
+#define NRF51_NVMC_ERASEUICR    0x514
+#define NRF51_NVMC_ERASE        0x01
+
+#define NRF51_FICR_SIZE         0x100
+
+#define NRF51_UICR_SIZE         0x100
+
+
+/* FICR Registers Assignments
+CODEPAGESIZE      0x010
+CODESIZE          0x014
+CLENR0            0x028
+PPFC              0x02C
+NUMRAMBLOCK       0x034
+SIZERAMBLOCKS     0x038
+SIZERAMBLOCK[0]   0x038
+SIZERAMBLOCK[1]   0x03C
+SIZERAMBLOCK[2]   0x040
+SIZERAMBLOCK[3]   0x044
+CONFIGID          0x05C
+DEVICEID[0]       0x060
+DEVICEID[1]       0x064
+ER[0]             0x080
+ER[1]             0x084
+ER[2]             0x088
+ER[3]             0x08C
+IR[0]             0x090
+IR[1]             0x094
+IR[2]             0x098
+IR[3]             0x09C
+DEVICEADDRTYPE    0x0A0
+DEVICEADDR[0]     0x0A4
+DEVICEADDR[1]     0x0A8
+OVERRIDEEN        0x0AC
+NRF_1MBIT[0]      0x0B0
+NRF_1MBIT[1]      0x0B4
+NRF_1MBIT[2]      0x0B8
+NRF_1MBIT[3]      0x0BC
+NRF_1MBIT[4]      0x0C0
+BLE_1MBIT[0]      0x0EC
+BLE_1MBIT[1]      0x0F0
+BLE_1MBIT[2]      0x0F4
+BLE_1MBIT[3]      0x0F8
+BLE_1MBIT[4]      0x0FC
+*/
+static const uint32_t ficr_content[64] = {
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0x00000400,
+        0x00000100, 0xFFFFFFFF, 0xFFFFFFFF, 0x00000002, 0x00002000,
+        0x00002000, 0x00002000, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0x00000003,
+        0x12345678, 0x9ABCDEF1, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF
+};
+
+static uint64_t ficr_read(void *opaque, hwaddr offset, unsigned int size)
+{
+    if (offset > (ARRAY_SIZE(ficr_content) - size)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                "%s: bad read offset 0x%" HWADDR_PRIx "\n", __func__, offset);
+        return 0;
+    }
+
+    return ficr_content[offset >> 2];
+}
+
+static const MemoryRegionOps ficr_ops = {
+    .read = ficr_read,
+    .impl.min_access_size = 4,
+    .impl.max_access_size = 4,
+    .impl.unaligned = false,
+};
+
+/* UICR Registers Assignments
+CLENR0           0x000
+RBPCONF          0x004
+XTALFREQ         0x008
+FWID             0x010
+BOOTLOADERADDR   0x014
+NRFFW[0]         0x014
+NRFFW[1]         0x018
+NRFFW[2]         0x01C
+NRFFW[3]         0x020
+NRFFW[4]         0x024
+NRFFW[5]         0x028
+NRFFW[6]         0x02C
+NRFFW[7]         0x030
+NRFFW[8]         0x034
+NRFFW[9]         0x038
+NRFFW[10]        0x03C
+NRFFW[11]        0x040
+NRFFW[12]        0x044
+NRFFW[13]        0x048
+NRFFW[14]        0x04C
+NRFHW[0]         0x050
+NRFHW[1]         0x054
+NRFHW[2]         0x058
+NRFHW[3]         0x05C
+NRFHW[4]         0x060
+NRFHW[5]         0x064
+NRFHW[6]         0x068
+NRFHW[7]         0x06C
+NRFHW[8]         0x070
+NRFHW[9]         0x074
+NRFHW[10]        0x078
+NRFHW[11]        0x07C
+CUSTOMER[0]      0x080
+CUSTOMER[1]      0x084
+CUSTOMER[2]      0x088
+CUSTOMER[3]      0x08C
+CUSTOMER[4]      0x090
+CUSTOMER[5]      0x094
+CUSTOMER[6]      0x098
+CUSTOMER[7]      0x09C
+CUSTOMER[8]      0x0A0
+CUSTOMER[9]      0x0A4
+CUSTOMER[10]     0x0A8
+CUSTOMER[11]     0x0AC
+CUSTOMER[12]     0x0B0
+CUSTOMER[13]     0x0B4
+CUSTOMER[14]     0x0B8
+CUSTOMER[15]     0x0BC
+CUSTOMER[16]     0x0C0
+CUSTOMER[17]     0x0C4
+CUSTOMER[18]     0x0C8
+CUSTOMER[19]     0x0CC
+CUSTOMER[20]     0x0D0
+CUSTOMER[21]     0x0D4
+CUSTOMER[22]     0x0D8
+CUSTOMER[23]     0x0DC
+CUSTOMER[24]     0x0E0
+CUSTOMER[25]     0x0E4
+CUSTOMER[26]     0x0E8
+CUSTOMER[27]     0x0EC
+CUSTOMER[28]     0x0F0
+CUSTOMER[29]     0x0F4
+CUSTOMER[30]     0x0F8
+CUSTOMER[31]     0x0FC
+*/
+
+static const uint32_t uicr_fixture[NRF51_UICR_FIXTURE_SIZE] = {
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+        0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF
+};
+
+static uint64_t uicr_read(void *opaque, hwaddr offset, unsigned int size)
+{
+    Nrf51NVMState *s = NRF51_NVM(opaque);
+
+    offset >>= 2;
+
+    if (offset >= ARRAY_SIZE(s->uicr_content)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                "%s: bad read offset 0x%" HWADDR_PRIx "\n", __func__, offset);
+        return 0;
+    }
+
+    return s->uicr_content[offset];
+}
+
+static void uicr_write(void *opaque, hwaddr offset, uint64_t value,
+        unsigned int size)
+{
+    Nrf51NVMState *s = NRF51_NVM(opaque);
+
+    offset >>= 2;
+
+    if (offset >= ARRAY_SIZE(s->uicr_content)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                "%s: bad read offset 0x%" HWADDR_PRIx "\n", __func__, offset);
+        return;
+    }
+
+    s->uicr_content[offset] = value;
+}
+
+static const MemoryRegionOps uicr_ops = {
+    .read = uicr_read,
+    .write = uicr_write,
+    .impl.min_access_size = 4,
+    .impl.max_access_size = 4,
+    .impl.unaligned = false,
+};
+
+
+static uint64_t io_read(void *opaque, hwaddr offset, unsigned int size)
+{
+    Nrf51NVMState *s = NRF51_NVM(opaque);
+    uint64_t r = 0;
+
+    switch (offset) {
+    case NRF51_NVMC_READY:
+        r = NRF51_NVMC_READY_READY;
+        break;
+    case NRF51_NVMC_CONFIG:
+        r = s->config;
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                "%s: bad read offset 0x%" HWADDR_PRIx "\n", __func__, offset);
+    }
+
+    return r;
+}
+
+static void io_write(void *opaque, hwaddr offset, uint64_t value,
+        unsigned int size)
+{
+    Nrf51NVMState *s = NRF51_NVM(opaque);
+
+    switch (offset) {
+    case NRF51_NVMC_CONFIG:
+        s->config = value & NRF51_NVMC_CONFIG_MASK;
+        break;
+    case NRF51_NVMC_ERASEPCR0:
+    case NRF51_NVMC_ERASEPCR1:
+        value &= ~(s->page_size - 1);
+        if (value < (s->code_size * s->page_size)) {
+            address_space_write(&s->as, value, MEMTXATTRS_UNSPECIFIED,
+                    s->empty_page, s->page_size);
+        }
+        break;
+    case NRF51_NVMC_ERASEALL:
+        if (value == NRF51_NVMC_ERASE) {
+            for (uint32_t i = 0; i < s->code_size; i++) {
+                address_space_write(&s->as, i * s->page_size,
+                MEMTXATTRS_UNSPECIFIED, s->empty_page, s->page_size);
+            }
+            memset(s->uicr_content, 0xFF, sizeof(s->uicr_content));
+        }
+        break;
+    case NRF51_NVMC_ERASEUICR:
+        if (value == NRF51_NVMC_ERASE) {
+            memset(s->uicr_content, 0xFF, sizeof(s->uicr_content));
+        }
+        break;
+
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                "%s: bad write offset 0x%" HWADDR_PRIx "\n", __func__, offset);
+    }
+}
+
+static const MemoryRegionOps io_ops = {
+        .read = io_read,
+        .write = io_write,
+        .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void nrf51_nvm_init(Object *obj)
+{
+    Nrf51NVMState *s = NRF51_NVM(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+
+    memory_region_init_io(&s->mmio, obj, &io_ops, s, "nrf51_soc.nvmc",
+            NRF51_NVMC_SIZE);
+    sysbus_init_mmio(sbd, &s->mmio);
+
+    memory_region_init_io(&s->ficr, NULL, &ficr_ops, s, "nrf51_soc.ficr",
+            NRF51_FICR_SIZE);
+    memory_region_set_readonly(&s->ficr, true);
+    sysbus_init_mmio(sbd, &s->ficr);
+
+    memcpy(s->uicr_content, uicr_fixture, sizeof(s->uicr_content));
+    memory_region_init_io(&s->uicr, NULL, &uicr_ops, s, "nrf51_soc.uicr",
+            NRF51_UICR_SIZE);
+    sysbus_init_mmio(sbd, &s->uicr);
+}
+
+static void nrf51_nvm_realize(DeviceState *dev, Error **errp)
+{
+    Nrf51NVMState *s = NRF51_NVM(dev);
+
+    if (!s->mr) {
+        error_setg(errp, "memory property was not set");
+        return;
+    }
+
+    if (s->page_size < NRF51_UICR_SIZE) {
+        error_setg(errp, "page size too small");
+        return;
+    }
+
+    address_space_init(&s->as, s->mr, "system-memory");
+}
+
+static void nrf51_nvm_unrealize(DeviceState *dev, Error **errp)
+{
+    Nrf51NVMState *s = NRF51_NVM(dev);
+
+    g_free(s->empty_page);
+    s->empty_page = NULL;
+}
+
+static void nrf51_nvm_reset(DeviceState *dev)
+{
+    Nrf51NVMState *s = NRF51_NVM(dev);
+
+    s->empty_page = g_malloc(s->page_size);
+    memset(s->empty_page, 0xFF, s->page_size);
+}
+
+
+static Property nrf51_nvm_properties[] = {
+    DEFINE_PROP_UINT16("page_size", Nrf51NVMState, page_size, 0x400),
+    DEFINE_PROP_UINT32("code_size", Nrf51NVMState, code_size, 0x100),
+    DEFINE_PROP_LINK("memory", Nrf51NVMState, mr, TYPE_MEMORY_REGION,
+                     MemoryRegion *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static const VMStateDescription vmstate_nvm = {
+    .name = "nrf51_soc.nvm",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(uicr_content, Nrf51NVMState,
+                NRF51_UICR_FIXTURE_SIZE),
+        VMSTATE_UINT32(config, Nrf51NVMState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void nrf51_nvm_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->props = nrf51_nvm_properties;
+    dc->vmsd = &vmstate_nvm;
+    dc->realize = nrf51_nvm_realize;
+    dc->unrealize = nrf51_nvm_unrealize;
+    dc->reset = nrf51_nvm_reset;
+}
+
+static const TypeInfo nrf51_nvm_info = {
+    .name = TYPE_NRF51_NVM,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(Nrf51NVMState),
+    .instance_init = nrf51_nvm_init,
+    .class_init = nrf51_nvm_class_init
+};
+
+static void nrf51_nvm_register_types(void)
+{
+    type_register_static(&nrf51_nvm_info);
+}
+
+type_init(nrf51_nvm_register_types)
diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
index 35dd71c3db..9c32d22abe 100644
--- a/include/hw/arm/nrf51_soc.h
+++ b/include/hw/arm/nrf51_soc.h
@@ -1,5 +1,5 @@
 /*
- * Nordic Semiconductor nRF51  SoC
+ * Nordic Semiconductor nRF51 SoC
  *
  * Copyright 2018 Joel Stanley <joel@jms.id.au>
  *
diff --git a/include/hw/nvram/nrf51_nvm.h b/include/hw/nvram/nrf51_nvm.h
new file mode 100644
index 0000000000..3d018de049
--- /dev/null
+++ b/include/hw/nvram/nrf51_nvm.h
@@ -0,0 +1,56 @@
+/*
+ * Nordic Semiconductor nRF51 non-volatile memory
+ *
+ * It provides an interface to erase regions in flash memory.
+ * Furthermore it provides the user and factory information registers.
+ *
+ * QEMU interface:
+ * + sysbus MMIO regions 0: NVMC peripheral registers
+ * + sysbus MMIO regions 1: FICR peripheral registers
+ * + sysbus MMIO regions 2: UICR peripheral registers
+ * + page_size property to set the page size in bytes.
+ * + code_size property to set the code size in number of pages.
+ *
+ * Accuracy of the peripheral model:
+ * + The NVMC is always ready, all requested erase operations succeed
+ *   immediately.
+ * + CONFIG.WEN and CONFIG.EEN flags can be written and read back
+ *   but are not evaluated to check whether a requested write/erase operation
+ *   is legal.
+ * + Code regions (MPU configuration) are disregarded.
+ *
+ * Copyright 2018 Steffen Görtz <contrib@steffen-goertz.de>
+ *
+ * This code is licensed under the GPL version 2 or later.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+#ifndef NRF51_NVM_H
+#define NRF51_NVM_H
+
+#include "hw/sysbus.h"
+#define TYPE_NRF51_NVM "nrf51_soc.nvm"
+#define NRF51_NVM(obj) OBJECT_CHECK(Nrf51NVMState, (obj), TYPE_NRF51_NVM)
+
+#define NRF51_UICR_FIXTURE_SIZE 64
+
+typedef struct Nrf51NVMState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion mmio;
+    MemoryRegion ficr;
+    MemoryRegion uicr;
+
+    uint32_t uicr_content[NRF51_UICR_FIXTURE_SIZE];
+    uint32_t code_size;
+    uint16_t page_size;
+    uint8_t *empty_page;
+    MemoryRegion *mr;
+    AddressSpace as;
+
+    uint32_t config;
+
+} Nrf51NVMState;
+
+
+#endif
-- 
2.18.0

[Qemu-devel] [RFC v3 2/2] tests: Add bbc:microbit / nRF51 test suite

[Steffen Görtz]

The microbit-test includes tests for the nRF51 NVMC
peripheral and will host future nRF51 peripheral tests
and board-level bbc:microbit tests.

Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
---
 tests/Makefile.include |   2 +
 tests/microbit-test.c  | 118 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 120 insertions(+)
 create mode 100644 tests/microbit-test.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3c8bde4f90..622da9288f 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -373,6 +373,7 @@ check-qtest-sparc64-y += tests/boot-serial-test$(EXESUF)
 
 check-qtest-arm-y = tests/tmp105-test$(EXESUF)
 check-qtest-arm-y += tests/ds1338-test$(EXESUF)
+check-qtest-arm-y += tests/microbit-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)
@@ -778,6 +779,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/microbit-test$(EXESUF): tests/microbit-test.o
 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)
diff --git a/tests/microbit-test.c b/tests/microbit-test.c
new file mode 100644
index 0000000000..c502ee3976
--- /dev/null
+++ b/tests/microbit-test.c
@@ -0,0 +1,118 @@
+ /*
+ * QTest testcase for Microbit board using the Nordic Semiconductor nRF51 SoC.
+ *
+ *
+ * Copyright (c) 2018 Steffen Görtz <contrib@steffen-goertz.de>
+ */
+
+
+#include "qemu/osdep.h"
+#include "exec/hwaddr.h"
+#include "libqtest.h"
+
+
+#define PAGE_SIZE           1024
+#define FLASH_SIZE          (256 * PAGE_SIZE)
+#define FLASH_BASE          0x00000000
+#define UICR_BASE           0x10001000
+#define UICR_SIZE           0x100
+#define NVMC_BASE           0x4001E000UL
+#define NVMC_READY          0x400
+#define NVMC_CONFIG         0x504
+#define NVMC_ERASEPAGE      0x508
+#define NVMC_ERASEPCR1      0x508
+#define NVMC_ERASEALL       0x50C
+#define NVMC_ERASEPCR0      0x510
+#define NVMC_ERASEUICR      0x514
+
+
+static void fill_and_erase(hwaddr base, hwaddr size, uint32_t address_reg)
+{
+    /* Fill memory */
+    writel(NVMC_BASE + NVMC_CONFIG, 0x01);
+    for (hwaddr i = 0; i < size; ++i) {
+        writeb(base + i, i);
+        g_assert_cmpuint(readb(base + i), ==, i & 0xFF);
+    }
+    writel(NVMC_BASE + NVMC_CONFIG, 0x00);
+
+    /* Erase Page */
+    writel(NVMC_BASE + NVMC_CONFIG, 0x02);
+    writel(NVMC_BASE + address_reg, base);
+    writel(NVMC_BASE + NVMC_CONFIG, 0x00);
+
+    /* Check memory */
+    for (hwaddr i = 0; i < size; ++i) {
+        g_assert_cmpuint(readb(base + i), ==, 0xFF);
+    }
+}
+
+static void test_nrf51_nvmc(void)
+{
+    uint32_t value;
+    /* Test always ready */
+    value = readl(NVMC_BASE + NVMC_READY);
+    g_assert_cmpuint(value & 0x01, ==, 0x01);
+
+    /* Test write-read config register */
+    writel(NVMC_BASE + NVMC_CONFIG, 0x03);
+    g_assert_cmpuint(readl(NVMC_BASE + NVMC_CONFIG), ==, 0x03);
+    writel(NVMC_BASE + NVMC_CONFIG, 0x00);
+    g_assert_cmpuint(readl(NVMC_BASE + NVMC_CONFIG), ==, 0x00);
+
+    /* Test PCR0 */
+    fill_and_erase(FLASH_BASE, PAGE_SIZE, NVMC_ERASEPCR0);
+    fill_and_erase(FLASH_BASE + PAGE_SIZE, PAGE_SIZE, NVMC_ERASEPCR0);
+
+    /* Test PCR1 */
+    fill_and_erase(FLASH_BASE, PAGE_SIZE, NVMC_ERASEPCR1);
+    fill_and_erase(FLASH_BASE + PAGE_SIZE, PAGE_SIZE, NVMC_ERASEPCR1);
+
+    /* Erase all */
+    writel(NVMC_BASE + NVMC_CONFIG, 0x01);
+    for (hwaddr i = 0; i < FLASH_SIZE / 4; i++) {
+        writel(FLASH_BASE + i * 4, i);
+        g_assert_cmpuint(readl(FLASH_BASE + i * 4), ==, i);
+    }
+    writel(NVMC_BASE + NVMC_CONFIG, 0x00);
+
+    writel(NVMC_BASE + NVMC_CONFIG, 0x02);
+    writel(NVMC_BASE + NVMC_ERASEALL, 0x01);
+    writel(NVMC_BASE + NVMC_CONFIG, 0x00);
+
+    for (hwaddr i = 0; i < FLASH_SIZE / 4; i++) {
+        g_assert_cmpuint(readl(FLASH_BASE + i * 4), ==, 0xFFFFFFFF);
+    }
+
+    /* Erase UICR */
+    writel(NVMC_BASE + NVMC_CONFIG, 0x01);
+    for (hwaddr i = 0; i < UICR_SIZE / 4; i++) {
+        writel(UICR_BASE + i * 4, i);
+        g_assert_cmpuint(readl(UICR_BASE + i * 4), ==, i);
+    }
+    writel(NVMC_BASE + NVMC_CONFIG, 0x00);
+
+    writel(NVMC_BASE + NVMC_CONFIG, 0x02);
+    writel(NVMC_BASE + NVMC_ERASEUICR, 0x01);
+    writel(NVMC_BASE + NVMC_CONFIG, 0x00);
+
+    for (hwaddr i = 0; i < UICR_SIZE / 4; i++) {
+        g_assert_cmpuint(readl(UICR_BASE + i * 4), ==, 0xFFFFFFFF);
+    }
+}
+
+int main(int argc, char **argv)
+{
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+
+    global_qtest = qtest_startf("-machine microbit");
+
+    qtest_add_func("/microbit/nrf51/nvmc", test_nrf51_nvmc);
+
+    ret = g_test_run();
+
+    qtest_quit(global_qtest);
+    return ret;
+}
-- 
2.18.0

Re: [Qemu-devel] [RFC v3 0/2] Add NRF51 SOC non-volatile memory controller

[Stefan Hajnoczi]

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

On Thu, Jul 12, 2018 at 12:12:17PM +0200, Steffen Görtz wrote:
> Add some non-volatile memories and a non-volatile
> memory controller for the nRF51.
> Furthermore, a testsuite for the bbc:microbit and
> nrf51 soc was added.
> 
> Examination of the real device showed that
> NVMs remained unchanged when the write/erase enabled
> bits are not set in the controller, so we can
> safely ignore all writes.
> More: https://github.com/douzepouze/gsoc18-qemu/blob/master/notes.md#test-nvmc-behavior-out-of-micropython-repl
> 
> The CODE/FLASH NVM is not currently included in this
> peripheral. It is hosted in the SOC and must be read-only
> to provide an accurate model.
> 
> Steffen Görtz (2):
>   arm: Add NRF51 SOC non-volatile memory controller
>   tests: Add bbc:microbit / nRF51 test suite

Please include a changelog in the cover letter so that it's clear what
has changed in this revision.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [RFC v3 1/2] arm: Add NRF51 SOC non-volatile memory controller

[Stefan Hajnoczi]

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

On Thu, Jul 12, 2018 at 12:12:18PM +0200, Steffen Görtz wrote:
> Changes in V3:
> - NVMs consolidated in one file
> - Removed bitfields
> - Add VMState
> - Add reset
> 
> Changes in V1/V2:
> - Code style changes
> 
> Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
> ---

Everything above '---' goes into the commit description and is
permanently in the git log.  Changelogs are not useful in the git log
since their context (the earlier revisions and the discussion) is
missing.  Please put changelogs below '---' as anything there doesn't
get included in the commit description.

> +static uint64_t ficr_read(void *opaque, hwaddr offset, unsigned int size)
> +{
> +    if (offset > (ARRAY_SIZE(ficr_content) - size)) {

Bug alert, the semantic types look suspicious:

  bytes_t > num_elements_t - bytes_t

You cannot subtract a byte count from the number of elements in an
array.

Did you mean:

  if (offset >= sizeof(ficr_content)) {

MemoryRegion was already declared with FICR_SIZE length, so nothing
should ever call ficr_read() with an out-of-bounds offset.

Instead of defining FICR_SIZE, which could become out-of-sync with the
definition of ficr_content[], I would use sizeof(ficr_content) as the
MemoryRegion length.  This way the code is guaranteed to be safe even if
the size of ficr_content[] is changed in the future.  No if statement
would be necessary.

> +static uint64_t uicr_read(void *opaque, hwaddr offset, unsigned int size)
> +{
> +    Nrf51NVMState *s = NRF51_NVM(opaque);
> +
> +    offset >>= 2;
> +
> +    if (offset >= ARRAY_SIZE(s->uicr_content)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                "%s: bad read offset 0x%" HWADDR_PRIx "\n", __func__, offset);
> +        return 0;
> +    }

The same applies to NRF51_UICR_SIZE.  It's simpler to use
sizeof(s->uicr_content) instead of defining a separate constant and then
using an if statement to check the offset.

> +static void nrf51_nvm_init(Object *obj)
> +{
> +    Nrf51NVMState *s = NRF51_NVM(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +
> +    memory_region_init_io(&s->mmio, obj, &io_ops, s, "nrf51_soc.nvmc",
> +            NRF51_NVMC_SIZE);
> +    sysbus_init_mmio(sbd, &s->mmio);
> +
> +    memory_region_init_io(&s->ficr, NULL, &ficr_ops, s, "nrf51_soc.ficr",
> +            NRF51_FICR_SIZE);
> +    memory_region_set_readonly(&s->ficr, true);
> +    sysbus_init_mmio(sbd, &s->ficr);
> +
> +    memcpy(s->uicr_content, uicr_fixture, sizeof(s->uicr_content));

uicr_content[] persists across device reset?  Doing it that way seems
fine to me, but I wanted to check that it's what you intended.

> +static void nrf51_nvm_reset(DeviceState *dev)
> +{
> +    Nrf51NVMState *s = NRF51_NVM(dev);
> +
> +    s->empty_page = g_malloc(s->page_size);
> +    memset(s->empty_page, 0xFF, s->page_size);
> +}

This function can be called multiple times and will leak the previous
s->empty_page.  It's easier to allocate s->empty_page in
nrf51_nvm_realize() instead.

> +
> +
> +static Property nrf51_nvm_properties[] = {
> +    DEFINE_PROP_UINT16("page_size", Nrf51NVMState, page_size, 0x400),
> +    DEFINE_PROP_UINT32("code_size", Nrf51NVMState, code_size, 0x100),

There is an intense battle between '-' and '_' for property names:

$ git grep 'DEFINE_PROP[^(]\+("[^"]\+_' | wc -l
234
$ git grep 'DEFINE_PROP[^(]\+("[^"]\+-' | wc -l
394

I like '-' is because -device foo,my-option=x is more consistent with
commonly-used properties than -device foo,my_option=x, but there are
plenty of properties with '_' lurking around too.

Up to you :).

> diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
> index 35dd71c3db..9c32d22abe 100644
> --- a/include/hw/arm/nrf51_soc.h
> +++ b/include/hw/arm/nrf51_soc.h
> @@ -1,5 +1,5 @@
>  /*
> - * Nordic Semiconductor nRF51  SoC
> + * Nordic Semiconductor nRF51 SoC

Unrelated to this patch.  Please drop.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [RFC v3 2/2] tests: Add bbc:microbit / nRF51 test suite

[Stefan Hajnoczi]

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

On Thu, Jul 12, 2018 at 12:12:19PM +0200, Steffen Görtz wrote:
> diff --git a/tests/microbit-test.c b/tests/microbit-test.c
> new file mode 100644
> index 0000000000..c502ee3976
> --- /dev/null
> +++ b/tests/microbit-test.c
> @@ -0,0 +1,118 @@
> + /*
> + * QTest testcase for Microbit board using the Nordic Semiconductor nRF51 SoC.
> + *
> + *
> + * Copyright (c) 2018 Steffen Görtz <contrib@steffen-goertz.de>

Please add a license.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]