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

[Antony Pavlov]

On Fri, 30 Aug 2013 15:16:42 +1000
Peter Crosthwaite <address@hidden> wrote:

> Hi Antony,
> 
> On Thu, Aug 29, 2013 at 7:33 PM, Antony Pavlov <address@hidden> wrote:
> > Signed-off-by: Antony Pavlov <address@hidden>
> > ---
> >  hw/arm/digic.c        |   3 +
> >  hw/char/Makefile.objs |   1 +
> >  hw/char/digic-uart.c  | 207 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 211 insertions(+)
> >  create mode 100644 hw/char/digic-uart.c
> >
> > diff --git a/hw/arm/digic.c b/hw/arm/digic.c
> > index 4ddf338..20a06a7 100644
> > --- a/hw/arm/digic.c
> > +++ b/hw/arm/digic.c
> > @@ -30,6 +30,7 @@
> >  #define DIGIC4_TIMER0    0xc0210000
> >  #define DIGIC4_TIMER1    0xc0210100
> >  #define DIGIC4_TIMER2    0xc0210200
> > +#define DIGIC4_UART      0xc0800000
> >
> >  typedef struct {
> >      ARMCPU *cpu;
> > @@ -54,6 +55,8 @@ static DigicState *digic4_create(void)
> >      sysbus_create_simple("digic-timer", DIGIC4_TIMER1, NULL);
> >      sysbus_create_simple("digic-timer", DIGIC4_TIMER2, NULL);
> >
> > +    sysbus_create_simple("digic-uart", DIGIC4_UART, NULL);
> > +
> >      return s;
> >  }
> >
> > 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..0bea32e
> > --- /dev/null
> > +++ b/hw/char/digic-uart.c
> > @@ -0,0 +1,207 @@
> > +/*
> > + * 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.
> > + *
> 
> FWIW, I think that's an older one.
> 
> > + * 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 "qemu/error-report.h"
> > +
> > +enum {
> > +    R_TX = 0x00,
> > +    R_RX,
> > +    R_ST = (0x14 >> 2),
> > +    R_MAX
> > +};
> > +
> > +enum {
> > +    ST_RX_RDY = (1 << 0),
> > +    ST_TX_RDY = (1 << 1),
> > +};
> > +
> > +#define TYPE_DIGIC_UART "digic-uart"
> > +#define DIGIC_UART(obj) \
> > +    OBJECT_CHECK(DigicUartState, (obj), TYPE_DIGIC_UART)
> > +
> > +struct DigicUartState {
> > +    SysBusDevice parent_obj;
> > +
> > +    MemoryRegion regs_region;
> > +    CharDriverState *chr;
> > +
> > +    uint32_t regs[R_MAX];
> > +};
> > +typedef struct DigicUartState DigicUartState;
> > +
> > +static uint64_t uart_read(void *opaque, hwaddr addr,
> > +                          unsigned size)
> 
> I think Andreas may have commented on other patches, but can you use a
> descriptive prefix to fn names? For one, it makes your code easier to
> debug in gdb when you have unique names for all fns tree wide.
> 
> > +{
> > +    DigicUartState *s = opaque;
> > +    uint32_t r = 0;
> > +
> > +    addr >>= 2;
> > +
> > +    switch (addr) {
> > +    case R_RX:
> > +        r = s->regs[addr];
> > +        s->regs[R_ST] &= ~(ST_RX_RDY);
> > +        break;
> > +
> > +    case R_ST:
> > +        r = s->regs[addr];
> > +        break;
> > +
> > +    default:
> > +        error_report("digic_uart: read access to unknown register 0x"
> > +                TARGET_FMT_plx, addr << 2);
> 
> qemu_log(LOG_GUEST_ERROR, ... here instead of error_report I think.
> error_report is more about QEMU misbehaving, not the guest sw.
> 
> > +        break;
> > +    }
> > +
> > +    return r;
> 
> Can you just return s->regs[addr] directly here to avoid unneeded
> variable r and its repeated setting to the same expression?
> 
> > +}
> > +
> > +static void 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(s->chr, &ch, 1);
> 
> qemu_chr_fe_write() is capable of returning 0 to indicate EAGAIN (and
> friends) and you don't handle this. You can just use
> qemu_chr_fe_write_all() to fix.
> 
> > +        }
> > +        break;
> > +
> > +    case R_ST:
> > +        /* ignore */

