[V2,1/3] exynos4210: add Exynos4210 i2c implementation

Create 9 exynos4210 i2c interfaces.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 Makefile.target     |    1 +
 hw/exynos4210.c     |   26 +++
 hw/exynos4210.h     |    3 +
 hw/exynos4210_i2c.c |  469 +++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 499 insertions(+), 0 deletions(-)
 create mode 100644 hw/exynos4210_i2c.c

Am 15.03.2012 08:35, schrieb Igor Mitsyanko:
> Create 9 exynos4210 i2c interfaces.
> 
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

for the non-technical aspects. One remark below.

> diff --git a/hw/exynos4210_i2c.c b/hw/exynos4210_i2c.c
> new file mode 100644
> index 0000000..42f770d
> --- /dev/null
> +++ b/hw/exynos4210_i2c.c

> +enum {
> +    idle,
> +    start_bit_received,
> +    sending_data,
> +    receiving_data
> +};

Usually our enum values are uppercase, not sure if that's a rule.

Andreas

On 15 March 2012 07:35, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> Create 9 exynos4210 i2c interfaces.
>
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>

Mostly this looks OK but I still find the i2c slave stuff
odd -- should the controller really register itself as
a slave on its own bus? Doesn't this mean that the
controller can effectively try to talk to itself? Does
the hardware let you do that?

I suspect that what's happening here is that the hardware
lets you put the i2c controller into slave mode so some
other device on the bus can be a master. But QEMU's
i2c bus abstraction doesn't cover that use case at all...

Anyway, I don't know enough about i2c to really be able
to review this patch properly in that area. Anybody?

-- PMM

On 03/21/2012 03:55 PM, Peter Maydell wrote:
> On 15 March 2012 07:35, Igor Mitsyanko<i.mitsyanko@samsung.com>  wrote:
>> Create 9 exynos4210 i2c interfaces.
>>
>> Signed-off-by: Igor Mitsyanko<i.mitsyanko@samsung.com>
>
> Mostly this looks OK but I still find the i2c slave stuff
> odd -- should the controller really register itself as
> a slave on its own bus? Doesn't this mean that the
> controller can effectively try to talk to itself? Does
> the hardware let you do that?
>

Controller's master and slave i2c interfaces operate on the same single 
bus, so I think it should. It can't talk to itself because controller's 
two modes of operation are mutually exclusive. As I said, I took this 
approach from pxa i2c implementation, the only difference is that they 
register slave interface on a separate bus, but I think it's not right 
to do that.

> I suspect that what's happening here is that the hardware
> lets you put the i2c controller into slave mode so some
> other device on the bus can be a master. But QEMU's
> i2c bus abstraction doesn't cover that use case at all...
>

Yes, I saw this statement in hw/i2c.h (and probably cpu i2c controller 
will never be used as i2c slave device by anyone), but I think we still 
have to implement devices exactly like they described in documentation.

On 21 March 2012 13:07, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> On 03/21/2012 03:55 PM, Peter Maydell wrote:
>> I suspect that what's happening here is that the hardware
>> lets you put the i2c controller into slave mode so some
>> other device on the bus can be a master. But QEMU's
>> i2c bus abstraction doesn't cover that use case at all...

> Yes, I saw this statement in hw/i2c.h (and probably cpu i2c controller will
> never be used as i2c slave device by anyone), but I think we still have to
> implement devices exactly like they described in documentation.

I agree with the sentiment, I'm just not sure if the code you've
written is actually doing that. The right way to model this would
be if our i2c bus implementation provided an interface so you
could register as a device which is a master but can switch into
slave mode. Failing that, maybe we should just not support switching
into slave mode at all. Registering as two separate devices on the
i2c bus doesn't sound right to me.

-- PMM

On 03/21/2012 05:09 PM, Peter Maydell wrote:
> On 21 March 2012 13:07, Igor Mitsyanko<i.mitsyanko@samsung.com>  wrote:
>> On 03/21/2012 03:55 PM, Peter Maydell wrote:
>>> I suspect that what's happening here is that the hardware
>>> lets you put the i2c controller into slave mode so some
>>> other device on the bus can be a master. But QEMU's
>>> i2c bus abstraction doesn't cover that use case at all...
>
>> Yes, I saw this statement in hw/i2c.h (and probably cpu i2c controller will
>> never be used as i2c slave device by anyone), but I think we still have to
>> implement devices exactly like they described in documentation.
>
> I agree with the sentiment, I'm just not sure if the code you've
> written is actually doing that. The right way to model this would
> be if our i2c bus implementation provided an interface so you
> could register as a device which is a master but can switch into
> slave mode.
I don't think we can do that without multiple inheritance, and it 
wouldn't worth an effort for a feature that never going to be used.

  Failing that, maybe we should just not support switching
> into slave mode at all.
Do you mean we shouldn't register EXYNOS4_I2C_SLAVE at all so some 
hypothetical bus master wouldn't even find EXYNOS4_I2C_SLAVE on a bus?
Maybe the best solution is to make exynos4210_i2c_slave_send() and 
exynos4210_i2c_slave_recv() always return -1, so a hypothetical bus 
master will treat EXYNOS4_I2C_SLAVE as a broken device. But that seems 
to behave exactly like "not register at all" approach..
And are we really sure that slave interface wouldn't work correctly in a 
current implementation? For example, emulated Exynos CPU issues some 
command to a device A on SPI line and device A in turn issues data on 
i2c line connected to Exynos i2c controller configured as slave. 
EXYNOS4_I2C_SLAVE receives a data and raises interrupt flag.

  Registering as two separate devices on the
> i2c bus doesn't sound right to me.
>

Why? That's how it's done in hardware, I think we can roughly consider 
it to be two separate devices multiplexing single bus.

On 21 March 2012 14:18, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> Do you mean we shouldn't register EXYNOS4_I2C_SLAVE at all so some
> hypothetical bus master wouldn't even find EXYNOS4_I2C_SLAVE on a bus?
> Maybe the best solution is to make exynos4210_i2c_slave_send() and
> exynos4210_i2c_slave_recv() always return -1, so a hypothetical bus master
> will treat EXYNOS4_I2C_SLAVE as a broken device. But that seems to behave
> exactly like "not register at all" approach..
> And are we really sure that slave interface wouldn't work correctly in a
> current implementation? For example, emulated Exynos CPU issues some command
> to a device A on SPI line and device A in turn issues data on i2c line
> connected to Exynos i2c controller configured as slave. EXYNOS4_I2C_SLAVE
> receives a data and raises interrupt flag.

If there's a valid configuration that works in the existing code
where we can end up receiving data correctly to the EXYNOS4_I2C_SLAVE
from some other device on the i2c bus, that's fine: we can test that
the code you have works OK.

