| Message ID | 20180529220338.10879-3-jusual@mail.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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])].
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
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 > >
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
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
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.
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.
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
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
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
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