Re: [Qemu-devel] [RFC v2 4/5] hw/arm/digic: add UART support

[Peter Crosthwaite]

Hi Antony,

On Mon, Sep 2, 2013 at 7:22 AM, Antony Pavlov <address@hidden> wrote:
> Signed-off-by: Antony Pavlov <address@hidden>
> ---
>  hw/arm/digic.c         |  14 ++++
>  hw/char/Makefile.objs  |   1 +
>  hw/char/digic-uart.c   | 197 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/char/digic-uart.h   |  27 +++++++
>  include/hw/arm/digic.h |   4 +
>  5 files changed, 243 insertions(+)
>  create mode 100644 hw/char/digic-uart.c
>  create mode 100644 hw/char/digic-uart.h
>
> diff --git a/hw/arm/digic.c b/hw/arm/digic.c
> index 0025f15..5edd1c9 100644
> --- a/hw/arm/digic.c
> +++ b/hw/arm/digic.c
> @@ -45,6 +45,11 @@ static void digic_init(Object *obj)
>          snprintf(name, 9, "timer[%d]", i);
>          object_property_add_child(obj, name, OBJECT(&s->timer[i]), NULL);
>      }
> +
> +    object_initialize(&s->uart, TYPE_DIGIC_UART);
> +    dev = DEVICE(&s->uart);
> +    qdev_set_parent_bus(dev, sysbus_get_default());
> +    object_property_add_child(obj, "uart", OBJECT(&s->uart), NULL);
>  }
>
>  static void digic_realize(DeviceState *dev, Error **errp)
> @@ -70,6 +75,15 @@ static void digic_realize(DeviceState *dev, Error **errp)
>          sbd = SYS_BUS_DEVICE(&s->timer[i]);
>          sysbus_mmio_map(sbd, 0, DIGIC4_TIMER_BASE(i));
>      }
> +
> +    object_property_set_bool(OBJECT(&s->uart), true, "realized", &err);
> +    if (err != NULL) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    sbd = SYS_BUS_DEVICE(&s->uart);
> +    sysbus_mmio_map(sbd, 0, DIGIC_UART_BASE);
>  }
>
>  static void digic_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> index f8f3dbc..00d37ac 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_DIGIC) += digic-uart.o
>
>  common-obj-$(CONFIG_ETRAXFS) += etraxfs_ser.o
>  common-obj-$(CONFIG_ISA_DEBUG) += debugcon.o
> diff --git a/hw/char/digic-uart.c b/hw/char/digic-uart.c
> new file mode 100644
> index 0000000..e0f006c
> --- /dev/null
> +++ b/hw/char/digic-uart.c
> @@ -0,0 +1,197 @@
> +/*
> + * QEMU model of the Canon Digic UART block.
> + *
> + * Copyright (C) 2013 Antony Pavlov <address@hidden>
> + *
> + * This model is based on reverse engineering efforts
> + * made by CHDK (http://chdk.wikia.com) and
> + * Magic Lantern (http://www.magiclantern.fm) projects
> + * contributors.
> + *
> + * See "Serial terminal" docs here:
> + *   http://magiclantern.wikia.com/wiki/Register_Map#Misc_Registers
> + *
> + * The QEMU model of the Milkymist UART block by Michael Walle
> + * is used as a template.
> + *
> + * 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 "sysemu/char.h"
> +
> +#include "hw/char/digic-uart.h"
> +
> +enum {
> +    ST_RX_RDY = (1 << 0),
> +    ST_TX_RDY = (1 << 1),
> +};
> +
> +static uint64_t digic_uart_read(void *opaque, hwaddr addr,
> +                          unsigned size)
> +{
> +    DigicUartState *s = opaque;
> +
> +    addr >>= 2;
> +
> +    switch (addr) {
> +    case R_RX:
> +        s->regs[R_ST] &= ~(ST_RX_RDY);
> +        break;
> +
> +    case R_ST:
> +        break;
> +

So I've been thinking about this some more, and I think you mentioned
this is due to the fact that this functionality is unimplemented
right? There is qemu_log(LOG_UNIMP for this very reason as well. Due
to the very nature of what you are trying to do with this series, it
may be a case of you have quite extensive use of both GUEST_ERROR and
UNIMP:

switch (addr) {
    case (FOO) :
        /* do some side effects */
        break;

    case (BAR):
    case (BAZ):
        qemu_log(LOG_GUEST_ERROR, "bad address %x", addr);
        break;

    default:
        qemu_log(LOG_UNIMP, "unknown address %x", addr);
}

Be specific on the cases you know about as guest errors and assume an
unimp for a default. Use of the ... syntax in switch case may help for
large ranges of unknown registers.

> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "digic_uart: read access to unknown register 0x"
> +            TARGET_FMT_plx, addr << 2);

return 0 here the guard against OOB array access,

> +    }
> +
> +    return s->regs[addr];
> +}
> +
> +static void digic_uart_write(void *opaque, hwaddr addr, uint64_t value,
> +                       unsigned size)
> +{
> +    DigicUartState *s = opaque;
> +    unsigned char ch = value;
> +
> +    addr >>= 2;
> +
> +    switch (addr) {
> +    case R_TX:
> +        if (s->chr) {
> +            qemu_chr_fe_write_all(s->chr, &ch, 1);
> +        }
> +        break;
> +
> +    case R_ST:
> +        /*
> +         * Ignore write to R_ST.
> +         *
> +         * The point is that this register is actively used
> +         * during receiving and transmitting symbols,
> +         * but we don't know the function of most of bits.
> +         *

This does sound a lot like a qemu_log(LOG_UNIMP

Regards,
Peter

> +         * Ignoring writes to R_ST is only a simplification
> +         * of the model. It has no perceptible side effects
> +         * for existing guests.
> +         */
> +        break;
> +