Changes since V1:
- Code style changes

Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
---
 hw/nvram/Makefile.objs        |   1 +
 hw/nvram/nrf51_nvmc.c         | 168 ++++++++++++++++++++++++++++++++++
 include/hw/nvram/nrf51_nvmc.h |  51 +++++++++++
 3 files changed, 220 insertions(+)
 create mode 100644 hw/nvram/nrf51_nvmc.c
 create mode 100644 include/hw/nvram/nrf51_nvmc.h

diff --git a/hw/nvram/Makefile.objs b/hw/nvram/Makefile.objs
index a912d25391..9edd61e8af 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_nvmc.o
diff --git a/hw/nvram/nrf51_nvmc.c b/hw/nvram/nrf51_nvmc.c
new file mode 100644
index 0000000000..5dde3088a8
--- /dev/null
+++ b/hw/nvram/nrf51_nvmc.c
@@ -0,0 +1,168 @@
+/*
+ * nrf51_nvmc.c
+ *
+ * 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 "hw/nvram/nrf51_nvmc.h"
+#include "exec/address-spaces.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    0x512
+#define NRF51_NVMC_ERASE        0x01
+
+#define NRF51_UICR_OFFSET       0x10001000UL
+#define NRF51_UICR_SIZE         0x100
+
+static uint64_t io_read(void *opaque, hwaddr offset, unsigned int size)
+{
+    Nrf51NVMCState *s = NRF51_NVMC(opaque);
+    uint64_t r = 0;
+
+    switch (offset) {
+    case NRF51_NVMC_READY:
+        r = NRF51_NVMC_READY_READY;
+        break;
+    case NRF51_NVMC_CONFIG:
+        r = s->state.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)
+{
+    Nrf51NVMCState *s = NRF51_NVMC(opaque);
+
+    switch (offset) {
+    case NRF51_NVMC_CONFIG:
+        s->state.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);
+            }
+            address_space_write(&s->as, NRF51_UICR_OFFSET,
+            MEMTXATTRS_UNSPECIFIED, s->empty_page, NRF51_UICR_SIZE);
+        }
+        break;
+    case NRF51_NVMC_ERASEUICR:
+        if (value == NRF51_NVMC_ERASE) {
+            address_space_write(&s->as, NRF51_UICR_OFFSET,
+            MEMTXATTRS_UNSPECIFIED, s->empty_page, NRF51_UICR_SIZE);
+        }
+        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_nvmc_init(Object *obj)
+{
+    Nrf51NVMCState *s = NRF51_NVMC(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+
+    memory_region_init_io(&s->mmio, obj, &io_ops, s,
+                          TYPE_NRF51_NVMC, NRF51_NVMC_SIZE);
+    sysbus_init_mmio(sbd, &s->mmio);
+}
+
+static void nrf51_nvmc_realize(DeviceState *dev, Error **errp)
+{
+    Nrf51NVMCState *s = NRF51_NVMC(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;
+    }
+
+    s->empty_page = g_malloc(s->page_size);
+    memset(s->empty_page, 0xFF, s->page_size);
+
+    address_space_init(&s->as, s->mr, "system-memory");
+}
+
+static void nrf51_nvmc_unrealize(DeviceState *dev, Error **errp)
+{
+    Nrf51NVMCState *s = NRF51_NVMC(dev);
+
+    g_free(s->empty_page);
+    s->empty_page = NULL;
+
+}
+
+static Property nrf51_nvmc_properties[] = {
+    DEFINE_PROP_UINT16("page_size", Nrf51NVMCState, page_size, 0x400),
+    DEFINE_PROP_UINT32("code_size", Nrf51NVMCState, code_size, 0x100),
+    DEFINE_PROP_LINK("memory", Nrf51NVMCState, mr, TYPE_MEMORY_REGION,
+                     MemoryRegion *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void nrf51_nvmc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->props = nrf51_nvmc_properties;
+    dc->realize = nrf51_nvmc_realize;
+    dc->unrealize = nrf51_nvmc_unrealize;
+}
+
+static const TypeInfo nrf51_nvmc_info = {
+    .name = TYPE_NRF51_NVMC,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(Nrf51NVMCState),
+    .instance_init = nrf51_nvmc_init,
+    .class_init = nrf51_nvmc_class_init
+};
+
+static void nrf51_nvmc_register_types(void)
+{
+    type_register_static(&nrf51_nvmc_info);
+}
+
+type_init(nrf51_nvmc_register_types)
diff --git a/include/hw/nvram/nrf51_nvmc.h b/include/hw/nvram/nrf51_nvmc.h
new file mode 100644
index 0000000000..3a63b7e5ad
--- /dev/null
+++ b/include/hw/nvram/nrf51_nvmc.h
@@ -0,0 +1,51 @@
+/*
+ * nrf51_nvmc.h
+ *
+ * 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.
+ *
+ * See Nrf51 reference manual 6 Non-Volatile Memory Controller (NVMC)
+ * See Nrf51 product sheet 8.22 NVMC specifications
+ *
+ * QEMU interface:
+ * + sysbus MMIO regions 0: Memory Region with registers
+ *   to be mapped to the peripherals instance address by the SOC.
+ * + 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.
+ */
+#ifndef NRF51_NVMC_H
+#define NRF51_NVMC_H
+
+#include "hw/sysbus.h"
+#define TYPE_NRF51_NVMC "nrf51_soc.nvmc"
+#define NRF51_NVMC(obj) OBJECT_CHECK(Nrf51NVMCState, (obj), TYPE_NRF51_NVMC)
+
+typedef struct Nrf51NVMCState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion mmio;
+
+    uint32_t code_size;
+    uint16_t page_size;
+    uint8_t *empty_page;
+    MemoryRegion *mr;
+    AddressSpace as;
+
+    struct {
+        uint32_t config:2;
+    } state;
+
+} Nrf51NVMCState;
+
+
+#endif
-- 
2.17.1

