[Qemu-devel] [PATCH v2 0/4] arm: Add nRF51 SoC UART support

[Qemu-devel] [PATCH v2 0/4] arm: Add nRF51 SoC UART support

[Julia Suvorova]

This series adds support for the nRF51 SoC UART, that used in
BBC Micro:bit board, and QTest for it.

v2:
    * Suspend/Enable functionality added
    * Connection to SoC moved to a separate patch
    * Added QTest for checking reception functionality
    * Mini-kernel test changed to fit current implementation
    * Addressed review comments on R_*, uart_can_receive, VMState,
      uart_transmit

Julia Suvorova (4):
  hw/char: Implement nRF51 SoC UART
  hw/arm/nrf51_soc: Connect UART to nRF51 SoC
  tests/boot-serial-test: Add microbit board testcase
  tests/microbit-test: Check nRF51 UART functionality

 hw/arm/nrf51_soc.c           |  20 +++
 hw/char/Makefile.objs        |   1 +
 hw/char/nrf51_uart.c         | 329 +++++++++++++++++++++++++++++++++++
 hw/char/trace-events         |   4 +
 include/hw/arm/nrf51_soc.h   |   3 +
 include/hw/char/nrf51_uart.h |  78 +++++++++
 tests/boot-serial-test.c     |  19 ++
 tests/microbit-test.c        | 106 ++++++++++-
 8 files changed, 557 insertions(+), 3 deletions(-)
 create mode 100644 hw/char/nrf51_uart.c
 create mode 100644 include/hw/char/nrf51_uart.h

-- 
2.17.1

[Qemu-devel] [PATCH v2 1/4] hw/char: Implement nRF51 SoC UART

[Julia Suvorova]

Not implemented: CTS/NCTS, PSEL*.

Signed-off-by: Julia Suvorova <jusual@mail.ru>
---
 hw/char/Makefile.objs        |   1 +
 hw/char/nrf51_uart.c         | 329 +++++++++++++++++++++++++++++++++++
 hw/char/trace-events         |   4 +
 include/hw/char/nrf51_uart.h |  78 +++++++++
 4 files changed, 412 insertions(+)
 create mode 100644 hw/char/nrf51_uart.c
 create mode 100644 include/hw/char/nrf51_uart.h

diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
index b570531291..c4947d7ae7 100644
--- a/hw/char/Makefile.objs
+++ b/hw/char/Makefile.objs
@@ -1,5 +1,6 @@
 common-obj-$(CONFIG_IPACK) += ipoctal232.o
 common-obj-$(CONFIG_ESCC) += escc.o
+common-obj-$(CONFIG_NRF51_SOC) += nrf51_uart.o
 common-obj-$(CONFIG_PARALLEL) += parallel.o
 common-obj-$(CONFIG_PARALLEL) += parallel-isa.o
 common-obj-$(CONFIG_PL011) += pl011.o
