Patchwork [1/2] Add i.MX I2C device emulator.

login
register
mail settings
Submitter Jean-Christophe DUBOIS
Date May 1, 2013, 7:53 p.m.
Message ID <1367438026-27573-1-git-send-email-jcd@tribudubois.net>
Download mbox | patch
Permalink /patch/240807/
State New
Headers show

Comments

Jean-Christophe DUBOIS - May 1, 2013, 7:53 p.m.
Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
---
 default-configs/arm-softmmu.mak |   2 +
 hw/i2c/Makefile.objs            |   1 +
 hw/i2c/imx_i2c.c                | 374 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 377 insertions(+)
 create mode 100644 hw/i2c/imx_i2c.c
Peter Crosthwaite - May 2, 2013, 12:16 p.m.
Hi Jean-Christophe,

Thanks for your contribution. Please run the patch through
scripts/checkpatch.pl to check for formatting errors.

On Thu, May 2, 2013 at 5:53 AM, Jean-Christophe DUBOIS
<jcd@tribudubois.net> wrote:
> Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
> ---
>  default-configs/arm-softmmu.mak |   2 +
>  hw/i2c/Makefile.objs            |   1 +
>  hw/i2c/imx_i2c.c                | 374 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 377 insertions(+)
>  create mode 100644 hw/i2c/imx_i2c.c
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index b3a0207..a20f112 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -81,3 +81,5 @@ CONFIG_VERSATILE_PCI=y
>  CONFIG_VERSATILE_I2C=y
>
>  CONFIG_SDHCI=y
> +
> +CONFIG_IMX_I2C=y
> diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
> index 648278e..d27bbaa 100644
> --- a/hw/i2c/Makefile.objs
> +++ b/hw/i2c/Makefile.objs
> @@ -4,4 +4,5 @@ common-obj-$(CONFIG_ACPI) += smbus_ich9.o
>  common-obj-$(CONFIG_APM) += pm_smbus.o
>  common-obj-$(CONFIG_BITBANG_I2C) += bitbang_i2c.o
>  common-obj-$(CONFIG_EXYNOS4) += exynos4210_i2c.o
> +common-obj-$(CONFIG_IMX_I2C) += imx_i2c.o
>  obj-$(CONFIG_OMAP) += omap_i2c.o
> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
> new file mode 100644
> index 0000000..30d3f5c
> --- /dev/null
> +++ b/hw/i2c/imx_i2c.c
> @@ -0,0 +1,374 @@
> +/*
> + *  i.MX I2C Bus Serial Interface Emulation
> + *
> + *  Copyright (C) 2013 Jean-Christophe Dubois.
> + *
> + *  This program is free software; you can redistribute it and/or modify it
> + *  under the terms of the GNU General Public License as published by the
> + *  Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful, but WITHOUT
> + *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + *  FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + *  for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "hw/sysbus.h"
> +#include "hw/i2c/i2c.h"
> +
> +#ifndef IMX_I2C_DEBUG
> +#define IMX_I2C_DEBUG                 0
> +#endif
> +
> +#define TYPE_IMX_I2C                  "imx.i2c"
> +#define IMX_I2C(obj)                  \
> +    OBJECT_CHECK(imx_i2c_state, (obj), TYPE_IMX_I2C)
> +
> +/* i.MX I2C memory map */
> +#define IMX_I2C_MEM_SIZE           0x14
> +#define IADR_ADDR                  0x00  /* address register */
> +#define IFDR_ADDR                  0x04  /* frequency divider register */
> +#define I2CR_ADDR                  0x08  /* control register */
> +#define I2SR_ADDR                  0x0c  /* status register */
> +#define I2DR_ADDR                  0x10  /* data register */
> +
> +#define IADR_MASK                  0xFE
> +#define IADR_RESET                 0
> +
> +#define IFDR_MASK                  0x3F
> +#define IFDR_RESET                 0
> +
> +#define I2CR_IEN                   (1 << 7)
> +#define I2CR_IIEN                  (1 << 6)
> +#define I2CR_MSTA                  (1 << 5)
> +#define I2CR_MTX                   (1 << 4)
> +#define I2CR_TXAK                  (1 << 3)
> +#define I2CR_RSTA                  (1 << 2)
> +#define I2CR_MASK                  0xFC
> +#define I2CR_RESET                 0
> +
> +#define I2SR_ICF                   (1 << 7)
> +#define I2SR_IAAF                  (1 << 6)
> +#define I2SR_IBB                   (1 << 5)
> +#define I2SR_IAL                   (1 << 4)
> +#define I2SR_SRW                   (1 << 2)
> +#define I2SR_IIF                   (1 << 1)
> +#define I2SR_RXAK                  (1 << 0)
> +#define I2SR_MASK                  0xE9
> +#define I2SR_RESET                 0x81
> +
> +#define I2DR_MASK                  0xFF
> +#define I2DR_RESET                 0
> +
> +#define ADDR_RESET                 0xFF00
> +
> +#if IMX_I2C_DEBUG
> +#define DPRINT(fmt, args...)              \
> +    do { fprintf(stderr, "%s: "fmt, __func__, ## args); } while (0)
> +
> +static const char *imx_i2c_get_regname(unsigned offset)
> +{
> +    switch (offset) {
> +    case IADR_ADDR:
> +        return "IADR";
> +    case IFDR_ADDR:
> +        return "IFDR";
> +    case I2CR_ADDR:
> +        return "I2CR";
> +    case I2SR_ADDR:
> +        return "I2SR";
> +    case I2DR_ADDR:
> +        return "I2DR";
> +    default:
> +        return "[?]";
> +    }
> +}
> +
> +#else
> +#define DPRINT(fmt, args...)              do { } while (0)
> +#endif
> +
> +typedef struct imx_i2c_state {
> +    SysBusDevice busdev;
> +    MemoryRegion iomem;
> +    i2c_bus *bus;
> +    qemu_irq irq;
> +
> +    uint16_t  address;
> +
> +    uint16_t iadr;
> +    uint16_t ifdr;
> +    uint16_t i2cr;
> +    uint16_t i2sr;
> +    uint16_t i2dr_read;
> +    uint16_t i2dr_write;
> +} imx_i2c_state;
> +
> +static inline bool imx_i2c_is_enabled(imx_i2c_state *s)
> +{
> +    return (s->i2cr & I2CR_IEN);

ERROR: return is not a function, parentheses are not required
#148: FILE: hw/i2c/imx_i2c.c:114:
+    return (s->i2cr & I2CR_IEN);

From checkpatch, here and below.


> +}
> +
> +static inline bool imx_i2c_interrupt_is_enabled(imx_i2c_state *s)
> +{
> +    return (s->i2cr & I2CR_IIEN);
> +}
> +
> +static inline bool imx_i2c_is_master(imx_i2c_state *s)
> +{
> +    return (s->i2cr & I2CR_MSTA);
> +}
> +
> +static inline bool imx_i2c_direction_is_tx(imx_i2c_state *s)
> +{
> +    return (s->i2cr & I2CR_MTX);
> +}
> +
> +static void imx_i2c_reset(DeviceState *d)
> +{
> +    imx_i2c_state *s = FROM_SYSBUS(imx_i2c_state, SYS_BUS_DEVICE(d));
> +

Please don't use FROM_SYSBUS in new code. Use QOM cast macros.

http://wiki.qemu.org/QOMConventions

Has useful guidelines for the rules around this for new devices.

> +    if (s->address != ADDR_RESET) {
> +        i2c_end_transfer(s->bus);
> +    }
> +
> +    s->address    = ADDR_RESET;
> +    s->iadr       = IADR_RESET;
> +    s->ifdr       = IFDR_RESET;
> +    s->i2cr       = I2CR_RESET;
> +    s->i2sr       = I2SR_RESET;
> +    s->i2dr_read  = I2DR_RESET;
> +    s->i2dr_write = I2DR_RESET;
> +}
> +
> +static inline void imx_i2c_raise_interrupt(imx_i2c_state *s)
> +{
> +    /*
> +     * raise an interrupt if the device is enabled and it is configured
> +     * to generate some interrupts.
> +     */
> +    if (imx_i2c_is_enabled(s) && imx_i2c_interrupt_is_enabled(s)) {
> +        s->i2sr |= I2SR_IIF;
> +        qemu_irq_raise(s->irq);
> +    }
> +}
> +
> +static uint64_t imx_i2c_read(void *opaque, hwaddr offset,
> +                             unsigned size)
> +{
> +    imx_i2c_state *s = (imx_i2c_state *)opaque;

QOM cast macro.

> +    uint16_t value;
> +
> +    switch (offset) {
> +    case IADR_ADDR:
> +        value = s->iadr;
> +        break;
> +    case IFDR_ADDR:
> +        value = s->ifdr;
> +        break;
> +    case I2CR_ADDR:
> +        value = s->i2cr;
> +        break;
> +    case I2SR_ADDR:
> +        value = s->i2sr;
> +        break;
> +    case I2DR_ADDR:
> +        value = s->i2dr_read;
> +        if (imx_i2c_is_master(s)) { // master mode

Do not use C style comments. (Checkpatch picks then up)

> +            int ret;
> +
> +            if (s->address == ADDR_RESET) {
> +                // something is wrong as the address is not set
> +                DPRINT("Trying to read with specifying the slave address\n");
> +            }
> +
> +            if ((ret = i2c_recv(s->bus)) >= 0) {
> +                s->i2dr_read = ret;
> +                imx_i2c_raise_interrupt(s);
> +            } else {
> +                DPRINT("read failed for device 0x%02x\n" s->address);
> +                s->i2dr_read = 0xff;
> +            }
> +        } else { // slave mode
> +        }

Empty else not required.

> +        break;
> +    default:
> +        hw_error("%s: Bad address 0x%x\n", __func__, (int)offset);
> +        break;
> +    }
> +
> +    DPRINT("read %s [0x%02x] -> 0x%02x\n", imx_i2c_get_regname(offset),
> +           (unsigned int)offset, value);
> +
> +    return (uint64_t)value;
> +}
> +
> +static void imx_i2c_write(void *opaque, hwaddr offset,
> +                          uint64_t value, unsigned size)
> +{
> +    imx_i2c_state *s = (imx_i2c_state *)opaque;
> +    uint16_t v = value & 0xff;
> +
> +    DPRINT("write %s [0x%02x] <- 0x%02x\n", imx_i2c_get_regname(offset),
> +           (unsigned int)offset, v);
> +
> +    switch (offset) {
> +    case IADR_ADDR:
> +        s->iadr = v & IADR_MASK;
> +        //i2c_set_slave_address(s->bus, (uint8_t)s->iadr);
> +        break;
> +    case IFDR_ADDR:
> +        s->ifdr = v & IFDR_MASK;
> +        break;
> +    case I2CR_ADDR:
> +        if (imx_i2c_is_enabled(s) && ((v & I2CR_IEN) == 0)) {
> +            /* This is a soft reset. IADR is preserved during soft resets */
> +            uint16_t iadr = s->iadr;
> +            imx_i2c_reset(&s->busdev.qdev);
> +            s->iadr = iadr;
> +        } else { // normal write
> +            s->i2cr = v & I2CR_MASK;
> +
> +            if (imx_i2c_is_master(s)) { // master mode
> +                // set the bus to busy
> +                s->i2sr |= I2SR_IBB;
> +            } else { // slave mode
> +                // bus is not busy anymore
> +                s->i2sr &= ~I2SR_IBB;
> +
> +                // if we unset the master mode then it ends the ongoing
> +                // transfer if any
> +                if (s->address != ADDR_RESET) {
> +                    i2c_end_transfer(s->bus);
> +                    s->address = ADDR_RESET;
> +                }
> +            }
> +
> +            if (s->i2cr & I2CR_RSTA) { // Restart
> +                // if this is a restart then it ends the ongoing transfer
> +                if (s->address != ADDR_RESET) {
> +                    i2c_end_transfer(s->bus);
> +                    s->address = ADDR_RESET;
> +                    s->i2cr &= ~I2CR_RSTA;
> +                }
> +            }
> +        }
> +        break;
> +    case I2SR_ADDR:
> +        /*
> +         * if the user writes 0 to IIF then lower the interrupt and
> +         * reset the bit
> +         */
> +        if ((s->i2sr & I2SR_IIF) && !(v & I2SR_IIF)) {
> +            s->i2sr &= ~I2SR_IIF;
> +            qemu_irq_lower(s->irq);
> +        }
> +
> +        /*
> +         * if the user writes 0 to IAL, reset the bit
> +         */
> +        if ((s->i2sr & I2SR_IAL) && !(v & I2SR_IAL)) {
> +            s->i2sr &= ~I2SR_IAL;
> +            /* Nothing more to do ? */
> +        }
> +
> +        break;
> +    case I2DR_ADDR:
> +        /* if the device is not enabled, nothing to do */
> +        if (!imx_i2c_is_enabled(s)) {
> +            break;
> +        }
> +
> +        s->i2dr_write = v & I2DR_MASK;
> +
> +        if (imx_i2c_is_master(s)) { // master mode
> +            /* If this is the first write cycle then it is the slave addr */
> +            if (s->address == ADDR_RESET) {
> +                if (!i2c_start_transfer(s->bus, (uint8_t)s->i2dr_write >> 1,
> +                                        (int)s->i2dr_write & 0x01)) {
> +                    // if 0 is returned, the adress is valid
> +                    s->address = s->i2dr_write;
> +                    s->i2sr &= ~I2SR_RXAK;
> +                    imx_i2c_raise_interrupt(s);
> +                } else { // can't start i2c transaction
> +                }
> +            } else { // This is a normal data write
> +                if (i2c_send(s->bus, s->i2dr_write)) {
> +                    // if the target return non zero then end the transfer
> +                    i2c_end_transfer(s->bus);
> +                }
> +                s->i2sr &= ~I2SR_RXAK;
> +                imx_i2c_raise_interrupt(s);
> +            }
> +        } else { // slave mode
> +        }
> +        break;
> +    default:
> +        hw_error("%s: Bad address 0x%x\n", __func__, (int)offset);
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps imx_i2c_ops = {
> +    .read = imx_i2c_read,
> +    .write = imx_i2c_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,

I think you may need a .valid definition here as it looks like you
have restrictions on access size and alignment? (looks like 4bytes
accesses only).

> +};
> +
> +static const VMStateDescription imx_i2c_vmstate = {
> +    .name = TYPE_IMX_I2C,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT16(address, imx_i2c_state),
> +        VMSTATE_UINT16(iadr, imx_i2c_state),
> +        VMSTATE_UINT16(ifdr, imx_i2c_state),
> +        VMSTATE_UINT16(i2cr, imx_i2c_state),
> +        VMSTATE_UINT16(i2sr, imx_i2c_state),
> +        VMSTATE_UINT16(i2dr_read, imx_i2c_state),
> +        VMSTATE_UINT16(i2dr_write, imx_i2c_state),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static int imx_i2c_init(SysBusDevice *dev)
> +{

Use of the SysBusDeviceClass::init function is deprecated. Please use
DeviceClass::realise or Object::init. With no reliance on properties I
would suggest this one can be done as just an Object::init fn.

Regards,
Peter

> +    imx_i2c_state *s = IMX_I2C(dev);
> +
> +    memory_region_init_io(&s->iomem, &imx_i2c_ops, s, TYPE_IMX_I2C,
> +                          IMX_I2C_MEM_SIZE);
> +    sysbus_init_mmio(dev, &s->iomem);
> +    sysbus_init_irq(dev, &s->irq);
> +    s->bus = i2c_init_bus(&dev->qdev, "i2c");
> +
> +    return 0;
> +}
> +
> +static void imx_i2c_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *sbdc = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    dc->vmsd = &imx_i2c_vmstate;
> +    dc->reset = imx_i2c_reset;
> +    sbdc->init = imx_i2c_init;
> +}
> +
> +static const TypeInfo imx_i2c_type_info = {
> +    .name = TYPE_IMX_I2C,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(imx_i2c_state),
> +    .class_init = imx_i2c_class_init,
> +};
> +
> +static void imx_i2c_register_types(void)
> +{
> +    type_register_static(&imx_i2c_type_info);
> +}
> +
> +type_init(imx_i2c_register_types);
> --
> 1.8.1.2
>
>
Andreas Färber - May 2, 2013, 12:38 p.m.
Hi,

Am 01.05.2013 21:53, schrieb Jean-Christophe DUBOIS:
> Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
> ---
>  default-configs/arm-softmmu.mak |   2 +
>  hw/i2c/Makefile.objs            |   1 +
>  hw/i2c/imx_i2c.c                | 374 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 377 insertions(+)
>  create mode 100644 hw/i2c/imx_i2c.c

Please thread your messages together so they can be reviewed in context.

Since you're adding a new I2C device and we have a qtest framework for
I2C, please supply an implementation for this device (which will require
some constant sharing via header file) and some simple test case for the
board you're using it on, to assure it keeps working.

In addition to Peter C.'s comments:

[...]
> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
> new file mode 100644
> index 0000000..30d3f5c
> --- /dev/null
> +++ b/hw/i2c/imx_i2c.c
[...]
> +type_init(imx_i2c_register_types);

This declares a function and is not a statement, so no semicolon please.

Regards,
Andreas
Jean-Christophe DUBOIS - May 2, 2013, 6:33 p.m.
Peter,

Thanks for you review.

I have a few questions below.

JC

On 05/02/2013 02:16 PM, Peter Crosthwaite wrote:
> Hi Jean-Christophe,
>
> Thanks for your contribution. Please run the patch through
> scripts/checkpatch.pl to check for formatting errors.
>
> On Thu, May 2, 2013 at 5:53 AM, Jean-Christophe DUBOIS
> <jcd@tribudubois.net> wrote:
> ERROR: return is not a function, parentheses are not required
> #148: FILE: hw/i2c/imx_i2c.c:114:
> +    return (s->i2cr & I2CR_IEN);
>
> >From checkpatch, here and below.

Will do.

>> +}
>> +
>> +static inline bool imx_i2c_interrupt_is_enabled(imx_i2c_state *s)
>> +{
>> +    return (s->i2cr & I2CR_IIEN);
>> +}
>> +
>> +static inline bool imx_i2c_is_master(imx_i2c_state *s)
>> +{
>> +    return (s->i2cr & I2CR_MSTA);
>> +}
>> +
>> +static inline bool imx_i2c_direction_is_tx(imx_i2c_state *s)
>> +{
>> +    return (s->i2cr & I2CR_MTX);
>> +}
>> +
>> +static void imx_i2c_reset(DeviceState *d)
>> +{
>> +    imx_i2c_state *s = FROM_SYSBUS(imx_i2c_state, SYS_BUS_DEVICE(d));
>> +
> Please don't use FROM_SYSBUS in new code. Use QOM cast macros.
>
> http://wiki.qemu.org/QOMConventions
>
> Has useful guidelines for the rules around this for new devices.

It is not very clear what you do expect. Could you point me to a driver 
that is up to date.

>> +    if (s->address != ADDR_RESET) {
>> +        i2c_end_transfer(s->bus);
>> +    }
>> +
>> +    s->address    = ADDR_RESET;
>> +    s->iadr       = IADR_RESET;
>> +    s->ifdr       = IFDR_RESET;
>> +    s->i2cr       = I2CR_RESET;
>> +    s->i2sr       = I2SR_RESET;
>> +    s->i2dr_read  = I2DR_RESET;
>> +    s->i2dr_write = I2DR_RESET;
>> +}
>> +
>> +static inline void imx_i2c_raise_interrupt(imx_i2c_state *s)
>> +{
>> +    /*
>> +     * raise an interrupt if the device is enabled and it is configured
>> +     * to generate some interrupts.
>> +     */
>> +    if (imx_i2c_is_enabled(s) && imx_i2c_interrupt_is_enabled(s)) {
>> +        s->i2sr |= I2SR_IIF;
>> +        qemu_irq_raise(s->irq);
>> +    }
>> +}
>> +
>> +static uint64_t imx_i2c_read(void *opaque, hwaddr offset,
>> +                             unsigned size)
>> +{
>> +    imx_i2c_state *s = (imx_i2c_state *)opaque;
> QOM cast macro.

Could you point me to an up to date driver using QOM cast macro?

>
> +
> +static const MemoryRegionOps imx_i2c_ops = {
> +    .read = imx_i2c_read,
> +    .write = imx_i2c_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> I think you may need a .valid definition here as it looks like you
> have restrictions on access size and alignment? (looks like 4bytes
> accesses only).

I'll look into this.

>> +};
>> +
>> +static const VMStateDescription imx_i2c_vmstate = {
>> +    .name = TYPE_IMX_I2C,
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT16(address, imx_i2c_state),
>> +        VMSTATE_UINT16(iadr, imx_i2c_state),
>> +        VMSTATE_UINT16(ifdr, imx_i2c_state),
>> +        VMSTATE_UINT16(i2cr, imx_i2c_state),
>> +        VMSTATE_UINT16(i2sr, imx_i2c_state),
>> +        VMSTATE_UINT16(i2dr_read, imx_i2c_state),
>> +        VMSTATE_UINT16(i2dr_write, imx_i2c_state),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static int imx_i2c_init(SysBusDevice *dev)
>> +{
> Use of the SysBusDeviceClass::init function is deprecated. Please use
> DeviceClass::realise or Object::init. With no reliance on properties I
> would suggest this one can be done as just an Object::init fn.
Could you point me to the documentation or an up to date example?
Andreas Färber - May 3, 2013, 12:35 p.m.
Am 02.05.2013 20:33, schrieb Jean-Christophe DUBOIS:
> On 05/02/2013 02:16 PM, Peter Crosthwaite wrote:
>> On Thu, May 2, 2013 at 5:53 AM, Jean-Christophe DUBOIS
>> <jcd@tribudubois.net> wrote:
>>> +static int imx_i2c_init(SysBusDevice *dev)
>>> +{
>> Use of the SysBusDeviceClass::init function is deprecated. Please use
>> DeviceClass::realise or Object::init. With no reliance on properties I
>> would suggest this one can be done as just an Object::init fn.
> Could you point me to the documentation or an up to date example?

Documentation is in include/hw/qdev-core.h and include/qom/object.h.

Andreas
Jean-Christophe DUBOIS - May 3, 2013, 3:16 p.m.
Andreas,

On 05/02/2013 02:38 PM, Andreas Färber wrote:
> Hi,
>
> Am 01.05.2013 21:53, schrieb Jean-Christophe DUBOIS:
>> Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
>> ---
>>   default-configs/arm-softmmu.mak |   2 +
>>   hw/i2c/Makefile.objs            |   1 +
>>   hw/i2c/imx_i2c.c                | 374 ++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 377 insertions(+)
>>   create mode 100644 hw/i2c/imx_i2c.c
> Please thread your messages together so they can be reviewed in context.
>
> Since you're adding a new I2C device and we have a qtest framework for
> I2C, please supply an implementation for this device (which will require
> some constant sharing via header file) and some simple test case for the
> board you're using it on, to assure it keeps working.
What is the "target" for building qtest?

And then how to run it?

Could you point me to some documentation please? It doesn't build out of 
the box for me.

JC
Andreas Färber - May 3, 2013, 3:20 p.m.
Am 03.05.2013 17:16, schrieb Jean-Christophe DUBOIS:
> Andreas,
> 
> On 05/02/2013 02:38 PM, Andreas Färber wrote:
>> Hi,
>>
>> Am 01.05.2013 21:53, schrieb Jean-Christophe DUBOIS:
>>> Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
>>> ---
>>>   default-configs/arm-softmmu.mak |   2 +
>>>   hw/i2c/Makefile.objs            |   1 +
>>>   hw/i2c/imx_i2c.c                | 374
>>> ++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 377 insertions(+)
>>>   create mode 100644 hw/i2c/imx_i2c.c
>> Please thread your messages together so they can be reviewed in context.
>>
>> Since you're adding a new I2C device and we have a qtest framework for
>> I2C, please supply an implementation for this device (which will require
>> some constant sharing via header file) and some simple test case for the
>> board you're using it on, to assure it keeps working.
> What is the "target" for building qtest?
> 
> And then how to run it?

make check

Andreas

> Could you point me to some documentation please? It doesn't build out of
> the box for me.
> 
> JC
>
Jean-Christophe DUBOIS - May 3, 2013, 4:04 p.m.
On 05/03/2013 05:20 PM, Andreas Färber wrote:
> Am 03.05.2013 17:16, schrieb Jean-Christophe DUBOIS:
>> Andreas,
>>
>> On 05/02/2013 02:38 PM, Andreas Färber wrote:
>>> Hi,
>>>
>>> Am 01.05.2013 21:53, schrieb Jean-Christophe DUBOIS:
>>>> Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
>>>> ---
>>>>    default-configs/arm-softmmu.mak |   2 +
>>>>    hw/i2c/Makefile.objs            |   1 +
>>>>    hw/i2c/imx_i2c.c                | 374
>>>> ++++++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 377 insertions(+)
>>>>    create mode 100644 hw/i2c/imx_i2c.c
>>> Please thread your messages together so they can be reviewed in context.
>>>
>>> Since you're adding a new I2C device and we have a qtest framework for
>>> I2C, please supply an implementation for this device (which will require
>>> some constant sharing via header file) and some simple test case for the
>>> board you're using it on, to assure it keeps working.
>> What is the "target" for building qtest?
>>
>> And then how to run it?
> make check

Thanks Andreas,

So I added a libi2c-imx.c file, a ds1338-test.c file ...

but when I run "make check" I get the following.

What am I missing?

JC

$ make check
GTESTER tests/check-qdict
GTESTER tests/check-qfloat
GTESTER tests/check-qint
GTESTER tests/check-qstring
GTESTER tests/check-qlist
GTESTER tests/check-qjson
GTESTER tests/test-qmp-output-visitor
GTESTER tests/test-qmp-input-visitor
GTESTER tests/test-qmp-input-strict
GTESTER tests/test-qmp-commands
GTESTER tests/test-string-input-visitor
GTESTER tests/test-string-output-visitor
GTESTER tests/test-coroutine
GTESTER tests/test-visitor-serialization
GTESTER tests/test-iov
GTESTER tests/test-aio
GTESTER tests/test-thread-pool
GTESTER tests/test-hbitmap
GTESTER tests/test-x86-cpuid
GTESTER tests/test-xbzrle
GTESTER tests/test-cutils
GTESTER tests/test-mul64
   CC    tests/libi2c-imx.o
   LINK  tests/tmp105-test
   CC    tests/ds1338-test.o
   LINK  tests/ds1338-test
GTESTER check-qtest-arm
Kernel image must be specified
Broken pipe
make: *** [check-qtest-arm] Error 1
$

>
> Andreas
>
>> Could you point me to some documentation please? It doesn't build out of
>> the box for me.
>>
>> JC
>>
>
Andreas Färber - May 3, 2013, 4:41 p.m.
Am 03.05.2013 18:04, schrieb Jean-Christophe DUBOIS:
> So I added a libi2c-imx.c file, a ds1338-test.c file ...

(Note that in light of http://patchwork.ozlabs.org/patch/241004/ this
will likely become libqos/i2c-imx.c.)

> but when I run "make check" I get the following.
[...]
> $ make check
[...]
>   CC    tests/libi2c-imx.o
>   LINK  tests/tmp105-test
>   CC    tests/ds1338-test.o
>   LINK  tests/ds1338-test
> GTESTER check-qtest-arm
> Kernel image must be specified

Without seeing your code and since tmp105-test is working on my side, I
can only guess that your ds1338-test is starting either your own machine
or some machine that terminates with above error.

The solution is to only mandate -kernel, -initrd, -dtb, etc. in the
machine init function if !qtest_enabled() or so.

> Broken pipe

That's just qtest complaining that the process unexpectedly is gone due
to the above.

> make: *** [check-qtest-arm] Error 1

Note: For getting your test working you can skip the other tests above
using either the specific name from the error or check-qtest:
$ make check-help

Andreas
Jean-Christophe DUBOIS - May 4, 2013, 8:22 a.m.
On 05/03/2013 06:41 PM, Andreas Färber wrote:
>    CC    tests/libi2c-imx.o
>    LINK  tests/tmp105-test
>    CC    tests/ds1338-test.o
>    LINK  tests/ds1338-test
> GTESTER check-qtest-arm
> Kernel image must be specified
> Without seeing your code and since tmp105-test is working on my side, I
> can only guess that your ds1338-test is starting either your own machine
> or some machine that terminates with above error.
the ds1338-test.c code is very similar to the tmp105-test.c code.

The tmp105-test.c code starts a n800 machine while the ds1338-test.c 
starts a imx25_3ds machine (to get the i.MX I2C device, it seems natural).

Nowhere in the tmp105-test.c code is any kernel or initrd file 
specified. So I did the same with ds1338-test.c.

But then "make check" is complaining the kernel is missing ...

So it seems the n800 allows qemu to start even if no parameters (ram 
size, kernel file, ...) are provided (you can actually confirm this by 
running "qemu-system-arm -machine n800"). This is not the default 
behavior for most implemented platform (including imx25_3ds) that will 
refuse to start if no parameter is given.

Is there a new requirement mandating platforms to start with guessed 
parameters if not provided on the command line in order to be compatible 
with qtest?
>
> The solution is to only mandate -kernel, -initrd, -dtb, etc. in the
> machine init function if !qtest_enabled() or so.

I am calling arm_load_kernel() with a pointer to a "struct arm_boot_info".

The first thing done by arm_load_kernel() is to check for the kernel 
parameter without checking for qtest_enable() and all.

     /* Load the kernel.  */
     if (!info->kernel_filename) {
         fprintf(stderr, "Kernel image must be specified\n");
         exit(1);
     }

Do you mean arm_load_kernel() should test for qtest_enable() or should I 
call arm_load_kernel() only if !qtest_enable() (there is not much 
platform checking for qtest_enable() at this time and it seems somebody 
need to set the "entry" field in the info struct).
Andreas Färber - May 4, 2013, 8:27 a.m.
Am 04.05.2013 10:22, schrieb Jean-Christophe DUBOIS:
> On 05/03/2013 06:41 PM, Andreas Färber wrote:
>>    CC    tests/libi2c-imx.o
>>    LINK  tests/tmp105-test
>>    CC    tests/ds1338-test.o
>>    LINK  tests/ds1338-test
>> GTESTER check-qtest-arm
>> Kernel image must be specified
>> Without seeing your code and since tmp105-test is working on my side, I
>> can only guess that your ds1338-test is starting either your own machine
>> or some machine that terminates with above error.
> the ds1338-test.c code is very similar to the tmp105-test.c code.
> 
> The tmp105-test.c code starts a n800 machine while the ds1338-test.c
> starts a imx25_3ds machine (to get the i.MX I2C device, it seems natural).
> 
> Nowhere in the tmp105-test.c code is any kernel or initrd file
> specified. So I did the same with ds1338-test.c.

The mentioned machine init is part of the emulation, not of qtest.

> But then "make check" is complaining the kernel is missing ...
> 
> So it seems the n800 allows qemu to start even if no parameters (ram
> size, kernel file, ...) are provided (you can actually confirm this by
> running "qemu-system-arm -machine n800"). This is not the default
> behavior for most implemented platform (including imx25_3ds) that will
> refuse to start if no parameter is given.
> 
> Is there a new requirement mandating platforms to start with guessed
> parameters if not provided on the command line in order to be compatible
> with qtest?
>>
>> The solution is to only mandate -kernel, -initrd, -dtb, etc. in the
>> machine init function if !qtest_enabled() or so.
> 
> I am calling arm_load_kernel() with a pointer to a "struct arm_boot_info".
> 
> The first thing done by arm_load_kernel() is to check for the kernel
> parameter without checking for qtest_enable() and all.
> 
>     /* Load the kernel.  */
>     if (!info->kernel_filename) {
>         fprintf(stderr, "Kernel image must be specified\n");
>         exit(1);
>     }
> 
> Do you mean arm_load_kernel() should test for qtest_enable() or should I
> call arm_load_kernel() only if !qtest_enable() (there is not much
> platform checking for qtest_enable() at this time and it seems somebody
> need to set the "entry" field in the info struct).

In short, qtest does not execute any code, so it does not need to load
binaries or set CPU entry addresses.

Yes, I would say there is no point calling arm_load_kernel() if
!qtest_enabled().

Andreas
Peter Maydell - May 4, 2013, 8:29 a.m.
On 4 May 2013 09:27, Andreas Färber <afaerber@suse.de> wrote:
> Am 04.05.2013 10:22, schrieb Jean-Christophe DUBOIS:
>> Do you mean arm_load_kernel() should test for qtest_enable() or should I
>> call arm_load_kernel() only if !qtest_enable() (there is not much
>> platform checking for qtest_enable() at this time and it seems somebody
>> need to set the "entry" field in the info struct).
>
> In short, qtest does not execute any code, so it does not need to load
> binaries or set CPU entry addresses.
>
> Yes, I would say there is no point calling arm_load_kernel() if
> !qtest_enabled().

I'd rather we didn't end up with qtest_enabled() checks infecting
every board model, please...

thanks
-- PMM
Jean-Christophe DUBOIS - May 4, 2013, 8:38 a.m.
On 05/04/2013 10:29 AM, Peter Maydell wrote:
> On 4 May 2013 09:27, Andreas Färber <afaerber@suse.de> wrote:
>> Am 04.05.2013 10:22, schrieb Jean-Christophe DUBOIS:
>>> Do you mean arm_load_kernel() should test for qtest_enable() or should I
>>> call arm_load_kernel() only if !qtest_enable() (there is not much
>>> platform checking for qtest_enable() at this time and it seems somebody
>>> need to set the "entry" field in the info struct).
>> In short, qtest does not execute any code, so it does not need to load
>> binaries or set CPU entry addresses.
>>
>> Yes, I would say there is no point calling arm_load_kernel() if
>> !qtest_enabled().
> I'd rather we didn't end up with qtest_enabled() checks infecting
> every board model, please...

Then should we put it in arm_load_kernel() so that all board model get it ?

JC

>
> thanks
> -- PMM
>

Patch

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index b3a0207..a20f112 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -81,3 +81,5 @@  CONFIG_VERSATILE_PCI=y
 CONFIG_VERSATILE_I2C=y
 
 CONFIG_SDHCI=y
+
+CONFIG_IMX_I2C=y
diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
index 648278e..d27bbaa 100644
--- a/hw/i2c/Makefile.objs
+++ b/hw/i2c/Makefile.objs
@@ -4,4 +4,5 @@  common-obj-$(CONFIG_ACPI) += smbus_ich9.o
 common-obj-$(CONFIG_APM) += pm_smbus.o
 common-obj-$(CONFIG_BITBANG_I2C) += bitbang_i2c.o
 common-obj-$(CONFIG_EXYNOS4) += exynos4210_i2c.o
+common-obj-$(CONFIG_IMX_I2C) += imx_i2c.o
 obj-$(CONFIG_OMAP) += omap_i2c.o
diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
new file mode 100644
index 0000000..30d3f5c
--- /dev/null
+++ b/hw/i2c/imx_i2c.c
@@ -0,0 +1,374 @@ 
+/*
+ *  i.MX I2C Bus Serial Interface Emulation
+ *
+ *  Copyright (C) 2013 Jean-Christophe Dubois.
+ *
+ *  This program is free software; you can redistribute it and/or modify it
+ *  under the terms of the GNU General Public License as published by the
+ *  Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful, but WITHOUT
+ *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ *  FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ *  for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "hw/sysbus.h"
+#include "hw/i2c/i2c.h"
+
+#ifndef IMX_I2C_DEBUG
+#define IMX_I2C_DEBUG                 0
+#endif
+
+#define TYPE_IMX_I2C                  "imx.i2c"
+#define IMX_I2C(obj)                  \
+    OBJECT_CHECK(imx_i2c_state, (obj), TYPE_IMX_I2C)
+
+/* i.MX I2C memory map */
+#define IMX_I2C_MEM_SIZE           0x14
+#define IADR_ADDR                  0x00  /* address register */
+#define IFDR_ADDR                  0x04  /* frequency divider register */
+#define I2CR_ADDR                  0x08  /* control register */
+#define I2SR_ADDR                  0x0c  /* status register */
+#define I2DR_ADDR                  0x10  /* data register */
+
+#define IADR_MASK                  0xFE
+#define IADR_RESET                 0
+
+#define IFDR_MASK                  0x3F
+#define IFDR_RESET                 0
+
+#define I2CR_IEN                   (1 << 7)
+#define I2CR_IIEN                  (1 << 6)
+#define I2CR_MSTA                  (1 << 5)
+#define I2CR_MTX                   (1 << 4)
+#define I2CR_TXAK                  (1 << 3)
+#define I2CR_RSTA                  (1 << 2)
+#define I2CR_MASK                  0xFC
+#define I2CR_RESET                 0
+
+#define I2SR_ICF                   (1 << 7)
+#define I2SR_IAAF                  (1 << 6)
+#define I2SR_IBB                   (1 << 5)
+#define I2SR_IAL                   (1 << 4)
+#define I2SR_SRW                   (1 << 2)
+#define I2SR_IIF                   (1 << 1)
+#define I2SR_RXAK                  (1 << 0)
+#define I2SR_MASK                  0xE9
+#define I2SR_RESET                 0x81
+
+#define I2DR_MASK                  0xFF
+#define I2DR_RESET                 0
+
+#define ADDR_RESET                 0xFF00
+
+#if IMX_I2C_DEBUG
+#define DPRINT(fmt, args...)              \
+    do { fprintf(stderr, "%s: "fmt, __func__, ## args); } while (0)
+
+static const char *imx_i2c_get_regname(unsigned offset)
+{
+    switch (offset) {
+    case IADR_ADDR:
+        return "IADR";
+    case IFDR_ADDR:
+        return "IFDR";
+    case I2CR_ADDR:
+        return "I2CR";
+    case I2SR_ADDR:
+        return "I2SR";
+    case I2DR_ADDR:
+        return "I2DR";
+    default:
+        return "[?]";
+    }
+}
+
+#else
+#define DPRINT(fmt, args...)              do { } while (0)
+#endif
+
+typedef struct imx_i2c_state {
+    SysBusDevice busdev;
+    MemoryRegion iomem;
+    i2c_bus *bus;
+    qemu_irq irq;
+
+    uint16_t  address;
+
+    uint16_t iadr;
+    uint16_t ifdr;
+    uint16_t i2cr;
+    uint16_t i2sr;
+    uint16_t i2dr_read;
+    uint16_t i2dr_write;
+} imx_i2c_state;
+
+static inline bool imx_i2c_is_enabled(imx_i2c_state *s)
+{
+    return (s->i2cr & I2CR_IEN);
+}
+
+static inline bool imx_i2c_interrupt_is_enabled(imx_i2c_state *s)
+{
+    return (s->i2cr & I2CR_IIEN);
+}
+
+static inline bool imx_i2c_is_master(imx_i2c_state *s)
+{
+    return (s->i2cr & I2CR_MSTA);
+}
+
+static inline bool imx_i2c_direction_is_tx(imx_i2c_state *s)
+{
+    return (s->i2cr & I2CR_MTX);
+}
+
+static void imx_i2c_reset(DeviceState *d)
+{
+    imx_i2c_state *s = FROM_SYSBUS(imx_i2c_state, SYS_BUS_DEVICE(d));
+
+    if (s->address != ADDR_RESET) {
+        i2c_end_transfer(s->bus);
+    }
+
+    s->address    = ADDR_RESET;
+    s->iadr       = IADR_RESET;
+    s->ifdr       = IFDR_RESET;
+    s->i2cr       = I2CR_RESET;
+    s->i2sr       = I2SR_RESET;
+    s->i2dr_read  = I2DR_RESET;
+    s->i2dr_write = I2DR_RESET;
+}
+
+static inline void imx_i2c_raise_interrupt(imx_i2c_state *s)
+{
+    /*
+     * raise an interrupt if the device is enabled and it is configured
+     * to generate some interrupts.
+     */
+    if (imx_i2c_is_enabled(s) && imx_i2c_interrupt_is_enabled(s)) {
+        s->i2sr |= I2SR_IIF;
+        qemu_irq_raise(s->irq);
+    }
+}
+
+static uint64_t imx_i2c_read(void *opaque, hwaddr offset,
+                             unsigned size)
+{
+    imx_i2c_state *s = (imx_i2c_state *)opaque;
+    uint16_t value;
+
+    switch (offset) {
+    case IADR_ADDR:
+        value = s->iadr;
+        break;
+    case IFDR_ADDR:
+        value = s->ifdr;
+        break;
+    case I2CR_ADDR:
+        value = s->i2cr;
+        break;
+    case I2SR_ADDR:
+        value = s->i2sr;
+        break;
+    case I2DR_ADDR:
+        value = s->i2dr_read;
+        if (imx_i2c_is_master(s)) { // master mode
+            int ret;
+
+            if (s->address == ADDR_RESET) {
+                // something is wrong as the address is not set
+                DPRINT("Trying to read with specifying the slave address\n");
+            }
+
+            if ((ret = i2c_recv(s->bus)) >= 0) {
+                s->i2dr_read = ret;
+                imx_i2c_raise_interrupt(s);
+            } else {
+                DPRINT("read failed for device 0x%02x\n" s->address);
+                s->i2dr_read = 0xff;
+            }
+        } else { // slave mode
+        }
+        break;
+    default:
+        hw_error("%s: Bad address 0x%x\n", __func__, (int)offset);
+        break;
+    }
+
+    DPRINT("read %s [0x%02x] -> 0x%02x\n", imx_i2c_get_regname(offset),
+           (unsigned int)offset, value);
+
+    return (uint64_t)value;
+}
+
+static void imx_i2c_write(void *opaque, hwaddr offset,
+                          uint64_t value, unsigned size)
+{
+    imx_i2c_state *s = (imx_i2c_state *)opaque;
+    uint16_t v = value & 0xff;
+
+    DPRINT("write %s [0x%02x] <- 0x%02x\n", imx_i2c_get_regname(offset),
+           (unsigned int)offset, v);
+
+    switch (offset) {
+    case IADR_ADDR:
+        s->iadr = v & IADR_MASK;
+        //i2c_set_slave_address(s->bus, (uint8_t)s->iadr);
+        break;
+    case IFDR_ADDR:
+        s->ifdr = v & IFDR_MASK;
+        break;
+    case I2CR_ADDR:
+        if (imx_i2c_is_enabled(s) && ((v & I2CR_IEN) == 0)) {
+            /* This is a soft reset. IADR is preserved during soft resets */
+            uint16_t iadr = s->iadr;
+            imx_i2c_reset(&s->busdev.qdev);
+            s->iadr = iadr;
+        } else { // normal write
+            s->i2cr = v & I2CR_MASK;
+
+            if (imx_i2c_is_master(s)) { // master mode
+                // set the bus to busy
+                s->i2sr |= I2SR_IBB;
+            } else { // slave mode
+                // bus is not busy anymore
+                s->i2sr &= ~I2SR_IBB;
+
+                // if we unset the master mode then it ends the ongoing
+                // transfer if any
+                if (s->address != ADDR_RESET) {
+                    i2c_end_transfer(s->bus);
+                    s->address = ADDR_RESET;
+                }
+            }
+
+            if (s->i2cr & I2CR_RSTA) { // Restart
+                // if this is a restart then it ends the ongoing transfer
+                if (s->address != ADDR_RESET) {
+                    i2c_end_transfer(s->bus);
+                    s->address = ADDR_RESET;
+                    s->i2cr &= ~I2CR_RSTA;
+                }
+            }
+        }
+        break;
+    case I2SR_ADDR:
+        /*
+         * if the user writes 0 to IIF then lower the interrupt and
+         * reset the bit
+         */
+        if ((s->i2sr & I2SR_IIF) && !(v & I2SR_IIF)) {
+            s->i2sr &= ~I2SR_IIF;
+            qemu_irq_lower(s->irq);
+        }
+
+        /*
+         * if the user writes 0 to IAL, reset the bit
+         */
+        if ((s->i2sr & I2SR_IAL) && !(v & I2SR_IAL)) {
+            s->i2sr &= ~I2SR_IAL;
+            /* Nothing more to do ? */
+        }
+
+        break;
+    case I2DR_ADDR:
+        /* if the device is not enabled, nothing to do */
+        if (!imx_i2c_is_enabled(s)) {
+            break;
+        }
+
+        s->i2dr_write = v & I2DR_MASK;
+
+        if (imx_i2c_is_master(s)) { // master mode
+            /* If this is the first write cycle then it is the slave addr */
+            if (s->address == ADDR_RESET) {
+                if (!i2c_start_transfer(s->bus, (uint8_t)s->i2dr_write >> 1,
+                                        (int)s->i2dr_write & 0x01)) {
+                    // if 0 is returned, the adress is valid
+                    s->address = s->i2dr_write;
+                    s->i2sr &= ~I2SR_RXAK;
+                    imx_i2c_raise_interrupt(s);
+                } else { // can't start i2c transaction
+                }
+            } else { // This is a normal data write
+                if (i2c_send(s->bus, s->i2dr_write)) {
+                    // if the target return non zero then end the transfer
+                    i2c_end_transfer(s->bus);
+                }
+                s->i2sr &= ~I2SR_RXAK;
+                imx_i2c_raise_interrupt(s);
+            }
+        } else { // slave mode
+        }
+        break;
+    default:
+        hw_error("%s: Bad address 0x%x\n", __func__, (int)offset);
+        break;
+    }
+}
+
+static const MemoryRegionOps imx_i2c_ops = {
+    .read = imx_i2c_read,
+    .write = imx_i2c_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static const VMStateDescription imx_i2c_vmstate = {
+    .name = TYPE_IMX_I2C,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT16(address, imx_i2c_state),
+        VMSTATE_UINT16(iadr, imx_i2c_state),
+        VMSTATE_UINT16(ifdr, imx_i2c_state),
+        VMSTATE_UINT16(i2cr, imx_i2c_state),
+        VMSTATE_UINT16(i2sr, imx_i2c_state),
+        VMSTATE_UINT16(i2dr_read, imx_i2c_state),
+        VMSTATE_UINT16(i2dr_write, imx_i2c_state),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static int imx_i2c_init(SysBusDevice *dev)
+{
+    imx_i2c_state *s = IMX_I2C(dev);
+
+    memory_region_init_io(&s->iomem, &imx_i2c_ops, s, TYPE_IMX_I2C,
+                          IMX_I2C_MEM_SIZE);
+    sysbus_init_mmio(dev, &s->iomem);
+    sysbus_init_irq(dev, &s->irq);
+    s->bus = i2c_init_bus(&dev->qdev, "i2c");
+
+    return 0;
+}
+
+static void imx_i2c_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    SysBusDeviceClass *sbdc = SYS_BUS_DEVICE_CLASS(klass);
+
+    dc->vmsd = &imx_i2c_vmstate;
+    dc->reset = imx_i2c_reset;
+    sbdc->init = imx_i2c_init;
+}
+
+static const TypeInfo imx_i2c_type_info = {
+    .name = TYPE_IMX_I2C,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(imx_i2c_state),
+    .class_init = imx_i2c_class_init,
+};
+
+static void imx_i2c_register_types(void)
+{
+    type_register_static(&imx_i2c_type_info);
+}
+
+type_init(imx_i2c_register_types);