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

[Peter Crosthwaite]

+Andreas

On Mon, Dec 16, 2013 at 10:48 AM, Peter Crosthwaite
<address@hidden> wrote:
> 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
>>
>>