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

[Antony Pavlov <antonynpavlov@gmail.com>]

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


-- 
-- 
Best regards,
  Antony Pavlov