Re: [Qemu-devel] [PATCH v2 1/2] Add UART for Moxie Marin SoC

[Peter Crosthwaite]

On Mon, Dec 16, 2013 at 12:59 AM, Anthony Green <address@hidden> wrote:
>
> This patch adds the Marin UART device.
>
> Signed-off-by: Anthony Green <address@hidden>
> ---
>  default-configs/moxie-softmmu.mak |   1 +
>  hw/char/Makefile.objs             |   1 +
>  hw/char/marin-uart.c              | 200 
> ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 202 insertions(+)
>  create mode 100644 hw/char/marin-uart.c
>
> diff --git a/default-configs/moxie-softmmu.mak 
> b/default-configs/moxie-softmmu.mak
> index 1a95476..65a21de 100644
> --- a/default-configs/moxie-softmmu.mak
> +++ b/default-configs/moxie-softmmu.mak
> @@ -1,5 +1,6 @@
>  # Default configuration for moxie-softmmu
>
>  CONFIG_MC146818RTC=y
> +CONFIG_MOXIE=y
>  CONFIG_SERIAL=y
>  CONFIG_VGA=y
> diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> index cbd6a00..48bc5d0 100644
> --- a/hw/char/Makefile.objs
> +++ b/hw/char/Makefile.objs
> @@ -14,6 +14,7 @@ obj-$(CONFIG_COLDFIRE) += mcf_uart.o
>  obj-$(CONFIG_OMAP) += omap_uart.o
>  obj-$(CONFIG_SH4) += sh_serial.o
>  obj-$(CONFIG_PSERIES) += spapr_vty.o
> +obj-$(CONFIG_MOXIE) += marin-uart.o
>
>  common-obj-$(CONFIG_ETRAXFS) += etraxfs_ser.o
>  common-obj-$(CONFIG_ISA_DEBUG) += debugcon.o
> diff --git a/hw/char/marin-uart.c b/hw/char/marin-uart.c
> new file mode 100644
> index 0000000..b3606a2
> --- /dev/null
> +++ b/hw/char/marin-uart.c
> @@ -0,0 +1,200 @@
> +/*
> + *  QEMU model of the Marin UART.
> + *
> + *  Copyright (c) 2013 Anthony Green <address@hidden>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "hw/hw.h"
> +#include "hw/sysbus.h"
> +#include "trace.h"
> +#include "sysemu/char.h"
> +#include "qemu/error-report.h"
> +

Cutting from here ...

> +enum {
> +    R_RXREADY = 0,
> +    R_TXREADY,
> +    R_RXBYTE,
> +    R_TXBYTE,
> +    R_MAX
> +};
> +
> +#define TYPE_MARIN_UART "marin-uart"
> +#define MARIN_UART(obj) OBJECT_CHECK(MarinUartState, (obj), TYPE_MARIN_UART)
> +
> +typedef struct MarinUartState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
> +    MemoryRegion regs_region;
> +    CharDriverState *chr;
> +    qemu_irq irq;
> +    uint16_t regs[R_MAX];
> +} MarinUartState;
> +

... to here: New device models should define the type struct and
programmers model defines in a device specific header. This allows for

1: Embeddeding your device in a SoC container.
2: Easy creation of programmers model aware test cases (with
register/field macros).

> +static uint64_t uart_read(void *opaque, hwaddr addr,
> +                          unsigned size)
> +{
> +    MarinUartState *s = opaque;
> +    uint32_t r = 0;
> +
> +    addr >>= 1;
> +    switch (addr) {
> +    case R_RXREADY:
> +        r = s->regs[R_RXREADY];
> +        break;
> +    case R_TXREADY:
> +        r = 1;

r = s->regs[R_TX_READY] for cosistency. If anyone ever comes along to
implement more elaborate flow control they will probably want to use
R_TX_READY as the actual state.

> +        break;
> +    case R_TXBYTE:
> +        r = s->regs[R_TXBYTE];
> +        break;
> +    case R_RXBYTE:
> +        r = s->regs[R_RXBYTE];
> +        s->regs[R_RXREADY] = 0;
> +        if (s->chr) {
> +            qemu_chr_accept_input(s->chr);
> +        }
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                "marin_uart: read access to unknown register 0x"
> +                TARGET_FMT_plx, addr << 1);
> +        break;
> +    }
> +
> +    return r;
> +}
> +
> +static void uart_write(void *opaque, hwaddr addr, uint64_t value,
> +                       unsigned size)
> +{
> +    MarinUartState *s = opaque;
> +    unsigned char ch = value;
> +
> +    addr >>= 1;
> +    switch (addr) {
> +    case R_TXBYTE:
> +        if (s->chr) {
> +            qemu_chr_fe_write(s->chr, &ch, 1);

I still think dropping the char on a busy backend is a bad idea. Its a
race condition between the emulation and host backend. Yes, its a UART
and the real HW may not have flow control, but real HW has a sense of
accurate timing that can be relied on. QEMU does not. To overcome
this, the philosophy is to implement QEMU specific flow controls that
avoid racy droppages. You can achieve this by simply using
qemu_chr_fe_write_all.

Note as well that you have the equivalent flow control on your RX path
as you have a non-trivial can_rx function so your rx and tx paths are
inconsistent with each other with rx flowing, and tx dropping.

Regards,
Peter

> +        }
> +        break;
> +
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                "marin_uart: write access to unknown register 0x"
> +                TARGET_FMT_plx, addr << 1);
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps uart_mmio_ops = {
> +    .read = uart_read,
> +    .write = uart_write,
> +    .valid = {
> +        .min_access_size = 2,
> +        .max_access_size = 2,
> +    },
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void uart_rx(void *opaque, const uint8_t *buf, int size)
> +{
> +    MarinUartState *s = opaque;
> +
> +    s->regs[R_RXBYTE] = *buf;
> +    s->regs[R_RXREADY] = 1;
> +}
> +
> +static int uart_can_rx(void *opaque)
> +{
> +    MarinUartState *s = opaque;
> +
> +    return !(s->regs[R_RXREADY]);
> +}
> +
> +static void uart_event(void *opaque, int event)
> +{
> +}
> +
> +static void marin_uart_reset(DeviceState *d)
> +{
> +    MarinUartState *s = MARIN_UART(d);
> +    int i;
> +
> +    for (i = 0; i < R_MAX; i++) {
> +        s->regs[i] = 0;
> +    }
> +    s->regs[R_TXREADY] = 1;
> +}
> +
> +static void marin_uart_realize(DeviceState *dev, Error **errp)
> +{
> +    MarinUartState *s = MARIN_UART(dev);
> +
> +    s->chr = qemu_char_get_next_serial();
> +    if (s->chr) {
> +        qemu_chr_add_handlers(s->chr, uart_can_rx, uart_rx, uart_event, s);
> +    }
> +}
> +
> +static void marin_uart_init(Object *obj)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    MarinUartState *s = MARIN_UART(obj);
> +
> +    sysbus_init_irq(sbd, &s->irq);
> +
> +    memory_region_init_io(&s->regs_region, OBJECT(s), &uart_mmio_ops, s,
> +            TYPE_MARIN_UART, R_MAX * 4);
> +    sysbus_init_mmio(sbd, &s->regs_region);
> +}
> +
> +static const VMStateDescription vmstate_marin_uart = {
> +    .name = TYPE_MARIN_UART,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT16_ARRAY(regs, MarinUartState, R_MAX),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void marin_uart_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = marin_uart_realize;
> +    dc->reset = marin_uart_reset;
> +    dc->vmsd = &vmstate_marin_uart;
> +}
> +
> +static const TypeInfo marin_uart_info = {
> +    .name          = TYPE_MARIN_UART,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(MarinUartState),
> +    .instance_init = marin_uart_init,
> +    .class_init    = marin_uart_class_init,
> +};
> +
> +static void marin_uart_register_types(void)
> +{
> +    type_register_static(&marin_uart_info);
> +}
> +
> +type_init(marin_uart_register_types)
> --
> 1.8.3.1
>
>