[Qemu-devel] [RFC 0/3] nRF51 SoC: Add UART support

[Qemu-devel] [RFC 0/3] nRF51 SoC: Add UART support

[Julia Suvorova]

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

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

Julia Suvorova (3):
  hw/arm/nrf51_soc: Fix compilation and memory regions
  hw/char/nrf51_uart: Implement nRF51 SoC UART
  tests/boot-serial-test: Add support for the microbit board

 hw/arm/nrf51_soc.c           |  19 ++-
 hw/char/Makefile.objs        |   1 +
 hw/char/nrf51_uart.c         | 232 +++++++++++++++++++++++++++++++++++
 include/hw/arm/nrf51_soc.h   |   2 +
 include/hw/char/nrf51_uart.h |  54 ++++++++
 tests/boot-serial-test.c     |  11 ++
 6 files changed, 314 insertions(+), 5 deletions(-)
 create mode 100644 hw/char/nrf51_uart.c
 create mode 100644 include/hw/char/nrf51_uart.h

-- 
2.17.0

[Qemu-devel] [RFC 1/3] hw/arm/nrf51_soc: Fix compilation and memory regions

[Julia Suvorova]

nRF51 SoC implementation is intended for the BBC Micro:bit board,
which has 256 KB flash and 16 KB RAM.
Added FICR defines.

Signed-off-by: Julia Suvorova <jusual@mail.ru>
---
 hw/arm/nrf51_soc.c         | 12 +++++++-----
 include/hw/arm/nrf51_soc.h |  1 +
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index e59ba7079f..6fe06dcfd2 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -26,15 +26,17 @@
 #define IOMEM_SIZE      0x20000000
 
 #define FLASH_BASE      0x00000000
-#define FLASH_SIZE      (144 * 1024)
+#define FLASH_SIZE      (256 * 1024)
+
+#define FICR_BASE       0x10000000
+#define FICR_SIZE       0x100
 
 #define SRAM_BASE       0x20000000
