[Qemu-devel] [PATCH 0/7] arm: Instantiation of nRF51 SOC and bbc:microbit devices

[Qemu-devel] [PATCH 0/7] arm: Instantiation of nRF51 SOC and bbc:microbit devices

[Steffen Görtz]

This patch series extends the stub of the nRF51 SOC and
bbc:microbit machine with the devices provided in the
previous patch series.

Based-on: 20180806100114.21410-1-contrib@steffen-goertz.de

Steffen Görtz (7):
  hw/arm/nrf51_soc: nRF51 Calculate peripheral id from base address
  arm: Move nRF51 machine state to dedicated header
  arm: Add chip variant property to nRF51 SoC
  arm: Add additional datasheets and copyright lines
  arm: Improve error propagation in nRF51 SOC
  arm: Instantiate nRF51 peripherals
  arm: Instantiate Microbit board-level devices

 hw/arm/microbit.c          |  91 ++++++++++++----
 hw/arm/nrf51_soc.c         | 213 +++++++++++++++++++++++++++++++++----
 include/hw/arm/microbit.h  |  30 ++++++
 include/hw/arm/nrf51_soc.h |  32 +++++-
 4 files changed, 321 insertions(+), 45 deletions(-)
 create mode 100644 include/hw/arm/microbit.h

-- 
2.18.0

[Qemu-devel] [PATCH 1/7] hw/arm/nrf51_soc: nRF51 Calculate peripheral id from base address

[Steffen Görtz]

The base address determines a peripherals id, which identifies its
interrupt line, see NRF51 reference manual section 10 peripheral
interface. This little gem calculates the peripheral id based
on its base address.

Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
---
 hw/arm/nrf51_soc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 9f9649c780..441d05e1ef 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -38,6 +38,9 @@
 #define NRF51822_FLASH_SIZE     (256 * 1024)
 #define NRF51822_SRAM_SIZE      (16 * 1024)
 
+/* IRQ lines can be derived from peripheral base addresses */
+#define BASE_TO_IRQ(base) (((base) >> 12) & 0x1F)
+
 static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
 {
     NRF51State *s = NRF51_SOC(dev_soc);
-- 
2.18.0

[Qemu-devel] [PATCH 2/7] arm: Move nRF51 machine state to dedicated header

[Steffen Görtz]

The machine state will be used to host the SOC
and board level devices like the LED matrix and
devices to handle to pushbuttons A and B.

Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
---
 hw/arm/microbit.c         | 38 ++++++++++++++++++++++++--------------
 include/hw/arm/microbit.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 14 deletions(-)
 create mode 100644 include/hw/arm/microbit.h

diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c
index 467cfbda23..8f3c446f52 100644
--- a/hw/arm/microbit.c
+++ b/hw/arm/microbit.c
@@ -14,22 +14,11 @@
 #include "hw/arm/arm.h"
 #include "exec/address-spaces.h"
 
-#include "hw/arm/nrf51_soc.h"
-
-typedef struct {
-    MachineState parent;
-
-    NRF51State nrf51;
-} MICROBITMachineState;
-
-#define TYPE_MICROBIT_MACHINE "microbit"
-
-#define MICROBIT_MACHINE(obj) \
-    OBJECT_CHECK(MICROBITMachineState, obj, TYPE_MICROBIT_MACHINE)
+#include "hw/arm/microbit.h"
 
 static void microbit_init(MachineState *machine)
 {
-    MICROBITMachineState *s = g_new(MICROBITMachineState, 1);
+    MicrobitMachineState *s = MICROBIT_MACHINE(machine);
     MemoryRegion *system_memory = get_system_memory();
     Object *soc;
 
@@ -45,10 +34,31 @@ static void microbit_init(MachineState *machine)
             NRF51_SOC(soc)->flash_size);
 }
 
+
 static void microbit_machine_init(MachineClass *mc)
 {
     mc->desc = "BBC micro:bit";
     mc->init = microbit_init;
     mc->max_cpus = 1;
 }
-DEFINE_MACHINE("microbit", microbit_machine_init);
+
+static void microbit_machine_init_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+    microbit_machine_init(mc);
+}
+
+static const TypeInfo microbit_machine_info = {
+    .name       = TYPE_MICROBIT_MACHINE,
+    .parent     = TYPE_MACHINE,
+    .instance_size = sizeof(MicrobitMachineState),
+    .class_init = microbit_machine_init_class_init,
+};
+
+static void microbit_machine_types(void)
+{
+    type_register_static(&microbit_machine_info);
+}
+
+type_init(microbit_machine_types)
+
diff --git a/include/hw/arm/microbit.h b/include/hw/arm/microbit.h
new file mode 100644
index 0000000000..89f0c6bc07
--- /dev/null
+++ b/include/hw/arm/microbit.h
@@ -0,0 +1,29 @@
+/*
+ * BBC micro:bit machine
+ *
+ * Copyright 2018 Joel Stanley <joel@jms.id.au>
+ * 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 MICROBIT_H
+#define MICROBIT_H
+
+#include "qemu/osdep.h"
+#include "hw/qdev-core.h"
+#include "hw/arm/nrf51_soc.h"
+#include "hw/display/led_matrix.h"
+
+#define TYPE_MICROBIT_MACHINE       MACHINE_TYPE_NAME("microbit")
+#define MICROBIT_MACHINE(obj) \
+    OBJECT_CHECK(MicrobitMachineState, (obj), TYPE_MICROBIT_MACHINE)
+
+typedef struct MicrobitMachineState {
+    /*< private >*/
+    MachineState parent_obj;
+
+    NRF51State nrf51;
+} MicrobitMachineState;
+
+#endif
-- 
2.18.0

[Qemu-devel] [PATCH 3/7] arm: Add chip variant property to nRF51 SoC

[Steffen Görtz]

Nordic Semiconductor nRF51 SOCs are available
in different "variants" which differ in available memory.
This patchs adds a "variant" attribute to the SOC
so that the user can choose between different variants and
thus memory sizes.

See product specification http://infocenter.nordicsemi.com/pdf/nRF51822_PS_v3.1.pdf
section 10.6

Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
---
 hw/arm/microbit.c          |  4 ++--
 hw/arm/nrf51_soc.c         | 40 +++++++++++++++++++++++++++-----------
 include/hw/arm/nrf51_soc.h | 15 +++++++++++---
 3 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c
index 8f3c446f52..d6776dea0a 100644
--- a/hw/arm/microbit.c
+++ b/hw/arm/microbit.c
@@ -27,11 +27,11 @@ static void microbit_init(MachineState *machine)
     object_property_add_child(OBJECT(machine), "nrf51", soc, &error_fatal);
     object_property_set_link(soc, OBJECT(system_memory),
                              "memory", &error_abort);
+    qdev_prop_set_uint32(DEVICE(soc), "variant", NRF51_VARIANT_AA);
 
     object_property_set_bool(soc, true, "realized", &error_abort);
 
-    armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
-            NRF51_SOC(soc)->flash_size);
+    armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename, 0x00);
 }
 
 
diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 441d05e1ef..85bce2c1e0 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -32,15 +32,21 @@
 #define FLASH_BASE      0x00000000
 #define SRAM_BASE       0x20000000
 
