Hi Antony, On Thu, Aug 29, 2013 at 7:33 PM, Antony Pavlov <[email protected]> wrote: > Signed-off-by: Antony Pavlov <[email protected]> > --- > 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 <[email protected]> > + * > + * 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 */ 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. > + 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 > >
