[03/13] mxs/imx23: Add uart driver

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 <buserror@gmail.com>
---
 hw/char/Makefile.objs |   1 +
 hw/char/mxs_uart.c    | 146 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 147 insertions(+)
 create mode 100644 hw/char/mxs_uart.c

On 11 December 2013 13:56, Michel Pollet <buserror@gmail.com> 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 <buserror@gmail.com>

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 <buserror@gmail.com>
> + *
> + * 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

On Tue, Jan 7, 2014 at 1:19 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 11 December 2013 13:56, Michel Pollet <buserror@gmail.com> 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 <buserror@gmail.com>
>
> 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 <buserror@gmail.com>
>> + *
>> + * 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.
>

To clarify further, make DPRINTF use a regular c-code if, rather than
conditional compilation. The reason being your debug printfery should
always be compile tested.

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

Check QOM conventions (I commented in other patch - see for details).

>> +
>> +    uint32_t r[UART_MAX];
>> +
>> +    struct {
>> +        uint16_t b[16];
>> +        int w, r;
>> +    } fifo[2];
>> +    qemu_irq irq;
>> +    CharDriverState *chr;

Dead variable. Just add it along with your functionality. Although the
functionality would help a lot. Its a bit of a trap, advertisiting a
UART which is just a NOP. Some qemu_log_mask(LOG_UNIMP, at various
places migt be in order, although depending on complexity it may not
be much harder to get basic txrx going.

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

Stray blank line at EOF.

Regards,
Peter

>
> thanks
> -- PMM
>