[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.

Signed-off-by: Hans Jang <hsjang@ok-labs.com>
Signed-off-by: Adam Clench <adamc@ok-labs.com>
Signed-off-by: Peter Chubb <peter.chubb@nicta.com.au>
---
 Makefile.target |    1 
 hw/imx_serial.c |  320 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 321 insertions(+)
 create mode 100644 hw/imx_serial.c

On 30 November 2011 03:36, Peter Chubb <peter.chubb@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 <hsjang@ok-labs.com>
> Signed-off-by: Adam Clench <adamc@ok-labs.com>
> Signed-off-by: Peter Chubb <peter.chubb@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

On 1 December 2011 16:55, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 November 2011 03:36, Peter Chubb <peter.chubb@nicta.com.au> wrote:
>> Signed-off-by: Hans Jang <hsjang@ok-labs.com>

Is this email address correct? Trying to send this email got me:
 550 550 <hsjang@ok-labs.com>... User not known (state 17).

>> Signed-off-by: Adam Clench <adamc@ok-labs.com>
>> Signed-off-by: Peter Chubb <peter.chubb@nicta.com.au>

-- PMM

>>>>> "Peter" == Peter Maydell <peter.maydell@linaro.org> writes:

Peter> On 1 December 2011 16:55, Peter Maydell
Peter> <peter.maydell@linaro.org> wrote:
>> On 30 November 2011 03:36, Peter Chubb <peter.chubb@nicta.com.au>
>> wrote:
>>> Signed-off-by: Hans Jang <hsjang@ok-labs.com>

Peter> Is this email address correct? Trying to send this email got
Peter> me: 550 550 <hsjang@ok-labs.com>... User not known (state 17).

He's left the company and noone knows where he is :-(

Peter> -- PMM


--
Dr Peter Chubb                                  peter DOT chubb AT nicta.com.au
http://www.ertos.nicta.com.au               ERTOS within National ICT Australia
All things shall perish from under the sky/Music alone shall live, never to die

>>>>> "Peter" == Peter Maydell <peter.maydell@linaro.org> writes:

Peter> On 30 November 2011 03:36, Peter Chubb
Peter> <peter.chubb@nicta.com.au> wrote: Commit messages should be
Peter> formatted with a short summary line, then a blank line, then a
Peter> more detailed description. You've put everything into one
Peter> extremely long summary line here, which looks odd in git
Peter> log. Try:

Thanks I've fixed that now.

>> top-level directory.

Peter> Still GPL v2 only.

Fixed.


>> +    uint32_t ubrm;

Peter> The MCIMX31RM calls this UBMR, not UBRM...

Fixed.


Peter> My copy of the MCIMX31RM says we also set RXDS on reset.

Fixed.

>> +    s->usr2 = USR2_TXFE | USR2_TXDC;

Peter> Presumably we don't set DCDIN here because we haven't
Peter> implemented the modem control signals yet?

Exactly.  But there's no harm in setting it, so I've added it in.

>> +    s->uts1 = UTS1_RXEMPTY | UTS1_TXEMPTY; +    s->ubrm = 0; +  
>>  s->ubrc = 0;

Peter> RM says the reset value for UBRC is 0x4.

OK.

Peter> You also need to reset s->ucr1. (If you want to retain that
Peter> hack in the init function then you still need to reset the
Peter> other bits even if you don't allow reset to clear UARTEN.)

OK.  Fixed.

>> +    s->readbuff = 0; 
>> +    imx_update(s);

Peter> This will call qemu_set_irq() which is a bad idea in a reset
Peter> function.  Don't call imx_update() here, instead call it after
Peter> calling imx_serial_reset() from your register write function.

Does the infrastructure guarantee that any interrupt will be cleared
when the reset function is called?

>> +   case  0x21: /* UCR2 */ 
>> +        return 1; /* reset complete */

Peter> UCR2_SRST.

Fixed.


>>    case 0x20: /* UCR1 */ 
>> +        s->ucr1 = value;

Peter> RM says the top 16 bits of UCR1 are read-only.

Fixed.

Peter> This doesn't implement writing to any of the other bits of
Peter> UCR2.

No, apart from the TXEN and RXEN bits they're all to do with 

>> +    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);

Peter> define and use USR2_FOO named constants.

Done.


>> UCR3 */ +    case 0x23: /* UCR4 */ +    case 0x24: /* UFCR */ +  
>>  case 0x25: /* USR1 */ +    case 0x2c: /* BIPR1 */ +        /* TODO
>> */

Peter> No IPRINTF() ?

This one gets hit too often.

>> +    .qdev.size  = sizeof (imx_state),

Peter> Unnecessary space (and checkpatch will complain).

I disagree -- there should be a space between the sizeof operator and
the cast that is its operand.  sizeof is not a function; and the
parentheses in this case are part of its operand.

I couldn't find any mention of sizeof in the Coding Style
document. However, I'll go with whatever makes you happier.... (but
note that the current qemu source mixes sizeof (type) and sizeof(type)
all over the place).

Look out for a new patch series real soon now...

Peter C
--
Dr Peter Chubb  http://www.gelato.unsw.edu.au  peterc AT gelato.unsw.edu.au
http://www.ertos.nicta.com.au           ERTOS within National ICT Australia