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