-/* The size and base is for the NRF51822 part. If other parts
- * are supported in the future, add a sub-class of NRF51SoC for
- * the specific variants */
-#define NRF51822_FLASH_SIZE     (256 * 1024)
-#define NRF51822_SRAM_SIZE      (16 * 1024)
+#define PAGE_SIZE       1024
 
 /* IRQ lines can be derived from peripheral base addresses */
 #define BASE_TO_IRQ(base) (((base) >> 12) & 0x1F)
 
+/* RAM and CODE size in number of pages for different NRF51Variants variants */
+struct {
+  hwaddr ram_size;
+  hwaddr flash_size;
+} NRF51VariantAttributes[] = {
+        [NRF51_VARIANT_AA] = {.ram_size = 16, .flash_size = 256 },
+        [NRF51_VARIANT_AB] = {.ram_size = 16, .flash_size = 128 },
+        [NRF51_VARIANT_AC] = {.ram_size = 32, .flash_size = 256 },
+};
+
 static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
 {
     NRF51State *s = NRF51_SOC(dev_soc);
@@ -51,13 +57,21 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
         return;
     }
 
+    if (!(s->part_variant > NRF51_VARIANT_INVALID
+            && s->part_variant < NRF51_VARIANT_MAX)) {
+        error_setg(errp, "VARIANT not set or invalid");
+        return;
+    }
+
     object_property_set_link(OBJECT(&s->cpu), OBJECT(&s->container), "memory",
             &err);
     object_property_set_bool(OBJECT(&s->cpu), true, "realized", &err);
 
     memory_region_add_subregion_overlap(&s->container, 0, s->board_memory, -1);
 
-    memory_region_init_ram(&s->flash, OBJECT(s), "nrf51.flash", s->flash_size,
+    /* FLASH */
+    memory_region_init_ram(&s->flash, NULL, "nrf51_soc.flash",
+            NRF51VariantAttributes[s->part_variant].flash_size * PAGE_SIZE,
             &err);
     if (err) {
         error_propagate(errp, err);
@@ -66,7 +80,9 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
     memory_region_set_readonly(&s->flash, true);
     memory_region_add_subregion(&s->container, FLASH_BASE, &s->flash);
 
-    memory_region_init_ram(&s->sram, NULL, "nrf51.sram", s->sram_size, &err);
+    /* SRAM */
+    memory_region_init_ram(&s->sram, NULL, "nrf51_soc.sram",
+            NRF51VariantAttributes[s->part_variant].ram_size * PAGE_SIZE, &err);
     if (err) {
         error_propagate(errp, err);
         return;
@@ -85,17 +101,19 @@ static void nrf51_soc_init(Object *obj)
     memory_region_init(&s->container, obj, "nrf51-container", UINT64_MAX);
 
     object_initialize(&s->cpu, sizeof(s->cpu), TYPE_ARMV7M);
-    object_property_add_child(OBJECT(s), "armv6m", OBJECT(&s->cpu), &error_abort);
+    object_property_add_child(OBJECT(s), "armv6m", OBJECT(&s->cpu),
+                              &error_abort);
     qdev_set_parent_bus(DEVICE(&s->cpu), sysbus_get_default());
-    qdev_prop_set_string(DEVICE(&s->cpu), "cpu-type", ARM_CPU_TYPE_NAME("cortex-m0"));
+    qdev_prop_set_string(DEVICE(&s->cpu), "cpu-type",
+                         ARM_CPU_TYPE_NAME("cortex-m0"));
     qdev_prop_set_uint32(DEVICE(&s->cpu), "num-irq", 32);
 }
 
 static Property nrf51_soc_properties[] = {
     DEFINE_PROP_LINK("memory", NRF51State, board_memory, TYPE_MEMORY_REGION,
                      MemoryRegion *),
-    DEFINE_PROP_UINT32("sram-size", NRF51State, sram_size, NRF51822_SRAM_SIZE),
-    DEFINE_PROP_UINT32("flash-size", NRF51State, flash_size, NRF51822_FLASH_SIZE),
+    DEFINE_PROP_INT32("variant", NRF51State, part_variant,
+                      NRF51_VARIANT_INVALID),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
index e380ec26b8..24212f9174 100644
--- a/include/hw/arm/nrf51_soc.h
+++ b/include/hw/arm/nrf51_soc.h
@@ -29,14 +29,23 @@ typedef struct NRF51State {
     MemoryRegion sram;
     MemoryRegion flash;
 
-    uint32_t sram_size;
-    uint32_t flash_size;
-
     MemoryRegion *board_memory;
 
     MemoryRegion container;
 
+    /* Properties */
+    int32_t part_variant;
 } NRF51State;
 
+
+/* Variants as described in nRF51 product specification section 10.6 table 73 */
+typedef enum {
+    NRF51_VARIANT_INVALID = -1,
+    NRF51_VARIANT_AA = 0,
+    NRF51_VARIANT_AB = 1,
+    NRF51_VARIANT_AC = 2,
+    NRF51_VARIANT_MAX = 3
+} NRF51Variants;
+
 #endif
 
-- 
2.18.0

[Qemu-devel] [PATCH 4/7] arm: Add additional datasheets and copyright lines

[Steffen Görtz]

This patch adds a link to the product specification
which contain additional information about the
Nordic Semiconductor nRF51 SOC series.

Furthermore it adds a copyright line to all
files that get changed significantly.

Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
---
 hw/arm/microbit.c          | 1 +
 hw/arm/nrf51_soc.c         | 4 +++-
 include/hw/arm/nrf51_soc.h | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c
index d6776dea0a..bb6ddb6a79 100644
--- a/hw/arm/microbit.c
+++ b/hw/arm/microbit.c
@@ -3,6 +3,7 @@
  * http://tech.microbit.org/hardware/
  *
  * Copyright 2018 Joel Stanley <joel@jms.id.au>
+ * 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.
diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 85bce2c1e0..2265d30352 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -1,8 +1,10 @@
 /*
  * Nordic Semiconductor nRF51 SoC
- * http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.1.pdf
+ * Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
+ * Product Spec: http://infocenter.nordicsemi.com/pdf/nRF51822_PS_v3.1.pdf
  *
  * Copyright 2018 Joel Stanley <joel@jms.id.au>
+ * 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.
diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
index 24212f9174..45d9671dc3 100644
--- a/include/hw/arm/nrf51_soc.h
+++ b/include/hw/arm/nrf51_soc.h
@@ -2,6 +2,7 @@
  * Nordic Semiconductor nRF51  SoC
  *
  * Copyright 2018 Joel Stanley <joel@jms.id.au>
+ * 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.
-- 
2.18.0

[Qemu-devel] [PATCH 5/7] arm: Improve error propagation in nRF51 SOC

[Steffen Görtz]

This patch takes care that errors that occur during
instantiation of the cortex-m0 cpu are properly propagated.

Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
---
 hw/arm/nrf51_soc.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 2265d30352..88a848de8b 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -66,8 +66,17 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
     }
 
     object_property_set_link(OBJECT(&s->cpu), OBJECT(&s->container), "memory",
-            &err);
-    object_property_set_bool(OBJECT(&s->cpu), true, "realized", &err);
+                            &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    object_property_set_bool(OBJECT(&s->cpu), true, "realized",
+                             &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
 
     memory_region_add_subregion_overlap(&s->container, 0, s->board_memory, -1);
 
-- 
2.18.0

[Qemu-devel] [PATCH 6/7] arm: Instantiate nRF51 peripherals

[Steffen Görtz]

Instantiate NVMs, NVMC, UART, RNG, GPIO and TIMERs.

Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
---
 hw/arm/nrf51_soc.c         | 153 +++++++++++++++++++++++++++++++++++--
 include/hw/arm/nrf51_soc.h |  16 +++-
 2 files changed, 161 insertions(+), 8 deletions(-)

diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 88a848de8b..a395d3a00d 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -25,15 +25,20 @@
 
 #include "hw/arm/nrf51_soc.h"
 
-#define IOMEM_BASE      0x40000000
-#define IOMEM_SIZE      0x20000000
-
-#define FICR_BASE       0x10000000
-#define FICR_SIZE       0x000000fc
-
 #define FLASH_BASE      0x00000000
+#define FICR_BASE       0x10000000
+#define UICR_BASE       0x10001000
 #define SRAM_BASE       0x20000000
 
+#define IOMEM_BASE      0x40000000
+#define IOMEM_SIZE      0x20000000
+
+#define UART_BASE       0x40002000
+#define TIMER_BASE      0x40008000
+#define RNG_BASE        0x4000D000
+#define NVMC_BASE       0x4001E000
+#define GPIO_BASE       0x50000000
+
 #define PAGE_SIZE       1024
 
 /* IRQ lines can be derived from peripheral base addresses */
@@ -49,10 +54,33 @@ struct {
         [NRF51_VARIANT_AC] = {.ram_size = 32, .flash_size = 256 },
 };
 
+
+static uint64_t clock_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u]\n",
+                  __func__, addr, size);
+    return 1;
+}
+
+static void clock_write(void *opaque, hwaddr addr, uint64_t data,
+                        unsigned int size)
+{
+    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n",
+                  __func__, addr, data, size);
+}
+
+static const MemoryRegionOps clock_ops = {
+    .read = clock_read,
+    .write = clock_write
+};
+
 static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
 {
     NRF51State *s = NRF51_SOC(dev_soc);
     Error *err = NULL;
+    MemoryRegion *mr = NULL;
+    size_t i;
+    qemu_irq irq;
 
     if (!s->board_memory) {
         error_setg(errp, "memory property was not set");
@@ -100,14 +128,102 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
     }
     memory_region_add_subregion(&s->container, SRAM_BASE, &s->sram);
 
+
+    /* UART */
+    qdev_prop_set_chr(DEVICE(&s->uart), "chardev", serial_hd(0));
+    object_property_set_bool(OBJECT(&s->uart), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->uart), 0);
+    memory_region_add_subregion_overlap(&s->container, UART_BASE, mr, 0);
+    irq = qdev_get_gpio_in(DEVICE(&s->cpu), BASE_TO_IRQ(UART_BASE));
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart), 0, irq);
+
+    /* TIMER */
+    for (i = 0; i < NRF51_TIMER_NUM; i++) {
+        object_property_set_bool(OBJECT(&s->timer[i]), true, "realized", &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
+
+        sysbus_mmio_map(SYS_BUS_DEVICE(&s->timer[i]), 0,
+                     TIMER_BASE + i * 0x1000);
+
+        irq = qdev_get_gpio_in(DEVICE(&s->cpu),
+                               BASE_TO_IRQ(TIMER_BASE + i * 0x1000));
+        sysbus_connect_irq(SYS_BUS_DEVICE(&s->timer[i]), 0, irq);
+    }
+
+    /* NVMC */
+    object_property_set_link(OBJECT(&s->nvm), OBJECT(&s->container),
+                                         "memory", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    object_property_set_uint(OBJECT(&s->nvm),
+            NRF51VariantAttributes[s->part_variant].flash_size, "code_size",
+            &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    object_property_set_bool(OBJECT(&s->nvm), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->nvm), 0);
+    memory_region_add_subregion_overlap(&s->container, NVMC_BASE, mr, 0);
+    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->nvm), 1);
+    memory_region_add_subregion_overlap(&s->container, FICR_BASE, mr, 0);
+    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->nvm), 2);
+    memory_region_add_subregion_overlap(&s->container, UICR_BASE, mr, 0);
+
+    /* RNG */
+    object_property_set_bool(OBJECT(&s->rng), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->rng), 0);
+    memory_region_add_subregion_overlap(&s->container, RNG_BASE, mr, 0);
+    irq = qdev_get_gpio_in(DEVICE(&s->cpu), BASE_TO_IRQ(RNG_BASE));
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->rng), 0, irq);
+
+    /* GPIO */
+    object_property_set_bool(OBJECT(&s->gpio), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gpio), 0);
+    memory_region_add_subregion_overlap(&s->container, GPIO_BASE, mr, 0);
+
+    /* Pass all GPIOs to the SOC layer so they are available to the board */
+    qdev_pass_gpios(DEVICE(&s->gpio), dev_soc, NULL);
+
+    /* STUB Peripherals */
+    memory_region_init_io(&s->clock, NULL, &clock_ops, NULL,
+                          "nrf51_soc.clock", 0x1000);
+    memory_region_add_subregion_overlap(&s->container, IOMEM_BASE, &s->clock,
+                                        -1);
+
     create_unimplemented_device("nrf51_soc.io", IOMEM_BASE, IOMEM_SIZE);
