From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34196) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VHpyO-0001xs-JW for qemu-devel@nongnu.org; Fri, 06 Sep 2013 02:56:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VHpyJ-0006mC-2w for qemu-devel@nongnu.org; Fri, 06 Sep 2013 02:56:52 -0400 Received: from mail-la0-x229.google.com ([2a00:1450:4010:c03::229]:52268) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VHpyI-0006lw-RU for qemu-devel@nongnu.org; Fri, 06 Sep 2013 02:56:47 -0400 Received: by mail-la0-f41.google.com with SMTP id ec20so2417307lab.14 for ; Thu, 05 Sep 2013 23:56:45 -0700 (PDT) Date: Fri, 6 Sep 2013 10:54:20 +0400 From: Antony Pavlov Message-Id: <20130906105420.3239815a8775ea9bfc2261eb@gmail.com> In-Reply-To: References: <1378367579-1099-1-git-send-email-antonynpavlov@gmail.com> <1378367579-1099-5-git-send-email-antonynpavlov@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC v4 4/5] hw/arm/digic: add UART support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers On Thu, 5 Sep 2013 19:17:50 +0100 Peter Maydell wrote: > On 5 September 2013 08:52, Antony Pavlov wrote: > > +static int uart_can_rx(void *opaque) > > +{ > > + DigicUartState *s =3D opaque; > > + > > + return !(s->regs[R_ST] & ST_RX_RDY); > > +} > > + > > +static void uart_rx(void *opaque, const uint8_t *buf, int size) > > +{ > > + DigicUartState *s =3D opaque; > > + > > + assert(uart_can_rx(opaque)); > > + > > + s->regs[R_ST] |=3D ST_RX_RDY; > > + s->regs[R_RX] =3D *buf; >=20 > 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 :). > > +} >=20 > > 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 @@ >=20 > Copyright/license header comment at start of all files, > please (ditto below). >=20 > > +#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 =3D 0x00, > > + R_RX, > > + R_ST =3D (0x14 >> 2), > > + R_MAX > > +}; > > + > > +typedef struct DigicUartState { > > + SysBusDevice parent_obj; > > + > > + MemoryRegion regs_region; > > + CharDriverState *chr; > > + > > + uint32_t regs[R_MAX]; >=20 > 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. >=20 > > +} 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 >=20 > 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. >=20 > (same is probably true of some of the other macros.) >=20 > > + > > #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 > > >=20 > thanks > -- PMM --=20 --=A0 Best regards, =A0 Antony Pavlov