| Message ID | 1378367579-1099-5-git-send-email-antonynpavlov@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 5 September 2013 08:52, Antony Pavlov <antonynpavlov@gmail.com> wrote: > +static int uart_can_rx(void *opaque) > +{ > + DigicUartState *s = opaque; > + > + return !(s->regs[R_ST] & ST_RX_RDY); > +} > + > +static void uart_rx(void *opaque, const uint8_t *buf, int size) > +{ > + DigicUartState *s = opaque; > + > + assert(uart_can_rx(opaque)); > + > + s->regs[R_ST] |= ST_RX_RDY; > + s->regs[R_RX] = *buf; Does this UART really not have a FIFO? > +} > diff --git a/hw/char/digic-uart.h b/hw/char/digic-uart.h > new file mode 100644 > index 0000000..ca48f4e > --- /dev/null > +++ b/hw/char/digic-uart.h > @@ -0,0 +1,27 @@ Copyright/license header comment at start of all files, please (ditto below). > +#ifndef HW_CHAR_DIGIC_UART_H > +#define HW_CHAR_DIGIC_UART_H > + > +#include "hw/sysbus.h" > +#include "qemu/typedefs.h" > + > +#define TYPE_DIGIC_UART "digic-uart" > +#define DIGIC_UART(obj) \ > + OBJECT_CHECK(DigicUartState, (obj), TYPE_DIGIC_UART) > + > +enum { > + R_TX = 0x00, > + R_RX, > + R_ST = (0x14 >> 2), > + R_MAX > +}; > + > +typedef struct DigicUartState { > + SysBusDevice parent_obj; > + > + MemoryRegion regs_region; > + CharDriverState *chr; > + > + uint32_t regs[R_MAX]; So this thing only has five registers, one of which at least (R_TX) doesn't have state that you'll be storing in this struct anyway, and you're not implementing reads-as-written behaviour for the unknown registers, so I think you should drop the regs[] array and just have individual uint32_t fields for the registers you implement. > +} DigicUartState; > + > +#endif /* HW_CHAR_DIGIC_UART_H */ > diff --git a/include/hw/arm/digic.h b/include/hw/arm/digic.h > index 48c9f9c..c587ade 100644 > --- a/include/hw/arm/digic.h > +++ b/include/hw/arm/digic.h > @@ -11,10 +11,13 @@ > #include "cpu-qom.h" > > #include "hw/timer/digic-timer.h" > +#include "hw/char/digic-uart.h" > > #define DIGIC4_NB_TIMERS 3 > #define DIGIC4_TIMER_BASE(n) (0xc0210000 + (n) * 0x100) > > +#define DIGIC_UART_BASE 0xc0800000 Does this really need to be in the header file? It seems like private implementation information that could go in the source file that needs it. (same is probably true of some of the other macros.) > + > #define TYPE_DIGIC "digic" > > #define DIGIC(obj) OBJECT_CHECK(DigicState, (obj), TYPE_DIGIC) > @@ -25,6 +28,7 @@ typedef struct DigicState { > ARMCPU cpu; > > DigicTimerState timer[DIGIC4_NB_TIMERS]; > + DigicUartState uart; > } DigicState; > > #endif /* __DIGIC_H__ */ > -- > 1.8.4.rc3 > thanks -- PMM
On Thu, 5 Sep 2013 19:17:50 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On 5 September 2013 08:52, Antony Pavlov <antonynpavlov@gmail.com> wrote: > > +static int uart_can_rx(void *opaque) > > +{ > > + DigicUartState *s = opaque; > > + > > + return !(s->regs[R_ST] & ST_RX_RDY); > > +} > > + > > +static void uart_rx(void *opaque, const uint8_t *buf, int size) > > +{ > > + DigicUartState *s = opaque; > > + > > + assert(uart_can_rx(opaque)); > > + > > + s->regs[R_ST] |= ST_RX_RDY; > > + s->regs[R_RX] = *buf; > > Does this UART really not have a FIFO? There is no public documentation on Digic chips. Only Canon's engineers know something about Digic's FIFO (if it exists :). > > +} > > > diff --git a/hw/char/digic-uart.h b/hw/char/digic-uart.h > > new file mode 100644 > > index 0000000..ca48f4e > > --- /dev/null > > +++ b/hw/char/digic-uart.h > > @@ -0,0 +1,27 @@ > > Copyright/license header comment at start of all files, > please (ditto below). > > > +#ifndef HW_CHAR_DIGIC_UART_H > > +#define HW_CHAR_DIGIC_UART_H > > + > > +#include "hw/sysbus.h" > > +#include "qemu/typedefs.h" > > + > > +#define TYPE_DIGIC_UART "digic-uart" > > +#define DIGIC_UART(obj) \ > > + OBJECT_CHECK(DigicUartState, (obj), TYPE_DIGIC_UART) > > + > > +enum { > > + R_TX = 0x00, > > + R_RX, > > + R_ST = (0x14 >> 2), > > + R_MAX > > +}; > > + > > +typedef struct DigicUartState { > > + SysBusDevice parent_obj; > > + > > + MemoryRegion regs_region; > > + CharDriverState *chr; > > + > > + uint32_t regs[R_MAX]; > > So this thing only has five registers, one of > which at least (R_TX) doesn't have state that > you'll be storing in this struct anyway, and you're > not implementing reads-as-written behaviour for the > unknown registers, so I think you should drop the > regs[] array and just have individual uint32_t fields > for the registers you implement. > > > +} DigicUartState; > > + > > +#endif /* HW_CHAR_DIGIC_UART_H */ > > diff --git a/include/hw/arm/digic.h b/include/hw/arm/digic.h > > index 48c9f9c..c587ade 100644 > > --- a/include/hw/arm/digic.h > > +++ b/include/hw/arm/digic.h > > @@ -11,10 +11,13 @@ > > #include "cpu-qom.h" > > > > #include "hw/timer/digic-timer.h" > > +#include "hw/char/digic-uart.h" > > > > #define DIGIC4_NB_TIMERS 3 > > #define DIGIC4_TIMER_BASE(n) (0xc0210000 + (n) * 0x100) > > > > +#define DIGIC_UART_BASE 0xc0800000 > > Does this really need to be in the header file? > It seems like private implementation information that > could go in the source file that needs it. > > (same is probably true of some of the other macros.) > > > + > > #define TYPE_DIGIC "digic" > > > > #define DIGIC(obj) OBJECT_CHECK(DigicState, (obj), TYPE_DIGIC) > > @@ -25,6 +28,7 @@ typedef struct DigicState { > > ARMCPU cpu; > > > > DigicTimerState timer[DIGIC4_NB_TIMERS]; > > + DigicUartState uart; > > } DigicState; > > > > #endif /* __DIGIC_H__ */ > > -- > > 1.8.4.rc3 > > > > thanks > -- PMM
On 6 September 2013 07:54, Antony Pavlov <antonynpavlov@gmail.com> wrote: > On Thu, 5 Sep 2013 19:17:50 +0100 > Peter Maydell <peter.maydell@linaro.org> wrote: >> Does this UART really not have a FIFO? > > There is no public documentation on Digic chips. > Only Canon's engineers know something about Digic's FIFO (if it exists :). You can deduce its existence though -- does the UART let you feed two or three characters to it at faster than whatever the serial line speed is before it sets the "stop sending me bits" status bit, or does it stop after the first? If the real firmware uses this for anything remotely serious (ie for more than trivial and default-disabled debug info) it probably does have a FIFO. However, let's assume it doesn't for now. -- PMM
On Fri, 6 Sep 2013 08:25:20 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On 6 September 2013 07:54, Antony Pavlov <antonynpavlov@gmail.com> wrote: > > On Thu, 5 Sep 2013 19:17:50 +0100 > > Peter Maydell <peter.maydell@linaro.org> wrote: > >> Does this UART really not have a FIFO? > > > > There is no public documentation on Digic chips. > > Only Canon's engineers know something about Digic's FIFO (if it exists :). > > You can deduce its existence though -- does the > UART let you feed two or three characters to it > at faster than whatever the serial line speed is > before it sets the "stop sending me bits" status > bit, or does it stop after the first? No, I can't :) How can I check this mysterious "stop sending me bits" status bit? Do you mean the UART hardware flow control pin? I can't use hardware flow control as I have only 3-wire UART on the camera. Moreover I have to enable hardware flow control in software, but I have no information how to do it. > If the real firmware uses this for anything remotely > serious (ie for more than trivial and default-disabled > debug info) it probably does have a FIFO. However, > let's assume it doesn't for now. > > -- PMM
On 6 September 2013 14:00, Antony Pavlov <antonynpavlov@gmail.com> wrote: > On Fri, 6 Sep 2013 08:25:20 +0100 > Peter Maydell <peter.maydell@linaro.org> wrote: >> You can deduce its existence though -- does the >> UART let you feed two or three characters to it >> at faster than whatever the serial line speed is >> before it sets the "stop sending me bits" status >> bit, or does it stop after the first? > > No, I can't :) > > How can I check this mysterious "stop sending me bits" status bit? > Do you mean the UART hardware flow control pin? I meant ST_TX_RDY, but looking again at the docs perhaps I misinterpreted it. Do you really have to do: * write byte to TX register * set TX_RDY bit in status register to get it to send out a byte? I was expecting a more standard UART interface which is typically: * write byte to TX register * if the UART is unable to accept any more bytes (ie FIFO full, or just immediately if no FIFO present) it clears TX_RDY * software has to wait for TX_RDY to be set before writing another byte to TX -- PMM
On Fri, 6 Sep 2013 14:40:14 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On 6 September 2013 14:00, Antony Pavlov <antonynpavlov@gmail.com> wrote: > > On Fri, 6 Sep 2013 08:25:20 +0100 > > Peter Maydell <peter.maydell@linaro.org> wrote: > >> You can deduce its existence though -- does the > >> UART let you feed two or three characters to it > >> at faster than whatever the serial line speed is > >> before it sets the "stop sending me bits" status > >> bit, or does it stop after the first? > > > > No, I can't :) > > > > How can I check this mysterious "stop sending me bits" status bit? > > Do you mean the UART hardware flow control pin? > > I meant ST_TX_RDY, but looking again at the > docs perhaps I misinterpreted it. Do you really > have to do: > * write byte to TX register > * set TX_RDY bit in status register > > to get it to send out a byte? I don't want publish Canon firmware disassembled listings and discuss it in the maillist for legal reasons. But Canon A1100 firmware can use this routine: 1. wait TX_RDY bit in status register; 2. write TX_RDY and some other (unknown behaviour) bit to status register; 3. write byte to TX register. IMHO the step 2 is bizarre. You can easely check me: the disassembly instructions and firmware images can be found via CHDK site. A puts()-like routine can be found at 0xffff18f0 in the Canon A1100IS firmware image. > I was expecting a more standard UART interface > which is typically: > * write byte to TX register > * if the UART is unable to accept any more > bytes (ie FIFO full, or just immediately if > no FIFO present) it clears TX_RDY > * software has to wait for TX_RDY to be set > before writing another byte to TX IMHO the more standart UART outc() routune is: * wait for UART "transmitter is ready to send next char" flag; * write char to TX register. E.g. see https://github.com/frantony/barebox/blob/master/arch/mips/include/asm/debug_ll_ns16550.h#L60 or https://github.com/frantony/barebox/blob/master/arch/arm/mach-imx/include/mach/debug_ll.h#L49 -- Best regards, Antony Pavlov
On 7 September 2013 06:45, Antony Pavlov <antonynpavlov@gmail.com> wrote: > But Canon A1100 firmware can use this routine: > 1. wait TX_RDY bit in status register; > 2. write TX_RDY and some other (unknown behaviour) bit to status register; > 3. write byte to TX register. > > IMHO the step 2 is bizarre. Maybe the TX_RDY bit is write-1-to-clear? Some devices use that kind of thing for a "conditions that happened that could trigger an interrupt" (and if you're using them for polling you can just poll the register). > IMHO the more standart UART outc() routune is: > * wait for UART "transmitter is ready to send next char" flag; > * write char to TX register. Yes, you're right. In either case you can see if there's a uart via: * wait for TX_RDY to be set * clear TX_RDY * write byte to TX * immediately look at TX_RDY -- if it's set straight away there's almost certainly a FIFO involved; otherwise it would have to wait for the char to go out over serial before setting it again This is kind of a sidetrack though, we can model it without a UART for the moment. -- PMM
diff --git a/hw/arm/digic.c b/hw/arm/digic.c index 5932a6a..d99ffd9 100644 --- a/hw/arm/digic.c +++ b/hw/arm/digic.c @@ -46,6 +46,11 @@ static void digic_init(Object *obj) snprintf(name, DIGIC_TIMER_NAME_MLEN, "timer[%d]", i); object_property_add_child(obj, name, OBJECT(&s->timer[i]), NULL); } + + object_initialize(&s->uart, sizeof(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) @@ -71,6 +76,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..f41f74a --- /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 <antonynpavlov@gmail.com> + * + * 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; + + default: + qemu_log_mask(LOG_UNIMP, + "digic-uart: read access to unknown register 0x" + TARGET_FMT_plx, addr << 2); + } + + 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. + * + * Ignoring writes to R_ST is only a simplification + * of the model. It has no perceptible side effects + * for existing guests. + */ + break; + + default: + qemu_log_mask(LOG_UNIMP, + "digic-uart: write access to unknown register 0x" + TARGET_FMT_plx, addr << 2); + } +} + +static const MemoryRegionOps uart_mmio_ops = { + .read = digic_uart_read, + .write = digic_uart_write, + .valid = { + .min_access_size = 4, + .max_access_size = 4, + }, + .endianness = DEVICE_NATIVE_ENDIAN, +}; + +static int uart_can_rx(void *opaque) +{ + DigicUartState *s = opaque; + + return !(s->regs[R_ST] & ST_RX_RDY); +} + +static void uart_rx(void *opaque, const uint8_t *buf, int size) +{ + DigicUartState *s = opaque; + + assert(uart_can_rx(opaque)); + + s->regs[R_ST] |= ST_RX_RDY; + s->regs[R_RX] = *buf; +} + +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 void digic_uart_realize(DeviceState *dev, Error **errp) +{ + DigicUartState *s = DIGIC_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 digic_uart_init(Object *obj) +{ + DigicUartState *s = DIGIC_UART(obj); + + memory_region_init_io(&s->regs_region, OBJECT(s), &uart_mmio_ops, s, + "digic-uart", R_MAX * 4); + sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->regs_region); +} + +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); + + dc->realize = digic_uart_realize; + 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), + .instance_init = digic_uart_init, + .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) diff --git a/hw/char/digic-uart.h b/hw/char/digic-uart.h new file mode 100644 index 0000000..ca48f4e --- /dev/null +++ b/hw/char/digic-uart.h @@ -0,0 +1,27 @@ +#ifndef HW_CHAR_DIGIC_UART_H +#define HW_CHAR_DIGIC_UART_H + +#include "hw/sysbus.h" +#include "qemu/typedefs.h" + +#define TYPE_DIGIC_UART "digic-uart" +#define DIGIC_UART(obj) \ + OBJECT_CHECK(DigicUartState, (obj), TYPE_DIGIC_UART) + +enum { + R_TX = 0x00, + R_RX, + R_ST = (0x14 >> 2), + R_MAX +}; + +typedef struct DigicUartState { + SysBusDevice parent_obj; + + MemoryRegion regs_region; + CharDriverState *chr; + + uint32_t regs[R_MAX]; +} DigicUartState; + +#endif /* HW_CHAR_DIGIC_UART_H */ diff --git a/include/hw/arm/digic.h b/include/hw/arm/digic.h index 48c9f9c..c587ade 100644 --- a/include/hw/arm/digic.h +++ b/include/hw/arm/digic.h @@ -11,10 +11,13 @@ #include "cpu-qom.h" #include "hw/timer/digic-timer.h" +#include "hw/char/digic-uart.h" #define DIGIC4_NB_TIMERS 3 #define DIGIC4_TIMER_BASE(n) (0xc0210000 + (n) * 0x100) +#define DIGIC_UART_BASE 0xc0800000 + #define TYPE_DIGIC "digic" #define DIGIC(obj) OBJECT_CHECK(DigicState, (obj), TYPE_DIGIC) @@ -25,6 +28,7 @@ typedef struct DigicState { ARMCPU cpu; DigicTimerState timer[DIGIC4_NB_TIMERS]; + DigicUartState uart; } DigicState; #endif /* __DIGIC_H__ */
Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com> --- 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