On Thu, 5 Sep 2013 19:17:50 +0100
Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 5 September 2013 08:52, Antony Pavlov <antonynpav...@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
--
--
Best regards,
Antony Pavlov