-    create_unimplemented_device("nrf51_soc.ficr", FICR_BASE, FICR_SIZE);
     create_unimplemented_device("nrf51_soc.private", 0xF0000000, 0x10000000);
 }
 
 static void nrf51_soc_init(Object *obj)
 {
     NRF51State *s = NRF51_SOC(obj);
+    size_t i;
 
     memory_region_init(&s->container, obj, "nrf51-container", UINT64_MAX);
 
@@ -118,6 +234,29 @@ static void nrf51_soc_init(Object *obj)
     qdev_prop_set_string(DEVICE(&s->cpu), "cpu-type",
                          ARM_CPU_TYPE_NAME("cortex-m0"));
     qdev_prop_set_uint32(DEVICE(&s->cpu), "num-irq", 32);
+
+    object_initialize(&s->uart, sizeof(s->uart), TYPE_NRF51_UART);
+    object_property_add_child(obj, "uart", OBJECT(&s->uart), &error_abort);
+    qdev_set_parent_bus(DEVICE(&s->uart), sysbus_get_default());
+
+    object_initialize(&s->nvm, sizeof(s->nvm), TYPE_NRF51_NVM);
+    object_property_add_child(obj, "nvm", OBJECT(&s->nvm), &error_abort);
+    qdev_set_parent_bus(DEVICE(&s->nvm), sysbus_get_default());
+
+    object_initialize(&s->rng, sizeof(s->rng), TYPE_NRF51_RNG);
+    object_property_add_child(obj, "rng", OBJECT(&s->rng), &error_abort);
+    qdev_set_parent_bus(DEVICE(&s->rng), sysbus_get_default());
+
+    object_initialize(&s->gpio, sizeof(s->gpio), TYPE_NRF51_GPIO);
+    object_property_add_child(obj, "gpio", OBJECT(&s->gpio), &error_abort);
+    qdev_set_parent_bus(DEVICE(&s->gpio), sysbus_get_default());
+
+    for (i = 0; i < NRF51_TIMER_NUM; i++) {
+        object_initialize(&s->timer[i], sizeof(s->timer[i]), TYPE_NRF51_TIMER);
+        object_property_add_child(obj, "timer[*]", OBJECT(&s->timer[i]), NULL);
+        qdev_set_parent_bus(DEVICE(&s->timer[i]), sysbus_get_default());
+    }
+
 }
 
 static Property nrf51_soc_properties[] = {
diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
index 45d9671dc3..d47b42fa37 100644
--- a/include/hw/arm/nrf51_soc.h
+++ b/include/hw/arm/nrf51_soc.h
@@ -14,11 +14,18 @@
 #include "qemu/osdep.h"
 #include "hw/sysbus.h"
 #include "hw/arm/armv7m.h"
+#include "hw/char/nrf51_uart.h"
+#include "hw/misc/nrf51_rng.h"
+#include "hw/nvram/nrf51_nvm.h"
+#include "hw/gpio/nrf51_gpio.h"
+#include "hw/timer/nrf51_timer.h"
 
 #define TYPE_NRF51_SOC "nrf51-soc"
 #define NRF51_SOC(obj) \
     OBJECT_CHECK(NRF51State, (obj), TYPE_NRF51_SOC)
 
+#define NRF51_TIMER_NUM 3
+
 typedef struct NRF51State {
     /*< private >*/
     SysBusDevice parent_obj;
@@ -31,9 +38,16 @@ typedef struct NRF51State {
     MemoryRegion flash;
 
     MemoryRegion *board_memory;
-
     MemoryRegion container;
 
+    MemoryRegion clock;
+
+    Nrf51UART uart;
+    Nrf51NVMState nvm;
+    Nrf51RNGState rng;
+    Nrf51GPIOState gpio;
+    Nrf51TimerState timer[NRF51_TIMER_NUM];
+
     /* Properties */
     int32_t part_variant;
 } NRF51State;
-- 
2.18.0

[Qemu-devel] [PATCH 7/7] arm: Instantiate Microbit board-level devices

[Steffen Görtz]

Instantiate the LED matrix and set board-level
pull-up default values to buttons A and B.
This is necessary to calm down the microsoft pxt
javascript runtime available for the micro:bit.

Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
---
 hw/arm/microbit.c         | 52 ++++++++++++++++++++++++++++++++++-----
 include/hw/arm/microbit.h |  1 +
 2 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c
index bb6ddb6a79..6fad0de2cd 100644
--- a/hw/arm/microbit.c
+++ b/hw/arm/microbit.c
@@ -13,34 +13,74 @@
 #include "qapi/error.h"
 #include "hw/boards.h"
 #include "hw/arm/arm.h"
+#include "sysemu/qtest.h"
 #include "exec/address-spaces.h"
 
 #include "hw/arm/microbit.h"
 
+#define BUTTON_A_PIN 17
+#define BUTTON_B_PIN 26
+
 static void microbit_init(MachineState *machine)
 {
     MicrobitMachineState *s = MICROBIT_MACHINE(machine);
     MemoryRegion *system_memory = get_system_memory();
-    Object *soc;
+    DeviceState *soc, *matrix;
 
     object_initialize(&s->nrf51, sizeof(s->nrf51), TYPE_NRF51_SOC);
-    soc = OBJECT(&s->nrf51);
-    object_property_add_child(OBJECT(machine), "nrf51", soc, &error_fatal);
-    object_property_set_link(soc, OBJECT(system_memory),
+    soc = DEVICE(&s->nrf51);
+    object_property_add_child(OBJECT(machine), "nrf51", OBJECT(soc),
+                              &error_fatal);
+    object_property_set_link(OBJECT(soc), OBJECT(system_memory),
                              "memory", &error_abort);
-    qdev_prop_set_uint32(DEVICE(soc), "variant", NRF51_VARIANT_AA);
+    qdev_prop_set_uint32(soc, "variant", NRF51_VARIANT_AA);
 
-    object_property_set_bool(soc, true, "realized", &error_abort);
+    object_property_set_bool(OBJECT(soc), true, "realized", &error_abort);
+
+    object_initialize(&s->matrix, sizeof(s->matrix), TYPE_LED_MATRIX);
+    matrix = DEVICE(&s->matrix);
+    qdev_prop_set_uint16(matrix, "rows", 3);
+    qdev_prop_set_uint16(matrix, "cols", 9);
+    object_property_set_bool(OBJECT(matrix), true, "realized", &error_fatal);
+
+    qdev_connect_gpio_out(soc, 4, qdev_get_gpio_in_named(matrix, "col", 0));
+    qdev_connect_gpio_out(soc, 5, qdev_get_gpio_in_named(matrix, "col", 1));
+    qdev_connect_gpio_out(soc, 6, qdev_get_gpio_in_named(matrix, "col", 2));
+    qdev_connect_gpio_out(soc, 7, qdev_get_gpio_in_named(matrix, "col", 3));
+    qdev_connect_gpio_out(soc, 8, qdev_get_gpio_in_named(matrix, "col", 4));
+    qdev_connect_gpio_out(soc, 9, qdev_get_gpio_in_named(matrix, "col", 5));
+    qdev_connect_gpio_out(soc, 10, qdev_get_gpio_in_named(matrix, "col", 6));
+    qdev_connect_gpio_out(soc, 11, qdev_get_gpio_in_named(matrix, "col", 7));
+    qdev_connect_gpio_out(soc, 12, qdev_get_gpio_in_named(matrix, "col", 8));
+
+    qdev_connect_gpio_out(soc, 13, qdev_get_gpio_in_named(matrix, "row", 0));
+    qdev_connect_gpio_out(soc, 14, qdev_get_gpio_in_named(matrix, "row", 1));
+    qdev_connect_gpio_out(soc, 15, qdev_get_gpio_in_named(matrix, "row", 2));
 
     armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename, 0x00);
 }
 
+static void microbit_reset(void)
+{
+    MachineState *machine = MACHINE(qdev_get_machine());
+    MicrobitMachineState *s = MICROBIT_MACHINE(machine);
+
+    qemu_devices_reset();
+
+    /* Board level pull-up */
+    if (!qtest_enabled()) {
+        qemu_set_irq(qdev_get_gpio_in(DEVICE(&s->nrf51), BUTTON_A_PIN), 1);
+        qemu_set_irq(qdev_get_gpio_in(DEVICE(&s->nrf51), BUTTON_B_PIN), 1);
+    }
+}
+
 
 static void microbit_machine_init(MachineClass *mc)
 {
     mc->desc = "BBC micro:bit";
     mc->init = microbit_init;
     mc->max_cpus = 1;
+    mc->reset = microbit_reset;
 }
 
 static void microbit_machine_init_class_init(ObjectClass *oc, void *data)
diff --git a/include/hw/arm/microbit.h b/include/hw/arm/microbit.h
index 89f0c6bc07..094326b4ae 100644
--- a/include/hw/arm/microbit.h
+++ b/include/hw/arm/microbit.h
@@ -24,6 +24,7 @@ typedef struct MicrobitMachineState {
     MachineState parent_obj;
 
     NRF51State nrf51;
+    LEDMatrixState matrix;
 } MicrobitMachineState;
 
 #endif
-- 
2.18.0

Re: [Qemu-devel] [PATCH 1/7] hw/arm/nrf51_soc: nRF51 Calculate peripheral id from base address

[Peter Maydell]

On 11 August 2018 at 10:08, Steffen Görtz <contrib@steffen-goertz.de> wrote:
> The base address determines a peripherals id, which identifies its
> interrupt line, see NRF51 reference manual section 10 peripheral
> interface. This little gem calculates the peripheral id based
> on its base address.
>
> Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
> ---
>  hw/arm/nrf51_soc.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
> index 9f9649c780..441d05e1ef 100644
> --- a/hw/arm/nrf51_soc.c
> +++ b/hw/arm/nrf51_soc.c
> @@ -38,6 +38,9 @@
>  #define NRF51822_FLASH_SIZE     (256 * 1024)
>  #define NRF51822_SRAM_SIZE      (16 * 1024)
>
> +/* IRQ lines can be derived from peripheral base addresses */
> +#define BASE_TO_IRQ(base) (((base) >> 12) & 0x1F)
> +

You could also have macros for getting the base address
from the peripheral ID and the IRQ line from the
peripheral ID, but this is fine too.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/7] arm: Move nRF51 machine state to dedicated header

[Peter Maydell]

On 11 August 2018 at 10:08, Steffen Görtz <contrib@steffen-goertz.de> wrote:
> The machine state will be used to host the SOC
> and board level devices like the LED matrix and
> devices to handle to pushbuttons A and B.
>
> Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
> ---
>  hw/arm/microbit.c         | 38 ++++++++++++++++++++++++--------------
>  include/hw/arm/microbit.h | 29 +++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+), 14 deletions(-)
>  create mode 100644 include/hw/arm/microbit.h

