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

[Peter Crosthwaite]

On Tue, Jan 7, 2014 at 1:19 AM, Peter Maydell <address@hidden> wrote:
> 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.
>

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
>