qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH V2 1/4] imx.31 and KZM board support: UART suppo


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH V2 1/4] imx.31 and KZM board support: UART support
Date: Thu, 24 Nov 2011 18:53:32 +0000

On 22 November 2011 04:32, Peter Chubb <address@hidden> wrote:
>
> 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 <address@hidden>
> Signed-off-by: Adam Clench <address@hidden>
> Signed-off-by: Peter Chubb <address@hidden>
> ---
>  hw/imx_serial.c |  307 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 307 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-22 14:47:09.242035743 +1100
> @@ -0,0 +1,307 @@
> +/*
> + * IMX31 UARTS
> + *
> + * Copyright (c) 2008 OKL
> + * Written by Hans
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.

Really v2 only? (you will need an ack from the copyright holder to say
'2 or later' here, though).

> + * 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

I assume readbuff is currently standing in as a single word fifo?
Might be worth a comment. (You'll break save/restore compatibility
when you add the fifos properly but I don't think we need to worry
about that.)

> + *  -- 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
> +
> +/*
> + * Print a message at most ten times.
> + */
> +#define scream(fmt, args...) \
> +    do { \
> +        static int printable = 10;\
> +        if (printable--) { \
> +            fprintf(stderr, fmt, ##args); \
> +        } \
> +    } while (0)

Drop this.

> +
> +
> +typedef struct {
> +    SysBusDevice busdev;
> +    MemoryRegion iomem;
> +    int32_t readbuff;
> +
> +    uint32_t usr1;
> +    uint32_t usr2;
> +    uint32_t ucr1;
> +    uint32_t uts1;
> +
> +    uint32_t 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 []) {

checkpatch complains about this space before the [].

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

This list seems to be missing readbuff.

> +    },
> +};
> +
> +
> +#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_RDR        (1<<0)    /* Receive data ready */
> +#define USR2_TXDC       (1<<3)    /* Transmission complete */
> +
> +#define UCR1_UARTEN     (1<<0)
> +#define UCR1_RRDYEN     (1<<9)
> +#define UCR1_TRDYEN     (1<<13)
> +#define UCR1_TXMPTYEN   (1<<6)

Is there any significance to the order of these?

> +#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;
> +    }
> +
> +    qemu_set_irq(s->irq, !!flags);
> +}
> +
> +static void imx_serial_reset(imx_state *s)
> +{
> +    s->usr1 = USR1_TRDY;
> +    s->usr2 = USR2_TXFE | USR2_TXDC;
> +    s->uts1 = UTS1_RXEMPTY;
> +    s->ubrm = 0;
> +    s->ubrc = 0;
> +    s->readbuff = 0;
> +}
> +
> +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;
> +        s->usr1 &= ~USR1_RRDY;
> +        s->usr2 &= ~USR2_RDR;
> +        s->uts1 |= UTS1_RXEMPTY;
> +        imx_update(s);
> +        qemu_chr_accept_input(s->chr);
> +        return c | URXD_CHARRDY;
> +
> +    case 0x20: /* UCR1 */
> +        return s->ucr1;
> +
> +    case 0x21: /* UCR2 */
> +        return 1; /* reset complete */
> +
> +    case 0x25: /* USR1 */
> +        imx_update(s);
> +        return s->usr1;

Why do we need to call imx_update() in the read function?
Surely it should be called only where we update the registers
which affect interrupt status?

> +
> +    case 0x26: /* USR2 */
> +        imx_update(s);
> +        return s->usr2;
> +
> +

Stray blank line. (There are a few of these in the file.)

> +    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:
> +        scream("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;
> +        DPRINTF("write(ucr1=%x)\n", (unsigned int)value);
> +        imx_update(s);
> +        break;
> +
> +    case 0x21: /* UCR2 */
> +        if (!(value & 1)) {
> +            imx_serial_reset(s);
> +        }
> +        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);
> +        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;
> +        break;
> +
> +    case 0x2a: /* UBRM */
> +        s->ubrm = value;
> +        break;
> +
> +    case 0x2d: /* UTS1 */
> +    case 0x22: /* UCR3 */
> +    case 0x23: /* UCR4 */
> +    case 0x24: /* UFCR */
> +    case 0x25: /* USR1 */
> +    case 0x2c: /* BIPR1 */
> +        /* TODO */
> +        break;
> +
> +    default:
> +        scream("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);
> +    imx_serial_reset(s);

If you put imx_serial_reset in a SysBusDeviceInfo struct as the
.qdev.reset pointer you don't need to call it from your init function.

> +    /*
> +     * enable the uart on boot, so messages from the linux decompresser
> +     * are visible
> +     */
> +    s->ucr1 = UCR1_UARTEN;

Is this matching the hardware's behaviour on cold boot vs warm
boot, or is it just a random hack?

> +    if (s->chr) {
> +        qemu_chr_add_handlers(s->chr, imx_can_receive, imx_receive,
> +                              imx_event, s);
> +    }
> +    vmstate_register(&dev->qdev, -1, &vmstate_imx_serial, s);

Put this in SysBusDeviceInfo .qdev.vmsd, don't call vmstate_register().

> +    return 0;
> +}
> +
> +
> +static void imx_serial_register_devices(void)
> +{
> +    DPRINTF("imx_serial_register_devices\n");
> +    sysbus_register_dev("imx_serial", sizeof(imx_state),
> +                        imx_serial_init);

Use sysbus_register_withprop() so you can pass it a SysBusDeviceInfo.

-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]