These changes should be squashed into Joel's initial
patch creating the machine, as per discussion on that patch.

I don't think we really need a separate header file,
unless there's something I'm missing because I haven't yet
read later patches in the series.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/7] arm: Add chip variant property to nRF51 SoC

[Peter Maydell]

On 11 August 2018 at 10:08, Steffen Görtz <contrib@steffen-goertz.de> wrote:
> Nordic Semiconductor nRF51 SOCs are available
> in different "variants" which differ in available memory.
> This patchs adds a "variant" attribute to the SOC
> so that the user can choose between different variants and
> thus memory sizes.
>
> See product specification http://infocenter.nordicsemi.com/pdf/nRF51822_PS_v3.1.pdf
> section 10.6
>
> Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
> ---
>  hw/arm/microbit.c          |  4 ++--
>  hw/arm/nrf51_soc.c         | 40 +++++++++++++++++++++++++++-----------
>  include/hw/arm/nrf51_soc.h | 15 +++++++++++---
>  3 files changed, 43 insertions(+), 16 deletions(-)
>
> diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c
> index 8f3c446f52..d6776dea0a 100644
> --- a/hw/arm/microbit.c
> +++ b/hw/arm/microbit.c
> @@ -27,11 +27,11 @@ static void microbit_init(MachineState *machine)
>      object_property_add_child(OBJECT(machine), "nrf51", soc, &error_fatal);
>      object_property_set_link(soc, OBJECT(system_memory),
>                               "memory", &error_abort);
> +    qdev_prop_set_uint32(DEVICE(soc), "variant", NRF51_VARIANT_AA);
>
>      object_property_set_bool(soc, true, "realized", &error_abort);
>
> -    armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
> -            NRF51_SOC(soc)->flash_size);
> +    armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename, 0x00);

