Re: [Qemu-devel] [PATCH V3 1/4] 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.

[Peter Maydell]

On 30 November 2011 03:36, Peter Chubb <address@hidden> 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 <address@hidden>
> Signed-off-by: Adam Clench <address@hidden>
> Signed-off-by: Peter Chubb <address@hidden>
> ---
>  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