Re: [Qemu-devel] [PATCH 03/13] mxs/imx23: Add uart driver

[Peter Maydell]

On 11 December 2013 13:56, Michel Pollet <address@hidden> 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 <address@hidden>

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 <address@hidden>
> + *
> + * 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