Hmm. Using 0x00 here doesn't seem ideal.

>  }
>
>
> diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
> index 441d05e1ef..85bce2c1e0 100644
> --- a/hw/arm/nrf51_soc.c
> +++ b/hw/arm/nrf51_soc.c
> @@ -32,15 +32,21 @@
>  #define FLASH_BASE      0x00000000
>  #define SRAM_BASE       0x20000000
>
> -/* The size and base is for the NRF51822 part. If other parts
> - * are supported in the future, add a sub-class of NRF51SoC for
> - * the specific variants */
> -#define NRF51822_FLASH_SIZE     (256 * 1024)
> -#define NRF51822_SRAM_SIZE      (16 * 1024)
> +#define PAGE_SIZE       1024

Page size isn't really an obvious concept for M profile.

You might find the KiB macro in qemu/units.h useful ?

>
>  /* IRQ lines can be derived from peripheral base addresses */
>  #define BASE_TO_IRQ(base) (((base) >> 12) & 0x1F)
>
> +/* RAM and CODE size in number of pages for different NRF51Variants variants */
> +struct {
> +  hwaddr ram_size;
> +  hwaddr flash_size;
> +} NRF51VariantAttributes[] = {
> +        [NRF51_VARIANT_AA] = {.ram_size = 16, .flash_size = 256 },
> +        [NRF51_VARIANT_AB] = {.ram_size = 16, .flash_size = 128 },
> +        [NRF51_VARIANT_AC] = {.ram_size = 32, .flash_size = 256 },
> +};
> +
>  static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>  {
>      NRF51State *s = NRF51_SOC(dev_soc);
> @@ -51,13 +57,21 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>          return;
>      }
>
> +    if (!(s->part_variant > NRF51_VARIANT_INVALID
> +            && s->part_variant < NRF51_VARIANT_MAX)) {
> +        error_setg(errp, "VARIANT not set or invalid");
> +        return;
> +    }
> +
>      object_property_set_link(OBJECT(&s->cpu), OBJECT(&s->container), "memory",
>              &err);
>      object_property_set_bool(OBJECT(&s->cpu), true, "realized", &err);
>
>      memory_region_add_subregion_overlap(&s->container, 0, s->board_memory, -1);
>
> -    memory_region_init_ram(&s->flash, OBJECT(s), "nrf51.flash", s->flash_size,
> +    /* FLASH */
> +    memory_region_init_ram(&s->flash, NULL, "nrf51_soc.flash",
> +            NRF51VariantAttributes[s->part_variant].flash_size * PAGE_SIZE,
>              &err);
>      if (err) {
>          error_propagate(errp, err);
> @@ -66,7 +80,9 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>      memory_region_set_readonly(&s->flash, true);
>      memory_region_add_subregion(&s->container, FLASH_BASE, &s->flash);
>
> -    memory_region_init_ram(&s->sram, NULL, "nrf51.sram", s->sram_size, &err);
> +    /* SRAM */
> +    memory_region_init_ram(&s->sram, NULL, "nrf51_soc.sram",
> +            NRF51VariantAttributes[s->part_variant].ram_size * PAGE_SIZE, &err);
>      if (err) {
>          error_propagate(errp, err);
>          return;
> @@ -85,17 +101,19 @@ static void nrf51_soc_init(Object *obj)
>      memory_region_init(&s->container, obj, "nrf51-container", UINT64_MAX);
>
>      object_initialize(&s->cpu, sizeof(s->cpu), TYPE_ARMV7M);
> -    object_property_add_child(OBJECT(s), "armv6m", OBJECT(&s->cpu), &error_abort);
> +    object_property_add_child(OBJECT(s), "armv6m", OBJECT(&s->cpu),
> +                              &error_abort);
>      qdev_set_parent_bus(DEVICE(&s->cpu), sysbus_get_default());
> -    qdev_prop_set_string(DEVICE(&s->cpu), "cpu-type", ARM_CPU_TYPE_NAME("cortex-m0"));
> +    qdev_prop_set_string(DEVICE(&s->cpu), "cpu-type",
> +                         ARM_CPU_TYPE_NAME("cortex-m0"));

Why have these lines been changed? (Rule of thumb: don't include code
reformatting or style cleanups in the same patch as substantive changes.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/7] arm: Add additional datasheets and copyright lines

[Peter Maydell]

On 11 August 2018 at 10:08, Steffen Görtz <contrib@steffen-goertz.de> wrote:
> This patch adds a link to the product specification
> which contain additional information about the
> Nordic Semiconductor nRF51 SOC series.
>
> Furthermore it adds a copyright line to all
> files that get changed significantly.
>
> Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
> ---
>  hw/arm/microbit.c          | 1 +
>  hw/arm/nrf51_soc.c         | 4 +++-
>  include/hw/arm/nrf51_soc.h | 1 +
>  3 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c
> index d6776dea0a..bb6ddb6a79 100644
> --- a/hw/arm/microbit.c
> +++ b/hw/arm/microbit.c
> @@ -3,6 +3,7 @@
>   * http://tech.microbit.org/hardware/
>   *
>   * Copyright 2018 Joel Stanley <joel@jms.id.au>
> + * 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.
> diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
> index 85bce2c1e0..2265d30352 100644
> --- a/hw/arm/nrf51_soc.c
> +++ b/hw/arm/nrf51_soc.c
> @@ -1,8 +1,10 @@
>  /*
>   * Nordic Semiconductor nRF51 SoC
> - * http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.1.pdf
> + * Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf

This is downgrading from the 3.0.1 reference manual to the older
3.0. Is that intentional?

> + * Product Spec: http://infocenter.nordicsemi.com/pdf/nRF51822_PS_v3.1.pdf
>   *
>   * Copyright 2018 Joel Stanley <joel@jms.id.au>
> + * 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.
> diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
> index 24212f9174..45d9671dc3 100644
> --- a/include/hw/arm/nrf51_soc.h
> +++ b/include/hw/arm/nrf51_soc.h
> @@ -2,6 +2,7 @@
>   * Nordic Semiconductor nRF51  SoC
>   *
>   * Copyright 2018 Joel Stanley <joel@jms.id.au>
> + * 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.

It probably makes more sense to put the copyright line in the
same patch where you add enough code to the file for it to be
a substantive change.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/7] arm: Improve error propagation in nRF51 SOC

[Peter Maydell]

On 11 August 2018 at 10:08, Steffen Görtz <contrib@steffen-goertz.de> wrote:
> This patch takes care that errors that occur during
> instantiation of the cortex-m0 cpu are properly propagated.
>
> Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
> ---
>  hw/arm/nrf51_soc.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
> index 2265d30352..88a848de8b 100644
> --- a/hw/arm/nrf51_soc.c
> +++ b/hw/arm/nrf51_soc.c
> @@ -66,8 +66,17 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>      }
>
>      object_property_set_link(OBJECT(&s->cpu), OBJECT(&s->container), "memory",
> -            &err);
> -    object_property_set_bool(OBJECT(&s->cpu), true, "realized", &err);
> +                            &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    object_property_set_bool(OBJECT(&s->cpu), true, "realized",
> +                             &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }

This should be squashed into the patch which had the bug in it
(one of Joel's, I think).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 6/7] arm: Instantiate nRF51 peripherals

[Peter Maydell]

On 11 August 2018 at 10:08, Steffen Görtz <contrib@steffen-goertz.de> wrote:
> Instantiate NVMs, NVMC, UART, RNG, GPIO and TIMERs.
>
> Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
> ---
>  hw/arm/nrf51_soc.c         | 153 +++++++++++++++++++++++++++++++++++--
>  include/hw/arm/nrf51_soc.h |  16 +++-
>  2 files changed, 161 insertions(+), 8 deletions(-)
>
> diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
> index 88a848de8b..a395d3a00d 100644
> --- a/hw/arm/nrf51_soc.c
> +++ b/hw/arm/nrf51_soc.c
> @@ -25,15 +25,20 @@
>
>  #include "hw/arm/nrf51_soc.h"
>
> -#define IOMEM_BASE      0x40000000
> -#define IOMEM_SIZE      0x20000000
> -
> -#define FICR_BASE       0x10000000
> -#define FICR_SIZE       0x000000fc
> -
>  #define FLASH_BASE      0x00000000
> +#define FICR_BASE       0x10000000
> +#define UICR_BASE       0x10001000
>  #define SRAM_BASE       0x20000000
>
> +#define IOMEM_BASE      0x40000000
> +#define IOMEM_SIZE      0x20000000
> +
> +#define UART_BASE       0x40002000
> +#define TIMER_BASE      0x40008000
> +#define RNG_BASE        0x4000D000
> +#define NVMC_BASE       0x4001E000
> +#define GPIO_BASE       0x50000000
> +
>  #define PAGE_SIZE       1024
>
>  /* IRQ lines can be derived from peripheral base addresses */
> @@ -49,10 +54,33 @@ struct {
>          [NRF51_VARIANT_AC] = {.ram_size = 32, .flash_size = 256 },
>  };
>
> +
> +static uint64_t clock_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u]\n",
> +                  __func__, addr, size);
> +    return 1;
> +}
> +
> +static void clock_write(void *opaque, hwaddr addr, uint64_t data,
> +                        unsigned int size)
> +{
> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n",
> +                  __func__, addr, data, size);
> +}
> +
> +static const MemoryRegionOps clock_ops = {
> +    .read = clock_read,
> +    .write = clock_write
> +};