-#define SRAM_SIZE       (6 * 1024)
+#define SRAM_SIZE       (16 * 1024)
 
 static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
 {
     NRF51State *s = NRF51_SOC(dev_soc);
-    DeviceState *nvic;
     Error *err = NULL;
 
     /* IO space */
@@ -69,8 +71,8 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
     memory_region_add_subregion(system_memory, SRAM_BASE, sram);
 
     /* TODO: implement a cortex m0 and update this */
-    nvic = armv7m_init(get_system_memory(), FLASH_SIZE, 96,
-            s->kernel_filename, ARM_CPU_TYPE_NAME("cortex-m3"));
+    s->nvic = armv7m_init(get_system_memory(), FLASH_SIZE, 96,
+               s->kernel_filename, ARM_CPU_TYPE_NAME("cortex-m3"));
 }
 
 static Property nrf51_soc_properties[] = {
diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
index 5431d200f8..a6bbe9f108 100644
--- a/include/hw/arm/nrf51_soc.h
+++ b/include/hw/arm/nrf51_soc.h
@@ -23,6 +23,7 @@ typedef struct NRF51State {
 
     /*< public >*/
     char *kernel_filename;
+    DeviceState *nvic;
 
     MemoryRegion iomem;
 } NRF51State;
-- 
2.17.0

[Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART

[Julia Suvorova]

Basic implementation of nRF51 SoC UART.
Description could be found here:
http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.1.pdf

The following features are not yet implemented:
    Control with SUSPEND/START*/STOP*
    CTS/NCTS flow control
    Mapping registers to pins

Signed-off-by: Julia Suvorova <jusual@mail.ru>
---
 hw/arm/nrf51_soc.c           |   7 ++
 hw/char/Makefile.objs        |   1 +
 hw/char/nrf51_uart.c         | 232 +++++++++++++++++++++++++++++++++++
 include/hw/arm/nrf51_soc.h   |   1 +
 include/hw/char/nrf51_uart.h |  54 ++++++++
 5 files changed, 295 insertions(+)
 create mode 100644 hw/char/nrf51_uart.c
 create mode 100644 include/hw/char/nrf51_uart.h

diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 6fe06dcfd2..a2ee6f3f3b 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -21,6 +21,7 @@
 #include "cpu.h"
 
 #include "hw/arm/nrf51_soc.h"
+#include "hw/char/nrf51_uart.h"
 
 #define IOMEM_BASE      0x40000000
 #define IOMEM_SIZE      0x20000000
@@ -34,6 +35,9 @@
 #define SRAM_BASE       0x20000000
 #define SRAM_SIZE       (16 * 1024)
 
+#define UART_BASE       0x40002000
+#define UART_SIZE       0x1000
+
 static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
 {
     NRF51State *s = NRF51_SOC(dev_soc);
@@ -73,6 +77,9 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
     /* TODO: implement a cortex m0 and update this */
     s->nvic = armv7m_init(get_system_memory(), FLASH_SIZE, 96,
                s->kernel_filename, ARM_CPU_TYPE_NAME("cortex-m3"));
+
+    s->uart = nrf51_uart_create(UART_BASE, qdev_get_gpio_in(s->nvic, 2),
+                                serial_hd(0));
 }
 
 static Property nrf51_soc_properties[] = {
diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
index 1b979100b7..1060c62a54 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..2da97aa0c4
--- /dev/null
+++ b/hw/char/nrf51_uart.c
@@ -0,0 +1,232 @@
+/*
+ * 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.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/registerfields.h"
+#include "hw/char/nrf51_uart.h"
+
+REG32(STARTRX, 0x000)
+REG32(STOPRX, 0x004)
+REG32(STARTTX, 0x008)
+REG32(STOPTX, 0x00C)
+REG32(SUSPEND, 0x01C)
+
+REG32(CTS, 0x100)
+REG32(NCTS, 0x104)
+REG32(RXDRDY, 0x108)
+REG32(TXDRDY, 0x11C)
+REG32(ERROR, 0x124)
+REG32(RXTO, 0x144)
+
+REG32(INTEN, 0x300)
+    FIELD(INTEN, CTS, 0, 1)
+    FIELD(INTEN, NCTS, 1, 1)
+    FIELD(INTEN, RXDRDY, 2, 1)
+    FIELD(INTEN, TXDRDY, 7, 1)
+    FIELD(INTEN, ERROR, 9, 1)
+    FIELD(INTEN, RXTO, 17, 1)
+REG32(INTENSET, 0x304)
+REG32(INTENCLR, 0x308)
+REG32(ERRORSRC, 0x480)
+REG32(ENABLE, 0x500)
+REG32(PSELRTS, 0x508)
+REG32(PSELTXD, 0x50C)
+REG32(PSELCTS, 0x510)
+REG32(PSELRXD, 0x514)
+REG32(RXD, 0x518)
+REG32(TXD, 0x51C)
+REG32(BAUDRATE, 0x524)
+REG32(CONFIG, 0x56C)
+
+static void nrf51_uart_update_irq(Nrf51UART *s)
+{
+    unsigned int irq = 0;
+
+    irq = irq || (s->reg[A_RXDRDY] && (s->reg[A_INTEN] & R_INTEN_RXDRDY_MASK));
+    irq = irq || (s->reg[A_TXDRDY] && (s->reg[A_INTEN] & R_INTEN_TXDRDY_MASK));
+    irq = irq || (s->reg[A_ERROR]  && (s->reg[A_INTEN] & R_INTEN_ERROR_MASK));
+    irq = irq || (s->reg[A_RXTO]   && (s->reg[A_INTEN] & R_INTEN_RXTO_MASK));
+
+    qemu_set_irq(s->irq, !!irq);
+}
+
+static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    Nrf51UART *s = NRF51_UART(opaque);
+    uint64_t r;
+
+    switch (addr) {
+    case A_RXD:
+        r = s->rx_fifo[s->rx_fifo_pos];
+        if (s->rx_fifo_len > 0) {
+            s->rx_fifo_pos = (s->rx_fifo_pos + 1) % UART_FIFO_LENGTH;
+            s->rx_fifo_len--;
+            qemu_chr_fe_accept_input(&s->chr);
+        }
+        break;
+
+    case A_INTENSET:
+    case A_INTENCLR:
+    case A_INTEN:
+        r = s->reg[A_INTEN];
+        break;
+    default:
+        r = s->reg[addr];
+        break;
+    }
+
+    return r;
+}
+
+static gboolean uart_transmit(GIOChannel *chan, GIOCondition cond, void *opaque)
+{
+    Nrf51UART *s = NRF51_UART(opaque);
+    int r;
+
+    s->watch_tag = 0;
+
+    r = qemu_chr_fe_write(&s->chr, (uint8_t *) &s->reg[A_TXD], 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) {
+            goto buffer_drained;
+        }
+        return FALSE;
+    }
+
+buffer_drained:
+    s->reg[A_TXDRDY] = 1;
+    nrf51_uart_update_irq(s);
+    return FALSE;
+}
+
+static void uart_cancel_transmit(Nrf51UART *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)
+{
+    Nrf51UART *s = NRF51_UART(opaque);
+
+    switch (addr) {
+    case A_TXD:
+        s->reg[A_TXD] = value;
+        uart_transmit(NULL, G_IO_OUT, s);
+        break;
+    case A_INTENSET:
+        s->reg[A_INTEN] |= value;
+        break;
+    case A_INTENCLR:
+        s->reg[A_INTEN] &= ~value;
+        break;
+    case A_CTS ... A_RXTO:
+        s->reg[addr] = value;
+        nrf51_uart_update_irq(s);
+    default:
+        s->reg[addr] = value;
+        break;
+    }
+}
+
+static const MemoryRegionOps uart_ops = {
+    .read =  uart_read,
+    .write = uart_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void nrf51_uart_reset(DeviceState *dev)
+{
+    Nrf51UART *s = NRF51_UART(dev);
+
+    uart_cancel_transmit(s);
+
+    memset(s->reg, 0, sizeof(s->reg));
+
+    s->rx_fifo_len = 0;
+    s->rx_fifo_pos = 0;
+}
+
+static void uart_receive(void *opaque, const uint8_t *buf, int size)
+{
+
+   Nrf51UART *s = NRF51_UART(opaque);
+
+   if (s->rx_fifo_len >= UART_FIFO_LENGTH) {
+        return;
+    }
+
+    s->rx_fifo[(s->rx_fifo_pos + s->rx_fifo_len) % UART_FIFO_LENGTH] = *buf;
+    s->rx_fifo_len++;
+
+    s->reg[A_RXDRDY] = 1;
+    nrf51_uart_update_irq(s);
+}
+
+static int uart_can_receive(void *opaque)
+{
+    Nrf51UART *s = NRF51_UART(opaque);
+
+    return (s->rx_fifo_len < sizeof(s->rx_fifo));
+}
+
+static void nrf51_uart_realize(DeviceState *dev, Error **errp)
+{
+    Nrf51UART *s = NRF51_UART(dev);
+
+    qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
+                             NULL, NULL, s, NULL, true);
+}
+
+static void nrf51_uart_init(Object *obj)
+{
+    Nrf51UART *s = NRF51_UART(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+
+    memory_region_init_io(&s->mmio, obj, &uart_ops, s,
+                          "nrf51_soc.uart", 0x1000);
+    sysbus_init_mmio(sbd, &s->mmio);
+    sysbus_init_irq(sbd, &s->irq);
+}
+
+static Property nrf51_uart_properties[] = {
+    DEFINE_PROP_CHR("chardev", Nrf51UART, 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;
+}
+
+static const TypeInfo nrf51_uart_info = {
+    .name = TYPE_NRF51_UART,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(Nrf51UART),
+    .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/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
index a6bbe9f108..a38b984675 100644
--- a/include/hw/arm/nrf51_soc.h
+++ b/include/hw/arm/nrf51_soc.h
@@ -24,6 +24,7 @@ typedef struct NRF51State {
     /*< public >*/
     char *kernel_filename;
     DeviceState *nvic;
+    DeviceState *uart;
 
     MemoryRegion iomem;
 } NRF51State;
diff --git a/include/hw/char/nrf51_uart.h b/include/hw/char/nrf51_uart.h
new file mode 100644
index 0000000000..758203f1c3
--- /dev/null
+++ b/include/hw/char/nrf51_uart.h
@@ -0,0 +1,54 @@
+/*
+ * 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"
+
+#define UART_FIFO_LENGTH 6
+
+#define TYPE_NRF51_UART "nrf51_soc.uart"
+#define NRF51_UART(obj) OBJECT_CHECK(Nrf51UART, (obj), TYPE_NRF51_UART)
+
+typedef struct Nrf51UART {
+    SysBusDevice parent_obj;
+
+    MemoryRegion mmio;
+    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[0x1000];
+} Nrf51UART;
+
+static inline DeviceState *nrf51_uart_create(hwaddr addr,
+                                             qemu_irq irq,
+                                             Chardev *chr)
+{
+    DeviceState *dev;
+    SysBusDevice *s;
+
+    dev = qdev_create(NULL, "nrf51_soc.uart");
+    s = SYS_BUS_DEVICE(dev);
+    qdev_prop_set_chr(dev, "chardev", chr);
+    qdev_init_nofail(dev);
+    sysbus_mmio_map(s, 0, addr);
+    sysbus_connect_irq(s, 0, irq);
+
+    return dev;
+}
+
+#endif
-- 
2.17.0

[Qemu-devel] [RFC 3/3] tests/boot-serial-test: Add support for the microbit board

[Julia Suvorova]

New mini-kernel test for nRF51 SoC UART.

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

diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
index 4d6815c3e0..e6dbc8a293 100644
--- a/tests/boot-serial-test.c
+++ b/tests/boot-serial-test.c
@@ -62,6 +62,16 @@ 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 */
+    0x01, 0x4b,                             /* ldr   r3,[pc,#4]    Get base */
+    0x54, 0x22,                             /* mov   r2,#'T' */
+    0x1a, 0x70,                             /* strb  r2,[r3] */
+    0x01, 0xe0,                             /* b     loop */
+    0x1c, 0x25, 0x00, 0x40,                 /* 0x40002000 = UART0 base addr */
+};
+
 typedef struct testdef {
     const char *arch;       /* Target architecture */
     const char *machine;    /* Name of the machine */
@@ -107,6 +117,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.0

Re: [Qemu-devel] [RFC 0/3] nRF51 SoC: Add UART support

[Philippe Mathieu-Daudé]

Hi Julia,

On 05/29/2018 07:03 PM, Julia Suvorova via Qemu-devel wrote:
> This series adds basic support for the nRF51 SoC UART, that used in
> BBC Micro:bit board, and QTest for it.
> 
> Based-on: <20180503090532.3113-1-joel@jms.id.au>
> 
> Julia Suvorova (3):
>   hw/arm/nrf51_soc: Fix compilation and memory regions

Since Joel's series isn't merged, you can simply take his patches, fix
them, and include them in your following series.  Usually you keep his
Signed-off-by line, add a comment with your changes, then add your
S-o-b, such:

    [PATCH ...] hw/arm: Add Nordic Semiconductor nRF51 SoC

    The nRF51 is a Cortex-M0 microcontroller with an on-board radio module,
    plus other common ARM SoC peripherals.

    This defines a basic model of the CPU and memory, with no peripherals
    implemented at this stage.

    Signed-off-by: Joel Stanley <joel@jms.id.au>
    [Julia: Fixed BBC Micro:bit board ROM/RAM size, added FICR defines]
    Signed-off-by: Julia Suvorova <jusual@mail.ru>

>   hw/char/nrf51_uart: Implement nRF51 SoC UART
>   tests/boot-serial-test: Add support for the microbit board
> 
>  hw/arm/nrf51_soc.c           |  19 ++-
>  hw/char/Makefile.objs        |   1 +
>  hw/char/nrf51_uart.c         | 232 +++++++++++++++++++++++++++++++++++
>  include/hw/arm/nrf51_soc.h   |   2 +
>  include/hw/char/nrf51_uart.h |  54 ++++++++
>  tests/boot-serial-test.c     |  11 ++
>  6 files changed, 314 insertions(+), 5 deletions(-)
>  create mode 100644 hw/char/nrf51_uart.c
>  create mode 100644 include/hw/char/nrf51_uart.h
> 

Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART

[Stefan Hajnoczi]

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

On Wed, May 30, 2018 at 01:03:37AM +0300, Julia Suvorova wrote:
> The following features are not yet implemented:
>     Control with SUSPEND/START*/STOP*

This is probably worth implementing for completeness.  Just rx_enabled
and tx_enabled boolean states will be sufficient.  SUSPEND flushes tx
and stops rx, it doesn't require any extra state.

>     CTS/NCTS flow control

CTS flow control is probably not necessary since we don't have bitwise
I/O and guest applications probably don't care either.

>     Mapping registers to pins

This is probably not necessary since the micro:bit only uses the UART in
one pin configuration (back to the USB interface chip).

Please make sure that every register is explicitly handle in the code
and falls into these categories:

1. Implemented.
2. Unimplemented - can be observed via a trace event.  This will make
   debugging easier in the future when an application doesn't work
   with the UART.
3. Stubbed out - explicitly marked as ignored in the code.

This way we can be confident about the completeness of this device
model.

> diff --git a/hw/char/nrf51_uart.c b/hw/char/nrf51_uart.c
> new file mode 100644
> index 0000000000..2da97aa0c4
> --- /dev/null
> +++ b/hw/char/nrf51_uart.c
> @@ -0,0 +1,232 @@
> +/*
> + * nRF51 SoC UART emulation

Please add a reference to the hardware specfication here:

  See nRF51 Series Reference Manual, "29 Universal Asynchronous
  Receiver/Transmitter" for hardware specifications:
  http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf

> +static gboolean uart_transmit(GIOChannel *chan, GIOCondition cond, void *opaque)
> +{
> +    Nrf51UART *s = NRF51_UART(opaque);
> +    int r;
> +
> +    s->watch_tag = 0;
> +
> +    r = qemu_chr_fe_write(&s->chr, (uint8_t *) &s->reg[A_TXD], 1);

This does not work on big-endian hosts:

  s->reg[A_TXD] = 'A';

little-endian memory layout: 41 00 00 00
big-endian memory layout:    00 00 00 41
(uint8_t *) &s->reg[A_TXD] --^

Instead please use a uint8_t local:

  uint8_t ch = s->reg[A_TXD];

  ...

  r = qemu_chr_fe_write(&s->chr, &ch, 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) {
> +            goto buffer_drained;

Please add a comment here:

  /* The hardware has no transmit error reporting, so silently drop the byte */

> +static void uart_receive(void *opaque, const uint8_t *buf, int size)
> +{
> +
> +   Nrf51UART *s = NRF51_UART(opaque);
> +
> +   if (s->rx_fifo_len >= UART_FIFO_LENGTH) {

Indentation is off.  Please use 4 spaces.

> +        return;
> +    }
> +
> +    s->rx_fifo[(s->rx_fifo_pos + s->rx_fifo_len) % UART_FIFO_LENGTH] = *buf;
> +    s->rx_fifo_len++;
> +
> +    s->reg[A_RXDRDY] = 1;
> +    nrf51_uart_update_irq(s);
> +}
> +
> +static int uart_can_receive(void *opaque)
> +{
> +    Nrf51UART *s = NRF51_UART(opaque);
> +
> +    return (s->rx_fifo_len < sizeof(s->rx_fifo));

This is very subtle: this function returns the number of bytes that can
be read.  This expression looks like a boolean return value but actually
relies on the implicit int cast (false -> 0 bytes, true -> 1 byte).

Please write it so it doesn't look like a boolean return value:

  if (s->rx_fifo_len >= sizeof(s->rx_fifo)) {
      return 0;
  }

  return 1 /* byte */;

Alternatively, you could handle more than 1 byte:

  return sizeof(s->rx_fifo) - s->rx_fifo_len;

but now uart_receive() needs to handle up to sizeof(s->rx_fifo) bytes of
data in a single call.

> +}
> +
> +static void nrf51_uart_realize(DeviceState *dev, Error **errp)
> +{
> +    Nrf51UART *s = NRF51_UART(dev);
> +
> +    qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
> +                             NULL, NULL, s, NULL, true);
> +}
> +
> +static void nrf51_uart_init(Object *obj)
> +{
> +    Nrf51UART *s = NRF51_UART(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +
> +    memory_region_init_io(&s->mmio, obj, &uart_ops, s,
> +                          "nrf51_soc.uart", 0x1000);

s/0x1000/UART_SIZE/

> +typedef struct Nrf51UART {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion mmio;
> +    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[0x1000];

Where does 0x1000 come from?  I think the actual 0x1000-byte range would
be uint32_t reg[UART_SIZE / sizeof(reg[0])].

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

Re: [Qemu-devel] [RFC 3/3] tests/boot-serial-test: Add support for the microbit board

[Stefan Hajnoczi]

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

On Wed, May 30, 2018 at 01:03:38AM +0300, Julia Suvorova wrote:
> New mini-kernel test for nRF51 SoC UART.
> 
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
>  tests/boot-serial-test.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
> index 4d6815c3e0..e6dbc8a293 100644
> --- a/tests/boot-serial-test.c
> +++ b/tests/boot-serial-test.c
> @@ -62,6 +62,16 @@ 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 */
> +    0x01, 0x4b,                             /* ldr   r3,[pc,#4]    Get base */
> +    0x54, 0x22,                             /* mov   r2,#'T' */
> +    0x1a, 0x70,                             /* strb  r2,[r3] */
> +    0x01, 0xe0,                             /* b     loop */
> +    0x1c, 0x25, 0x00, 0x40,                 /* 0x40002000 = UART0 base addr */
> +};

Good for now, will need STARTTX in the future.

Stefan

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

Re: [Qemu-devel] [RFC 0/3] nRF51 SoC: Add UART support

[Stefan Hajnoczi]

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

On Tue, May 29, 2018 at 11:57:47PM -0300, Philippe Mathieu-Daudé wrote:
> Hi Julia,
> 
> On 05/29/2018 07:03 PM, Julia Suvorova via Qemu-devel wrote:
> > This series adds basic support for the nRF51 SoC UART, that used in
> > BBC Micro:bit board, and QTest for it.
> > 
> > Based-on: <20180503090532.3113-1-joel@jms.id.au>
> > 
> > Julia Suvorova (3):
> >   hw/arm/nrf51_soc: Fix compilation and memory regions
> 
> Since Joel's series isn't merged, you can simply take his patches, fix
> them, and include them in your following series.  Usually you keep his
> Signed-off-by line, add a comment with your changes, then add your
> S-o-b, such:
> 
>     [PATCH ...] hw/arm: Add Nordic Semiconductor nRF51 SoC
> 
>     The nRF51 is a Cortex-M0 microcontroller with an on-board radio module,
>     plus other common ARM SoC peripherals.
> 
>     This defines a basic model of the CPU and memory, with no peripherals
>     implemented at this stage.
> 
>     Signed-off-by: Joel Stanley <joel@jms.id.au>
>     [Julia: Fixed BBC Micro:bit board ROM/RAM size, added FICR defines]
>     Signed-off-by: Julia Suvorova <jusual@mail.ru>

I think Joel will send another revision of his series.  It would make
sense for him to squash in this patch.

Stefan

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

Re: [Qemu-devel] [RFC 1/3] hw/arm/nrf51_soc: Fix compilation and memory regions

[Peter Maydell]

On 29 May 2018 at 23:03, Julia Suvorova <jusual@mail.ru> wrote:
> nRF51 SoC implementation is intended for the BBC Micro:bit board,
> which has 256 KB flash and 16 KB RAM.
> Added FICR defines.
>
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
>  hw/arm/nrf51_soc.c         | 12 +++++++-----
>  include/hw/arm/nrf51_soc.h |  1 +
>  2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
> index e59ba7079f..6fe06dcfd2 100644
> --- a/hw/arm/nrf51_soc.c
> +++ b/hw/arm/nrf51_soc.c
> @@ -26,15 +26,17 @@
>  #define IOMEM_SIZE      0x20000000
>
>  #define FLASH_BASE      0x00000000
> -#define FLASH_SIZE      (144 * 1024)
> +#define FLASH_SIZE      (256 * 1024)
> +
> +#define FICR_BASE       0x10000000
> +#define FICR_SIZE       0x100
>
>  #define SRAM_BASE       0x20000000
> -#define SRAM_SIZE       (6 * 1024)
> +#define SRAM_SIZE       (16 * 1024)
>
>  static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>  {
>      NRF51State *s = NRF51_SOC(dev_soc);
> -    DeviceState *nvic;
>      Error *err = NULL;
>
>      /* IO space */
> @@ -69,8 +71,8 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>      memory_region_add_subregion(system_memory, SRAM_BASE, sram);
>
>      /* TODO: implement a cortex m0 and update this */
> -    nvic = armv7m_init(get_system_memory(), FLASH_SIZE, 96,
> -            s->kernel_filename, ARM_CPU_TYPE_NAME("cortex-m3"));
> +    s->nvic = armv7m_init(get_system_memory(), FLASH_SIZE, 96,
> +               s->kernel_filename, ARM_CPU_TYPE_NAME("cortex-m3"));
>  }
>
>  static Property nrf51_soc_properties[] = {
> diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
> index 5431d200f8..a6bbe9f108 100644
> --- a/include/hw/arm/nrf51_soc.h
> +++ b/include/hw/arm/nrf51_soc.h
> @@ -23,6 +23,7 @@ typedef struct NRF51State {
>
>      /*< public >*/
>      char *kernel_filename;
> +    DeviceState *nvic;
>
>      MemoryRegion iomem;
>  } NRF51State;

The better approach here (which I think I suggested in review
of Joel's patches) is to have the armv7m object embedded in the
NRF51State. Then you can initialize and realize it in-place,
and the board code calls armv7m_load_kernel() rather than
having to pass the kernel filename to the SoC. (Nothing then
needs to call armv7m_init() at all.)

thanks
-- PMM

Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART

[Peter Maydell]

On 29 May 2018 at 23:03, Julia Suvorova <jusual@mail.ru> wrote:
> Basic implementation of nRF51 SoC UART.
> Description could be found here:
> http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.1.pdf
>
> The following features are not yet implemented:
>     Control with SUSPEND/START*/STOP*
>     CTS/NCTS flow control
>     Mapping registers to pins
>
> Signed-off-by: Julia Suvorova <jusual@mail.ru>

Hi; thanks for this patch; the UART implementation looks generally
in good shape. I have some specific review comments below.

> ---
>  hw/arm/nrf51_soc.c           |   7 ++
>  hw/char/Makefile.objs        |   1 +
>  hw/char/nrf51_uart.c         | 232 +++++++++++++++++++++++++++++++++++
>  include/hw/arm/nrf51_soc.h   |   1 +
>  include/hw/char/nrf51_uart.h |  54 ++++++++

I recommend having "implement the UART" in one patch, and
"add the new UART to this SoC" as a separate patch.

>  5 files changed, 295 insertions(+)
>  create mode 100644 hw/char/nrf51_uart.c
>  create mode 100644 include/hw/char/nrf51_uart.h
>
> diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
> index 6fe06dcfd2..a2ee6f3f3b 100644
> --- a/hw/arm/nrf51_soc.c
> +++ b/hw/arm/nrf51_soc.c
> @@ -21,6 +21,7 @@
>  #include "cpu.h"
>
>  #include "hw/arm/nrf51_soc.h"
> +#include "hw/char/nrf51_uart.h"
>
>  #define IOMEM_BASE      0x40000000
>  #define IOMEM_SIZE      0x20000000
> @@ -34,6 +35,9 @@
>  #define SRAM_BASE       0x20000000
>  #define SRAM_SIZE       (16 * 1024)
>
> +#define UART_BASE       0x40002000
> +#define UART_SIZE       0x1000
> +
>  static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>  {
>      NRF51State *s = NRF51_SOC(dev_soc);
> @@ -73,6 +77,9 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>      /* TODO: implement a cortex m0 and update this */
>      s->nvic = armv7m_init(get_system_memory(), FLASH_SIZE, 96,
>                 s->kernel_filename, ARM_CPU_TYPE_NAME("cortex-m3"));
> +
> +    s->uart = nrf51_uart_create(UART_BASE, qdev_get_gpio_in(s->nvic, 2),
> +                                serial_hd(0));

I would recommend having the UART struct embedded in the SoC
struct and using in-place init/realize.

Devices in an SoC object should also be being mapped into a
container memory region, rather than mapping themselves
directly into system memory.

>  }
>
>  static Property nrf51_soc_properties[] = {
> diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> index 1b979100b7..1060c62a54 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..2da97aa0c4
> --- /dev/null
> +++ b/hw/char/nrf51_uart.c
> @@ -0,0 +1,232 @@
> +/*
> + * 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.
> + */

I would suggest having a comment with the data sheet URL here.

> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/registerfields.h"
> +#include "hw/char/nrf51_uart.h"
> +
> +REG32(STARTRX, 0x000)
> +REG32(STOPRX, 0x004)
> +REG32(STARTTX, 0x008)
> +REG32(STOPTX, 0x00C)
> +REG32(SUSPEND, 0x01C)
> +
> +REG32(CTS, 0x100)
> +REG32(NCTS, 0x104)
> +REG32(RXDRDY, 0x108)
> +REG32(TXDRDY, 0x11C)
> +REG32(ERROR, 0x124)
> +REG32(RXTO, 0x144)
> +
> +REG32(INTEN, 0x300)
> +    FIELD(INTEN, CTS, 0, 1)
> +    FIELD(INTEN, NCTS, 1, 1)
> +    FIELD(INTEN, RXDRDY, 2, 1)
> +    FIELD(INTEN, TXDRDY, 7, 1)
> +    FIELD(INTEN, ERROR, 9, 1)
> +    FIELD(INTEN, RXTO, 17, 1)
> +REG32(INTENSET, 0x304)
> +REG32(INTENCLR, 0x308)
> +REG32(ERRORSRC, 0x480)
> +REG32(ENABLE, 0x500)
> +REG32(PSELRTS, 0x508)
> +REG32(PSELTXD, 0x50C)
> +REG32(PSELCTS, 0x510)
> +REG32(PSELRXD, 0x514)
> +REG32(RXD, 0x518)
> +REG32(TXD, 0x51C)
> +REG32(BAUDRATE, 0x524)
> +REG32(CONFIG, 0x56C)
> +
> +static void nrf51_uart_update_irq(Nrf51UART *s)
> +{
> +    unsigned int irq = 0;

Since this is just holding a boolean, you can make it 'bool' type,
and then you don't need the !! when you pass it to qemu_set_irq().

> +
> +    irq = irq || (s->reg[A_RXDRDY] && (s->reg[A_INTEN] & R_INTEN_RXDRDY_MASK));
> +    irq = irq || (s->reg[A_TXDRDY] && (s->reg[A_INTEN] & R_INTEN_TXDRDY_MASK));
> +    irq = irq || (s->reg[A_ERROR]  && (s->reg[A_INTEN] & R_INTEN_ERROR_MASK));
> +    irq = irq || (s->reg[A_RXTO]   && (s->reg[A_INTEN] & R_INTEN_RXTO_MASK));
> +
> +    qemu_set_irq(s->irq, !!irq);
> +}
> +
> +static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    Nrf51UART *s = NRF51_UART(opaque);
> +    uint64_t r;
> +
> +    switch (addr) {
> +    case A_RXD:
> +        r = s->rx_fifo[s->rx_fifo_pos];
> +        if (s->rx_fifo_len > 0) {
> +            s->rx_fifo_pos = (s->rx_fifo_pos + 1) % UART_FIFO_LENGTH;
> +            s->rx_fifo_len--;
> +            qemu_chr_fe_accept_input(&s->chr);
> +        }
> +        break;
> +
> +    case A_INTENSET:
> +    case A_INTENCLR:
> +    case A_INTEN:
> +        r = s->reg[A_INTEN];
> +        break;
> +    default:
> +        r = s->reg[addr];
> +        break;
> +    }
> +
> +    return r;
> +}
> +
> +static gboolean uart_transmit(GIOChannel *chan, GIOCondition cond, void *opaque)
> +{
> +    Nrf51UART *s = NRF51_UART(opaque);
> +    int r;
> +
> +    s->watch_tag = 0;
> +
> +    r = qemu_chr_fe_write(&s->chr, (uint8_t *) &s->reg[A_TXD], 1);

This won't work if the host is big-endian. You need to do something like

   uint8_t c = s->reg[A_TXD];
   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) {
> +            goto buffer_drained;
> +        }
> +        return FALSE;
> +    }
> +
> +buffer_drained:
> +    s->reg[A_TXDRDY] = 1;
> +    nrf51_uart_update_irq(s);
> +    return FALSE;
> +}
> +
> +static void uart_cancel_transmit(Nrf51UART *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)
> +{
> +    Nrf51UART *s = NRF51_UART(opaque);
> +
> +    switch (addr) {
> +    case A_TXD:
> +        s->reg[A_TXD] = value;
> +        uart_transmit(NULL, G_IO_OUT, s);

If you just call uart_transmit() here and we were already
in the process of transmitting a character, we'll end up
registering a second 'watch' and will leak the old one.
Sadly there isn't any guest-visible state that tracks
"there is a byte in the TX register that has not yet been sent",
so we'll have to have state that's not guest visible. You
can do this by having a new s->pending_tx_byte bool, whose
semantics should look like the R_STATE_TXFULL_MASK bit in
the hw/char/cmsdk-apb-uart.c UART:

 * uart_transmit() does nothing if pending_tx_byte is false
 * in uart_transmit(), when we write TXDRDY to 1 we also
   set pending_tx_byte to false
 * in this "write to TXD" code, if pending_tx_byte is true
   we do nothing (because this UART doesn't tell the guest
   about tx overruns); otherwise we set pending_tx_byte
   to true and call uart_transmit()
 * in the migration state's post_load function, if
   pending_tx_byte is true then we register a watch on the
   char frontend

(In the CMSDK uart we use a bit in a state register because
that UART happens to have guest-visible state that tracks
this, so we can avoid having an extra bool.)

> +        break;
> +    case A_INTENSET:
> +        s->reg[A_INTEN] |= value;
> +        break;
> +    case A_INTENCLR:
> +        s->reg[A_INTEN] &= ~value;
> +        break;
> +    case A_CTS ... A_RXTO:
> +        s->reg[addr] = value;
> +        nrf51_uart_update_irq(s);

ERRORSRC is write-1-to-clear, so needs to be specially handled.

RXD is read-only, so also needs handling.

> +    default:
> +        s->reg[addr] = value;
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps uart_ops = {
> +    .read =  uart_read,
> +    .write = uart_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void nrf51_uart_reset(DeviceState *dev)
> +{
> +    Nrf51UART *s = NRF51_UART(dev);
> +
> +    uart_cancel_transmit(s);
> +
> +    memset(s->reg, 0, sizeof(s->reg));
> +
> +    s->rx_fifo_len = 0;
> +    s->rx_fifo_pos = 0;

The PSEL* registers are documented as having an 0xffffffff reset value.
BAUDRATE also has a non-zero reset value.

> +}
> +
> +static void uart_receive(void *opaque, const uint8_t *buf, int size)
> +{
> +
> +   Nrf51UART *s = NRF51_UART(opaque);
> +
> +   if (s->rx_fifo_len >= UART_FIFO_LENGTH) {
> +        return;
> +    }
> +
> +    s->rx_fifo[(s->rx_fifo_pos + s->rx_fifo_len) % UART_FIFO_LENGTH] = *buf;
> +    s->rx_fifo_len++;

The 'size' argument to this function is a byte count of how
many bytes are in the buffer, so you should take as many
of them as will fill the fifo, not just one.

> +
> +    s->reg[A_RXDRDY] = 1;
> +    nrf51_uart_update_irq(s);
> +}
> +
> +static int uart_can_receive(void *opaque)
> +{
> +    Nrf51UART *s = NRF51_UART(opaque);
> +
> +    return (s->rx_fifo_len < sizeof(s->rx_fifo));

Could you be consistent about whether you use sizeof(s->rx_fifo) or
UART_FIFO_LENGTH in checks on the fifo length, please?

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

This UART supports detection of the 'break' condition. You
can implement that with a uart_event function (4th argument
here), which does the right thing when it gets a CHR_EVENT_BREAK.
(The data sheet says that a break means "set the FRAMING and
BREAK bits in the ERRORSRC register and raise ERROR.")

> +}
> +
> +static void nrf51_uart_init(Object *obj)
> +{
> +    Nrf51UART *s = NRF51_UART(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +
> +    memory_region_init_io(&s->mmio, obj, &uart_ops, s,
> +                          "nrf51_soc.uart", 0x1000);
> +    sysbus_init_mmio(sbd, &s->mmio);
> +    sysbus_init_irq(sbd, &s->irq);
> +}
> +
> +static Property nrf51_uart_properties[] = {
> +    DEFINE_PROP_CHR("chardev", Nrf51UART, 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;

All new devices should have a VMState description struct
and set dc->vmsd, so that they support state save/restore.

> +}
> +
> +static const TypeInfo nrf51_uart_info = {
> +    .name = TYPE_NRF51_UART,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(Nrf51UART),
> +    .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/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
> index a6bbe9f108..a38b984675 100644
> --- a/include/hw/arm/nrf51_soc.h
> +++ b/include/hw/arm/nrf51_soc.h
> @@ -24,6 +24,7 @@ typedef struct NRF51State {
>      /*< public >*/
>      char *kernel_filename;
>      DeviceState *nvic;
> +    DeviceState *uart;
>
>      MemoryRegion iomem;
>  } NRF51State;
> diff --git a/include/hw/char/nrf51_uart.h b/include/hw/char/nrf51_uart.h
> new file mode 100644
> index 0000000000..758203f1c3
> --- /dev/null
> +++ b/include/hw/char/nrf51_uart.h
> @@ -0,0 +1,54 @@
> +/*
> + * 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"
> +
> +#define UART_FIFO_LENGTH 6
> +
> +#define TYPE_NRF51_UART "nrf51_soc.uart"
> +#define NRF51_UART(obj) OBJECT_CHECK(Nrf51UART, (obj), TYPE_NRF51_UART)
> +
> +typedef struct Nrf51UART {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion mmio;
> +    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[0x1000];
> +} Nrf51UART;
> +
> +static inline DeviceState *nrf51_uart_create(hwaddr addr,
> +                                             qemu_irq irq,
> +                                             Chardev *chr)
> +{
> +    DeviceState *dev;
> +    SysBusDevice *s;
> +
> +    dev = qdev_create(NULL, "nrf51_soc.uart");
> +    s = SYS_BUS_DEVICE(dev);
> +    qdev_prop_set_chr(dev, "chardev", chr);
> +    qdev_init_nofail(dev);
> +    sysbus_mmio_map(s, 0, addr);
> +    sysbus_connect_irq(s, 0, irq);
> +
> +    return dev;
> +}
> +
> +#endif
> --
> 2.17.0


thanks
-- PMM

Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART

[sundeep subbaraya]

Hi,

On Wed, May 30, 2018 at 3:33 AM, Julia Suvorova via Qemu-devel
<qemu-devel@nongnu.org> wrote:
> Basic implementation of nRF51 SoC UART.
> Description could be found here:
> http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.1.pdf
>
> The following features are not yet implemented:
>     Control with SUSPEND/START*/STOP*
>     CTS/NCTS flow control
>     Mapping registers to pins
>
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
>  hw/arm/nrf51_soc.c           |   7 ++
>  hw/char/Makefile.objs        |   1 +
>  hw/char/nrf51_uart.c         | 232 +++++++++++++++++++++++++++++++++++
>  include/hw/arm/nrf51_soc.h   |   1 +
>  include/hw/char/nrf51_uart.h |  54 ++++++++
>  5 files changed, 295 insertions(+)
>  create mode 100644 hw/char/nrf51_uart.c
>  create mode 100644 include/hw/char/nrf51_uart.h
>
> diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
> index 6fe06dcfd2..a2ee6f3f3b 100644
> --- a/hw/arm/nrf51_soc.c
> +++ b/hw/arm/nrf51_soc.c
> @@ -21,6 +21,7 @@
>  #include "cpu.h"
>
>  #include "hw/arm/nrf51_soc.h"
> +#include "hw/char/nrf51_uart.h"
>
>  #define IOMEM_BASE      0x40000000
>  #define IOMEM_SIZE      0x20000000
> @@ -34,6 +35,9 @@
>  #define SRAM_BASE       0x20000000
>  #define SRAM_SIZE       (16 * 1024)
>
> +#define UART_BASE       0x40002000
> +#define UART_SIZE       0x1000
> +
>  static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>  {
>      NRF51State *s = NRF51_SOC(dev_soc);
> @@ -73,6 +77,9 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>      /* TODO: implement a cortex m0 and update this */
>      s->nvic = armv7m_init(get_system_memory(), FLASH_SIZE, 96,
>                 s->kernel_filename, ARM_CPU_TYPE_NAME("cortex-m3"));
> +
> +    s->uart = nrf51_uart_create(UART_BASE, qdev_get_gpio_in(s->nvic, 2),
> +                                serial_hd(0));
>  }
>
>  static Property nrf51_soc_properties[] = {
> diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> index 1b979100b7..1060c62a54 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..2da97aa0c4
> --- /dev/null
> +++ b/hw/char/nrf51_uart.c
> @@ -0,0 +1,232 @@
> +/*
> + * 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.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/registerfields.h"
> +#include "hw/char/nrf51_uart.h"
> +
> +REG32(STARTRX, 0x000)
> +REG32(STOPRX, 0x004)
> +REG32(STARTTX, 0x008)
> +REG32(STOPTX, 0x00C)
> +REG32(SUSPEND, 0x01C)
> +
> +REG32(CTS, 0x100)
> +REG32(NCTS, 0x104)
> +REG32(RXDRDY, 0x108)
> +REG32(TXDRDY, 0x11C)
> +REG32(ERROR, 0x124)
> +REG32(RXTO, 0x144)
> +
> +REG32(INTEN, 0x300)
> +    FIELD(INTEN, CTS, 0, 1)
> +    FIELD(INTEN, NCTS, 1, 1)
> +    FIELD(INTEN, RXDRDY, 2, 1)
> +    FIELD(INTEN, TXDRDY, 7, 1)
> +    FIELD(INTEN, ERROR, 9, 1)
> +    FIELD(INTEN, RXTO, 17, 1)
> +REG32(INTENSET, 0x304)
> +REG32(INTENCLR, 0x308)
> +REG32(ERRORSRC, 0x480)
> +REG32(ENABLE, 0x500)
> +REG32(PSELRTS, 0x508)
> +REG32(PSELTXD, 0x50C)
> +REG32(PSELCTS, 0x510)
> +REG32(PSELRXD, 0x514)
> +REG32(RXD, 0x518)
> +REG32(TXD, 0x51C)
> +REG32(BAUDRATE, 0x524)
> +REG32(CONFIG, 0x56C)
> +
> +static void nrf51_uart_update_irq(Nrf51UART *s)
> +{
> +    unsigned int irq = 0;
> +
> +    irq = irq || (s->reg[A_RXDRDY] && (s->reg[A_INTEN] & R_INTEN_RXDRDY_MASK));
> +    irq = irq || (s->reg[A_TXDRDY] && (s->reg[A_INTEN] & R_INTEN_TXDRDY_MASK));
> +    irq = irq || (s->reg[A_ERROR]  && (s->reg[A_INTEN] & R_INTEN_ERROR_MASK));
> +    irq = irq || (s->reg[A_RXTO]   && (s->reg[A_INTEN] & R_INTEN_RXTO_MASK));
> +
> +    qemu_set_irq(s->irq, !!irq);
> +}
> +
> +static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    Nrf51UART *s = NRF51_UART(opaque);
> +    uint64_t r;
> +
> +    switch (addr) {
> +    case A_RXD:
> +        r = s->rx_fifo[s->rx_fifo_pos];
> +        if (s->rx_fifo_len > 0) {
> +            s->rx_fifo_pos = (s->rx_fifo_pos + 1) % UART_FIFO_LENGTH;
> +            s->rx_fifo_len--;
> +            qemu_chr_fe_accept_input(&s->chr);
> +        }
> +        break;
> +
> +    case A_INTENSET:
> +    case A_INTENCLR:
> +    case A_INTEN:
> +        r = s->reg[A_INTEN];
> +        break;
> +    default:
> +        r = s->reg[addr];

You can use R_* macros for registers and access regs[ ] with addr/4 as index.
It is better than using big regs[ ] array out of which most of
locations go unused.
Say, for 0x4 register you are using regs[4] and regs[1], regs[2],
regs[3] remain unused.

Thanks,
Sundeep

> +        break;
> +    }
> +
> +    return r;
> +}
> +
> +static gboolean uart_transmit(GIOChannel *chan, GIOCondition cond, void *opaque)
> +{
> +    Nrf51UART *s = NRF51_UART(opaque);
> +    int r;
> +
> +    s->watch_tag = 0;
> +
> +    r = qemu_chr_fe_write(&s->chr, (uint8_t *) &s->reg[A_TXD], 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) {
> +            goto buffer_drained;
> +        }
> +        return FALSE;
> +    }
> +
> +buffer_drained:
> +    s->reg[A_TXDRDY] = 1;
> +    nrf51_uart_update_irq(s);
> +    return FALSE;
> +}
> +
> +static void uart_cancel_transmit(Nrf51UART *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)
> +{
> +    Nrf51UART *s = NRF51_UART(opaque);
> +
> +    switch (addr) {
> +    case A_TXD:
> +        s->reg[A_TXD] = value;
> +        uart_transmit(NULL, G_IO_OUT, s);
> +        break;
> +    case A_INTENSET:
> +        s->reg[A_INTEN] |= value;
> +        break;
> +    case A_INTENCLR:
> +        s->reg[A_INTEN] &= ~value;
> +        break;
> +    case A_CTS ... A_RXTO:
> +        s->reg[addr] = value;
> +        nrf51_uart_update_irq(s);
> +    default:
> +        s->reg[addr] = value;
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps uart_ops = {
> +    .read =  uart_read,
> +    .write = uart_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void nrf51_uart_reset(DeviceState *dev)
> +{
> +    Nrf51UART *s = NRF51_UART(dev);
> +
> +    uart_cancel_transmit(s);
> +
> +    memset(s->reg, 0, sizeof(s->reg));
> +
> +    s->rx_fifo_len = 0;
> +    s->rx_fifo_pos = 0;
> +}
> +
> +static void uart_receive(void *opaque, const uint8_t *buf, int size)
> +{
> +
> +   Nrf51UART *s = NRF51_UART(opaque);
> +
> +   if (s->rx_fifo_len >= UART_FIFO_LENGTH) {
> +        return;
> +    }
> +
> +    s->rx_fifo[(s->rx_fifo_pos + s->rx_fifo_len) % UART_FIFO_LENGTH] = *buf;
> +    s->rx_fifo_len++;
> +
> +    s->reg[A_RXDRDY] = 1;
> +    nrf51_uart_update_irq(s);
> +}
> +
> +static int uart_can_receive(void *opaque)
> +{
> +    Nrf51UART *s = NRF51_UART(opaque);
> +
> +    return (s->rx_fifo_len < sizeof(s->rx_fifo));
> +}
> +
> +static void nrf51_uart_realize(DeviceState *dev, Error **errp)
> +{
> +    Nrf51UART *s = NRF51_UART(dev);
> +
> +    qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
> +                             NULL, NULL, s, NULL, true);
> +}
> +
> +static void nrf51_uart_init(Object *obj)
> +{
> +    Nrf51UART *s = NRF51_UART(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +
> +    memory_region_init_io(&s->mmio, obj, &uart_ops, s,
> +                          "nrf51_soc.uart", 0x1000);
> +    sysbus_init_mmio(sbd, &s->mmio);
> +    sysbus_init_irq(sbd, &s->irq);
> +}
> +
> +static Property nrf51_uart_properties[] = {
> +    DEFINE_PROP_CHR("chardev", Nrf51UART, 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;
> +}
> +
> +static const TypeInfo nrf51_uart_info = {
> +    .name = TYPE_NRF51_UART,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(Nrf51UART),
> +    .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/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
> index a6bbe9f108..a38b984675 100644
> --- a/include/hw/arm/nrf51_soc.h
> +++ b/include/hw/arm/nrf51_soc.h
> @@ -24,6 +24,7 @@ typedef struct NRF51State {
>      /*< public >*/
>      char *kernel_filename;
>      DeviceState *nvic;
> +    DeviceState *uart;
>
>      MemoryRegion iomem;
>  } NRF51State;
> diff --git a/include/hw/char/nrf51_uart.h b/include/hw/char/nrf51_uart.h
> new file mode 100644
> index 0000000000..758203f1c3
> --- /dev/null
> +++ b/include/hw/char/nrf51_uart.h
> @@ -0,0 +1,54 @@
> +/*
> + * 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"
> +
> +#define UART_FIFO_LENGTH 6
> +
> +#define TYPE_NRF51_UART "nrf51_soc.uart"
> +#define NRF51_UART(obj) OBJECT_CHECK(Nrf51UART, (obj), TYPE_NRF51_UART)
> +
> +typedef struct Nrf51UART {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion mmio;
> +    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[0x1000];
> +} Nrf51UART;
> +
> +static inline DeviceState *nrf51_uart_create(hwaddr addr,
> +                                             qemu_irq irq,
> +                                             Chardev *chr)
> +{
> +    DeviceState *dev;
> +    SysBusDevice *s;
> +
> +    dev = qdev_create(NULL, "nrf51_soc.uart");
> +    s = SYS_BUS_DEVICE(dev);
> +    qdev_prop_set_chr(dev, "chardev", chr);
> +    qdev_init_nofail(dev);
> +    sysbus_mmio_map(s, 0, addr);
> +    sysbus_connect_irq(s, 0, irq);
> +
> +    return dev;
> +}
> +
> +#endif
> --
> 2.17.0
>
>

Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART

[Stefan Hajnoczi]

On Thu, May 31, 2018 at 2:58 PM, sundeep subbaraya
<sundeep.lkml@gmail.com> wrote:
> On Wed, May 30, 2018 at 3:33 AM, Julia Suvorova via Qemu-devel
> <qemu-devel@nongnu.org> wrote:
>> +static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
>> +{
>> +    Nrf51UART *s = NRF51_UART(opaque);
>> +    uint64_t r;
>> +
>> +    switch (addr) {
>> +    case A_RXD:
>> +        r = s->rx_fifo[s->rx_fifo_pos];
>> +        if (s->rx_fifo_len > 0) {
>> +            s->rx_fifo_pos = (s->rx_fifo_pos + 1) % UART_FIFO_LENGTH;
>> +            s->rx_fifo_len--;
>> +            qemu_chr_fe_accept_input(&s->chr);
>> +        }
>> +        break;
>> +
>> +    case A_INTENSET:
>> +    case A_INTENCLR:
>> +    case A_INTEN:
>> +        r = s->reg[A_INTEN];
>> +        break;
>> +    default:
>> +        r = s->reg[addr];
>
> You can use R_* macros for registers and access regs[ ] with addr/4 as index.
> It is better than using big regs[ ] array out of which most of
> locations go unused.

Good point.  The bug is more severe than an inefficiency.
s->reg[addr] allows out-of-bounds accesses.  This is a security bug.

The memory region is 0x1000 *bytes* long, but the array has 0x1000
32-bit *elements*.  A read from address 0xfffc results in a memory
load from s->reg + 0xfffc * sizeof(s->reg[0]).  That's beyond the end
of the array!

s->reg[A_*] should be changed to s->reg[R_*].  s->reg[addr] needs to
be s->reg[addr / sizeof(s->reg[0])].

It may be worth adding a warning to scripts/checkpatch.pl for
array[A_*] so this bug is reported automatically in the future.

Stefan

Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART

[Stefan Hajnoczi]

On Fri, Jun 1, 2018 at 11:41 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, May 31, 2018 at 2:58 PM, sundeep subbaraya
> <sundeep.lkml@gmail.com> wrote:
>> On Wed, May 30, 2018 at 3:33 AM, Julia Suvorova via Qemu-devel
>> <qemu-devel@nongnu.org> wrote:
>>> +static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
>>> +{
>>> +    Nrf51UART *s = NRF51_UART(opaque);
>>> +    uint64_t r;
>>> +
>>> +    switch (addr) {
>>> +    case A_RXD:
>>> +        r = s->rx_fifo[s->rx_fifo_pos];
>>> +        if (s->rx_fifo_len > 0) {
>>> +            s->rx_fifo_pos = (s->rx_fifo_pos + 1) % UART_FIFO_LENGTH;
>>> +            s->rx_fifo_len--;
>>> +            qemu_chr_fe_accept_input(&s->chr);
>>> +        }
>>> +        break;
>>> +
>>> +    case A_INTENSET:
>>> +    case A_INTENCLR:
>>> +    case A_INTEN:
>>> +        r = s->reg[A_INTEN];
>>> +        break;
>>> +    default:
>>> +        r = s->reg[addr];
>>
>> You can use R_* macros for registers and access regs[ ] with addr/4 as index.
>> It is better than using big regs[ ] array out of which most of
>> locations go unused.
>
> Good point.  The bug is more severe than an inefficiency.
> s->reg[addr] allows out-of-bounds accesses.  This is a security bug.
>
> The memory region is 0x1000 *bytes* long, but the array has 0x1000
> 32-bit *elements*.  A read from address 0xfffc results in a memory
> load from s->reg + 0xfffc * sizeof(s->reg[0]).  That's beyond the end
> of the array!

Sorry, I was wrong.  The array is large enough after all.  It's just
an inefficiency, but still worth fixing.  Similar issues could lead to
out-of-bound accesses.

Stefan

Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART

[Julia Suvorova]

On 31.05.2018 12:42, Stefan Hajnoczi wrote:
 > On Wed, May 30, 2018 at 01:03:37AM +0300, Julia Suvorova wrote:
 >> The following features are not yet implemented:
 >>      Control with SUSPEND/START*/STOP*
 >
 > This is probably worth implementing for completeness.  Just rx_enabled
 > and tx_enabled boolean states will be sufficient.  SUSPEND flushes tx
 > and stops rx, it doesn't require any extra state.
 >
 >>      CTS/NCTS flow control
 >
 > CTS flow control is probably not necessary since we don't have bitwise
 > I/O and guest applications probably don't care either.
 >
 >>      Mapping registers to pins
 >
 > This is probably not necessary since the micro:bit only uses the UART in
 > one pin configuration (back to the USB interface chip).
 >
 > Please make sure that every register is explicitly handle in the code
 > and falls into these categories:
 >
 > 1. Implemented.
 > 2. Unimplemented - can be observed via a trace event.  This will make
 >     debugging easier in the future when an application doesn't work
 >     with the UART.
 > 3. Stubbed out - explicitly marked as ignored in the code.
 >
 > This way we can be confident about the completeness of this device
 > model.

I've definitely missed handling some registers, so marking it seems a
good idea.

 >> diff --git a/hw/char/nrf51_uart.c b/hw/char/nrf51_uart.c
 >> new file mode 100644
 >> index 0000000000..2da97aa0c4
 >> --- /dev/null
 >> +++ b/hw/char/nrf51_uart.c
 >> @@ -0,0 +1,232 @@
 >> +/*
 >> + * nRF51 SoC UART emulation
 >
 > Please add a reference to the hardware specfication here:
 >
 >    See nRF51 Series Reference Manual, "29 Universal Asynchronous
 >    Receiver/Transmitter" for hardware specifications:
 >    http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
 >

Sure, I'll add it.

 >> +static gboolean uart_transmit(GIOChannel *chan, GIOCondition cond, void *opaque)
 >> +{
 >> +    Nrf51UART *s = NRF51_UART(opaque);
 >> +    int r;
 >> +
 >> +    s->watch_tag = 0;
 >> +
 >> +    r = qemu_chr_fe_write(&s->chr, (uint8_t *) &s->reg[A_TXD], 1);
 >
 > This does not work on big-endian hosts:
 >
 >    s->reg[A_TXD] = 'A';
 >
 > little-endian memory layout: 41 00 00 00
 > big-endian memory layout:    00 00 00 41
 > (uint8_t *) &s->reg[A_TXD] --^
 >
 > Instead please use a uint8_t local:
 >
 >    uint8_t ch = s->reg[A_TXD];
 >
 >    ...
 >
 >    r = qemu_chr_fe_write(&s->chr, &ch, 1);
 >

A real bug! I'll fix this.

 >> +    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) {
 >> +            goto buffer_drained;
 >
 > Please add a comment here:
 >
 >    /* The hardware has no transmit error reporting, so silently drop the byte */
 >

The QEMU code is inconsistent with comments. Some files are full of
comments, some have only code. Is there any policy how to comment
the files?

 >> +static void uart_receive(void *opaque, const uint8_t *buf, int size)
 >> +{
 >> +
 >> +   Nrf51UART *s = NRF51_UART(opaque);
 >> +
 >> +   if (s->rx_fifo_len >= UART_FIFO_LENGTH) {
 >
 > Indentation is off.  Please use 4 spaces.

Sorry, have no idea how it happened.

 >> +        return;
 >> +    }
 >> +
 >> +    s->rx_fifo[(s->rx_fifo_pos + s->rx_fifo_len) % UART_FIFO_LENGTH] = *buf;
 >> +    s->rx_fifo_len++;
 >> +
 >> +    s->reg[A_RXDRDY] = 1;
 >> +    nrf51_uart_update_irq(s);
 >> +}
 >> +
 >> +static int uart_can_receive(void *opaque)
 >> +{
 >> +    Nrf51UART *s = NRF51_UART(opaque);
 >> +
 >> +    return (s->rx_fifo_len < sizeof(s->rx_fifo));
 >
 > This is very subtle: this function returns the number of bytes that can
 > be read.  This expression looks like a boolean return value but actually
 > relies on the implicit int cast (false -> 0 bytes, true -> 1 byte).
 >
 > Please write it so it doesn't look like a boolean return value:
 >
 >    if (s->rx_fifo_len >= sizeof(s->rx_fifo)) {
 >        return 0;
 >    }
 >
 >    return 1 /* byte */;
 >
 > Alternatively, you could handle more than 1 byte:
 >
 >    return sizeof(s->rx_fifo) - s->rx_fifo_len;
 >
 > but now uart_receive() needs to handle up to sizeof(s->rx_fifo) bytes of
 > data in a single call.

I will rewrite uart_receive function to accept many bytes at once.

 >> +}
 >> +
 >> +static void nrf51_uart_realize(DeviceState *dev, Error **errp)
 >> +{
 >> +    Nrf51UART *s = NRF51_UART(dev);
 >> +
 >> +    qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
 >> +                             NULL, NULL, s, NULL, true);
 >> +}
 >> +
 >> +static void nrf51_uart_init(Object *obj)
 >> +{
 >> +    Nrf51UART *s = NRF51_UART(obj);
 >> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 >> +
 >> +    memory_region_init_io(&s->mmio, obj, &uart_ops, s,
 >> +                          "nrf51_soc.uart", 0x1000);
 >
 > s/0x1000/UART_SIZE/

Should I just add a new define or move the existing one from nrf51_soc.c to
some header (or any other way to pass the UART_SIZE)?

Best regards, Julia Suvorova.

Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART

[Julia Suvorova]

On 01.06.2018 13:44, Stefan Hajnoczi wrote:
> On Fri, Jun 1, 2018 at 11:41 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Thu, May 31, 2018 at 2:58 PM, sundeep subbaraya
>> <sundeep.lkml@gmail.com> wrote:
>>> On Wed, May 30, 2018 at 3:33 AM, Julia Suvorova via Qemu-devel
>>> <qemu-devel@nongnu.org> wrote:
>>>> +static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
>>>> +{
>>>> +    Nrf51UART *s = NRF51_UART(opaque);
>>>> +    uint64_t r;
>>>> +
>>>> +    switch (addr) {
>>>> +    case A_RXD:
>>>> +        r = s->rx_fifo[s->rx_fifo_pos];
>>>> +        if (s->rx_fifo_len > 0) {
>>>> +            s->rx_fifo_pos = (s->rx_fifo_pos + 1) % UART_FIFO_LENGTH;
>>>> +            s->rx_fifo_len--;
>>>> +            qemu_chr_fe_accept_input(&s->chr);
>>>> +        }
>>>> +        break;
>>>> +
>>>> +    case A_INTENSET:
>>>> +    case A_INTENCLR:
>>>> +    case A_INTEN:
>>>> +        r = s->reg[A_INTEN];
>>>> +        break;
>>>> +    default:
>>>> +        r = s->reg[addr];
>>>
>>> You can use R_* macros for registers and access regs[ ] with addr/4 as index.
>>> It is better than using big regs[ ] array out of which most of
>>> locations go unused.
>>
>> Good point.  The bug is more severe than an inefficiency.
>> s->reg[addr] allows out-of-bounds accesses.  This is a security bug.
>>
>> The memory region is 0x1000 *bytes* long, but the array has 0x1000
>> 32-bit *elements*.  A read from address 0xfffc results in a memory
>> load from s->reg + 0xfffc * sizeof(s->reg[0]).  That's beyond the end
>> of the array!
> 
> Sorry, I was wrong.  The array is large enough after all.  It's just
> an inefficiency, but still worth fixing.  Similar issues could lead to
> out-of-bound accesses.

Even if I change the reg array to work with the R_* macros, it will
still be inefficient, since registers are not sequentially located.
I can define the registers in the Nrf51UART structure separately, but
this will increase the code size. Is there a proper way?

Best regards, Julia Suvorova.

Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART

[Peter Maydell]

On 1 June 2018 at 16:21, Julia Suvorova <jusual@mail.ru> wrote:
> On 31.05.2018 12:42, Stefan Hajnoczi wrote:
>> Please add a comment here:
>>
>>    /* The hardware has no transmit error reporting, so silently drop the
>> byte */
>>
>
> The QEMU code is inconsistent with comments. Some files are full of
> comments, some have only code. Is there any policy how to comment
> the files?

The only policy we have written down is that you should use /* */
even for one-line comments.
    /* one line comments like this */

We're inconsistent about how to format multiline comments, but
my preference for the arm parts of the codebase is:
    /* multi line comments look
     * like this
     */

For the actual content of comments:
 * comments generally should describe the 'why' rather than the 'what'
   (but not "why did we change this?", which goes in the commit message)
 * comments are helpful where the code looks wrong or strange
   (as an indication of "this is deliberate and this is why";
   Stefan's suggested comment above is in this category). If
   there's a way to write the code that doesn't look wrong then
   that's obviously preferable :-)
 * you don't need to comment things that are obvious from just
   reading the code
 * I prefer to err on the side of more comments rather than fewer
 * you don't need to replicate lots of info that's in the data sheet,
   but don't assume your readers know all the weird corners of the
   hardware behaviour inside-out, either
 * known missing or broken things can be commented with TODO or
   FIXME comments (but if you have too many of these for easy things
   we may ask you to just fix some of them :-))

Code reviewers will let you know if you've got the balance wrong.

As a special case:
 * any new function with global scope should have a doc-comment
   (starts with "/**") describing its purpose and API

Optionally:
 * for a few recent devices I thought it was useful to document
   in their header what the "QEMU interface" (what the various
   QOM properties, named GPIO outputs, inputs, etc are). You can
   see examples with 'grep "QEMU interface" include/'. If that seems
   helpful, feel free to have a comment like that; if not, skip it.

Overall advice: QEMU is a large code base, and it has both new
and well maintained parts and also very old parts that are
unloved and often use no-longer-recommended style or APIs.
When you're looking around for examples to copy the style of,
it's better to try to find something that's been added or
overhauled recently.

>>> +static int uart_can_receive(void *opaque)
>>> +{
>>> +    Nrf51UART *s = NRF51_UART(opaque);
>>> +
>>> +    return (s->rx_fifo_len < sizeof(s->rx_fifo));
>>
>> This is very subtle: this function returns the number of bytes that can
>> be read.  This expression looks like a boolean return value but actually
>> relies on the implicit int cast (false -> 0 bytes, true -> 1 byte).
>>
>> Please write it so it doesn't look like a boolean return value:
>>
>>    if (s->rx_fifo_len >= sizeof(s->rx_fifo)) {
>>        return 0;
>>    }
>>
>>    return 1 /* byte */;
>>
>> Alternatively, you could handle more than 1 byte:
>>
>>    return sizeof(s->rx_fifo) - s->rx_fifo_len;
>>
>> but now uart_receive() needs to handle up to sizeof(s->rx_fifo) bytes of
>> data in a single call.
>
> I will rewrite uart_receive function to accept many bytes at once.

Stefan, I was wondering about this when I read this patch.
What is the API for the can_receive and receive functions?
There's no documentation of the semantics either with the
IOReadHandler or IOCanReadHandler typedefs in main-loop.h,
or in the doc comment for qemu_chr_fe_set_handlers.

I'm guessing that the answer is "your can_read handler should
return the number of bytes you can accept, and a subsequent call
to the read handler then must accept that many bytes, it can't
change its mind and only accept a smaller number" (with some sort
of guarantee by the caller that it won't let guest execution or
other simulation-changes things between the call to can_receive
and receive) ?

(Similarly we don't document the semantics for the NetClientInfo
can_receive and receive functions.)

>>> +}
>>> +
>>> +static void nrf51_uart_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    Nrf51UART *s = NRF51_UART(dev);
>>> +
>>> +    qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
>>> +                             NULL, NULL, s, NULL, true);
>>> +}
>>> +
>>> +static void nrf51_uart_init(Object *obj)
>>> +{
>>> +    Nrf51UART *s = NRF51_UART(obj);
>>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>>> +
>>> +    memory_region_init_io(&s->mmio, obj, &uart_ops, s,
>>> +                          "nrf51_soc.uart", 0x1000);
>>
>> s/0x1000/UART_SIZE/
>
> Should I just add a new define or move the existing one from nrf51_soc.c to
> some header (or any other way to pass the UART_SIZE)?

Usual thing here is that the uart's own header file defines this sort
of constant, and then the SoC's .c file includes that header file
and uses the constant.

thanks
-- PMM

Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART

[Stefan Hajnoczi]

On Fri, Jun 1, 2018 at 4:58 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 June 2018 at 16:21, Julia Suvorova <jusual@mail.ru> wrote:
>> On 31.05.2018 12:42, Stefan Hajnoczi wrote:
>>>> +static int uart_can_receive(void *opaque)
>>>> +{
>>>> +    Nrf51UART *s = NRF51_UART(opaque);
>>>> +
>>>> +    return (s->rx_fifo_len < sizeof(s->rx_fifo));
>>>
>>> This is very subtle: this function returns the number of bytes that can
>>> be read.  This expression looks like a boolean return value but actually
>>> relies on the implicit int cast (false -> 0 bytes, true -> 1 byte).
>>>
>>> Please write it so it doesn't look like a boolean return value:
>>>
>>>    if (s->rx_fifo_len >= sizeof(s->rx_fifo)) {
>>>        return 0;
>>>    }
>>>
>>>    return 1 /* byte */;
>>>
>>> Alternatively, you could handle more than 1 byte:
>>>
>>>    return sizeof(s->rx_fifo) - s->rx_fifo_len;
>>>
>>> but now uart_receive() needs to handle up to sizeof(s->rx_fifo) bytes of
>>> data in a single call.
>>
>> I will rewrite uart_receive function to accept many bytes at once.
>
> Stefan, I was wondering about this when I read this patch.
> What is the API for the can_receive and receive functions?
> There's no documentation of the semantics either with the
> IOReadHandler or IOCanReadHandler typedefs in main-loop.h,
> or in the doc comment for qemu_chr_fe_set_handlers.
>
> I'm guessing that the answer is "your can_read handler should
> return the number of bytes you can accept, and a subsequent call
> to the read handler then must accept that many bytes, it can't
> change its mind and only accept a smaller number" (with some sort
> of guarantee by the caller that it won't let guest execution or
> other simulation-changes things between the call to can_receive
> and receive) ?
>
> (Similarly we don't document the semantics for the NetClientInfo
> can_receive and receive functions.)

I'll send documentation patches.

Stefan