From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40668) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W0By2-0002rX-L0 for qemu-devel@nongnu.org; Mon, 06 Jan 2014 10:19:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W0Bxx-0004fg-Mi for qemu-devel@nongnu.org; Mon, 06 Jan 2014 10:19:50 -0500 Received: from mail-la0-f48.google.com ([209.85.215.48]:36530) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W0Bxx-0004fT-BP for qemu-devel@nongnu.org; Mon, 06 Jan 2014 10:19:45 -0500 Received: by mail-la0-f48.google.com with SMTP id n7so9943063lam.35 for ; Mon, 06 Jan 2014 07:19:44 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1386770192-19585-4-git-send-email-buserror@gmail.com> References: <1386770192-19585-1-git-send-email-buserror@gmail.com> <1386770192-19585-4-git-send-email-buserror@gmail.com> From: Peter Maydell Date: Mon, 6 Jan 2014 15:19:24 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH 03/13] mxs/imx23: Add uart driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michel Pollet Cc: QEMU Developers On 11 December 2013 13:56, Michel Pollet wrote: > Prototype driver for the mxs/imx23 uart IO block. This has no > real 'uart' functional code, apart from letting itself be > initialized by linux without generating a timeout error. > > Signed-off-by: Michel Pollet Hi; there are some minor code style/formatting errors with this patch. You can catch these by running the scripts/checkpatch.pl script on your patches. (It doesn't catch everything, and sometimes it gets confused and gives bogus results, but it's a good sanity check.) > --- > hw/char/Makefile.objs | 1 + > hw/char/mxs_uart.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 147 insertions(+) > create mode 100644 hw/char/mxs_uart.c > > diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs > index cbd6a00..8ea5670 100644 > --- a/hw/char/Makefile.objs > +++ b/hw/char/Makefile.objs > @@ -19,6 +19,7 @@ common-obj-$(CONFIG_ETRAXFS) += etraxfs_ser.o > common-obj-$(CONFIG_ISA_DEBUG) += debugcon.o > common-obj-$(CONFIG_GRLIB) += grlib_apbuart.o > common-obj-$(CONFIG_IMX) += imx_serial.o > +common-obj-$(CONFIG_MXS) += mxs_uart.o This should be a CONFIG_MXS_UART (see remark on earlier patch). > common-obj-$(CONFIG_LM32) += lm32_juart.o > common-obj-$(CONFIG_LM32) += lm32_uart.o > common-obj-$(CONFIG_MILKYMIST) += milkymist-uart.o > diff --git a/hw/char/mxs_uart.c b/hw/char/mxs_uart.c > new file mode 100644 > index 0000000..79b2582 > --- /dev/null > +++ b/hw/char/mxs_uart.c > @@ -0,0 +1,146 @@ > +/* > + * mxs_uart.c > + * > + * Copyright: Michel Pollet > + * > + * QEMU Licence This is too vague. If you mean GPLv2 please say so. > + */ > + > +/* > + * Work in progress ! Right now there's just enough so that linux driver > + * will instantiate after a probe, there is no functional code. > + */ > +#include "hw/sysbus.h" > +#include "hw/arm/mxs.h" > + > +#define D(w) w Please get rid of this. You can use a similar DPRINTF type macro as other devices do, or no debug tracing at all, as you wish. > + > +enum { > + UART_CTRL = 0x0, > + UART_CTRL1 = 0x1, > + UART_CTRL2 = 0x2, > + UART_LINECTRL = 0x3, > + UART_LINECTRL2 = 0x4, > + UART_INTR = 0x5, > + UART_APP_DATA = 0x6, > + UART_APP_STAT = 0x7, > + UART_APP_DEBUG = 0x8, > + UART_APP_VERSION = 0x9, > + UART_APP_AUTOBAUD = 0xa, > + > + UART_MAX, > +}; > +typedef struct mxs_uart_state { > + SysBusDevice busdev; > + MemoryRegion iomem; > + > + uint32_t r[UART_MAX]; > + > + struct { > + uint16_t b[16]; > + int w, r; > + } fifo[2]; > + qemu_irq irq; > + CharDriverState *chr; > +} mxs_uart_state; Structured type names should be in CamelCase; see CODING_STYLE. > +static uint64_t mxs_uart_read( > + void *opaque, hwaddr offset, unsigned size) > +{ > + mxs_uart_state *s = (mxs_uart_state *) opaque; > + uint32_t res = 0; > + > + D(printf("%s %04x (%d) = ", __func__, (int)offset, size);) > + switch (offset >> 4) { > + case 0 ... UART_MAX: This indent is wrong, as checkpatch.pl will tell you. > + res = s->r[offset >> 4]; > + break; > + default: > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: bad offset 0x%x\n", __func__, (int) offset); > + break; > + } > + D(printf("%08x\n", res);) > + > + return res; > +} > + > +static void mxs_uart_write(void *opaque, hwaddr offset, > + uint64_t value, unsigned size) > +{ > + mxs_uart_state *s = (mxs_uart_state *) opaque; > + uint32_t oldvalue = 0; > + > + D(printf("%s %04x %08x(%d)\n", __func__, (int)offset, (int)value, size);) > + switch (offset >> 4) { > + case 0 ... UART_MAX: > + mxs_write(&s->r[offset >> 4], offset, value, size); > + break; > + default: > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: bad offset 0x%x\n", __func__, (int) offset); > + break; > + } > + switch (offset >> 4) { > + case UART_CTRL: > + if ((oldvalue ^ s->r[UART_CTRL]) == 0x80000000 > + && !(oldvalue & 0x80000000)) { > + printf("%s reseting, anding clockgate\n", __func__); Stray debug printout. > + s->r[UART_CTRL] |= 0x40000000; > + } > + break; > + } > +} > + > +static void mxs_uart_set_irq(void *opaque, int irq, int level) > +{ > +// mxs_uart_state *s = (mxs_uart_state *)opaque; Don't leave commented out code in your patches, please. > + printf("%s %3d = %d\n", __func__, irq, level); > +} > + > +static const MemoryRegionOps mxs_uart_ops = { > + .read = mxs_uart_read, > + .write = mxs_uart_write, > + .endianness = DEVICE_NATIVE_ENDIAN, > +}; > + > + > +static int mxs_uart_init(SysBusDevice *dev) > +{ > + mxs_uart_state *s = OBJECT_CHECK(mxs_uart_state, dev, "mxs_uart"); > + DeviceState *qdev = DEVICE(dev); > + > + qdev_init_gpio_in(qdev, mxs_uart_set_irq, 32 * 3); Why has a UART got so many inbound GPIO signals? At minimum, there should be a comment here describing what they are. > + sysbus_init_irq(dev, &s->irq); > + memory_region_init_io(&s->iomem, OBJECT(s), &mxs_uart_ops, s, "mxs_uart", 0x2000); > + sysbus_init_mmio(dev, &s->iomem); > + > + s->r[UART_CTRL] = 0xc0030000; > + s->r[UART_CTRL2] = 0x00220180; > + s->r[UART_APP_STAT] = 0x89f00000; > + s->r[UART_APP_VERSION] = 0x03000000; Don't do reset here, do it in a reset function (which you set up by setting the DeviceClass reset function pointer in the class init function). Reset is called for you after init, so you don't need to do any reset in init. > + return 0; > +} > + > + > +static void mxs_uart_class_init(ObjectClass *klass, void *data) > +{ > + SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass); > + > + sdc->init = mxs_uart_init; You need a vmstate structure to support save/load and VM migration, and then to set the DeviceClass vmsd field to point to it here. > +} > + > +static TypeInfo uart_info = { > + .name = "mxs_uart", > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(mxs_uart_state), > + .class_init = mxs_uart_class_init, > +}; > + > +static void mxs_uart_register(void) > +{ > + type_register_static(&uart_info); > +} > + > +type_init(mxs_uart_register) > + thanks -- PMM