You don't need to roll your own "do nothing but log" device:
you can use the TYPE_UNIMPLEMENTED_DEVICE to do this.

>  static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>  {
>      NRF51State *s = NRF51_SOC(dev_soc);
>      Error *err = NULL;
> +    MemoryRegion *mr = NULL;
> +    size_t i;
> +    qemu_irq irq;
>
>      if (!s->board_memory) {
>          error_setg(errp, "memory property was not set");
> @@ -100,14 +128,102 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>      }
>      memory_region_add_subregion(&s->container, SRAM_BASE, &s->sram);
>
> +
> +    /* UART */
> +    qdev_prop_set_chr(DEVICE(&s->uart), "chardev", serial_hd(0));
> +    object_property_set_bool(OBJECT(&s->uart), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->uart), 0);
> +    memory_region_add_subregion_overlap(&s->container, UART_BASE, mr, 0);
> +    irq = qdev_get_gpio_in(DEVICE(&s->cpu), BASE_TO_IRQ(UART_BASE));
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart), 0, irq);

Usual approach is
   sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart), 0,
                      qdev_get_gpio_in(DEVICE(&s->cpu),
                                       BASE_TO_IRQ(UART_BASE));

rather than a local qemu_irq (which tends to look like the SoC itself
has an irq line floating around, rather than just doing plumbing).

> +
> +    /* TIMER */
> +    for (i = 0; i < NRF51_TIMER_NUM; i++) {
> +        object_property_set_bool(OBJECT(&s->timer[i]), true, "realized", &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->timer[i]), 0,
> +                     TIMER_BASE + i * 0x1000);
> +
> +        irq = qdev_get_gpio_in(DEVICE(&s->cpu),
> +                               BASE_TO_IRQ(TIMER_BASE + i * 0x1000));

Put the timer_base in a local rather than recalculating the expression
for sysbus_mmio_map() and BASE_TO_IRQ().

> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->timer[i]), 0, irq);
> +    }
> +
> +    /* NVMC */
> +    object_property_set_link(OBJECT(&s->nvm), OBJECT(&s->container),
> +                                         "memory", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    object_property_set_uint(OBJECT(&s->nvm),
> +            NRF51VariantAttributes[s->part_variant].flash_size, "code_size",
> +            &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    object_property_set_bool(OBJECT(&s->nvm), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->nvm), 0);
> +    memory_region_add_subregion_overlap(&s->container, NVMC_BASE, mr, 0);
> +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->nvm), 1);
> +    memory_region_add_subregion_overlap(&s->container, FICR_BASE, mr, 0);
> +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->nvm), 2);
> +    memory_region_add_subregion_overlap(&s->container, UICR_BASE, mr, 0);
> +
> +    /* RNG */
> +    object_property_set_bool(OBJECT(&s->rng), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->rng), 0);
> +    memory_region_add_subregion_overlap(&s->container, RNG_BASE, mr, 0);
> +    irq = qdev_get_gpio_in(DEVICE(&s->cpu), BASE_TO_IRQ(RNG_BASE));
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->rng), 0, irq);
> +
> +    /* GPIO */
> +    object_property_set_bool(OBJECT(&s->gpio), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gpio), 0);
> +    memory_region_add_subregion_overlap(&s->container, GPIO_BASE, mr, 0);
> +
> +    /* Pass all GPIOs to the SOC layer so they are available to the board */
> +    qdev_pass_gpios(DEVICE(&s->gpio), dev_soc, NULL);

Consider making these named GPIO lines rather than unnamed ones?
Unnamed is clear enough for the GPIO device itself but a bit odd
on the SoC device.

> +
> +    /* STUB Peripherals */
> +    memory_region_init_io(&s->clock, NULL, &clock_ops, NULL,
> +                          "nrf51_soc.clock", 0x1000);
> +    memory_region_add_subregion_overlap(&s->container, IOMEM_BASE, &s->clock,
> +                                        -1);
> +
>      create_unimplemented_device("nrf51_soc.io", IOMEM_BASE, IOMEM_SIZE);
> -    create_unimplemented_device("nrf51_soc.ficr", FICR_BASE, FICR_SIZE);
>      create_unimplemented_device("nrf51_soc.private", 0xF0000000, 0x10000000);
>  }
>
>  static void nrf51_soc_init(Object *obj)
>  {
>      NRF51State *s = NRF51_SOC(obj);
> +    size_t i;
>
>      memory_region_init(&s->container, obj, "nrf51-container", UINT64_MAX);
>
> @@ -118,6 +234,29 @@ static void nrf51_soc_init(Object *obj)
>      qdev_prop_set_string(DEVICE(&s->cpu), "cpu-type",
>                           ARM_CPU_TYPE_NAME("cortex-m0"));
>      qdev_prop_set_uint32(DEVICE(&s->cpu), "num-irq", 32);
> +
> +    object_initialize(&s->uart, sizeof(s->uart), TYPE_NRF51_UART);
> +    object_property_add_child(obj, "uart", OBJECT(&s->uart), &error_abort);
> +    qdev_set_parent_bus(DEVICE(&s->uart), sysbus_get_default());

These should be using sys_bus_init_child_obj() (which probably didn't
exist when you wrote this code).

> +
> +    object_initialize(&s->nvm, sizeof(s->nvm), TYPE_NRF51_NVM);
> +    object_property_add_child(obj, "nvm", OBJECT(&s->nvm), &error_abort);
> +    qdev_set_parent_bus(DEVICE(&s->nvm), sysbus_get_default());
> +
> +    object_initialize(&s->rng, sizeof(s->rng), TYPE_NRF51_RNG);
> +    object_property_add_child(obj, "rng", OBJECT(&s->rng), &error_abort);
> +    qdev_set_parent_bus(DEVICE(&s->rng), sysbus_get_default());
> +
> +    object_initialize(&s->gpio, sizeof(s->gpio), TYPE_NRF51_GPIO);
> +    object_property_add_child(obj, "gpio", OBJECT(&s->gpio), &error_abort);
> +    qdev_set_parent_bus(DEVICE(&s->gpio), sysbus_get_default());
> +
> +    for (i = 0; i < NRF51_TIMER_NUM; i++) {
> +        object_initialize(&s->timer[i], sizeof(s->timer[i]), TYPE_NRF51_TIMER);
> +        object_property_add_child(obj, "timer[*]", OBJECT(&s->timer[i]), NULL);
> +        qdev_set_parent_bus(DEVICE(&s->timer[i]), sysbus_get_default());
> +    }
> +
>  }
>
>  static Property nrf51_soc_properties[] = {
> diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
> index 45d9671dc3..d47b42fa37 100644
> --- a/include/hw/arm/nrf51_soc.h
> +++ b/include/hw/arm/nrf51_soc.h
> @@ -14,11 +14,18 @@
>  #include "qemu/osdep.h"
>  #include "hw/sysbus.h"
>  #include "hw/arm/armv7m.h"
> +#include "hw/char/nrf51_uart.h"
> +#include "hw/misc/nrf51_rng.h"
> +#include "hw/nvram/nrf51_nvm.h"
> +#include "hw/gpio/nrf51_gpio.h"
> +#include "hw/timer/nrf51_timer.h"
>
>  #define TYPE_NRF51_SOC "nrf51-soc"
>  #define NRF51_SOC(obj) \
>      OBJECT_CHECK(NRF51State, (obj), TYPE_NRF51_SOC)
>
> +#define NRF51_TIMER_NUM 3

NRF51_NUM_TIMERS I think makes the code read a little better.


> +
>  typedef struct NRF51State {
>      /*< private >*/
>      SysBusDevice parent_obj;
> @@ -31,9 +38,16 @@ typedef struct NRF51State {
>      MemoryRegion flash;
>
>      MemoryRegion *board_memory;
> -
>      MemoryRegion container;
>
> +    MemoryRegion clock;
> +
> +    Nrf51UART uart;
> +    Nrf51NVMState nvm;
> +    Nrf51RNGState rng;
> +    Nrf51GPIOState gpio;
> +    Nrf51TimerState timer[NRF51_TIMER_NUM];

Can we be consistent about whether we name the structs
Nrf51Thingy (as here) or NRF51Thingy (as in the NRF51State
struct we're adding them to). NRF51Thingy fits our
conventions better, I think.

> +
>      /* Properties */
>      int32_t part_variant;
>  } NRF51State;
> --
> 2.18.0
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 7/7] arm: Instantiate Microbit board-level devices

[Peter Maydell]

On 11 August 2018 at 10:08, Steffen Görtz <contrib@steffen-goertz.de> wrote:
> Instantiate the LED matrix and set board-level
> pull-up default values to buttons A and B.
> This is necessary to calm down the microsoft pxt
> javascript runtime available for the micro:bit.
>
> Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>

> +static void microbit_reset(void)
> +{
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +    MicrobitMachineState *s = MICROBIT_MACHINE(machine);
> +
> +    qemu_devices_reset();
> +
> +    /* Board level pull-up */
> +    if (!qtest_enabled()) {
> +        qemu_set_irq(qdev_get_gpio_in(DEVICE(&s->nrf51), BUTTON_A_PIN), 1);
> +        qemu_set_irq(qdev_get_gpio_in(DEVICE(&s->nrf51), BUTTON_B_PIN), 1);
> +    }

Reset functions should not call qemu_set_irq().

The problem is that there's no ordering on device reset functions,
so there's no guarantee that the reset function on this end is
called after the reset function for the device it's trying to set
the input GPIO line for. If the two resets happen in the other order
then the device will forget that the input line was high.

There is no good way to handle a line that is supposed to be
pulled high on reset, I'm afraid. You just have to try to avoid
modelling things in a way that requires it.

(Yes, our modelling of reset is basically broken. It's a
hard design problem that nobody's found the time to tackle
properly. You'll see various more-or-less dubious workarounds
if you dig in the right parts of the code base...)

> +}
> +
>
>  static void microbit_machine_init(MachineClass *mc)
>  {
>      mc->desc = "BBC micro:bit";
>      mc->init = microbit_init;
>      mc->max_cpus = 1;
> +    mc->reset = microbit_reset;
>  }
>
>  static void microbit_machine_init_class_init(ObjectClass *oc, void *data)
> diff --git a/include/hw/arm/microbit.h b/include/hw/arm/microbit.h
> index 89f0c6bc07..094326b4ae 100644
> --- a/include/hw/arm/microbit.h
> +++ b/include/hw/arm/microbit.h
> @@ -24,6 +24,7 @@ typedef struct MicrobitMachineState {
>      MachineState parent_obj;
>
>      NRF51State nrf51;
> +    LEDMatrixState matrix;
>  } MicrobitMachineState;

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/7] arm: Add chip variant property to nRF51 SoC

[Steffen Görtz]

Hi Peter,

object_property_set_bool(soc, true, "realized", &error_abort);>>
>> -    armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
>> -            NRF51_SOC(soc)->flash_size);
>> +    armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename, 0x00);
> 
> Hmm. Using 0x00 here doesn't seem ideal.