On Tue, Jun 26, 2018 at 11:32:04AM +0200, Steffen Görtz wrote:
> Changes since V1:
> - Code style changes

Please put the changelog below '---'.

> diff --git a/hw/nvram/nrf51_nvmc.c b/hw/nvram/nrf51_nvmc.c
> new file mode 100644
> index 0000000000..5dde3088a8
> --- /dev/null
> +++ b/hw/nvram/nrf51_nvmc.c
> @@ -0,0 +1,168 @@
> +/*
> + * nrf51_nvmc.c
> + *
> + * 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.
> + */

Please mention the full name of the peripheral so its purpose is clear
and include a link to the datasheet.

It's convenient to have this information in the .c file, since that's
where the majority of code changes happen.

> diff --git a/include/hw/nvram/nrf51_nvmc.h b/include/hw/nvram/nrf51_nvmc.h
> new file mode 100644
> index 0000000000..3a63b7e5ad
> --- /dev/null
> +++ b/include/hw/nvram/nrf51_nvmc.h
> @@ -0,0 +1,51 @@
> +/*
> + * nrf51_nvmc.h
> + *
> + * 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.
> + *
> + * See Nrf51 reference manual 6 Non-Volatile Memory Controller (NVMC)
> + * See Nrf51 product sheet 8.22 NVMC specifications
> + *
> + * QEMU interface:
> + * + sysbus MMIO regions 0: Memory Region with registers
> + *   to be mapped to the peripherals instance address by the SOC.
> + * + 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.

Checking CONFIG.EEN would be easy, please do it.

Peter: CONFIG.WEN determines whether stores to the flash memory region
are allowed.  What is the best way to implement this protection?

> +    MemoryRegion *mr;
> +    AddressSpace as;

I remember you said you tried
address_space_read/write(get_system_memory(), ...) but it didn't work.
Did you figure out why?

> +    struct {
> +        uint32_t config:2;
> +    } state;

I noticed you used bit-fields in recent patches.  They are rarely-used
in QEMU.

The representation of bit-fields is (compiler) implementation-defined,
so they cannot be used for live-migration where values are serialized
(marshaled).

For boolean flags just 'bool' is preferred because 'true'/'false' is
clearer in code than '0'/'1' (the reader doesn't know whether it's a
bool or used as an integer at some point unless they study all the
code).

For this field I would use just uint32_t config.

Stefan

On 27 June 2018 at 10:44, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Jun 26, 2018 at 11:32:04AM +0200, Steffen Görtz wrote:
>> Changes since V1:
>> - Code style changes
>
> Please put the changelog below '---'.
>
>> diff --git a/hw/nvram/nrf51_nvmc.c b/hw/nvram/nrf51_nvmc.c
>> new file mode 100644
>> index 0000000000..5dde3088a8
>> --- /dev/null
>> +++ b/hw/nvram/nrf51_nvmc.c
>> @@ -0,0 +1,168 @@
>> +/*
>> + * nrf51_nvmc.c
>> + *
>> + * 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.
>> + */
>
> Please mention the full name of the peripheral so its purpose is clear
> and include a link to the datasheet.
>
> It's convenient to have this information in the .c file, since that's
> where the majority of code changes happen.
>
>> diff --git a/include/hw/nvram/nrf51_nvmc.h b/include/hw/nvram/nrf51_nvmc.h
>> new file mode 100644
>> index 0000000000..3a63b7e5ad
>> --- /dev/null
>> +++ b/include/hw/nvram/nrf51_nvmc.h
>> @@ -0,0 +1,51 @@
>> +/*
>> + * nrf51_nvmc.h
>> + *
>> + * 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.
>> + *
>> + * See Nrf51 reference manual 6 Non-Volatile Memory Controller (NVMC)
>> + * See Nrf51 product sheet 8.22 NVMC specifications
>> + *
>> + * QEMU interface:
>> + * + sysbus MMIO regions 0: Memory Region with registers
>> + *   to be mapped to the peripherals instance address by the SOC.
>> + * + 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.
>
> Checking CONFIG.EEN would be easy, please do it.
>
> Peter: CONFIG.WEN determines whether stores to the flash memory region
> are allowed.  What is the best way to implement this protection?

What accesses do they gate? If this is just accesses that go
via the io_read() and io_write() functions then this is easy:
just put a check in those functions in the appropriate place.
The harder case is if they affect accesses to a region that
is implemented as an MMIO ram region which can be executed from;
that can be done, but gets annoyingly complicated:
 * unmap the RAM and map an MMIO region which handles the errors
   (only possible if the config bit gates the reads as well)
 * use a ROM device memory region (only possible if reads are
   always ok but writes might not be
 * use the IOMMU APIs (maximum flexibility, maximum pain)

A quick scan of the code suggests you're in the easy case
where you can just have io_read() and io_write() handle
the config switch checks, though.

>> +    MemoryRegion *mr;
>> +    AddressSpace as;
>
> I remember you said you tried
> address_space_read/write(get_system_memory(), ...) but it didn't work.
> Did you figure out why?

Devices directly talking to get_system_memory() isn't a great
design anyway.

It's not clear to me what this device is doing with its
AddressSpace, though; comments that went into more detail
about what the device is and what the "memory" property is
for might help.

>> +    struct {
>> +        uint32_t config:2;
>> +    } state;
>
> I noticed you used bit-fields in recent patches.  They are rarely-used
> in QEMU.

Agreed that avoiding bitfields is preferable.

thanks
-- PMM

On Mon, Jul 02, 2018 at 01:18:35PM +0100, Peter Maydell wrote:
> On 27 June 2018 at 10:44, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Tue, Jun 26, 2018 at 11:32:04AM +0200, Steffen Görtz wrote:
> >> Changes since V1:
> >> - Code style changes
> >
> > Please put the changelog below '---'.
> >
> >> diff --git a/hw/nvram/nrf51_nvmc.c b/hw/nvram/nrf51_nvmc.c
> >> new file mode 100644
> >> index 0000000000..5dde3088a8
> >> --- /dev/null
> >> +++ b/hw/nvram/nrf51_nvmc.c
> >> @@ -0,0 +1,168 @@
> >> +/*
> >> + * nrf51_nvmc.c
> >> + *
> >> + * 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.
> >> + */
> >
> > Please mention the full name of the peripheral so its purpose is clear
> > and include a link to the datasheet.
> >
> > It's convenient to have this information in the .c file, since that's
> > where the majority of code changes happen.
> >
> >> diff --git a/include/hw/nvram/nrf51_nvmc.h b/include/hw/nvram/nrf51_nvmc.h
> >> new file mode 100644
> >> index 0000000000..3a63b7e5ad
> >> --- /dev/null
> >> +++ b/include/hw/nvram/nrf51_nvmc.h
> >> @@ -0,0 +1,51 @@
> >> +/*
> >> + * nrf51_nvmc.h
> >> + *
> >> + * 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.
> >> + *
> >> + * See Nrf51 reference manual 6 Non-Volatile Memory Controller (NVMC)
> >> + * See Nrf51 product sheet 8.22 NVMC specifications
> >> + *
> >> + * QEMU interface:
> >> + * + sysbus MMIO regions 0: Memory Region with registers
> >> + *   to be mapped to the peripherals instance address by the SOC.
> >> + * + 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.
> >
> > Checking CONFIG.EEN would be easy, please do it.
> >
> > Peter: CONFIG.WEN determines whether stores to the flash memory region
> > are allowed.  What is the best way to implement this protection?
> 
> What accesses do they gate? If this is just accesses that go
> via the io_read() and io_write() functions then this is easy:
> just put a check in those functions in the appropriate place.
> The harder case is if they affect accesses to a region that
> is implemented as an MMIO ram region which can be executed from;
> that can be done, but gets annoyingly complicated:
>  * unmap the RAM and map an MMIO region which handles the errors
>    (only possible if the config bit gates the reads as well)
>  * use a ROM device memory region (only possible if reads are
>    always ok but writes might not be
>  * use the IOMMU APIs (maximum flexibility, maximum pain)
> 
> A quick scan of the code suggests you're in the easy case
> where you can just have io_read() and io_write() handle
> the config switch checks, though.

Thanks!

> >> +    MemoryRegion *mr;
> >> +    AddressSpace as;
> >
> > I remember you said you tried
> > address_space_read/write(get_system_memory(), ...) but it didn't work.
> > Did you figure out why?
> 
> Devices directly talking to get_system_memory() isn't a great
> design anyway.
> 
> It's not clear to me what this device is doing with its
> AddressSpace, though; comments that went into more detail
> about what the device is and what the "memory" property is
> for might help.

I understand this issue now.  It's the same as qtest.

This device is only visible from cpu->as, it's not visible from
address_space_memory (system_memory).

The NVMC needs access to the SoC's flash memory region, and that's what
mr/as achieve here.  I agree that comments explaining the purpose of
mr/as would be useful.

On 3 July 2018 at 10:31, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Jul 02, 2018 at 01:18:35PM +0100, Peter Maydell wrote:
>> It's not clear to me what this device is doing with its
>> AddressSpace, though; comments that went into more detail
>> about what the device is and what the "memory" property is
>> for might help.
>
> I understand this issue now.  It's the same as qtest.
>
> This device is only visible from cpu->as, it's not visible from
> address_space_memory (system_memory).
>
> The NVMC needs access to the SoC's flash memory region, and that's what
> mr/as achieve here.  I agree that comments explaining the purpose of
> mr/as would be useful.

Is the flash memory region only accessible to the CPU
via the NVMC, or can the CPU get at it both directly
and via the NVMC? (That is, in hardware, does the flash
sit "behind" the NVMC?)

thanks
-- PMM

> Is the flash memory region only accessible to the CPU
> via the NVMC, or can the CPU get at it both directly
> and via the NVMC? (That is, in hardware, does the flash
> sit "behind" the NVMC?)
> 
> thanks
> -- PMM
> 

The latter is the case.

Steffen

On 3 July 2018 at 10:31, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Jul 02, 2018 at 01:18:35PM +0100, Peter Maydell wrote:
>> It's not clear to me what this device is doing with its
>> AddressSpace, though; comments that went into more detail
>> about what the device is and what the "memory" property is
>> for might help.
>
> I understand this issue now.  It's the same as qtest.
>
> This device is only visible from cpu->as, it's not visible from
> address_space_memory (system_memory).
>
> The NVMC needs access to the SoC's flash memory region, and that's what
> mr/as achieve here.  I agree that comments explaining the purpose of
> mr/as would be useful.

I've fished out a copy of the NRF51 datasheet, which helps.
Roughly, what we have is:

 [ CPU ] -+- [ NVM, either FICR, UICR or CODE ]
          |      |
          \- [ NVMC ]

where the CPU can talk to either the NVMC or the NVMs, and the
NVMC also has a control connection to the NVM. (See the block
diagram in the nRF51 manual.)

The behaviour this allows is:

 * the CPU can directly read and execute from the NVMs (which it
   can see in its address space at 0x00000000, 0x10000000, 0x10001000)
 * the NVMC controls whether the CPU can write to the NVMs. If the
   NVMC CONFIG register permits writes then the CPU writes directly
   to the mapped NVMs, and gets the usual flash-memory behaviour that
   writes can turn 1s to 0s but not 0s to 1s.
 * the NVMC also has support for performing erases of either pages
   or entire NVMs (which writes them to all-1s)

My suggestion for the best way to model this is:

 * the NVMs are (sysbus) devices which expose a MemoryRegion which
   they have created with memory_region_init_rom_device().
   MRs of this sort can be directly read (from a ram block), but
   writes always go via a MemoryRegionOps write function. This
   will let us do fast execution and reading, but catch writes so
   we can make them behave correctly (be forbidden, not allow
   writing of 0->1, etc).
 * the NVMs should implement a QOM interface that defines
   how we model the "control connection" from the NVMC --
   basically this will have some methods for "erase all",
   "erase this page", and "set the writes permitted flag".
 * the NVMC should have QOM link properties for its NVMCs, and
   the SoC code sets those link properties to pointers to the
   NVMCs
 * the NVMC implements an ordinary IO MemoryRegion for its
   register interface; guest writes to the registers may cause
   it to tell the NVMs what to do by calling methods on the
   NVMs (which it has pointers to via the link properties)
 * the SoC code maps the NVMs and the NVMC's registers into
   the right places in its address space

There's nothing here that requires extra functionality from
core QEMU code, but on the other hand it is quite complicated,
so you might prefer to start by modelling the NVMs as plain
ROM MRs (ie never-writable) and the NVMC as a register interface
which just complains (LOG_UNIMP) if the guest tries to enable
writes or erases, plus a comment giving a sketch of the
design changes needed to add write/erase support later.
(I would prefer that to the code in this patch which implements
erases by writing to a single passed-in memory region which
covers the whole address space.)

thanks
-- PMM

Hello Peter,

thank you for your very elaborate remarks!
>  [ CPU ] -+- [ NVM, either FICR, UICR or CODE ]
>           |      |
>           \- [ NVMC ]
> 
> where the CPU can talk to either the NVMC or the NVMs, and the
> NVMC also has a control connection to the NVM. (See the block
> diagram in the nRF51 manual.)
> 
> The behaviour this allows is:
> 
>  * the CPU can directly read and execute from the NVMs (which it
>    can see in its address space at 0x00000000, 0x10000000, 0x10001000)
>  * the NVMC controls whether the CPU can write to the NVMs. If the
>    NVMC CONFIG register permits writes then the CPU writes directly
>    to the mapped NVMs, and gets the usual flash-memory behaviour that
>    writes can turn 1s to 0s but not 0s to 1s.
>  * the NVMC also has support for performing erases of either pages
>    or entire NVMs (which writes them to all-1s)
> 
> My suggestion for the best way to model this is:
> 
>  * the NVMs are (sysbus) devices which expose a MemoryRegion which
>    they have created with memory_region_init_rom_device().
>    MRs of this sort can be directly read (from a ram block), but
>    writes always go via a MemoryRegionOps write function. This
>    will let us do fast execution and reading, but catch writes so
>    we can make them behave correctly (be forbidden, not allow
>    writing of 0->1, etc).

In the latest revision of my NVMC model i included the FICR/UICR as
sysbus devices which expose a memory region using
memory_region_init_device. I can change this using init_rom_device for
performance. Furthermore i will add the CODE device.

I would like to provide all four devices (NVMC, CODE, FICR, UICR) from
one module and rename nrf51_nvmc.[ch] to nrf51_nvm.[ch]. Is this
acceptable or would you rather prefer three modules?

>  * the NVMs should implement a QOM interface that defines
>    how we model the "control connection" from the NVMC --
>    basically this will have some methods for "erase all",
>    "erase this page", and "set the writes permitted flag".
>  * the NVMC should have QOM link properties for its NVMCs, and
>    the SoC code sets those link properties to pointers to the
>    NVMCs
>  * the NVMC implements an ordinary IO MemoryRegion for its
>    register interface; guest writes to the registers may cause
>    it to tell the NVMs what to do by calling methods on the
>    NVMs (which it has pointers to via the link properties)
>  * the SoC code maps the NVMs and the NVMC's registers into
>    the right places in its address space

Thank you for these remarks, i will follow them.

Steffen

On 5 July 2018 at 18:14, Steffen Görtz <mail@steffen-goertz.de> wrote:
> I would like to provide all four devices (NVMC, CODE, FICR, UICR) from
> one module and rename nrf51_nvmc.[ch] to nrf51_nvm.[ch]. Is this
> acceptable or would you rather prefer three modules?

Yeah, that would be ok too.

thanks
-- PMM