diff --git a/hw/char/nrf51_uart.c b/hw/char/nrf51_uart.c
new file mode 100644
index 0000000000..55404e8f37
--- /dev/null
+++ b/hw/char/nrf51_uart.c
@@ -0,0 +1,329 @@
+/*
+ * nRF51 SoC UART emulation
+ *
+ * See nRF51 Series Reference Manual, "29 Universal Asynchronous
+ * Receiver/Transmitter" for hardware specifications:
+ * http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
+ *
+ * Copyright (c) 2018 Julia Suvorova <jusual@mail.ru>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 or
+ * (at your option) any later version.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/char/nrf51_uart.h"
+#include "trace.h"
+
+static void nrf51_uart_update_irq(NRF51UARTState *s)
+{
+    bool irq = false;
+
+    irq |= (s->reg[R_UART_RXDRDY] &&
+            (s->reg[R_UART_INTEN] & R_UART_INTEN_RXDRDY_MASK));
+    irq |= (s->reg[R_UART_TXDRDY] &&
+            (s->reg[R_UART_INTEN] & R_UART_INTEN_TXDRDY_MASK));
+    irq |= (s->reg[R_UART_ERROR]  &&
+            (s->reg[R_UART_INTEN] & R_UART_INTEN_ERROR_MASK));
+    irq |= (s->reg[R_UART_RXTO]   &&
+            (s->reg[R_UART_INTEN] & R_UART_INTEN_RXTO_MASK));
+
+    qemu_set_irq(s->irq, irq);
+}
+
+static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    NRF51UARTState *s = NRF51_UART(opaque);
+    uint64_t r;
+
+    if (!s->enabled) {
+        return 0;
+    }
+
+    switch (addr) {
+    case A_UART_RXD:
+        r = s->rx_fifo[s->rx_fifo_pos];
+        if (s->rx_started && s->rx_fifo_len) {
+            qemu_chr_fe_accept_input(&s->chr);
+            s->rx_fifo_pos = (s->rx_fifo_pos + 1) % UART_FIFO_LENGTH;
+            s->rx_fifo_len--;
+            if (s->rx_fifo_len) {
+                s->reg[R_UART_RXDRDY] = 1;
+                nrf51_uart_update_irq(s);
+            }
+        }
+        break;
+    case A_UART_INTENSET:
+    case A_UART_INTENCLR:
+    case A_UART_INTEN:
+        r = s->reg[R_UART_INTEN];
+        break;
+    default:
+        r = s->reg[addr / 4];
+        break;
+    }
+
+    trace_nrf51_uart_read(addr, r, size);
+
+    return r;
+}
+
+static gboolean uart_transmit(GIOChannel *chan, GIOCondition cond, void *opaque)
+{
+    NRF51UARTState *s = NRF51_UART(opaque);
+    int r;
+    uint8_t c = s->reg[R_UART_TXD];
+
+    s->watch_tag = 0;
+
+    r = qemu_chr_fe_write(&s->chr, &c, 1);
+    if (r <= 0) {
+        s->watch_tag = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
+                                             uart_transmit, s);
+        if (!s->watch_tag) {
+            /* The hardware has no transmit error reporting,
+             * so silently drop the byte
+             */
+            goto buffer_drained;
+        }
+        return FALSE;
+    }
+
+buffer_drained:
+    s->reg[R_UART_TXDRDY] = 1;
+    s->pending_tx_byte = false;
+    return FALSE;
+}
+
+static void uart_cancel_transmit(NRF51UARTState *s)
+{
+    if (s->watch_tag) {
+        g_source_remove(s->watch_tag);
+        s->watch_tag = 0;
+    }
+}
+
+static void uart_write(void *opaque, hwaddr addr,
+                       uint64_t value, unsigned int size)
+{
+    NRF51UARTState *s = NRF51_UART(opaque);
+
+    trace_nrf51_uart_write(addr, value, size);
+
+    if (!s->enabled && (addr != A_UART_ENABLE)) {
+        return;
+    }
+
+    switch (addr) {
+    case A_UART_TXD:
+        if (!s->pending_tx_byte && s->tx_started) {
+            s->reg[R_UART_TXD] = value;
+            s->pending_tx_byte = true;
+            uart_transmit(NULL, G_IO_OUT, s);
+        }
+        break;
+    case A_UART_INTEN:
+        s->reg[R_UART_INTEN] = value;
+        break;
+    case A_UART_INTENSET:
+        s->reg[R_UART_INTEN] |= value;
+        break;
+    case A_UART_INTENCLR:
+        s->reg[R_UART_INTEN] &= ~value;
+        break;
+    case A_UART_TXDRDY ... A_UART_RXTO:
+        s->reg[addr / 4] = value;
+        break;
+    case A_UART_ERRORSRC:
+        s->reg[addr / 4] &= ~value;
+        break;
+    case A_UART_RXD:
+        break;
+    case A_UART_RXDRDY:
+        if (value == 0) {
+            s->reg[R_UART_RXDRDY] = 0;
+        }
+        break;
+    case A_UART_STARTTX:
+        if (value == 1) {
+            s->tx_started = true;
+        }
+        break;
+    case A_UART_STARTRX:
+        if (value == 1) {
+            s->rx_started = true;
+        }
+        break;
+    case A_UART_ENABLE:
+        if (value) {
+            if (value == 4) {
+                s->enabled = true;
+            }
+            break;
+        }
+        s->enabled = false;
+        value = 1;
+        /* fall through */
+    case A_UART_SUSPEND:
+    case A_UART_STOPTX:
+        if (value == 1) {
+            s->tx_started = false;
+        }
+    case A_UART_STOPRX:
+        if (addr != A_UART_STOPTX && value == 1) {
+            s->rx_started = false;
+            s->reg[R_UART_RXTO] = 1;
+        }
+        break;
+    default:
+        s->reg[addr / 4] = value;
+        break;
+    }
+    nrf51_uart_update_irq(s);
+}
+
+static const MemoryRegionOps uart_ops = {
+    .read =  uart_read,
+    .write = uart_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void nrf51_uart_reset(DeviceState *dev)
+{
+    NRF51UARTState *s = NRF51_UART(dev);
+
+    s->pending_tx_byte = 0;
+
+    uart_cancel_transmit(s);
+
+    memset(s->reg, 0, sizeof(s->reg));
+
+    s->reg[R_UART_PSELRTS] = 0xFFFFFFFF;
+    s->reg[R_UART_PSELTXD] = 0xFFFFFFFF;
+    s->reg[R_UART_PSELCTS] = 0xFFFFFFFF;
+    s->reg[R_UART_PSELRXD] = 0xFFFFFFFF;
+    s->reg[R_UART_BAUDRATE] = 0x4000000;
+
+    s->rx_fifo_len = 0;
+    s->rx_fifo_pos = 0;
+    s->rx_started = false;
+    s->tx_started = false;
+    s->enabled = false;
+}
+
+static void uart_receive(void *opaque, const uint8_t *buf, int size)
+{
+
+    NRF51UARTState *s = NRF51_UART(opaque);
+    int i;
+
+    if (size == 0 || s->rx_fifo_len >= UART_FIFO_LENGTH) {
+        return;
+    }
+
+    for (i = 0; i < size; i++) {
+        uint32_t pos = (s->rx_fifo_pos + s->rx_fifo_len) % UART_FIFO_LENGTH;
+        s->rx_fifo[pos] = buf[i];
+        s->rx_fifo_len++;
+    }
+
+    s->reg[R_UART_RXDRDY] = 1;
+    nrf51_uart_update_irq(s);
+}
+
+static int uart_can_receive(void *opaque)
+{
+    NRF51UARTState *s = NRF51_UART(opaque);
+
+    return s->rx_started ? (UART_FIFO_LENGTH - s->rx_fifo_len) : 0;
+}
+
+static void uart_event(void *opaque, int event)
+{
+    NRF51UARTState *s = NRF51_UART(opaque);
+
+    if (event == CHR_EVENT_BREAK) {
+        s->reg[R_UART_ERRORSRC] |= 3;
+        s->reg[R_UART_ERROR] = 1;
+        nrf51_uart_update_irq(s);
+    }
+}
+
+static void nrf51_uart_realize(DeviceState *dev, Error **errp)
+{
+    NRF51UARTState *s = NRF51_UART(dev);
+
+    qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
+                             uart_event, NULL, s, NULL, true);
+}
+
+static void nrf51_uart_init(Object *obj)
+{
+    NRF51UARTState *s = NRF51_UART(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+
+    memory_region_init_io(&s->iomem, obj, &uart_ops, s,
+                          "nrf51_soc.uart", UART_SIZE);
+    sysbus_init_mmio(sbd, &s->iomem);
+    sysbus_init_irq(sbd, &s->irq);
+}
+
+static int nrf51_uart_post_load(void *opaque, int version_id)
+{
+    NRF51UARTState *s = NRF51_UART(opaque);
+
+    if (s->pending_tx_byte) {
+        s->watch_tag = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
+                                             uart_transmit, s);
+    }
+
+    return 0;
+}
+
+static const VMStateDescription nrf51_uart_vmstate = {
+    .name = "nrf51_soc.uart",
+    .post_load = nrf51_uart_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(reg, NRF51UARTState, 0x56C),
+        VMSTATE_UINT8_ARRAY(rx_fifo, NRF51UARTState, UART_FIFO_LENGTH),
+        VMSTATE_UINT32(rx_fifo_pos, NRF51UARTState),
+        VMSTATE_UINT32(rx_fifo_len, NRF51UARTState),
+        VMSTATE_BOOL(rx_started, NRF51UARTState),
+        VMSTATE_BOOL(tx_started, NRF51UARTState),
+        VMSTATE_BOOL(pending_tx_byte, NRF51UARTState),
+        VMSTATE_BOOL(enabled, NRF51UARTState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static Property nrf51_uart_properties[] = {
+    DEFINE_PROP_CHR("chardev", NRF51UARTState, chr),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void nrf51_uart_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = nrf51_uart_reset;
+    dc->realize = nrf51_uart_realize;
+    dc->props = nrf51_uart_properties;
+    dc->vmsd = &nrf51_uart_vmstate;
+}
+
+static const TypeInfo nrf51_uart_info = {
+    .name = TYPE_NRF51_UART,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(NRF51UARTState),
+    .instance_init = nrf51_uart_init,
+    .class_init = nrf51_uart_class_init
+};
+
+static void nrf51_uart_register_types(void)
+{
+    type_register_static(&nrf51_uart_info);
+}
+
+type_init(nrf51_uart_register_types)
diff --git a/hw/char/trace-events b/hw/char/trace-events
index b64213d4dd..de34a74399 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -73,3 +73,7 @@ cmsdk_apb_uart_receive(uint8_t c) "CMSDK APB UART: got character 0x%x from backe
 cmsdk_apb_uart_tx_pending(void) "CMSDK APB UART: character send to backend pending"
 cmsdk_apb_uart_tx(uint8_t c) "CMSDK APB UART: character 0x%x sent to backend"
 cmsdk_apb_uart_set_params(int speed) "CMSDK APB UART: params set to %d 8N1"
+
+# hw/char/nrf51_uart.c
+nrf51_uart_read(uint64_t addr, uint64_t r, unsigned int size) "addr 0x%" PRIx64 " value 0x%" PRIx64 " size %u"
+nrf51_uart_write(uint64_t addr, uint64_t value, unsigned int size) "addr 0x%" PRIx64 " value 0x%" PRIx64 " size %u"
diff --git a/include/hw/char/nrf51_uart.h b/include/hw/char/nrf51_uart.h
new file mode 100644
index 0000000000..1d49b53da7
--- /dev/null
+++ b/include/hw/char/nrf51_uart.h
@@ -0,0 +1,78 @@
+/*
+ * nRF51 SoC UART emulation
+ *
+ * Copyright (c) 2018 Julia Suvorova <jusual@mail.ru>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 or
+ * (at your option) any later version.
+ */
+
+#ifndef NRF51_UART_H
+#define NRF51_UART_H
+
+#include "hw/sysbus.h"
+#include "chardev/char-fe.h"
+#include "hw/registerfields.h"
+
+#define UART_FIFO_LENGTH 6
+#define UART_BASE 0x40002000
+#define UART_SIZE 0x1000
+
+#define TYPE_NRF51_UART "nrf51_soc.uart"
+#define NRF51_UART(obj) OBJECT_CHECK(NRF51UARTState, (obj), TYPE_NRF51_UART)
+
+REG32(UART_STARTRX, 0x000)
+REG32(UART_STOPRX, 0x004)
+REG32(UART_STARTTX, 0x008)
+REG32(UART_STOPTX, 0x00C)
+REG32(UART_SUSPEND, 0x01C)
+
+REG32(UART_CTS, 0x100)
+REG32(UART_NCTS, 0x104)
+REG32(UART_RXDRDY, 0x108)
+REG32(UART_TXDRDY, 0x11C)
+REG32(UART_ERROR, 0x124)
+REG32(UART_RXTO, 0x144)
+
+REG32(UART_INTEN, 0x300)
+    FIELD(UART_INTEN, CTS, 0, 1)
+    FIELD(UART_INTEN, NCTS, 1, 1)
+    FIELD(UART_INTEN, RXDRDY, 2, 1)
+    FIELD(UART_INTEN, TXDRDY, 7, 1)
+    FIELD(UART_INTEN, ERROR, 9, 1)
+    FIELD(UART_INTEN, RXTO, 17, 1)
+REG32(UART_INTENSET, 0x304)
+REG32(UART_INTENCLR, 0x308)
+REG32(UART_ERRORSRC, 0x480)
+REG32(UART_ENABLE, 0x500)
+REG32(UART_PSELRTS, 0x508)
+REG32(UART_PSELTXD, 0x50C)
+REG32(UART_PSELCTS, 0x510)
+REG32(UART_PSELRXD, 0x514)
+REG32(UART_RXD, 0x518)
+REG32(UART_TXD, 0x51C)
+REG32(UART_BAUDRATE, 0x524)
+REG32(UART_CONFIG, 0x56C)
+
+typedef struct NRF51UARTState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion iomem;
+    CharBackend chr;
+    qemu_irq irq;
+    guint watch_tag;
+
+    uint8_t rx_fifo[UART_FIFO_LENGTH];
+    unsigned int rx_fifo_pos;
+    unsigned int rx_fifo_len;
+
+    uint32_t reg[0x56C];
+
+    bool rx_started;
+    bool tx_started;
+    bool pending_tx_byte;
+    bool enabled;
+} NRF51UARTState;
+
+#endif
-- 
2.17.1

[Qemu-devel] [PATCH v2 2/4] hw/arm/nrf51_soc: Connect UART to nRF51 SoC

[Julia Suvorova]

Wire up nRF51 UART in the corresponding SoC using in-place init/realize.

Based-on: <20180803052137.10602-1-joel@jms.id.au>

Signed-off-by: Julia Suvorova <jusual@mail.ru>
---
 hw/arm/nrf51_soc.c         | 20 ++++++++++++++++++++
 include/hw/arm/nrf51_soc.h |  3 +++
 2 files changed, 23 insertions(+)

diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 9f9649c780..8b5602f363 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -38,9 +38,12 @@
 #define NRF51822_FLASH_SIZE     (256 * 1024)
 #define NRF51822_SRAM_SIZE      (16 * 1024)
 
+#define BASE_TO_IRQ(base) ((base >> 12) & 0x1F)
+
 static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
 {
     NRF51State *s = NRF51_SOC(dev_soc);
+    MemoryRegion *mr = NULL;
     Error *err = NULL;
 
     if (!s->board_memory) {
@@ -70,6 +73,19 @@ 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);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart), 0,
+                       qdev_get_gpio_in(DEVICE(&s->cpu),
+                                        BASE_TO_IRQ(UART_BASE)));
+
     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);
