[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC 4/5] hw/arm/digic: add UART support
From: |
Peter Crosthwaite |
Subject: |
Re: [Qemu-devel] [RFC 4/5] hw/arm/digic: add UART support |
Date: |
Fri, 30 Aug 2013 15:16:42 +1000 |
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 */
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
>
>
- [Qemu-devel] [RFC 2/5] hw/arm: add very initial support for Canon DIGIC SoC, (continued)
Re: [Qemu-devel] [RFC 2/5] hw/arm: add very initial support for Canon DIGIC SoC, Condello, 2013/08/29
[Qemu-devel] [RFC 3/5] hw/arm/digic: add timer support, Antony Pavlov, 2013/08/29
[Qemu-devel] [RFC 4/5] hw/arm/digic: add UART support, Antony Pavlov, 2013/08/29
- Re: [Qemu-devel] [RFC 4/5] hw/arm/digic: add UART support,
Peter Crosthwaite <=