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?
> +}
> 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
