Re: [Qemu-devel] [RFC v3 1/2] arm: Add NRF51 SOC non-volatile memory controller
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
> ---
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.
signature.asc
Description: PGP signature
[Qemu-devel] [RFC v3 1/2] arm: Add NRF51 SOC non-volatile memory controller
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
---
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 00..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
+ *
+ * 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_READY0x400
+#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_ERASEPCR10x508
+#define NRF51_NVMC_ERASEPCR00x510
+#define NRF51_NVMC_ERASEALL 0x50C
+#define NRF51_NVMC_ERASEUICR0x514
+#define NRF51_NVMC_ERASE0x01
+
+#define NRF51_FICR_SIZE 0x100
+
+#define NRF51_UICR_SIZE 0x100
+
+
+/* FICR Registers Assignments
+CODEPAGESIZE 0x010
+CODESIZE 0x014
+CLENR00x028
+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
+DEVICEADDRTYPE0x0A0
+DEVICEADDR[0] 0x0A4
+DEVICEADDR[1] 0x0A8
+OVERRIDEEN0x0AC
+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] = {
+0x, 0x, 0x, 0x, 0x0400,
+0x0100, 0x, 0x, 0x0002, 0x2000,
+0x2000, 0x2000, 0x, 0x, 0x,
+0x, 0x, 0x, 0x, 0x,
+0x, 0x, 0x, 0x, 0x0003,
+0x12345678, 0x9ABCDEF1, 0x, 0x, 0x,
+0x, 0x, 0x, 0x, 0x,
+0x, 0x, 0x, 0x, 0x,
+0x, 0x, 0x, 0x, 0x,
+0x, 0x, 0x, 0x, 0x,
+0x, 0x, 0x, 0x, 0x,
+0x, 0x, 0x, 0x, 0x,
+0x, 0x, 0x, 0x
+};
+
+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] 0x02