i agree. The actual sizes for RAM/ROM are calculated in the SOC at the moment.
I do not really know what impact max_sz has here. I could set the it to the
maximum code size defined in the memory map of the NRF51 series. Another way would be 
to lift the calculation of the ROM size from the SOC to the Board. What would you propose?
>> +#define PAGE_SIZE       1024
> 
> Page size isn't really an obvious concept for M profile.

Flash pages are referred to here, not MMU pages. This constant is moved and renamed to NRF51_PAGE_SIZE in the upcoming revision. Would you suggest another name or can with stick with this?

> Why have these lines been changed? (Rule of thumb: don't include code
> reformatting or style cleanups in the same patch as substantive changes.)

These lines where > 80 chars before. I should rather suggest a change of Joel's patch series.

Thank you for your remarks.

Steffen

Re: [Qemu-devel] [PATCH 6/7] arm: Instantiate nRF51 peripherals

[Steffen Görtz]

Hi Peter,
>> +
>> +static uint64_t clock_read(void *opaque, hwaddr addr, unsigned int size)
>> +{
>> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u]\n",
>> +                  __func__, addr, size);
>> +    return 1;
>> +}
>> +
>> +static void clock_write(void *opaque, hwaddr addr, uint64_t data,
>> +                        unsigned int size)
>> +{
>> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n",
>> +                  __func__, addr, data, size);
>> +}
>> +
>> +static const MemoryRegionOps clock_ops = {
>> +    .read = clock_read,
>> +    .write = clock_write
>> +};
> 
> You don't need to roll your own "do nothing but log" device:
> you can use the TYPE_UNIMPLEMENTED_DEVICE to do this.
> 
Until we have a more proper STUB for POWER/CLOCK/MPU peripherals we actually need this,
because the unimplemented device will return 0 on reads which will cause guest code to stall.

Agree to and regarded the rest of your remarks.

Steffen

Re: [Qemu-devel] [PATCH 6/7] arm: Instantiate nRF51 peripherals

[Julia Suvorova]

On 21.08.2018 14:43, Steffen Görtz wrote:
> Hi Peter,
>>> +
>>> +static uint64_t clock_read(void *opaque, hwaddr addr, unsigned int size)
>>> +{
>>> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u]\n",
>>> +                  __func__, addr, size);
>>> +    return 1;
>>> +}
>>> +
>>> +static void clock_write(void *opaque, hwaddr addr, uint64_t data,
>>> +                        unsigned int size)
>>> +{
>>> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n",
>>> +                  __func__, addr, data, size);
>>> +}
>>> +
>>> +static const MemoryRegionOps clock_ops = {
>>> +    .read = clock_read,
>>> +    .write = clock_write
>>> +};
>>
>> You don't need to roll your own "do nothing but log" device:
>> you can use the TYPE_UNIMPLEMENTED_DEVICE to do this.
>
> Until we have a more proper STUB for POWER/CLOCK/MPU peripherals we actually need this,
> because the unimplemented device will return 0 on reads which will cause guest code to stall.

Do you need this stub to run micropython code? Could you delete this,
please, so that I can post the correct stub structures for js
directly on Joel's code?

Best regards, Julia Suvorova.

Re: [Qemu-devel] [PATCH 3/7] arm: Add chip variant property to nRF51 SoC

[Peter Maydell]

On 21 August 2018 at 11:03, Steffen Görtz <mail@steffen-goertz.de> wrote:
> Hi Peter,
>
> object_property_set_bool(soc, true, "realized", &error_abort);>>
>>> -    armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
>>> -            NRF51_SOC(soc)->flash_size);
>>> +    armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename, 0x00);
>>
>> Hmm. Using 0x00 here doesn't seem ideal.
>
> i agree. The actual sizes for RAM/ROM are calculated in the SOC at the moment.
> I do not really know what impact max_sz has here. I could set the it to the
> maximum code size defined in the memory map of the NRF51 series. Another way would be
> to lift the calculation of the ROM size from the SOC to the Board. What would you propose?

I think we need to specify a value somehow, otherwise we
won't be able to load raw images (which I think are the only
ones where we enforce the limit value). You could just use
the largest possible size, I suppose.

>>> +#define PAGE_SIZE       1024
>>
>> Page size isn't really an obvious concept for M profile.
>
> Flash pages are referred to here, not MMU pages. This constant is moved and renamed to NRF51_PAGE_SIZE in the upcoming revision. Would you suggest another name or can with stick with this?

I guess NRF51_FLASH_PAGE_SIZE would be a little clearer ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 6/7] arm: Instantiate nRF51 peripherals

[Peter Maydell]

On 21 August 2018 at 12:43, Steffen Görtz <mail@steffen-goertz.de> wrote:
> Hi Peter,
>>> +
>>> +static uint64_t clock_read(void *opaque, hwaddr addr, unsigned int size)
>>> +{
>>> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u]\n",
>>> +                  __func__, addr, size);
>>> +    return 1;
>>> +}
>>> +
>>> +static void clock_write(void *opaque, hwaddr addr, uint64_t data,
>>> +                        unsigned int size)
>>> +{
>>> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n",
>>> +                  __func__, addr, data, size);
>>> +}
>>> +
>>> +static const MemoryRegionOps clock_ops = {
>>> +    .read = clock_read,
>>> +    .write = clock_write
>>> +};
>>
>> You don't need to roll your own "do nothing but log" device:
>> you can use the TYPE_UNIMPLEMENTED_DEVICE to do this.
>>
> Until we have a more proper STUB for POWER/CLOCK/MPU peripherals we actually need this,
> because the unimplemented device will return 0 on reads which will cause guest code to stall.

Oh, I see. I hadn't noticed that it was doing read-as-all-ones.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 6/7] arm: Instantiate nRF51 peripherals

[Steffen Görtz]

Hi Julia,
> Do you need this stub to run micropython code? Could you delete this,
> please, so that I can post the correct stub structures for js
> directly on Joel's code?
Sure i can remove the code from my patch. micropython will not work but i dont think this is a problem as long as you provide patches shortly after. 

Steffen