On 30 November 2011 03:36, Peter Chubb <peter.ch...@nicta.com.au> wrote:
Commit messages should be formatted with a short summary line,
then a blank line, then a more detailed description. You've
put everything into one extremely long summary line here, which
looks odd in git log. Try:
===begin===
hw/imx_serial: Implement the FreeScale i.MX UART
Implement the FreeScale i.MX UART. This uart is used in a variety
of SoCs, including some by Motorola, as well as in the FreeScale
i.MX series.
Signed-off-by: &c.
===endit===
Similarly for the other patches in this series.
> Signed-off-by: Hans Jang <hsj...@ok-labs.com>
> Signed-off-by: Adam Clench <ad...@ok-labs.com>
> Signed-off-by: Peter Chubb <peter.ch...@nicta.com.au>
> ---
> Makefile.target | 1
> hw/imx_serial.c | 320
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 321 insertions(+)
> create mode 100644 hw/imx_serial.c
>
> Index: qemu-working/hw/imx_serial.c
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ qemu-working/hw/imx_serial.c 2011-11-30 13:38:24.434778115 +1100
> @@ -0,0 +1,320 @@
> +/*
> + * IMX31 UARTS
> + *
> + * Copyright (c) 2008 OKL
> + * Originally Written by Hans Jiang
> + * Copyright (c) 2011 NICTA Pty Ltd.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
Still GPL v2 only.
> + *
> + * This is a `bare-bones' implementation of the IMX series serial ports.
> + * TODO:
> + * -- implement FIFOs. The real hardware has 32 word transmit
> + * and receive FIFOs; we currently use a 1-char buffer
> + * -- implement DMA
> + * -- implement BAUD-rate and modem lines, for when the backend
> + * is a real serial device.
> + */
> +
> +#include "hw.h"
> +#include "sysbus.h"
> +#include "qemu-char.h"
> +
> +//#define DEBUG_SERIAL 1
> +
> +#ifdef DEBUG_SERIAL
> +#define DPRINTF(fmt, args...) \
> +do { printf("imx_serial: " fmt , ##args); } while (0)
> +#else
> +#define DPRINTF(fmt, args...) do {} while (0)
> +#endif
> +
> +/*
> + * Define to 1 for messages about attempts to
> + * access unimplemented registers or similar.
> + */
> +#define DEBUG_IMPLEMENTATION 1
> +#if DEBUG_IMPLEMENTATION
> +# define IPRINTF(fmt, args...) \
> + do { fprintf(stderr, "imx_serial: " fmt, ##args); } while (0)
> +#else
> +# define IPRINTF(fmt, args...) do {} while (0)
> +#endif
> +
> +typedef struct {
> + SysBusDevice busdev;
> + MemoryRegion iomem;
> + int32_t readbuff;
> +
> + uint32_t usr1;
> + uint32_t usr2;
> + uint32_t ucr1;
> + uint32_t uts1;
> +
> + uint32_t ubrm;
The MCIMX31RM calls this UBMR, not UBRM...
> + uint32_t ubrc;
> +
> + qemu_irq irq;
> + CharDriverState *chr;
> +} imx_state;
> +
> +static const VMStateDescription vmstate_imx_serial = {
> + .name = "imx-serial",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .minimum_version_id_old = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_INT32(readbuff, imx_state),
> + VMSTATE_UINT32(usr1, imx_state),
> + VMSTATE_UINT32(usr2, imx_state),
> + VMSTATE_UINT32(ucr1, imx_state),
> + VMSTATE_UINT32(uts1, imx_state),
> + VMSTATE_UINT32(ubrm, imx_state),
> + VMSTATE_UINT32(ubrc, imx_state),
> + VMSTATE_END_OF_LIST()
> + },
> +};
> +
> +
> +#define URXD_CHARRDY (1<<15) /* character read is valid */
> +
> +#define USR1_TRDY (1<<13) /* Xmitter ready */
> +#define USR1_RRDY (1<<9) /* receiver ready */
> +
> +#define USR2_TXFE (1<<14) /* Transmit FIFO empty */
> +#define USR2_TXDC (1<<3) /* Transmission complete */
> +#define USR2_RDR (1<<0) /* Receive data ready */
> +
> +#define UCR1_TRDYEN (1<<13)
> +#define UCR1_RRDYEN (1<<9)
> +#define UCR1_TXMPTYEN (1<<6)
> +#define UCR1_UARTEN (1<<0)
> +
> +#define UTS1_TXEMPTY (1<<6)
> +#define UTS1_RXEMPTY (1<<5)
> +#define UTS1_TXFULL (1<<4)
> +#define UTS1_RXFULL (1<<3)
> +
> +static void imx_update(imx_state *s)
> +{
> + uint32_t flags;
> +
> + flags = ((s->usr1 & s->ucr1)) & (USR1_TRDY|USR1_RRDY);
> + if (!(s->ucr1 & UCR1_TXMPTYEN)) {
> + flags &= ~USR1_TRDY;
> + }
> +
> + if (flags) {
> + DPRINTF("imx_serial: raising interrupt\n");
> + }
> +
> + qemu_set_irq(s->irq, !!flags);
> +}
> +
> +static void imx_serial_reset(DeviceState *dev)
> +{
> + imx_state *s = container_of(dev, imx_state, busdev.qdev);
> +
> + s->usr1 = USR1_TRDY;
My copy of the MCIMX31RM says we also set RXDS on reset.
> + s->usr2 = USR2_TXFE | USR2_TXDC;
Presumably we don't set DCDIN here because we haven't implemented
the modem control signals yet?
> + s->uts1 = UTS1_RXEMPTY | UTS1_TXEMPTY;
> + s->ubrm = 0;
> + s->ubrc = 0;
RM says the reset value for UBRC is 0x4.
You also need to reset s->ucr1. (If you want to retain that
hack in the init function then you still need to reset the other
bits even if you don't allow reset to clear UARTEN.)
> + s->readbuff = 0;
> +
> + imx_update(s);
This will call qemu_set_irq() which is a bad idea in a reset function.
Don't call imx_update() here, instead call it after calling imx_serial_reset()
from your register write function.
> +}
> +
> +static uint64_t imx_serial_read(void *opaque, target_phys_addr_t offset,
> + unsigned size)
> +{
> + imx_state *s = (imx_state *)opaque;
> + uint32_t c;
> +
> + DPRINTF("read(offset=%x)\n", offset >> 2);
> + switch (offset >> 2) {
> + case 0x0: /* URXD */
> + c = s->readbuff;
> + if (!(s->uts1 & UTS1_RXEMPTY)) {
> + /* Character is valid */
> + c |= URXD_CHARRDY;
> + s->usr1 &= ~USR1_RRDY;
> + s->usr2 &= ~USR2_RDR;
> + s->uts1 |= UTS1_RXEMPTY;
> + imx_update(s);
> + qemu_chr_accept_input(s->chr);
> + }
> + return c;
> +
> + case 0x20: /* UCR1 */
> + return s->ucr1;
> +
> + case 0x21: /* UCR2 */
> + return 1; /* reset complete */
UCR2_SRST.
> +
> + case 0x25: /* USR1 */
> + return s->usr1;
> +
> + case 0x26: /* USR2 */
> + return s->usr2;
> +
> + case 0x2A: /* BRM Modulator */
> + return s->ubrm;
> +
> + case 0x2B: /* Baud Rate Count */
> + return s->ubrc;
> +
> + case 0x2d: /* UTS1 */
> + return s->uts1;
> +
> + case 0x22: /* UCR3 */
> + case 0x23: /* UCR4 */
> + case 0x24: /* UFCR */
> + case 0x29: /* BRM Incremental */
> + return 0x0; /* TODO */
> +
> + default:
> + IPRINTF("imx_serial_read: bad offset: 0x%x\n", (int)offset);
> + return 0;
> + }
> +}
> +
> +static void imx_serial_write(void *opaque, target_phys_addr_t offset,
> + uint64_t value, unsigned size)
> +{
> + imx_state *s = (imx_state *)opaque;
> + unsigned char ch;
> +
> + DPRINTF("write(offset=%x, value = %x)\n", offset >> 2, (unsigned
> int)value);
> + switch (offset >> 2) {
> + case 0x10: /* UTXD */
> + ch = value;
> + if (s->chr) {
> + qemu_chr_fe_write(s->chr, &ch, 1);
> + }
> + s->usr1 &= ~USR1_TRDY;
> + imx_update(s);
> + s->usr1 |= USR1_TRDY;
> + imx_update(s);
> + break;
> +
> + case 0x20: /* UCR1 */
> + s->ucr1 = value;
RM says the top 16 bits of UCR1 are read-only.
> + DPRINTF("write(ucr1=%x)\n", (unsigned int)value);
> + imx_update(s);
> + break;
> +
> + case 0x21: /* UCR2 */
> + if (!(value & 1)) {
UCR2_SRST, not 1.
> + imx_serial_reset(&s->busdev.qdev);
> + }
This doesn't implement writing to any of the other bits of
UCR2.
> + break;
> +
> + case 0x26: /* USR2 */
> + /*
> + * Writing 1 to some bits clears them; all other
> + * values are ignored
> + */
> + value &= (1<<15) | (1<<13) | (1<<12) | (1<<11) | (1<<10)|
> + (1<<8) | (1<<7) | (1<<6) | (1<<4) | (1<<2) | (1<<1);
define and use USR2_FOO named constants.
> + s->usr2 &= ~value;
> + break;
> +
> + /* Linux expects to see what it writes here. */
> + /* We don't currently alter the baud rate */
> + case 0x29: /* UBIR */
> + s->ubrc = value;
Top 16 bits shouldn't be writable.
> + break;
> +
> + case 0x2a: /* UBRM */
> + s->ubrm = value;
Ditto.
> + break;
> +
> + case 0x2d: /* UTS1 */
> + case 0x22: /* UCR3 */
> + case 0x23: /* UCR4 */
> + case 0x24: /* UFCR */
> + case 0x25: /* USR1 */
> + case 0x2c: /* BIPR1 */
> + /* TODO */
No IPRINTF() ?
> + break;
> +
> + default:
> + IPRINTF("imx_serial_write: Bad offset 0x%x\n", (int)offset);
> + }
> +}
> +
> +static int imx_can_receive(void *opaque)
> +{
> + imx_state *s = (imx_state *)opaque;
> + return !(s->usr1 & USR1_RRDY);
> +}
> +
> +static void imx_put_data(void *opaque, uint32_t value)
> +{
> + imx_state *s = (imx_state *)opaque;
> +
> + s->usr1 |= USR1_RRDY;
> + s->usr2 |= USR2_RDR;
> + s->uts1 &= ~UTS1_RXEMPTY;
> + s->readbuff = value;
> + imx_update(s);
> +}
> +
> +static void imx_receive(void *opaque, const uint8_t *buf, int size)
> +{
> + imx_put_data(opaque, *buf);
> +}
> +
> +static void imx_event(void *opaque, int event)
> +{
> + if (event == CHR_EVENT_BREAK) {
> + imx_put_data(opaque, 0x400);
> + }
> +}
> +
> +
> +static const struct MemoryRegionOps imx_serial_ops = {
> + .read = imx_serial_read,
> + .write = imx_serial_write,
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static int imx_serial_init(SysBusDevice *dev)
> +{
> + imx_state *s = FROM_SYSBUS(imx_state, dev);
> +
> + memory_region_init_io(&s->iomem, &imx_serial_ops, s, "imx-serial",
> 0x1000);
> + sysbus_init_mmio_region(dev, &s->iomem);
> + sysbus_init_irq(dev, &s->irq);
> + s->chr = qdev_init_chardev(&dev->qdev);
> + /*
> + * enable the uart on boot, so messages from the linux decompresser
> + * are visible. On real hardware this is done by the boot rom
> + * before anything else is loaded.
> + */
> + s->ucr1 = UCR1_UARTEN;
> +
> + if (s->chr) {
> + qemu_chr_add_handlers(s->chr, imx_can_receive, imx_receive,
> + imx_event, s);
> + }
> + return 0;
> +}
> +
> +static SysBusDeviceInfo imx_serial_info = {
> + .qdev.name = "imx_serial",
> + .qdev.desc = "i.MX series UART",
> + .qdev.size = sizeof (imx_state),
Unnecessary space (and checkpatch will complain).
> + .qdev.vmsd = &vmstate_imx_serial,
> + .qdev.reset = imx_serial_reset,
> + .init = imx_serial_init,
> +};
> +
> +static void imx_serial_register_devices(void)
> +{
> + sysbus_register_withprop(&imx_serial_info);
> +}
> +
> +device_init(imx_serial_register_devices)
> Index: qemu-working/Makefile.target
> ===================================================================
> --- qemu-working.orig/Makefile.target 2011-11-30 13:38:19.886754597 +1100
> +++ qemu-working/Makefile.target 2011-11-30 13:38:24.434778115 +1100
> @@ -361,20 +361,21 @@ obj-arm-y += mst_fpga.o mainstone.o
> obj-arm-y += z2.o
> obj-arm-y += musicpal.o bitbang_i2c.o marvell_88w8618_audio.o
> obj-arm-y += framebuffer.o
> obj-arm-y += syborg.o syborg_fb.o syborg_interrupt.o syborg_keyboard.o
> obj-arm-y += syborg_serial.o syborg_timer.o syborg_pointer.o syborg_rtc.o
> obj-arm-y += syborg_virtio.o
> obj-arm-y += vexpress.o
> obj-arm-y += strongarm.o
> obj-arm-y += collie.o
> obj-arm-y += pl041.o lm4549.o
> +obj-arm-y += imx_serial.o
>
> obj-sh4-y = shix.o r2d.o sh7750.o sh7750_regnames.o tc58128.o
> obj-sh4-y += sh_timer.o sh_serial.o sh_intc.o sh_pci.o sm501.o
> obj-sh4-y += ide/mmio.o
>
> obj-m68k-y = an5206.o mcf5206.o mcf_uart.o mcf_intc.o mcf5208.o mcf_fec.o
> obj-m68k-y += m68k-semi.o dummy_m68k.o
>
> obj-s390x-y = s390-virtio-bus.o s390-virtio.o
This is OK, but whatever you're generating patches with seems to have
produced an awful lot of context lines here :-)
-- PMM