If there is no valid configuration that will do that (because we
don't have any support for any other device being a bus master)
then the code is completely useless, untested and untestable and
we shouldn't put it in.

-- PMM

Let me add my two cents here.  I tested this patch slightly and didn't
find any problems.

On 2012-03-15 11:35, Igor Mitsyanko wrote:
> Create 9 exynos4210 i2c interfaces.
>
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
> ---
>  Makefile.target     |    1 +
>  hw/exynos4210.c     |   26 +++
>  hw/exynos4210.h     |    3 +
>  hw/exynos4210_i2c.c |  469 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 499 insertions(+), 0 deletions(-)
>  create mode 100644 hw/exynos4210_i2c.c
>
> diff --git a/Makefile.target b/Makefile.target
> index eb25941..5e8a1d4 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -357,6 +357,7 @@ obj-arm-y += realview_gic.o realview.o arm_sysctl.o arm11mpcore.o a9mpcore.o
>  obj-arm-y += exynos4210_gic.o exynos4210_combiner.o exynos4210.o
>  obj-arm-y += exynos4_boards.o exynos4210_uart.o exynos4210_pwm.o
>  obj-arm-y += exynos4210_pmu.o exynos4210_mct.o exynos4210_fimd.o
> +obj-arm-y += exynos4210_i2c.o
>  obj-arm-y += arm_l2x0.o
>  obj-arm-y += arm_mptimer.o a15mpcore.o
>  obj-arm-y += armv7m.o armv7m_nvic.o stellaris.o pl022.o stellaris_enet.o
> diff --git a/hw/exynos4210.c b/hw/exynos4210.c
> index f904370..464f157 100644
> --- a/hw/exynos4210.c
> +++ b/hw/exynos4210.c
> @@ -35,6 +35,13 @@
>  /* MCT */
>  #define EXYNOS4210_MCT_BASE_ADDR       0x10050000
>  
> +/* I2C */
> +#define EXYNOS4210_I2C_SHIFT           0x00010000
> +#define EXYNOS4210_I2C_BASE_ADDR       0x13860000
> +/* Interrupt Group of External Interrupt Combiner for I2C */
> +#define EXYNOS4210_I2C_INTG            27
> +#define EXYNOS4210_HDMI_INTG           16
> +
>  /* UART's definitions */
>  #define EXYNOS4210_UART0_BASE_ADDR     0x13800000
>  #define EXYNOS4210_UART1_BASE_ADDR     0x13810000
> @@ -242,6 +249,25 @@ Exynos4210State *exynos4210_init(MemoryRegion *system_mem,
>              s->irq_table[exynos4210_get_irq(35, 3)]);
>      sysbus_mmio_map(busdev, 0, EXYNOS4210_MCT_BASE_ADDR);
>  
> +    /*** I2C ***/
> +    for (n = 0; n < EXYNOS4210_I2C_NUMBER; n++) {
> +        uint32_t addr = EXYNOS4210_I2C_BASE_ADDR + EXYNOS4210_I2C_SHIFT * n;
> +        qemu_irq i2c_irq;
> +
> +        if (n < 8) {
> +            i2c_irq = s->irq_table[exynos4210_get_irq(EXYNOS4210_I2C_INTG, n)];
> +        } else {
> +            i2c_irq = s->irq_table[exynos4210_get_irq(EXYNOS4210_HDMI_INTG, 1)];
> +        }
> +
> +        dev = qdev_create(NULL, "exynos4210.i2c");
> +        qdev_init_nofail(dev);
> +        busdev = sysbus_from_qdev(dev);
> +        sysbus_connect_irq(busdev, 0, i2c_irq);
> +        sysbus_mmio_map(busdev, 0, addr);
> +        s->i2c_if[n] = (i2c_bus *)qdev_get_child_bus(dev, "i2c");
> +    }
> +
>      /*** UARTs ***/
>      exynos4210_uart_create(EXYNOS4210_UART0_BASE_ADDR,
>                             EXYNOS4210_UART0_FIFO_SIZE, 0, NULL,
> diff --git a/hw/exynos4210.h b/hw/exynos4210.h
> index c112e03..1205fb5 100644
> --- a/hw/exynos4210.h
> +++ b/hw/exynos4210.h
> @@ -74,6 +74,8 @@
>  #define EXYNOS4210_EXT_GIC_NIRQ     (160-32)
>  #define EXYNOS4210_INT_GIC_NIRQ     64
>  
> +#define EXYNOS4210_I2C_NUMBER               9
> +
>  typedef struct Exynos4210Irq {
>      qemu_irq int_combiner_irq[EXYNOS4210_MAX_INT_COMBINER_IN_IRQ];
>      qemu_irq ext_combiner_irq[EXYNOS4210_MAX_EXT_COMBINER_IN_IRQ];
> @@ -95,6 +97,7 @@ typedef struct Exynos4210State {
>      MemoryRegion dram1_mem;
>      MemoryRegion boot_secondary;
>      MemoryRegion bootreg_mem;
> +    i2c_bus *i2c_if[EXYNOS4210_I2C_NUMBER];
>  } Exynos4210State;
>  
>  Exynos4210State *exynos4210_init(MemoryRegion *system_mem,
> diff --git a/hw/exynos4210_i2c.c b/hw/exynos4210_i2c.c
> new file mode 100644
> index 0000000..42f770d
> --- /dev/null
> +++ b/hw/exynos4210_i2c.c
> @@ -0,0 +1,469 @@
> +/*
> + *  Exynos4210 I2C Bus Serial Interface Emulation
> + *
> + *  Copyright (C) 2012 Samsung Electronics Co Ltd.
> + *    Maksim Kozlov, <m.kozlov@samsung.com>
> + *    Igor Mitsyanko, <i.mitsyanko@samsung.com>
> + *
> + *  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 "qemu-timer.h"
> +#include "sysbus.h"
> +#include "i2c.h"
> +
> +#ifndef EXYNOS4_I2C_DEBUG
> +#define EXYNOS4_I2C_DEBUG                 0
> +#endif
> +
> +#define TYPE_EXYNOS4_I2C                  "exynos4210.i2c"
> +#define TYPE_EXYNOS4_I2C_SLAVE            "exynos4210.i2c-slave"
> +#define EXYNOS4_I2C(obj)                  \
> +    OBJECT_CHECK(Exynos4210I2CState, (obj), TYPE_EXYNOS4_I2C)
> +#define EXYNOS4_I2C_SLAVE(obj)            \
> +    OBJECT_CHECK(Exynos4210I2CSlave, (obj), TYPE_EXYNOS4_I2C_SLAVE)
> +
> +/* Exynos4210 I2C memory map */
> +#define EXYNOS4_I2C_MEM_SIZE              0x14
> +#define I2CCON_ADDR                       0x00  /* control register */
> +#define I2CSTAT_ADDR                      0x04  /* control/status register */
> +#define I2CADD_ADDR                       0x08  /* address register */
> +#define I2CDS_ADDR                        0x0c  /* data shift register */
> +#define I2CLC_ADDR                        0x10  /* line control register */
> +
> +#define I2CCON_INTERRUPTS_EN              (1 << 5)
> +#define I2CCON_INTERRUPT_PEND             (1 << 4)
> +
> +#define EXYNOS4_I2C_MODE(reg)             (((reg) >> 6) & 3)
> +#define I2C_IN_MASTER_MODE(reg)           (((reg) >> 6) & 2)
> +#define I2CSTAT_MODE_SLAVE_Rx             0x0
> +#define I2CSTAT_MODE_SLAVE_Tx             0x1
> +#define I2CMODE_MASTER_Rx                 0x2
> +#define I2CMODE_MASTER_Tx                 0x3
> +#define I2CSTAT_LAST_BIT                  (1 << 0)
> +#define I2CSTAT_SLAVE_ADDR_ZERO           (1 << 1)
> +#define I2CSTAT_SLAVE_ADDR_MATCH          (1 << 2)
> +#define I2CSTAT_ARBITR_STATUS             (1 << 3)
> +#define I2CSTAT_OUTPUT_EN                 (1 << 4)
> +#define I2CSTAT_START_BUSY                (1 << 5)
> +
> +
> +#if EXYNOS4_I2C_DEBUG
> +#define DPRINT(fmt, args...)              \
> +    do {fprintf(stderr, "QEMU I2C: "fmt, ## args); } while (0)
> +
> +static const char *exynos4_i2c_regs_names[] = {
> +    [I2CCON_ADDR  >> 2] = "I2CCON",
> +    [I2CSTAT_ADDR >> 2] = "I2CSTAT",
> +    [I2CADD_ADDR  >> 2] = "I2CADD",
> +    [I2CDS_ADDR   >> 2] = "I2CDS",
> +    [I2CLC_ADDR   >> 2] = "I2CLC",
> +};
> +
> +#else
> +#define DPRINT(fmt, args...)              do { } while (0)
> +#endif
> +
> +typedef struct Exynos4210I2CSlave Exynos4210I2CSlave;
> +
> +typedef struct Exynos4210I2CState {
> +    SysBusDevice busdev;
> +    MemoryRegion iomem;
> +    i2c_bus *bus;
> +    Exynos4210I2CSlave *slave;
> +    qemu_irq irq;
> +    QEMUTimer *receive_timer;
> +    QEMUTimer *send_timer;
> +
> +    uint8_t i2ccon;
> +    uint8_t i2cstat;
> +    uint8_t i2cadd;
> +    uint8_t i2cds;
> +    uint8_t i2clc;
> +} Exynos4210I2CState;
> +
> +enum {
> +    idle,
> +    start_bit_received,
> +    sending_data,
> +    receiving_data
> +};
> +
> +struct Exynos4210I2CSlave {
> +    I2CSlave i2c;
> +    Exynos4210I2CState *host;
> +    uint32_t status;
> +};
> +
> +
> +static inline void exynos4210_i2c_set_irq(Exynos4210I2CState *s)
> +{
> +    if (s->i2ccon & I2CCON_INTERRUPTS_EN) {
> +        s->i2ccon |= I2CCON_INTERRUPT_PEND;
> +        qemu_irq_raise(s->irq);
> +    }
> +}
> +
> +static void exynos4210_i2c_data_received(void *opaque)
> +{
> +    Exynos4210I2CState *s = (Exynos4210I2CState *)opaque;
> +    int ret;
> +
> +    ret = i2c_recv(s->bus);
> +    if (ret < 0) {
> +        s->i2cstat |= I2CSTAT_LAST_BIT;  /* Data is not acknowledged */
> +    } else {
> +        s->i2cds = ret;
> +        s->i2cstat &= ~I2CSTAT_LAST_BIT; /* Data is acknowledged */
> +    }
> +    exynos4210_i2c_set_irq(s);
> +}
> +
> +static void exynos4210_i2c_data_sent(void *opaque)
> +{
> +    Exynos4210I2CState *s = (Exynos4210I2CState *)opaque;
> +
> +    if (i2c_send(s->bus, s->i2cds) < 0) {
> +        s->i2cstat |= I2CSTAT_LAST_BIT;  /* Data is not acknowledged */
> +    } else {
> +        s->i2cstat &= ~I2CSTAT_LAST_BIT; /* Data is acknowledged */
> +    }
> +    exynos4210_i2c_set_irq(s);
> +}
> +
> +static uint64_t exynos4210_i2c_read(void *opaque, target_phys_addr_t offset,
> +                                 unsigned size)
> +{
> +    Exynos4210I2CState *s = (Exynos4210I2CState *)opaque;
> +    uint8_t value;
> +
> +    switch (offset) {
> +    case I2CCON_ADDR:
> +        value = s->i2ccon;
> +        break;
> +    case I2CSTAT_ADDR:
> +        value = s->i2cstat;
> +        break;
> +    case I2CADD_ADDR:
> +        value = s->i2cadd;
> +        break;
> +    case I2CDS_ADDR:
> +        value = s->i2cds;
> +        break;
> +    case I2CLC_ADDR:
> +        value = s->i2clc;
> +        break;
> +    default:
> +        value = 0;
> +        DPRINT("ERROR: Bad read offset 0x%x\n", (unsigned int)offset);
> +        break;
> +    }
> +
> +    DPRINT("read %s [0x%02x] -> 0x%02x\n", exynos4_i2c_regs_names[offset >> 2],
> +            (unsigned int)offset, value);
Just for security it's better to check offset to not go out of the array
borders.
> +    return value;
> +}
> +
> +static void exynos4210_i2c_write(void *opaque, target_phys_addr_t offset,
> +                              uint64_t value, unsigned size)
> +{
> +    Exynos4210I2CState *s = (Exynos4210I2CState *)opaque;
> +    uint8_t v = value & 0xff;
> +
> +    DPRINT("write %s [0x%02x] <- 0x%02x\n", exynos4_i2c_regs_names[offset >> 2],
> +            (unsigned int)offset, v);
> +
> +    switch (offset) {
> +    case I2CCON_ADDR:
> +        if ((s->i2ccon & I2CCON_INTERRUPT_PEND) &&
> +                !(v & I2CCON_INTERRUPT_PEND)) {
> +            s->i2ccon &= ~I2CCON_INTERRUPT_PEND;
> +            if (!(s->i2ccon & I2CCON_INTERRUPTS_EN)) {
> +                s->i2cstat &= ~I2CSTAT_START_BUSY;
> +            }
> +            qemu_irq_lower(s->irq);
> +
> +            if (!(s->i2cstat & I2CSTAT_OUTPUT_EN) ||
> +                    !(s->i2cstat & I2CSTAT_START_BUSY)) {
> +                s->i2cstat &= ~I2CSTAT_START_BUSY;   /* Line is not busy */
> +                break;
> +            }
> +            /* Continue operation if transfer is ongoing */
> +            if (EXYNOS4_I2C_MODE(s->i2cstat) == I2CMODE_MASTER_Rx) {
> +                qemu_mod_timer(s->receive_timer, qemu_get_clock_ns(vm_clock)+1);
> +            } else if (EXYNOS4_I2C_MODE(s->i2cstat) == I2CMODE_MASTER_Tx) {
> +                qemu_mod_timer(s->send_timer, qemu_get_clock_ns(vm_clock) + 1);
Why do you need to use timers here?  Why not just call the corresponding
functions directly?  I can guess because when sending new message Linux
driver first enables the interrupt and then writes the stat register. 
Am I right?
> +            }
> +        }
> +
> +        /* bit 4 can't be written */
> +        s->i2ccon = (v & ~I2CCON_INTERRUPT_PEND) |
> +                (s->i2ccon & I2CCON_INTERRUPT_PEND);
> +        break;
> +    case I2CSTAT_ADDR:
> +        s->i2cstat =
> +                (s->i2cstat & I2CSTAT_START_BUSY) | (v & ~I2CSTAT_START_BUSY);
> +
> +        if (!(s->i2cstat & I2CSTAT_OUTPUT_EN)) {
> +            s->i2cstat &= ~I2CSTAT_START_BUSY;
> +            qemu_del_timer(s->receive_timer);
> +            qemu_del_timer(s->send_timer);
> +            break;
> +        }
> +
> +        /* Nothing to do if in i2c slave mode */
> +        if (!I2C_IN_MASTER_MODE(s->i2cstat)) {
> +            break;
> +        }
> +
> +        if (v & I2CSTAT_START_BUSY) {
> +            s->i2cstat |= I2CSTAT_START_BUSY;    /* Line is busy */
> +            /* Generate start bit and send slave address */
> +            i2c_start_transfer(s->bus, s->i2cds >> 1, s->i2cds & 0x1);
> +            exynos4210_i2c_data_sent(s);
> +        } else {
> +            i2c_end_transfer(s->bus);
> +            /* Linux driver may start next i2c transaction before it
> +             * acknowledges an interrupt request from previous transaction.
> +             * This may result in timeout waiting for I2C bus idle and
> +             * corrupted transfer. Releasing I2C bus only after interrupt is
> +             * accepted prevents this corruption. */
> +            if (!(s->i2ccon & I2CCON_INTERRUPT_PEND)) {
> +                s->i2cstat &= ~I2CSTAT_START_BUSY;
> +            }
> +        }
> +        break;
> +    case I2CADD_ADDR:
> +        if ((s->i2cstat & I2CSTAT_OUTPUT_EN) == 0) {
> +            s->i2cadd = v;
> +            i2c_set_slave_address(&s->slave->i2c, v);
> +        }
> +        break;
> +    case I2CDS_ADDR:
> +        if (s->i2cstat & I2CSTAT_OUTPUT_EN) {
> +            s->i2cds = v;
> +        }
> +        break;
> +    case I2CLC_ADDR:
> +        s->i2clc = v;
> +        break;
> +    default:
> +        DPRINT("ERROR: Bad write offset 0x%x\n", (unsigned int)offset);
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps exynos4210_i2c_ops = {
> +    .read = exynos4210_i2c_read,
> +    .write = exynos4210_i2c_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static const VMStateDescription exynos4210_i2c_slave_vmstate = {
> +    .name = TYPE_EXYNOS4_I2C_SLAVE,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_I2C_SLAVE(i2c, Exynos4210I2CSlave),
> +        VMSTATE_UINT32(status, Exynos4210I2CSlave),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const VMStateDescription exynos4210_i2c_vmstate = {
> +    .name = TYPE_EXYNOS4_I2C,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(i2ccon, Exynos4210I2CState),
> +        VMSTATE_UINT8(i2cstat, Exynos4210I2CState),
> +        VMSTATE_UINT8(i2cds, Exynos4210I2CState),
> +        VMSTATE_UINT8(i2clc, Exynos4210I2CState),
Should i2cadd also be saved here?
> +        VMSTATE_TIMER(receive_timer, Exynos4210I2CState),
> +        VMSTATE_TIMER(send_timer, Exynos4210I2CState),
> +        VMSTATE_STRUCT_POINTER(slave, Exynos4210I2CState,
> +                exynos4210_i2c_slave_vmstate, Exynos4210I2CSlave *),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void exynos4210_i2c_reset(DeviceState *d)
> +{
> +    Exynos4210I2CState *s = EXYNOS4_I2C(d);
> +
> +    s->i2ccon  = 0x0f; /* 0x0X */
> +    s->i2cstat = 0x00;
> +    s->i2cds   = 0xff; /* 0xXX */
> +    s->i2clc   = 0x00;
> +    s->i2cadd  = 0xff; /* 0xXX */
> +    s->slave->status = idle;
> +    qemu_del_timer(s->receive_timer);
> +    qemu_del_timer(s->send_timer);
> +}
> +
> +static int exynos4210_i2c_init(SysBusDevice *dev)
> +{
> +    Exynos4210I2CState *s = EXYNOS4_I2C(dev);
> +    DeviceState *slave;
> +
> +    memory_region_init_io(&s->iomem, &exynos4210_i2c_ops, s, TYPE_EXYNOS4_I2C,
> +                          EXYNOS4_I2C_MEM_SIZE);
> +    sysbus_init_mmio(dev, &s->iomem);
> +    sysbus_init_irq(dev, &s->irq);
> +    s->bus = i2c_init_bus(&dev->qdev, "i2c");
> +    s->receive_timer =
> +            qemu_new_timer_ns(vm_clock, exynos4210_i2c_data_received, s);
> +    s->send_timer = qemu_new_timer_ns(vm_clock, exynos4210_i2c_data_sent, s);
> +
> +    /* i2c slave initialization */
> +    slave = i2c_create_slave(s->bus, TYPE_EXYNOS4_I2C_SLAVE, 0xff);
> +    s->slave = EXYNOS4_I2C_SLAVE(slave);
> +    s->slave->host = s;
> +
> +    return 0;
> +}
> +
> +static void exynos4210_i2c_class_init(ObjectClass *class, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(class);
> +    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class);
> +
> +    dc->vmsd = &exynos4210_i2c_vmstate;
> +    dc->reset = exynos4210_i2c_reset;
> +    sbc->init = exynos4210_i2c_init;
> +}
> +
> +static TypeInfo exynos4210_i2c_info = {
> +    .name = TYPE_EXYNOS4_I2C,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(Exynos4210I2CState),
> +    .class_init = exynos4210_i2c_class_init,
> +};
> +
> +static int exynos4210_i2c_slave_init(I2CSlave *i2c)
> +{
> +    Exynos4210I2CSlave *s = EXYNOS4_I2C_SLAVE(i2c);
> +
> +    s->status = idle;
> +    return 0;
> +}
> +
> +static void exynos4210_i2c_slave_event(I2CSlave *i2c, enum i2c_event event)
> +{
> +    Exynos4210I2CSlave *s = EXYNOS4_I2C_SLAVE(i2c);
> +    Exynos4210I2CState *host = s->host;
> +
> +    if (!(host->i2cstat & I2CSTAT_OUTPUT_EN) ||
> +            I2C_IN_MASTER_MODE(host->i2cstat)) {
> +        return;
> +    }
> +
> +    switch (event) {
> +    case I2C_START_RECV:
> +    case I2C_START_SEND:
> +        host->i2cstat &= ~(I2CSTAT_SLAVE_ADDR_ZERO | I2CSTAT_SLAVE_ADDR_MATCH);
> +        s->status = start_bit_received;
> +        host->i2cstat |= I2CSTAT_START_BUSY;    /* Line is busy */
> +        break;
> +    case I2C_FINISH:
> +        s->status = idle;
> +        host->i2cstat &= ~(I2CSTAT_SLAVE_ADDR_ZERO | I2CSTAT_SLAVE_ADDR_MATCH |
> +                I2CSTAT_START_BUSY);
> +        break;
> +    default:
> +        break;
> +    }
> +}
> +
> +static int exynos4210_i2c_slave_recv(I2CSlave *i2c)
> +{
> +    Exynos4210I2CSlave *s = EXYNOS4_I2C_SLAVE(i2c);
> +    Exynos4210I2CState *host = s->host;
> +
> +    if (!(host->i2cstat & I2CSTAT_OUTPUT_EN) ||
> +            I2C_IN_MASTER_MODE(host->i2cstat)) {
> +        return -1;
> +    }
> +
> +    if ((s->status != sending_data) || (EXYNOS4_I2C_MODE(host->i2cstat) !=
> +            I2CSTAT_MODE_SLAVE_Tx)) {
> +        return -1;
> +    }
> +
> +    exynos4210_i2c_set_irq(host);
> +    return host->i2cds;
> +}
> +
> +static int exynos4210_i2c_slave_send(I2CSlave *i2c, uint8_t data)
> +{
> +    Exynos4210I2CSlave *s = EXYNOS4_I2C_SLAVE(i2c);
> +    Exynos4210I2CState *host = s->host;
> +
> +    if (!(host->i2cstat & I2CSTAT_OUTPUT_EN) ||
> +            I2C_IN_MASTER_MODE(host->i2cstat)) {
> +        return -1;
> +    }
> +
> +    if (s->status == start_bit_received) {
> +        host->i2cds = data;
> +        s->status = idle;
> +        if ((data & 0xFE) == 0) {
> +            host->i2cstat |= I2CSTAT_SLAVE_ADDR_ZERO;
> +            return -1;
> +        }
> +        if ((host->i2cadd & 0xFE) != (data & 0xFE)) {
> +            return -1;
> +        }
> +        host->i2cstat |= I2CSTAT_SLAVE_ADDR_MATCH;
> +        (data & 0x1) ? (s->status = receiving_data) :
> +                (s->status = sending_data);
> +        exynos4210_i2c_set_irq(host);
> +        return 0;
> +    }
> +
> +    if ((s->status != receiving_data) || (EXYNOS4_I2C_MODE(host->i2cstat) !=
> +            I2CSTAT_MODE_SLAVE_Rx)) {
> +        return -1;
> +    }
> +
> +    host->i2cds = data;
> +    exynos4210_i2c_set_irq(host);
> +    return 0;
> +}
> +
> +static void exynos4210_i2cslave_class_init(ObjectClass *klass, void *data)
> +{
> +    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
> +
> +    k->init = exynos4210_i2c_slave_init;
> +    k->event = exynos4210_i2c_slave_event;
> +    k->recv = exynos4210_i2c_slave_recv;
> +    k->send = exynos4210_i2c_slave_send;
> +}
> +
> +static TypeInfo exynos4210_i2c_slave = {
> +    .name = TYPE_EXYNOS4_I2C_SLAVE,
> +    .parent = TYPE_I2C_SLAVE,
> +    .instance_size = sizeof(Exynos4210I2CSlave),
> +    .class_init = exynos4210_i2cslave_class_init,
> +};
> +
> +static void exynos4210_i2c_register_types(void)
> +{
> +    type_register_static(&exynos4210_i2c_slave);
> +    type_register_static(&exynos4210_i2c_info);
> +}
> +
> +type_init(exynos4210_i2c_register_types)
PS: This I2C implementation seems to be rather complex.  Did you check
what we wrote for S5PC110 (ah, I also learnt that Samsung renamed it to
Exynos3210) almost three years ago
(https://gitorious.org/samsung-linux-platform/qemu/blobs/s5pc110-stable/hw/s5pc1xx_i2c.c)? 
When I compared documentation and kernel driver sources for both devices
I couldn't find any significant difference.  Generally speaking, is it
worth redoing everything from scratch instead of updating?

On 04/03/2012 05:54 PM, Dmitry Zhurikhin wrote:
> Let me add my two cents here.  I tested this patch slightly and didn't
> find any problems.
>
Great, thanks!
> On 2012-03-15 11:35, Igor Mitsyanko wrote:
>> Create 9 exynos4210 i2c interfaces.
>>
>> Signed-off-by: Igor Mitsyanko<i.mitsyanko@samsung.com>
>> ---
>>   Makefile.target     |    1 +
>>   hw/exynos4210.c     |   26 +++
>>   hw/exynos4210.h     |    3 +
>>   hw/exynos4210_i2c.c |  469 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 499 insertions(+), 0 deletions(-)
>>   create mode 100644 hw/exynos4210_i2c.c
>>
>> diff --git a/Makefile.target b/Makefile.target
>> index eb25941..5e8a1d4 100644
>> --- a/Makefile.target
>> +++ b/Makefile.target
>> @@ -357,6 +357,7 @@ obj-arm-y += realview_gic.o realview.o arm_sysctl.o arm11mpcore.o a9mpcore.o
>>   obj-arm-y += exynos4210_gic.o exynos4210_combiner.o exynos4210.o
>>   obj-arm-y += exynos4_boards.o exynos4210_uart.o exynos4210_pwm.o
>>   obj-arm-y += exynos4210_pmu.o exynos4210_mct.o exynos4210_fimd.o
>> +obj-arm-y += exynos4210_i2c.o
>>   obj-arm-y += arm_l2x0.o
>>   obj-arm-y += arm_mptimer.o a15mpcore.o
>>   obj-arm-y += armv7m.o armv7m_nvic.o stellaris.o pl022.o stellaris_enet.o
>> diff --git a/hw/exynos4210.c b/hw/exynos4210.c
>> index f904370..464f157 100644
>> --- a/hw/exynos4210.c
>> +++ b/hw/exynos4210.c
>> @@ -35,6 +35,13 @@
>>   /* MCT */
>>   #define EXYNOS4210_MCT_BASE_ADDR       0x10050000
>>
>> +/* I2C */
>> +#define EXYNOS4210_I2C_SHIFT           0x00010000
>> +#define EXYNOS4210_I2C_BASE_ADDR       0x13860000
>> +/* Interrupt Group of External Interrupt Combiner for I2C */
>> +#define EXYNOS4210_I2C_INTG            27
>> +#define EXYNOS4210_HDMI_INTG           16
>> +
>>   /* UART's definitions */
>>   #define EXYNOS4210_UART0_BASE_ADDR     0x13800000
>>   #define EXYNOS4210_UART1_BASE_ADDR     0x13810000
>> @@ -242,6 +249,25 @@ Exynos4210State *exynos4210_init(MemoryRegion *system_mem,
>>               s->irq_table[exynos4210_get_irq(35, 3)]);
>>       sysbus_mmio_map(busdev, 0, EXYNOS4210_MCT_BASE_ADDR);
>>
>> +    /*** I2C ***/
>> +    for (n = 0; n<  EXYNOS4210_I2C_NUMBER; n++) {
>> +        uint32_t addr = EXYNOS4210_I2C_BASE_ADDR + EXYNOS4210_I2C_SHIFT * n;
>> +        qemu_irq i2c_irq;
>> +
>> +        if (n<  8) {
>> +            i2c_irq = s->irq_table[exynos4210_get_irq(EXYNOS4210_I2C_INTG, n)];
>> +        } else {
>> +            i2c_irq = s->irq_table[exynos4210_get_irq(EXYNOS4210_HDMI_INTG, 1)];
>> +        }
>> +
>> +        dev = qdev_create(NULL, "exynos4210.i2c");
>> +        qdev_init_nofail(dev);
>> +        busdev = sysbus_from_qdev(dev);
>> +        sysbus_connect_irq(busdev, 0, i2c_irq);
>> +        sysbus_mmio_map(busdev, 0, addr);
>> +        s->i2c_if[n] = (i2c_bus *)qdev_get_child_bus(dev, "i2c");
>> +    }
>> +
>>       /*** UARTs ***/
>>       exynos4210_uart_create(EXYNOS4210_UART0_BASE_ADDR,
>>                              EXYNOS4210_UART0_FIFO_SIZE, 0, NULL,
>> diff --git a/hw/exynos4210.h b/hw/exynos4210.h
>> index c112e03..1205fb5 100644
>> --- a/hw/exynos4210.h
>> +++ b/hw/exynos4210.h
>> @@ -74,6 +74,8 @@
>>   #define EXYNOS4210_EXT_GIC_NIRQ     (160-32)
>>   #define EXYNOS4210_INT_GIC_NIRQ     64
>>
>> +#define EXYNOS4210_I2C_NUMBER               9
>> +
>>   typedef struct Exynos4210Irq {
>>       qemu_irq int_combiner_irq[EXYNOS4210_MAX_INT_COMBINER_IN_IRQ];
>>       qemu_irq ext_combiner_irq[EXYNOS4210_MAX_EXT_COMBINER_IN_IRQ];
>> @@ -95,6 +97,7 @@ typedef struct Exynos4210State {
>>       MemoryRegion dram1_mem;
>>       MemoryRegion boot_secondary;
>>       MemoryRegion bootreg_mem;
>> +    i2c_bus *i2c_if[EXYNOS4210_I2C_NUMBER];
>>   } Exynos4210State;
>>
>>   Exynos4210State *exynos4210_init(MemoryRegion *system_mem,
>> diff --git a/hw/exynos4210_i2c.c b/hw/exynos4210_i2c.c
>> new file mode 100644
>> index 0000000..42f770d
>> --- /dev/null
>> +++ b/hw/exynos4210_i2c.c
>> @@ -0,0 +1,469 @@
>> +/*
>> + *  Exynos4210 I2C Bus Serial Interface Emulation
>> + *
>> + *  Copyright (C) 2012 Samsung Electronics Co Ltd.
>> + *    Maksim Kozlov,<m.kozlov@samsung.com>
>> + *    Igor Mitsyanko,<i.mitsyanko@samsung.com>
>> + *
>> + *  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 "qemu-timer.h"
>> +#include "sysbus.h"
>> +#include "i2c.h"
>> +
>> +#ifndef EXYNOS4_I2C_DEBUG
>> +#define EXYNOS4_I2C_DEBUG                 0
>> +#endif
>> +
>> +#define TYPE_EXYNOS4_I2C                  "exynos4210.i2c"
>> +#define TYPE_EXYNOS4_I2C_SLAVE            "exynos4210.i2c-slave"
>> +#define EXYNOS4_I2C(obj)                  \
>> +    OBJECT_CHECK(Exynos4210I2CState, (obj), TYPE_EXYNOS4_I2C)
>> +#define EXYNOS4_I2C_SLAVE(obj)            \
>> +    OBJECT_CHECK(Exynos4210I2CSlave, (obj), TYPE_EXYNOS4_I2C_SLAVE)
>> +
>> +/* Exynos4210 I2C memory map */
>> +#define EXYNOS4_I2C_MEM_SIZE              0x14
>> +#define I2CCON_ADDR                       0x00  /* control register */
>> +#define I2CSTAT_ADDR                      0x04  /* control/status register */
>> +#define I2CADD_ADDR                       0x08  /* address register */
>> +#define I2CDS_ADDR                        0x0c  /* data shift register */
>> +#define I2CLC_ADDR                        0x10  /* line control register */
>> +
>> +#define I2CCON_INTERRUPTS_EN              (1<<  5)
>> +#define I2CCON_INTERRUPT_PEND             (1<<  4)
>> +
>> +#define EXYNOS4_I2C_MODE(reg)             (((reg)>>  6)&  3)
>> +#define I2C_IN_MASTER_MODE(reg)           (((reg)>>  6)&  2)
>> +#define I2CSTAT_MODE_SLAVE_Rx             0x0
>> +#define I2CSTAT_MODE_SLAVE_Tx             0x1
>> +#define I2CMODE_MASTER_Rx                 0x2
>> +#define I2CMODE_MASTER_Tx                 0x3
>> +#define I2CSTAT_LAST_BIT                  (1<<  0)
>> +#define I2CSTAT_SLAVE_ADDR_ZERO           (1<<  1)
>> +#define I2CSTAT_SLAVE_ADDR_MATCH          (1<<  2)
>> +#define I2CSTAT_ARBITR_STATUS             (1<<  3)
>> +#define I2CSTAT_OUTPUT_EN                 (1<<  4)
>> +#define I2CSTAT_START_BUSY                (1<<  5)
>> +
>> +
>> +#if EXYNOS4_I2C_DEBUG
>> +#define DPRINT(fmt, args...)              \
>> +    do {fprintf(stderr, "QEMU I2C: "fmt, ## args); } while (0)
>> +
>> +static const char *exynos4_i2c_regs_names[] = {
>> +    [I2CCON_ADDR>>  2] = "I2CCON",
>> +    [I2CSTAT_ADDR>>  2] = "I2CSTAT",
>> +    [I2CADD_ADDR>>  2] = "I2CADD",
>> +    [I2CDS_ADDR>>  2] = "I2CDS",
>> +    [I2CLC_ADDR>>  2] = "I2CLC",
>> +};
>> +
>> +#else
>> +#define DPRINT(fmt, args...)              do { } while (0)
>> +#endif
>> +
>> +typedef struct Exynos4210I2CSlave Exynos4210I2CSlave;
>> +
>> +typedef struct Exynos4210I2CState {
>> +    SysBusDevice busdev;
>> +    MemoryRegion iomem;
>> +    i2c_bus *bus;
>> +    Exynos4210I2CSlave *slave;
>> +    qemu_irq irq;
>> +    QEMUTimer *receive_timer;
>> +    QEMUTimer *send_timer;
>> +
>> +    uint8_t i2ccon;
>> +    uint8_t i2cstat;
>> +    uint8_t i2cadd;
>> +    uint8_t i2cds;
>> +    uint8_t i2clc;
>> +} Exynos4210I2CState;
>> +
>> +enum {
>> +    idle,
>> +    start_bit_received,
>> +    sending_data,
>> +    receiving_data
>> +};
>> +
>> +struct Exynos4210I2CSlave {
>> +    I2CSlave i2c;
>> +    Exynos4210I2CState *host;
>> +    uint32_t status;
>> +};
>> +
>> +
>> +static inline void exynos4210_i2c_set_irq(Exynos4210I2CState *s)
>> +{
>> +    if (s->i2ccon&  I2CCON_INTERRUPTS_EN) {
>> +        s->i2ccon |= I2CCON_INTERRUPT_PEND;
>> +        qemu_irq_raise(s->irq);
>> +    }
>> +}
>> +
>> +static void exynos4210_i2c_data_received(void *opaque)
>> +{
>> +    Exynos4210I2CState *s = (Exynos4210I2CState *)opaque;
>> +    int ret;
>> +
>> +    ret = i2c_recv(s->bus);
>> +    if (ret<  0) {
>> +        s->i2cstat |= I2CSTAT_LAST_BIT;  /* Data is not acknowledged */
>> +    } else {
>> +        s->i2cds = ret;
>> +        s->i2cstat&= ~I2CSTAT_LAST_BIT; /* Data is acknowledged */
>> +    }
>> +    exynos4210_i2c_set_irq(s);
>> +}
>> +
>> +static void exynos4210_i2c_data_sent(void *opaque)
>> +{
>> +    Exynos4210I2CState *s = (Exynos4210I2CState *)opaque;
>> +
>> +    if (i2c_send(s->bus, s->i2cds)<  0) {
>> +        s->i2cstat |= I2CSTAT_LAST_BIT;  /* Data is not acknowledged */
>> +    } else {
>> +        s->i2cstat&= ~I2CSTAT_LAST_BIT; /* Data is acknowledged */
>> +    }
>> +    exynos4210_i2c_set_irq(s);
>> +}
>> +
>> +static uint64_t exynos4210_i2c_read(void *opaque, target_phys_addr_t offset,
>> +                                 unsigned size)
>> +{
>> +    Exynos4210I2CState *s = (Exynos4210I2CState *)opaque;
>> +    uint8_t value;
>> +
>> +    switch (offset) {
>> +    case I2CCON_ADDR:
>> +        value = s->i2ccon;
>> +        break;
>> +    case I2CSTAT_ADDR:
>> +        value = s->i2cstat;
>> +        break;
>> +    case I2CADD_ADDR:
>> +        value = s->i2cadd;
>> +        break;
>> +    case I2CDS_ADDR:
>> +        value = s->i2cds;
>> +        break;
>> +    case I2CLC_ADDR:
>> +        value = s->i2clc;
>> +        break;
>> +    default:
>> +        value = 0;
>> +        DPRINT("ERROR: Bad read offset 0x%x\n", (unsigned int)offset);
>> +        break;
>> +    }
>> +
>> +    DPRINT("read %s [0x%02x] ->  0x%02x\n", exynos4_i2c_regs_names[offset>>  2],
>> +            (unsigned int)offset, value);
> Just for security it's better to check offset to not go out of the array
> borders.
Ok, good point.
>> +    return value;
>> +}
>> +
>> +static void exynos4210_i2c_write(void *opaque, target_phys_addr_t offset,
>> +                              uint64_t value, unsigned size)
>> +{
>> +    Exynos4210I2CState *s = (Exynos4210I2CState *)opaque;
>> +    uint8_t v = value&  0xff;
>> +
>> +    DPRINT("write %s [0x%02x]<- 0x%02x\n", exynos4_i2c_regs_names[offset>>  2],
>> +            (unsigned int)offset, v);
>> +
>> +    switch (offset) {
>> +    case I2CCON_ADDR:
>> +        if ((s->i2ccon&  I2CCON_INTERRUPT_PEND)&&
>> +                !(v&  I2CCON_INTERRUPT_PEND)) {
>> +            s->i2ccon&= ~I2CCON_INTERRUPT_PEND;
>> +            if (!(s->i2ccon&  I2CCON_INTERRUPTS_EN)) {
>> +                s->i2cstat&= ~I2CSTAT_START_BUSY;
>> +            }
>> +            qemu_irq_lower(s->irq);
>> +
>> +            if (!(s->i2cstat&  I2CSTAT_OUTPUT_EN) ||
>> +                    !(s->i2cstat&  I2CSTAT_START_BUSY)) {
>> +                s->i2cstat&= ~I2CSTAT_START_BUSY;   /* Line is not busy */
>> +                break;
>> +            }
>> +            /* Continue operation if transfer is ongoing */
>> +            if (EXYNOS4_I2C_MODE(s->i2cstat) == I2CMODE_MASTER_Rx) {
>> +                qemu_mod_timer(s->receive_timer, qemu_get_clock_ns(vm_clock)+1);
>> +            } else if (EXYNOS4_I2C_MODE(s->i2cstat) == I2CMODE_MASTER_Tx) {
>> +                qemu_mod_timer(s->send_timer, qemu_get_clock_ns(vm_clock) + 1);
> Why do you need to use timers here?  Why not just call the corresponding
> functions directly?  I can guess because when sending new message Linux
> driver first enables the interrupt and then writes the stat register.
> Am I right?
Yes, and  I wanted to emulate a small delay between qemu_irq_lower()
and qemu_raise_irq(), But I've just tried to replace timer usage with direct
functions calling and it still works stably, I think I wil remove timers 
in next version.
>> +            }
>> +        }
>> +
>> +        /* bit 4 can't be written */
>> +        s->i2ccon = (v&  ~I2CCON_INTERRUPT_PEND) |
>> +                (s->i2ccon&  I2CCON_INTERRUPT_PEND);
>> +        break;
>> +    case I2CSTAT_ADDR:
>> +        s->i2cstat =
>> +                (s->i2cstat&  I2CSTAT_START_BUSY) | (v&  ~I2CSTAT_START_BUSY);
>> +
>> +        if (!(s->i2cstat&  I2CSTAT_OUTPUT_EN)) {
>> +            s->i2cstat&= ~I2CSTAT_START_BUSY;
>> +            qemu_del_timer(s->receive_timer);
>> +            qemu_del_timer(s->send_timer);
>> +            break;
>> +        }
>> +
>> +        /* Nothing to do if in i2c slave mode */
>> +        if (!I2C_IN_MASTER_MODE(s->i2cstat)) {
>> +            break;
>> +        }
>> +
>> +        if (v&  I2CSTAT_START_BUSY) {
>> +            s->i2cstat |= I2CSTAT_START_BUSY;    /* Line is busy */
>> +            /* Generate start bit and send slave address */
>> +            i2c_start_transfer(s->bus, s->i2cds>>  1, s->i2cds&  0x1);
>> +            exynos4210_i2c_data_sent(s);
>> +        } else {
>> +            i2c_end_transfer(s->bus);
>> +            /* Linux driver may start next i2c transaction before it
>> +             * acknowledges an interrupt request from previous transaction.
>> +             * This may result in timeout waiting for I2C bus idle and
>> +             * corrupted transfer. Releasing I2C bus only after interrupt is
>> +             * accepted prevents this corruption. */
>> +            if (!(s->i2ccon&  I2CCON_INTERRUPT_PEND)) {
>> +                s->i2cstat&= ~I2CSTAT_START_BUSY;
>> +            }
>> +        }
>> +        break;
>> +    case I2CADD_ADDR:
>> +        if ((s->i2cstat&  I2CSTAT_OUTPUT_EN) == 0) {
>> +            s->i2cadd = v;
>> +            i2c_set_slave_address(&s->slave->i2c, v);
>> +        }
>> +        break;
>> +    case I2CDS_ADDR:
>> +        if (s->i2cstat&  I2CSTAT_OUTPUT_EN) {
>> +            s->i2cds = v;
>> +        }
>> +        break;
>> +    case I2CLC_ADDR:
>> +        s->i2clc = v;
>> +        break;
>> +    default:
>> +        DPRINT("ERROR: Bad write offset 0x%x\n", (unsigned int)offset);
>> +        break;
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps exynos4210_i2c_ops = {
>> +    .read = exynos4210_i2c_read,
>> +    .write = exynos4210_i2c_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +static const VMStateDescription exynos4210_i2c_slave_vmstate = {
>> +    .name = TYPE_EXYNOS4_I2C_SLAVE,
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_I2C_SLAVE(i2c, Exynos4210I2CSlave),
>> +        VMSTATE_UINT32(status, Exynos4210I2CSlave),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static const VMStateDescription exynos4210_i2c_vmstate = {
>> +    .name = TYPE_EXYNOS4_I2C,
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT8(i2ccon, Exynos4210I2CState),
>> +        VMSTATE_UINT8(i2cstat, Exynos4210I2CState),
>> +        VMSTATE_UINT8(i2cds, Exynos4210I2CState),
>> +        VMSTATE_UINT8(i2clc, Exynos4210I2CState),
> Should i2cadd also be saved here?
Yes
>> +        VMSTATE_TIMER(receive_timer, Exynos4210I2CState),
>> +        VMSTATE_TIMER(send_timer, Exynos4210I2CState),
>> +        VMSTATE_STRUCT_POINTER(slave, Exynos4210I2CState,
>> +                exynos4210_i2c_slave_vmstate, Exynos4210I2CSlave *),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static void exynos4210_i2c_reset(DeviceState *d)
>> +{
>> +    Exynos4210I2CState *s = EXYNOS4_I2C(d);
>> +
>> +    s->i2ccon  = 0x0f; /* 0x0X */
>> +    s->i2cstat = 0x00;
>> +    s->i2cds   = 0xff; /* 0xXX */
>> +    s->i2clc   = 0x00;
>> +    s->i2cadd  = 0xff; /* 0xXX */
>> +    s->slave->status = idle;
>> +    qemu_del_timer(s->receive_timer);
>> +    qemu_del_timer(s->send_timer);
>> +}
>> +
>> +static int exynos4210_i2c_init(SysBusDevice *dev)
>> +{
>> +    Exynos4210I2CState *s = EXYNOS4_I2C(dev);
>> +    DeviceState *slave;
>> +
>> +    memory_region_init_io(&s->iomem,&exynos4210_i2c_ops, s, TYPE_EXYNOS4_I2C,
>> +                          EXYNOS4_I2C_MEM_SIZE);
>> +    sysbus_init_mmio(dev,&s->iomem);
>> +    sysbus_init_irq(dev,&s->irq);
>> +    s->bus = i2c_init_bus(&dev->qdev, "i2c");
>> +    s->receive_timer =
>> +            qemu_new_timer_ns(vm_clock, exynos4210_i2c_data_received, s);
>> +    s->send_timer = qemu_new_timer_ns(vm_clock, exynos4210_i2c_data_sent, s);
>> +
>> +    /* i2c slave initialization */
>> +    slave = i2c_create_slave(s->bus, TYPE_EXYNOS4_I2C_SLAVE, 0xff);
>> +    s->slave = EXYNOS4_I2C_SLAVE(slave);
>> +    s->slave->host = s;
>> +
>> +    return 0;
>> +}
>> +
>> +static void exynos4210_i2c_class_init(ObjectClass *class, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(class);
>> +    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class);
>> +
>> +    dc->vmsd =&exynos4210_i2c_vmstate;
>> +    dc->reset = exynos4210_i2c_reset;
>> +    sbc->init = exynos4210_i2c_init;
>> +}
>> +
>> +static TypeInfo exynos4210_i2c_info = {
>> +    .name = TYPE_EXYNOS4_I2C,
>> +    .parent = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(Exynos4210I2CState),
>> +    .class_init = exynos4210_i2c_class_init,
>> +};
>> +
>> +static int exynos4210_i2c_slave_init(I2CSlave *i2c)
>> +{
>> +    Exynos4210I2CSlave *s = EXYNOS4_I2C_SLAVE(i2c);
>> +
>> +    s->status = idle;
>> +    return 0;
>> +}
>> +
>> +static void exynos4210_i2c_slave_event(I2CSlave *i2c, enum i2c_event event)
>> +{
>> +    Exynos4210I2CSlave *s = EXYNOS4_I2C_SLAVE(i2c);
>> +    Exynos4210I2CState *host = s->host;
>> +
>> +    if (!(host->i2cstat&  I2CSTAT_OUTPUT_EN) ||
>> +            I2C_IN_MASTER_MODE(host->i2cstat)) {
>> +        return;
>> +    }
>> +
>> +    switch (event) {
>> +    case I2C_START_RECV:
>> +    case I2C_START_SEND:
>> +        host->i2cstat&= ~(I2CSTAT_SLAVE_ADDR_ZERO | I2CSTAT_SLAVE_ADDR_MATCH);
>> +        s->status = start_bit_received;
>> +        host->i2cstat |= I2CSTAT_START_BUSY;    /* Line is busy */
>> +        break;
>> +    case I2C_FINISH:
>> +        s->status = idle;
>> +        host->i2cstat&= ~(I2CSTAT_SLAVE_ADDR_ZERO | I2CSTAT_SLAVE_ADDR_MATCH |
>> +                I2CSTAT_START_BUSY);
>> +        break;
>> +    default:
>> +        break;
>> +    }
>> +}
>> +
>> +static int exynos4210_i2c_slave_recv(I2CSlave *i2c)
>> +{
>> +    Exynos4210I2CSlave *s = EXYNOS4_I2C_SLAVE(i2c);
>> +    Exynos4210I2CState *host = s->host;
>> +
>> +    if (!(host->i2cstat&  I2CSTAT_OUTPUT_EN) ||
>> +            I2C_IN_MASTER_MODE(host->i2cstat)) {
>> +        return -1;
>> +    }
>> +
>> +    if ((s->status != sending_data) || (EXYNOS4_I2C_MODE(host->i2cstat) !=
>> +            I2CSTAT_MODE_SLAVE_Tx)) {
>> +        return -1;
>> +    }
>> +
>> +    exynos4210_i2c_set_irq(host);
>> +    return host->i2cds;
>> +}
>> +
>> +static int exynos4210_i2c_slave_send(I2CSlave *i2c, uint8_t data)
>> +{
>> +    Exynos4210I2CSlave *s = EXYNOS4_I2C_SLAVE(i2c);
>> +    Exynos4210I2CState *host = s->host;
>> +
>> +    if (!(host->i2cstat&  I2CSTAT_OUTPUT_EN) ||
>> +            I2C_IN_MASTER_MODE(host->i2cstat)) {
>> +        return -1;
>> +    }
>> +
>> +    if (s->status == start_bit_received) {
>> +        host->i2cds = data;
>> +        s->status = idle;
>> +        if ((data&  0xFE) == 0) {
>> +            host->i2cstat |= I2CSTAT_SLAVE_ADDR_ZERO;
>> +            return -1;
>> +        }
>> +        if ((host->i2cadd&  0xFE) != (data&  0xFE)) {
>> +            return -1;
>> +        }
>> +        host->i2cstat |= I2CSTAT_SLAVE_ADDR_MATCH;
>> +        (data&  0x1) ? (s->status = receiving_data) :
>> +                (s->status = sending_data);
>> +        exynos4210_i2c_set_irq(host);
>> +        return 0;
>> +    }
>> +
>> +    if ((s->status != receiving_data) || (EXYNOS4_I2C_MODE(host->i2cstat) !=
>> +            I2CSTAT_MODE_SLAVE_Rx)) {
>> +        return -1;
>> +    }
>> +
>> +    host->i2cds = data;
>> +    exynos4210_i2c_set_irq(host);
>> +    return 0;
>> +}
>> +
>> +static void exynos4210_i2cslave_class_init(ObjectClass *klass, void *data)
>> +{
>> +    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
>> +
>> +    k->init = exynos4210_i2c_slave_init;
>> +    k->event = exynos4210_i2c_slave_event;
>> +    k->recv = exynos4210_i2c_slave_recv;
>> +    k->send = exynos4210_i2c_slave_send;
>> +}
>> +
>> +static TypeInfo exynos4210_i2c_slave = {
>> +    .name = TYPE_EXYNOS4_I2C_SLAVE,
>> +    .parent = TYPE_I2C_SLAVE,
>> +    .instance_size = sizeof(Exynos4210I2CSlave),
>> +    .class_init = exynos4210_i2cslave_class_init,
>> +};
>> +
>> +static void exynos4210_i2c_register_types(void)
>> +{
>> +    type_register_static(&exynos4210_i2c_slave);
>> +    type_register_static(&exynos4210_i2c_info);
>> +}
>> +
>> +type_init(exynos4210_i2c_register_types)
> PS: This I2C implementation seems to be rather complex.  Did you check
> what we wrote for S5PC110 (ah, I also learnt that Samsung renamed it to
> Exynos3210) almost three years ago
> (https://gitorious.org/samsung-linux-platform/qemu/blobs/s5pc110-stable/hw/s5pc1xx_i2c.c)?
> When I compared documentation and kernel driver sources for both devices
> I couldn't find any significant difference.  Generally speaking, is it
> worth redoing everything from scratch instead of updating?
mm, it doesn't worth it, something went wrong here I think ...