Thanks for your profitable comments! I'm agree with all of them but thisone.

> This is probably a guest error as you are writing a read only
> register? If so, I'd convert the check below to guest error (as I have
> commented above) and just drop this special case.

There is no guest error here.
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.

Please see the barebox digic uart driver:
https://github.com/frantony/barebox/blob/next.digic.20130829/drivers/serial/serial_digic.c#L57

and expecially the comments on the digic_serial_tstc() function:
https://github.com/frantony/barebox/blob/next.digic.20130829/drivers/serial/serial_digic.c#L76

Ignoring writes to R_ST is only a simplification of the model that has no 
perceptible side effects for existing guests.

Moreover, using existing canon disassembled code I can't unambiguously restore 
the R_ST behaviour rules.

> > +        break;
> > +
> > +    default:
> > +        error_report("digic_uart: write access to unknown register 0x"
> > +                TARGET_FMT_plx, addr << 2);
> > +        break;
> > +    }
> > +}
> > +
> > +static const MemoryRegionOps uart_mmio_ops = {
> > +    .read = uart_read,
> > +    .write = uart_write,
> > +    .valid = {
> > +        .min_access_size = 4,
> > +        .max_access_size = 4,
> > +    },
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > +};
> > +
> > +static void uart_rx(void *opaque, const uint8_t *buf, int size)
> > +{
> > +    DigicUartState *s = opaque;
> > +
> > +    assert(!(s->regs[R_ST] & ST_RX_RDY));
> > +
> 
> You could just call uart_can_rx instead of replicated expression to
> make what your asserting a little more self documenting.
> 
> > +    s->regs[R_ST] |= ST_RX_RDY;
> > +    s->regs[R_RX] = *buf;
> > +}
> > +
> > +static int uart_can_rx(void *opaque)
> > +{
> > +    DigicUartState *s = opaque;
> > +
> > +    return !(s->regs[R_ST] & ST_RX_RDY);
> > +}
> > +
> > +static void uart_event(void *opaque, int event)
> > +{
> > +}
> > +
> > +static void digic_uart_reset(DeviceState *d)
> > +{
> > +    DigicUartState *s = DIGIC_UART(d);
> > +    int i;
> > +
> > +    for (i = 0; i < R_MAX; i++) {
> > +        s->regs[i] = 0;
> > +    }
> > +    s->regs[R_ST] = ST_TX_RDY;
> > +}
> > +
> > +static int digic_uart_init(SysBusDevice *dev)
> > +{
> > +    DigicUartState *s = DIGIC_UART(dev);
> > +
> > +    memory_region_init_io(&s->regs_region, OBJECT(s), &uart_mmio_ops, s,
> > +                          "digic-uart", R_MAX * 4);
> > +    sysbus_init_mmio(dev, &s->regs_region);
> > +
> > +    s->chr = qemu_char_get_next_serial();
> > +    if (s->chr) {
> > +        qemu_chr_add_handlers(s->chr, uart_can_rx, uart_rx, uart_event, s);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static const VMStateDescription vmstate_digic_uart = {
> > +    .name = "digic-uart",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .minimum_version_id_old = 1,
> > +    .fields      = (VMStateField[]) {
> > +        VMSTATE_UINT32_ARRAY(regs, DigicUartState, R_MAX),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> > +static void digic_uart_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> > +
> 
> Use of SysBusDevice::init is deprecated. Please use Device::realize
> instead of SysBusDevice::init. Check dma/pl330.c for an example of the
> pattern.
> 
> Regards,
> Peter
> 
> > +    k->init = digic_uart_init;
> > +    dc->reset = digic_uart_reset;
> > +    dc->vmsd = &vmstate_digic_uart;
> > +}
> > +
> > +static const TypeInfo digic_uart_info = {
> > +    .name          = TYPE_DIGIC_UART,
> > +    .parent        = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size = sizeof(DigicUartState),
> > +    .class_init    = digic_uart_class_init,
> > +};
> > +
> > +static void digic_uart_register_types(void)
> > +{
> > +    type_register_static(&digic_uart_info);
> > +}
> > +
> > +type_init(digic_uart_register_types)
> > --
> > 1.8.4.rc3
> >
> >


-- 
-- 
Best regards,
  Antony Pavlov