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 <address@hidden> 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 <address@hidden>

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 <address@hidden>
> + *
> + * 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 <address@hidden>
> + *
> + * 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