@@ -86,6 +102,10 @@ static void nrf51_soc_init(Object *obj)
     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_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());
 }
 
 static Property nrf51_soc_properties[] = {
diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
index e380ec26b8..46a1c1a66c 100644
--- a/include/hw/arm/nrf51_soc.h
+++ b/include/hw/arm/nrf51_soc.h
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 #include "hw/sysbus.h"
 #include "hw/arm/armv7m.h"
+#include "hw/char/nrf51_uart.h"
 
 #define TYPE_NRF51_SOC "nrf51-soc"
 #define NRF51_SOC(obj) \
@@ -25,6 +26,8 @@ typedef struct NRF51State {
     /*< public >*/
     ARMv7MState cpu;
 
+    NRF51UARTState uart;
+
     MemoryRegion iomem;
     MemoryRegion sram;
     MemoryRegion flash;
-- 
2.17.1

[Qemu-devel] [PATCH v2 3/4] tests/boot-serial-test: Add microbit board testcase

[Julia Suvorova]

New mini-kernel test for nRF51 SoC UART.

Signed-off-by: Julia Suvorova <jusual@mail.ru>
---
 tests/boot-serial-test.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
index 952a2e7ead..19714c3f87 100644
--- a/tests/boot-serial-test.c
+++ b/tests/boot-serial-test.c
@@ -62,6 +62,24 @@ static const uint8_t kernel_aarch64[] = {
     0xfd, 0xff, 0xff, 0x17,                 /* b       -12 (loop) */
 };
 
+static const uint8_t kernel_nrf51[] = {
+    0x00, 0x00, 0x00, 0x00,                 /* Stack top address */
+    0x09, 0x00, 0x00, 0x00,                 /* Reset handler address */
+    0x04, 0x4a,                             /* ldr  r2, [pc, #16] Get ENABLE */
+    0x04, 0x21,                             /* movs r1, #4 */
+    0x11, 0x60,                             /* str  r1, [r2] */
+    0x04, 0x4a,                             /* ldr  r2, [pc, #16] Get STARTTX */
+    0x01, 0x21,                             /* movs r1, #1 */
+    0x11, 0x60,                             /* str  r1, [r2] */
+    0x03, 0x4a,                             /* ldr  r2, [pc, #12] Get TXD */
+    0x54, 0x21,                             /* movs r1, 'T' */
+    0x11, 0x60,                             /* str  r1, [r2] */
+    0xfe, 0xe7,                             /* b    . */
+    0x00, 0x25, 0x00, 0x40,                 /* 0x40002500 = UART ENABLE */
+    0x08, 0x20, 0x00, 0x40,                 /* 0x40002008 = UART STARTTX */
+    0x1c, 0x25, 0x00, 0x40                  /* 0x4000251c = UART TXD */
+};
+
 typedef struct testdef {
     const char *arch;       /* Target architecture */
     const char *machine;    /* Name of the machine */
@@ -107,6 +125,7 @@ static testdef_t tests[] = {
     { "hppa", "hppa", "", "SeaBIOS wants SYSTEM HALT" },
     { "aarch64", "virt", "-cpu cortex-a57", "TT", sizeof(kernel_aarch64),
       kernel_aarch64 },
+    { "arm", "microbit", "", "T", sizeof(kernel_nrf51), kernel_nrf51 },
 
     { NULL }
 };
-- 
2.17.1

[Qemu-devel] [PATCH v2 4/4] tests/microbit-test: Check nRF51 UART functionality

[Julia Suvorova]

Some functional tests for:
    Basic reception/transmittion
    Suspending
    INTEN* registers

Based-on: <20180806100114.21410-6-contrib@steffen-goertz.de>

Signed-off-by: Julia Suvorova <jusual@mail.ru>
---
 tests/microbit-test.c | 106 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 103 insertions(+), 3 deletions(-)

diff --git a/tests/microbit-test.c b/tests/microbit-test.c
index 08e2210916..8b69d83684 100644
--- a/tests/microbit-test.c
+++ b/tests/microbit-test.c
@@ -17,7 +17,10 @@
 #include "qemu/osdep.h"
 #include "exec/hwaddr.h"
 #include "libqtest.h"
+#include "hw/char/nrf51_uart.h"
 
+#include <sys/socket.h>
+#include <sys/un.h>
 
 #define PAGE_SIZE           1024
 #define FLASH_SIZE          (256 * PAGE_SIZE)
@@ -48,7 +51,6 @@
 #define GPIO_PULLDOWN 1
 #define GPIO_PULLUP 3
 
-
 static void fill_and_erase(hwaddr base, hwaddr size, uint32_t address_reg)
 {
     /* Fill memory */
@@ -204,19 +206,117 @@ static void test_nrf51_gpio(void)
     g_assert_false(true);
 }
 
+static bool wait_for_event(uint32_t event_addr)
+{
+    int i;
+
+    for (i = 0; i < 1000; i++) {
+        if (readl(event_addr) == 1) {
+            writel(event_addr, 0x00);
+            return true;
+        }
+        g_usleep(10000);
+    }
+
+    return false;
+}
+
+static void rw_to_rxd(int sock_fd, const char *in, char *out)
+{
+    int i;
+
+    g_assert(write(sock_fd, in, strlen(in)) == strlen(in));
+    for (i = 0; i < strlen(in); i++) {
+        g_assert(wait_for_event(UART_BASE + A_UART_RXDRDY));
+        out[i] = readl(UART_BASE + A_UART_RXD);
+    }
+    out[i] = '\0';
+}
+
+static void w_to_txd(const char *in)
+{
+    int i;
+
+    for (i = 0; i < strlen(in); i++) {
+        writel(UART_BASE + A_UART_TXD, in[i]);
+        g_assert(wait_for_event(UART_BASE + A_UART_TXDRDY));
+    }
+}
+
+static void test_nrf51_uart(const void *data)
+{
+    int sock_fd = *((const int *) data);
+    char s[10];
+
+    g_assert(write(sock_fd, "c", 1) == 1);
+    g_assert(readl(UART_BASE + A_UART_RXD) == 0);
+
+    writel(UART_BASE + A_UART_ENABLE, 0x04);
+    writel(UART_BASE + A_UART_STARTRX, 0x01);
+
+    g_assert(wait_for_event(UART_BASE + A_UART_RXDRDY));
+    writel(UART_BASE + A_UART_RXDRDY, 0x00);
+    g_assert(readl(UART_BASE + A_UART_RXD) == 'c');
+
+    writel(UART_BASE + A_UART_INTENSET, 0x04);
+    g_assert(readl(UART_BASE + A_UART_INTEN) == 0x04);
+    writel(UART_BASE + A_UART_INTENCLR, 0x04);
+    g_assert(readl(UART_BASE + A_UART_INTEN) == 0x00);
+
+    rw_to_rxd(sock_fd, "hello", s);
+    g_assert(strcmp(s, "hello") == 0);
+
+    writel(UART_BASE + A_UART_STARTTX, 0x01);
+    w_to_txd("d");
+    g_assert(read(sock_fd, s, 10) == 1);
+    g_assert(s[0] == 'd');
+
+    writel(UART_BASE + A_UART_SUSPEND, 0x01);
+    writel(UART_BASE + A_UART_TXD, 'h');
+    writel(UART_BASE + A_UART_STARTTX, 0x01);
+    w_to_txd("world");
+    g_assert(read(sock_fd, s, 10) == 5);
+    g_assert(strcmp(s, "world") == 0);
+}
+
 int main(int argc, char **argv)
 {
     int ret;
+    char serialtmpdir[] = "/tmp/qtest-microbit-serial-sXXXXXX";
+    char serialtmp[40];
+    int sock_fd;
+    struct sockaddr_un addr;
+
+    g_assert(mkdtemp(serialtmpdir));
+    sprintf(serialtmp, "%s/sock", serialtmpdir);
+
+    sock_fd = socket(AF_UNIX, SOCK_STREAM, 0);
+    g_assert(sock_fd != -1);
+
+    memset(&addr, 0, sizeof(struct sockaddr_un));
+
+    addr.sun_family = AF_UNIX;
+    strncpy(addr.sun_path, serialtmp, sizeof(addr.sun_path) - 1);
 
     g_test_init(&argc, &argv, NULL);
 
-    global_qtest = qtest_startf("-machine microbit");
+    global_qtest = qtest_startf("-machine microbit "
+                                "-chardev socket,id=s0,path=%s,server,nowait "
+                                "-no-shutdown -serial chardev:s0",
+                                serialtmp);
+
+    g_assert(connect(sock_fd, (const struct sockaddr *) &addr,
+                     sizeof(struct sockaddr_un)) != -1);
 
     qtest_add_func("/microbit/nrf51/nvmc", test_nrf51_nvmc);
     qtest_add_func("/microbit/nrf51/gpio", test_nrf51_gpio);
-
+    qtest_add_data_func("/microbit/nrf51/uart", &sock_fd, test_nrf51_uart);
     ret = g_test_run();
 
     qtest_quit(global_qtest);
+
+    close(sock_fd);
+    rmdir(serialtmpdir);
+
     return ret;
 }
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 3/4] tests/boot-serial-test: Add microbit board testcase

[Thomas Huth]

On 08/08/2018 11:07 PM, Julia Suvorova via Qemu-devel wrote:
> New mini-kernel test for nRF51 SoC UART.
> 
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
>  tests/boot-serial-test.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
> index 952a2e7ead..19714c3f87 100644
> --- a/tests/boot-serial-test.c
> +++ b/tests/boot-serial-test.c
> @@ -62,6 +62,24 @@ static const uint8_t kernel_aarch64[] = {
>      0xfd, 0xff, 0xff, 0x17,                 /* b       -12 (loop) */
>  };
>  
> +static const uint8_t kernel_nrf51[] = {
> +    0x00, 0x00, 0x00, 0x00,                 /* Stack top address */
> +    0x09, 0x00, 0x00, 0x00,                 /* Reset handler address */
> +    0x04, 0x4a,                             /* ldr  r2, [pc, #16] Get ENABLE */
> +    0x04, 0x21,                             /* movs r1, #4 */
> +    0x11, 0x60,                             /* str  r1, [r2] */
> +    0x04, 0x4a,                             /* ldr  r2, [pc, #16] Get STARTTX */
> +    0x01, 0x21,                             /* movs r1, #1 */
> +    0x11, 0x60,                             /* str  r1, [r2] */
> +    0x03, 0x4a,                             /* ldr  r2, [pc, #12] Get TXD */
> +    0x54, 0x21,                             /* movs r1, 'T' */
> +    0x11, 0x60,                             /* str  r1, [r2] */
> +    0xfe, 0xe7,                             /* b    . */
> +    0x00, 0x25, 0x00, 0x40,                 /* 0x40002500 = UART ENABLE */
> +    0x08, 0x20, 0x00, 0x40,                 /* 0x40002008 = UART STARTTX */
> +    0x1c, 0x25, 0x00, 0x40                  /* 0x4000251c = UART TXD */
> +};
> +
>  typedef struct testdef {
>      const char *arch;       /* Target architecture */
>      const char *machine;    /* Name of the machine */
> @@ -107,6 +125,7 @@ static testdef_t tests[] = {
>      { "hppa", "hppa", "", "SeaBIOS wants SYSTEM HALT" },
>      { "aarch64", "virt", "-cpu cortex-a57", "TT", sizeof(kernel_aarch64),
>        kernel_aarch64 },
> +    { "arm", "microbit", "", "T", sizeof(kernel_nrf51), kernel_nrf51 },
>  
>      { NULL }
>  };
> 

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 3/4] tests/boot-serial-test: Add microbit board testcase

[Stefan Hajnoczi]

On Wed, Aug 8, 2018 at 10:07 PM, Julia Suvorova <jusual@mail.ru> wrote:
> New mini-kernel test for nRF51 SoC UART.
>
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
>  tests/boot-serial-test.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH v2 1/4] hw/char: Implement nRF51 SoC UART

[Stefan Hajnoczi]

On Wed, Aug 8, 2018 at 10:07 PM, Julia Suvorova <jusual@mail.ru> wrote:
> +static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    NRF51UARTState *s = NRF51_UART(opaque);
> +    uint64_t r;
> +
> +    if (!s->enabled) {
> +        return 0;
> +    }
> +
> +    switch (addr) {
> +    case A_UART_RXD:
> +        r = s->rx_fifo[s->rx_fifo_pos];
> +        if (s->rx_started && s->rx_fifo_len) {
> +            qemu_chr_fe_accept_input(&s->chr);

Should this be called after popping a byte from the rx fifo?  That way
.can_receive() will return true again.

> +static void nrf51_uart_realize(DeviceState *dev, Error **errp)
> +{
> +    NRF51UARTState *s = NRF51_UART(dev);
> +
> +    qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
> +                             uart_event, NULL, s, NULL, true);
> +}

unrealize() should set the handlers to NULL.  That way the device can
be removed without leaving callbacks registered.

Re: [Qemu-devel] [PATCH v2 2/4] hw/arm/nrf51_soc: Connect UART to nRF51 SoC

[Stefan Hajnoczi]

On Wed, Aug 8, 2018 at 10:07 PM, Julia Suvorova <jusual@mail.ru> wrote:
> @@ -70,6 +73,19 @@ 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;

Is there cleanup missing (e.g. unrealizing sub-devices or freeing resources)?

Re: [Qemu-devel] [PATCH v2 4/4] tests/microbit-test: Check nRF51 UART functionality

[Stefan Hajnoczi]

On Wed, Aug 8, 2018 at 10:07 PM, Julia Suvorova <jusual@mail.ru> wrote:
>      ret = g_test_run();
>
>      qtest_quit(global_qtest);
> +
> +    close(sock_fd);
> +    rmdir(serialtmpdir);

This temporary directory is leaked if the test fails.  Please remove
it immediately after connect().

Re: [Qemu-devel] [PATCH v2 1/4] hw/char: Implement nRF51 SoC UART

[Julia Suvorova]

On 10.08.2018 09:02, Stefan Hajnoczi wrote:
> On Wed, Aug 8, 2018 at 10:07 PM, Julia Suvorova <jusual@mail.ru> wrote:
>> +static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
>> +{
>> +    NRF51UARTState *s = NRF51_UART(opaque);
>> +    uint64_t r;
>> +
>> +    if (!s->enabled) {
>> +        return 0;
>> +    }
>> +
>> +    switch (addr) {
>> +    case A_UART_RXD:
>> +        r = s->rx_fifo[s->rx_fifo_pos];
>> +        if (s->rx_started && s->rx_fifo_len) {
>> +            qemu_chr_fe_accept_input(&s->chr);
> 
> Should this be called after popping a byte from the rx fifo?  That way
> .can_receive() will return true again.

Could you explain more, please?

>> +static void nrf51_uart_realize(DeviceState *dev, Error **errp)
>> +{
>> +    NRF51UARTState *s = NRF51_UART(dev);
>> +
>> +    qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
>> +                             uart_event, NULL, s, NULL, true);
>> +}
> 
> unrealize() should set the handlers to NULL.  That way the device can
> be removed without leaving callbacks registered.

I don't know the reason, but almost all char devices do not implement
this function. Maybe, because when you quit qemu, qemu_chr_cleanup() is called.

Best regards, Julia Suvorova.

Re: [Qemu-devel] [PATCH v2 1/4] hw/char: Implement nRF51 SoC UART

[Stefan Hajnoczi]

On Mon, Aug 13, 2018 at 10:08 AM Julia Suvorova <jusual@mail.ru> wrote:
> On 10.08.2018 09:02, Stefan Hajnoczi wrote:
> > On Wed, Aug 8, 2018 at 10:07 PM, Julia Suvorova <jusual@mail.ru> wrote:
> >> +static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
> >> +{
> >> +    NRF51UARTState *s = NRF51_UART(opaque);
> >> +    uint64_t r;
> >> +
> >> +    if (!s->enabled) {
> >> +        return 0;
> >> +    }
> >> +
> >> +    switch (addr) {
> >> +    case A_UART_RXD:
> >> +        r = s->rx_fifo[s->rx_fifo_pos];
> >> +        if (s->rx_started && s->rx_fifo_len) {
> >> +            qemu_chr_fe_accept_input(&s->chr);
> >
> > Should this be called after popping a byte from the rx fifo?  That way
> > .can_receive() will return true again.
>
> Could you explain more, please?

This calls into the chardev's ->chr_accept_input() function.  That
function may do anything it wants.

At this point we haven't popped a byte from our rx fifo yet, so if
->chr_accept_input() calls back into the chardev frontend (us!) it
sees that we cannot receive.  That's strange since we just told the
backend we want to accept input!

I haven't checked if there is any code path where this can happen, but
it's safer to first update internal state before letting the outside
world know that we can accept more input.

> >> +static void nrf51_uart_realize(DeviceState *dev, Error **errp)
> >> +{
> >> +    NRF51UARTState *s = NRF51_UART(dev);
> >> +
> >> +    qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
> >> +                             uart_event, NULL, s, NULL, true);
> >> +}
> >
> > unrealize() should set the handlers to NULL.  That way the device can
> > be removed without leaving callbacks registered.
>
> I don't know the reason, but almost all char devices do not implement
> this function. Maybe, because when you quit qemu, qemu_chr_cleanup() is called.

It's an assumption that on-board devices cannot be hot unplugged and
that the machine type stays alive until QEMU terminates.

Making this assumption saves 1 call to qemu_chr_fe_set_handlers().
The cost is that we cannot safely stop the system-on-chip because its
devices don't clean up properly.

Since cleanup is so trivial here I think it's worthwhile.

Stefan

Re: [Qemu-devel] [PATCH v2 1/4] hw/char: Implement nRF51 SoC UART

[Julia Suvorova]

On 13.08.2018 12:47, Stefan Hajnoczi wrote:
> On Mon, Aug 13, 2018 at 10:08 AM Julia Suvorova <jusual@mail.ru> wrote:
>> On 10.08.2018 09:02, Stefan Hajnoczi wrote:
>>> On Wed, Aug 8, 2018 at 10:07 PM, Julia Suvorova <jusual@mail.ru> wrote:
>>>> +static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
>>>> +{
>>>> +    NRF51UARTState *s = NRF51_UART(opaque);
>>>> +    uint64_t r;
>>>> +
>>>> +    if (!s->enabled) {
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    switch (addr) {
>>>> +    case A_UART_RXD:
>>>> +        r = s->rx_fifo[s->rx_fifo_pos];
>>>> +        if (s->rx_started && s->rx_fifo_len) {
>>>> +            qemu_chr_fe_accept_input(&s->chr);
>>>
>>> Should this be called after popping a byte from the rx fifo?  That way
>>> .can_receive() will return true again.
>>
>> Could you explain more, please?
> 
> This calls into the chardev's ->chr_accept_input() function.  That
> function may do anything it wants.
> 
> At this point we haven't popped a byte from our rx fifo yet, so if
> ->chr_accept_input() calls back into the chardev frontend (us!) it
> sees that we cannot receive.  That's strange since we just told the
> backend we want to accept input!
> 
> I haven't checked if there is any code path where this can happen, but
> it's safer to first update internal state before letting the outside
> world know that we can accept more input.

Thanks, I got it. I'll change the order.

>>>> +static void nrf51_uart_realize(DeviceState *dev, Error **errp)
>>>> +{
>>>> +    NRF51UARTState *s = NRF51_UART(dev);
>>>> +
>>>> +    qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
>>>> +                             uart_event, NULL, s, NULL, true);
>>>> +}
>>>
>>> unrealize() should set the handlers to NULL.  That way the device can
>>> be removed without leaving callbacks registered.
>>
>> I don't know the reason, but almost all char devices do not implement
>> this function. Maybe, because when you quit qemu, qemu_chr_cleanup() is called.
> 
> It's an assumption that on-board devices cannot be hot unplugged and
> that the machine type stays alive until QEMU terminates.
> 
> Making this assumption saves 1 call to qemu_chr_fe_set_handlers().
> The cost is that we cannot safely stop the system-on-chip because its
> devices don't clean up properly.
> 
> Since cleanup is so trivial here I think it's worthwhile.

Ok, I'll implement unrealize with a qemu_chr_fe_deinit() in it.

Best regards, Julia Suvorova.

Re: [Qemu-devel] [PATCH v2 1/4] hw/char: Implement nRF51 SoC UART

[Peter Maydell]

On 13 August 2018 at 10:47, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> It's an assumption that on-board devices cannot be hot unplugged and
> that the machine type stays alive until QEMU terminates.
>
> Making this assumption saves 1 call to qemu_chr_fe_set_handlers().
> The cost is that we cannot safely stop the system-on-chip because its
> devices don't clean up properly.
>
> Since cleanup is so trivial here I think it's worthwhile.

I would be more in favour of adding an unrealize function to
random always-present-never-unpluggable devices if we had
better documentation of exactly what the QOM lifecycle is
and what needs to be done in init/realize/unrealize/etc.
As it is, I don't know myself and therefore can't review
whether devices with unrealize methods get it right. And
obviously the code is completely untestable because it can
never run...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/4] hw/char: Implement nRF51 SoC UART

[Paolo Bonzini]

On 14/08/2018 12:02, Peter Maydell wrote:
> On 13 August 2018 at 10:47, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> It's an assumption that on-board devices cannot be hot unplugged and
>> that the machine type stays alive until QEMU terminates.
>>
>> Making this assumption saves 1 call to qemu_chr_fe_set_handlers().
>> The cost is that we cannot safely stop the system-on-chip because its
>> devices don't clean up properly.
>>
>> Since cleanup is so trivial here I think it's worthwhile.
> I would be more in favour of adding an unrealize function to
> random always-present-never-unpluggable devices if we had
> better documentation of exactly what the QOM lifecycle is
> and what needs to be done in init/realize/unrealize/etc.
> As it is, I don't know myself and therefore can't review
> whether devices with unrealize methods get it right. And
> obviously the code is completely untestable because it can
> never run...

I agree.  Don't bother with unrealize unless the device hot-unpluggable.
 What matters is that init/finalize are done right but that is tested by
device-introspection-test.

Paolo

Re: [Qemu-devel] [PATCH v2 2/4] hw/arm/nrf51_soc: Connect UART to nRF51 SoC

[Peter Maydell]

On 8 August 2018 at 22:07, Julia Suvorova <jusual@mail.ru> wrote:
> Wire up nRF51 UART in the corresponding SoC using in-place init/realize.
>
> Based-on: <20180803052137.10602-1-joel@jms.id.au>
>
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
>  hw/arm/nrf51_soc.c         | 20 ++++++++++++++++++++
>  include/hw/arm/nrf51_soc.h |  3 +++
>  2 files changed, 23 insertions(+)
>
> diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
> index 9f9649c780..8b5602f363 100644
> --- a/hw/arm/nrf51_soc.c
> +++ b/hw/arm/nrf51_soc.c
> @@ -38,9 +38,12 @@
>  #define NRF51822_FLASH_SIZE     (256 * 1024)
>  #define NRF51822_SRAM_SIZE      (16 * 1024)
>
> +#define BASE_TO_IRQ(base) ((base >> 12) & 0x1F)
> +
>  static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>  {
>      NRF51State *s = NRF51_SOC(dev_soc);
> +    MemoryRegion *mr = NULL;

This variable is unconditionally assigned before use later,
so you don't need to initialize it here. (Some compilers and
analysis tools will complain that the value assigned here is
never used.)

>      Error *err = NULL;
>
>      if (!s->board_memory) {
> @@ -70,6 +73,19 @@ 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));

serial_hd() ideally is something that should only be used by board
code. It's better to have the SoC object define a chardev property
which the board then connects up to the serial_hd() chardev.

You can do this by calling
   object_property_add_alias(obj, "serial0", OBJECT(&s->uart), "chardev",
                             &error_abort);
in the SoC's init function. That will create a property "serial0"
on the SoC object that just passes through to the "chardev" property
on the UART. Then in the board code you can set the SoC serial0
property to 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);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart), 0,
> +                       qdev_get_gpio_in(DEVICE(&s->cpu),
> +                                        BASE_TO_IRQ(UART_BASE)));
> +
>      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);
> @@ -86,6 +102,10 @@ static void nrf51_soc_init(Object *obj)
>      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_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());

Instead of these three lines, use sysbus_init_child_obj() (which
probably didn't exist when you wrote this code). This avoids a
leak of a refcount on the object.

>  }
>
>  static Property nrf51_soc_properties[] = {
> diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
> index e380ec26b8..46a1c1a66c 100644
> --- a/include/hw/arm/nrf51_soc.h
> +++ b/include/hw/arm/nrf51_soc.h
> @@ -13,6 +13,7 @@
>  #include "qemu/osdep.h"
>  #include "hw/sysbus.h"
>  #include "hw/arm/armv7m.h"
> +#include "hw/char/nrf51_uart.h"
>
>  #define TYPE_NRF51_SOC "nrf51-soc"
>  #define NRF51_SOC(obj) \
> @@ -25,6 +26,8 @@ typedef struct NRF51State {
>      /*< public >*/
>      ARMv7MState cpu;
>
> +    NRF51UARTState uart;
> +
>      MemoryRegion iomem;
>      MemoryRegion sram;
>      MemoryRegion flash;
> --
> 2.17.1

thanks
-- PMM