Re: [Qemu-devel] [PATCH v8 13/24] hw/arm: add FTI2C010 I2C controller support

[Peter Crosthwaite]

On Fri, Mar 15, 2013 at 11:15 PM, Kuo-Jung Su <address@hidden> wrote:
> From: Kuo-Jung Su <address@hidden>
>
> The FTI2C010 is a simple I2C master controller.
>
> Signed-off-by: Kuo-Jung Su <address@hidden>
> ---
>  hw/arm/Makefile.objs      |    1 +
>  hw/arm/faraday_a369_soc.c |    6 ++
>  hw/arm/fti2c010.c         |  212 
> +++++++++++++++++++++++++++++++++++++++++++++
>  hw/arm/fti2c010.h         |   71 +++++++++++++++
>  4 files changed, 290 insertions(+)
>  create mode 100644 hw/arm/fti2c010.c
>  create mode 100644 hw/arm/fti2c010.h
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index 2622f3f..63ba519 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -46,3 +46,4 @@ obj-y += ftrtc011.o
>  obj-y += ftdmac020.o
>  obj-y += ftapbbrg020.o
>  obj-y += ftnandc021.o
> +obj-y += fti2c010.o
> diff --git a/hw/arm/faraday_a369_soc.c b/hw/arm/faraday_a369_soc.c
> index 0088915..89610d9 100644
> --- a/hw/arm/faraday_a369_soc.c
> +++ b/hw/arm/faraday_a369_soc.c
> @@ -241,6 +241,12 @@ a369soc_device_init(FaradaySoCState *s)
>      req = qdev_get_gpio_in(s->hdma[0], 15);
>      qdev_connect_gpio_out(s->hdma[0], 15, ack);
>      qdev_connect_gpio_out(ds, 0, req);
> +
> +    /* fti2c010 */
> +    ds = sysbus_create_simple("fti2c010", 0x92900000, s->pic[51]);
> +    s->i2c[0] = ds;
> +    ds = sysbus_create_simple("fti2c010", 0x92A00000, s->pic[52]);
> +    s->i2c[1] = ds;
>  }
>
>  static void a369soc_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/arm/fti2c010.c b/hw/arm/fti2c010.c
> new file mode 100644
> index 0000000..95f6a9d
> --- /dev/null
> +++ b/hw/arm/fti2c010.c
> @@ -0,0 +1,212 @@
> +/*
> + * QEMU model of the FTI2C010 Controller
> + *
> + * Copyright (C) 2012 Faraday Technology
> + * Written by Dante Su <address@hidden>
> + *
> + * This file is licensed under GNU GPL v2+.
> + */
> +
> +#include "hw/sysbus.h"
> +#include "hw/i2c.h"
> +#include "sysemu/sysemu.h"
> +
> +#include "fti2c010.h"
> +
> +#define I2C_RD  1
> +#define I2C_WR  0
> +
> +#define TYPE_FTI2C010   "fti2c010"
> +
> +typedef struct Fti2c010State {
> +    SysBusDevice busdev;
> +    MemoryRegion mmio;
> +
> +    qemu_irq irq;
> +    i2c_bus *bus;
> +
> +    uint8_t  recv;    /* I2C RD = 1; I2C WR = 0 */
> +    uint8_t  addr;    /* 7-bits device address */
> +
> +    /* HW register cache */
> +    uint32_t cr;
> +    uint32_t sr;
> +    uint32_t cdr;
> +    uint32_t dr;
> +    uint32_t tgsr;
> +} Fti2c010State;
> +
> +#define FTI2C010(obj) \
> +    OBJECT_CHECK(Fti2c010State, obj, TYPE_FTI2C010)
> +
> +static void
> +fti2c010_update_irq(Fti2c010State *s)
> +{
> +    uint32_t sr = extract32(s->sr, 4, 8);
> +    uint32_t cr = extract32(s->cr, 8, 8);
> +    qemu_set_irq(s->irq, (sr & cr) ? 1 : 0);
> +}
> +
> +static uint64_t
> +fti2c010_mem_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    Fti2c010State *s = FTI2C010(opaque);
> +    uint32_t ret = 0;
> +
> +    switch (addr) {
> +    case REG_CR:
> +        return s->cr;
> +    case REG_SR:
> +        ret = s->sr | (i2c_bus_busy(s->bus) ? SR_BB : 0);
> +        s->sr &= 0xfffff00f;    /* clear RC status bits */
> +        fti2c010_update_irq(s);
> +        break;
> +    case REG_CDR:
> +        return s->cdr;
> +    case REG_DR:
> +        return s->dr;
> +    case REG_TGSR:
> +        return s->tgsr;
> +    case REG_BMR:
> +        return 0x00000003;  /* Slave mode: SCL=1, SDA=1 */
> +    case REG_REVR:
> +        return 0x00011000;  /* REV. 1.10.0 */
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "fti2c010: undefined memory address@hidden" HWADDR_PRIx "\n", 
> addr);
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +static void
> +fti2c010_mem_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> +{
> +    Fti2c010State *s = FTI2C010(opaque);
> +
> +    switch (addr) {
> +    case REG_CR:
> +        s->cr = (uint32_t)val;
> +        if (s->cr & CR_I2CRST) {
> +            s->dr = 0;
> +            s->sr = 0;
> +        } else if ((s->cr & CR_MASTER_EN) && (s->cr & CR_TBEN)) {
> +            s->sr &= ~SR_ACK;
> +            if (s->cr & CR_START) {
> +                s->recv = (s->dr & I2C_RD) ? 1 : 0;
> +                s->addr = extract32(s->dr, 1, 7);
> +                if (!i2c_start_transfer(s->bus, s->addr, s->recv)) {

This is the one and only use or s->addr AFAICT, and its used
immediately after unconditional assignment. You should drop addr
completely and just inline to save on redundant device state (the
desired infomation is in s->dr(8:1).

> +                    s->sr |= SR_DT | SR_ACK;
> +                } else {
> +                    s->sr &= ~SR_DT;
> +                }
> +            } else {
> +                if (s->recv) {
> +                    s->dr = i2c_recv(s->bus);
> +                    s->sr |= SR_DR;
> +                } else {
> +                    i2c_send(s->bus, (uint8_t)s->dr);
> +                    s->sr |= SR_DT;
> +                }
> +                if (s->cr & CR_NACK) {
> +                    i2c_nack(s->bus);
> +                }
> +                s->sr |= SR_ACK;
> +                if (s->cr & CR_STOP) {
> +                    i2c_end_transfer(s->bus);
> +                }
> +            }
> +        }
> +        s->cr &= ~(CR_TBEN | CR_I2CRST);
> +        fti2c010_update_irq(s);
> +        break;
> +    case REG_CDR:
> +        s->cdr = (uint32_t)val & 0x3ffff;
> +        break;
> +    case REG_DR:
> +        s->dr  = (uint32_t)val & 0xff;
> +        break;
> +    case REG_TGSR:
> +        s->tgsr = (uint32_t)val & 0x1fff;
> +        break;
> +        /* slave mode is useless to QEMU, ignore it. */
> +    case REG_SAR:

may be worth a qemu_log_mask(LOG_UNIMP, same on attempt to set to slave mode.

> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "fti2c010: undefined memory address@hidden" HWADDR_PRIx "\n", 
> addr);
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps mmio_ops = {
> +    .read  = fti2c010_mem_read,
> +    .write = fti2c010_mem_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4
> +    }
> +};
> +
> +static void fti2c010_reset(DeviceState *ds)
> +{
> +    Fti2c010State *s = FTI2C010(SYS_BUS_DEVICE(ds));
> +
> +    s->cr   = 0;
> +    s->sr   = 0;
> +    s->cdr  = 0;
> +    s->tgsr = TGSR_TSR(1) | TGSR_GSR(1);
> +
> +    qemu_set_irq(s->irq, 0);
> +}
> +
> +static void fti2c010_realize(DeviceState *dev, Error **errp)
> +{
> +    Fti2c010State *s = FTI2C010(dev);
> +
> +    s->bus = i2c_init_bus(&s->busdev.qdev, "i2c");
> +
> +    memory_region_init_io(&s->mmio, &mmio_ops, s, TYPE_FTI2C010, 0x1000);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
> +    sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);

Last I read of the QOM conventions guide, you should only cast to
OBJECT() inline not SYS_BUS_DEVICE and friends.

http://wiki.qemu.org/QOMConventions

> +}
> +
> +static const VMStateDescription vmstate_fti2c010 = {
> +    .name = TYPE_FTI2C010,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(cr, Fti2c010State),
> +        VMSTATE_UINT32(sr, Fti2c010State),
> +        VMSTATE_UINT32(cdr, Fti2c010State),
> +        VMSTATE_UINT32(dr, Fti2c010State),
> +        VMSTATE_UINT32(tgsr, Fti2c010State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void fti2c010_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->vmsd    = &vmstate_fti2c010;
> +    dc->reset   = fti2c010_reset;
> +    dc->realize = fti2c010_realize;
> +    dc->no_user = 1;
> +}
> +
> +static const TypeInfo fti2c010_info = {
> +    .name           = TYPE_FTI2C010,
> +    .parent         = TYPE_SYS_BUS_DEVICE,
> +    .instance_size  = sizeof(Fti2c010State),
> +    .class_init     = fti2c010_class_init,
> +};
> +
> +static void fti2c010_register_types(void)
> +{
> +    type_register_static(&fti2c010_info);
> +}
> +
> +type_init(fti2c010_register_types)
> diff --git a/hw/arm/fti2c010.h b/hw/arm/fti2c010.h
> new file mode 100644
> index 0000000..2c4419b
> --- /dev/null
> +++ b/hw/arm/fti2c010.h
> @@ -0,0 +1,71 @@
> +/*
> + * QEMU model of the FTI2C010 Controller
> + *
> + * Copyright (C) 2012 Faraday Technology
> + * Written by Dante Su <address@hidden>
> + *
> + * This file is licensed under GNU GPL v2+.
> + */
> +
> +#ifndef HW_ARM_FTI2C010_H
> +#define HW_ARM_FTI2C010_H
> +
> +#include "qemu/bitops.h"
> +

Why? Let implementation files inlcude bitops themselves if they need it.

> +/*
> + * FTI2C010 registers
> + */
> +#define REG_CR              0x00    /* control register */
> +#define REG_SR              0x04    /* status register */
> +#define REG_CDR             0x08    /* clock division register */
> +#define REG_DR              0x0C    /* data register */
> +#define REG_SAR             0x10    /* slave address register */
> +#define REG_TGSR            0x14    /* time & glitch suppression register */
> +#define REG_BMR             0x18    /* bus monitor register */
> +#define REG_REVR            0x30    /* revision register */
> +
> +/*
> + * REG_CR
> + */
> +#define CR_STARTIEN         0x4000  /* start condition */
> +#define CR_ALIEN            0x2000  /* Arbitration lose */
> +#define CR_SAMIEN           0x1000  /* slave address match */
> +#define CR_STOPIEN          0x800   /* stop condition */
> +#define CR_BERRIEN          0x400   /* non ACK response */
> +#define CR_DRIEN            0x200   /* data receive */
> +#define CR_DTIEN            0x100   /* data transmit */
> +#define CR_TBEN             0x80    /* transfer byte enable */
> +#define CR_NACK             0x40
> +#define CR_STOP             0x20    /* stop */
> +#define CR_START            0x10    /* start */
> +#define CR_GCEN             0x8     /* general call */
> +#define CR_SCLEN            0x4     /* enable clock */
> +#define CR_I2CEN            0x2     /* enable I2C */
> +#define CR_I2CRST           0x1     /* reset I2C */
> +#define CR_MASTER_INTR      (CR_ALIEN | CR_BERRIEN | CR_DRIEN | CR_DTIEN)
> +#define CR_MASTER_EN        (CR_SCLEN | CR_I2CEN)
> +#define CR_MASTER_MODE      (CR_MASTER_INTR | CR_MASTER_EN)
> +
> +/*
> + * REG_SR
> + */
> +#define SR_START            0x800
> +#define SR_AL               0x400
> +#define SR_GC               0x200
> +#define SR_SAM              0x100
> +#define SR_STOP             0x80
> +#define SR_BERR             0x40
> +#define SR_DR               0x20    /* received one new data byte */
> +#define SR_DT               0x10    /* trandmitted one data byte */
> +#define SR_BB               0x8     /* set when i2c bus is busy */
> +#define SR_I2CB             0x4     /* set when fti2c010 is busy */
> +#define SR_ACK              0x2
> +#define SR_RW               0x1
> +
> +/*
> + * REG_TGSR
> + */
> +#define TGSR_TSR(x)         ((x) & 0x3f)            /* setup/hold time */
> +#define TGSR_GSR(x)         (((x) & 0x07) << 10)    /* glitch suppression */
> +
> +#endif
> --
> 1.7.9.5
>
>